thermal: Add ereports by jamesmunns · Pull Request #2639 · oxidecomputer/hubris

jamesmunns · 2026-08-13T10:24:16Z

Adds ereports for the thermal task, as a follow up to #2630

jamesmunns · 2026-08-13T15:36:00Z

+    }
+}
+
+#[derive(microcbor::EncodeFields)]


@hawkw I'm particularly open to feedback on what to put in these ereports.

Right now I'm sending SensorId as id, which on one hand is nice, because that's something that could directly be used to query the fan RPM. On the other hand, it's an unstable generated ID, and does nothing to explain which fan this is.

We could include a more string-y "East South East" name (for sidecar), or some kind of refdes, or something. This was the easiest to do, but I can plumb other info through if we need it.

Right now I'm sending SensorId as id, which on one hand is nice, because that's something that could directly be used to query the fan RPM. On the other hand, it's an unstable generated ID, and does nothing to explain which fan this is.

We could include a more string-y "East South East" name (for sidecar), or some kind of refdes, or something. This was the easiest to do, but I can plumb other info through if we need it.

IIUC there is no way currently for the control plane to turn the sensor ID into anything that's actually meaningful to it. While the ID can be used to request fan RPM from Hubris, it cannot be used to query fan speeds that the control plane has previously read from Hubris and stored in Clickhouse by Oximeter, since those timeseries data points are not labeled by the sensor ID (which is on purpose, because as you mentioned, it is not stable across Hubris builds).

Therefore, I think we should probably be including the refdes, as that can be used both for querying historical measurements from Clickhouse and for reading the current value via component-details. Since querying by refdes will give us the metrics for all the sensors associated with that fan controller, we will also need to include the name assigned to that sensor in the app.toml. For instance, on sidecar:

hubris/app/sidecar/base.toml

Line 512 in c1328f7

sensors = { speed = 4, names = [ "ESE_fan0", "ENE_fan0", "SE_fan1", "NE_fan1" ] }

I think that the combination of the refdes and sensor name should be sufficient for the control plane to both retrieve historical measurements from Clickhouse and poll new ones from the SP if it chooses to. I'm on the fence as to whether we ought to also keep the sensor ID or not. It's more or less useless to the control plane, but it's also only a couple bytes, so we may as well throw it in.

I think that it will require a bit more work to plumb through the refdes and sensor names in a way that they can be retrieved from a sensor ID; I started describing that in #2364.

My abortive PR #2383 included a codegen change for looking up a refdes from a sensor ID. That won't give us the sensor name, as written, but it could be added there. I think the reason I didn't finish that at the time is mostly because I had been hoping to figure out a way to do that that didn't require generating a giant LUT of sensor IDs that would use a lot of flash.

One or both of us could blow some of the dust out of that PR, perhaps pulling out just the codegen-related stuff in there?

okay, i think #2645 should've added all the requisite codegen :)

jamesmunns · 2026-08-13T15:40:33Z

@hawkw another open question is "should we send ereports on power-on"? Right now on sidecar which has 8 fans, we'll send 8x "fan is/is not present" messages, and 8x "fan is/is not nominal" messages.

I can add some more logic that suppresses this if we want. It seems consistent to send them, but also I don't know if we've observed any "mad rush" of this kind of state transmission, and what we should do if the outgoing ereport queue fills up.

We also could pay attention to whether deliver_ereport succeeds or not, and try again every fan tick (1hz) until it succeeds, though other services all seem to ignore whether it succeeded or not.

hawkw · 2026-08-13T17:14:04Z

+#[derive(Encode)]
+#[ereport(class = "hw.fan.rpm.err", version = 0)]
+struct FanRpmReadFailed {
+    id: u32,
+}


part of me thinks we might really want to have a more general ereport in the shared ereports crate for "trying to read a sensor over I2C failed"?

I would be open to discuss this! How do we still maintain the context: "this i2c read is specifically for a fan and we are sad we don't have its data"?

hawkw · 2026-08-13T17:18:12Z

We also could pay attention to whether deliver_ereport succeeds or not, and try again every fan tick (1hz) until it succeeds, though other services all seem to ignore whether it succeeded or not.

This is something I have thought about doing in several places. I think it's not a bad idea, although one deficiency in the way ereports are currently implemented is that the timestamp is always decided by packrat when it receives the ereport, which means that if a task is hanging onto an ereport and retrying it, the timestamp associated with the ereport will be the time it was received by packrat, not the time at which the thing actually occurred. We might want to have a way for the task to explicitly override the timestamp, for cases like this.

For now, we haven't actually ever dropped ereports due to full buffers, so I haven't been worrying about retries too much yet.

jamesmunns · 2026-08-13T17:19:52Z

For now, we haven't actually ever dropped ereports due to full buffers

Do we have a way to reasonably detect when this happens? I can add a little stress and see if we hit it, and back off if not.

hawkw · 2026-08-19T23:39:11Z

Do we have a way to reasonably detect when this happens?

there are ringbuf counters in each task that will tell you precisely how many times it tried and failed to submit an ereport. but, more importantly, the ereport ring buffer code in packrat will create a "loss report" for upstack software that tells it "hey, i have dropped this many ereports, sorry about that". this is part of the same stream as the actual ereports. as far as i know, we have never actually seen such a loss report in a production system.

I can add a little stress and see if we hit it, and back off if not.

for what it's worth, such a test is really only going to be interesting if there is a control plane collecting ereports (and therefore draining the buffer); if you run it against a bench system, you will see a bunch of stuff get dropped, but that's kind of anticipated.

jamesmunns · 2026-08-20T15:30:17Z

-            Fps::TooFast(rpm) => Trace::FanOverspeed(id, rpm),
-            Fps::TooSlow(rpm) => Trace::FanUnderspeed(id, rpm),
+        let fan_info = || FanInfo {
+            name: fan.name,


So @hawkw interesting point here:

If I do id.name() here, it ends up inflating the .rodata, likely with the table of all sensor names:

james@fool:~/oxide/hubris git:(james/ereport-thermals) ✗ arm-none-eabi-size -A target/cosmo-b-dev/dist/thermal.elf target/cosmo-b-dev/dist/thermal.elf : section size addr .text 17632 0 .rodata 4640 0 .data 580 0 .bss 2452 0

But with the change to make .name() a const fn, and storing .name in the const-fn constructor of Fan::new(), then we don't pull in the whole array:

james@fool:~/oxide/hubris git:(james/ereport-thermals) ✗ arm-none-eabi-size -A target/cosmo-b-dev/dist/thermal.elf target/cosmo-b-dev/dist/thermal.elf : section size addr .text 17544 0 .rodata 2552 0 .data 628 0 .bss 2452 0 .uninit 0 0

I don't think this is a make or break, a ~2KiB flash difference (on cosmo) isn't really THAT big of a deal, but it does seem lightly footgun-y if you aren't Too Clever about it.

jamesmunns · 2026-08-20T18:55:11Z

@hawkw this should be ready for a fresh look, IMO.

jamesmunns added 2 commits August 13, 2026 11:29

Start adding ereport mechanisms for thermals

3a96768

Fill in thermal ereports

c4b2b31

jamesmunns requested review from hawkw and labbott August 13, 2026 10:24

jamesmunns added 5 commits August 13, 2026 12:37

Fix a few things

28b4ead

Remove unused task slot

0500cc1

Add missing import

1dd736d

Adjust ruby sizes

6c1795b

Don't get reply-faulted by packrat because ereports aren't enabled

8844316

jamesmunns commented Aug 13, 2026

View reviewed changes

hawkw reviewed Aug 13, 2026

View reviewed changes

Merge branch 'master' into james/ereport-thermals

4b0f4db

jamesmunns mentioned this pull request Aug 20, 2026

[WIP] thermal task ereports #2383

Draft

Switch from sensor_id to name

5c16251

jamesmunns commented Aug 20, 2026

View reviewed changes

jamesmunns added 4 commits August 20, 2026 17:37

Also include component_id

5fe008f

Ram adjustment

99b585e

Rework to include raw response code in FanRpmReadFailed

df409fb

Fix comment

d36cdf3

Uh oh!

Conversation

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

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!

jamesmunns commented Aug 13, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw commented Aug 13, 2026

Uh oh!

jamesmunns commented Aug 13, 2026

Uh oh!

hawkw commented Aug 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesmunns commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jamesmunns commented Aug 13, 2026 •

edited

Loading