From f4d51aec1f747d0c3ee60d83e789cd591581c6ad Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 26 May 2026 10:41:19 -0400 Subject: [PATCH 1/2] Move hiffy / Idol decoding to a single place --- Cargo.lock | 1 + cmd/console-proxy/src/posix.rs | 4 -- cmd/hiffy/src/lib.rs | 9 ++- cmd/host/src/lib.rs | 4 +- cmd/monorail/src/lib.rs | 35 +++------- cmd/net/src/lib.rs | 10 +-- cmd/powershelf/src/lib.rs | 19 +++--- cmd/qspi/src/lib.rs | 2 - cmd/rendmp/src/lib.rs | 19 +++--- cmd/sbrmi/src/lib.rs | 18 +++-- cmd/tofino-eeprom/src/lib.rs | 2 - cmd/validate/src/lib.rs | 2 +- cmd/vpd/src/lib.rs | 16 ++--- humility-auxflash/src/lib.rs | 18 ++--- humility-hiffy/src/lib.rs | 91 +++---------------------- humility-idol/Cargo.toml | 1 + humility-idol/src/lib.rs | 119 ++++++++++++++++++++++++++++++--- 17 files changed, 178 insertions(+), 192 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 65cce9628..36db12544 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2709,6 +2709,7 @@ dependencies = [ "idol", "parse_int", "serde", + "thiserror 2.0.18", ] [[package]] diff --git a/cmd/console-proxy/src/posix.rs b/cmd/console-proxy/src/posix.rs index 9ccd76e0e..c4ce879bf 100644 --- a/cmd/console-proxy/src/posix.rs +++ b/cmd/console-proxy/src/posix.rs @@ -48,7 +48,6 @@ impl<'a> UartConsoleHandler<'a> { let op = self.hubris.get_idol_command("ControlPlaneAgent.uart_read")?; let v = humility_hiffy::hiffy_call::( - self.hubris, self.core, &mut self.context, &op, @@ -67,7 +66,6 @@ impl<'a> UartConsoleHandler<'a> { let buf = &buf[..usize::min(buf.len(), HIFFY_BUF_SIZE)]; let v = humility_hiffy::hiffy_call::( - self.hubris, self.core, &mut self.context, &op, @@ -159,7 +157,6 @@ impl<'a> UartConsoleHandler<'a> { .get_idol_command("ControlPlaneAgent.set_humility_uart_client")?; humility_hiffy::hiffy_call::<()>( - self.hubris, self.core, &mut self.context, &op, @@ -176,7 +173,6 @@ impl<'a> UartConsoleHandler<'a> { .get_idol_command("ControlPlaneAgent.get_uart_client")?; let value = humility_hiffy::hiffy_call::( - self.hubris, self.core, &mut self.context, &op, diff --git a/cmd/hiffy/src/lib.rs b/cmd/hiffy/src/lib.rs index 4fe2ccca4..146fc21bf 100644 --- a/cmd/hiffy/src/lib.rs +++ b/cmd/hiffy/src/lib.rs @@ -124,14 +124,14 @@ pub fn hiffy_list(hubris: &HubrisArchive, filter: Vec) -> Result<()> { } match idol::lookup_reply(hubris, module, op.0) { - Ok((_, idol::IdolError::CLike(e))) => match &op.1.reply { + Ok((_, idol::IdolErrorType::CLike(e))) => match &op.1.reply { Reply::Result { ok, .. } => { println!("{}{:<27} {}", margin, "", ok.ty); println!("{}{:<27} {}", margin, "", e.name); } _ => warn!("mismatch on reply: found {op:?}"), }, - Ok((_, idol::IdolError::Complex(t))) => match &op.1.reply { + Ok((_, idol::IdolErrorType::Complex(t))) => match &op.1.reply { Reply::Result { ok, .. } => { println!("{}{:<27} {}", margin, "", ok.ty); println!("{}{:<27} {}", margin, "", t.name); @@ -139,7 +139,7 @@ pub fn hiffy_list(hubris: &HubrisArchive, filter: Vec) -> Result<()> { _ => warn!("mismatch on reply: found {op:?}"), }, - Ok((_, idol::IdolError::None)) => match &op.1.reply { + Ok((_, idol::IdolErrorType::None)) => match &op.1.reply { Reply::Result { ok, .. } => { // // This is possible if the only error is ServerDeath @@ -312,7 +312,6 @@ fn hiffy(subargs: HiffyArgs, context: &mut ExecutionContext) -> Result<()> { ( match hiffy_call( - hubris, core, &mut context, &op, @@ -328,7 +327,7 @@ fn hiffy(subargs: HiffyArgs, context: &mut ExecutionContext) -> Result<()> { ) }; - hiffy_print_result(hubris, &op, return_code)?; + hiffy_print_result(hubris, &op, &return_code)?; if let Some(data) = output { if let Some(out) = &subargs.output { std::fs::write(out, &data) diff --git a/cmd/host/src/lib.rs b/cmd/host/src/lib.rs index 0f11466fc..43c7d06a0 100644 --- a/cmd/host/src/lib.rs +++ b/cmd/host/src/lib.rs @@ -357,7 +357,6 @@ fn host_post_codes( )?; let op = hubris.get_idol_command("Sequencer.post_code_buffer_len")?; let count = humility_hiffy::hiffy_call::( - hubris, core, &mut context, &op, @@ -417,7 +416,7 @@ 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)?; + let v = op.decode(&r)?; handle_value(v); } } @@ -438,7 +437,6 @@ fn host_last_post_code( )?; let op = hubris.get_idol_command("Sequencer.last_post_code")?; let v = humility_hiffy::hiffy_call::( - hubris, core, &mut context, &op, diff --git a/cmd/monorail/src/lib.rs b/cmd/monorail/src/lib.rs index bf36d7983..b53ee5239 100644 --- a/cmd/monorail/src/lib.rs +++ b/cmd/monorail/src/lib.rs @@ -172,8 +172,8 @@ use humility::core::Core; use humility::hubris::*; use humility::reflect::*; use humility_cli::{ExecutionContext, humility_cmd}; -use humility_hiffy::{HiffyContext, HiffyError}; -use humility_idol::{HubrisIdol, IdolArgument}; +use humility_hiffy::HiffyContext; +use humility_idol::{HubrisIdol, IdolArgument, IdolDecodeError, IdolError}; use anyhow::{Result, anyhow}; @@ -310,7 +310,6 @@ fn monorail_read( let op = hubris.get_idol_command("Monorail.read_vsc7448_reg")?; let value = humility_hiffy::hiffy_call::( - hubris, core, context, &op, @@ -345,7 +344,6 @@ fn monorail_write( let op = hubris.get_idol_command("Monorail.write_vsc7448_reg")?; humility_hiffy::hiffy_call::<()>( - hubris, core, context, &op, @@ -423,7 +421,6 @@ fn monorail_phy_read( println!("Reading from port {} PHY, register {}", port, reg.name); let op = hubris.get_idol_command("Monorail.read_phy_reg")?; let value = humility_hiffy::hiffy_call::( - hubris, core, context, &op, @@ -456,7 +453,6 @@ fn monorail_phy_write( pretty_print_fields(value as u32, ®.fields, 0); let op = hubris.get_idol_command("Monorail.write_phy_reg")?; humility_hiffy::hiffy_call::<()>( - hubris, core, context, &op, @@ -599,7 +595,7 @@ fn monorail_dump( let results = context.run(core, ops.as_slice(), None)?; results .into_iter() - .map(move |r| humility_hiffy::hiffy_decode(hubris, &op_read, r)) + .map(move |r| op_read.decode(&r)) .collect::, _>>()? }; for (i, value) in results.iter().enumerate() { @@ -674,19 +670,11 @@ fn monorail_status( let port_results = port_results .into_iter() - .map(move |r| { - humility_hiffy::hiffy_decode::( - hubris, &op_port, r, - ) - }) + .map(move |r| op_port.decode::(&r)) .collect::>(); let phy_results = phy_results .into_iter() - .map(move |r| { - humility_hiffy::hiffy_decode::( - hubris, &op_phy, r, - ) - }) + .map(move |r| op_phy.decode::(&r)) .collect::>(); // Decode the port and phy status values into reflect::Value @@ -775,7 +763,7 @@ fn monorail_status( ) } Err(e) => { - if let HiffyError::Hiffy(e) = &e + if let IdolDecodeError::Idol(IdolError::Named(e)) = &e && e == "UnconfiguredPort" { print!( @@ -799,7 +787,7 @@ fn monorail_status( ) } Err(e) => { - if let HiffyError::Hiffy(e) = &e + if let IdolDecodeError::Idol(IdolError::Named(e)) = &e && (e == "UnconfiguredPort" || e == "NoPhy") { println!("{}", "-- -- --".dimmed()); @@ -824,7 +812,6 @@ fn monorail_mac_table( // - Read the number of entries in the MAC table // - Loop over the table that many times, reading entries let mac_count = humility_hiffy::hiffy_call::( - hubris, core, context, &op_mac_count, @@ -864,11 +851,7 @@ fn monorail_mac_table( let results = context.run(core, ops.as_slice(), None)?; let results = results .into_iter() - .map(move |r| { - humility_hiffy::hiffy_decode::( - hubris, &op, r, - ) - }) + .map(move |r| op.decode::(&r)) .collect::>>(); let mut mac_table: BTreeMap> = BTreeMap::new(); @@ -917,7 +900,6 @@ fn monorail_reset_counters( ) -> Result<()> { let op = hubris.get_idol_command("Monorail.reset_port_counters")?; humility_hiffy::hiffy_call::<()>( - hubris, core, context, &op, @@ -936,7 +918,6 @@ fn monorail_counters( ) -> Result<()> { let op = hubris.get_idol_command("Monorail.get_port_counters")?; let s = humility_hiffy::hiffy_call::( - hubris, core, context, &op, diff --git a/cmd/net/src/lib.rs b/cmd/net/src/lib.rs index 9d01aedc6..453a6f8ef 100644 --- a/cmd/net/src/lib.rs +++ b/cmd/net/src/lib.rs @@ -96,7 +96,6 @@ fn net_ip( let op = hubris.get_idol_command("Net.get_mac_address")?; let v = humility_hiffy::hiffy_call::( - hubris, core, &mut hiffy_context, &op, @@ -147,7 +146,6 @@ fn net_mac_table( // - Read the number of entries in the MAC table // - Loop over the table that many times, reading entries let mac_count = humility_hiffy::hiffy_call::( - hubris, core, &mut hiffy_context, &op_mac_count, @@ -198,11 +196,7 @@ fn net_mac_table( let results = hiffy_context.run(core, ops.as_slice(), None)?; let results = results .into_iter() - .map(move |r| { - humility_hiffy::hiffy_decode::( - hubris, &op, r, - ) - }) + .map(move |r| op.decode::(&r)) .collect::>(); let mut mac_table: BTreeMap> = BTreeMap::new(); @@ -254,7 +248,6 @@ fn net_status( let op = hubris.get_idol_command("Net.management_link_status")?; let s = humility_hiffy::hiffy_call::( - hubris, core, &mut hiffy_context, &op, @@ -318,7 +311,6 @@ fn net_counters( let op = hubris.get_idol_command("Net.management_counters")?; let s = humility_hiffy::hiffy_call::( - hubris, core, &mut hiffy_context, &op, diff --git a/cmd/powershelf/src/lib.rs b/cmd/powershelf/src/lib.rs index 9f447a900..21fcd6f7c 100644 --- a/cmd/powershelf/src/lib.rs +++ b/cmd/powershelf/src/lib.rs @@ -20,7 +20,7 @@ use humility::hubris::HubrisArchive; use humility::hubris::HubrisEnum; use humility_cli::{ExecutionContext, humility_cmd}; use humility_hiffy::*; -use humility_idol::{self as idol, HubrisIdol}; +use humility_idol::{self as idol, HubrisIdol, IdolDecodeError}; #[derive(Parser, Debug)] #[clap(name = "powershelf", about = env!("CARGO_PKG_DESCRIPTION"))] @@ -165,20 +165,17 @@ fn powershelf_run( let results = context.run(core, ops.as_slice(), None)?; for (ndx, variant) in operation.variants.iter().enumerate() { - 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), - }; + let result = + match idol_cmd.decode::(&results[ndx]) { + Ok(s) => Ok(s), + Err(IdolDecodeError::Idol(s)) => Err(s), + Err(IdolDecodeError::Failed(e)) => return Err(e.into()), + }; println!( "{:<20} => {}", variant.name, - hiffy_format_result(hubris, result.clone()) + hiffy_format_result(hubris, &result) ); if subargs.verbose { diff --git a/cmd/qspi/src/lib.rs b/cmd/qspi/src/lib.rs index 02dcfdce6..ce21543e1 100644 --- a/cmd/qspi/src/lib.rs +++ b/cmd/qspi/src/lib.rs @@ -494,7 +494,6 @@ fn qspi(subargs: QspiArgs, context: &mut ExecutionContext) -> Result<()> { let s = s.unwrap(); humility::msg!("Setting slot to {s}"); hiffy_call::<()>( - hubris, core, &mut context, &hubris.get_idol_command("HostFlash.set_dev")?, @@ -1025,7 +1024,6 @@ fn qspi(subargs: QspiArgs, context: &mut ExecutionContext) -> Result<()> { _ => bail!("dev_select must be 0 or 1"), }; hiffy_call::<()>( - hubris, core, &mut context, &hubris.get_idol_command("HostFlash.write_persistent_data")?, diff --git a/cmd/rendmp/src/lib.rs b/cmd/rendmp/src/lib.rs index a8c03f234..3e2626f8a 100644 --- a/cmd/rendmp/src/lib.rs +++ b/cmd/rendmp/src/lib.rs @@ -148,7 +148,7 @@ use humility::warn; use humility_cli::{ExecutionContext, humility_cmd}; use humility_hiffy::*; use humility_i2c::I2cArgs; -use humility_idol::{HubrisIdol, IdolOperation}; +use humility_idol::{HubrisIdol, IdolDecodeError, IdolOperation}; use anyhow::{Context, Result, anyhow, bail}; use clap::Parser; @@ -1126,7 +1126,6 @@ fn rendmp_blackbox( let (addr, _dev) = check_addr(&subargs, hubris)?; let op = hubris.get_idol_command("Power.rendmp_blackbox_dump")?; let e = hiffy_call::( - hubris, core, context, &op, @@ -1207,7 +1206,7 @@ fn get_pin_states( // Decode Hiffy results let mut values = vec![]; for r in results { - values.push(hiffy_decode::(hubris, &op, r)?); + values.push(op.decode::(&r)?); } let phases = dev.phases(); @@ -1339,7 +1338,6 @@ impl Call { } struct HifWorker<'a, 'b> { - hubris: &'a HubrisArchive, context: &'a mut HiffyContext<'b>, read_dma: IdolOperation<'a>, @@ -1441,7 +1439,6 @@ impl<'a, 'b> HifWorker<'a, 'b> { .collect::>>()?; Ok(Self { - hubris, context, read_dma, @@ -1592,7 +1589,7 @@ impl<'a, 'b> HifWorker<'a, 'b> { fn run( &mut self, core: &mut dyn humility::core::Core, - ) -> Result>> { + ) -> Result>> { let mut ops = std::mem::take(&mut self.ops); ops.push(Op::Done); let r = self.context.run(core, &ops, None)?; @@ -1612,7 +1609,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); + let v = op.decode::(&value); match v { Ok(v) => match (v, call) { (Base::U32(v), Call::ReadDma(..)) => { @@ -1641,8 +1638,8 @@ impl<'a, 'b> HifWorker<'a, 'b> { bail!("got unexpected result {base} for {op:?}") } }, - Err(HiffyError::Hiffy(e)) => out.push(Err(e)), - Err(HiffyError::Other(e)) => { + Err(IdolDecodeError::Idol(e)) => out.push(Err(e)), + Err(IdolDecodeError::Failed(e)) => { return Err(e).with_context(|| { format!("failed to decode {call:?} result") }); @@ -1665,7 +1662,7 @@ fn rendmp_phase_check<'a>( .get_idol_command("Sequencer.tofino_seq_state") .or_else(|_| hubris.get_idol_command("Sequencer.get_state")) .context("could not get power state HIF operation")?; - let r = hiffy_call(hubris, core, context, &power_state_op, &[], None, None); + let r = hiffy_call(core, context, &power_state_op, &[], None, None); let v = match r { Ok(r) => Ok(r), Err(HiffyError::Hiffy(s)) => Err(s), @@ -1673,7 +1670,7 @@ fn rendmp_phase_check<'a>( return Err(e.context("power state check")); } }; - if hiffy_format_result(hubris, v) != "A2" { + if hiffy_format_result(hubris, &v) != "A2" { bail!("must be in A2 when checking phases"); } diff --git a/cmd/sbrmi/src/lib.rs b/cmd/sbrmi/src/lib.rs index 7a9c4efa4..4e370dc51 100644 --- a/cmd/sbrmi/src/lib.rs +++ b/cmd/sbrmi/src/lib.rs @@ -153,7 +153,7 @@ fn call_cpuid( ops.push(Op::Done); let results = context.run(core, ops.as_slice(), None)?; - let registers = context.idol_result::<[u32; 4]>(&op, &results[0])?; + let registers = op.decode::<[u32; 4]>(&results[0])?; Ok(CpuIdResult { eax: registers[0], @@ -393,8 +393,8 @@ fn mca( let ipid_results = &results[nbanks as usize..]; for (bank, r) in results[..nbanks as usize].iter().enumerate() { - let status = context.idol_result::(&op, r)?; - let ipid = context.idol_result::(&op, &ipid_results[bank])?; + let status = op.decode::(r)?; + let ipid = op.decode::(&ipid_results[bank])?; if !all_mca { if status == 0 { @@ -441,7 +441,7 @@ fn mca( let mut values = HashMap::new(); for (ndx, reg) in reg_results.iter().enumerate() { - let v = context.idol_result::(&op, reg)?; + let v = op.decode::(reg)?; values.insert(allregs[ndx], v); } @@ -535,12 +535,10 @@ fn sbrmi(subargs: SbrmiArgs, context: &mut ExecutionContext) -> Result<()> { let results = context.run(core, ops.as_slice(), None)?; - let nthreads = context.idol_result::(&nthreads, &results[0])?; - let enabled = - threadmap(&context.idol_result::>(&enabled, &results[1])?)?; - let alert = - threadmap(&context.idol_result::>(&alert, &results[2])?)?; - let mcg_cap = context.idol_result::(&mcg_cap, &results[3])?; + let nthreads = nthreads.decode::(&results[0])?; + let enabled = threadmap(&enabled.decode::>(&results[1])?)?; + let alert = threadmap(&alert.decode::>(&results[2])?)?; + let mcg_cap = mcg_cap.decode::(&results[3])?; if subargs.mca { let thread = subargs.thread.unwrap(); diff --git a/cmd/tofino-eeprom/src/lib.rs b/cmd/tofino-eeprom/src/lib.rs index 804d47182..69f8494e1 100644 --- a/cmd/tofino-eeprom/src/lib.rs +++ b/cmd/tofino-eeprom/src/lib.rs @@ -86,7 +86,6 @@ impl<'a> EepromHandler<'a> { for (i, chunk) in out.chunks_mut(READ_CHUNK_SIZE).enumerate() { let offset = i * READ_CHUNK_SIZE; humility_hiffy::hiffy_call::<()>( - self.hubris, self.core, &mut self.context, &op, @@ -121,7 +120,6 @@ impl<'a> EepromHandler<'a> { for (i, chunk) in data.chunks(WRITE_CHUNK_SIZE).enumerate() { let offset = i * WRITE_CHUNK_SIZE; humility_hiffy::hiffy_call::<()>( - self.hubris, self.core, &mut self.context, &op, diff --git a/cmd/validate/src/lib.rs b/cmd/validate/src/lib.rs index 0e7dcb74e..59b99ec14 100644 --- a/cmd/validate/src/lib.rs +++ b/cmd/validate/src/lib.rs @@ -239,7 +239,7 @@ fn validate( } } Err(IpcError::Error(e)) => { - if let idol::IdolError::CLike(err) = op.error { + if let idol::IdolErrorType::CLike(err) = op.error { // TODO: assumes discriminant is a u8. Since this is using // Hiffy call results instead of looking at a Rust value in // memory, it's not clear from context what changes would be diff --git a/cmd/vpd/src/lib.rs b/cmd/vpd/src/lib.rs index ab9065f1e..166a58b93 100644 --- a/cmd/vpd/src/lib.rs +++ b/cmd/vpd/src/lib.rs @@ -248,7 +248,7 @@ fn list( }; let result = if let (Some(lop), Some(rs)) = (&locked_op, &results) { - Some(context.idol_result(lop, &rs[ndx])) + Some(lop.decode(&rs[ndx])) } else { None }; @@ -418,7 +418,7 @@ fn vpd_write( let results = context.run(core, ops.as_slice(), None)?; for (o, result) in results.iter().enumerate() { - context.idol_result::<()>(&op, result).with_context(|| { + op.decode::<()>(result).with_context(|| { format!("failed to write VPD at offset {}", offset + o) })?; } @@ -468,8 +468,7 @@ fn vpd_read_at( let results = context.run(core, ops.as_slice(), None)?; - context - .idol_result::>(op, &results[0]) + op.decode::>(&results[0]) .with_context(|| format!("failed to read at offset {offset}")) } @@ -591,8 +590,7 @@ fn vpd_lock( let results = context.run(core, ops.as_slice(), None)?; - context - .idol_result::<()>(&op, &results[0]) + op.decode::<()>(&results[0]) .with_context(|| format!("failed to lock {index}"))?; humility::msg!("successfully locked VPD"); @@ -635,7 +633,7 @@ fn vpd_lock_all( use humility::reflect::Base::Bool; use humility::reflect::Value::Base; - let result = context.idol_result(&op, r); + let result = op.decode(r); match result { Ok(Base(Bool(true))) => { @@ -696,8 +694,8 @@ fn vpd_lock_all( let mut success = 0; for (ndx, r) in locking.iter().zip(results.iter()) { - context - .idol_result::<()>(&lock_op, r) + lock_op + .decode::<()>(r) .with_context(|| format!("failed to lock VPD {ndx}"))?; success += 1; } diff --git a/humility-auxflash/src/lib.rs b/humility-auxflash/src/lib.rs index 94529eef0..c252b0232 100644 --- a/humility-auxflash/src/lib.rs +++ b/humility-auxflash/src/lib.rs @@ -45,7 +45,6 @@ impl<'a> AuxFlashHandler<'a> { pub fn slot_count(&mut self) -> Result { let op = self.hubris.get_idol_command("AuxFlash.slot_count")?; let value = humility_hiffy::hiffy_call::( - self.hubris, self.core, &mut self.context, &op, @@ -62,7 +61,6 @@ impl<'a> AuxFlashHandler<'a> { .hubris .get_idol_command("AuxFlash.scan_and_get_active_slot")?; let value = humility_hiffy::hiffy_call::( - self.hubris, self.core, &mut self.context, &op, @@ -72,7 +70,11 @@ impl<'a> AuxFlashHandler<'a> { ); match value { Ok(v) => Ok(Some(v)), - Err(HiffyError::Hiffy(s)) if s == "NoActiveSlot" => Ok(None), + Err(HiffyError::Hiffy(humility_idol::IdolError::Named(e))) + if e == "NoActiveSlot" => + { + Ok(None) + } Err(e) => Err(e.into()), } } @@ -81,7 +83,6 @@ impl<'a> AuxFlashHandler<'a> { pub fn slot_erase(&mut self, slot: u32) -> Result<()> { let op = self.hubris.get_idol_command("AuxFlash.erase_slot")?; humility_hiffy::hiffy_call::<()>( - self.hubris, self.core, &mut self.context, &op, @@ -99,7 +100,6 @@ impl<'a> AuxFlashHandler<'a> { pub fn slot_status(&mut self, slot: u32) -> Result> { let op = self.hubris.get_idol_command("AuxFlash.read_slot_chck")?; let value = humility_hiffy::hiffy_call::<([u8; 32],)>( - self.hubris, self.core, &mut self.context, &op, @@ -109,7 +109,11 @@ impl<'a> AuxFlashHandler<'a> { ); match value { Ok(v) => Ok(Some(v.0)), - Err(HiffyError::Hiffy(e)) if e == "MissingChck" => Ok(None), + Err(HiffyError::Hiffy(humility_idol::IdolError::Named(e))) + if e == "MissingChck" => + { + Ok(None) + } Err(e) => Err(e.into()), } } @@ -137,7 +141,6 @@ impl<'a> AuxFlashHandler<'a> { for (i, chunk) in out.chunks_mut(READ_CHUNK_SIZE).enumerate() { let offset = i * READ_CHUNK_SIZE; humility_hiffy::hiffy_call::<()>( - self.hubris, self.core, &mut self.context, &op, @@ -236,7 +239,6 @@ impl<'a> AuxFlashHandler<'a> { for (i, chunk) in data.chunks(data_size).enumerate() { let offset = i * data_size; humility_hiffy::hiffy_call::<()>( - self.hubris, self.core, &mut self.context, &op, diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index b9d9ade0a..9a16fb468 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -990,32 +990,6 @@ impl<'a> HiffyContext<'a> { ) } - /// Convenience routine to pull out the result of an Idol call - pub fn idol_result( - &mut self, - op: &idol::IdolOperation, - result: &Result, IpcError>, - ) -> Result { - use humility::reflect::{deserialize_value, load_value}; - - let value = match result { - Ok(val) => { - let ty = self.hubris.lookup_type(op.ok).unwrap(); - match op.operation.encoding { - ::idol::syntax::Encoding::Zerocopy => { - load_value(self.hubris, val, ty, 0)? - } - ::idol::syntax::Encoding::Ssmarshal - | ::idol::syntax::Encoding::Hubpack => { - deserialize_value(self.hubris, val, ty)?.0 - } - } - } - Err(e) => bail!("{}", op.strerror(*e)), - }; - T::from_value(&value) - } - /// Begins HIF execution. This is potentially non-blocking with respect to /// the HIF program, so you will need to poll [Self::done] to check for /// completion. @@ -1442,8 +1416,8 @@ fn has_task_started( #[derive(thiserror::Error, Debug)] pub enum HiffyError { /// A function called in the HIF program returned an error - #[error("hiffy error: {0}")] - Hiffy(String), + #[error("hiffy error")] + Hiffy(#[from] idol::IdolError), /// There was an error invoking the HIF program #[error(transparent)] Other(#[from] anyhow::Error), @@ -1451,7 +1425,6 @@ pub enum HiffyError { /// Executes a Hiffy call, returning the output value (or an error) pub fn hiffy_call( - hubris: &HubrisArchive, core: &mut dyn Core, context: &mut HiffyContext, op: &idol::IdolOperation, @@ -1513,59 +1486,15 @@ pub fn hiffy_call( 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. -/// -/// Returns an outer error if decoding fails, or an inner error if the Hiffy -/// call returns an error code (formatted as a String). -pub fn hiffy_decode( - hubris: &HubrisArchive, - op: &idol::IdolOperation, - val: Result, impl Into>, -) -> Result { - let r = match val.map_err(Into::into) { - Ok(val) => { - let ty = hubris.lookup_type(op.ok).unwrap(); - let value = match op.operation.encoding { - ::idol::syntax::Encoding::Zerocopy => { - humility::reflect::load_value(hubris, &val, ty, 0)? - } - ::idol::syntax::Encoding::Ssmarshal - | ::idol::syntax::Encoding::Hubpack => { - humility::reflect::deserialize_value(hubris, &val, ty)?.0 - } - }; - Ok(T::from_value(&value)?) - } - Err(IpcError::Error(e)) => match op.error { - idol::IdolError::CLike(error) => { - // TODO potentially sign-extended discriminator represented as - // u32 and then zero-extended to u64; won't work for signed - // values. Can't use determine_variant here because it's not - // laid out in memory, it's been unfolded onto the return stack. - if let Some(v) = - error.lookup_variant_by_tag(Tag::from(e as u64)) - { - Err(v.name.to_string()) - } else { - Err(format!("")) - } - } - idol::IdolError::Complex(error) => { - Err(format!("", error.name)) - } - _ => Err(format!("")), - }, - Err(dead @ IpcError::ServerDied(_)) => Err(format!("<{dead}>")), - }; - r.map_err(HiffyError::Hiffy) + op.decode(&v).map_err(|e| match e { + idol::IdolDecodeError::Failed(v) => HiffyError::Other(v.into()), + idol::IdolDecodeError::Idol(v) => HiffyError::Hiffy(v), + }) } -pub fn hiffy_format_result( +pub fn hiffy_format_result( hubris: &HubrisArchive, - result: std::result::Result, + result: &std::result::Result, ) -> String { let fmt = HubrisPrintFormat { newline: false, @@ -1586,10 +1515,10 @@ pub fn hiffy_format_result( } } -pub fn hiffy_print_result( +pub fn hiffy_print_result( hubris: &HubrisArchive, op: &idol::IdolOperation, - result: std::result::Result, + result: &std::result::Result, ) -> Result<()> { println!( "{}.{}() => {}", diff --git a/humility-idol/Cargo.toml b/humility-idol/Cargo.toml index 0214ea25f..7e37c6e53 100644 --- a/humility-idol/Cargo.toml +++ b/humility-idol/Cargo.toml @@ -9,5 +9,6 @@ hubpack.workspace = true idol.workspace = true parse_int.workspace = true serde.workspace = true +thiserror.workspace = true humility.workspace = true diff --git a/humility-idol/src/lib.rs b/humility-idol/src/lib.rs index ad8e54df8..22ebb3aaa 100644 --- a/humility-idol/src/lib.rs +++ b/humility-idol/src/lib.rs @@ -23,7 +23,48 @@ pub struct IdolOperation<'a> { pub code: u16, pub args: Option<&'a HubrisStruct>, pub ok: HubrisGoff, - pub error: IdolError<'a>, + pub error: IdolErrorType<'a>, +} + +/// Error returned from [`IdolOperation::decode`] +/// +/// This error is either an error reported by Idol ([`IdolError`]) or an error +/// that occured in the process of decoding ([`IdolDecodeFailed`]) +#[derive(Debug, thiserror::Error)] +pub enum IdolDecodeError { + /// Error reported from Idol + #[error(transparent)] + Idol(#[from] IdolError), + + /// Decoding the result failed in Humility + #[error(transparent)] + Failed(#[from] IdolDecodeFailed), +} + +/// Idol decode errors happening outside of Idol +#[derive(Debug, thiserror::Error)] +pub enum IdolDecodeFailed { + #[error("failed to load zerocopy value")] + BytecastFailed(#[source] anyhow::Error), + #[error("failed to deserialize value")] + DeserializeValueFailed(#[source] anyhow::Error), + #[error("failed to load value with reflection")] + LoadFailed(#[source] anyhow::Error), + #[error("operation has no error type, but decode was called with an error")] + NoErrorType, +} + +/// Errors reported by Idol +#[derive(Debug, thiserror::Error)] +pub enum IdolError { + #[error("")] + ServerDied(u32), + #[error("")] + ComplexError(String), + #[error("")] + UnknownErrorVariant(u32), + #[error("{0}")] + Named(String), } #[derive(Debug)] @@ -42,7 +83,7 @@ where } #[derive(Debug)] -pub enum IdolError<'a> { +pub enum IdolErrorType<'a> { CLike(&'a HubrisEnum), Complex(&'a HubrisEnum), None, @@ -206,7 +247,7 @@ impl<'a> IdolOperation<'a> { pub fn strerror(&self, error: impl Into) -> String { match error.into() { IpcError::Error(code) => { - let variant = if let IdolError::CLike(error) = self.error { + let variant = if let IdolErrorType::CLike(error) = self.error { // TODO: assumes discriminant is a u8. Since this is using Hiffy // call results instead of looking at a Rust value in memory, it's // not clear from context what changes would be required to fix @@ -239,7 +280,7 @@ impl<'a> IdolOperation<'a> { ::idol::syntax::Encoding::Ssmarshal | ::idol::syntax::Encoding::Hubpack => { let ok = self.hubris.hubpack_serialized_maxsize(self.ok)?; - if let IdolError::Complex(e) = self.error { + if let IdolErrorType::Complex(e) = self.error { ok.max(self.hubris.hubpack_serialized_maxsize(e.goff)?) } else { ok @@ -248,6 +289,66 @@ impl<'a> IdolOperation<'a> { }; Ok(reply_size) } + + /// Decodes a value returned from calling the given Idol operation + pub fn decode( + &self, + val: &Result, IpcError>, + ) -> Result { + match val { + Ok(val) => { + let ty = self.hubris.lookup_type(self.ok).unwrap(); + let value = match self.operation.encoding { + ::idol::syntax::Encoding::Zerocopy => { + humility::reflect::load_value(self.hubris, val, ty, 0) + .map_err(IdolDecodeFailed::BytecastFailed)? + } + ::idol::syntax::Encoding::Ssmarshal + | ::idol::syntax::Encoding::Hubpack => { + humility::reflect::deserialize_value( + self.hubris, + val, + ty, + ) + .map_err(IdolDecodeFailed::DeserializeValueFailed)? + .0 + } + }; + Ok(T::from_value(&value) + .map_err(IdolDecodeFailed::DeserializeValueFailed)?) + } + Err(e) => Err(self.decode_error(*e)), + } + } + + pub fn decode_error(&self, err: IpcError) -> IdolDecodeError { + let out = match err { + IpcError::Error(e) => match self.error { + IdolErrorType::CLike(error) => { + // TODO potentially sign-extended discriminator represented + // as u32 and then zero-extended to u64; won't work for + // signed values. Can't use determine_variant here because + // it's not laid out in memory, it's been unfolded onto the + // return stack. + if let Some(v) = + error.lookup_variant_by_tag(Tag::from(e as u64)) + { + IdolError::Named(v.name.to_string()) + } else { + IdolError::UnknownErrorVariant(e) + } + } + IdolErrorType::Complex(error) => { + IdolError::ComplexError(error.name.to_string()) + } + IdolErrorType::None => { + return IdolDecodeFailed::NoErrorType.into(); + } + }, + IpcError::ServerDied(c) => IdolError::ServerDied(c), + }; + IdolDecodeError::Idol(out) + } } impl IpcError { @@ -797,7 +898,7 @@ pub fn lookup_reply<'a>( hubris: &'a HubrisArchive, m: &HubrisModule, op: &str, -) -> Result<(HubrisGoff, IdolError<'a>)> { +) -> Result<(HubrisGoff, IdolErrorType<'a>)> { let iface = m.iface.as_ref().unwrap(); let reply = &iface .ops @@ -854,19 +955,19 @@ pub fn lookup_reply<'a>( || anyhow!("failed to find error type {reply:?}"), )?; - IdolError::CLike(t) + IdolErrorType::CLike(t) } - ::idol::syntax::Error::ServerDeath => IdolError::None, + ::idol::syntax::Error::ServerDeath => IdolErrorType::None, ::idol::syntax::Error::Complex(t) => { let t = m.lookup_enum_byname(hubris, t)?.ok_or_else( || anyhow!("failed to find error type {reply:?}"), )?; - IdolError::Complex(t) + IdolErrorType::Complex(t) } }; Ok((lookup_ok(&ok.ty)?, err)) } - Reply::Simple(ok) => Ok((lookup_ok(&ok.ty)?, IdolError::None)), + Reply::Simple(ok) => Ok((lookup_ok(&ok.ty)?, IdolErrorType::None)), } } From 519042868a7fa4fb1337d467b90d805d43b1394d Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 26 May 2026 15:42:25 -0400 Subject: [PATCH 2/2] Post-review comments --- cmd/powershelf/src/lib.rs | 2 +- cmd/rendmp/src/lib.rs | 2 +- humility-hiffy/src/lib.rs | 2 +- humility-idol/src/lib.rs | 85 +++++++++++++++++++++++++++------------ 4 files changed, 63 insertions(+), 28 deletions(-) diff --git a/cmd/powershelf/src/lib.rs b/cmd/powershelf/src/lib.rs index 21fcd6f7c..39748f5ef 100644 --- a/cmd/powershelf/src/lib.rs +++ b/cmd/powershelf/src/lib.rs @@ -169,7 +169,7 @@ fn powershelf_run( match idol_cmd.decode::(&results[ndx]) { Ok(s) => Ok(s), Err(IdolDecodeError::Idol(s)) => Err(s), - Err(IdolDecodeError::Failed(e)) => return Err(e.into()), + Err(IdolDecodeError::DecodeFailed(e)) => return Err(e.into()), }; println!( diff --git a/cmd/rendmp/src/lib.rs b/cmd/rendmp/src/lib.rs index 3e2626f8a..0ea89cf85 100644 --- a/cmd/rendmp/src/lib.rs +++ b/cmd/rendmp/src/lib.rs @@ -1639,7 +1639,7 @@ impl<'a, 'b> HifWorker<'a, 'b> { } }, Err(IdolDecodeError::Idol(e)) => out.push(Err(e)), - Err(IdolDecodeError::Failed(e)) => { + Err(IdolDecodeError::DecodeFailed(e)) => { return Err(e).with_context(|| { format!("failed to decode {call:?} result") }); diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index 9a16fb468..d878cea28 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -1487,7 +1487,7 @@ pub fn hiffy_call( data.copy_from_slice(&extra_data); } op.decode(&v).map_err(|e| match e { - idol::IdolDecodeError::Failed(v) => HiffyError::Other(v.into()), + idol::IdolDecodeError::DecodeFailed(v) => HiffyError::Other(v.into()), idol::IdolDecodeError::Idol(v) => HiffyError::Hiffy(v), }) } diff --git a/humility-idol/src/lib.rs b/humility-idol/src/lib.rs index 22ebb3aaa..d7cfe5e7b 100644 --- a/humility-idol/src/lib.rs +++ b/humility-idol/src/lib.rs @@ -23,13 +23,14 @@ pub struct IdolOperation<'a> { pub code: u16, pub args: Option<&'a HubrisStruct>, pub ok: HubrisGoff, + pub ok_ty: HubrisType<'a>, pub error: IdolErrorType<'a>, } /// Error returned from [`IdolOperation::decode`] /// /// This error is either an error reported by Idol ([`IdolError`]) or an error -/// that occured in the process of decoding ([`IdolDecodeFailed`]) +/// that occurred in the process of decoding ([`IdolDecodeFailed`]) #[derive(Debug, thiserror::Error)] pub enum IdolDecodeError { /// Error reported from Idol @@ -38,22 +39,39 @@ pub enum IdolDecodeError { /// Decoding the result failed in Humility #[error(transparent)] - Failed(#[from] IdolDecodeFailed), + DecodeFailed(#[from] IdolDecodeFailed), } /// Idol decode errors happening outside of Idol #[derive(Debug, thiserror::Error)] pub enum IdolDecodeFailed { - #[error("failed to load zerocopy value")] - BytecastFailed(#[source] anyhow::Error), - #[error("failed to deserialize value")] - DeserializeValueFailed(#[source] anyhow::Error), + /// Got error when deserializing a value + #[error( + "failed to deserialize value using {} encoding", + encoding_str(*encoding) + )] + DeserializeFailed { + encoding: ::idol::syntax::Encoding, + #[source] + err: anyhow::Error, + }, + /// Calling [`reflect::Load`](humility::reflect::Load) failed #[error("failed to load value with reflection")] LoadFailed(#[source] anyhow::Error), + /// The operation has [`IdolErrorType::None`] but had an error #[error("operation has no error type, but decode was called with an error")] NoErrorType, } +/// Helper function to stringify an [`::idol::syntax::Encoding`] value +fn encoding_str(e: ::idol::syntax::Encoding) -> &'static str { + match e { + ::idol::syntax::Encoding::Ssmarshal => "ssmarshal", + ::idol::syntax::Encoding::Hubpack => "hubpack", + ::idol::syntax::Encoding::Zerocopy => "zerocopy", + } +} + /// Errors reported by Idol #[derive(Debug, thiserror::Error)] pub enum IdolError { @@ -111,11 +129,22 @@ impl<'a> IdolOperation<'a> { let module = hubris.lookup_module(task)?; let args = module.lookup_struct_byname(hubris, &t)?; let (ok, error) = lookup_reply(hubris, module, operation)?; + let ok_ty = hubris.lookup_type(ok)?; // // We have our fully formed Idol call! // - Ok(Self { hubris, name, task, operation: op, code, args, ok, error }) + Ok(Self { + hubris, + name, + task, + operation: op, + code, + args, + ok, + ok_ty, + error, + }) } fn args_size(&self) -> usize { @@ -297,32 +326,41 @@ impl<'a> IdolOperation<'a> { ) -> Result { match val { Ok(val) => { - let ty = self.hubris.lookup_type(self.ok).unwrap(); let value = match self.operation.encoding { ::idol::syntax::Encoding::Zerocopy => { - humility::reflect::load_value(self.hubris, val, ty, 0) - .map_err(IdolDecodeFailed::BytecastFailed)? + humility::reflect::load_value( + self.hubris, + val, + self.ok_ty, + 0, + ) } ::idol::syntax::Encoding::Ssmarshal | ::idol::syntax::Encoding::Hubpack => { humility::reflect::deserialize_value( self.hubris, val, - ty, + self.ok_ty, ) - .map_err(IdolDecodeFailed::DeserializeValueFailed)? - .0 + .map(|v| v.0) } - }; + } + .map_err(|err| { + IdolDecodeFailed::DeserializeFailed { + err, + encoding: self.operation.encoding, + } + })?; + Ok(T::from_value(&value) - .map_err(IdolDecodeFailed::DeserializeValueFailed)?) + .map_err(IdolDecodeFailed::LoadFailed)?) } Err(e) => Err(self.decode_error(*e)), } } pub fn decode_error(&self, err: IpcError) -> IdolDecodeError { - let out = match err { + match err { IpcError::Error(e) => match self.error { IdolErrorType::CLike(error) => { // TODO potentially sign-extended discriminator represented @@ -333,21 +371,18 @@ impl<'a> IdolOperation<'a> { if let Some(v) = error.lookup_variant_by_tag(Tag::from(e as u64)) { - IdolError::Named(v.name.to_string()) + IdolError::Named(v.name.to_string()).into() } else { - IdolError::UnknownErrorVariant(e) + IdolError::UnknownErrorVariant(e).into() } } IdolErrorType::Complex(error) => { - IdolError::ComplexError(error.name.to_string()) - } - IdolErrorType::None => { - return IdolDecodeFailed::NoErrorType.into(); + IdolError::ComplexError(error.name.to_string()).into() } + IdolErrorType::None => IdolDecodeFailed::NoErrorType.into(), }, - IpcError::ServerDied(c) => IdolError::ServerDied(c), - }; - IdolDecodeError::Idol(out) + IpcError::ServerDied(c) => IdolError::ServerDied(c).into(), + } } }