Add PMBus status retrieval to MGS API#497
Conversation
This is a private type that is intended to be shared across wire types that act as a null-padded fixed length container format. This is intended to be used in a follow up PR for supporting oxidecomputer/hubris#2463, but this is separated to make reviewing this helper easier.
Cleanup NullStr a bit, add basic tests Add command APIs
jamesmunns
force-pushed
the
james/nullstr-pt1
branch
2 times, most recently
from
468144f to
604614c
Compare
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
Overall, this seems like a good start, I left a few suggestions.
Also, it would be nice to add support for this to faux-mgs. If you want to be fancy, it would be really cool if the non-JSON output for the faux-mgs command pretty-printed the decoded value of each register using the pmbus crate, in addition to printing the raw value of the register. That would probably make life much easier for the folks who end up using the MGS command to debug stuff. Feel free to steal from the pretty-printer I wrote for erebor and/or the humility pmbus command, as you see fit (though note that humility pmbus' pretty-printer is way more complex, as it also decodes PMBus registers that are not always just bitflags, as the status registers are)...
jamesmunns
force-pushed
the
james/nullstr-pt1
branch
from
604614c to
d06ea64
Compare
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
This looks good. I had some smallish suggestions.
| lines.push(format!("Rail '{name}':")); | ||
| lines.push(format!("WORD 0x{status_word:04x}")); | ||
| let data = [ | ||
| ("VOUT ", status_vout), | ||
| ("IOUT ", status_iout), | ||
| ("TEMPERATURE ", status_temperature), | ||
| ("CML ", status_cml), | ||
| ("OTHER ", status_other), | ||
| ("INPUT ", status_input), | ||
| ("MFR_SPECIFIC ", status_mfr_specific), | ||
| ("FANS_1_2 ", status_fans_1_2), | ||
| ("FANS_3_4 ", status_fans_3_4), | ||
| ]; | ||
|
|
||
| use std::fmt::Write; | ||
| for (n, v) in data { | ||
| let mut l = n.to_string(); | ||
| match v { | ||
| Ok(b) => write!(&mut l, "0x{b:02x}")?, | ||
| Err(e) => write!(&mut l, "Err({e:?})")?, | ||
| } | ||
| lines.push(l); | ||
| } |
There was a problem hiding this comment.
this is cute! in future it might be nice to hook this onto the pmbus crate to decode the bit patterns, but i very much do not care about doing it in this PR.
| /// Received unexpected data on the I2C bus | ||
| InvalidBusData, |
There was a problem hiding this comment.
if we stop trying to interpret the status bits and then turn the m back into u8s, i think we can just get rid of this?
| if let Some(s) = self.as_str() { | ||
| debug.field("name", &s); | ||
| } else { | ||
| debug.field("name", self.name.contents()); |
There was a problem hiding this comment.
i kinda wonder if NullStr should just have a Debug impl that does this, so that we can derive Debug for PowerRailName and SpComponent?
There was a problem hiding this comment.
Will do this, that's a good idea.
2 participants
Follow-on PR to #496.
Part of oxidecomputer/hubris#2463.