Added new (yet unproven) blocking Read trait for a rngs

[therealprof]

Implements #38 . Identical custom trait already implemented in nrf51-hal crate and used in
examples (directly and to seed a PRNG) in the microbit crate.

Signed-off-by: Daniel Egger [email protected]

[ryankurte]

I'm not immediately clear on how this is to be used, is the intention with this to block until entropy is available or return an error in the case it's not?
And given entropy is often limited on the platforms we use, is there a reason this is better served by a blocking call than by polling on a non-blocking one (which is then easily wrapped to block)?

[therealprof]

I'm not immediately clear on how this is to be used, is the intention with this to block until entropy is available or return an error in the case it's not?

Almost. The Error is meant for situations where no entropy can be generated at all, e.g. if there's not random generator or it is not available for some reason (like being used by another hardware block).

And given entropy is often limited on the platforms we use, is there a reason this is better served by a blocking call than by polling on a non-blocking one (which is then easily wrapped to block)?

Well, if you need entropy then you often need a specific amount of it and I find it a bit inconvenient to handle such a situation with async calls other than blocking on the spot. Different platform will also yield a different numbers of bytes per round, e.g. with NRF51 you'll get 1 byte per round while the STM32s will yield 4 byte per round.

If you need a larger amount of data I'd recommend setting up an interrupt handler and simply using the blocking call with the amount of data a platform will yield. Of course we still don't have any support whatsoever to properly include interrupt setup and handling in the HAL drivers so that requires some dancing around...

One could certainly craft an async interface, too, but at the moment I don't see too much benefit in that.

[austinglaser]

This trait should be gated behind #[cfg(feature = "unproven")], in addition to the gate in prelude.

At least, that's what I gather from the crate docs and the other unproven traits (like digital::InputPin)

[therealprof]

That can certainly be done. However the rules say that a trait is proven if at least two implementations and a driver or demo application exists and that is actually already the case. 😉

[austinglaser]

The benefit I can see to an async interface is interaction with nonblocking application models, such as those based on await! or futures.

The nb crate is designed for exactly this kind of interoperability, and making the trait itself compatible should be as simple as replacing the Result with an nb::Result

[therealprof]

@austinglaser And where would you store the data in the meantime? The API is modelled like the other blocking APIs in that you pass in a buffer to be filled. Otherwise you can only return a single value in the result.

[austinglaser]

@therealprof That's a good point -- I hadn't thought through that aspect enough.

What about a trait (eventually, and in addition to this one) which returns a fixed-size random value?

That fixed-size value could be (taking a page out of the spi traits' book) determined on a per-implementation basis, or it could simply be a u8 -- meaning that on hardware like the STM32, you might just hold on to three bytes of data to give out on later calls.

I think I favor the simple u8 approach, because it makes filling a buffer with values simple at the cost of a little more complexity for the implementer.

Something like this?

// rng.rs
pub trait Read {
    type Error;

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

But this is getting beyond the scope of discussion for this particular pull request, I think.

[therealprof]

@austinglaser A single u8 might be somewhat limiting if that's not what you need in a go. I'm not sure how you would store the data internally in an implementation; once you leave the function any previous values stored on the stack would be gone, unless you have a (yuck!) static buffer.

[austinglaser]

@therealprof I'd say that forcing a user to block is limiting in a different way -- at the trait level, at least, it makes sense to allow both usage patterns.

I'm not sure how you would store the data internally in an implementation

You'd store the data in a context struct you pass back into the function each time. If that doesn't sound terribly ergonomic, you could use futures or generators, which do much the same thing behind the scenes but (especially in the generator case) are a little cleaner to use.

[austinglaser]

For a semi real-world example, take a look at the docs for the nb crate. Note that the LED "thread" in each case has some mutable data (state) which is maintained across resumptions/polls of the asynchronous function.

[therealprof]

I'd say that forcing a user to block is limiting in a different way -- at the trait level, at least, it makes sense to allow both usage patterns.

Blocking on a MCU is relative anyway since you can always interrupt the wait. But sure, if you have a use case for partial random data being available whenever it shows up that an async interface can easily be added.

You'd store the data in a context struct you pass back into the function each time. If that doesn't sound terribly ergonomic, you could use futures or generators, which do much the same thing behind the scenes but (especially in the generator case) are a little cleaner to use.

You're absolutely right that this is not ergonomic. ;) I'm familiar with the concepts of futures and generators in general. I'm still not sure about the use case in with a TRNG; maybe if you throw in DMA to automatically build up a pool from which you can deplete. But with a register based implementation there's very little point in having this; instead if you need large amount of data, use the TRNG to seed a PRNG and use that to generate random numbers whenever you need them.

But hey, feel free to propose the non-blocking version of this trait. I'd even provide a non-blocking implementation, just for the fun of it. 😉

[austinglaser]

But hey, feel free to propose the non-blocking version of this trait. I'd even provide a non-blocking implementation, just for the fun of it.

Fair enough!

[japaric]

LGTM but I'm wondering if we should add a clause to read about what happens when an error in encountered. For example std::io::Read::read_exact has this in its documentation:

If this function encounters an "end of file" before completely filling the buffer, it returns an error of the kind ErrorKind::UnexpectedEof. The contents of buf are unspecified in this case.

If any other read error is encountered then this function immediately returns. The contents of buf are unspecified in this case.

We could say something similar here: if an error is encountered the contents of buf are unspecified.

[japaric]

Oh and kind of off-topic but there are a few traits that look very similar to each other and that look similar to some of the API in std::io::{Read,Write} so I've been wondering if it would make sense to have some core blocking::ReadExact trait with method fn read_exact(&mut self, buf: &mut [u8]) -> Result<(), Self::Error> and then either (a) have traits like blocking::Rng be a super trait over that or (b) have blocking::Rng become a marker trait then the user would have to use where RNG: blocking::ReadExact + Rng.

[therealprof]

@japaric Would you like me to change the name to read_exact() to be in line with the std::io::Read implementation?