thermal: Add ereports

[jamesmunns]

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

closes #2603

[jamesmunns]

@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]

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"?

[jamesmunns]

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]

So, I was thinking more that we might want to have a conventional field name/payload structure for an I2C read error that would include the raw response code and either a refdes or a controller/bus/mux/addr (I think refdes is more concise) and include that structure in any I2C error ereport. Having that payload struct be included in the "fan sensor read error" ereport is how we would indicate it was for a fan.

I think it's fine to punt on this now and go and factor it out only once we are trying to make ereports like this in multiple places.

[hawkw]

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]

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]

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]

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]

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

[hawkw]

I left some suggestions on the ereport structures. I would really like it if we could figure out a way to get the fan slot numbers as marked on the Sidecar chassis here, but I realize there may not be a great way to do that with the way the sensor config works currently...

[hawkw]

So, I was thinking more that we might want to have a conventional field name/payload structure for an I2C read error that would include the raw response code and either a refdes or a controller/bus/mux/addr (I think refdes is more concise) and include that structure in any I2C error ereport. Having that payload struct be included in the "fan sensor read error" ereport is how we would indicate it was for a fan.

I think it's fine to punt on this now and go and factor it out only once we are trying to make ereports like this in multiple places.

[jamesmunns]

@hawkw one thing I noticed is that there is a slight discrepancy in how we number fans at the chassis level and how the thermal task "thinks" about fans. For example on sidecar:

The chassis thinks in terms of "fan assemblies", each of which are a pair of counter-rotating fans with individual control and monitoring. When reporting in the ereport, would you like the slot to reflect the chassis numbering (e.g. we'll have [0, 0, 1, 1, 2, 2, 3, 3]), or the logical ordering (e.g. we'll have [0, 1, 2, 3, 4, 5, 6, 7]), OR add a second field (e.g. we'll have [(0, 0), (0, 1), ... (3, 0), (3, 1)], or get cute and pack it into one u8 still (e.g. we'll have [00, 01, ... 30, 31]), or even go full enum on it (e.g. we'll have [Fan0A, Fan0B, ... Fan3A, Fan3B])?

In 04bc618 I added plumbing for the "slot" field to the ereport, but the numbering currently is almost certainly wrong (I need to make sure that the ordering defined in the BSP initializer matches the schematic/chassis at least for all PCBAs).

edit: The fans are also definitely not numbered in the order that I've placed them in currently:

[jamesmunns]

Also @hawkw should we rename these functions to refdes and sensor respectively, to avoid other folks making the same mistake I did wrt naming?

hubris/task/sensor-api/src/lib.rs

Lines 85 to 102 in 1cbf835

	/// Returns the component ID (refdes) corresponding to this sensor.
	///
	/// Note that multiple sensor IDs may have the same component ID, when a
	/// single device exposes multiple measurement channels.
	#[cfg(feature = "component-id-lookup")]
	pub fn component_id(
	&self,
	) -> fixedstr::FixedStr<'static, { config::MAX_COMPONENT_ID_LEN }> {
	config::SENSOR_ID_TO_COMPONENT_ID[self.0 as usize]
	}
	/// Returns the name of this sensor.
	#[cfg(feature = "sensor-name-lookup")]
	pub fn name(
	&self,
	) -> fixedstr::FixedStr<'static, { config::MAX_SENSOR_NAME_LEN }> {
	config::SENSOR_ID_TO_NAME[self.0 as usize]
	}

[jamesmunns]

Chatted with @hawkw:

• We probably want to focus on the "field replaceable unit", e.g. the fan assembly unit, when we talk about "slots". I need to hunt down this info from mechanical details
• As a short term hack, I MAY just revert the slot field for now, and add it back later in a versioned rev.
• This is actually a bit more complicated field to deal with, because a "fan assembly" isn't the refdes+name that we're reporting from, because it's coming from the fan controller's perspective
• We probably need to discuss how to report this upstack, and take into account things like the VPD eeprom on fans, which are a different refdes and such.
• I will open a separate hubris issue to discuss "how do we think about this", it may be more of a host software concern, but it's at least useful to capture here while it's fresh, and will be relevant to how hubris should report this in ereports and such.