Move thermal buffers off the stack by mkeeter · Pull Request #2428 · oxidecomputer/hubris

mkeeter · 2026-03-06T21:58:17Z

I tried to make a change to thermal and the stack complained. I then realized that main's stack in thermal is huge:

thermal: 7000 bytes (limit is 8096)
     [+0] _start
  [+6008] main
   [+896] task_thermal::control::ThermalControl::transition_to_running
    [+56] task_thermal::control::ThermalControl::record_leaving_overheat
    [+40] <ringbuf::CountedRingbuf<T,C,_> as ringbuf::RecordEntry<T>>::record_entry

This PR shrinks it down to a more reasonable size by moving temperature data arrays into a static buffer:

thermal: 2704 bytes (limit is 8096)
     [+0] _start
  [+2320] main
   [+112] task_thermal::control::ThermalControl::set_pwm
    [+16] task_thermal::bsp::Bsp::fan_control
    [+64] task_thermal::control::Max31790State::initialize
    [+16] drv_i2c_devices::max31790::write_reg8
    [+88] drv_i2c_api::I2cDevice::write
    [+24] core::panicking::panic_fmt
    [+56] __rustc[b0c4d0eb67640fa4]
     [+8] core::fmt::Write::write_fmt

There's a little bit of trickery here: we store a single [Option<TemperatureReading>; TEMPERATURE_ARRAY_SIZE] array, then wrap it in a special type when it's guaranteed that all the values are Some(..).

We also have to (unfortunately) add a ThermalControlState::Empty, because we need a valid temporary value to put into the state position while modifying + replacing it.

hawkw

Neat, I had wanted to do something like this last time I was looking at this code but decided to defer it at the time. I had some very minor nitpicks but no real concerns.

task/thermal/src/control.rs

mkeeter · 2026-03-10T13:59:11Z

@hawkw I switched to an array of [Cell<Option<TemperatureReading>>; ..], which removes a bunch of the complexity - no need for ThermalControlState::Empty or the TemperatureArrayToken.

mkeeter · 2026-03-10T14:57:55Z

I did a quick stackmargin audit (since I had edited margins by vibes) and they still seem reasonable:

ID TASK                STACKBASE  STACKSIZE   MAXDEPTH     MARGIN
19 thermal            0x2404c000       8096       6952       1144 # old sidecar
 7 thermal            0x24002000       6000       4968       1032 # old gimlet
 8 thermal            0x24002000       6000       2720       3280 # old cosmo
19 thermal            0x2404c000       4000       2688       1312 # new sidecar
 7 thermal            0x24002000       2000       1296        704 # new gimlet
 8 thermal            0x24002000       3000       1288       1712 # new cosmo

labbott

LGTM I love shrinking stacks 😎

hawkw

Okay, I like the new approach with Cells, but I did leave a few small nitpicks, mostly about docs etc.

hawkw · 2026-03-11T16:43:15Z

task/thermal/src/control.rs

+    }
 }

+mod temperature_array {


Nit, take it or leave it: I wonder somewhat if a higher level theory of operation for the relationship between the RawTemperatureArray, OptionalTemperatureArray, and TemperatureArray types could be included in the module-level docs for this?

hawkw · 2026-03-11T16:43:52Z

task/thermal/src/control.rs

+    /// Temperature state iterator with `TemperatureReading` values
+    impl TemperatureArray {
+        pub fn zip_temperatures<'a>(


I think the doc comment is meant to be on the method

Suggested change

/// Temperature state iterator with `TemperatureReading` values

impl TemperatureArray {

pub fn zip_temperatures<'a>(

impl TemperatureArray {

/// Temperature state iterator with `TemperatureReading` values

pub fn zip_temperatures<'a>(

also, why are there two separate impl blocks here?

Stuff got shuffled around, both things are now fixed

hawkw · 2026-03-11T16:45:26Z

task/thermal/src/control.rs

+    /// Returns an iterator over tuples of `(sensor_id, value, thermal model)`
+    ///
+    /// The `values` array contains static and dynamic values (in order);
+    /// this function will panic if sizes are mismatched.
+    ///
+    /// Every dynamic input is represented by an `Option<DynamicInputChannel>`.
+    /// If the input is not present right now, it will be `None`, but will
+    /// continue to take up space to preserve ordering.
+    ///
+    /// In cases where dynamic inputs are not present (i.e. they are `None` in
+    /// the array), the iterator will skip that entire tuple.


i feel like some of this documentation could also be included on the public OptionalTemperatureArray::zip_temperatures, but 🤷‍♀️ --- we never build API docs for this stuff (see #2422), so putting it on private functions is probably fine.

I didn't want to repeat this on both OptionalTemperature::zip_temperatures and TemperatureArray::zip_temperatures, so I figured this was the right place (since as you said we don't actually build docs) 🤷🏻‍♂️

mkeeter requested review from hawkw and labbott March 6, 2026 21:58

mkeeter force-pushed the mkeeter/shrink-thermal-stack branch 4 times, most recently from fd4eca1 to 7eb937e Compare March 9, 2026 15:04

mkeeter mentioned this pull request Mar 9, 2026

Add PWM sensor to thermal task #2430

Open

hawkw approved these changes Mar 9, 2026

View reviewed changes

mkeeter force-pushed the mkeeter/shrink-thermal-stack branch 2 times, most recently from a27d9e8 to 2d6751c Compare March 9, 2026 22:33

mkeeter force-pushed the mkeeter/shrink-thermal-stack branch 5 times, most recently from 588cbe9 to 7010013 Compare March 10, 2026 14:50

mkeeter mentioned this pull request Mar 10, 2026

Allow humility stackmargin to take task names as arguments oxidecomputer/humility#604

Merged

mkeeter force-pushed the mkeeter/shrink-thermal-stack branch from 7010013 to c64bf6a Compare March 11, 2026 14:07

labbott approved these changes Mar 11, 2026

View reviewed changes

hawkw approved these changes Mar 11, 2026

View reviewed changes

mkeeter force-pushed the mkeeter/shrink-thermal-stack branch 2 times, most recently from 8e2c3a1 to 234e459 Compare March 12, 2026 13:13

mkeeter added 3 commits March 12, 2026 09:28

Move thermal buffers off the stack

5b534ec

Use Cell<Option<TemperatureReading>> to avoid state shenanigans

9307eb2

Review feedback

323f754

mkeeter force-pushed the mkeeter/shrink-thermal-stack branch from 234e459 to 323f754 Compare March 12, 2026 13:28

mkeeter enabled auto-merge (squash) March 12, 2026 13:36

mkeeter merged commit 3fd4873 into master Mar 12, 2026
180 checks passed

mkeeter deleted the mkeeter/shrink-thermal-stack branch March 12, 2026 13:45

Conversation

mkeeter commented Mar 6, 2026

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkeeter commented Mar 10, 2026

Uh oh!

mkeeter commented Mar 10, 2026

Uh oh!

labbott left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

mkeeter Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

mkeeter Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

mkeeter Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkeeter Mar 11, 2026 •

edited

Loading