RAA229620A phase check and open pin support#591
Conversation
This is preparing for adding more variants to SupportedDevices. The list of SupportedDevices was present, explicitly or implicitly, in several places in the code: - The enum definition itself - A list of strings used to filter device names - A set of match statements used to convert those strings into variants of the enum - Error messages listing support device types, which hardcoded the variant names. - A Display impl that repeated the names. This was noisy and potentially error-prone. I have simplified it by applying the `strum` crate to derive traits for the SupportedDevice macro: - `FromStr` (derived by EnumString) eliminates all handrolled parsing and match code - `VariantNames` (derived by EnumVariantNames) allows the error messages to list expected names without hardcoding them - `Display` can now be derived automatically. I chose `strum` because of its option to make string comparisons case-insensitive, which neatly covers this command's use case of converting lowercase manifest device names into uppercase enum variants.
cbiffle
force-pushed
the
cbiffle/cosmo-rendmp
branch
from
13112e2 to
e3c0a76
Compare
cbiffle
force-pushed
the
cbiffle/cosmo-rendmp
branch
from
6560468 to
a9a9bcd
Compare
cbiffle
changed the title
| bail!("rail {rail} is not a supported device"); | ||
| } | ||
| let Ok(dev) = SupportedDevice::from_str(&d.device) else { | ||
| let supported = SupportedDevice::VARIANTS.join(", "); |
There was a problem hiding this comment.
it's from https://docs.rs/strum_macros/0.25.3/strum_macros/derive.EnumVariantNames.html , which I found in our existing dependency graph (the cheapest place to shop for crates)
We're on quite an old version of strum, it's been renamed to just VariantNames in the past couple of years. But, useful macro.
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
seems good to me. couple little comments
| // The 20 here looks like a copy paste mistake from the | ||
| // '618, but is deliberate; we believe the bit offset here | ||
| // is the same as the '618 despite the smaller phase count. |
There was a problem hiding this comment.
we believe
...why do we believe this?
| warn!( | ||
| "DIMM presence check disabled, if this check hangs, remove the \ | ||
| DIMMs and retry." | ||
| ); |
There was a problem hiding this comment.
this is confusing to me: this says "DIMM presence check disabled, if this check hangs, remove the DIMMS and retry". after thinking about it for a minute or two, i realized that "DIMM presence check" means the thing that would skip the phase check if DIMMs are present, but it sounds like it's saying, "the check i am about to perform has been disabled. if the check i am about to perform, which i just told you was disabled, hangs..." which feels weird.
There was a problem hiding this comment.
Hmmmm, yes, I should reword this.
3 participants
This appears to work on Cosmo.