Add nonblocking RNG trait

[austinglaser]

As discussed in #45, this is a nonblocking variant of an RNG interface.

[Nemo157]

You could take a hint from std::io::Read and have the function return the number of bytes written to the buffer to support RNG devices that can generate >1 byte at a time.

/// Pull some bytes from this source into the specified buffer,
/// returning how many bytes were read.
///
/// If an object needs to block for a read it will return
/// an `Err(nb::Error::WouldBlock)` return value.
///
/// If the return value of this method is `Ok(n)`, then it must be
/// guaranteed that `0 <= n <= buf.len()`. A nonzero `n` value
/// indicates that the buffer buf has been filled in with `n` bytes
/// of data from this source.
fn read(&mut self, buf: &mut [u8]) -> nb::Result<usize, Self::Error>

In fact, it seems like embedded-hal should include some sort of core Read and Write traits similar to the std::io ones, then RNG devices could just implement those instead of having to have their own dedicated traits.

---

Looking a bit wider, it seems https://github.com/rust-lang-nursery/rand is no_std compatible. I haven't looked into whether it is actually suitable for implementing on microcontrollers, but it seems like integrating that might be a good way to have embedded RNG's work better with the wider ecosystem.

[therealprof]

fn read(&mut self, buf: &mut [u8]) -> nb::Result<usize, Self::Error>

Well, one could do this but would it be very user friendly? I have some doubts about that. If you need 3 bytes and only get 1 per call, what would you do? Slice'n'dice the buffer and call again until the 3 bytes are complete?

Looking a bit wider, it seems https://github.com/rust-lang-nursery/rand is no_std compatible. I haven't looked into whether it is actually suitable for implementing on microcontrollers, but it seems like integrating that might be a good way to have embedded RNG's work better with the wider ecosystem.

I'm using that and my proposed blocking version to seed it in an microbit crate example: https://github.com/therealprof/microbit/blob/master/examples/rng_hal_printrandserial.rs

And I fully agree that Read/Write implementations would be nice for some things, like Serial (as you can also see from the mentioned example 😉).

[Nemo157]

Well, one could do this but would it be very user friendly? I have some doubts about that. If you need 3 bytes and only get 1 per call, what would you do? Slice'n'dice the buffer and call again until the 3 bytes are complete?

Yep, but there could be more ergonomic APIs provided built on top of this low-level API. For example in the tokio ecosystem there is tokio_io::read_exact that will handle the slicing and re-invoking until you finish reading the buffer from a non-blocking std::io::Read and represents that as a single future.

[Nemo157]

Taking a look at the implementation of rand 0.5 (as of yet unreleased, but nearly release candidate) it definitely cannot support non-blocking RNG sources. It allows sources to return NotReady while they prepare, but isn't designed so that sources can return something like WouldBlock in between generating bytes. Once they're ready they need to be able to return entire values at once a continuous stream of data.

[therealprof]

@Nemo157 Not sure I follow. I definitely have no interest using rand backed directly from a TRNG on an MCU. The application would do little more than wait for more random bits to be spit out all the time. Those TRNGs are really more meant to seed a PRNG, maybe even continuously...

[Nemo157]

I was hoping it would be possible to use something like the ReseedingRng trait by providing a non-blocking RngCore implementation using hardware RNG as the reseeder. As I said above though, the rand crate just doesn't really support having a non-blocking RngCore implementation.

I guess what it could do is have an internal buffer that is filled via interrupts, then when a new seed is requested it's only returned if the buffer has been refilled enough. That seems a bit annoying to do though.

[therealprof]

Yeah, but then again most of the non-blocking stuff is rather wasteful if you don't have a trigger.
Re reseeding the documentation even says:

Reseeding is never strictly necessary. Cryptographic PRNGs don't have a limited number of bytes they can output, or at least not a limit reachable in any practical way. There is no such thing as 'running out of entropy'.

So why not do it as I do and just seed the RNG once in a blocking fashion during initialisation with TRNG data and let that be?

[Nemo157]

So why not do it as I do and just seed the RNG once in a blocking fashion during initialisation with TRNG data and let that be?

For my usecase I want to be generating the seed in parallel to other setup tasks and some radio communication to minimize wake time. I think avoiding RngCore for the TRNG and having some non-blocking API like this is fine for that, then once the seed is ready I can just create a PRNG from that. So basically like you're doing, but with a non-blocking TRNG.

[Nemo157]

For comparison I've thrown up a quick implementation of an nb compatible Read trait and read_exact method: Nemo157/embedded-hal@master...read-trait

[austinglaser]

@Nemo157 I'm happy to modify the trait interface if you think there'd be a cleaner way to go about it. I think I'm with @therealprof in that an unknown-till-return data size won't work very well. But what about taking a page out of the spi traits, and having a variable word-size, that matches the underlying RNG?

pub trait Read<Word> {
    type Error;

    fn read(&mut self) -> nb::Result<Word, Self::Error>;
}

[austinglaser]

@Nemo157 Regardless of what we discuss here, those seem like hugely valuable traits to have available -- especially if they're implemented for serial interfaces and the like

[Nemo157]

But what about taking a page out of the spi traits, and having a variable word-size, that matches the underlying RNG?

pub trait Read<Word> {
    type Error;

    fn read(&mut self) -> nb::Result<Word, Self::Error>;
}

First reaction, that could be really annoying if you use a library that requires an rng::Read<u8> but your system's HAL only provides rng::Read<u16>. There could be adaptor structs for transforming them, but that's a lot of boilerplate code.

---

Thinking about it some more, having the implementation on a non-u8 providing device simply store some state and shift out a byte at a time wouldn't be too bad (which, re-reading the blocking RNG PR I see you actually mentioned, #45 (comment)).

So, afterall I'm pretty happy with this implementation, and it should be pretty trivial to write an adaptor from this trait to a generic io::Read trait like I'm proposing (EDIT: did it, super trivial).

[austinglaser]

Cool! I love the idea of adapters to io-esque Read/Write

[japaric]

pub trait Read<Word> {

You could have an Unsize<[u8]> bound on Word which basically means that Word is some array [u8; N]. Unsize is kind of a hack until we get proper const generics but with that in place it would be possible to provide a default implemantion of the blocking::Rng trait proposed in #45 on top of this trait:

warning untested; may contain errors

#![feature(unsize)]
#![no_std]

#[macro_use(block)]
extern crate nb;

use core::marker::Unsize;

pub trait Read {
    type Bytes: Unsize<[u8]>; // poor man's [u8; N]
    type Error;

    fn read(&mut self) -> nb::Result<Self::Bytes, Self::Error>;
}

mod blocking {
    pub trait ReadExact {
        type Error;

        fn read_exact(&mut self, buf: &mut [u8]) -> Result<(), Self::Error>;
    }

    mod read_exact {
        use core::cmp;

        pub trait Default {}

        impl<RNG, E> super::ReadExact for RNG
        where
            RNG: ::Read<Error = E> + Default,
        {
            type Error = E;

            fn read_exact(&mut self, buf: &mut [u8]) -> Result<(), E> {
                let mut i = 0;
                while i < buf.len() {
                    let bytes = block!(self.read())?;
                    let slice: &[u8] = &bytes;

                    let n = cmp::min(slice.len(), buf.len() - i);
                    buf[i..i + n].copy_from_slice(&slice[..n]);
                    i += n;
                }

                Ok(())
            }
        }
    }
}

[austinglaser]

So an impl of this trait might look like the following?

impl rng::Read for SomeHardwareRng {
    type Bytes = [u8; 4];
    type Error = SomeError;

    // ...
}

It does seem like doing this otherwise would violate the "no stored state" aspect of nb::Result

[japaric]

So an impl of this trait might look like the following?

Yes

It does seem like doing this otherwise would violate the "no stored state" aspect of nb::Result

How so? Bytes is to be picked to match the number of random bytes the interface can generate "in one go", or rather it matches the internal buffer of the hardware interface. So the implementer doesn't have to store anything in the program state; it will always read out the whole RNG buffer.

This is no different than the Rng<Word> where Word = {u8,u16,u32} that was mentioned before; the difference is that [u8; N] is easier to copy into a bigger buffer (pool) in a generic fashion.

[austinglaser]

@japaric I think I was unclear in my comment before -- I was in fact saying that some of the previous suggestions (giving out a byte at a time, and holding onto the rest of the data) would violate the "no stored state" aspect, while your suggestion looks ergonomic while not doing so.

I like it; I'll update the pull request. Any objection to me amending/force pushing to this branch vs. adding another commit?

[japaric]

@austinglaser Thanks for updating the PR. As of the upcoming v0.2.0 release we are not going to add dependencies to unstable features because we want this crate to compile on the beta and stable channels.

Could you try replacing Unsized with GenericArray. tstellanova/l3gd20#4 should give you an idea of the required changes.

[austinbes]

Totally! I'll try to get to this over the weekend.

[hannobraun]

@austinglaser Are you still interested in updating this PR?

[austinglaser]

Yes, though I'm not sure when I'll be able to get to it. My available free time has been quickly swallowed by other things.

With that said, the changes should be fairly minimal. I'll try to get it done within two weeks -- does that sound reasonable?

[hannobraun]

@austinglaser That sounds great, thank you. Take as much time as you need!

[therealprof]

@austinglaser Ping. Any updates?

[austinglaser]

@therealprof Still around, still busy. I do still want to update this -- I'll try to make time soon™. Thanks for the ping to get this back on my radar

[hannobraun]

Why this version? The newest version is 0.12.0.

[austinglaser]

Good call -- must have been moving too quickly when I added it. I could have sworn that the build badge said 0.11

[hannobraun]

Okay, so we need to decide how to go forward here. To recap: The current approach (based on feedback previously provided on this pull request) doesn't seem viable (see my comment). Now we need to decide which interface we want to go forward with.

The options:

1. Single-byte:

fn read(&mut self) -> nb::Result<u8, Self::Error>

2. Slice-based:

fn read(&mut self, buf: &mut [u8]) -> nb::Result<usize, Self::Error>

I prefer option 2, but I'm fine with option 1.

My reasons for preferring option 2:

It's simple, takes full advantage of hardware capabilities, and it's straight-forward to write a (blocking or non-blocking) read_all wrapper, that provides as many bytes as the user wants.

Any other views?

[jamwaffles]

Any other views?

Slices seem to be the most versatile to me, although I'm not sure how an implementation would signal to the user how many bytes will be put into the buffer. Is that even necessary? Should the user have to check the length of the buffer after it's written to? I think I've seen some other Rust embedded APIs that return the number of bytes read/received/whatever like this:

let num_recvd = TheRandomThing::read(&mut buf);

which would make sense.

[hannobraun]

Thanks for the comment, @jamwaffles.

Slices seem to be the most versatile to me, although I'm not sure how an implementation would signal to the user how many bytes will be put into the buffer.

The method signature from my comment includes a usize return value, so it would work exactly as you proposed.

Is that even necessary? Should the user have to check the length of the buffer after it's written to?

It is necessary, as this is a non-blocking API which might not be able to fill the buffer in one call.

Aside: It might be nicer, if the method returned a slice with all the bytes written instead of a number. I'm not sure if that plays well with all use cases, however, and right now it would lead straight into lifetime hell anyway, I think (NLL should help with this though, once it arrives on stable). In any case, it's a topic for later.

[jamwaffles]

The method signature from my comment includes a usize return value, so it would work exactly as you proposed.

Ah, I should've skimmed the PR more slowly. In that case, this looks fine to me :)

[austinglaser]

OK @hannobraun, I'm sold. The clincher is that a simple implementation of option 1 is easy to make in terms of option 2, and that option 2 makes it easier to take full advantage of the hardware.

Will try to update this PR again (and drop in the default blocking implementation while I'm at it) over the weekend.

[Disasm]

@austinglaser Any progress on this?

[austinglaser]

@hannobraun @Disasm Apologies for the year-late update, but this should be up-to-date based on all the discussions.

I removed the unproven gate on the rng mod, since that appears to be the pattern elsewhere. Please let me know if that was the wrong way to do it.

[hannobraun]

@austinglaser Thanks, this is great! Unfortunately, I no longer have the power to merge anything, so let's hope someone else will take a look soon :-)

[no111u3]

It's lgtm.

[Disasm]

Suggested change

	/// For infallible implementations, will be `Void`
	/// For infallible implementations, will be `Infallible`

[austinglaser]

Done

[Disasm]

@austinglaser sorry for the late response and thanks for the contribution! Could you apply the change above and add a CHANGELOG entry?

[austinglaser]

Added the changelog entry, and rebased on master

[Disasm]

Thanks!

@therealprof could you review this?

[therealprof]

LGTM

[therealprof]

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push