sensors-api: add lookup tables from SensorID to component ID/name - #2645
sensors-api: add lookup tables from SensorID to component ID/name#2645hawkw wants to merge 6 commits into
Conversation
|
This is currently a draft because I need to figure out what to put in the (now mandatory) |
|
using @jamesmunns's new xtask from #2644, we can see that there's no actual change to the eliza@hekate ~/Code/oxide/hubris $ cargo xtask i2c-codegen -d sensors --fmt --output cosmo-i2c-before.rs app/cosmo/rev-b.toml
Finished `dev` profile [optimized + debuginfo] target(s) in 0.13s
Running `target/debug/xtask i2c-codegen -d sensors --fmt --output cosmo-i2c-before.rs app/cosmo/rev-b.toml`
will invoke: /home/eliza/.rustup/toolchains/1.95.0-x86_64-unknown-linux-gnu/bin/rustfmt
eliza@hekate ~/Code/oxide/hubris $ git co eliza/turn-an-id-into-a-refdes
Switched to branch 'eliza/turn-an-id-into-a-refdes'
Your branch is ahead of 'origin/eliza/turn-an-id-into-a-refdes' by 4 commits.
(use "git push" to publish your local commits)
eliza@hekate ~/Code/oxide/hubris $ cargo xtask i2c-codegen -d sensors --fmt --output cosmo-i2c-after.rs app/cosmo/rev-b.toml
Compiling build-i2c v0.1.0 (/home/eliza/Code/oxide/hubris/build/i2c)
Compiling xtask v1.0.0 (/home/eliza/Code/oxide/hubris/build/xtask)
Finished `dev` profile [optimized + debuginfo] target(s) in 0.90s
Running `target/debug/xtask i2c-codegen -d sensors --fmt --output cosmo-i2c-after.rs app/cosmo/rev-b.toml`
will invoke: /home/eliza/.rustup/toolchains/1.95.0-x86_64-unknown-linux-gnu/bin/rustfmt
eliza@hekate ~/Code/oxide/hubris $ diff cosmo-i2c-before.rs cosmo-i2c-after.rs
eliza@hekate ~/Code/oxide/hubris $ |
jamesmunns
left a comment
jamesmunns
left a comment
There was a problem hiding this comment.
Overall looks reasonable to me, added some suggestions.
At some point, I wonder if we should move the sensor-api and validate codegen into build-i2c, if for no other reason than to benefit from any refactoring and the ability to snapshot the output. I don't think that needs to be done in this PR, but might be something to keep in mind for #501.
| self.generate_footer()?; | ||
|
|
||
| Ok(self.output) | ||
| Ok((self.output, outputs)) |
There was a problem hiding this comment.
Would it maybe make more sense to make CodegenOutputs contain both the output string as well as the sensors contents?
That way we could return a single struct instead of a tuple.
I guess then codegen_to_string doesn't make as much sense, but I'm open to other names.
| build_i2c::codegen(build_i2c::Disposition::Sensors)?; | ||
| let i2c_outputs = build_i2c::codegen(build_i2c::Disposition::Sensors)?; | ||
|
|
||
| let i2c_sensors = i2c_outputs.sensors.expect( |
There was a problem hiding this comment.
I don't love that we have an Option field vs a different codegen method, but I also don't know if it's worth a more invasive change (yet).
It might be worth documenting that CodegenOutputs.sensors is only populated in Disposition::Sensors.
I do wonder if at some point we want to get rid of the disposition enum and have one codegen method per disposition instead.
Mostly thinking out loud, feel free to ignore for this PR if it's not worth it.
| "{}_{}_{sensor_type}", | ||
| d.device.to_ascii_uppercase(), | ||
| d.name.to_ascii_uppercase(), | ||
| sensor_type.to_ascii_uppercase() |
There was a problem hiding this comment.
Does it matter we lost the to_ascii_uppercase()?
| #[cfg(feature = "component-id-lookup")] | ||
| pub fn component_id( | ||
| &self, | ||
| ) -> fixedstr::FixedStr<'static, { config::MAX_COMPONENT_ID_LEN }> { |
There was a problem hiding this comment.
Could we get away with returning &'static fixedstr::FixedStr<'static, { config::MAX_COMPONENT_ID_LEN } here to avoid making a stack copy, if we're likely going to just copy this into some output structure anyway?
|
re-kicked CI, it looks like it timed out, likely due to github jank during the outage yesterday. |
2 participants
As I described in #2364, there are several places (mostly in the
thermaltask for now, but this may change) where it is necessary to be able to go from a numeric, build-specificSensorIdto a sensor name (stable, defined in the app.toml), or aComponentId(the same, but also used for SP/MGS communication). This branch implements such a mechanism by enhancing thetask-sensor-apibuild script with the (optional) ability to generate lookup tables mapping a sensor ID index to the corresponding component ID and/or sensor name strings. This is feature flagged so that only tasks that depend on the ability to look up these values can generate the LUTs.While working on this, I also did a bit of refactoring to
build-i2c; in particular, I changed it up so that we are now storingDeviceSensorin anArcand usingiddqd::IdOrdMapto store them, so that the ID field defined in theDeviceSensorstruct need not be duplicated to serve as the map key. I think there's room to continue along these lines by adding newtypes that wrap anArc<DeviceSensor>and implementIdOrdItemwith all of the different key types by which we might look up a sensor, and replace some of the other maps with a similar pattern. I didn't do that here to keep the change fairly minimal, but I think there are a lot of places where we might benefit from using Rain's work oniddqdin the I2C codegen datastructures.Also, it was necessary to add refdes strings to a few sensors in
app.tomls that didn't previously have one. So I did that too.Fixes #2364