-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Device IDs #444
base: main
Are you sure you want to change the base?
Device IDs #444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to introduce a trait, defined in riot-rs-embassy-common
, with an associated type for the error type, which each manufacturer-specific crate (riot-rs-nrf
) could then implement. The identity
module could then provided at riot_rs
root, instead of in the arch
module.
examples/hello-world/src/main.rs
Outdated
@@ -7,8 +7,9 @@ use riot_rs::debug::{exit, log::*}; | |||
#[riot_rs::thread(autostart)] | |||
fn main() { | |||
info!( | |||
"Hello from main()! Running on a {} board.", | |||
"Hello from main()! Running on {} board identified as {:x}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a new example for this. We might not actually support device_id()
on every board, limiting where hello-world
can be built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, do we expect this to be available everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do expect these to be available everywhere. As we chatted before, most devices will have something like that, even if it's "just" a MAC address. The API is fallible to also cater for devices that can't do it, but I think the API should be available anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prints the wrapping Result
of device_identity()
; this could be fine on a DeviceId
-specific test/example but I don't think it looks too good on a hello world-example. I would agree with @kaspar030 here regarding having a separate test/example (or format it differently, but that seems out-of-scope of a hello world example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it can be as simple as .unwrap_or("(a device has no name)")
and then be pretty, can it stay? (Not sure that's realistic, but we'll know that once the types are final).
This is now reworked into a trait, has all comments so far addressed (mostly in code, some in replies). It also provides an erring implementation for platforms where it is not implemented (including a provided error type for when it turns out a platform really can't do it), and uses what is already in embassy to provide an identifier for STM32 devices – but I haven't tested it for lack of compatible devices. ESP and RPi are left unimplemented because it's more complicated there and I have zero experience with either so far. |
This now checks all the boxes to the best of my understanding. Note that for ticking the checkbox on code style I added a remark there (quoted here for easy access):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for introducing this trait, I think it looks good!
Two general remarks:
-
I think the terminology should be harmonized in this PR; sometimes the words "board", "platform", and "device" are used to refer to what's seems to me to be the same thing. I'd rather we stick with "device" everywhere that makes sense in the context of this device id (as it may also be based on ids provided by chips other than the main MCU, such as the MAC address of another chip).
-
@kaspar030 what do we think of "Open questions" sections in rustdoc docs? To me these docs are mostly intended for users and I'd rather we keep these questions as comments intended for developers only. wdyt?
examples/hello-world/src/main.rs
Outdated
@@ -7,8 +7,9 @@ use riot_rs::debug::{exit, log::*}; | |||
#[riot_rs::thread(autostart)] | |||
fn main() { | |||
info!( | |||
"Hello from main()! Running on a {} board.", | |||
"Hello from main()! Running on {} board identified as {:x}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prints the wrapping Result
of device_identity()
; this could be fine on a DeviceId
-specific test/example but I don't think it looks too good on a hello world-example. I would agree with @kaspar030 here regarding having a separate test/example (or format it differently, but that seems out-of-scope of a hello world example).
Before I start harmonizing this from board/chip/device/arch, it'd be helpful to build a shared understanding of which fallbacks we're ready to take. If an MCU itself has no unique properties, how far do we go? We probably do peek into the on-SoC radio module's MAC address. It seems from embedded-rust discussions that for RPi and ESP there is a conventional place on the attached flash to place some cross-flash stable number, but that's going in the direction of conventionality already. If a board has an explict MAC ROM/OTP, does that count? |
…general accessor See-Also: future-proof-iot#444 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have a few concerns regarding this API:
- I think the biggest one is whether
riot_rs::identity::device_identity()
should return animpl DeviceId
or an arch-dependent concrete type. I personally thought it was going to be the first one, which seemed consistent with the id being opaque. Now that the id is not documented as such, I don't know anymore. TheDeviceId
trait documentation also mentions possibly providing additional methods in the future (I read this as the methods would be provided on the concrete types), which would depend on the nature of the device id (e.g., serial number, MAC address). To me that seems conflicting with the initial design of that id being opaque; if this is something that's really needed, I suppose I would more be comfortable with additional trait methods which would be "property-based" (e.g.,is_network_related()
,is_globally_unique()
), but I may well have misunderstood the potential use cases here.
If we opt for returningimpl DeviceId
fromriot_rs::identity::device_id()
, I strongly believe additional traits should be added as supertraits (at leastPartialEq
, so that the device id can be compared for equality). - I still believe that only one of "board", "platform", and "device" should be used for consistency.
- I am still personally not a fan of making
defmt
non-optional.
I'll let @kaspar030 weigh in on these.
#[cfg(context = "nrf52840")] | ||
let ficr = unsafe { nrf52840_pac::Peripherals::steal().FICR }; | ||
#[cfg(context = "nrf52832")] | ||
let ficr = unsafe { nrf52832_pac::Peripherals::steal().FICR }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg(context = "nrf52840")] | |
let ficr = unsafe { nrf52840_pac::Peripherals::steal().FICR }; | |
#[cfg(context = "nrf52832")] | |
let ficr = unsafe { nrf52832_pac::Peripherals::steal().FICR }; | |
#[cfg(context = "nrf52832")] | |
let ficr = unsafe { nrf52832_pac::Peripherals::steal().FICR }; | |
#[cfg(context = "nrf52840")] | |
let ficr = unsafe { nrf52840_pac::Peripherals::steal().FICR }; |
nit: this keeps the order consistent with the manifest
I think it should return an impl. The more we can hide that there are different concrete types implementing what we promise to be portable interfaces, the less users will run into surprises when something works on one platform but not on another. Just means we should do the same with the error type (see below).
The added methods would be trait methods. Depending on how far we seal up the trait (agin, see below), we could only add provided methods (
How would PartialEq be used, or which other traits would make sense? It's not like you can even get a DeviceId that is different from another one you could get. (You can compare the
I'll do an editing run to make it consistently "board".
Unless we seal the traits, we can't pull in traits depending on a feature -- someone might implement them on a type, and then all of a sudden things break when defmt is enabled and another trait is required. (I'll argue that even if we seal, making defmt optional is just a big hassle for no benefit). It's not like we're pulling any mechanism. If an application doesn't enable defmt, it has no impact on the generated code, just like a What's your issue with defmt (as a means of getting the trait impls -- this is not about making it used in any way) as a dependency? So, sealing. (Keep in mind that it's all only really relevant when we release, but I'd like to get this right right away). We are currently pub using the DeviceId trait in riot_rs::identity, which enables any other crate to implement it. Our arch crates implement it from riot_rs_embassy_common, which we kind of declare as internal, but Cargo doesn't know such a distinction. Still, to ensure that implementers are perfectly aware of what they are doing, I'm just about add a riot_rs_embassy_common::Sealed trait. It's not technical sealing, but it documents that whoever implements it opts out of whichever release guarantees RIOT-rs will give applications. That enables us to extend the DeviceId trait later in an implementer-breaking fashion. |
Totally agree 👍
I would also be fine with backtracking on the "making the error an associated type" thing and, if it makes sense, simply provide an
Fine by me for now.
Given this is called
I was initially under the impression that the only properties required for a
|
The latest added patches follow up on the most recent promises. In particular, going with There are to things I'd like to point out about those changes that I find noteworthy but are IMO defensible:
|
A lof of that happened in that the AT is now gone. I'm still with a trait there, though: An enum would be visible at runtime, which means that short of optimizations that are not guaranteed, error handling for "we didn't get a device ID" could still be around at runtime, whereas using an uninhabited error type ensures that the the application's error handling is zero cost when it can be.
Device is very broad. Board is at least less broad, especially as we (and AIU we do) use RDM0003 and narrow it down to "between devices with the same board name". Hm, there is "device" in there, I'll give it a try. Might still become a mix of terms, but would be consistent in that Device is used when talking of a concrete thing, Board to talk about the class, and "implemented for a" could use either (and if you prefer I go with device) because "being implemented at" can apply to any level.
They are there for comparison, but are compared to other identifiers that are not turned into a DeviceId (which itself may be zero-sized) through their bytes property (or other properties). Formatting is more a convenience property for debugging; any efficient channel will transport the identifiers as bytes. The description admits that they may be random and thus prone to collisions. Such static material is not suitable for deriving keys, and that is not planned. The way they are planned to use with #446 is as identifiers. In terms of consequences, a collision may render one of two devices inoperable, but not insecure: They would still generate individual keys, but the second that is being provisioned may lead to the first being revoked. |
This makes defmt optional on a crate level (curious to see whether CI even catches the errors)
This addresses fallout from rebasing.
As per chat discussion, the interface is now simpified to just provide an My summary:
Please give this a rough check as to whether it's the direction you want. |
Documentation is now updated to reflect all the changes, and consistently speaks of IDs of "devices" but with the identifiers scoped to the RIOT-rs "board". I think that this should address all concerns that were left. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the two tiny suggestions from my part, this now LGTM.
@kaspar030 do you want to re-review?
@@ -0,0 +1,32 @@ | |||
//! Access to unique identifiers provided by the device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first actual code in the riot-rs
crate. Also, the first that adds docs.
random
and storage
get their own crate. Maybe move this into e.g., riot-rs-identity
as well?
I'm not quite convinced of the error/infallible handling. Do we expect |
Description
At latest for provisioning, we'll need some sense of identity of a device (like serial numbers) – no just "this is a microbit:v2", but "this is this microbit:v2". While cryptographic identities can work around this by just minting a new identity every time a device is flashed, serial-number-ish identifiers help keep the number of simultaneously active identities at bay: They allow observing that a device now has a new cryptographic identity, and its old one can be retired. (The same would be possible with a coordinated configuration storage, but that'll require a stable format that is read before flashing and understood, and even then things fall flat when another OS is flashed onto the same board inbetween).
At the current stage, this is implemented just for nRF; where do we document such trait-but-through-module-switcharoo interfaces? I've seen that at least the HWRNG now has an unimplemented version in
./src/riot-rs-embassy/src/arch/hwrng.rs
, is that where it should be documented, and can architectures where it is not implemented just pull in from there?Open Questions
(Copying from documentation in there)
Uniqueness is currently described in the scope of the board, but this is implemented as part of the
arch
. Implementation experience will show what is more realistic.Ideally, those should be coorinated even outside of the project, so that a hypothetical extension to probe-rs that connects to devices by their ID can reuse the identifiers. This will require some more agreement on their structure as well as their scope. (probe-rs does not work on boards but just on chips).
On errors:
Change checklist
There are body-full mods in the "wrong" place in 3 files, but all of them already violate that rule, so they the new
pub mod identity
was placed in a logical spot within the current style of the module rather than shuffling around inside the code.