Skip to content

Commit

Permalink
Improve PMBus handling for PSC (oxidecomputer#252)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mkeeter authored and beezow committed Nov 11, 2022
1 parent 7a580ae commit 3571654
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 42 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 <bryan@oxide.computer>"]
edition = "2018"
license = "MPL-2.0"
Expand Down
21 changes: 17 additions & 4 deletions cmd/pmbus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
}
Expand Down
92 changes: 60 additions & 32 deletions humility-core/src/hubris.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -116,6 +116,14 @@ struct HubrisConfigI2cPower {
rails: Option<Vec<String>>,
#[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<Vec<HubrisSensorKind>>,
}

impl HubrisConfigI2cPower {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -2118,7 +2127,8 @@ impl HubrisArchive {
}

let sensor_name = |d: &HubrisConfigI2cDevice,
idx: usize|
idx: usize,
kind: HubrisSensorKind|
-> Result<String> {
if let Some(pmbus) = &d.pmbus {
if let Some(rails) = &pmbus.rails {
Expand All @@ -2128,10 +2138,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 {
Expand Down Expand Up @@ -2162,6 +2177,14 @@ impl HubrisArchive {
Ok(format!("{}#{}", d.device, idx))
}
};
let get_sensor = |d: &HubrisConfigI2cDevice,
i: usize,
ndx: usize,
kind: HubrisSensorKind|
-> Result<HubrisSensor> {
let name = sensor_name(d, i, kind)?;
Ok(HubrisSensor { name, kind, device: ndx })
};

if let Some(ref devices) = i2c.devices {
for device in devices {
Expand Down Expand Up @@ -2198,41 +2221,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,
)?);
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/cmd/chip.trycmd
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ For more information try --help

```
$ humility --chip this-can-be-anything -V
humility 0.9.2
humility 0.9.3

```

Expand All @@ -28,7 +28,7 @@ For more information try --help

```
$ humility -c apx432 -V
humility 0.9.2
humility 0.9.3

```

4 changes: 2 additions & 2 deletions tests/cmd/version.trycmd
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ Long version flag:

```
$ humility --version
humility 0.9.2
humility 0.9.3

```

Short version flag:

```
$ humility -V
humility 0.9.2
humility 0.9.3

```

0 comments on commit 3571654

Please sign in to comment.