Move hiffy / Idol decoding to a single place

[mkeeter]

Previously, we had two different functions to go from a Result<Vec<u8>, IpcError> to a decoded type: idol_result and hiffy_decode. This PR removes them both in favor of a decode function on IdolOperation:

// Old (two different ways of decoding, somewhat verbose):
hiffy_decode::<u32>(hubris, &op, r)?
context.idol_result::<u32>(&op, &r)?;
// New (one way to decode, shorter):
op.decode::<u32>(&r)?;

The IdolOperation already has a handle to the &HubrisArchive, so we can remove that argument from hiffy_call.

The only significant change is in error design: we now have a strongly-typed IdolDecodeError, which separates "you're holding it wrong" (IdolDecodeError::Failed) from "Idol returned an error" (IdolDecodeError::Idol). The latter is strongly-typed all the way down; the former wraps anyhow::Error in a few places.

[hawkw]

overall, i like this change! i had a few smallish suggestions.

[hawkw]

how do you feel about naming this something like

Suggested change

	/// Decoding the result failed in Humility
	#[error(transparent)]
	Failed(#[from] IdolDecodeFailed),
	/// Decoding the result failed in Humility
	#[error(transparent)]
	DecodeFailed(#[from] IdolDecodeFailed),

instead? Failed feels a bit unclear to me --- it could just as easily mean "the idol call failed", which is the opposite of what this one means...

[mkeeter]

Seems reasonable, done!

[hawkw]

hm, since we are returning errors in all the other "you're holding it wrong" cases, i kind of wonder if we might want to return another error variant here to say "the type you tried to decode the thing to couldn't be found"? maybe not, I dunno.

[mkeeter]

I decided to just call self.hubris.lookup_type in the constructor and store the value in the IdolOperation, making this a moot point.

[hawkw]

i wonder a bit if we might want to have separate error variants for Ssmarshal and hubpack here so that we can tell you which encoding we were trying to use? or, does the error from humility::reflect::deserialize_value already tell you that?

[mkeeter]

I decided to instead glomp everything in a DeserializeFailed variant, then include the actual Idol encoding.

[hawkw]

to be honest, i found the use of out here a little hard to follow, i only noticed that after seeing that in the IdleErrorType::None case, there was an explicit return. Part of me wants to say, why don't we just make all the paths that return an IdolErrorexplicitly wrap that inIdolDecodeError::Idol` for clarity, even if it's much uglier-looking? i dunno...

[mkeeter]

Done (slightly differently, by just calling .into() everywhere)

[hawkw]

Thanks for addressing the view feedback, this looks great!

[labbott]

LGTM I'll let others take a pass before I approve

[Copilot]

Pull request overview

Consolidates two redundant Idol result-decoding helpers (HiffyContext::idol_result and hiffy_decode) into a single IdolOperation::decode method that leverages the &HubrisArchive already held by IdolOperation. A new strongly-typed IdolDecodeError (with Idol/DecodeFailed variants) and IdolError enum replace the previous String-based error reporting, allowing callers to pattern-match on specific error names rather than comparing stringified messages. The hubris argument is also dropped from hiffy_call and related call sites since it can be obtained via op.

Changes:

• Introduce IdolOperation::decode plus strongly-typed IdolDecodeError/IdolError/IdolDecodeFailed enums, and rename the lifetime-bearing IdolError<'a> to IdolErrorType<'a>.
• Remove hiffy_decode and HiffyContext::idol_result, and drop the redundant hubris parameter from hiffy_call.
• Update all callers (auxflash, hiffy, host, monorail, net, powershelf, qspi, rendmp, sbrmi, tofino-eeprom, validate, vpd, console-proxy) to use the new API, including pattern-matching on IdolError::Named(...) instead of comparing strings.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
humility-idol/src/lib.rs	Adds decode/decode_error on IdolOperation, new typed error enums, renames IdolError → IdolErrorType, caches ok_ty.
humility-idol/Cargo.toml	Adds thiserror dependency for new error types.
humility-hiffy/src/lib.rs	Removes hiffy_decode/idol_result, drops hubris arg from hiffy_call, changes HiffyError::Hiffy to wrap IdolError, generalizes formatting helpers over any Display error.
humility-auxflash/src/lib.rs	Updates hiffy_call call sites and matches on IdolError::Named for "NoActiveSlot"/"MissingChck".
cmd/vpd/src/lib.rs	Replaces idol_result calls with op.decode.
cmd/validate/src/lib.rs	Updates IdolError::CLike → IdolErrorType::CLike.
cmd/tofino-eeprom/src/lib.rs	Drops hubris arg from hiffy_call.
cmd/sbrmi/src/lib.rs	Switches to op.decode.
cmd/rendmp/src/lib.rs	Removes stored hubris, switches to op.decode, returns IdolError instead of String, matches on new IdolDecodeError variants.
cmd/qspi/src/lib.rs	Drops hubris arg from hiffy_call.
cmd/powershelf/src/lib.rs	Switches to idol_cmd.decode, matches on IdolDecodeError, passes &result to formatter.
cmd/net/src/lib.rs	Updates hiffy_call/hiffy_decode call sites to new API.
cmd/monorail/src/lib.rs	Updates calls to new API and matches IdolError::Named for "UnconfiguredPort"/"NoPhy".
cmd/host/src/lib.rs	Drops hubris arg from hiffy_call, uses op.decode.
cmd/hiffy/src/lib.rs	Updates IdolError → IdolErrorType matches, drops hubris arg, passes &return_code to hiffy_print_result.
cmd/console-proxy/src/posix.rs	Drops hubris arg from hiffy_call.
Cargo.lock	Adds thiserror to humility-idol dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.