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]

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]

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

[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.

[jamesmunns]

Booted this on a cosmo, I see the "fan added" ereports as expected on boot:

jamesmunns@alfred ~ $ pfexec ./humility0908 -t cosmo-hubris-sp -a ./cosmo-b-5f51ba353.zip ereport dump
humility: WARNING: archive on command-line overriding archive in environment file
humility: attached to 1fc9:0143:XJACFXSEKTQJS via CMSIS-DAP V1
task:      packrat (6)
timestamp: 0
ena:       1
  lost: null

task:      thermal (8)
timestamp: 9498
ena:       2
  class: hw.fan.ok,
  version: 0,
  sensor: "V12_U2A_A0",
  refdes: "J200/U1",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9498
ena:       3
  class: hw.fan.ok,
  version: 0,
  sensor: "V12_U2A_A0",
  refdes: "J200/U1",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9498
ena:       4
  class: hw.fan.ok,
  version: 0,
  sensor: "V12_U2A_A0",
  refdes: "J200/U1",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9498
ena:       5
  class: hw.fan.ok,
  version: 0,
  sensor: "V12_U2A_A0",
  refdes: "J200/U1",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9498
ena:       6
  class: hw.fan.ok,
  version: 0,
  sensor: "V12_U2A_A0",
  refdes: "J200/U1",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9498
ena:       7
  class: hw.fan.ok,
  version: 0,
  sensor: "V12_U2A_A0",
  refdes: "J200/U1",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

[jamesmunns]

Actually, those are not the details I expect.

[[config.i2c.devices]]
bus = "front"
mux = 1
segment = 1
address = 0b0111_000
device = "max5970"
description = "U.2 Sharkfin A hot swap controller"
power = { rails = [ "V12_U2A_A0", "V3P3_U2A_A0" ], pmbus = false }
sensors = { voltage = 2, current = 2 }
name = "sharkfin_a_hsc"
refdes = ["J200", "U1"]
removable = true

vs

[[config.i2c.devices]]
bus = "rear"
address = 0x20
device = "max31790"
description = "Fan controller"
sensors = { speed = 6, names = [
    "Southeast", "Northeast", "South", "North", "Southwest", "Northwest"
] }
refdes = "U58"

I'm gunna look into this.

[jamesmunns]

Okay that's better:

jamesmunns@alfred ~ $ pfexec ./humility0908 -t cosmo-hubris-sp -a ./cosmo-b-d3e14015a.zip ereport dump
humility: WARNING: archive on command-line overriding archive in environment file
humility: attached to 1fc9:0143:XJACFXSEKTQJS via CMSIS-DAP V1
task:      packrat (6)
timestamp: 0
ena:       1
  lost: null

task:      thermal (8)
timestamp: 9497
ena:       2
  class: hw.fan.ok,
  version: 0,
  sensor: "Southeast",
  refdes: "U58",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9497
ena:       3
  class: hw.fan.ok,
  version: 0,
  sensor: "Northeast",
  refdes: "U58",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9497
ena:       4
  class: hw.fan.ok,
  version: 0,
  sensor: "South",
  refdes: "U58",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9497
ena:       5
  class: hw.fan.ok,
  version: 0,
  sensor: "North",
  refdes: "U58",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9497
ena:       6
  class: hw.fan.ok,
  version: 0,
  sensor: "Southwest",
  refdes: "U58",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500

task:      thermal (8)
timestamp: 9497
ena:       7
  class: hw.fan.ok,
  version: 0,
  sensor: "Northwest",
  refdes: "U58",
  lo_rpm_lim: 500,
  hi_rpm_lim: 13500