From 12f278688a624ddcb5f0ea40567b27165785b5a1 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 2 Sep 2026 12:01:10 +0200 Subject: [PATCH 01/10] static-cell: Change unsafe Sync impl for ClaimOnceCell The previous impl was insufficiently broad, and did not allow for types like `core::cell::Cell` to be placed in the ClaimOnceCell. I believe the impl was copied from StaticCell, which needs to have a more restrictive implementation as the user is not given a full `&mut T`. --- lib/static-cell/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/static-cell/src/lib.rs b/lib/static-cell/src/lib.rs index 147cd8b932..70a7434822 100644 --- a/lib/static-cell/src/lib.rs +++ b/lib/static-cell/src/lib.rs @@ -99,7 +99,7 @@ pub struct ClaimOnceCell { // Safety: because a `ClaimOnceCell` may only create a single mutable reference to // the inner value a single time, it can implement `Sync` freely, as the inner // `UnsafeCell`'s value cannot be mutably aliased. -unsafe impl Sync for ClaimOnceCell where for<'a> &'a T: Send {} +unsafe impl Sync for ClaimOnceCell where T: Send {} impl ClaimOnceCell { /// Returns a new `ClaimOnceCell` containing the provided `value`. From f08e458f64ffdee221bb93985ae1ea2c229d73f4 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 2 Sep 2026 11:48:26 +0200 Subject: [PATCH 02/10] Implement double-sampling of transceiver temps --- drv/transceivers-server/src/main.rs | 223 ++++++++++++++++++++++------ 1 file changed, 180 insertions(+), 43 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index b00d587a40..800b276261 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -81,6 +81,19 @@ enum Trace { DisableFailed(usize, LogicalPortMask), ClearDisabledPorts(LogicalPortMask), SeqError(SeqError), + TemperatureGlitch(usize, MilliCelsiusFixed), +} + +/// Trace requires that types are Eq, and floats are not Eq. Make a little +/// helper type that stores milli-celcius as a fixed point number +#[derive(Copy, Clone, PartialEq, Eq)] +struct MilliCelsiusFixed(u32); + +impl From for MilliCelsiusFixed { + fn from(value: f32) -> Self { + let value = value * 1000.0; + Self(value as u32) + } } counted_ringbuf!(Trace, 16, Trace::None); @@ -437,59 +450,43 @@ impl ServerImpl { } } - let temperature = match m.interface { - ManagementInterface::Cmis => self.read_cmis_temperature(port), - ManagementInterface::Sff8636 => { - self.read_sff8636_temperature(port) - } - ManagementInterface::Unknown(..) => { - // We should never get here, because we only assign - // `self.thermal_models[i]` if the management interface is - // known. - continue; - } - }; - let mut got_error = false; - match temperature { + let res = self.get_temperature_resample(port, i, m); + match res { Ok(t) => { - // We got a temperature! Send it over to the thermal task self.sensor_api .post_now(TRANSCEIVER_TEMPERATURE_SENSORS[i], t.0); + self.consecutive_errors[i] = 0; } - // We failed to read a temperature :( - // - // This could be because someone unplugged the transceiver - // at exactly the right time, in which case, the error will - // be transient (and we'll remove the transceiver on the - // next pass through this function). - Err(FpgaError::ImplError(e)) => { - use Reg::QSFP::PORT0_STATUS::ErrorEncoded; - match ErrorEncoded::try_from(e) { - Ok(val) => { - got_error |= matches!( - val, - ErrorEncoded::I2CAddressNack - | ErrorEncoded::I2CSclStretchTimeout - ); - ringbuf_entry!(Trace::TemperatureReadError(i, val)) + Err(e) => { + // Log error to ringbuf + e.ringbuf(); + + // TODO(AJM): Old behavior here is a bit funky, we should + // review this. + match e.kind { + // Previously, this did a "continue", and didn't affect + // consecutive errors at all. + TempReadErrorKind::UnknownInterface => {} + // This would *increment* consecutive errors + TempReadErrorKind::ReallyBadTempRead(_) => { + self.consecutive_errors[i] = + self.consecutive_errors[i].saturating_add(1); } - Err(_) => { - // Error code cannot be decoded - ringbuf_entry!(Trace::InvalidPortStatusError(i, e)) + // All of these *actually reset* the error count. Do + // we want this? + TempReadErrorKind::BadTempRead(_) + | TempReadErrorKind::BadPortStatus(_) + | TempReadErrorKind::UnexpectedFpgaErr(_) => { + self.consecutive_errors[i] = 0; } + // We probably don't want to count this against the + // device, since it could have been a momentary I2C + // glitch. + TempReadErrorKind::UnexpectedVariance(_) => {} } } - Err(e) => { - ringbuf_entry!(Trace::TemperatureReadUnexpectedError(i, e)); - } } - self.consecutive_errors[i] = if got_error { - self.consecutive_errors[i].saturating_add(1) - } else { - 0 - }; - if self.consecutive_errors[i] >= MAX_CONSECUTIVE_ERRORS { to_disable.set(port); } @@ -563,6 +560,146 @@ impl ServerImpl { self.update_thermal_loop(status); } + + fn get_temperature_resample( + &self, + port: LogicalPort, + idx: usize, + m: &ThermalModel, + ) -> Result { + // Attempt to get the temperature twice to determine if we get a stable + // temperature. If either attempt fails, just return the error. + let a = self.get_temperature_once(port, idx, m)?; + let b = self.get_temperature_once(port, idx, m)?; + let diff = (a.0 - b.0).abs(); + + // It has probably been milliseconds, we don't expect the temperature + // to change significantly in this time. + if diff >= 5.0f32 { + Err(TempReadError { + idx, + kind: TempReadErrorKind::UnexpectedVariance( + MilliCelsiusFixed::from(diff), + ), + }) + } else { + Ok(a) + } + } + + fn get_temperature_once( + &self, + port: LogicalPort, + idx: usize, + m: &ThermalModel, + ) -> Result { + let res = match m.interface { + ManagementInterface::Cmis => self.read_cmis_temperature(port), + ManagementInterface::Sff8636 => self.read_sff8636_temperature(port), + ManagementInterface::Unknown(..) => { + // We should never get here, because we only assign + // `self.thermal_models[i]` if the management interface is + // known. + return Err(TempReadError { + idx, + kind: TempReadErrorKind::UnknownInterface, + }); + } + }; + + res.map_err(|e| TempReadError::from_idx_err(idx, e)) + } +} + +struct TempReadError { + idx: usize, + kind: TempReadErrorKind, +} + +impl TempReadError { + fn from_idx_err(idx: usize, err: FpgaError) -> Self { + // We only expect an ImplError here + let FpgaError::ImplError(code) = err else { + return Self { + idx, + kind: TempReadErrorKind::UnexpectedFpgaErr(err), + }; + }; + + // We failed to read a temperature :( + // + // This could be because someone unplugged the transceiver + // at exactly the right time, in which case, the error will + // be transient (and we'll remove the transceiver on the + // next pass through `update_thermal_loop()`). + + use Reg::QSFP::PORT0_STATUS::ErrorEncoded; + let res = ErrorEncoded::try_from(code); + let Ok(decoded) = res else { + // Error code cannot be decoded + return Self { + idx, + kind: TempReadErrorKind::BadPortStatus(code), + }; + }; + + let is_read_err = match decoded { + // We consider these errors as potentially worth invalidating the + // QSFP over. + ErrorEncoded::I2CAddressNack => true, + ErrorEncoded::I2CSclStretchTimeout => true, + + // TODO(AJM): why *don't* we consider these errors as worth + // potentially invalidating the QSFP? + ErrorEncoded::NoError => false, + ErrorEncoded::NoModule => false, + ErrorEncoded::NoPower => false, + ErrorEncoded::PowerFault => false, + ErrorEncoded::NotInitialized => false, + ErrorEncoded::I2CByteNack => false, + ErrorEncoded::I2CTransactionTimeout => false, + }; + + Self { + idx, + kind: if is_read_err { + TempReadErrorKind::ReallyBadTempRead(decoded) + } else { + TempReadErrorKind::BadTempRead(decoded) + }, + } + } + + fn ringbuf(&self) { + let trace = match self.kind { + // We don't ringbuf for this, should never happen + TempReadErrorKind::UnknownInterface => return, + + TempReadErrorKind::ReallyBadTempRead(e) + | TempReadErrorKind::BadTempRead(e) => { + Trace::TemperatureReadError(self.idx, e) + } + TempReadErrorKind::BadPortStatus(r) => { + Trace::InvalidPortStatusError(self.idx, r) + } + TempReadErrorKind::UnexpectedFpgaErr(e) => { + Trace::TemperatureReadUnexpectedError(self.idx, e) + } + TempReadErrorKind::UnexpectedVariance(diff) => { + Trace::TemperatureGlitch(self.idx, diff) + } + }; + ringbuf_entry!(trace); + } +} + +enum TempReadErrorKind { + UnknownInterface, + ReallyBadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), + BadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), + BadPortStatus(u8), + UnexpectedFpgaErr(FpgaError), + UnexpectedVariance(MilliCelsiusFixed), } //////////////////////////////////////////////////////////////////////////////// From 439e9b0712682293b722edbdb49e4f3d28fdc9a0 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 2 Sep 2026 12:00:01 +0200 Subject: [PATCH 03/10] Make the transceivers server a ClaimOnceCell so we can pull it in dumps --- drv/transceivers-server/src/main.rs | 33 ++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 800b276261..826956d9dd 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -5,6 +5,8 @@ #![no_std] #![no_main] +use core::mem::MaybeUninit; + use counters::Count; use idol_runtime::{NotificationHandler, RequestError}; use multitimer::{Multitimer, Repeat}; @@ -48,6 +50,12 @@ task_slot!(SEQ, seq); #[cfg(feature = "thermal-control")] task_slot!(THERMAL, thermal); +/// We store the ServerImpl as a static so we can pull information about it +/// in a dump. +#[unsafe(no_mangle)] +static XCVR_SERVER_IMPL: ClaimOnceCell> = + ClaimOnceCell::new(MaybeUninit::uninit()); + include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); #[allow(dead_code)] @@ -112,6 +120,19 @@ counted_ringbuf!(Trace, 16, Trace::None); /// whole system (because the transceiver stops reporting its temperature). const MAX_CONSECUTIVE_ERRORS: u8 = 3; +/// We have potentially observed transceivers reporting temperature values that +/// do not make sense, and in some cases reporting an unbelievably high value +/// that causes the sidecar's thermal protection loop to shut down immediately. +/// +/// For this reason, we double-sample each transceiver, and if the two readings +/// taken in short succession vary by more than this amount, we discard the +/// reading entirely, trying again on the next tick. +/// +/// This number is a "wild guess", but across two readings a few milliseconds +/// apart, we expect very little difference (probably <1.0C), even factoring in +/// potential sample noise and precision limitations. +const MAX_RESAMPLE_VARIANCE_CELSIUS: f32 = 5.0f32; + //////////////////////////////////////////////////////////////////////////////// #[derive(Copy, Clone)] @@ -465,7 +486,8 @@ impl ServerImpl { // review this. match e.kind { // Previously, this did a "continue", and didn't affect - // consecutive errors at all. + // consecutive errors at all. This should never happen + // because we only add models to known interface types TempReadErrorKind::UnknownInterface => {} // This would *increment* consecutive errors TempReadErrorKind::ReallyBadTempRead(_) => { @@ -575,7 +597,7 @@ impl ServerImpl { // It has probably been milliseconds, we don't expect the temperature // to change significantly in this time. - if diff >= 5.0f32 { + if diff >= MAX_RESAMPLE_VARIANCE_CELSIUS { Err(TempReadError { idx, kind: TempReadErrorKind::UnexpectedVariance( @@ -782,7 +804,8 @@ fn main() -> ! { #[cfg(feature = "thermal-control")] let thermal_api = Thermal::from(THERMAL.get_task_id()); - let mut server = ServerImpl { + let server = XCVR_SERVER_IMPL.claim(); + let server = server.write(ServerImpl { transceivers, leds, net, @@ -799,7 +822,7 @@ fn main() -> ! { thermal_api, sensor_api, thermal_models: [None; NUM_PORTS as usize], - }; + }); // There are two timers, one for each communication bus: #[derive(Copy, Clone, Enum)] @@ -894,7 +917,7 @@ fn main() -> ! { server .check_net(tx_data_buf.as_mut_slice(), rx_data_buf.as_mut_slice()); - idol_runtime::dispatch(&mut buffer, &mut server); + idol_runtime::dispatch(&mut buffer, server); } } From 755934d7d1ecf2b18440293710953a95f5e3565c Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 2 Sep 2026 13:43:21 +0200 Subject: [PATCH 04/10] Implement metadata and stats --- drv/transceivers-server/src/main.rs | 330 ++++++++++++++++------------ drv/transceivers-server/src/udp.rs | 36 +-- 2 files changed, 209 insertions(+), 157 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 826956d9dd..134d2d59b5 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -145,11 +145,41 @@ enum FrontIOStatus { Ready, } +struct PortMetadata { + /// Number of consecutive NACKS seen on a given port + consecutive_errors: u8, + /// Largest observed temperature drift between two samples + peak_diff: f32, + /// Number of times the temperature has been discarded, saturates + discarded_temps: u32, + /// Thermal models are populated by the host + // TODO(AJM): Is the above comment true? We actually fill this in with a + // basic model for each, and I don't *think* there's a UDP api to set this? + model: Option, +} + +impl PortMetadata { + /// New metadata with no model and zeroed counters/stats + const fn new() -> Self { + Self { + consecutive_errors: 0, + peak_diff: 0.0, + discarded_temps: 0, + model: None, + } + } + + /// Like [`Self::new()`] but with the given model. + fn init(&mut self, model: Option) { + *self = Self::new(); + self.model = model; + } +} + struct ServerImpl { - transceivers: Transceivers, + xcvr_api: XcvrApi, leds: Leds, net: task_net_api::Net, - modules_present: LogicalPortMask, /// The Front IO board is not guaranteed to be present and ready front_io_board_present: FrontIOStatus, @@ -161,11 +191,17 @@ struct ServerImpl { blink_on: bool, system_led_state: LedState, + // TODO(AJM): move these two into port metadata? It's less efficient than + // a pair of bitmasks, but might be easier to keep all the per-port state + // in a single place, maybe with a more explicit state machine. It would + // require some rework, as the logic is currently very bitmask-set oriented + // + /// Modules that are physically present + modules_present: LogicalPortMask, /// Modules that are physically present but disabled by Hubris disabled: LogicalPortMask, - /// Number of consecutive NACKS seen on a given port - consecutive_errors: [u8; NUM_PORTS as usize], + ports: [PortMetadata; NUM_PORTS as usize], /// Handle to write thermal models and presence to the `thermal` task #[cfg(feature = "thermal-control")] @@ -173,10 +209,8 @@ struct ServerImpl { /// Handle to write temperatures to the `sensors` task sensor_api: Sensor, - - /// Thermal models are populated by the host - thermal_models: [Option; NUM_PORTS as usize], } + #[derive(Copy, Clone)] struct ThermalModel { /// What kind of transceiver is this? @@ -265,7 +299,14 @@ impl ServerImpl { } } -impl ServerImpl { +/// Wrapper struct that encapsulates logic that only uses Transceivers +/// +/// This exists to make it easier to split the borrows of the ServerImpl. +struct XcvrApi { + transceivers: Transceivers, +} + +impl XcvrApi { /// Returns the temperature from a CMIS transceiver. /// /// `port` is a logical port index, i.e. 0-31. @@ -385,23 +426,74 @@ impl ServerImpl { } } + fn get_temperature_resample( + &self, + port: LogicalPort, + m: &ThermalModel, + ) -> Result<(Celsius, f32), TempReadError> { + // Attempt to get the temperature twice to determine if we get a stable + // temperature. If either attempt fails, just return the error. + let a = self.get_temperature_once(port, m)?; + let b = self.get_temperature_once(port, m)?; + let diff = (a.0 - b.0).abs(); + + // It has probably been milliseconds, we don't expect the temperature + // to change significantly in this time. + if diff >= MAX_RESAMPLE_VARIANCE_CELSIUS { + Err(TempReadError::UnexpectedVariance(diff)) + } else { + Ok((a, diff)) + } + } + + fn get_temperature_once( + &self, + port: LogicalPort, + m: &ThermalModel, + ) -> Result { + let res = match m.interface { + ManagementInterface::Cmis => self.read_cmis_temperature(port), + ManagementInterface::Sff8636 => self.read_sff8636_temperature(port), + ManagementInterface::Unknown(..) => { + // We should never get here, because we only assign + // `self.thermal_models[i]` if the management interface is + // known. + return Err(TempReadError::UnknownInterface); + } + }?; + + Ok(res) + } +} + +impl ServerImpl { fn update_thermal_loop(&mut self, status: ModuleStatus) { - #[allow(clippy::needless_range_loop)] - for i in 0..self.thermal_models.len() { + // Break up the borrows so we can hold multiple items mutably at the + // same time. + let ServerImpl { + xcvr_api, + ports, + sensor_api, + #[cfg(feature = "thermal-control")] + thermal_api, + disabled, + .. + } = self; + + for (i, meta) in ports.iter_mut().enumerate() { let port = LogicalPort(i as u8); let mask = 1 << i; let operational = (!status.modprsl & mask) != 0 && (status.power_good & mask) != 0 && (status.resetl & mask) != 0 - && (self.disabled & port).is_empty(); + && (*disabled & port).is_empty(); // A wild transceiver just appeared! Read it to decide whether it's // using SFF-8636 or CMIS. - if operational && self.thermal_models[i].is_none() { - match self.get_transceiver_interface(port) { + if operational && meta.model.is_none() { + match xcvr_api.get_transceiver_interface(port) { Ok(interface) => { - self.thermal_models[i] = - self.decode_interface(port, interface) + meta.init(xcvr_api.decode_interface(port, interface)); } Err(FpgaError::ImplError(e)) => { match Reg::QSFP::PORT0_STATUS::ErrorEncoded::try_from(e) @@ -424,38 +516,37 @@ impl ServerImpl { )); } } - } else if !operational && self.thermal_models[i].is_some() { + } else if !operational && meta.model.is_some() { #[cfg(feature = "thermal-control")] { // This transceiver went away; remove it from the thermal loop - if let Err(e) = self.thermal_api.remove_dynamic_input(i) { + if let Err(e) = thermal_api.remove_dynamic_input(i) { ringbuf_entry!(Trace::ThermalError(i, e)); } } // Tell the `sensor` task that this device is no longer present - self.sensor_api.nodata_now( + sensor_api.nodata_now( TRANSCEIVER_TEMPERATURE_SENSORS[i], NoData::DeviceNotPresent, ); - if (self.disabled & port).is_empty() { + if (*disabled & port).is_empty() { ringbuf_entry!(Trace::UnpluggedModule(i)); } else { ringbuf_entry!(Trace::RemovedDisabledModuleThermalModel(i)); } - self.thermal_models[i] = None; + meta.model = None; } } // Accumulate ports to disable (but don't disable them in the loop), to // avoid issues with the borrow checker. let mut to_disable = LogicalPortMask(0); - for (i, m) in self.thermal_models.iter().enumerate() { + for (i, meta) in ports.iter_mut().enumerate() { let port = LogicalPort(i as u8); - let m = match m { - Some(m) => m, - None => continue, + let Some(m) = meta.model.as_ref() else { + continue; }; #[cfg(feature = "thermal-control")] @@ -465,51 +556,60 @@ impl ServerImpl { // will return a `NotInAutoMode` error if the thermal loop is in // manual mode; this is harmless and will be ignored (instead of // cluttering up the logs). - match self.thermal_api.update_dynamic_input(i, m.model) { + match thermal_api.update_dynamic_input(i, m.model) { Ok(()) | Err(ThermalError::NotInAutoMode) => (), Err(e) => ringbuf_entry!(Trace::ThermalError(i, e)), } } - let res = self.get_temperature_resample(port, i, m); + // Sample the transceiver temperature multiple times, seeing if + // we are successful and the samples are steady enough to report. + let res = xcvr_api.get_temperature_resample(port, m); match res { - Ok(t) => { - self.sensor_api - .post_now(TRANSCEIVER_TEMPERATURE_SENSORS[i], t.0); - self.consecutive_errors[i] = 0; + Ok((reading, diff)) => { + sensor_api.post_now( + TRANSCEIVER_TEMPERATURE_SENSORS[i], + reading.0, + ); + meta.peak_diff = meta.peak_diff.max(diff); + meta.consecutive_errors = 0; } Err(e) => { // Log error to ringbuf - e.ringbuf(); + e.ringbuf(i); // TODO(AJM): Old behavior here is a bit funky, we should // review this. - match e.kind { + match e { // Previously, this did a "continue", and didn't affect // consecutive errors at all. This should never happen // because we only add models to known interface types - TempReadErrorKind::UnknownInterface => {} + TempReadError::UnknownInterface => {} // This would *increment* consecutive errors - TempReadErrorKind::ReallyBadTempRead(_) => { - self.consecutive_errors[i] = - self.consecutive_errors[i].saturating_add(1); + TempReadError::ReallyBadTempRead(_) => { + meta.consecutive_errors = + meta.consecutive_errors.saturating_add(1); } // All of these *actually reset* the error count. Do // we want this? - TempReadErrorKind::BadTempRead(_) - | TempReadErrorKind::BadPortStatus(_) - | TempReadErrorKind::UnexpectedFpgaErr(_) => { - self.consecutive_errors[i] = 0; + TempReadError::BadTempRead(_) + | TempReadError::BadPortStatus(_) + | TempReadError::UnexpectedFpgaErr(_) => { + meta.consecutive_errors = 0; } // We probably don't want to count this against the // device, since it could have been a momentary I2C // glitch. - TempReadErrorKind::UnexpectedVariance(_) => {} + TempReadError::UnexpectedVariance(diff) => { + meta.peak_diff = meta.peak_diff.max(diff); + meta.discarded_temps = + meta.discarded_temps.saturating_add(1); + } } } } - if self.consecutive_errors[i] >= MAX_CONSECUTIVE_ERRORS { + if meta.consecutive_errors >= MAX_CONSECUTIVE_ERRORS { to_disable.set(port); } } @@ -528,7 +628,7 @@ impl ServerImpl { .iter() .enumerate() { - let err = f(&mut self.transceivers, mask).error(); + let err = f(self.transceivers(), mask).error(); if !err.is_empty() { ringbuf_entry!(Trace::DisableFailed(step, err)); } @@ -560,7 +660,7 @@ impl ServerImpl { fn handle_spi_loop(&mut self) { // Query module presence as this drives other state - let (status, _) = self.transceivers.get_module_status(); + let (status, _) = self.transceivers().get_module_status(); let modules_present = LogicalPortMask(!status.modprsl); if modules_present != self.modules_present { @@ -570,7 +670,7 @@ impl ServerImpl { self.modules_present & !modules_present & self.disabled; if !disabled_ports_removed.is_empty() { self.disabled &= !disabled_ports_removed; - self.transceivers.enable_power(disabled_ports_removed); + self.transceivers().enable_power(disabled_ports_removed); ringbuf_entry!(Trace::ClearDisabledPorts( disabled_ports_removed )); @@ -583,69 +683,38 @@ impl ServerImpl { self.update_thermal_loop(status); } - fn get_temperature_resample( - &self, - port: LogicalPort, - idx: usize, - m: &ThermalModel, - ) -> Result { - // Attempt to get the temperature twice to determine if we get a stable - // temperature. If either attempt fails, just return the error. - let a = self.get_temperature_once(port, idx, m)?; - let b = self.get_temperature_once(port, idx, m)?; - let diff = (a.0 - b.0).abs(); - - // It has probably been milliseconds, we don't expect the temperature - // to change significantly in this time. - if diff >= MAX_RESAMPLE_VARIANCE_CELSIUS { - Err(TempReadError { - idx, - kind: TempReadErrorKind::UnexpectedVariance( - MilliCelsiusFixed::from(diff), - ), - }) - } else { - Ok(a) - } - } - - fn get_temperature_once( - &self, - port: LogicalPort, - idx: usize, - m: &ThermalModel, - ) -> Result { - let res = match m.interface { - ManagementInterface::Cmis => self.read_cmis_temperature(port), - ManagementInterface::Sff8636 => self.read_sff8636_temperature(port), - ManagementInterface::Unknown(..) => { - // We should never get here, because we only assign - // `self.thermal_models[i]` if the management interface is - // known. - return Err(TempReadError { - idx, - kind: TempReadErrorKind::UnknownInterface, - }); - } - }; - - res.map_err(|e| TempReadError::from_idx_err(idx, e)) + pub(crate) fn transceivers(&mut self) -> &mut Transceivers { + &mut self.xcvr_api.transceivers } } -struct TempReadError { - idx: usize, - kind: TempReadErrorKind, +/// Error while reading temperature from QSFP transceiver +enum TempReadError { + /// Tried to read a transceiver that had no assigned thermal model + /// This is a programming error. + UnknownInterface, + /// We failed to read, and the FPGA reported an error code that makes us + /// suspicious that the xcvr is not present or operating correctly in a + /// way that might lead us to disable it. + ReallyBadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), + /// We failed to read, but in some kind of forgivable way that *doesn't* + /// make us suspect that the xcvr should be disabled. + BadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), + /// The FPGA gave us back a status codethat we were unable to decode. This + /// is probably a pretty serious mismatch in FPGA and SP versioning. + BadPortStatus(u8), + /// The FPGA gave us back an FpgaError that we didn't expect at all when + /// reading I2C. + UnexpectedFpgaErr(FpgaError), + /// We read the sensor multiple times, and the + UnexpectedVariance(f32), } -impl TempReadError { - fn from_idx_err(idx: usize, err: FpgaError) -> Self { +impl From for TempReadError { + fn from(err: FpgaError) -> Self { // We only expect an ImplError here let FpgaError::ImplError(code) = err else { - return Self { - idx, - kind: TempReadErrorKind::UnexpectedFpgaErr(err), - }; + return Self::UnexpectedFpgaErr(err); }; // We failed to read a temperature :( @@ -659,10 +728,7 @@ impl TempReadError { let res = ErrorEncoded::try_from(code); let Ok(decoded) = res else { // Error code cannot be decoded - return Self { - idx, - kind: TempReadErrorKind::BadPortStatus(code), - }; + return Self::BadPortStatus(code); }; let is_read_err = match decoded { @@ -682,48 +748,35 @@ impl TempReadError { ErrorEncoded::I2CTransactionTimeout => false, }; - Self { - idx, - kind: if is_read_err { - TempReadErrorKind::ReallyBadTempRead(decoded) - } else { - TempReadErrorKind::BadTempRead(decoded) - }, + if is_read_err { + Self::ReallyBadTempRead(decoded) + } else { + Self::BadTempRead(decoded) } } +} - fn ringbuf(&self) { - let trace = match self.kind { +impl TempReadError { + fn ringbuf(&self, idx: usize) { + let trace = match self { // We don't ringbuf for this, should never happen - TempReadErrorKind::UnknownInterface => return, + Self::UnknownInterface => return, - TempReadErrorKind::ReallyBadTempRead(e) - | TempReadErrorKind::BadTempRead(e) => { - Trace::TemperatureReadError(self.idx, e) - } - TempReadErrorKind::BadPortStatus(r) => { - Trace::InvalidPortStatusError(self.idx, r) + Self::ReallyBadTempRead(e) | Self::BadTempRead(e) => { + Trace::TemperatureReadError(idx, *e) } - TempReadErrorKind::UnexpectedFpgaErr(e) => { - Trace::TemperatureReadUnexpectedError(self.idx, e) + Self::BadPortStatus(r) => Trace::InvalidPortStatusError(idx, *r), + Self::UnexpectedFpgaErr(e) => { + Trace::TemperatureReadUnexpectedError(idx, *e) } - TempReadErrorKind::UnexpectedVariance(diff) => { - Trace::TemperatureGlitch(self.idx, diff) + Self::UnexpectedVariance(diff) => { + Trace::TemperatureGlitch(idx, (*diff).into()) } }; ringbuf_entry!(trace); } } -enum TempReadErrorKind { - UnknownInterface, - ReallyBadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), - BadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), - BadPortStatus(u8), - UnexpectedFpgaErr(FpgaError), - UnexpectedVariance(MilliCelsiusFixed), -} - //////////////////////////////////////////////////////////////////////////////// impl idl::InOrderTransceiversImpl for ServerImpl { @@ -732,7 +785,7 @@ impl idl::InOrderTransceiversImpl for ServerImpl { _msg: &userlib::RecvMessage, ) -> Result> { - let (mod_status, result) = self.transceivers.get_module_status(); + let (mod_status, result) = self.transceivers().get_module_status(); if result.error().is_empty() { Ok(mod_status) } else { @@ -806,7 +859,7 @@ fn main() -> ! { let server = XCVR_SERVER_IMPL.claim(); let server = server.write(ServerImpl { - transceivers, + xcvr_api: XcvrApi { transceivers }, leds, net, modules_present: LogicalPortMask(0), @@ -817,11 +870,10 @@ fn main() -> ! { blink_on: false, system_led_state: LedState::Off, disabled: LogicalPortMask(0), - consecutive_errors: [0; NUM_PORTS as usize], #[cfg(feature = "thermal-control")] thermal_api, sensor_api, - thermal_models: [None; NUM_PORTS as usize], + ports: [const { PortMetadata::new() }; NUM_PORTS as usize], }); // There are two timers, one for each communication bus: @@ -873,7 +925,7 @@ fn main() -> ! { // LED drivers if server.front_io_board_present == FrontIOStatus::Ready { ringbuf_entry!(Trace::LEDInit); - match server.transceivers.enable_led_controllers() { + match server.transceivers().enable_led_controllers() { Ok(_) => server.led_init(), Err(e) => { ringbuf_entry!(Trace::LEDEnableError(e)) diff --git a/drv/transceivers-server/src/udp.rs b/drv/transceivers-server/src/udp.rs index 5c21c44d90..cbbc50c187 100644 --- a/drv/transceivers-server/src/udp.rs +++ b/drv/transceivers-server/src/udp.rs @@ -492,7 +492,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.assert_reset(mask) + self.transceivers().assert_reset(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -517,7 +517,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.deassert_reset(mask) + self.transceivers().deassert_reset(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -542,7 +542,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.assert_lpmode(mask) + self.transceivers().assert_lpmode(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -567,7 +567,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.deassert_lpmode(mask) + self.transceivers().deassert_lpmode(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -592,7 +592,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.enable_power(mask) + self.transceivers().enable_power(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -617,7 +617,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.disable_power(mask) + self.transceivers().disable_power(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -668,7 +668,7 @@ impl ServerImpl { let result = if self.front_io_board_present == FrontIOStatus::Ready { - self.transceivers.clear_power_fault(mask) + self.transceivers().clear_power_fault(mask) } else { ModuleResultNoFailure::new(LogicalPortMask(0), mask) .unwrap_lite() @@ -985,7 +985,7 @@ impl ServerImpl { ) -> (usize, ModuleResultNoFailure) { // This will get the status of every module, so we will have to only // select the data which was requested. - let (mod_status, full_result) = self.transceivers.get_module_status(); + let (mod_status, full_result) = self.transceivers().get_module_status(); // adjust the result success mask to be only our requested modules let desired_result = ModuleResultNoFailure::new( full_result.success() & modules, @@ -1040,7 +1040,7 @@ impl ServerImpl { ) -> (usize, ModuleResultNoFailure) { // This will get the status of every module, so we will have to only // select the data which was requested. - let (mod_status, full_result) = self.transceivers.get_module_status(); + let (mod_status, full_result) = self.transceivers().get_module_status(); // adjust the result success mask to be only our requested modules let desired_result = ModuleResultNoFailure::new( full_result.success() & modules, @@ -1110,8 +1110,8 @@ impl ServerImpl { // We can always write the lower page; upper pages require modifying // registers in the transceiver to select it. if let Some(page) = page.page() { - self.transceivers.set_i2c_write_buffer(&[page], mask); - result = result.chain(self.transceivers.setup_i2c_write( + self.transceivers().set_i2c_write_buffer(&[page], mask); + result = result.chain(self.transceivers().setup_i2c_write( PAGE_SELECT, 1, mask, @@ -1129,8 +1129,8 @@ impl ServerImpl { } if let Some(bank) = page.bank() { - self.transceivers.set_i2c_write_buffer(&[bank], mask); - result = result.chain(self.transceivers.setup_i2c_write( + self.transceivers().set_i2c_write_buffer(&[bank], mask); + result = result.chain(self.transceivers().setup_i2c_write( BANK_SELECT, 1, result.success(), @@ -1153,7 +1153,7 @@ impl ServerImpl { // failure: The I2C operation failed. // error: The SP could not communicate with the FPGA. fn wait_and_check_i2c(&mut self, mask: LogicalPortMask) -> ModuleResult { - self.transceivers.wait_and_check_i2c(mask) + self.transceivers().wait_and_check_i2c(mask) } fn read( @@ -1166,7 +1166,7 @@ impl ServerImpl { let mut result = self.select_page(*mem.page(), modules); // Ask the FPGA to start the read - result = result.chain(self.transceivers.setup_i2c_read( + result = result.chain(self.transceivers().setup_i2c_read( mem.offset(), mem.len(), result.success(), @@ -1186,7 +1186,7 @@ impl ServerImpl { // If we have not encountered any errors, keep pulling full // status + buffer payloads. let status = match self - .transceivers + .transceivers() .get_i2c_status_and_read_buffer(port_loc, &mut buf[0..buf_len]) { Ok(status) => status, @@ -1235,11 +1235,11 @@ impl ServerImpl { let mut result = self.select_page(*mem.page(), modules); // Copy data into the FPGA write buffer - self.transceivers + self.transceivers() .set_i2c_write_buffer(&data[..mem.len() as usize], modules); // Trigger a multicast write to all transceivers in the mask - result = result.chain(self.transceivers.setup_i2c_write( + result = result.chain(self.transceivers().setup_i2c_write( mem.offset(), mem.len(), result.success(), From afaeaa4b2d1ac594c079a2251c391a2d2ac4c4f7 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 2 Sep 2026 18:57:50 +0200 Subject: [PATCH 05/10] Better ringbuf, no Milli-celsius --- drv/transceivers-server/src/main.rs | 123 +++++++++++++++++----------- 1 file changed, 76 insertions(+), 47 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 134d2d59b5..41d77b4f7f 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -59,7 +59,7 @@ static XCVR_SERVER_IMPL: ClaimOnceCell> = include!(concat!(env!("OUT_DIR"), "/i2c_config.rs")); #[allow(dead_code)] -#[derive(Copy, Clone, PartialEq, Eq, Count)] +#[derive(Copy, Clone, PartialEq, Count)] enum Trace { #[count(skip)] None, @@ -77,31 +77,47 @@ enum Trace { TransceiversError(#[count(children)] TransceiversError), GotInterface(u8, ManagementInterface), UnknownInterface(u8, ManagementInterface), - UnpluggedModule(usize), - RemovedDisabledModuleThermalModel(usize), - TemperatureReadError(usize, Reg::QSFP::PORT0_STATUS::ErrorEncoded), - TemperatureReadUnexpectedError(usize, FpgaError), - ThermalError(usize, ThermalError), - GetInterfaceError(usize, Reg::QSFP::PORT0_STATUS::ErrorEncoded), - GetInterfaceUnexpectedError(usize, FpgaError), - InvalidPortStatusError(usize, u8), + UnpluggedModule { + port: LogicalPort, + }, + RemovedDisabledModuleThermalModel { + port: LogicalPort, + }, + TemperatureReadError { + port: LogicalPort, + err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, + }, + TemperatureReadUnexpectedError { + port: LogicalPort, + err: FpgaError, + }, + ThermalError { + port: LogicalPort, + err: ThermalError, + }, + GetInterfaceError { + port: LogicalPort, + err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, + }, + GetInterfaceUnexpectedError { + port: LogicalPort, + err: FpgaError, + }, + InvalidPortStatusError { + port: LogicalPort, + raw_err: u8, + }, DisablingPorts(LogicalPortMask), - DisableFailed(usize, LogicalPortMask), + DisableFailed { + step: usize, + mask: LogicalPortMask, + }, ClearDisabledPorts(LogicalPortMask), SeqError(SeqError), - TemperatureGlitch(usize, MilliCelsiusFixed), -} - -/// Trace requires that types are Eq, and floats are not Eq. Make a little -/// helper type that stores milli-celcius as a fixed point number -#[derive(Copy, Clone, PartialEq, Eq)] -struct MilliCelsiusFixed(u32); - -impl From for MilliCelsiusFixed { - fn from(value: f32) -> Self { - let value = value * 1000.0; - Self(value as u32) - } + TemperatureGlitch { + port: LogicalPort, + variance: Celsius, + }, } counted_ringbuf!(Trace, 16, Trace::None); @@ -481,7 +497,8 @@ impl ServerImpl { } = self; for (i, meta) in ports.iter_mut().enumerate() { - let port = LogicalPort(i as u8); + let port_idx = i as u8; + let port = LogicalPort(port_idx); let mask = 1 << i; let operational = (!status.modprsl & mask) != 0 && (status.power_good & mask) != 0 @@ -498,30 +515,35 @@ impl ServerImpl { Err(FpgaError::ImplError(e)) => { match Reg::QSFP::PORT0_STATUS::ErrorEncoded::try_from(e) { - Ok(val) => { - ringbuf_entry!(Trace::GetInterfaceError(i, val)) + Ok(err) => { + ringbuf_entry!(Trace::GetInterfaceError { + port, + err + }) } Err(_) => { // Error code cannot be decoded - ringbuf_entry!(Trace::InvalidPortStatusError( - i, e - )) + ringbuf_entry!(Trace::InvalidPortStatusError { + port, + raw_err: e + }) } } } - Err(e) => { + Err(err) => { // Not much we can do here if reading failed - ringbuf_entry!(Trace::GetInterfaceUnexpectedError( - i, e - )); + ringbuf_entry!(Trace::GetInterfaceUnexpectedError { + port, + err + }); } } } else if !operational && meta.model.is_some() { #[cfg(feature = "thermal-control")] { // This transceiver went away; remove it from the thermal loop - if let Err(e) = thermal_api.remove_dynamic_input(i) { - ringbuf_entry!(Trace::ThermalError(i, e)); + if let Err(err) = thermal_api.remove_dynamic_input(i) { + ringbuf_entry!(Trace::ThermalError { port, err }); } } @@ -532,9 +554,11 @@ impl ServerImpl { ); if (*disabled & port).is_empty() { - ringbuf_entry!(Trace::UnpluggedModule(i)); + ringbuf_entry!(Trace::UnpluggedModule { port }); } else { - ringbuf_entry!(Trace::RemovedDisabledModuleThermalModel(i)); + ringbuf_entry!(Trace::RemovedDisabledModuleThermalModel { + port + }); } meta.model = None; } @@ -558,7 +582,9 @@ impl ServerImpl { // cluttering up the logs). match thermal_api.update_dynamic_input(i, m.model) { Ok(()) | Err(ThermalError::NotInAutoMode) => (), - Err(e) => ringbuf_entry!(Trace::ThermalError(i, e)), + Err(err) => { + ringbuf_entry!(Trace::ThermalError { port, err }) + } } } @@ -576,7 +602,7 @@ impl ServerImpl { } Err(e) => { // Log error to ringbuf - e.ringbuf(i); + e.ringbuf(port); // TODO(AJM): Old behavior here is a bit funky, we should // review this. @@ -630,7 +656,7 @@ impl ServerImpl { { let err = f(self.transceivers(), mask).error(); if !err.is_empty() { - ringbuf_entry!(Trace::DisableFailed(step, err)); + ringbuf_entry!(Trace::DisableFailed { step, mask: err }); } } self.disabled |= mask; @@ -757,21 +783,24 @@ impl From for TempReadError { } impl TempReadError { - fn ringbuf(&self, idx: usize) { + fn ringbuf(&self, port: LogicalPort) { let trace = match self { // We don't ringbuf for this, should never happen Self::UnknownInterface => return, Self::ReallyBadTempRead(e) | Self::BadTempRead(e) => { - Trace::TemperatureReadError(idx, *e) + Trace::TemperatureReadError { port, err: *e } } - Self::BadPortStatus(r) => Trace::InvalidPortStatusError(idx, *r), - Self::UnexpectedFpgaErr(e) => { - Trace::TemperatureReadUnexpectedError(idx, *e) + Self::BadPortStatus(r) => { + Trace::InvalidPortStatusError { port, raw_err: *r } } - Self::UnexpectedVariance(diff) => { - Trace::TemperatureGlitch(idx, (*diff).into()) + Self::UnexpectedFpgaErr(e) => { + Trace::TemperatureReadUnexpectedError { port, err: *e } } + Self::UnexpectedVariance(diff) => Trace::TemperatureGlitch { + port, + variance: Celsius(*diff), + }, }; ringbuf_entry!(trace); } From 3adcb7a0f0b7c8589dd58d77216fbc963963b9e5 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 2 Sep 2026 19:06:52 +0200 Subject: [PATCH 06/10] Fix some review comments --- drv/transceivers-server/src/main.rs | 33 +++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 41d77b4f7f..3f1e0b77f5 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -87,6 +87,10 @@ enum Trace { port: LogicalPort, err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, }, + PotentialRemovalError { + port: LogicalPort, + err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, + }, TemperatureReadUnexpectedError { port: LogicalPort, err: FpgaError, @@ -161,7 +165,7 @@ enum FrontIOStatus { Ready, } -struct PortMetadata { +struct PortData { /// Number of consecutive NACKS seen on a given port consecutive_errors: u8, /// Largest observed temperature drift between two samples @@ -174,7 +178,7 @@ struct PortMetadata { model: Option, } -impl PortMetadata { +impl PortData { /// New metadata with no model and zeroed counters/stats const fn new() -> Self { Self { @@ -217,7 +221,7 @@ struct ServerImpl { /// Modules that are physically present but disabled by Hubris disabled: LogicalPortMask, - ports: [PortMetadata; NUM_PORTS as usize], + ports: [PortData; NUM_PORTS as usize], /// Handle to write thermal models and presence to the `thermal` task #[cfg(feature = "thermal-control")] @@ -612,7 +616,7 @@ impl ServerImpl { // because we only add models to known interface types TempReadError::UnknownInterface => {} // This would *increment* consecutive errors - TempReadError::ReallyBadTempRead(_) => { + TempReadError::PotentialRemoval(_) => { meta.consecutive_errors = meta.consecutive_errors.saturating_add(1); } @@ -722,17 +726,18 @@ enum TempReadError { /// We failed to read, and the FPGA reported an error code that makes us /// suspicious that the xcvr is not present or operating correctly in a /// way that might lead us to disable it. - ReallyBadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), + PotentialRemoval(Reg::QSFP::PORT0_STATUS::ErrorEncoded), /// We failed to read, but in some kind of forgivable way that *doesn't* /// make us suspect that the xcvr should be disabled. BadTempRead(Reg::QSFP::PORT0_STATUS::ErrorEncoded), - /// The FPGA gave us back a status codethat we were unable to decode. This + /// The FPGA gave us back a status code that we were unable to decode. This /// is probably a pretty serious mismatch in FPGA and SP versioning. BadPortStatus(u8), /// The FPGA gave us back an FpgaError that we didn't expect at all when /// reading I2C. UnexpectedFpgaErr(FpgaError), - /// We read the sensor multiple times, and the + /// We read the sensor multiple times, and the difference between samples + /// exceeded a reasonable threshold. UnexpectedVariance(f32), } @@ -757,7 +762,7 @@ impl From for TempReadError { return Self::BadPortStatus(code); }; - let is_read_err = match decoded { + let is_removal = match decoded { // We consider these errors as potentially worth invalidating the // QSFP over. ErrorEncoded::I2CAddressNack => true, @@ -774,8 +779,8 @@ impl From for TempReadError { ErrorEncoded::I2CTransactionTimeout => false, }; - if is_read_err { - Self::ReallyBadTempRead(decoded) + if is_removal { + Self::PotentialRemoval(decoded) } else { Self::BadTempRead(decoded) } @@ -787,8 +792,10 @@ impl TempReadError { let trace = match self { // We don't ringbuf for this, should never happen Self::UnknownInterface => return, - - Self::ReallyBadTempRead(e) | Self::BadTempRead(e) => { + Self::PotentialRemoval(e) => { + Trace::PotentialRemovalError { port, err: *e } + } + Self::BadTempRead(e) => { Trace::TemperatureReadError { port, err: *e } } Self::BadPortStatus(r) => { @@ -902,7 +909,7 @@ fn main() -> ! { #[cfg(feature = "thermal-control")] thermal_api, sensor_api, - ports: [const { PortMetadata::new() }; NUM_PORTS as usize], + ports: [const { PortData::new() }; NUM_PORTS as usize], }); // There are two timers, one for each communication bus: From d1e08661ff2aeb9f0c779275de195d72e02092e2 Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 3 Sep 2026 11:59:48 +0200 Subject: [PATCH 07/10] Add ringbuf specifically for glitches --- drv/transceivers-server/src/main.rs | 73 ++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 3f1e0b77f5..6b414d9f3a 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -5,7 +5,10 @@ #![no_std] #![no_main] -use core::mem::MaybeUninit; +use core::{ + mem::MaybeUninit, + sync::atomic::{AtomicU32, Ordering}, +}; use counters::Count; use idol_runtime::{NotificationHandler, RequestError}; @@ -126,6 +129,65 @@ enum Trace { counted_ringbuf!(Trace, 16, Trace::None); +#[derive(PartialEq, Debug, Clone, Copy)] +struct TempGlitch { + index: u8, + first: Celsius, + second: Celsius, +} + +impl TempGlitch { + /// Okay so ringbuf requires an initializer, and this value is actually + /// somewhat obnoxious because it *is* a plausible value to observe. + /// However, "all zeroes" lets us put the initializer in .bss and not .data, + /// and since this is just for debugging, we hope that a reasonable person + /// would realize that the delta between the two entries is zero. I really + /// didn't feel like making this an Option, or using u8::MAX as the index, + /// or even adding 1 to the port and then using NonZero or something. + const RINGBUF_INIT: Self = Self { + index: 0, + first: Celsius(0.0), + second: Celsius(0.0), + }; +} + +/// Keep counters for how often each port has experienced temperature glitches. +/// +/// This *does not* reset when ports are disabled or qsfp xcvrs are removed or +/// re-added. +impl Count for TempGlitch { + type Counters = [AtomicU32; NUM_PORTS as usize]; + + #[allow(clippy::declare_interior_mutable_const)] + const NEW_COUNTERS: Self::Counters = + [const { AtomicU32::new(0) }; NUM_PORTS as usize]; + + fn count(&self, counters: &Self::Counters) { + // This should never happen, but just in case. + let Some(ctr) = counters.get(self.index as usize) else { + return; + }; + ctr.fetch_add(1, Ordering::Relaxed); + } +} + +// Keep counters each time the delta between two samples exceeds the value +// of `MAX_RESAMPLE_VARIANCE_CELSIUS`. This also counts how often it happens +// on a per-port basis, as well as the last 32 instances it has been observed. +// +// This augments the per-port info kept in [`PortData`]. +// +// We hope to observe the reported values differing in a way that can be +// explained by data bits being flipped or shifted, which would be consistent +// with "I2C weather" as opposed to a complete hiccup of the transceiver +// itself. +counted_ringbuf!( + TEMP_GLITCH_RINGBUF, + TempGlitch, + 32, + TempGlitch::RINGBUF_INIT +); + //////////////////////////////////////////////////////////////////////////////// /// After seeing this many NACKs or timeouts, we disable the port by policy. @@ -460,6 +522,15 @@ impl XcvrApi { // It has probably been milliseconds, we don't expect the temperature // to change significantly in this time. if diff >= MAX_RESAMPLE_VARIANCE_CELSIUS { + // Record glitches specifically in the ringbuf + ringbuf_entry!( + TEMP_GLITCH_RINGBUF, + TempGlitch { + index: port.0, + first: a, + second: b, + } + ); Err(TempReadError::UnexpectedVariance(diff)) } else { Ok((a, diff)) From 0c080db0fd8ef06026235ce9a92c87538cb30b4c Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 3 Sep 2026 12:19:20 +0200 Subject: [PATCH 08/10] Count differently --- drv/front-io-api/src/transceivers.rs | 21 +++++++++++++++ drv/transceivers-server/src/main.rs | 40 +++++++++++++++++----------- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/drv/front-io-api/src/transceivers.rs b/drv/front-io-api/src/transceivers.rs index 3724140cb9..ff29c50ff3 100644 --- a/drv/front-io-api/src/transceivers.rs +++ b/drv/front-io-api/src/transceivers.rs @@ -2,9 +2,12 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use core::sync::atomic::{AtomicU32, Ordering}; + use crate::{Addr, FrontIOError, Reg}; use drv_fpga_api::{FpgaError, FpgaUserDesign, ReadOp, WriteOp}; use drv_transceivers_api::{ModuleStatus, NUM_PORTS}; +use ringbuf::Count; use transceiver_messages::ModuleId; use userlib::UnwrapLite; use zerocopy::{ @@ -134,6 +137,24 @@ impl LogicalPort { PortLocation::from(*self) } } + +/// Implement the ringbuf trait on LogicalPort to allow for per-port metrics +impl Count for LogicalPort { + type Counters = [AtomicU32; NUM_PORTS as usize]; + + #[allow(clippy::declare_interior_mutable_const)] + const NEW_COUNTERS: Self::Counters = + [const { AtomicU32::new(0) }; NUM_PORTS as usize]; + + fn count(&self, counters: &Self::Counters) { + // This should never happen, but just in case. + let Some(ctr) = counters.get(self.0 as usize) else { + return; + }; + ctr.fetch_add(1, Ordering::Relaxed); + } +} + /// Represents a set of selected logical ports, i.e. a 32-bit bitmask #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] pub struct LogicalPortMask(pub u32); diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index 6b414d9f3a..b38f97a2a3 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -5,10 +5,7 @@ #![no_std] #![no_main] -use core::{ - mem::MaybeUninit, - sync::atomic::{AtomicU32, Ordering}, -}; +use core::mem::MaybeUninit; use counters::Count; use idol_runtime::{NotificationHandler, RequestError}; @@ -81,36 +78,45 @@ enum Trace { GotInterface(u8, ManagementInterface), UnknownInterface(u8, ManagementInterface), UnpluggedModule { + #[count(children)] port: LogicalPort, }, RemovedDisabledModuleThermalModel { + #[count(children)] port: LogicalPort, }, TemperatureReadError { + #[count(children)] port: LogicalPort, err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, }, PotentialRemovalError { + #[count(children)] port: LogicalPort, err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, }, TemperatureReadUnexpectedError { + #[count(children)] port: LogicalPort, err: FpgaError, }, ThermalError { + #[count(children)] port: LogicalPort, err: ThermalError, }, GetInterfaceError { + #[count(children)] port: LogicalPort, err: Reg::QSFP::PORT0_STATUS::ErrorEncoded, }, GetInterfaceUnexpectedError { + #[count(children)] port: LogicalPort, err: FpgaError, }, InvalidPortStatusError { + #[count(children)] port: LogicalPort, raw_err: u8, }, @@ -122,6 +128,7 @@ enum Trace { ClearDisabledPorts(LogicalPortMask), SeqError(SeqError), TemperatureGlitch { + #[count(children)] port: LogicalPort, variance: Celsius, }, @@ -131,7 +138,7 @@ counted_ringbuf!(Trace, 16, Trace::None); #[derive(PartialEq, Debug, Clone, Copy)] struct TempGlitch { - index: u8, + index: LogicalPort, first: Celsius, second: Celsius, } @@ -145,7 +152,7 @@ impl TempGlitch { /// didn't feel like making this an Option, or using u8::MAX as the index, /// or even adding 1 to the port and then using NonZero or something. const RINGBUF_INIT: Self = Self { - index: 0, + index: LogicalPort(0), first: Celsius(0.0), second: Celsius(0.0), }; @@ -154,20 +161,16 @@ impl TempGlitch { /// Keep counters for how often each port has experienced temperature glitches. /// /// This *does not* reset when ports are disabled or qsfp xcvrs are removed or -/// re-added. +/// re-added. We just pass-through to the existing LogicalPort impl of Count. impl Count for TempGlitch { - type Counters = [AtomicU32; NUM_PORTS as usize]; + type Counters = ::Counters; #[allow(clippy::declare_interior_mutable_const)] - const NEW_COUNTERS: Self::Counters = - [const { AtomicU32::new(0) }; NUM_PORTS as usize]; + const NEW_COUNTERS: Self::Counters = ::NEW_COUNTERS; + #[inline] fn count(&self, counters: &Self::Counters) { - // This should never happen, but just in case. - let Some(ctr) = counters.get(self.index as usize) else { - return; - }; - ctr.fetch_add(1, Ordering::Relaxed); + self.index.count(counters); } } @@ -234,6 +237,9 @@ struct PortData { peak_diff: f32, /// Number of times the temperature has been discarded, saturates discarded_temps: u32, + /// Number of times the temperature has been samples. This counts resample + /// times, not individul i2c queries + total_temp_samples: u32, /// Thermal models are populated by the host // TODO(AJM): Is the above comment true? We actually fill this in with a // basic model for each, and I don't *think* there's a UDP api to set this? @@ -247,6 +253,7 @@ impl PortData { consecutive_errors: 0, peak_diff: 0.0, discarded_temps: 0, + total_temp_samples: 0, model: None, } } @@ -526,7 +533,7 @@ impl XcvrApi { ringbuf_entry!( TEMP_GLITCH_RINGBUF, TempGlitch { - index: port.0, + index: port, first: a, second: b, } @@ -666,6 +673,7 @@ impl ServerImpl { // Sample the transceiver temperature multiple times, seeing if // we are successful and the samples are steady enough to report. let res = xcvr_api.get_temperature_resample(port, m); + meta.total_temp_samples = meta.total_temp_samples.saturating_add(1); match res { Ok((reading, diff)) => { sensor_api.post_now( From ff00d9736fc8ee0361ac36adc80de0b3799d51f6 Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 3 Sep 2026 13:08:57 +0200 Subject: [PATCH 09/10] Address some comments from Aaron --- drv/transceivers-server/src/main.rs | 41 +++++++++++++---------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index b38f97a2a3..e3bb30ee51 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -138,7 +138,7 @@ counted_ringbuf!(Trace, 16, Trace::None); #[derive(PartialEq, Debug, Clone, Copy)] struct TempGlitch { - index: LogicalPort, + port: LogicalPort, first: Celsius, second: Celsius, } @@ -152,7 +152,7 @@ impl TempGlitch { /// didn't feel like making this an Option, or using u8::MAX as the index, /// or even adding 1 to the port and then using NonZero or something. const RINGBUF_INIT: Self = Self { - index: LogicalPort(0), + port: LogicalPort(0), first: Celsius(0.0), second: Celsius(0.0), }; @@ -170,7 +170,7 @@ impl Count for TempGlitch { #[inline] fn count(&self, counters: &Self::Counters) { - self.index.count(counters); + self.port.count(counters); } } @@ -240,9 +240,10 @@ struct PortData { /// Number of times the temperature has been samples. This counts resample /// times, not individul i2c queries total_temp_samples: u32, - /// Thermal models are populated by the host - // TODO(AJM): Is the above comment true? We actually fill this in with a - // basic model for each, and I don't *think* there's a UDP api to set this? + /// Thermal models for the given transceiver. Currently this is fixed to + /// a single basic model. + /// + /// See https://github.com/oxidecomputer/hubris/issues/2670. model: Option, } @@ -496,6 +497,8 @@ impl XcvrApi { ManagementInterface::Sff8636 | ManagementInterface::Cmis => { ringbuf_entry!(Trace::GotInterface(p.0, interface)); // TODO: this is made up + // + // See: https://github.com/oxidecomputer/hubris/issues/2670 Some(ThermalModel { interface, model: ThermalProperties { @@ -533,7 +536,7 @@ impl XcvrApi { ringbuf_entry!( TEMP_GLITCH_RINGBUF, TempGlitch { - index: port, + port: port, first: a, second: b, } @@ -687,25 +690,18 @@ impl ServerImpl { // Log error to ringbuf e.ringbuf(port); - // TODO(AJM): Old behavior here is a bit funky, we should - // review this. match e { - // Previously, this did a "continue", and didn't affect - // consecutive errors at all. This should never happen - // because we only add models to known interface types - TempReadError::UnknownInterface => {} + // Failure to read neither increments nor resets the + // counter of errors + TempReadError::UnknownInterface + | TempReadError::BadTempRead(_) + | TempReadError::BadPortStatus(_) + | TempReadError::UnexpectedFpgaErr(_) => {} // This would *increment* consecutive errors TempReadError::PotentialRemoval(_) => { meta.consecutive_errors = meta.consecutive_errors.saturating_add(1); } - // All of these *actually reset* the error count. Do - // we want this? - TempReadError::BadTempRead(_) - | TempReadError::BadPortStatus(_) - | TempReadError::UnexpectedFpgaErr(_) => { - meta.consecutive_errors = 0; - } // We probably don't want to count this against the // device, since it could have been a momentary I2C // glitch. @@ -847,8 +843,9 @@ impl From for TempReadError { ErrorEncoded::I2CAddressNack => true, ErrorEncoded::I2CSclStretchTimeout => true, - // TODO(AJM): why *don't* we consider these errors as worth - // potentially invalidating the QSFP? + // We expect these are transient errors, and will likely be resolved + // in short order, either by waiting to detect the QSFP device has + // been removed, or obtaining some other kind of error. ErrorEncoded::NoError => false, ErrorEncoded::NoModule => false, ErrorEncoded::NoPower => false, From 7b824bd82b3cde727fd264f5842873761f3d48fd Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 3 Sep 2026 13:16:01 +0200 Subject: [PATCH 10/10] Sorry, clippy --- drv/transceivers-server/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drv/transceivers-server/src/main.rs b/drv/transceivers-server/src/main.rs index e3bb30ee51..12f220b99f 100644 --- a/drv/transceivers-server/src/main.rs +++ b/drv/transceivers-server/src/main.rs @@ -536,7 +536,7 @@ impl XcvrApi { ringbuf_entry!( TEMP_GLITCH_RINGBUF, TempGlitch { - port: port, + port, first: a, second: b, }