transceivers: Implement double-polling for transceiver temperatures by jamesmunns · Pull Request #2668 · oxidecomputer/hubris

jamesmunns · 2026-09-02T11:48:16Z

CC #2664 (though probably not a permanent solution).

This PR:

Makes the transceiver server a static, so we can pull it from dumps
Now polls each transceiver twice, discarding samples that are >= 5.0C apart
Keeps stats (in the server) about each enabled port, including max sample variance and number of discarded samples

The hope for this PR is to get more trackable information and maybe catch a misbehaving I2C poll in action.

We may want to also add an ereport for this, I'd like to leave that for a follow-on PR though.

This is also currently based on #2667, we should merge that first.

This did require some refactoring of the transceiver server code, I tried to keep the diffs pretty stable and the commits pretty useful so far, if that makes things easier to review. I'll flag a couple of "wuts" I saw along the way.

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`.

jamesmunns

Added some commentary.

hawkw · 2026-09-02T16:17:38Z

    DisableFailed(usize, LogicalPortMask),
    ClearDisabledPorts(LogicalPortMask),
    SeqError(SeqError),
+    TemperatureGlitch(usize, MilliCelsiusFixed),


I was going to ask what the usize means here (I am guessing it's the port and not a timestamp?) and propose we made it a named field so its meaning would be obvious when looking at the ringbuf. But, I see that that there are a bunch of similarly shaped ringbuf entries here already. I'd kinda like to see them all become named fields, but 🤷‍♀️

Incidentally, it occurs to me --- and this is probably best saved for another PR --- it would be nice if we used an enum of port numbers rather than usize here, so that the ringbuf events could add #[count(children)] for the port. The counters crate doesn't support #[count(children)] on integer fields since well, we would have to generate a table of usize::MAX counters (or dynamically allocate them which kind of defeats the purpose!)

@hawkw I think I did this in 0c080db, since I wanted 90% of this for Ben's request as well.

hawkw · 2026-09-02T16:30:41Z

    DisableFailed(usize, LogicalPortMask),
    ClearDisabledPorts(LogicalPortMask),
    SeqError(SeqError),
+    TemperatureGlitch(usize, MilliCelsiusFixed),


Incidentally, it occurs to me --- and this is probably best saved for another PR --- it would be nice if we used an enum of port numbers rather than usize here, so that the ringbuf events could add #[count(children)] for the port. The counters crate doesn't support #[count(children)] on integer fields since well, we would have to generate a table of usize::MAX counters (or dynamically allocate them which kind of defeats the purpose!)

hawkw · 2026-09-04T21:19:57Z

    }
 }
+
+/// Implement the ringbuf trait on LogicalPort to allow for per-port metrics


counters trait?

hawkw · 2026-09-04T21:28:48Z

+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);
+    }
+}


this is cute; I wonder if we might want the counters crate to have a macro or something for saying "yes, this looks like a u32, but it will always be in some range, so you can derive counters for it without having to worry"; elsewhere, I've tended to just use repr(N) enums for this sort of thing, like this thing:

hubris/drv/psc-seq-server/src/main.rs

Lines 390 to 403 in 0d1ba04

/// PSU numbers represented as an enum. This is intended for use with

/// `counted_ringbuf!`, instead of representing PSU numbers as raw u8s, which

/// cannot derive `counters::Count` (and would have to generate a counter table

/// with 256 entries rather than just 6).

#[derive(Copy, Clone, Eq, PartialEq, counters::Count)]

#[repr(u8)]

enum Slot {

Psu0 = 0,

Psu1 = 1,

Psu2 = 2,

Psu3 = 3,

Psu4 = 4,

Psu5 = 5,

}

but that then requires some weirdish boilerplate for converting between the enum and integers if you also want to index arrays or whatever.

very much not a blocker for this PR, but I wonder if we might throw together a little newtype-integer-counter derive or something that works like this.

hawkw · 2026-09-04T21:36:37Z

+/// 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. We just pass-through to the existing LogicalPort impl of Count.
+impl Count for TempGlitch {
+    type Counters = <LogicalPort as Count>::Counters;
+
+    #[allow(clippy::declare_interior_mutable_const)]
+    const NEW_COUNTERS: Self::Counters = <LogicalPort as Count>::NEW_COUNTERS;
+
+    #[inline]
+    fn count(&self, counters: &Self::Counters) {
+        self.port.count(counters);
+    }
+}


Oh, huh, I think this is necessary because the counters crate doesn't support deriving Count for a struct, even if it has count(Children) on a field that implements Count? It could be worth adding that to better support cases like this (not in this PR, I'll go make a ticket).

hawkw · 2026-09-04T21:40:26Z

+    /// 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.


Yeah, I agree that this is pretty reasonable; the offset one where we use NonZero, in particular, actually sounds like kind of a bad idea, because the primary consumer of ringbufs, humility, does not have any idea that we have done such an offsetting and will end up just lying to the reader about which port it is, unless they went and read the code to learn about this.

With that said, I'm not actually sure if making it an Option and initializing the ringbuf to None is actually all that bad, since we don't actually really work with the entries all that meaningfully? It might make things a bit less confusing for a reader...

hawkw · 2026-09-04T21:42:44Z

+struct TempGlitch {
+    port: LogicalPort,
+    first: Celsius,
+    second: Celsius,
+}


i wonder if we might also want to include a timestamp in these, so that a reader can get some sense of how frequently they occur/whether they are clustered temporally or not?

hawkw · 2026-09-04T21:47:42Z

+    fn get_temperature_once(
+        &self,
+        port: LogicalPort,
+        m: &ThermalModel,
+    ) -> Result<Celsius, TempReadError> {
+        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)
+    }


i think the layering of get_temperature_resample -> get_temperature_once is conceptually reasonable, but it is a bit silly that we end up matching on m.interface twice every time we read temps from a thing. the performance hit from that is probably not actually important but it feels silly. probably not worth messing with.

hawkw · 2026-09-04T21:48:27Z

        // 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() {


bit odd that we renamed this to PortData but the variable is still meta...

jamesmunns added 4 commits September 2, 2026 12:01

Implement double-sampling of transceiver temps

f08e458

Make the transceivers server a ClaimOnceCell so we can pull it in dumps

439e9b0

Implement metadata and stats

755934d

jamesmunns requested review from Aaron-Hartwig and hawkw September 2, 2026 11:48

jamesmunns commented Sep 2, 2026

View reviewed changes

labbott reviewed Sep 2, 2026

View reviewed changes

Comment thread drv/transceivers-server/src/main.rs Outdated

hawkw reviewed Sep 2, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into james/xcvr-doubletap

8618c34

hawkw reviewed Sep 2, 2026

View reviewed changes

jamesmunns added 2 commits September 2, 2026 18:57

Better ringbuf, no Milli-celsius

afaeaa4

Fix some review comments

3adcb7a

bnaecker reviewed Sep 2, 2026

View reviewed changes

Comment thread drv/transceivers-server/src/main.rs

bnaecker reviewed Sep 2, 2026

View reviewed changes

Comment thread drv/transceivers-server/src/main.rs

jamesmunns added 4 commits September 3, 2026 11:59

Add ringbuf specifically for glitches

d1e0866

Count differently

0c080db

Address some comments from Aaron

ff00d97

Sorry, clippy

7b824bd

jamesmunns requested review from hawkw and labbott September 3, 2026 13:41

hawkw reviewed Sep 4, 2026

View reviewed changes

This was referenced Sep 4, 2026

want to derive counters::Count for structs when they have count(children)able enum fields #2674

Open

want a way to have counters derive counters for newtypes around integers with "reasonable" max values #2675

Open

	/// PSU numbers represented as an enum. This is intended for use with
	/// `counted_ringbuf!`, instead of representing PSU numbers as raw u8s, which
	/// cannot derive `counters::Count` (and would have to generate a counter table
	/// with 256 entries rather than just 6).
	#[derive(Copy, Clone, Eq, PartialEq, counters::Count)]
	#[repr(u8)]
	enum Slot {
	Psu0 = 0,
	Psu1 = 1,
	Psu2 = 2,
	Psu3 = 3,
	Psu4 = 4,
	Psu5 = 5,
	}

Uh oh!

Conversation

jamesmunns commented Sep 2, 2026

Uh oh!

jamesmunns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants