Duplicate hardware metrics on sensor ambiguity #2634

Activity

1. hawkw commented on Aug 11, 2026

   

   hawkw

   on Aug 11, 2026

   Member

   More actions

   Yeah, I think you're right that we need to add something in Hubris to disambiguate both these and the fan speed sensors (which, IIRC, are for the fans on the front and back of the PSU?). We should probably move this issue to the Hubris repo, since I believe the fix for this is going to be entirely there.
2. hawkw commented on Aug 11, 2026

   

   hawkw

   on Aug 11, 2026

   Member

   More actions

   It would also be helpful to include some kind of check to avoid duplicate sensor names if possible

   We can probably do something about that in the Hubris build system, good idea!
3. jmcarp

   transferred this issue fromoxidecomputer/omicronon Aug 12, 2026
4. hawkw commented on Aug 12, 2026

   

   hawkw

   on Aug 12, 2026

   Member

   More actions

   Looking closer at the Hubris build system, we do currently have a way to assign names to multiple sensors of the same type on a given device. See, for example, the Cosmo fan controller:
   

   hubris/app/cosmo/base.toml

   Lines 1148 to 1150 in 1a6600c

   	sensors = { speed = 6, names = [
   	"Southeast", "Northeast", "South", "North", "Southwest", "Northwest"
   	] }

   However, the way this works currently is pretty dumb: when generating sensor descriptions, a sensor's name is determined as follows:

   • if the device is a PMBus device which defines a list of power rails, we select the name of the power rail that corresponds to that sensor, in such a way that the multiple sensor types (i.e. input current, output current, input voltage, output voltage...) generated for each rail all get the rail's name
   • if it isn't a PMBus device, or if the sensor type is not in the list of sensors being declared as "power sensors" on the devcice, and there is a list of sensor names, we select the n + 1 th index from the list of names, where n is the total number of sensors added so far.
   • if there isn't a list of names, we just use the device's name. This is the path that the PSU fan and temp sensors currently hit.

   Here's the code that implements this:

   hubris/build/i2c/src/lib.rs

   Lines 382 to 403 in 1a6600c

   	let name: Option<String> = if let Some(power) = d.power_for_kind(kind) {
   	if let Some(rails) = &power.rails {
   	if idx < rails.len() {
   	Some(rails[idx].clone())
   	} else {
   	panic!("sensor count exceeds rails for {d:?}",);
   	}
   	} else {
   	d.name.clone()
   	}
   	} else if let Some(names) = &d.sensors.as_ref().unwrap().names {
   	if idx >= names.len() {
   	panic!(
   	"name array is too short ({}) for sensor index ({idx})",
   	names.len(),
   	);
   	} else {
   	Some(names[idx].clone())
   	}
   	} else {
   	d.name.clone()
   	};

   Note

   The way this works is both confusing and remarkably under-documented. We really need to add some better documentation explaining how sensor names are (or are not) selected.

   The unfortunate thing about this code is that the names are determined based on the order that the sensors are added. In particular, sensors are added in order based on the type, here:

   hubris/build/i2c/src/lib.rs

   Lines 329 to 359 in 1a6600c

   	for (d_index, d) in devices.iter().enumerate() {
   	if let Some(s) = &d.sensors {
   	for i in 0..s.temperature {
   	desc.add_sensor(Sensor::Temperature, d, i, d_index);
   	}
   	for i in 0..s.power {
   	desc.add_sensor(Sensor::Power, d, i, d_index);
   	}
   	for i in 0..s.current {
   	desc.add_sensor(Sensor::Current, d, i, d_index);
   	}
   	for i in 0..s.voltage {
   	desc.add_sensor(Sensor::Voltage, d, i, d_index);
   	}
   	for i in 0..s.input_current {
   	desc.add_sensor(Sensor::InputCurrent, d, i, d_index);
   	}
   	for i in 0..s.input_voltage {
   	desc.add_sensor(Sensor::InputVoltage, d, i, d_index);
   	}
   	for i in 0..s.speed {
   	desc.add_sensor(Sensor::Speed, d, i, d_index);
   	}
   	}
   	}

   I think this includes the indices generated for the various PMBus sensors, even though they don't actually hit the names list. So there will need to be dummy entries in that list for the PMbus sensors so that it's the correct length.

   The code in task-sensor-polling that reads the temperature and fan speed sensors is just selecting the PMbus page for each such sensor by index and posting to the corresponding sensor ID:
   

   hubris/task/sensor-polling/src/main.rs

   Lines 109 to 131 in 9e553c7

   	for (i, &s) in self.temperature_sensors.iter().enumerate() {
   	let m = Mwocp68::new(&dev, i.try_into().unwrap());
   	match m.read_temperature() {
   	Ok(v) => sensor_api.post_now(s, v.0),
   	Err(e) => {
   	let e = Error::Mwocp68Error(e);
   	ringbuf_entry!(Trace::TemperatureReadFailed(s, e));
   	sensor_api.nodata_now(s, e.into())
   	}
   	}
   	}
   	for (i, &s) in self.speed_sensors.iter().enumerate() {
   	let m = Mwocp68::new(&dev, i.try_into().unwrap());
   	match m.read_fan_speed() {
   	Ok(v) => sensor_api.post_now(s, v.0),
   	Err(e) => {
   	let e = Error::Mwocp68Error(e);
   	ringbuf_entry!(Trace::SpeedReadFailed(s, e));
   	sensor_api.nodata_now(s, e.into())
   	}
   	}
   	}
   	}

   So, we'd need to:

   1. figure out from the datasheet which sensor each PMBus page index corresponds to by name
   2. using our brains and our understanding of the guts of build_i2c, determine which index into the number of sensors added for the PSU corresponds to each PMBus page index used when polling sensors
   3. construct a list of names that puts the correct name at the correct index

   Alternatively, we could change the way sensor name lists are specified to make the way they are selected not be quite so confusing. It seems like if we changed the I2C config file so that sensor name lists are specified per sensor type, it would be much simpler to add the correct names here. I don't even think that would break existing code too badly, since at a glance, the main place we're using the sensor name lists is for fan speed controllers, which only have one sensor type. That's probably why no one ever really noticed that using sensor name lists on devices that have multiple types of sensors is a nightmare.
5. hawkw

   added a parent issue on Aug 12, 2026

   • Tracking issue for build_i2c usability, debuggability, and documentation improvements #2636
6. hawkw

   removed a parent issue on Aug 12, 2026

   • Tracking issue for build_i2c usability, debuggability, and documentation improvements #2636
7. hawkw

   added a parent issue on Aug 12, 2026

   • I2C codegen could use some love #501
8. hawkw

   added a sub-issue on Aug 12, 2026

   • build-i2c should probably not permit multiple sensors of the same type to have the same name #2637
9. jamesmunns commented on Aug 21, 2026

   

   jamesmunns

   on Aug 21, 2026

   Contributor

   More actions

   I added the following check to I2cSensorsDescription::new():

   let mut deviations = vec![];
   for (key, ids) in desc.by_name.iter() {
       if ids.len() > 1 {
           let refer =
               format!("({}:{}:{})", key.device, key.name, key.kind);
           deviations.push((refer, ids.len()));
       }
   }
   
   if !deviations.is_empty() {
       let mut output = String::new();
       output += "cargo::warning=The following sensors are duplicated \
           with the same name. This is likely an error. \
           See https://github.com/oxidecomputer/hubris/issues/2634 for \
           details: ";
       for (r, ct) in deviations {
           _ = write!(&mut output, "{r}: {ct} instances, ");
       }
       println!("{output}");
   }

   This gives me the following warning for psc-c (lightly edited for readability):

   warning: task-sensor-api@0.1.0: The following sensors are duplicated with the same name. This is likely an error. See #2634 for details:

   • (mwocp68:psu0mcu:TEMPERATURE): 3 instances
   • (mwocp68:psu0mcu:SPEED): 2 instances
   • (mwocp68:psu1mcu:TEMPERATURE): 3 instances
   • (mwocp68:psu1mcu:SPEED): 2 instances
   • (mwocp68:psu2mcu:TEMPERATURE): 3 instances
   • (mwocp68:psu2mcu:SPEED): 2 instances
   • (mwocp68:psu3mcu:TEMPERATURE): 3 instances
   • (mwocp68:psu3mcu:SPEED): 2 instances
   • (mwocp68:psu4mcu:TEMPERATURE): 3 instances
   • (mwocp68:psu4mcu:SPEED): 2 instances
   • (mwocp68:psu5mcu:TEMPERATURE): 3 instances
   • (mwocp68:psu5mcu:SPEED): 2 instances
10. hawkw

    added a commit that references this issue on Sep 30, 2026

    fix MGS sensor metrics test

    ...

    6379ae0