From 20eb4dd9a8c77649f83678b454b9ee5bfc66cc83 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 27 Oct 2022 13:24:27 -0400 Subject: [PATCH] Improve PMBus handling for PSC (#252) When bringing up the PSC firmware, I ran into a few issues. Both Hubris and Humility assume that for an I2C device with a ```toml power = { rails = [ ... ] } ``` field, all sensors (voltage / current / temperature / etc) correspond one-to-one with power rails. This is problematic for the power shelves, which have two rails (54V and 12V), two fans (uncorrelated to voltage rails), and _three_ temperature sensors (also unrelated to rails!). This PR adds support for an optional `sensors = [ ... ]` field within power, e.g. ```toml [[config.i2c.devices]] bus = "backplane" name = "psu0mcu" address = 0b1011_000 device = "mwocp68" description = "PSU 0 MCU" power = { rails = [ "V54_PSU0", "V12_PSU0" ], sensors = ["voltage", "current"] } sensors = { voltage = 2, current = 2, temperature = 3, speed = 2 } ``` The `sensors` field indicates that only `voltage` and `current` correspond one-to-one with rails; other sensor types are treated as independent. If the `sensors` field is blank (i.e. all of our existing TOML files), then every sensor corresponds one-to-one with rails, matching the current behavior. In addition, `humility pmbus --summarize` is modified to not `bail!` on an error; instead, it prints the error and continues. This makes working with the PSC nicer, because not all 6 PSUs will be populated at all times. --- Cargo.lock | 2 +- Cargo.toml | 2 +- cmd/pmbus/src/lib.rs | 21 +++++++-- humility-core/src/hubris.rs | 92 ++++++++++++++++++++++++------------- tests/cmd/chip.trycmd | 4 +- tests/cmd/version.trycmd | 4 +- 6 files changed, 83 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a2f5d9e7c..ce3e627d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -992,7 +992,7 @@ dependencies = [ [[package]] name = "humility" -version = "0.9.2" +version = "0.9.3" dependencies = [ "anyhow", "bitfield", diff --git a/Cargo.toml b/Cargo.toml index 37cc646e2..25eb1055e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ name = "humility" # # Be sure to check in and push all of the files that change. Happy versioning! # -version = "0.9.2" +version = "0.9.3" authors = ["Bryan Cantrill "] edition = "2018" license = "MPL-2.0" diff --git a/cmd/pmbus/src/lib.rs b/cmd/pmbus/src/lib.rs index 832dd34b9..5e6346566 100644 --- a/cmd/pmbus/src/lib.rs +++ b/cmd/pmbus/src/lib.rs @@ -759,18 +759,24 @@ fn summarize( let results = context.run(core, ops.as_slice(), None)?; let mut base = 0; - print!("{:13} {:16} {:3} {:4}", "DEVICE", "RAIL", "PG?", "#FLT"); + print!( + "{:13} {:16} {:3} {:4}", + "DEVICE".bold(), + "RAIL".bold(), + "PG?".bold(), + "#FLT".bold() + ); for (_, header) in commands.iter() { if let Some(header) = header { - print!(" {:>width$}", header, width = width); + print!(" {:>width$}", header.bold(), width = width); } } println!(); for (device, driver, rail, calls) in &work { - summarize_rail( + if let Err(e) = summarize_rail( subargs, device, driver, @@ -779,7 +785,14 @@ fn summarize( &results[base..base + calls.len()], func, width, - )?; + ) { + println!( + " {0} {1} {2} {0} ", + "--".dimmed(), + "error:".yellow(), + e, + ); + } base += calls.len(); } diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 5a7563ecf..60d21f418 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -36,7 +36,7 @@ const OXIDE_NT_BASE: u32 = 0x1de << 20; const OXIDE_NT_HUBRIS_ARCHIVE: u32 = OXIDE_NT_BASE + 1; const OXIDE_NT_HUBRIS_REGISTERS: u32 = OXIDE_NT_BASE + 2; -const MAX_HUBRIS_VERSION: u32 = 3; +const MAX_HUBRIS_VERSION: u32 = 4; #[derive(Default, Debug)] pub struct HubrisManifest { @@ -116,6 +116,14 @@ struct HubrisConfigI2cPower { rails: Option>, #[serde(default = "HubrisConfigI2cPower::default_pmbus")] pmbus: bool, + + /// Lists which sensor types have a one-to-one association with power rails + /// + /// When `None`, we assume that all sensor types are mapped one-to-one with + /// rails. Otherwise, *only* the listed sensor types are associated with + /// rails (which is the case in systems with independent temperature sensor + /// and power rails). + sensors: Option>, } impl HubrisConfigI2cPower { @@ -208,7 +216,8 @@ pub struct HubrisI2cDevice { pub removable: bool, } -#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Copy, Clone, Deserialize, Debug, PartialEq, Eq, Hash)] +#[serde(rename_all = "lowercase")] pub enum HubrisSensorKind { Temperature, Power, @@ -2218,7 +2227,8 @@ impl HubrisArchive { } let sensor_name = |d: &HubrisConfigI2cDevice, - idx: usize| + idx: usize, + kind: HubrisSensorKind| -> Result { if let Some(pmbus) = &d.pmbus { if let Some(rails) = &pmbus.rails { @@ -2228,10 +2238,15 @@ impl HubrisArchive { bail!("sensor count exceeds rails for {:?}", d); } } - } - - if let Some(power) = &d.power { - if let Some(rails) = &power.rails { + } else if d.power.is_some() + && d.power + .as_ref() + .unwrap() + .sensors + .as_ref() + .map_or(true, |s| s.contains(&kind)) + { + if let Some(rails) = &d.power.as_ref().unwrap().rails { if idx < rails.len() { return Ok(rails[idx].clone()); } else { @@ -2262,6 +2277,14 @@ impl HubrisArchive { Ok(format!("{}#{}", d.device, idx)) } }; + let get_sensor = |d: &HubrisConfigI2cDevice, + i: usize, + ndx: usize, + kind: HubrisSensorKind| + -> Result { + let name = sensor_name(d, i, kind)?; + Ok(HubrisSensor { name, kind, device: ndx }) + }; if let Some(ref devices) = i2c.devices { for device in devices { @@ -2298,41 +2321,46 @@ impl HubrisArchive { let ndx = self.manifest.i2c_devices.len(); for i in 0..sensors.temperature { - self.manifest.sensors.push(HubrisSensor { - name: sensor_name(device, i)?, - kind: HubrisSensorKind::Temperature, - device: ndx, - }); + self.manifest.sensors.push(get_sensor( + device, + i, + ndx, + HubrisSensorKind::Temperature, + )?); } for i in 0..sensors.power { - self.manifest.sensors.push(HubrisSensor { - name: sensor_name(device, i)?, - kind: HubrisSensorKind::Power, - device: ndx, - }); + self.manifest.sensors.push(get_sensor( + device, + i, + ndx, + HubrisSensorKind::Power, + )?); } for i in 0..sensors.current { - self.manifest.sensors.push(HubrisSensor { - name: sensor_name(device, i)?, - kind: HubrisSensorKind::Current, - device: ndx, - }); + self.manifest.sensors.push(get_sensor( + device, + i, + ndx, + HubrisSensorKind::Current, + )?); } for i in 0..sensors.voltage { - self.manifest.sensors.push(HubrisSensor { - name: sensor_name(device, i)?, - kind: HubrisSensorKind::Voltage, - device: ndx, - }); + self.manifest.sensors.push(get_sensor( + device, + i, + ndx, + HubrisSensorKind::Voltage, + )?); } for i in 0..sensors.speed { - self.manifest.sensors.push(HubrisSensor { - name: sensor_name(device, i)?, - kind: HubrisSensorKind::Speed, - device: ndx, - }); + self.manifest.sensors.push(get_sensor( + device, + i, + ndx, + HubrisSensorKind::Speed, + )?); } } diff --git a/tests/cmd/chip.trycmd b/tests/cmd/chip.trycmd index 69e3dd62c..88389ff9c 100644 --- a/tests/cmd/chip.trycmd +++ b/tests/cmd/chip.trycmd @@ -13,7 +13,7 @@ For more information try --help ``` $ humility --chip this-can-be-anything -V -humility 0.9.2 +humility 0.9.3 ``` @@ -28,7 +28,7 @@ For more information try --help ``` $ humility -c apx432 -V -humility 0.9.2 +humility 0.9.3 ``` diff --git a/tests/cmd/version.trycmd b/tests/cmd/version.trycmd index def9fce72..c3a0945f2 100644 --- a/tests/cmd/version.trycmd +++ b/tests/cmd/version.trycmd @@ -2,7 +2,7 @@ Long version flag: ``` $ humility --version -humility 0.9.2 +humility 0.9.3 ``` @@ -10,6 +10,6 @@ Short version flag: ``` $ humility -V -humility 0.9.2 +humility 0.9.3 ```