From 24d7716a5d2e24880628873e67891eddc929227a Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 7 May 2026 14:33:57 -0400 Subject: [PATCH] Make hiffy_decode return a non-nested error --- cmd/hiffy/src/lib.rs | 4 +- cmd/host/src/lib.rs | 10 ++--- cmd/monorail/src/lib.rs | 76 ++++++++++++++---------------------- cmd/net/src/lib.rs | 2 +- cmd/powershelf/src/lib.rs | 10 ++++- cmd/rendmp/src/lib.rs | 20 +++++----- humility-auxflash/src/lib.rs | 6 +-- humility-hiffy/src/lib.rs | 35 +++++++---------- 8 files changed, 73 insertions(+), 90 deletions(-) diff --git a/cmd/hiffy/src/lib.rs b/cmd/hiffy/src/lib.rs index eb5bbb3c3..238dfa92c 100644 --- a/cmd/hiffy/src/lib.rs +++ b/cmd/hiffy/src/lib.rs @@ -322,8 +322,8 @@ fn hiffy(context: &mut ExecutionContext) -> Result<()> { output.as_deref_mut(), ) { Ok(s) => Ok(s), - Err(HiffyCallError::Hiffy(s)) => Err(s), - Err(HiffyCallError::Other(e)) => return Err(e), + Err(HiffyError::Hiffy(s)) => Err(s), + Err(HiffyError::Other(e)) => return Err(e), }, output, ) diff --git a/cmd/host/src/lib.rs b/cmd/host/src/lib.rs index e953ef2bc..adff82210 100644 --- a/cmd/host/src/lib.rs +++ b/cmd/host/src/lib.rs @@ -100,7 +100,7 @@ //! # etc... //! ``` -use anyhow::{Result, anyhow, bail}; +use anyhow::{Result, anyhow}; use chrono::DateTime; use clap::{CommandFactory, Parser}; @@ -369,9 +369,6 @@ fn host_post_codes( let op = hubris.get_idol_command("Sequencer.get_post_code")?; let handle_value = |v| { - let Ok(reflect::Value::Base(reflect::Base::U32(v))) = v else { - bail!("Got bad value from get_post_code: expected U32, got {v:?}"); - }; if raw { println!("{v:08x}"); } else { @@ -379,7 +376,6 @@ fn host_post_codes( let detail = decoded.lines().join("\n"); println!("{detail}"); } - Ok(()) }; let send = context.get_function("Send", 4)?; @@ -422,8 +418,8 @@ fn host_post_codes( ops.push(Op::Done); // Finish for r in context.run(core, ops.as_slice(), None)? { - let v = humility_hiffy::hiffy_decode(hubris, &op, r)?; - handle_value(v)?; + let v = humility_hiffy::hiffy_decode::(hubris, &op, r)?; + handle_value(v); } } Ok(()) diff --git a/cmd/monorail/src/lib.rs b/cmd/monorail/src/lib.rs index 01283642d..c859ad80b 100644 --- a/cmd/monorail/src/lib.rs +++ b/cmd/monorail/src/lib.rs @@ -172,10 +172,10 @@ use humility::core::Core; use humility::hubris::*; use humility::reflect::*; use humility_cli::ExecutionContext; -use humility_hiffy::HiffyContext; +use humility_hiffy::{HiffyContext, HiffyError}; use humility_idol::{HubrisIdol, IdolArgument}; -use anyhow::{Result, anyhow, bail}; +use anyhow::{Result, anyhow}; use clap::{CommandFactory, Parser}; use colored::Colorize; @@ -600,14 +600,9 @@ fn monorail_dump( results .into_iter() .map(move |r| humility_hiffy::hiffy_decode(hubris, &op_read, r)) - .collect::>>>()? + .collect::, _>>()? }; - for (i, v) in results.iter().enumerate() { - let value = if let Ok(Value::Base(Base::U32(v))) = v { - v - } else { - bail!("Got bad reflected value: expected U32, got {v:?}"); - }; + for (i, value) in results.iter().enumerate() { let addr = format!("{}", start_address as usize + i * 4); // XXX this is inefficient match parse_reg_or_addr(&addr) { @@ -684,7 +679,7 @@ fn monorail_status( hubris, &op_port, r, ) }) - .collect::>>>()?; + .collect::>(); let phy_results = phy_results .into_iter() .map(move |r| { @@ -692,7 +687,7 @@ fn monorail_status( hubris, &op_phy, r, ) }) - .collect::>>>()?; + .collect::>(); // Decode the port and phy status values into reflect::Value port_results.into_iter().zip(phy_results).collect::>() @@ -717,19 +712,10 @@ fn monorail_status( v => panic!("Expected enum, got {:?}", v), }; // Extracts a device name from a reflected value, e.g. "DEV1G_0" - let decode_dev = |value: &Value| match value { - Value::Tuple(dev) => { - let d = match &dev[0] { - Value::Enum(d) => d.disc(), - d => panic!("Could not get enum from {:?}", d), - }; - let n = match &dev[1] { - Value::Base(Base::U8(n)) => n, - d => panic!("Could not get U8 from {:?}", d), - }; - format!("{}_{}", d.to_uppercase(), n) - } - dev => panic!("Expected tuple, got {:?}", dev), + let decode_dev = |dev: &Tuple| -> Result<_> { + let d = dev.field::(0)?; + let n = dev.field::(1)?; + Ok(format!("{}_{}", d.disc().to_uppercase(), n)) }; let fmt_link = |v: &Value| match v { @@ -759,20 +745,18 @@ fn monorail_status( match port_value { Ok(s) => { assert_eq!(s.name(), "PortStatus"); - let (dev, serdes, mode, speed) = match &s["cfg"] { - Value::Struct(cfg) => { - assert_eq!(cfg.name(), "PortConfig"); - let dev = decode_dev(&cfg["dev"]); - let serdes = decode_dev(&cfg["serdes"]); - let (mode, speed) = decode_mode(&cfg["mode"]); - ( - dev.replace("DEV", ""), - serdes.replace("SERDES", ""), - mode, - speed, - ) - } - v => panic!("Expected Struct, got {:?}", v), + let (dev, serdes, mode, speed) = { + let cfg = s.field::("cfg")?; + assert_eq!(cfg.name(), "PortConfig"); + let dev = decode_dev(&cfg.field("dev")?)?; + let serdes = decode_dev(&cfg.field("serdes")?)?; + let (mode, speed) = decode_mode(&cfg["mode"]); + ( + dev.replace("DEV", ""), + serdes.replace("SERDES", ""), + mode, + speed, + ) }; let fmt_mode = match mode.as_str() { "SGMII" => mode.cyan(), @@ -791,7 +775,9 @@ fn monorail_status( ) } Err(e) => { - if e == "UnconfiguredPort" { + if let HiffyError::Hiffy(e) = &e + && e == "UnconfiguredPort" + { print!( "{}", "-- -- -- -- -- ".dimmed() @@ -805,19 +791,17 @@ fn monorail_status( match phy_value { Ok(s) => { assert_eq!(s.name(), "PhyStatus"); - let phy_ty = match &s["ty"] { - Value::Enum(e) => e.disc().to_uppercase(), - v => panic!("Expected struct, got {:?}", v), - }; println!( "{:<6} {:<8} {:<10}", - phy_ty, + s.field::("ty")?.disc().to_uppercase(), fmt_link(&s["mac_link_up"]), fmt_link(&s["media_link_up"]), ) } Err(e) => { - if e == "UnconfiguredPort" || e == "NoPhy" { + if let HiffyError::Hiffy(e) = &e + && (e == "UnconfiguredPort" || e == "NoPhy") + { println!("{}", "-- -- --".dimmed()); } else { println!("Got unexpected error {e}"); @@ -885,7 +869,7 @@ fn monorail_mac_table( hubris, &op, r, ) }) - .collect::>>>()?; + .collect::>>(); let mut mac_table: BTreeMap> = BTreeMap::new(); for r in results { diff --git a/cmd/net/src/lib.rs b/cmd/net/src/lib.rs index 7e74c5d97..64cf47e4e 100644 --- a/cmd/net/src/lib.rs +++ b/cmd/net/src/lib.rs @@ -204,7 +204,7 @@ fn net_mac_table( hubris, &op, r, ) }) - .collect::>>>()?; + .collect::>(); let mut mac_table: BTreeMap> = BTreeMap::new(); for r in results { diff --git a/cmd/powershelf/src/lib.rs b/cmd/powershelf/src/lib.rs index 0d64a39c4..0ebf15206 100644 --- a/cmd/powershelf/src/lib.rs +++ b/cmd/powershelf/src/lib.rs @@ -164,7 +164,15 @@ fn powershelf_run(context: &mut ExecutionContext) -> Result<()> { let results = context.run(core, ops.as_slice(), None)?; for (ndx, variant) in operation.variants.iter().enumerate() { - let result = hiffy_decode(hubris, &idol_cmd, results[ndx].clone())?; + let result = match hiffy_decode::( + hubris, + &idol_cmd, + results[ndx].clone(), + ) { + Ok(s) => Ok(s), + Err(HiffyError::Hiffy(s)) => Err(s), + Err(HiffyError::Other(e)) => return Err(e), + }; println!( "{:<20} => {}", diff --git a/cmd/rendmp/src/lib.rs b/cmd/rendmp/src/lib.rs index ad3cde1aa..af8846498 100644 --- a/cmd/rendmp/src/lib.rs +++ b/cmd/rendmp/src/lib.rs @@ -1208,11 +1208,7 @@ fn get_pin_states( // Decode Hiffy results let mut values = vec![]; for r in results { - match hiffy_decode(hubris, &op, r)? { - Err(e) => bail!("hiffy error: {e}"), - Ok(Value::Base(Base::U32(b))) => values.push(b), - v => bail!("unexpected type in result: {v:?}"), - } + values.push(hiffy_decode::(hubris, &op, r)?); } let phases = dev.phases(); @@ -1617,8 +1613,7 @@ impl<'a, 'b> HifWorker<'a, 'b> { Call::WriteByte => &self.write_byte, Call::WriteWord32 => &self.write_word32, }; - let v = hiffy_decode::(self.hubris, op, value) - .with_context(|| format!("failed to decode {call:?} result"))?; + let v = hiffy_decode::(self.hubris, op, value); match v { Ok(v) => match (v, call) { (Base::U32(v), Call::ReadDma(..)) => { @@ -1647,7 +1642,12 @@ impl<'a, 'b> HifWorker<'a, 'b> { bail!("got unexpected result {base} for {op:?}") } }, - Err(e) => out.push(Err(e)), + Err(HiffyError::Hiffy(e)) => out.push(Err(e)), + Err(HiffyError::Other(e)) => { + return Err(e).with_context(|| { + format!("failed to decode {call:?} result") + }); + } } } Ok(out) @@ -1669,8 +1669,8 @@ fn rendmp_phase_check<'a>( 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)) => { + Err(HiffyError::Hiffy(s)) => Err(s), + Err(HiffyError::Other(e)) => { return Err(e.context("power state check")); } }; diff --git a/humility-auxflash/src/lib.rs b/humility-auxflash/src/lib.rs index 2fb340148..94529eef0 100644 --- a/humility-auxflash/src/lib.rs +++ b/humility-auxflash/src/lib.rs @@ -6,7 +6,7 @@ use indicatif::{ProgressBar, ProgressStyle}; use humility::core::Core; use humility::hubris::*; -use humility_hiffy::{HiffyCallError, HiffyContext}; +use humility_hiffy::{HiffyContext, HiffyError}; use humility_idol::{HubrisIdol, IdolArgument}; use std::time::Duration; @@ -72,7 +72,7 @@ impl<'a> AuxFlashHandler<'a> { ); match value { Ok(v) => Ok(Some(v)), - Err(HiffyCallError::Hiffy(s)) if s == "NoActiveSlot" => Ok(None), + Err(HiffyError::Hiffy(s)) if s == "NoActiveSlot" => Ok(None), Err(e) => Err(e.into()), } } @@ -109,7 +109,7 @@ impl<'a> AuxFlashHandler<'a> { ); match value { Ok(v) => Ok(Some(v.0)), - Err(HiffyCallError::Hiffy(e)) if e == "MissingChck" => Ok(None), + Err(HiffyError::Hiffy(e)) if e == "MissingChck" => Ok(None), Err(e) => Err(e.into()), } } diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index d3589aad2..b9d9ade0a 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -1440,7 +1440,7 @@ fn has_task_started( /// Error returned from [`hiffy_call`] #[derive(thiserror::Error, Debug)] -pub enum HiffyCallError { +pub enum HiffyError { /// A function called in the HIF program returned an error #[error("hiffy error: {0}")] Hiffy(String), @@ -1458,7 +1458,7 @@ pub fn hiffy_call( args: &[(&str, idol::IdolArgument)], lease_write: Option<&[u8]>, lease_read: Option<&mut [u8]>, -) -> Result { +) -> Result { check_op(op)?; check_leases(op, lease_write, lease_read.as_deref())?; @@ -1496,7 +1496,7 @@ pub fn hiffy_call( let mut results = context.run(core, ops.as_slice(), lease_write)?; if results.len() != 1 { - return Err(HiffyCallError::Other(anyhow!( + return Err(HiffyError::Other(anyhow!( "unexpected results length: {:?}", results ))); @@ -1504,21 +1504,16 @@ pub fn hiffy_call( let mut v: Result, IpcError> = results.pop().unwrap(); - // If this is a Read operation, steal extra data from the returned stack - // and copy it into the incoming 'read' argument - let out = match lease_read { - Some(data) => { - let ok_size = op.reply_size()?; - if let Ok(v) = v.as_mut() { - let extra_data = v.drain(ok_size..).collect::>(); - data.copy_from_slice(&extra_data); - } - // Shoehorn that extra data in, assuming decoding worked. - hiffy_decode(hubris, op, v)? - } - _ => hiffy_decode(hubris, op, v)?, - }; - out.map_err(HiffyCallError::Hiffy) + // If this is a successful Read operation, steal extra data from the + // returned stack and copy it into the incoming 'read' argument + if let Ok(v) = v.as_mut() + && let Some(data) = lease_read + { + let ok_size = op.reply_size()?; + let extra_data = v.drain(ok_size..).collect::>(); + data.copy_from_slice(&extra_data); + } + hiffy_decode(hubris, op, v) } /// Decodes a value returned from [hiffy_call] or equivalent. @@ -1529,7 +1524,7 @@ pub fn hiffy_decode( hubris: &HubrisArchive, op: &idol::IdolOperation, val: Result, impl Into>, -) -> Result> { +) -> Result { let r = match val.map_err(Into::into) { Ok(val) => { let ty = hubris.lookup_type(op.ok).unwrap(); @@ -1565,7 +1560,7 @@ pub fn hiffy_decode( }, Err(dead @ IpcError::ServerDied(_)) => Err(format!("<{dead}>")), }; - Ok(r) + r.map_err(HiffyError::Hiffy) } pub fn hiffy_format_result(