From 872022087a07baeacf80e682a2c0fcb9caf0897a Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 27 May 2026 13:10:07 +0200 Subject: [PATCH 1/5] Start code generation for validate-api This provides rail to function mapping to obtain I2cDevice and rail indexes. --- build/i2c/src/lib.rs | 1 + task/validate-api/build.rs | 59 ++++++++++++++++++++++++++++++++++-- task/validate-api/src/lib.rs | 2 ++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index 4305f8ccf..9fcd0bbf1 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -1776,6 +1776,7 @@ pub fn codegen(settings: impl Into) -> Result<()> { Ok(()) } +#[derive(Debug, Clone)] pub struct I2cDeviceDescription { pub device: String, pub description: String, diff --git a/task/validate-api/build.rs b/task/validate-api/build.rs index 4f0d3c8b4..4aaaa2e5b 100644 --- a/task/validate-api/build.rs +++ b/task/validate-api/build.rs @@ -5,6 +5,11 @@ use std::io::Write; fn main() -> Result<(), Box> { + if let Err(e) = build_i2c::codegen(build_i2c::Disposition::Devices) { + println!("cargo::error=failed to generate I2C devices: {e}"); + std::process::exit(1); + } + write_pub_device_descriptions()?; idol::client::build_client_stub( @@ -21,7 +26,7 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { let out_dir = std::env::var("OUT_DIR")?; let dest_path = std::path::Path::new(&out_dir).join("device_descriptions.rs"); - let file = std::fs::File::create(dest_path)?; + let file = std::fs::File::create(&dest_path)?; let mut file = std::io::BufWriter::new(file); writeln!( @@ -57,13 +62,14 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { // key. // let mut id2idx = std::collections::BTreeMap::new(); + let mut pmbus_rail_names = std::collections::BTreeSet::new(); - for (idx, dev) in devices.into_iter().enumerate() { + for (idx, dev) in devices.iter().cloned().enumerate() { let is_pmbus = dev.is_pmbus(); writeln!(file, " DeviceDescription {{")?; writeln!(file, " device: {:?},", dev.device)?; writeln!(file, " description: {:?},", dev.description)?; - if let Some(id) = dev.device_id { + if let Some(ref id) = dev.device_id { if let Ok(component) = SpComponent::try_from(id.as_ref()) { writeln!(file, " id: {:?},", component.id)?; if id2idx.insert(component.id, idx).is_some() { @@ -77,6 +83,15 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { ); ids_too_long += 1; } + + if let Some(ref pmbus) = dev.pmbus { + for rail in pmbus.rails.iter() { + // Returns "is unique", unlike `BTreeMap::insert().is_some()`! + if !pmbus_rail_names.insert(rail.name.clone()) { + panic!("cargo::warn=dupe: {:?}, {:?}", rail.name, dev); + } + } + } } else { println!( "cargo::error=device {:?} ({:?}) hath no device ID (refdes)", @@ -114,6 +129,40 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { } writeln!(file, "];")?; + let max_len = pmbus_rail_names.iter().map(String::len).max(); + if let Some(_len) = max_len { + + // TODO: do we need fixed-length bstrings? If we want to binary search, we probably want them to + // be truncated to some maximum length, so we either need to define the max length at the + // protocol level so mgs knows how long of a string to send us, or we could instead trim the + // trailing nulls and search by that instead. + // + // writeln!(file, "pub const MAX_PMBUS_RAIL_NAME: usize = {len};")?; + // writeln!(file, "pub const PMBUS_RAIL_TO_I2C_DEVICE_MAP: [([u8; MAX_PMBUS_RAIL_NAME], fn(TaskId) -> (drv_i2c_api::I2cDevice, u8)); {}] = [", pmbus_rail_names.len())?; + // for rail in pmbus_rail_names.iter() { + // write!(file, " (*b\"{rail}")?; + // for _ in 0..(len.checked_sub(rail.len()).unwrap()) { + // write!(file, "\\0")?; + // } + // write!(file, "\", ")?; + // write!(file, "crate::i2c_config::pmbus::{}", rail.to_lowercase())?; + // writeln!(file, "),")?; + // } + // writeln!(file, "];")?; + + // Assuming we are going the trimmed route... + writeln!(file)?; + writeln!(file, "pub const PMBUS_RAIL_TO_I2C_DEVICE_MAP: [(&[u8], fn(TaskId) -> (drv_i2c_api::I2cDevice, u8)); {}] = [", pmbus_rail_names.len())?; + for rail in pmbus_rail_names.iter() { + write!(file, " (b\"{rail}\", ")?; + // TODO: Do we need a fancier rail -> func conversion than "to_lowercase"? Probably should check + // what build_i2c does for generating the names and match that. + write!(file, "crate::i2c_config::pmbus::{}", rail.to_lowercase())?; + writeln!(file, "),")?; + } + writeln!(file, "];")?; + } + file.flush()?; anyhow::ensure!(missing_ids == 0, "{missing_ids} devices have no ID!"); @@ -129,5 +178,9 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { SpComponent::MAX_ID_LENGTH, ); + + // panic!("{}", dest_path.display()); + // panic!("{max_len:?} {:#?}", pmbus_rail_names); + Ok(()) } diff --git a/task/validate-api/src/lib.rs b/task/validate-api/src/lib.rs index b55d93cd1..97354a717 100644 --- a/task/validate-api/src/lib.rs +++ b/task/validate-api/src/lib.rs @@ -79,3 +79,5 @@ pub struct DeviceDescription { } include!(concat!(env!("OUT_DIR"), "/device_descriptions.rs")); + +include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); From 4c1fe97af8f9534f2a3c43e93539ffad9cc7672a Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 27 May 2026 19:29:37 +0200 Subject: [PATCH 2/5] Add i2c-devices helper for getting Pmbus Status --- build/i2c/src/lib.rs | 2 ++ drv/i2c-devices/src/lib.rs | 63 +++++++++++++++++++++++++++++++++++--- task/validate-api/build.rs | 3 +- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index 9fcd0bbf1..a8fd453f0 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -1381,6 +1381,8 @@ impl ConfigGenerator { all.sort(); for (rail, (device, index)) in &all { + // if we update this code to be more clever than just to-lowercase'ing the rail names, + // you might need to go update the mapping in `validate-api`! write!( &mut self.output, r##" diff --git a/drv/i2c-devices/src/lib.rs b/drv/i2c-devices/src/lib.rs index d94c9e219..39d4e4d86 100644 --- a/drv/i2c-devices/src/lib.rs +++ b/drv/i2c-devices/src/lib.rs @@ -66,22 +66,29 @@ macro_rules! pmbus_read { } macro_rules! pmbus_rail_read { - ($device:expr, $rail:expr, $cmd:ident) => { + (@raw => $device:expr, $rail:expr, $cmd_code:expr, $len:expr) => { $device - .write_read_reg::( - $cmd::CommandData::code(), + .write_read_reg::( + $cmd_code, &[PAGE::CommandData::code(), $rail], ) .map_err(|code| Error::BadRead { - cmd: $cmd::CommandData::code(), + cmd: $cmd_code, code, }) + }; + + ($device:expr, $rail:expr, $cmd:ident) => {{ + let cmd_code = $cmd::CommandData::code(); + const CMD_LEN: usize = $cmd::CommandData::len(); + + pmbus_rail_read!(@raw => $device, $rail, cmd_code, CMD_LEN) .and_then(|rval| { $cmd::CommandData::from_slice(&rval).ok_or(Error::BadData { cmd: $cmd::CommandData::code(), }) }) - }; + }}; ($device:expr, $rail:expr, $dev:ident::$cmd:ident) => {{ use $dev::{PAGE, $cmd}; @@ -271,6 +278,52 @@ pub trait Validate> { } } +// grumble grumble, copied from `gateway_messages::sp_to_mgs::PmbusStatus` +// grumble grumble, also basically the same as `ereports/src/pwr` +pub struct PmbusStatus { + pub status_word: u16, + pub status_vout: u8, + pub status_iout: u8, + pub status_temperature: u8, + pub status_cml: u8, + pub status_other: u8, + pub status_input: u8, + pub status_mfr_specific: u8, + pub status_fans_1_2: u8, + pub status_fans_3_4: u8, +} + +pub enum PmbusStatusError { + BadRead { cmd: u8, code: ResponseCode }, + BadData { cmd: u8, }, +} + +impl PmbusStatus { + pub fn read_from(dev: &I2cDevice, rail_idx: u8) -> Result { + use pmbus::commands::*; + use PmbusStatusError as Error; + + Ok(PmbusStatus { + status_word: pmbus_rail_read!(dev, rail_idx, STATUS_WORD).map_err(drop)?.0, + status_vout: pmbus_rail_read!(dev, rail_idx, STATUS_VOUT).map_err(drop)?.0, + status_iout: pmbus_rail_read!(dev, rail_idx, STATUS_IOUT).map_err(drop)?.0, + status_temperature: pmbus_rail_read!(dev, rail_idx, STATUS_TEMPERATURE).map_err(drop)?.0, + status_cml: pmbus_rail_read!(dev, rail_idx, STATUS_CML).map_err(drop)?.0, + status_other: pmbus_rail_read!(dev, rail_idx, STATUS_OTHER).map_err(drop)?.0, + status_input: pmbus_rail_read!(dev, rail_idx, STATUS_INPUT).map_err(drop)?.0, + status_fans_1_2: pmbus_rail_read!(dev, rail_idx, STATUS_FANS_1_2).map_err(drop)?.0, + status_fans_3_4: pmbus_rail_read!(dev, rail_idx, STATUS_FANS_3_4).map_err(drop)?.0, + + // Unfortunately, STATUS_MFR_SPECIFIC *is* defined in the pmbus crate, but doesn't have a + // "structured" representation, so instead use a raw representation. It *could* be argued + // that since we're not actually peeking at any of the introspection stuff, we could do + // the same for all the items above, and save ourselved a little indirection, but for now + // just hole-punch the minimum amount necessary + status_mfr_specific: pmbus_rail_read!(@raw => dev, rail_idx, CommandCode::STATUS_MFR_SPECIFIC as u8, 1).map_err(drop)?[0], + }) + } +} + pub mod adm127x; pub mod adt7420; pub mod at24csw080; diff --git a/task/validate-api/build.rs b/task/validate-api/build.rs index 4aaaa2e5b..83a824095 100644 --- a/task/validate-api/build.rs +++ b/task/validate-api/build.rs @@ -155,8 +155,7 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { writeln!(file, "pub const PMBUS_RAIL_TO_I2C_DEVICE_MAP: [(&[u8], fn(TaskId) -> (drv_i2c_api::I2cDevice, u8)); {}] = [", pmbus_rail_names.len())?; for rail in pmbus_rail_names.iter() { write!(file, " (b\"{rail}\", ")?; - // TODO: Do we need a fancier rail -> func conversion than "to_lowercase"? Probably should check - // what build_i2c does for generating the names and match that. + // build_i2c *also* only to-lowercases the rail names to make functions write!(file, "crate::i2c_config::pmbus::{}", rail.to_lowercase())?; writeln!(file, "),")?; } From 41d352d3718545c0ed7bfef721a1932566588367 Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 28 May 2026 14:24:51 +0200 Subject: [PATCH 3/5] Allow for ephemeral hiccups on retrieving status bytes --- drv/i2c-devices/src/lib.rs | 69 ++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/drv/i2c-devices/src/lib.rs b/drv/i2c-devices/src/lib.rs index 39d4e4d86..ebfa3fd82 100644 --- a/drv/i2c-devices/src/lib.rs +++ b/drv/i2c-devices/src/lib.rs @@ -66,7 +66,7 @@ macro_rules! pmbus_read { } macro_rules! pmbus_rail_read { - (@raw => $device:expr, $rail:expr, $cmd_code:expr, $len:expr) => { + (@raw => $device:expr, $rail:expr, $cmd_code:expr, $len:expr $(,)?) => { $device .write_read_reg::( $cmd_code, @@ -78,7 +78,7 @@ macro_rules! pmbus_rail_read { }) }; - ($device:expr, $rail:expr, $cmd:ident) => {{ + ($device:expr, $rail:expr, $cmd:ident $(,)?) => {{ let cmd_code = $cmd::CommandData::code(); const CMD_LEN: usize = $cmd::CommandData::len(); @@ -90,7 +90,7 @@ macro_rules! pmbus_rail_read { }) }}; - ($device:expr, $rail:expr, $dev:ident::$cmd:ident) => {{ + ($device:expr, $rail:expr, $dev:ident::$cmd:ident $(,)?) => {{ use $dev::{PAGE, $cmd}; pmbus_rail_read!($device, $rail, $cmd) }}; @@ -282,44 +282,61 @@ pub trait Validate> { // grumble grumble, also basically the same as `ereports/src/pwr` pub struct PmbusStatus { pub status_word: u16, - pub status_vout: u8, - pub status_iout: u8, - pub status_temperature: u8, - pub status_cml: u8, - pub status_other: u8, - pub status_input: u8, - pub status_mfr_specific: u8, - pub status_fans_1_2: u8, - pub status_fans_3_4: u8, + pub status_vout: Result, + pub status_iout: Result, + pub status_temperature: Result, + pub status_cml: Result, + pub status_other: Result, + pub status_input: Result, + pub status_mfr_specific: Result, + pub status_fans_1_2: Result, + pub status_fans_3_4: Result, } +/// An error for querying PMBus `STATUS_*` registers. +#[derive(Copy, Clone, Debug, PartialEq)] pub enum PmbusStatusError { BadRead { cmd: u8, code: ResponseCode }, BadData { cmd: u8, }, } impl PmbusStatus { - pub fn read_from(dev: &I2cDevice, rail_idx: u8) -> Result { + /// Attempt to read a [`PmbusStatus`] from the given device and rail. + /// + /// This function returns an error if the initial attempt to obtain `STATUS_WORD` from the + /// device fails, otherwise returnining successfully even if "leaf" status bytes were unable + /// to be read, either due to ephemeral hiccups, or that status byte being unsupported by + /// the device queried. + pub fn try_read_from(dev: &I2cDevice, rail_idx: u8) -> Result { use pmbus::commands::*; use PmbusStatusError as Error; Ok(PmbusStatus { - status_word: pmbus_rail_read!(dev, rail_idx, STATUS_WORD).map_err(drop)?.0, - status_vout: pmbus_rail_read!(dev, rail_idx, STATUS_VOUT).map_err(drop)?.0, - status_iout: pmbus_rail_read!(dev, rail_idx, STATUS_IOUT).map_err(drop)?.0, - status_temperature: pmbus_rail_read!(dev, rail_idx, STATUS_TEMPERATURE).map_err(drop)?.0, - status_cml: pmbus_rail_read!(dev, rail_idx, STATUS_CML).map_err(drop)?.0, - status_other: pmbus_rail_read!(dev, rail_idx, STATUS_OTHER).map_err(drop)?.0, - status_input: pmbus_rail_read!(dev, rail_idx, STATUS_INPUT).map_err(drop)?.0, - status_fans_1_2: pmbus_rail_read!(dev, rail_idx, STATUS_FANS_1_2).map_err(drop)?.0, - status_fans_3_4: pmbus_rail_read!(dev, rail_idx, STATUS_FANS_3_4).map_err(drop)?.0, + // Status word *must* succeed, otherwise we don't have reasonable data to return. + // We may want to consider making some/all of these retryable, but for now you either + // get them or you don't. + status_word: pmbus_rail_read!(dev, rail_idx, STATUS_WORD)?.0, + status_vout: pmbus_rail_read!(dev, rail_idx, STATUS_VOUT).map(|v| v.0), + status_iout: pmbus_rail_read!(dev, rail_idx, STATUS_IOUT).map(|v| v.0), + status_temperature: pmbus_rail_read!(dev, rail_idx, STATUS_TEMPERATURE).map(|v| v.0), + status_cml: pmbus_rail_read!(dev, rail_idx, STATUS_CML).map(|v| v.0), + status_other: pmbus_rail_read!(dev, rail_idx, STATUS_OTHER).map(|v| v.0), + status_input: pmbus_rail_read!(dev, rail_idx, STATUS_INPUT).map(|v| v.0), + status_fans_1_2: pmbus_rail_read!(dev, rail_idx, STATUS_FANS_1_2).map(|v| v.0), + status_fans_3_4: pmbus_rail_read!(dev, rail_idx, STATUS_FANS_3_4).map(|v| v.0), // Unfortunately, STATUS_MFR_SPECIFIC *is* defined in the pmbus crate, but doesn't have a - // "structured" representation, so instead use a raw representation. It *could* be argued + // "structured" representation, so instead use a raw accessor. It *could* be argued // that since we're not actually peeking at any of the introspection stuff, we could do - // the same for all the items above, and save ourselved a little indirection, but for now - // just hole-punch the minimum amount necessary - status_mfr_specific: pmbus_rail_read!(@raw => dev, rail_idx, CommandCode::STATUS_MFR_SPECIFIC as u8, 1).map_err(drop)?[0], + // the same for all the items above, and save ourselves a little indirection, but for now + // just hole-punch the minimum amount necessary. Indexing is fine here because we know + // (statically) the returned value is `[u8; 1]`. + status_mfr_specific: pmbus_rail_read!( + @raw => dev, + rail_idx, + CommandCode::STATUS_MFR_SPECIFIC as u8, + 1, + ).map(|v| v[0]), }) } } From c806bcbad43d43da44a43ef6cb5cd859b8934cf7 Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 28 May 2026 15:06:32 +0200 Subject: [PATCH 4/5] Move codegen from `validate-api` to `control-plane-agent` --- Cargo.lock | 2 + task/control-plane-agent/Cargo.toml | 2 + task/control-plane-agent/build.rs | 58 ++++++++++++++++++++++++++++ task/control-plane-agent/src/main.rs | 4 ++ task/validate-api/build.rs | 58 ++-------------------------- task/validate-api/src/lib.rs | 2 - 6 files changed, 69 insertions(+), 57 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ece9f7bd9..da9827434 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5654,6 +5654,7 @@ name = "task-control-plane-agent" version = "0.1.0" dependencies = [ "anyhow", + "build-i2c", "build-util", "cfg-if", "counters", @@ -5662,6 +5663,7 @@ dependencies = [ "drv-caboose-pos", "drv-cpu-seq-api", "drv-hf-api", + "drv-i2c-api", "drv-ignition-api", "drv-lpc55-update-api", "drv-monorail-api", diff --git a/task/control-plane-agent/Cargo.toml b/task/control-plane-agent/Cargo.toml index 4ab7d0657..6cad251d9 100644 --- a/task/control-plane-agent/Cargo.toml +++ b/task/control-plane-agent/Cargo.toml @@ -25,6 +25,7 @@ drv-caboose = { path = "../../drv/caboose" } drv-caboose-pos = { path = "../../drv/caboose-pos" } drv-hf-api = { path = "../../drv/hf-api", optional = true } drv-cpu-seq-api = { path = "../../drv/cpu-seq-api", optional = true } +drv-i2c-api = { path = "../../drv/i2c-api" } drv-ignition-api = { path = "../../drv/ignition-api", optional = true } drv-lpc55-update-api = { path = "../../drv/lpc55-update-api" } drv-monorail-api = { path = "../../drv/monorail-api", optional = true } @@ -53,6 +54,7 @@ userlib = { path = "../../sys/userlib", features = ["panic-messages"] } static-cell = { path = "../../lib/static-cell" } [build-dependencies] +build-i2c = { path = "../../build/i2c" } build-util = { path = "../../build/util" } anyhow = { workspace = true } idol = { workspace = true } diff --git a/task/control-plane-agent/build.rs b/task/control-plane-agent/build.rs index cab8cd7b2..6aaf0c7c6 100644 --- a/task/control-plane-agent/build.rs +++ b/task/control-plane-agent/build.rs @@ -35,6 +35,64 @@ fn main() -> Result<(), Box> { if let Some(cfg) = cfg { write_keys(cfg)?; } + + do_pmbus()?; + + Ok(()) +} + +fn do_pmbus() -> Result<(), Box> { + let out_dir = std::env::var("OUT_DIR")?; + let dest_path = + std::path::Path::new(&out_dir).join("pmbus_mapping.rs"); + let file = std::fs::File::create(&dest_path)?; + let mut file = std::io::BufWriter::new(file); + + // Generate the necessary rail names + if let Err(e) = build_i2c::codegen(build_i2c::Disposition::Devices) { + println!("cargo::error=failed to generate I2C devices: {e}"); + std::process::exit(1); + } + + let devices = build_i2c::device_descriptions() + .collect::>(); + + let mut pmbus_rail_names = std::collections::BTreeSet::new(); + let mut pmbus_rail_dupes = 0; + for dev in devices { + // We only need to map PMBus devices + let Some(ref pmbus) = dev.pmbus else { + continue; + }; + + // pmbus devices must have a refdes + assert!(dev.device_id.is_some()); + + // Aggregate a list of all PMBus-visible rails + for rail in pmbus.rails.iter() { + // `BTreeSet::insert` return value means "is unique", which is the inverse of + // `BTreeMap::insert().is_some()`! + if !pmbus_rail_names.insert(rail.name.clone()) { + pmbus_rail_dupes += 1; + println!("cargo::error=Duplicate Rail name: {:?}", rail.name); + } + } + } + + // Create a mapping between rail names and generated accessor functions for obtaining + // the device handle and rail index + writeln!(file)?; + writeln!(file, "pub const PMBUS_RAIL_TO_I2C_DEVICE_MAP: [(&[u8], fn(userlib::TaskId) -> (drv_i2c_api::I2cDevice, u8)); {}] = [", pmbus_rail_names.len())?; + for rail in pmbus_rail_names.iter() { + write!(file, " (b\"{rail}\", ")?; + // build_i2c *also* only to-lowercases the rail names to make functions + write!(file, "crate::i2c_config::pmbus::{}", rail.to_lowercase())?; + writeln!(file, "),")?; + } + writeln!(file, "];")?; + + assert_eq!(pmbus_rail_dupes, 0); + Ok(()) } diff --git a/task/control-plane-agent/src/main.rs b/task/control-plane-agent/src/main.rs index 7a30270fc..2c8dc8387 100644 --- a/task/control-plane-agent/src/main.rs +++ b/task/control-plane-agent/src/main.rs @@ -627,3 +627,7 @@ mod idl { } include!(concat!(env!("OUT_DIR"), "/notifications.rs")); + +include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); + +include!(concat!(env!("OUT_DIR"), "/pmbus_mapping.rs")); diff --git a/task/validate-api/build.rs b/task/validate-api/build.rs index 83a824095..4f0d3c8b4 100644 --- a/task/validate-api/build.rs +++ b/task/validate-api/build.rs @@ -5,11 +5,6 @@ use std::io::Write; fn main() -> Result<(), Box> { - if let Err(e) = build_i2c::codegen(build_i2c::Disposition::Devices) { - println!("cargo::error=failed to generate I2C devices: {e}"); - std::process::exit(1); - } - write_pub_device_descriptions()?; idol::client::build_client_stub( @@ -26,7 +21,7 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { let out_dir = std::env::var("OUT_DIR")?; let dest_path = std::path::Path::new(&out_dir).join("device_descriptions.rs"); - let file = std::fs::File::create(&dest_path)?; + let file = std::fs::File::create(dest_path)?; let mut file = std::io::BufWriter::new(file); writeln!( @@ -62,14 +57,13 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { // key. // let mut id2idx = std::collections::BTreeMap::new(); - let mut pmbus_rail_names = std::collections::BTreeSet::new(); - for (idx, dev) in devices.iter().cloned().enumerate() { + for (idx, dev) in devices.into_iter().enumerate() { let is_pmbus = dev.is_pmbus(); writeln!(file, " DeviceDescription {{")?; writeln!(file, " device: {:?},", dev.device)?; writeln!(file, " description: {:?},", dev.description)?; - if let Some(ref id) = dev.device_id { + if let Some(id) = dev.device_id { if let Ok(component) = SpComponent::try_from(id.as_ref()) { writeln!(file, " id: {:?},", component.id)?; if id2idx.insert(component.id, idx).is_some() { @@ -83,15 +77,6 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { ); ids_too_long += 1; } - - if let Some(ref pmbus) = dev.pmbus { - for rail in pmbus.rails.iter() { - // Returns "is unique", unlike `BTreeMap::insert().is_some()`! - if !pmbus_rail_names.insert(rail.name.clone()) { - panic!("cargo::warn=dupe: {:?}, {:?}", rail.name, dev); - } - } - } } else { println!( "cargo::error=device {:?} ({:?}) hath no device ID (refdes)", @@ -129,39 +114,6 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { } writeln!(file, "];")?; - let max_len = pmbus_rail_names.iter().map(String::len).max(); - if let Some(_len) = max_len { - - // TODO: do we need fixed-length bstrings? If we want to binary search, we probably want them to - // be truncated to some maximum length, so we either need to define the max length at the - // protocol level so mgs knows how long of a string to send us, or we could instead trim the - // trailing nulls and search by that instead. - // - // writeln!(file, "pub const MAX_PMBUS_RAIL_NAME: usize = {len};")?; - // writeln!(file, "pub const PMBUS_RAIL_TO_I2C_DEVICE_MAP: [([u8; MAX_PMBUS_RAIL_NAME], fn(TaskId) -> (drv_i2c_api::I2cDevice, u8)); {}] = [", pmbus_rail_names.len())?; - // for rail in pmbus_rail_names.iter() { - // write!(file, " (*b\"{rail}")?; - // for _ in 0..(len.checked_sub(rail.len()).unwrap()) { - // write!(file, "\\0")?; - // } - // write!(file, "\", ")?; - // write!(file, "crate::i2c_config::pmbus::{}", rail.to_lowercase())?; - // writeln!(file, "),")?; - // } - // writeln!(file, "];")?; - - // Assuming we are going the trimmed route... - writeln!(file)?; - writeln!(file, "pub const PMBUS_RAIL_TO_I2C_DEVICE_MAP: [(&[u8], fn(TaskId) -> (drv_i2c_api::I2cDevice, u8)); {}] = [", pmbus_rail_names.len())?; - for rail in pmbus_rail_names.iter() { - write!(file, " (b\"{rail}\", ")?; - // build_i2c *also* only to-lowercases the rail names to make functions - write!(file, "crate::i2c_config::pmbus::{}", rail.to_lowercase())?; - writeln!(file, "),")?; - } - writeln!(file, "];")?; - } - file.flush()?; anyhow::ensure!(missing_ids == 0, "{missing_ids} devices have no ID!"); @@ -177,9 +129,5 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { SpComponent::MAX_ID_LENGTH, ); - - // panic!("{}", dest_path.display()); - // panic!("{max_len:?} {:#?}", pmbus_rail_names); - Ok(()) } diff --git a/task/validate-api/src/lib.rs b/task/validate-api/src/lib.rs index 97354a717..b55d93cd1 100644 --- a/task/validate-api/src/lib.rs +++ b/task/validate-api/src/lib.rs @@ -79,5 +79,3 @@ pub struct DeviceDescription { } include!(concat!(env!("OUT_DIR"), "/device_descriptions.rs")); - -include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); From b13f7f30ba1564b2b79c2f60b89edff87013d7e3 Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 28 May 2026 19:48:43 +0200 Subject: [PATCH 5/5] Start plumbing API method --- Cargo.lock | 3 +- Cargo.toml | 3 +- build/i2c/src/lib.rs | 2 +- task/control-plane-agent/Cargo.toml | 1 + task/control-plane-agent/src/inventory.rs | 4 +- task/control-plane-agent/src/main.rs | 1 + .../src/mgs_compute_sled.rs | 44 ++++++++++++++++++- task/control-plane-agent/src/update/rot.rs | 2 +- task/validate-api/build.rs | 4 +- 9 files changed, 55 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index da9827434..8037a27ca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3154,7 +3154,7 @@ dependencies = [ [[package]] name = "gateway-messages" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/management-gateway-service#745a508cb97b7ca9b4c10ec9592c980eb769b10d" +source = "git+https://github.com/oxidecomputer/management-gateway-service?branch=james%2Fpmbus-please#437f02bcfbf6064d94e34c88774a641c7b467b4f" dependencies = [ "bitflags 2.9.4", "hubpack", @@ -5664,6 +5664,7 @@ dependencies = [ "drv-cpu-seq-api", "drv-hf-api", "drv-i2c-api", + "drv-i2c-devices", "drv-ignition-api", "drv-lpc55-update-api", "drv-monorail-api", diff --git a/Cargo.toml b/Cargo.toml index 5c5d47e35..1beb5096a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -152,7 +152,8 @@ apob = { git = "https://github.com/oxidecomputer/apob", default-features = false # for the migration. attest-data = { git = "https://github.com/oxidecomputer/dice-util", default-features = false, version = "0.4.0", rev = "a0811d06c75c757a6e12c91ed6ea81fde137ba43" } dice-mfg-msgs = { git = "https://github.com/oxidecomputer/dice-util", default-features = false, version = "0.2.1", rev = "a0811d06c75c757a6e12c91ed6ea81fde137ba43" } -gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", default-features = false, features = ["smoltcp"] } +# gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", default-features = false, features = ["smoltcp"] } +gateway-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", branch = "james/pmbus-please", default-features = false, features = ["smoltcp"] } gateway-ereport-messages = { git = "https://github.com/oxidecomputer/management-gateway-service", default-features = false } gimlet-inspector-protocol = { git = "https://github.com/oxidecomputer/gimlet-inspector-protocol", version = "0.1.0" } hif = { git = "https://github.com/oxidecomputer/hif", default-features = false } diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs index a8fd453f0..bd35c5484 100644 --- a/build/i2c/src/lib.rs +++ b/build/i2c/src/lib.rs @@ -1382,7 +1382,7 @@ impl ConfigGenerator { for (rail, (device, index)) in &all { // if we update this code to be more clever than just to-lowercase'ing the rail names, - // you might need to go update the mapping in `validate-api`! + // you might need to go update the mapping in `control-plane-agent`! write!( &mut self.output, r##" diff --git a/task/control-plane-agent/Cargo.toml b/task/control-plane-agent/Cargo.toml index 6cad251d9..9c81559a9 100644 --- a/task/control-plane-agent/Cargo.toml +++ b/task/control-plane-agent/Cargo.toml @@ -26,6 +26,7 @@ drv-caboose-pos = { path = "../../drv/caboose-pos" } drv-hf-api = { path = "../../drv/hf-api", optional = true } drv-cpu-seq-api = { path = "../../drv/cpu-seq-api", optional = true } drv-i2c-api = { path = "../../drv/i2c-api" } +drv-i2c-devices = { path = "../../drv/i2c-devices" } drv-ignition-api = { path = "../../drv/ignition-api", optional = true } drv-lpc55-update-api = { path = "../../drv/lpc55-update-api" } drv-monorail-api = { path = "../../drv/monorail-api", optional = true } diff --git a/task/control-plane-agent/src/inventory.rs b/task/control-plane-agent/src/inventory.rs index 1ccc860bd..6ca19ee07 100644 --- a/task/control-plane-agent/src/inventory.rs +++ b/task/control-plane-agent/src/inventory.rs @@ -130,7 +130,7 @@ impl Inventory { capabilities |= DeviceCapabilities::HAS_MEASUREMENT_CHANNELS; } DeviceDescription { - component: SpComponent { id: device.id }, + component: todo!("AJM"), // SpComponent { id: device.id } device: device.device, description: device.description, capabilities, @@ -179,7 +179,7 @@ impl TryFrom<&'_ SpComponent> for Index { fn try_from(component: &'_ SpComponent) -> Result { if let Ok(entry_idx) = task_validate_api::DEVICE_INDICES_BY_SORTED_ID - .binary_search_by_key(&component.id, |&(id, _)| id) + .binary_search_by_key(component.id(), |&(id, _)| id) { let &(_, index) = task_validate_api::DEVICE_INDICES_BY_SORTED_ID .get(entry_idx) diff --git a/task/control-plane-agent/src/main.rs b/task/control-plane-agent/src/main.rs index 2c8dc8387..ce3b2629f 100644 --- a/task/control-plane-agent/src/main.rs +++ b/task/control-plane-agent/src/main.rs @@ -51,6 +51,7 @@ mod ignition_controller; task_slot!(JEFE, jefe); task_slot!(NET, net); task_slot!(SYS, sys); +task_slot!(I2C, i2c_driver); #[allow(dead_code)] // Not all cases are used by all variants #[derive(Clone, Copy, PartialEq, ringbuf::Count)] diff --git a/task/control-plane-agent/src/mgs_compute_sled.rs b/task/control-plane-agent/src/mgs_compute_sled.rs index 503402e01..82a836283 100644 --- a/task/control-plane-agent/src/mgs_compute_sled.rs +++ b/task/control-plane-agent/src/mgs_compute_sled.rs @@ -22,7 +22,7 @@ use gateway_messages::{ PostCode, PowerState, PowerStateTransition, RotBootInfo, RotRequest, RotResponse, SERIAL_CONSOLE_IDLE_TIMEOUT, SensorRequest, SensorResponse, SpComponent, SpError, SpPort as GwSpPort, SpRequest, SpStateV2, - SpUpdatePrepare, UpdateChunk, UpdateId, UpdateStatus, ignition, + SpUpdatePrepare, UpdateChunk, UpdateId, UpdateStatus, ignition, PowerRailName, PmbusStatus, PmbusStatusError }; use heapless::{Deque, Vec}; use host_sp_messages::HostStartupOptions; @@ -1259,6 +1259,48 @@ impl SpHandler for MgsHandler { })); self.host_flash_update.get_hash(slot) } + + fn get_pmbus_status(&mut self, rail: &PowerRailName) + -> Result, SpError> + { + let Some(name) = rail.as_str() else { + panic!(); + }; + // this is silly, I'm mostly just trimming trailing nulls, I should add a method for this + let name = name.as_bytes(); + + // TODO: Define error for "unknown rail" + let idx = crate::PMBUS_RAIL_TO_I2C_DEVICE_MAP.binary_search_by_key(&name, |row| row.0).unwrap(); + let (_name, func) = &crate::PMBUS_RAIL_TO_I2C_DEVICE_MAP[idx]; + let (device, rail) = func(crate::I2C.get_task_id()); + + // TODO: Error for read failed? + let res = drv_i2c_devices::PmbusStatus::try_read_from(&device, rail); + + // TODO: Define mgs::PmbusStatusError variants for errors found in drv_i2c_devices::PmbusStatusError + fn err_fixer(_val: drv_i2c_devices::PmbusStatusError) -> PmbusStatusError { + todo!() + } + + match res { + Ok(status) => Ok(Ok(PmbusStatus { + status_word: status.status_word, + status_vout: status.status_vout.map_err(err_fixer), + status_iout: status.status_iout.map_err(err_fixer), + status_temperature: status.status_temperature.map_err(err_fixer), + status_cml: status.status_cml.map_err(err_fixer), + status_other: status.status_other.map_err(err_fixer), + status_input: status.status_input.map_err(err_fixer), + status_mfr_specific: status.status_mfr_specific.map_err(err_fixer), + status_fans_1_2: status.status_fans_1_2.map_err(err_fixer), + status_fans_3_4: status.status_fans_3_4.map_err(err_fixer), + })), + + // TODO: Should this just become an arm of `SpError`? Is it still worth keeping + // the request? + Err(e) => Ok(Err(err_fixer(e))), + } + } } struct UsartHandler { diff --git a/task/control-plane-agent/src/update/rot.rs b/task/control-plane-agent/src/update/rot.rs index 302bc395a..f5aa91b9f 100644 --- a/task/control-plane-agent/src/update/rot.rs +++ b/task/control-plane-agent/src/update/rot.rs @@ -78,7 +78,7 @@ impl ComponentUpdater for RotUpdate { .map_err(SpError::OtherComponentUpdateInProgress)?; // Which target are we updating? - ringbuf_entry!(Trace::Target(update.component.id[0], update.slot)); + ringbuf_entry!(Trace::Target(update.component.id()[0], update.slot)); let target = match (update.component, update.slot) { (SpComponent::ROT, 0) => UpdateTarget::ImageA, (SpComponent::ROT, 1) => UpdateTarget::ImageB, diff --git a/task/validate-api/build.rs b/task/validate-api/build.rs index 4f0d3c8b4..c06623dd3 100644 --- a/task/validate-api/build.rs +++ b/task/validate-api/build.rs @@ -65,8 +65,8 @@ fn write_pub_device_descriptions() -> anyhow::Result<()> { writeln!(file, " description: {:?},", dev.description)?; if let Some(id) = dev.device_id { if let Ok(component) = SpComponent::try_from(id.as_ref()) { - writeln!(file, " id: {:?},", component.id)?; - if id2idx.insert(component.id, idx).is_some() { + writeln!(file, " id: {:?},", component.id())?; + if id2idx.insert(*component.id(), idx).is_some() { println!("cargo::error=duplicate device id {id:?}",); duplicate_ids += 1; }