Refactor of the `thermal` task by jamesmunns · Pull Request #2609 · oxidecomputer/hubris

jamesmunns · 2026-07-23T20:32:06Z

This is a WIP refactor in service of simplifying the current state tracking performed by the thermal task.

Some of this is still Not Great, particularly the places where I pass 2-4 closures into a single function.

However, by pushing most state tracking down into the BSP, I hope to make it easier to Trait-ify the BSP interface and make it easier to build a "host native" thermal loop for testing purposes.

(most of the builds are failing because I've only refactored cosmo and none of the other bsps yet)

I kept getting these confused, and making it just very very verbose is I think the best way to make this clear.

labbott

I forgot to hit submit yesterday, I'll take another pass today

labbott · 2026-07-27T19:49:40Z

+    // When we first start up, try to initialize the fan controller a few
+    // times, in case there's a transient I2C error.


Is this still the case? I see this was originally added for grapefruit as part of 725bdfdd8b01de8e095199ee3f0ee218ffe71414

I'm not sure! I haven't actually tested this on hardware yet, and tried as best as possible to preserve the existing behavior, whatever it was.

Also worth noting, thermal is only active for grapefruit-ruby, and I'm not sure where to even obtain one of those for testing.

labbott · 2026-07-27T19:58:16Z

+                // This is mimicing the old state value logic for deciding if
+                // we persist the data in `run_control`, that ONLY cleared the
+                // persisted value if:
+                //
+                // - The sensor is not present AND removable
+                // - The sensor is error prone
+                //
+                // Replicate that logic here, doing some type shenanigans
+                // because we aren't round-tripping through the Sensor API
+                // anymore.
+                let se = SensorError::from(NoData::from(e));
+                match (self.metadata.ty, se) {
+                    (ChannelType::Removable, SensorError::NotPresent) => {
+                        self.last_reading = TemperatureReading::Inactive;
+                    }
+                    (ChannelType::RemovableAndErrorProne, _) => {
+                        self.last_reading = TemperatureReading::Inactive;
+                    }
+                    _ => {
+                        // In all other cases, just leave whatever the last
+                        // present value was so that the state estimation
+                        // can continue estimating state.
+                    }
+                }
+
+                // This logic comes from what was done in `read_sensors`,
+                // which is only deciding whether it's worth logging about.
+                // In either case, it will push NoData to the sensor api.
+                //
+                // This is *not* the same logic that is used above to decide
+                // whether we clear the previous state or not, despite being
+                // *very* similar!
+                let removable = matches!(
+                    self.metadata.ty,
+                    ChannelType::Removable
+                        | ChannelType::RemovableAndErrorProne
+                );
+                let removed =
+                    e == SensorReadError::I2cError(ResponseCode::NoDevice);
+                let unexpected_failure = !(removable && removed);
+                if unexpected_failure {
+                    InputReadingOutcome::UnacceptableMissing { id, err: e }
+                } else {
+                    InputReadingOutcome::AcceptableMissing { id, err: e }
+                }


The comments are very good but I still find this tricky to follow. I don't have any great ideas here (might take a pass later)

I think this could be simplified IMO, this is a little awkward in service of preserving the old behavior.

jamesmunns · 2026-07-28T13:02:41Z

@labbott if you're making another pass, 5c803a7 added some trait docs as well as some overall concepts, which might be useful to read first!

jamesmunns · 2026-07-28T13:28:30Z

Taking this out of draft/WIP state, and marking it as ready for review. Builds are now passing for all targets, and I think this change is Good Actually, even if a bit large.

It definitely need on-target testing still, but I'll need to coordinate that. It would be good to make sure there aren't any major review items to address first.

jamesmunns · 2026-07-28T18:06:25Z

Smoke testing on Hubris seems to work, the system is unloaded so there's not a lot of heat to react to, but the current software handles the transitions from A2 -> A0 and A0 -> A2 as I expect, and ControlPwm is going up:

humility: ring buffer task_thermal::__RINGBUF in thermal:
   TOTAL VARIANT
     632 ControlPwm
       4 AutoState(Boot)
       3 AutoState(Running)
       6 FanAdded
       3 PowerModeChanged
       1 Start
       1 ThermalMode(Auto)
       1 SensorReadFailed
       1 FanControllerInitialized
       1 SetFanWatchdogOk
 NDX LINE      GEN    COUNT PAYLOAD
   0  393        1        1 Start
   1   72        1        1 FanControllerInitialized
   2  198        1        1 ThermalMode(Auto)
   3  676        1        1 AutoState(Boot)
   4  692        1        1 SetFanWatchdogOk
   5  704        1        1 FanAdded(0x0)
   6  704        1        1 FanAdded(0x1)
   7  704        1        1 FanAdded(0x2)
   8  704        1        1 FanAdded(0x3)
   9  704        1        1 FanAdded(0x4)
  10  704        1        1 FanAdded(0x5)
  11  808        1        1 PowerModeChanged(PowerBitmask(0b1))
  12  676        1        1 AutoState(Boot)
  13 1000        1        1 AutoState(Running)
  14  967        1      322 ControlPwm(0x0)
  15  775        1        1 SensorReadFailed(SensorId(0x37), I2cError(NoDevice))
  16  808        1        1 PowerModeChanged(PowerBitmask(0b10))
  17  676        1        1 AutoState(Boot)
  18  967        1        1 ControlPwm(0x64)
  19 1000        1        1 AutoState(Running)
  20  967        1      303 ControlPwm(0x0)
  21  808        1        1 PowerModeChanged(PowerBitmask(0b1))
  22  676        1        1 AutoState(Boot)
  23 1000        1        1 AutoState(Running)
  24  967        1        6 ControlPwm(0x0)

labbott

This is a big diff but I do think it's an improvement. I think keeping the existing behavior is the correct approach. I would also like to capture areas that are particularly weird and you think should be improved (maybe a single issue with some links code lines?)

labbott · 2026-07-29T14:40:48Z

+        if idx >= MAX_FANS {
+            panic!("Out of range!");


I don't love the extra potential runtime panic but it also didn't seem to increase the code size so 🤷

This is a const-fn, and is only currently used to generate constants, which means that any panics that occur would be compile-time errors.

You are right, if someone inadvertently calls it at runtime, it would potentially panic in that case.

jamesmunns · 2026-07-29T16:00:48Z

Capturing a list of "some things that are 'eh' about the current state, and would like to fix later, but didn't because I didn't want to churn the diff even more":

All the `dead_code` annotations

Right now, we don't use proper features for a bunch of things, like fans, fan controller types, etc., and some things are only used in some bsps.

I think this would be fixed by either:

Splitting thermal-core out into a crate with pub items, making the dead-code no longer relevant
Adding proper feature flagging to thermal

The `#[path]` module stuff for `bsps/common`

https://github.com/oxidecomputer/hubris/pull/2609/changes#diff-a9b9fa16760e6bde87da1614252f333f4eef08e39dba9c5e1697fc1fc3e65f05R20-R29

This is awkward. It should probably be feature-flagged or somewhere that multiple BSPs can use it. For now it's an awkward pseudo thing.

tbf the thermal task was already doing this for bsps. I think that need to be fixed too, same options as above

Be okay with some changes to ringbufs

Some of the trait API surface was made pretty much exclusively to keep us able to do ringbuf logging in the same/place way as we currently do. A lot of this is "log an event when updating status/presence". We might decide it is okay to ringbuf this differently, or let some of that information live in the bsps (or the state types like TemperatureInputs/Fans) themselves).

This also includes things like this.

That being said, where we ringbuf today, we probably want to ereport tomorrow, so that might inform what is/isn't reasonable.

Revisit how we do state transitions

https://github.com/oxidecomputer/hubris/pull/2609/changes#diff-f4eab2a6583ffdb099d7d21aff862a465dc6db83665b99618329f357f8a1cad2R891-R893

This could be a bit simpler I think, where we check for the "deviant" states in descending importance, and go directly there vs matching on the current state.

This isn't that big of a deal though, might just dedupe some repeated code.

Duplication between emc2305 and max31790

These are very similar, there might be a way to dedupe these better with a trait or something, and have the bsps use that. Idk, I ran out of time, but it's also not hugely problematic IMO.

Duplication/verbosity across bsps

They are currently a little wordy, with a couple of impls being almost the same. Again, with some more time we might find a better to "compose" the common parts, but the total volume of bsps is pretty low, and it seemed like the current amount of duplication was within the threshold of acceptable.

Things that still need to be done to make this portable

I need to do something with SeqError and PowerBitmask, these are bsp-specific types, and they probably need to be surfaced as associated types on the trait.
I probably need to make the sensor-api stuff and maybe ringbufs a separate trait so we can replace them in hosted configurations.
Actually pull out most of control.rs into a thermal-core crate, use that from the thermal task

Once we do all that, I think we will be at a place where the logic of thermal is largely testable!

jamesmunns · 2026-07-29T16:01:48Z

I would also like to capture areas that are particularly weird and you think should be improved (maybe a single issue with some links code lines?)

Wrote up #2609 (comment) for now, I can turn that into an issue (or issues) with correct line links once this merges!

jamesmunns · 2026-07-29T17:53:57Z

@labbott addressed most of the comments so far!

hawkw

I haven't fully read through this change, but I wanted to leave the notes I have so far. Overall, I like the refactor, but have some nitpicks.

hawkw · 2026-07-29T17:57:04Z

+use task_thermal_api::{SensorReadError, ThermalError};
+
+use crate::{
+    __RINGBUF, Trace,


FWIW, the ringbuf crate also has a ringbuf_entry_root! macro which is intended to allow submodules to add entries to ringbufs declared in the crate root without having to import __RINGBUF. Not a big deal, as what you're doing also works fine in this case (as there is no additional ringbuf declared in this module, which ringbuf_entry_root! helps with). I just figured I'd mention it.

I've also seen code use ringbuf::ringbuf_entry_root as ringbuf_entry; in cases where a module doesn't have its own ringbuf. I dunno whether or not I love that pattern, but it's an existing convention... 🤷‍♀️

hawkw · 2026-07-29T18:00:31Z

+#[allow(dead_code, clippy::upper_case_acronyms)]
+pub enum Device {
+    Tmp117,
+    Tmp451(drv_i2c_devices::tmp451::Target),
+    CPU,
+    Dimm,
+    U2,
+    M2,
+    LM75,
+}


turbo nitpick: I think the two "upper case acronyms" clippy doesn't like here are LM75 and CPU; what do you think about just doing this instead:

Suggested change

#[allow(dead_code, clippy::upper_case_acronyms)]

pub enum Device {

Tmp117,

Tmp451(drv_i2c_devices::tmp451::Target),

CPU,

Dimm,

U2,

M2,

LM75,

}

#[allow(dead_code)]

pub enum Device {

Tmp117,

Tmp451(drv_i2c_devices::tmp451::Target),

Cpu,

Dimm,

U2,

M2,

Lm75,

}

I note that every other acronym in this enum (TMP117, TMP451, DIMM) are camel-cased...we could also fully embrace upper-case acronyms here, but if we're going to do that, I'd kinda like to see DIMM and CPU in the same case?)

also, take it or leave it, but I kind of wonder if we ought to change this so that it's the name of the actual I2C device we use for each thing (i.e. Tse2005Av instead of Dimm, Sbtsi instead of CPU, and NvmeBmc instead of U2 and M2)? i'm thinking that because this enum is really being used to determine which I2C device's driver to use, rather than which component's thermal model to use (AFAICT?), we might be better off making that a bit more explicit --- in this case, the U2 and M2 variants don't actually seem necessary, since they're both just NVME BMC devices, and I can kind of imagine some future product where DIMM and/or CPU temperatures are read through a different I2C device (though we're probably not going to release, say, an intel compute sled any time soon, so that's probably not worth optimizing for...)

ah, i see that this was copied directly from the old code, it just lived in a different file. in that case, feel free to ignore my nits for now, though it might be worth cleaning up later...

hawkw · 2026-07-29T18:05:38Z

+    /// Thermal properties of the associated component
+    model: ThermalProperties,
+
+    /// Mask with bits set based on the Bsp's `power_mode` bits


nitpick:

Suggested change

/// Mask with bits set based on the Bsp's `power_mode` bits

/// Mask with bits set based on the BSP's `power_mode` bits

hawkw · 2026-07-29T18:06:23Z

+/// InputChannelMetadata is the constant description portion of an InputChannel.
+///
+/// We split it off because InputChannel is mutable to contain the last state,
+/// and if we left it inlined, then we would end up including all of this
+/// metadata in RAM, despite never changing it! So instead, we break it off and
+/// have InputChannel hold an `&'static` reference instead, so we only waste
+/// a wee little pointer (4 bytes) to flash space in each InputChannel entry,
+/// instead of (at the time of writing) 36 bytes, which for example in Cosmo
+/// that has 14 input channels, is over 500 bytes of RAM!


style nit: this feels like it ought to be on the type declaration, rather than on the impl block?

hawkw · 2026-07-29T18:06:58Z

+/// instead of (at the time of writing) 36 bytes, which for example in Cosmo
+/// that has 14 input channels, is over 500 bytes of RAM!
+impl InputChannelMetadata {
+    pub const fn new(


style nit: the type is pub(crate) but the constructor is pub (i thought clippy didn't like this, but i guess it doesn't seem to care...)

Suggested change

pub const fn new(

pub(crate) const fn new(

hawkw · 2026-07-29T18:10:23Z

+        }
+    }
+
+    pub fn do_reading(


nit: IMO, it would be nice if there was a doc comment on this explaining its relationship with status() --- it looks like do_reading actually asks the temperature sensor for what temperature it is, but returns a thing that represents the result of the I2C request, and status() returns a thing representing the current temperature or its non-existence, but I'd like to see that written down.

hawkw · 2026-07-29T18:16:58Z

+    /// Sensor was not read because the power mode indicated that it would not
+    /// be enabled in this state.
+    Unpowered { id: SensorId },
+    /// This sensor was missing, but it's okay because it was either Removable
+    /// and not there, or Removable and Error Prone and had any kind of error.
+    AcceptableMissing { id: SensorId, err: SensorReadError },
+    /// Any read error that didn't match the "Acceptable" cases listed above.
+    UnacceptableMissing { id: SensorId, err: SensorReadError },
+    /// We read the data! Hooray!
+    Success {
+        id: SensorId,
+        now: u64,
+        value: Celsius,
+    },


this is the type of thing where normally I would say "huh, it seems kind of like it might be easier to use this if we pulled the SensorId out into a struct so that you didn't have to unpack it from each variant"...but after actually looking at the code, I see that it doesn't matter and it's probably simpler to just leave it this way. carry on.

hawkw · 2026-07-29T18:17:58Z

+pub enum FanStatus {
+    PresentSuccess {
+        rpm: Rpm,
+        sensor_id: SensorId,
+    },
+    PresentError {
+        error: SensorReadError,
+        sensor_id: SensorId,
+    },
+    #[allow(dead_code)] // Some bsps don't have removable fans
+    NotPresent {
+        sensor_id: SensorId,
+    },


very unimportant style nit: in InputReadingOutcome and InputStatus, the sensor ID field is called id; here, it's called sensor_id. would maybe be worth making it consistent everywhere

hawkw · 2026-07-29T18:18:36Z

+#[allow(dead_code)] // Not all bsps have dynamic inputs
 pub(crate) struct DynamicInputChannel {
-    model: ThermalProperties,
+    pub sensor_id: SensorId,


similarly, InputStatus calls this id, while DynamicInputChannel calls it id...maybe worth using the same name?

hawkw · 2026-07-29T18:53:18Z

+            model: ThermalProperties {
+                target_temperature: Celsius(0.),
+                critical_temperature: Celsius(0.),
+                power_down_temperature: Celsius(0.),
+                temperature_slew_deg_per_sec: 0.,
+            },


I get why we're representing the thermal model of a dynamic input which hasn't been added as a ThermalProperties where everything is 0, and it saves at least a byte of enum discriminant and probably some padding versus making this an Option. But, it really doesn't feel good. We are initializing this to an invalid state: an input channel that is at its target temperature, its critical temperature, and its power-down temperature, all at the same time. And we're just relying on other code respecting the has_been_queried flag to avoid making the system thermal shutdown immediately...which it does, but it feels sketchy to me.

If we're going to do this, I'd feel a bit less sketched out if we initialized these to NaN instead of 0. That way, at least the comparisons against the threshold values would always compare false while we're in the uninitialized state, which feels less scary than initializing all dynamic channels to always be over the power-down temp...

hawkw · 2026-07-29T23:54:36Z

-enum TemperatureReading {
-    /// Normal reading, timestamped using monotonic system time
-    Valid(TimestampedTemperatureReading),
+#[allow(dead_code)] // Not all bsps have inputs!


diversity is an Oxide value :)

jamesmunns added 30 commits July 22, 2026 19:35

Try to push down some state into the BSP itself

3123a58

Also misc_sensors

ded5468

WIP: start working on input

c1919e6

WIP: Extract state to BSP

45e7f1e

Bring us back down to even

e3fb65d

Start decomposing run_control

a87ed16

Push logic into InputChannel

823a7dc

Hoist logic back into control

4d2fdbd

Split out InputMetadata to reduce static RAM usage

29418d2

More putzing

dc7a597

I think it was wrong to split up fan presence and rpm reading.

49f959d

Split presence/read back up, finish cleaning up fans

952d1bf

It's a trait!

d959df3

Port over gimlet to new style impl

a05d9fb

Address review comments

36f5ac8

Push down I2C logic into bsp-specific code

befe410

Port grapefruit

e1e8bef

Medusa does not have a thermal task

9abcdfe

Port over minibar

eac9093

WIP sidecar, but I think I need to check something

b2de0ab

Clarify state tracking of InputChannel

8102d44

Implement the rest of sidecar

96e716c

Helper function for const init

908f0c6

Fix grapefruit

2658936

More dead code annotations

c1c18f4

More dead code

d765077

Track fan presence instead of a pseudo-status

e06c5b0

Make Input states *really* explicit.

35ccf1a

I kept getting these confused, and making it just very very verbose is I think the best way to make this clear.

Address a couple small review comments

9df220f

Merge branch 'master' into james/thermal-noodling

e9891b5

labbott reviewed Jul 28, 2026

View reviewed changes

Make USE_CONTROLLER a trait const, add docs

5c803a7

jamesmunns changed the title ~~WIP: Refactor of the thermal task~~ Refactor of the thermal task Jul 28, 2026

jamesmunns marked this pull request as ready for review July 28, 2026 13:26

labbott reviewed Jul 29, 2026

View reviewed changes

jamesmunns added 3 commits July 29, 2026 19:45

Address some review comments and correct some stale comments

a297744

One more

2547038

Update docs

6b78b8a

hawkw reviewed Jul 29, 2026

View reviewed changes

		// When we first start up, try to initialize the fan controller a few
		// times, in case there's a transient I2C error.

	/// Mask with bits set based on the Bsp's `power_mode` bits
	/// Mask with bits set based on the BSP's `power_mode` bits

Uh oh!

Conversation

jamesmunns commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

labbott left a comment

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!

jamesmunns commented Jul 28, 2026

Uh oh!

jamesmunns commented Jul 28, 2026

Uh oh!

jamesmunns commented Jul 28, 2026

Uh oh!

labbott left a comment

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!

Uh oh!

Uh oh!

jamesmunns commented Jul 29, 2026

All the dead_code annotations

The #[path] module stuff for bsps/common

Be okay with some changes to ringbufs

Revisit how we do state transitions

Duplication between emc2305 and max31790

Duplication/verbosity across bsps

Things that still need to be done to make this portable

Uh oh!

jamesmunns commented Jul 29, 2026

Uh oh!

jamesmunns commented Jul 29, 2026

Uh oh!

hawkw left a comment

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!

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

jamesmunns commented Jul 23, 2026 •

edited

Loading

All the `dead_code` annotations

The `#[path]` module stuff for `bsps/common`