Rng: Add prng, hwrng support #54
Conversation
There was a problem hiding this comment.
I haven't plowed through the random.rs yet, but do I get this right that while hwrng.rs and prng.rs's rand_prng already give the user all they need, the riot_prng (backed by random.rs) is "only" there to avoid pulling in code for StdRng in situations when RIOT's rng is either already there or for other reasons works better than the pure Rust version?
(The "only" is in quotes because these are legitimate reasons; I wonder though whether we should have unified API, and what can be done to make this more visible.)
src/prng.rs
Outdated
| /// See this modules description regarding quality of the used seeds. | ||
| pub fn rand_prng() -> StdRng { | ||
| let mut buffer = [0u8; 32]; | ||
| unsafe { |
There was a problem hiding this comment.
No need for unsafe here; as per above the error type is uninhabited, the error handling will just collapse.
(And if the RIOT HWRNG ever does become fallible, we'll get a clean crash rather than UB).
* remove unsafe blocks * make error type empty * update comments
src/lib.rs
Outdated
| @@ -146,6 +146,15 @@ pub mod socket_embedded_nal_tcp; | |||
| #[cfg(riot_module_periph_gpio)] | |||
| pub mod gpio; | |||
|
|
|||
| #[cfg(riot_module_periph_hwrng)] | |||
| pub mod hwrng; | |||
There was a problem hiding this comment.
As I understand, the sensible way to use all of this is through the prng module.
Is there any good reason that the hwrng module and the random module are pub in the first place, or that RandomSeed::new_from_hwrng would be public?
There was a problem hiding this comment.
Yes it is intended to be used through prng. I just left the rest public in case someone might need it e.g to seed his own prng but we could make it private to make people use the "intended" way. I do not have a strong opinion on that matter.
There was a problem hiding this comment.
Then please limit the public parts. Public APIs that are not used are a needless compatibility liability -- whereas the two functions that will stay pub can easily be maintained. If it turns out that any part of this is needed, it's still easy to make a few more parts pub, whereas going back on that is a breaking change.
There was a problem hiding this comment.
I made the two modules random and hwrng private
|
Is this even still relevant with the other random module now present in the wrappers? As far as I can see they both do roughly the same with the main differences being:
Depending on any of the above being desirable this PR could just be closed or some of the changes could be merged into the current solution. |
This is an idea to provide prngs seeded by RIOTs
hwrnginterface. This usesrandomas one prng.In addition I implemented
rand:RngCoreandrand::SeedableRngwithrandomto provide therand::Rnginterface to be usable here. Also a function is provided to seed anrand::StdRng(Seems not need std despite its name) withhwrngto have two different prngs at hand.The
randpart was mostly added because I like the interface ofrand::Rngbut it's just an idea and it definitely needs to be discussed if it is worth the extra dependencies. Or maybe should be behind a feature.Structure looks like this:
hwrng.rs: Wrapper aroundhwrnginterfacerandom.rs: Wrapper aroundrandommodule + implementation ofrandinterfacesprng.rs: User faced helper methods to create a prng viahwrngseedingEdit: This needs a change in
rust-riot-systo create bindings torandom(See RIOT-OS/rust-riot-sys#26)