transceivers: Implement double-polling for transceiver temperatures - #2668
Conversation
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
left a comment
jamesmunns
left a comment
There was a problem hiding this comment.
Added some commentary.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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
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.
| /// 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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).
|
Noting that this is still probably a thing we want to land, it's just lower prio than #2678 right now. |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
This looks good to me overall! I had some smallish docs and naming nits.
| /// Like [`Self::new()`] but with the given model. | ||
| fn init(&mut self, model: Option<ThermalModel>) { |
There was a problem hiding this comment.
unimportant nit, feel free to ignore: hm, this isn't really "like Self::new()", because it initializes self in place...
| } | ||
|
|
||
| /// Error while reading temperature from QSFP transceiver | ||
| enum TempReadError { |
There was a problem hiding this comment.
i feel like some of these, in the fullness of time, should probably become ereports. not necessarily in this PR, though.
Co-authored-by: Eliza Weisman <eliza@elizas.website>
5 participants
CC #2664 (though probably not a permanent solution).
This PR:
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.