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

Based on experience with other PMBus devices, I would guess that it's just that the value was the same on both pages; I'd expect that it should be possible to see a fault on one rail and not the other. I think we could probably test this by setting a warning threshold on one page (say VIN_WARN_LIMIT or something) but not the other, and seeing if the warning bit gets set on one page and not the other. I could be wrong about this, but I think it's worth investigating, because the current behavior of both MWOCPx drivers always sending page 0 feels sketchy to me.

I'm fine with merging this as-is and investigating as a follow-up though.

I'll leave a comment on #2410 about investigating this more

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

hawkw · 2026-06-10T16:25:31Z

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


Based on experience with other PMBus devices, I would guess that it's just that the value was the same on both pages; I'd expect that it should be possible to see a fault on one rail and not the other. I think we could probably test this by setting a warning threshold on one page (say VIN_WARN_LIMIT or something) but not the other, and seeing if the warning bit gets set on one page and not the other. I could be wrong about this, but I think it's worth investigating, because the current behavior of both MWOCPx drivers always sending page 0 feels sketchy to me.

I'm fine with merging this as-is and investigating as a follow-up though.

hawkw · 2026-06-10T16:34:19Z

@@ -308,6 +321,7 @@ const ALL_PSU_PWR_OK_PINS: sys_api::PinSet =

 // Our notification configuration system doesn't have any concept of arrays, so,
 // collect its predefined masks into convenient arrays.
+#[cfg(any(target_board = "psc-b", target_board = "psc-c"))]
 const PSU_PWR_OK_NOTIF: [u32; PSU_COUNT] = [
    notifications::PSU_PWR_OK_1_MASK,
    notifications::PSU_PWR_OK_2_MASK,
@@ -316,8 +330,18 @@ const PSU_PWR_OK_NOTIF: [u32; PSU_COUNT] = [
    notifications::PSU_PWR_OK_5_MASK,
    notifications::PSU_PWR_OK_6_MASK,
 ];
+#[cfg(target_board = "observer-a")]
+const PSU_PWR_OK_NOTIF: [u32; PSU_COUNT] = [
+    notifications::PSU_PWR_OK_0_MASK,
+    notifications::PSU_PWR_OK_1_MASK,
+    notifications::PSU_PWR_OK_2_MASK,
+    notifications::PSU_PWR_OK_3_MASK,
+    notifications::PSU_PWR_OK_4_MASK,
+    notifications::PSU_PWR_OK_5_MASK,
+];

 /// In order to get the PMBus devices by PSU index, we need a little lookup table guy.
+#[cfg(any(target_board = "psc-b", target_board = "psc-c"))]
 const PSU_PMBUS_DEVS: [fn(TaskId) -> (I2cDevice, u8); PSU_COUNT] = [
    i2c_config::pmbus::v54_psu0,
    i2c_config::pmbus::v54_psu1,
@@ -326,6 +350,15 @@ const PSU_PMBUS_DEVS: [fn(TaskId) -> (I2cDevice, u8); PSU_COUNT] = [
    i2c_config::pmbus::v54_psu4,
    i2c_config::pmbus::v54_psu5,
 ];
+#[cfg(target_board = "observer-a")]
+const PSU_PMBUS_DEVS: [fn(TaskId) -> (I2cDevice, u8); PSU_COUNT] = [
+    i2c_config::pmbus::v50_main_psu0,
+    i2c_config::pmbus::v50_main_psu1,
+    i2c_config::pmbus::v50_main_psu2,
+    i2c_config::pmbus::v50_main_psu3,
+    i2c_config::pmbus::v50_main_psu4,
+    i2c_config::pmbus::v50_main_psu5,
+];


not a huge deal, but a fairly common practice in tasks that support multiple slightly different boards with mostly common code is to put all of the board specific stuff in a BSP (Board Support Package) submodule which is #[cfg(...)] flagged for the appropriate board, rather than having to put a bunch of #[cfg(...)] attributes on every definition. so something like this:

#[cfg(target_board = "observer-a")] mod bsp { pub(crate) const PSU_ENABLE_L_PORT = ...; pub(crate) const PSU_ENABLE_L_PINS = ...; pub(crate) const ALL_PSU_ENABLE_L_PINS = ...; pub(crate) const PSU_PWR_OK_NOTIF = ...; pub(crate) const PSU_PMBUS_DEVS = ...; } #[cfg(any(target_board = "psc-b", target_board = "psc-c"))] mod bsp { pub(crate) const PSU_ENABLE_L_PORT = ...; pub(crate) const PSU_ENABLE_L_PINS = ...; pub(crate) const ALL_PSU_ENABLE_L_PINS = ...; pub(crate) const PSU_PWR_OK_NOTIF = ...; pub(crate) const PSU_PMBUS_DEVS = ...; } use self::bsp::*;

this ends up being a little neater most of the time, IMO.

Another pattern is to have an explicit bsp module, then change its path depending on board, e.g.

#[cfg_attr( any(target_board = "minibar-a", target_board = "minibar-b"), path = "bsp/minibar.rs" )] #[cfg_attr( any(target_board = "cosmo-a", target_board = "cosmo-b"), path = "bsp/cosmo_ab.rs" )] mod bsp;

I created a bsp module, but it got a bit messy and might need some tweaks.

I duplicated the PRESENT and OK pin arrays in both files even though they happen to be the same on both boards. I think that's cleaner than interleaving shared and board-specific arrays, but maybe not.

The bsp files need to use the PSU_COUNT constant in their array definitions. I had them each define their own constant then I assert that's its equal to 6. I guess I could define a shared PSU_COUNT somewhere else instead.

The bsp files need to use the codegen'ed notifications and i2c_config modules, so I stuck include!(concat!(env!("OUT_DIR"), "/notifications.rs")); etc in both of them. Not sure how weird that is.

I duplicated the PRESENT and OK pin arrays in both files even though they happen to be the same on both boards. I think that's cleaner than interleaving shared and board-specific arrays, but maybe not.

I think this is fine; just because it happens to be the same on both boards doesn't mean it needs to be.

The bsp files need to use the PSU_COUNT constant in their array definitions. I had them each define their own constant then I assert that's its equal to 6. I guess I could define a shared PSU_COUNT somewhere else instead.

Hmm, I was going to say that we should actually not have to do this and that all the code should basically just work with arrays of any length, but then I remembered about the Slot enum which has six slot numbers. That kinda makes me think that it would be nicer to just define PSU_COUNT in main.rs and have the BSPs use super::PSU_COUNT, rather than asserting that every BSP uses 6, which feels a bit convoltued.

The bsp files need to use the codegen'ed notifications and i2c_config modules, so I stuck include!(concat!(env!("OUT_DIR"), "/notifications.rs")); etc in both of them. Not sure how weird that is.

This seems fine to me. You could also leave them in main.rs and have the BSPs use super::i2c_config and super::notifications, but since (at a glance) they are now basically just referenced in the BSP modules, making them belong to the BSP is also totally fine.

Ok I moved PSU_COUNT to main. I've tended to think of use super:: as a code-smell but I don't think that's actually true, and it's definitely an improvement in this situation.

I moved the codegen out too because main was using bsp::notifications::TIMER_MASK.

hawkw

One last thing. After that, I think this will be all good!

hawkw · 2026-06-10T19:15:38Z

+// Currently the psc-seq-server only supports boards with 6 PSU slots.
+const _: () = assert!(bsp::PSU_COUNT == 6);


As I mentioned, I think I'd really prefer we just define PSU_COUNT in main and make all the BSPs reference it as super::PSU_COUNT. Making the BSP define the constant and then asserting that it defined the right thing feels a bit goofy.

(also, the place where we initialize a [Slot; bsp::PSU_COUNT] to a 6-element array literal also serves as a slightly more implicit version of this assertion...but again, I think we should just not be asserting it)

hawkw

looks good to me! thank you for all your hard work on this!

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

evan-oxide added 2 commits June 9, 2026 17:33

Merge branch 'master' into evan/observer-pmbus

ec1157d

add mwocp67 to PMBUS_GENERATOR list

fd79687

evan-oxide mentioned this pull request Jun 9, 2026

Notch out known-unsupported PMBus status registers oxidecomputer/pmbus#35

Merged

evan-oxide commented Jun 9, 2026

View reviewed changes

Comment thread task/control-plane-agent/build.rs

update pmbus crate, for unsupported status registers

52698b9

hawkw reviewed Jun 10, 2026

View reviewed changes

mkeeter reviewed Jun 10, 2026

View reviewed changes

Comment thread task/sensor-polling/src/main.rs Outdated

improve comments

9f77c25

evan-oxide added 4 commits June 10, 2026 13:34

remove unused fixedstr dependency from i2c-devices

6f379c0

deduplicate match arms

d832acf

move board-specific definitions into bsp module

23c9810

add missing license

f9a65c0

evan-oxide requested a review from hawkw June 10, 2026 18:55

fix doc link

c429032

evan-oxide requested a review from mkeeter June 10, 2026 19:05

hawkw reviewed Jun 10, 2026

View reviewed changes

evan-oxide added 2 commits June 10, 2026 15:24

move PSU_COUNT into main

ef71efe

move codegen includes out of bsp files

581d878

mkeeter approved these changes Jun 10, 2026

View reviewed changes

hawkw approved these changes Jun 10, 2026

View reviewed changes

Merge branch 'master' into evan/observer-pmbus

85c3261

evan-oxide enabled auto-merge (squash) June 10, 2026 20:04

evan-oxide merged commit 3dc0535 into master Jun 10, 2026
190 checks passed

evan-oxide deleted the evan/observer-pmbus branch June 10, 2026 20:16

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

		// Currently the psc-seq-server only supports boards with 6 PSU slots.
		const _: () = assert!(bsp::PSU_COUNT == 6);

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

evan-oxide commented Jun 4, 2026 •

edited

Loading

evan-oxide Jun 4, 2026 •

edited

Loading