Adapted for latest rand APIs
#474
Conversation
| bt-hci = { version = "0.6" } | ||
| embassy-futures = "0.1.1" | ||
| embassy-sync = { version = "0.7" } | ||
| embassy-sync = { version = "0.7" } # host-macros |
There was a problem hiding this comment.
marked the reason of existence for certain dependencies (here and static_cell, heapless, below).
| pub mod ble_advertise; | ||
| pub mod ble_advertise_multiple; | ||
| pub mod ble_bas_central; | ||
| #[cfg(feature = "security")] |
There was a problem hiding this comment.
*_sec were treated as feature-flagged, except here.
| trouble-host = { path = "../../host", features = ["derive", "scan"] } | ||
| trouble-example-apps = { version = "0.1.0", path = "../apps", features = ["defmt"] } | ||
|
|
||
| defmt = "1.0" | ||
| defmt-rtt = "1.0" | ||
| panic-probe = { version = "1.0", features = ["print-defmt"] } | ||
|
|
||
| cortex-m = { version = "0.7.6" } | ||
| chacha20 = { version = "0.10.0-rc.2", default-features = false, features = ["rng"], optional = true } |
There was a problem hiding this comment.
rand 0.10 changelog:
The dependency on
rand_chachahas been replaced with a dependency onchacha20.
Moved it upwards, to have the list in roughly alphabetical order (imho, a good idea). Do you agree with this, or should it be in-place of the earlier rand_core?
| cortex-m-rt = "0.7.0" | ||
| rand = { version = "0.8.5", default-features = false } | ||
| rand_core = { version = "0.9.3", default-features = false, optional = true } |
There was a problem hiding this comment.
rand_core is interesting. crates.io for 0.9.3 states:
This crate is mainly of interest to crates publishing implementations of RngCore. Other users are encouraged to use the rand crate instead which re-exports the main traits and error types.
But the current README says it differently:
The main rand crate re-exports most items defined in this crate, along with tools [...] Most users should prefer to use the main rand crate.
I feel there's a justification for keeping rand_core, if the code only needs the interface of the RNG's, and doesn't actually create random numbers. What do you think?
That's the approach I took here - nrf-sdc - but in host I've opted for rand and use the interfaces as rand::rand_core::....
host/Cargo.toml
Outdated
| rand_core = "0.6" | ||
| rand_chacha = { version = "0.3", default-features = false, optional = true } | ||
| rand = { version="0.8", default-features = false, optional = true} | ||
| p256 = { version = "0.14.0-pre.11", default-features = false, features = ["ecdh","arithmetic"], optional = true } |
There was a problem hiding this comment.
The 0.14.0-pre.3 seems to be the release that has the rand 0.6 -> 0.9 upgrade.
This actually is the BIG THING about the PR. Whether we want to jump onto -pre or wait until p256 0.14.0 proper has been released.
| use heapless::Vec; | ||
| use rand_core::{CryptoRng, RngCore}; | ||
| #[cfg(feature = "security")] | ||
| use rand::rand_core::{CryptoRng, RngCore}; |
There was a problem hiding this comment.
Note: here, we could go on with the earlier lines, if rand_core is added (alongside rand), as a dependency to trouble-host. It's a compromise between either:
- shorter code; two dependencies
- slightly longer code; just
randdep
The upstream seems to recomment going rand only, but it's really our choice. Which way is better?
- please inform of the choice.
| /// | ||
| /// Only relevant if the feature `security` is enabled. | ||
| pub fn set_io_capabilities(self, io_capabilities: IoCapabilities) -> Self { | ||
| #[cfg(feature = "security")] |
There was a problem hiding this comment.
Moved the conditional above the method name. This was made possible by making the security feature handling more consistent, elsewhere.
Should be beneficial: one only needs to see such method signatures in IDE if the security feature is enabled.
| pairing_data.local_secret_ra = | ||
| rng.sample(rand::distributions::Uniform::new_inclusive(0, 999999)); | ||
| rng.sample(rand::distr::Uniform::new_inclusive(0, 999999).unwrap()); | ||
|
|
There was a problem hiding this comment.
Here, I chose to leave the original code (after API catch-up), but the rand authors recommend:
Constructors may do extra work up front to allow faster sampling of multiple values. Where only a single sample is required it is suggested to use
Rng::random_range [...]".
Should we?
This comment was marked as outdated.
This comment was marked as outdated.
|
A bit about the status:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
/test |
finalyards
force-pushed
the
pr-rand-catchup
branch
2 times, most recently
from
8d76b38 to
9655720
Compare
|
/test |
finalyards
force-pushed
the
pr-rand-catchup
branch
from
9655720 to
86e2aae
Compare
|
The CI does not really fail - it says "No space left of device". The PR now only includes randomness, and security-related changes (+ ~4 unrelated comments). While it introduces a dependency on Could this be considered for a merge, or do we need to wait (in which case I'll mark this back as a "draft" and bring it back when the dependencies have released solid releases). |
finalyards
force-pushed
the
pr-rand-catchup
branch
from
86e2aae to
221a952
Compare
finalyards
force-pushed
the
pr-rand-catchup
branch
2 times, most recently
from
74c3a8d to
63e36fe
Compare
…es to 'security' feature flag use.
finalyards
force-pushed
the
pr-rand-catchup
branch
from
63e36fe to
e911506
Compare
3 participants
The version of
randused in the project has fallen behind, mainly due top256still relying on the earlier version.I wanted to give it a try, and attached is a PR that:
randto0.10.0-rc.0(currently latest)rand(securityfeature)The changes to 0.9.x were extensive, covering not only the API but the whole
randecosystem, as well.I chose to push past the 0.9, since we already can see what the 0.10 API would look like. Happy to keep maintaining this, as a PR, until the 0.10 proper is released.
Some questions to authors in the comments below. I need guidance.
Discussed previously: #423