Automatically Load the hiffy_call result#664
Merged
Merged
Conversation
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
from
34d42b2 to
9899a0c
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
2 times, most recently
from
89d1b37 to
b3a0238
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
from
9899a0c to
76fc855
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
from
b3a0238 to
8b10ba0
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
from
76fc855 to
7c9ee88
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
from
8b10ba0 to
379e0b9
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
3 times, most recently
from
a98eb7d to
3aa54d7
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
from
511a371 to
356f019
Compare
labbott
reviewed
May 20, 2026
|
|
||
| let op = hubris.get_idol_command("Monorail.write_vsc7448_reg")?; | ||
| let value = humility_hiffy::hiffy_call( | ||
| humility_hiffy::hiffy_call::<()>( |
Contributor
There was a problem hiding this comment.
I don't think this is actually () because a match value statement got removed below
Contributor
Author
There was a problem hiding this comment.
The match expects a Base(U0), which is equivalent to (). Writing a random register works fine:
matt@niles ~ () $ pfexec ./humility -t sidecar-b monorail write PTP_DOM[0]:PTP_CLOCK_ID_LSB 0x123
humility: attached to 0483:3754:002600184D4B500E20373831 via ST-Link V3
humility: Writing 0x123 to ANA_ACL:PTP_DOM[0]:PTP_CLOCK_ID_LSB at 0x71434470
bits | value | field
31:0 | 0x123 | CLOCK_ID_LSB
matt@niles ~ () $ pfexec ./humility -t sidecar-b monorail read PTP_DOM[0]:PTP_CLOCK_ID_LSB
humility: attached to 0483:3754:002600184D4B500E20373831 via ST-Link V3
humility: Reading ANA_ACL:PTP_DOM[0]:PTP_CLOCK_ID_LSB from 0x71434470
ANA_ACL:PTP_DOM[0]:PTP_CLOCK_ID_LSB => 0x123
bits | value | field
31:0 | 0x123 | CLOCK_ID_LSB
Contributor
There was a problem hiding this comment.
Ahhh I parsed that as unsigned 0 but I see () now
| pretty_print_fields(value as u32, ®.fields, 0); | ||
| let op = hubris.get_idol_command("Monorail.write_phy_reg")?; | ||
| let value = humility_hiffy::hiffy_call( | ||
| humility_hiffy::hiffy_call::<()>( |
Contributor
There was a problem hiding this comment.
Another case where I'm not sure this is actually ()
Contributor
Author
There was a problem hiding this comment.
Same here, the match was looking for Base(U0), which is ():
matt@niles ~ () $ pfexec ./humility -t sidecar-b monorail phy write -p43 13 12
humility: attached to 0483:3754:002600184D4B500E20373831 via ST-Link V3
Writing 0xc to port 43 PHY, register STANDARD:MMD_EEE_ACCESS
matt@niles ~ () $ pfexec ./humility -t sidecar-b monorail phy read -p43 13
humility: attached to 0483:3754:002600184D4B500E20373831 via ST-Link V3
Reading from port 43 PHY, register STANDARD:MMD_EEE_ACCESS
Got result 0xc
Comment on lines
+1669
to
+1676
| let r = hiffy_call(hubris, core, context, &power_state_op, &[], None, None); | ||
| let v = match r { | ||
| Ok(r) => Ok(r), | ||
| Err(HiffyCallError::Hiffy(s)) => Err(s), | ||
| Err(HiffyCallError::Other(e)) => { | ||
| return Err(e.context("power state check")); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Vibes and doesn't need to be addressed here: I've seen this pattern with hiffy_call a few times now but I don't know if it can be made common
Contributor
Author
There was a problem hiding this comment.
Yeah, this is a bit wordier but I wanted to preserve the original behavior (which actually cares about the difference between a Humility error and an error reported by Hiffy).
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
from
356f019 to
a45b581
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
2 times, most recently
from
2f8ca39 to
8bba220
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
2 times, most recently
from
759562f to
a88a132
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
from
8bba220 to
2d71dee
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
from
a88a132 to
cecefd8
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
2 times, most recently
from
e0235e1 to
edfb6c2
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
2 times, most recently
from
e363734 to
ff617b0
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
2 times, most recently
from
9e5eb49 to
eced31a
Compare
mkeeter
force-pushed
the
mkeeter/generic-cells
branch
from
2856341 to
d10f467
Compare
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
from
eced31a to
1347d84
Compare
Contributor
Author
|
Going through and testing modified commands one-by-one:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the humility_hiffy call path to return typed decoded values (T: reflect::Load) directly from hiffy_call, while also collapsing the previously nested error shape into a single HiffyCallError enum. It also updates downstream callers and enhances the Load derive macro to support generic parameters and an optional reflected-type name check.
Changes:
- Change
hiffy_callto return a loadedT: reflect::Loadand introduceHiffyCallErrorto unify error handling. - Update callers across commands/crates to use typed
hiffy_call::<T>()/hiffy_decode::<T>()and simplify manualValueunpacking. - Extend
#[derive(Load)]to support generics and add#[load(check_name)]for runtime reflected-name validation (used by newly genericdoppelcell wrappers).
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| load_derive/src/lib.rs | Adds #[load(check_name)] support and generic-aware Load impl generation (with some correctness gaps noted). |
| humility-spd/src/lib.rs | Switches packrat buffer reads to typed reflect::read_variable and generic ClaimOnceCell. |
| humility-hiffy/src/lib.rs | Introduces HiffyCallError; makes hiffy_call/hiffy_decode generic over T: Load. |
| humility-hiffy/Cargo.toml | Removes unused parse_int dependency from humility-hiffy. |
| humility-doppel/src/lib.rs | Makes ClaimOnceCell/StaticCell/UnsafeCell/MaybeUninit generic and adds #[load(check_name)]. |
| humility-core/src/reflect.rs | Moves Load for Array impl earlier (no functional change). |
| humility-auxflash/src/lib.rs | Updates auxflash operations to use typed hiffy_call and HiffyCallError matching. |
| cmd/tofino-eeprom/src/lib.rs | Updates EEPROM read/write loops to typed hiffy_call::<()>(). |
| cmd/ringbuf/src/lib.rs | Updates StaticCell usage to StaticCell<Ringbuf>. |
| cmd/rendmp/src/lib.rs | Updates blackbox dump and worker decode path for typed hiffy_call/hiffy_decode. |
| cmd/qspi/src/lib.rs | Updates QSPI calls to typed hiffy_call::<()>() and simplifies success handling. |
| cmd/net/src/lib.rs | Updates network commands to typed hiffy_call / hiffy_decode::<Struct> and simplifies parsing. |
| cmd/monorail/src/lib.rs | Updates monorail reads/writes and status decoding to typed hiffy_call / hiffy_decode::<Struct>. |
| cmd/host/src/lib.rs | Updates host buffer reads and post-code operations to typed hiffy_call. |
| cmd/hiffy/src/lib.rs | Adapts CLI call path to new HiffyCallError while preserving “return_code” printing behavior. |
| cmd/console-proxy/src/posix.rs | Updates UART operations to typed hiffy_call and removes now-unused imports/logic. |
| Cargo.lock | Removes parse_int from humility-hiffy dependency list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+59
| fn has_check_name(attrs: &[syn::Attribute]) -> bool { | ||
| attrs.iter().any(|a| { | ||
| if !a.path().is_ident("load") { | ||
| return false; | ||
| } | ||
| let mut found = false; | ||
| let _ = a.parse_nested_meta(|meta| { | ||
| if meta.path.is_ident("check_name") { | ||
| found = true; | ||
| } | ||
| Ok(()) | ||
| }); | ||
| found |
| }; | ||
| let v = hiffy_decode(self.hubris, op, value) | ||
| let v = hiffy_decode::<Base>(self.hubris, op, value) | ||
| .context("failed to decode {op:?} result")?; |
labbott
approved these changes
May 21, 2026
Contributor
labbott
left a comment
labbott
left a comment
There was a problem hiding this comment.
We love a net negative PR 😎
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
2 times, most recently
from
b5d57e1 to
b40ed6c
Compare
Contributor
Author
|
Copilot review also looked at the tail end of the previous PR (#663), so those can be ignored. |
mkeeter
force-pushed
the
mkeeter/hiffy-call-unpack
branch
from
b40ed6c to
cc28c4f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(staged on #663)
This PR changes the return type from
hiffy_callfromResult<Result<Value, String>>to aResult<T: Load, HiffyCallError>. There are two changes here:T: Load(instead of returning aValue)enum HiffyCallError, which represents both casesThis ends up being a bunch of churn, but is a significant cleanup overall.