Make hiffy_decode return a non-nested error

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,
             )

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,17 +369,13 @@ 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 {
             let decoded = turin_post_decoder::decode(v);
             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::<u32>(hubris, &op, r)?;
+            handle_value(v);
         }
     }
     Ok(())

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::<Result<Vec<Result<_, _>>>>()?
+            .collect::<Result<Vec<u32>, _>>()?
     };
-    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,15 +679,15 @@ fn monorail_status(
                     hubris, &op_port, r,
                 )
             })
-            .collect::<Result<Vec<Result<_, _>>>>()?;
+            .collect::<Vec<_>>();
         let phy_results = phy_results
             .into_iter()
             .map(move |r| {
                 humility_hiffy::hiffy_decode::<humility::reflect::Struct>(
                     hubris, &op_phy, r,
                 )
             })
-            .collect::<Result<Vec<Result<_, _>>>>()?;
+            .collect::<Vec<_>>();
 
         // Decode the port and phy status values into reflect::Value
         port_results.into_iter().zip(phy_results).collect::<Vec<_>>()
@@ -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::<Enum>(0)?;
+        let n = dev.field::<u8>(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::<Struct>("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::<Enum>("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::<Result<Vec<Result<_, _>>>>()?;
+        .collect::<Vec<Result<_, _>>>();
 
     let mut mac_table: BTreeMap<u16, Vec<[u8; 6]>> = BTreeMap::new();
     for r in results {

cmd/net/src/lib.rs

@@ -204,7 +204,7 @@ fn net_mac_table(
                 hubris, &op, r,
             )
         })
-        .collect::<Result<Vec<Result<_, _>>>>()?;
+        .collect::<Vec<_>>();
 
     let mut mac_table: BTreeMap<u16, Vec<[u8; 6]>> = BTreeMap::new();
     for r in results {

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::<humility::reflect::Value>(
+            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} => {}",

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::<u32>(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::<Base>(self.hubris, op, value)
-                .with_context(|| format!("failed to decode {call:?} result"))?;
+            let v = hiffy_decode::<Base>(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"));
         }
     };

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()),
         }
     }

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<T: humility::reflect::Load>(
     args: &[(&str, idol::IdolArgument)],
     lease_write: Option<&[u8]>,
     lease_read: Option<&mut [u8]>,
-) -> Result<T, HiffyCallError> {
+) -> Result<T, HiffyError> {
     check_op(op)?;
     check_leases(op, lease_write, lease_read.as_deref())?;
 
@@ -1496,29 +1496,24 @@ pub fn hiffy_call<T: humility::reflect::Load>(
     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
         )));
     }
 
     let mut v: Result<Vec<u8>, 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::<Vec<u8>>();
-                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::<Vec<u8>>();
+        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<T: humility::reflect::Load>(
     hubris: &HubrisArchive,
     op: &idol::IdolOperation,
     val: Result<Vec<u8>, impl Into<IpcError>>,
-) -> Result<std::result::Result<T, String>> {
+) -> Result<T, HiffyError> {
     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<T: humility::reflect::Load>(
         },
         Err(dead @ IpcError::ServerDied(_)) => Err(format!("<{dead}>")),
     };
-    Ok(r)
+    r.map_err(HiffyError::Hiffy)
 }
 
 pub fn hiffy_format_result(