observer: MWOCP67 PMBus Support by evan-oxide · Pull Request #2549 · oxidecomputer/hubris

evan-oxide · 2026-06-04T18:34:31Z

This PR implements oxidecomputer/pmbus#28. It:

Adds support for PMBus communication with the MWOCP67 PSUs.
Combines the psc-seq-server and observer-seq-server back into one task, since the differences can be managed by a few cfgs. (I considered using an Mwocp6X trait instead to make it generic over PSU models, but that's hard because of the methods that return device-specific CommandData types).
Polls the onboard tmp117 temperature sensor.
Adds the framulator task to dev builds, to test that the FRAM works.

Known issues / remaining todos:

It's still unclear how many power rails the MWOCP67 actually has, what their voltages are, and what pages are used to access them. It looks suspiciously like it's reporting stats for the same 50V rail on both pages 0 and 1. There's also insufficient documentation about which other registers are paged.
The MWOCP67 has 2 new temperature sensors on the positive and negative bus bar clips and they both report ~170°C. This is probably incorrect.
I wasn't able to manually control the fan speed. I don't know if we care.
I'm not sure what model of EEPROM is built into the PSUs. I'm assuming it's still the M24C02, like the MWOCP68 had, and I was able to read some bytes off it.
I skipped implementing the blackbox feature because it looks complicated and we weren't using the (somewhat different) blackbox feature on the PSC.
PSU firmware updates aren't supported yet.

Testing:

hawkw · 2026-06-04T20:44:59Z

+# This one device actually appears at 8 different i2c addresses. 0b1010_000 -
+# 0b1010_011 let you access the main memory registers, where the 2 LSBs of the
+# device address are the 2 MSBs of the memory address. 0b1011_000 - 0b1011_011
+# let you access the security registers and write protection registers.


FWIW --- and I am not saying that we should get rid of this comment, for the record --- this is an EEPROM we use kinda all over the place (there's one on pretty much every board), so perhaps we should document this in a more centralized location.

Any thoughts about where it should go? This info is in the datasheet, I just added a note here because I was confused about why I saw 8 devices on the bus.

hawkw · 2026-06-04T20:56:09Z

+        const REVISION_LEN: usize = 14;
+
+        let mut data = [0u8; REVISION_LEN];
+        let expected = b"XXXX-YYYY-0000";
+
+        let len = self
+            .device
+            .read_block(CommandCode::MFR_REVISION as u8, &mut data)
+            .map_err(|code| Error::BadFirmwareRevRead { code })?;
+
+        //
+        // Per ACAN-157, we are expecting this to be of the format:
+        //
+        //    XXXX-YYYY-0000
+        //
+        // Where XXXX is the firmware revision on the primary MCU (AC input
+        // side) and YYYY is the firmware revision on the secondary MCU (DC
+        // output side).  We aren't going to be rigid about the format of
+        // either revision, but we will be rigid about the rest of the format.
+        //
+        if len != REVISION_LEN {
+            return Err(Error::BadFirmwareRevLength);
+        }
+
+        for index in 0..len {
+            if expected[index] == b'X' || expected[index] == b'Y' {
+                continue;
+            }
+
+            if data[index] != expected[index] {
+                return Err(Error::BadFirmwareRev { index: index as u8 });
+            }
+        }
+
+        //
+        // Return the primary MCU version
+        //
+        Ok(FirmwareRev([data[0], data[1], data[2], data[3]]))


This looks like the same format as the one used by the MWOCP68, right? That observation kinda makes me want to push for pulling out the parsing code into a common "Murata FRU ID" thingy. But, well...I don't think that actually gains us all that much, I just have some aesthetic desire to not duplicate things.

I pulled it out into a mwocp6x module along with the shared types. What do you think of the new module structure? It might need to change later if the firmware update process ends up being significantly different, but hopefully not.

It's unfortunate that we can't share more of the implementations - they look similar but use different types from the pmbus crate.

hawkw · 2026-06-04T20:58:18Z

+        // ACAN-157 doesn't specify what page this is on.
+        // Assume it's on page 0, as it is on the better-documented mwocp68.


Out of curiosity, have we tested what happens when we read this (and the other status registers) on page 1? Does it also work, or does the MCU get mad at us?

I tried reading a bunch of these registers on both pages and always got the same value on both. I don't know if that's because the page is ignored or they happened to be the same on both pages.

hawkw · 2026-06-04T21:03:34Z

+                Slot::Psu5 => '5',
            };
-            FixedString::try_from_utf8(&v54_psu[..]).unwrap_lite()
+            Mwocp6x::rail_name(psu_label)


I found the decision to move this into the drivers a bit weird at first once I saw it, but now that I see how it's being used, it's not unreasonable. However, I don't love putting the rail name in the drv-i2c-devices crate, since we could imagine a universe where we have a MWOCP6x PSU on a board where we use different rail names (although admittedly, that's unlikely). Personally, I might have considered just leaving all the code here, and having one of two different "base" strings ("V54_PSUx" or "V50_PSUx") based on the board cfg. I'm not super bothered by the current approach, but I feel like it might be a bit simpler that way. Up to you!

Yeah I don't love this approach either. On one hand it seems like the Mwocp67 class shouldn't need to know about rail naming conventions, and on the other hand scattering too many cfgs in the psc-seq-server could get confusing. So is this what you're suggesting?

let rail = { #[cfg(any(target_board = "psc-b", target_board = "psc-c"))] let mut rail_name = *b"V54_PSUx"; #[cfg(target_board = "observer-a")] let mut rail_name = *b"V50_MAIN_PSUx"; rail_name[rail_name.len() - 1] = match self.slot { Slot::Psu0 => b'0', ... }; FixedString::try_from_utf8(&rail_name[..]).unwrap_lite() };

Actually it's a problem that the rail names are different lengths now but EreportFields is still using FixedString<8>.

Yeah, we will have to either just make it longer and null pad it on the MWOCP68 version, or make the const size depend on which board we're building. But I do think having all of this right there in that file feels better to me, rather than hiding it in the driver.

FWIW I agree that the names should be here in psc-seq-server, rather than in the I2C driver (which should be rail-agnostic).

Done. I should probably test this... any suggestions for how to generate an ereport and see if the rail name looks good?

Tested! I generated a spurious ereport and read it with humility. The rail name looks good.

hawkw · 2026-06-04T21:28:27Z

+            Device::Tmp117 => {
+                for &s in self.temperature_sensors.iter() {
+                    let m = Tmp117::new(&dev);
+                    match m.read_temperature() {
+                        Ok(v) => sensor_api.post_now(s, v.0),
+                        Err(e) => {
+                            let e = Error::Tmp117Error(e);
+                            ringbuf_entry!(Trace::TemperatureReadFailed(s, e));
+                            sensor_api.nodata_now(s, e.into())
+                        }
+                    }
+                }
+                // The tmp117 is just a temperature sensor, not a speed sensor.
+                assert_eq!(self.speed_sensors.len(), 0);
+            }


Am I correct that, after this PR merges, if we were to add a temperature sensor to the TMP117 in the PSC config, we would also be able to fix #2547?

Yeah you'd need to update the PSC's base.toml, add an entry to the SENSORS array in sensor-polling, and test that the ComponentDetails part works (not just humility sensors).

evan-oxide added 7 commits June 4, 2026 14:33

update pmbus crate for mwocp67

0dd5ac9

add psus to base.toml

92052b7

add mwocp67 support

05f9fe6

delete observer-seq-server, make psc-seq-server generic with cfg

9bf35d2

rename rails to V50_MAIN and V50_AUX, still not sure if correct

f1a589f

poll the tmp117 temperature sensor

1946c63

add framulator (FRAM testing task) to dev builds

971c9c5

evan-oxide added the Observer PSC + 5.5 kW fun. label Jun 4, 2026

evan-oxide marked this pull request as ready for review June 4, 2026 18:34

evan-oxide requested review from hawkw and mkeeter June 4, 2026 19:58

hawkw reviewed Jun 4, 2026

View reviewed changes

evan-oxide added 7 commits June 4, 2026 18:42

remove ringbuf meant for debugging

3761283

fix index into rail name

915234d

add new error for BadSerialNumberRead instead of reusing another

087230a

add comments about removing framulator someday

360959d

move rail names back out of drivers

b3ec176

move shared types and fw rev parsing into a mwocp6x module

0d2f0cc

add license to new file, fix formatting

a228c9b

mkeeter reviewed Jun 5, 2026

View reviewed changes

Comment thread drv/i2c-devices/src/mwocp6x/mod.rs

rename read_speed() to read_fan_speed()

68b28e9

evan-oxide requested a review from hawkw June 8, 2026 19:59

		// ACAN-157 doesn't specify what page this is on.
		// Assume it's on page 0, as it is on the better-documented mwocp68.

Conversation

evan-oxide commented Jun 4, 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!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evan-oxide Jun 4, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

evan-oxide commented Jun 4, 2026 •

edited

Loading

evan-oxide Jun 4, 2026 •

edited

Loading