Make psc-seq-server work without the enable pins#2601
Conversation
| /// Clears faults and status registers, allowing the PSU to resume operation | ||
| /// if it was latched off due to a fault. | ||
| pub fn clear_faults_and_latch(&self) -> Result<(), Error> { | ||
| let mut data = pmbus_read!(self.device, MB_PSU_SETTING)?; | ||
| data.set_clear_faults(MB_PSU_SETTING::ClearFaults::Clear); | ||
| pmbus_write!(self.device, MB_PSU_SETTING, data) | ||
| } |
There was a problem hiding this comment.
Just double-checking, we're sure this command isn't paged, right? The PMBus standard's CLEAR_FAULTS is paged, and...silently does Not What You Expect if you don't send a page code. I presume the Murata-specific one actually doesn't expect a page, but I wanted to double-check.
There was a problem hiding this comment.
Yup the datasheet says that CLEAR_FAULTS is paged but MB_PSU_SETTING is not, that's why I picked it. And it has been working without us setting a page, so I think we're good.
| /// The unused arguments are needed by other models of PSU. | ||
| pub fn do_action( | ||
| action: super::ActionRequired, | ||
| _sys: &sys_api::Sys, | ||
| psu_index: usize, | ||
| dev: &mut Mwocp67, | ||
| now: u64, |
There was a problem hiding this comment.
it occurs to me that if we're irritated by having to pass different arguments depending on which BSP we're calling into, we could let the BSP define its own type which has the same methods but consists of different values, or something. Not sure if it's worth torturing the code over though.
There was a problem hiding this comment.
I don't think that's worth it. It would move the irritation to wherever we construct the different structs for each board.
| /// How long to wait after a PSU is inserted, before we attempt to turn it on | ||
| /// (MWOCP68 only). This does double-duty in both debouncing the presence line, | ||
| /// and ensuring that things are firmly mated before activating anything. | ||
| const INSERT_DEBOUNCE_MS: u64 = 1_000; // Current value is somewhat arbitrary. |
There was a problem hiding this comment.
Nitpicky: perhaps we ought to rename this to MWOCP68_INSERT_DEBOUNCE_MS or something, if it's only used on the '68?
There was a problem hiding this comment.
The state machine does wait for the debounce time for both PSUs, it's just that the MWOCP67 is automatically enabled before the debounce period and the MWOCP68 is manually enabled after. I'll make the comment clearer.
| /// The PSU is enabled, as in the `On` state, but we're not convinced the | ||
| /// PSU is okay. We enter this state when the PSU is first inserted and when | ||
| /// bringing a PSU out of an observed fault state, and it causes us to | ||
| /// ignore its OK output for a brief period (the deadline parameter, | ||
| /// initialized as current time plus `PROBATION_MS`). | ||
| /// | ||
| /// We do this because PSUs have been observed, in practice, taking up to | ||
| /// ~100ms to assert OK after being enabled. | ||
| /// We do this because PSUs have been observed, in practice, taking up to 2s | ||
| /// to assert OK after being enabled. The MWOCP67 is much slower to assert | ||
| /// OK than the MWOCP68 is. | ||
| /// | ||
| /// Once the deadline elapses, we'll transition to the `On` state and start | ||
| /// requiring OK to be asserted. | ||
| OnProbation { deadline: u64 }, | ||
| OnProbation { | ||
| deadline: u64, | ||
| reason: ProbationReason, | ||
| }, |
There was a problem hiding this comment.
Hm, I think I get why we're using the OnProbation state to represent the "newly inserted, waiting for PSU to assert PWR_OK of its own free will", rather than the NewlyInserted state --- we want behavior more like the "probation" behavior than the "wait for MWOCP68 pin debounce" behavior. But it feels weird to not represent this using the NewlyInserted state. I'm not convinced this code isn't the right thing, for the record, it just...makes me feel weird about the semantic meaning of and relationship between these two states...
There was a problem hiding this comment.
I was thinking about that too. We could extend the time spent in the NewlyInserted state (INSERT_DEBOUNCE_MS = 4000) then transition directly to On, instead of going NewlyInserted -> OnProbation -> On. It would work correctly for the mwocp67, but the downsides are:
- After you plug in an mwocp68, it would sit idle for 4s before turning on. That's not terrible, but might be inconvenient or confusing for an operator.
- That approach (and the old code) don't leave any grace period between when the mwocp68 is enabled on insertion and when it's
PWR_OKis required to be asserted. Apparently that's been working fine - eitherPWR_OKis always asserted very quickly, or we just haven't noticed that sometimes a hot-inserted mwocp68 triggers a fault recovery loop and takes an extra 5s to turn on. But usingOnProbationmeans that both PSU types get a grace period, which seems nice and robust.
In terms of the semantic meanings of the states, in an ideal world I think the meanings would be:
NewlyInserted: debouncing the contacts, not yet enabled.OnProbation: enabled, not yet monitoringPWR_OK.On: enabled, requiringPWR_OKto be asserted.
Of course the mwocp67 is enabled during NewlyInserted, but I'm treating that as an unfortunate accident. I think those definitions are still better and more consistent than:
NewlyInserted:- mwocp68: debouncing the contacts, not yet enabled.
- mwocp67: enabled, not yet monitoring
PWR_OK.
On: enabled, requiringPWR_OKto be asserted.
| /// stable before turning it on. (Waiting in this state provides some | ||
| /// debouncing for contact scrape.) | ||
| /// stable before turning it on (MWOCP68 only). (Waiting in this state | ||
| /// provides some debouncing for contact scrape.) |
There was a problem hiding this comment.
Can we maybe explain in this comment the reason this behavior is MWOCP68 only? I realize it's because the '67 tries to turn itself on immediately, but it might be nice if the doc said that a bit more explicitly.
There was a problem hiding this comment.
I ended up making it less specific, does that work?
2 participants
The PSC sequencer enables and disables the old MWOCP68 PSUs by toggling GPIOs. Unfortunately that doesn't work on the Observer's new MWOCP67 PSUs, so we need to use pmbus commands instead. This PR moves the now hardware-specific enable/disable code into the BSP modules.
I found another difference during testing: the MWOCP67 takes much longer to assert it's
PWR_OKsignal after being enabled or inserted. It often takes 1-2s, whereas code comments say that the MWOCP68 only took 100ms. This meant that when the sequencer re-enabled the PSU after clearing a fault,PWR_OKwould stay de-asserted for too long and the sequencer would think the fault condition was still present. I fixed this by increasing the length of the probation period at the end of fault recovery, and adding a similar probation period after insertion.This closes #2564
Testing
Tested with a fault-injection setup (ie. asking Eric to repeatedly dead-short the main rail).
Observer + MWOCP67:
set_enableis hardcoded to return an error on alternating calls, fault recovery works)do_actionfails, it does eventually recoverset_enablecalls are hardcoded to return an error, fault recovery eventually works)PSC + MWOCP68: