Implement the embedded-hal Read trait for the RNG peripheral

[jessebraham]

Simple peripheral, equally simple implementation. Did not include an example but did verify that it works as expected locally.

[MabezDev]

Implementation looks fine, but can we figure out if the RNG has actually been enabled? This document talks about what's required for the RNG to work, and given that we are using the second stage boot loader it will work but in the future that might change. We don't have to implement the clock enable in this PR, but maybe as a bare minimum we can add check to make sure it has?

[bjoernQ]

Code looks good to me. But Scott's point about the pre-conditions is very good. I'd probably have totally missed that.

The scary thing is that apparently when the pre-conditions are not met it will produce pseudo-random numbers so it is not obvious to the user. A PRNG might probably be good enough for many things but for some things it's not

Not sure if we should add a runtime-check and panic - as said maybe some users might be totally fine with a PRNG.
So, the very least would be to document this behavior (i.e. TRNG vs PRNG). And/or maybe have an optional constructor that checks it and panics?

Also, is this worth to have an example for? The API is simple enough so most people will figure out how to use the RNG

[jessebraham]

I'm sort of on the fence with regards to how we should handle this. My gut says that we should avoid runtime checks whenever possible, but I'm happy to hear any refutations to this.

Generally speaking, I'm against trying to hold the user's hand too much. If somebody is not using the bootloader from ESP-IDF I assume that they know well enough to properly initialize the device. I'm not sure if this is a reasonable assumption or not, though.

Given that we're still in the early stages of the HAL, and that there are already technically incorrect implementations present in the repo (eg. the Xtensa delays) I'm not sure how big of a deal we need to make of this. I think adding a comment stating the pre-conditions is probably adequate for the time being, but please let me know how you both feel about this.

[bjoernQ]

I personally would be totally fine with a reasonable warning (with a reference to the TRM) in the comments and I agree that a bare-metal HAL is usually not a "batteries-included" thing. Also, since this is not named TRng hopefully people will read the docs to find out if it's TRng or PRng.

Maybe it's also a good occasion to think about the general philosophy of the HAL (i.e. should it be as much "batteries-included" as possible or not). While there are a lot of wild assumptions currently (e.g. regarding the various clock speeds) which we need to address there are also other things more similar to this (e.g. is synchronization something that should be taken care of at the call-site (with some hints in the docs where needed) or not)

As said, I personally think a bare-metal HAL could assume more care taken by the user than e.g. the ESP-IDF-HAL. Which makes we wonder if the wording no-std and std isn't a bit confusing since people could do no-std on top of ESP_IDF ... but that's a different story certainly

[jessebraham]

I've added documentation for this driver, should be adequate I believe.

@bjoernQ I think you're right and those discussions need to be had. I've created a discussion where we can talk about such things.

[bjoernQ]

Looks good to me! Nice we have that peripheral supported now