From 21c5fce7cde4dee1c8b0b7e93b7302f6a3269c7e Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 22 Apr 2026 16:33:26 -0400 Subject: [PATCH 1/3] Add `field` function on `struct` --- cmd/net/src/lib.rs | 67 +++++++++++++++--------------------- humility-core/src/reflect.rs | 57 +++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 41 deletions(-) diff --git a/cmd/net/src/lib.rs b/cmd/net/src/lib.rs index 09d22e08f..3271fd5cc 100644 --- a/cmd/net/src/lib.rs +++ b/cmd/net/src/lib.rs @@ -111,10 +111,7 @@ fn net_ip( }; let v = v.as_tuple()?; assert_eq!(v.name(), "MacAddress"); - let mut mac = [0; 6]; - for (i, byte) in v[0].as_array()?.iter().enumerate() { - mac[i] = byte.as_base()?.as_u8().unwrap(); - } + let mac = v.field::<[u8; 6]>(0)?; print!("{}: ", "MAC address".bold()); for (i, byte) in mac.iter().enumerate() { if i > 0 { @@ -227,11 +224,8 @@ fn net_mac_table( if let Ok(r) = r { let s = r.as_struct()?; assert_eq!(s.name(), "KszMacTableEntry"); - let port = s["port"].as_base().unwrap().as_u16().unwrap(); - let mut mac = [0; 6]; - for (i, m) in s["mac"].as_array().unwrap().iter().enumerate() { - mac[i] = m.as_base().unwrap().as_u8().unwrap() - } + let port = s.field::("port").unwrap(); + let mac = s.field::<[u8; 6]>("mac")?; if mac == [0; 6] && port == 0xFFFF { humility::msg!("Skipping empty MAC address"); } else { @@ -290,16 +284,9 @@ fn net_status( let s = v.as_struct()?; assert_eq!(s.name(), "ManagementLinkStatus"); - let to_bool_vec = |name| -> Result> { - Ok(s[name] - .as_array()? - .iter() - .map(|i| i.as_base().unwrap().as_bool().unwrap()) - .collect()) - }; - let ksz_100base_fx = to_bool_vec("ksz8463_100base_fx_link_up")?; - let vsc_100base_fx = to_bool_vec("vsc85x2_100base_fx_link_up")?; - let vsc_sgmii = to_bool_vec("vsc85x2_sgmii_link_up")?; + let ksz_100base_fx: Vec = s.field("ksz8463_100base_fx_link_up")?; + let vsc_100base_fx: Vec = s.field("vsc85x2_100base_fx_link_up")?; + let vsc_sgmii: Vec = s.field("vsc85x2_sgmii_link_up")?; let up_down = |b| { if b { " UP ".green() } else { "DOWN".red() } @@ -382,8 +369,8 @@ fn net_counters( } fn net_counters_table(s: &Struct) -> Result<()> { - let k_tx = s["ksz8463_tx"].as_array()?; - let k_rx = s["ksz8463_rx"].as_array()?; + let k_tx = s.field::<[_; 3]>("ksz8463_tx")?; + let k_rx = s.field::<[_; 3]>("ksz8463_rx")?; let value = |k: &Struct, s: &str| { let k = k[s].as_base().unwrap().as_u32().unwrap(); let out = format!("{:>6}", k); @@ -407,8 +394,8 @@ fn net_counters_table(s: &Struct) -> Result<()> { " |-----------|--------|--------|--------|--------|--------|--------|" ); for i in 0..3 { - let k_tx = k_tx[i].as_struct()?; - let k_rx = k_rx[i].as_struct()?; + let k_tx = &k_tx[i]; + let k_rx = &k_rx[i]; println!( " | Port {} | {} | {} | {} | {} | {} | {} |", i + 1, @@ -426,12 +413,12 @@ fn net_counters_table(s: &Struct) -> Result<()> { println!(); - let v_tx = s["vsc85x2_tx"].as_array()?; - let v_rx = s["vsc85x2_rx"].as_array()?; - let v_mac_valid = s["vsc85x2_mac_valid"].as_base()?.as_bool().unwrap(); + let v_tx = s.field::<[_; 2]>("vsc85x2_tx")?; + let v_rx = s.field::<[_; 2]>("vsc85x2_rx")?; + let v_mac_valid = s.field::("vsc85x2_mac_valid")?; let value = |v: &Struct, s: &str| { - let v = v[s].as_base().unwrap().as_u16().unwrap(); + let v = v.field::(s).unwrap(); let out = format!("{:>6}", v); if v > 0 { if s.contains("good") { out.green() } else { out.red() } @@ -447,8 +434,8 @@ fn net_counters_table(s: &Struct) -> Result<()> { println!(" | | Good | Bad | Good | Bad |"); println!(" |-----------------------------------------------------|"); for i in 0..2 { - let v_tx = v_tx[i].as_struct()?; - let v_rx = v_rx[i].as_struct()?; + let v_tx = &v_tx[i]; + let v_rx = &v_rx[i]; if v_mac_valid { println!( " | Port {} | MAC | {} | {} | {} | {} |", @@ -479,33 +466,33 @@ fn net_counters_table(s: &Struct) -> Result<()> { } fn net_counters_diagram(s: &Struct) -> Result<()> { - let k_tx = s["ksz8463_tx"].as_array()?; - let k_rx = s["ksz8463_rx"].as_array()?; - let value = |k: &Struct, s: &str| k[s].as_base().unwrap().as_u32().unwrap(); + let k_tx = s.field::<[_; 3]>("ksz8463_tx")?; + let k_rx = s.field::<[_; 3]>("ksz8463_rx")?; + let value = |k: &Struct, s: &str| k.field::(s).unwrap(); let mut ksz_tx = [0; 3]; let mut ksz_rx = [0; 3]; for port in 0..3 { - let k_tx = k_tx[port].as_struct()?; - let k_rx = k_rx[port].as_struct()?; + let k_tx = &k_tx[port]; + let k_rx = &k_rx[port]; for t in ["unicast", "broadcast", "multicast"] { ksz_tx[port] += value(k_tx, t); ksz_rx[port] += value(k_rx, t); } } - let v_tx = s["vsc85x2_tx"].as_array()?; - let v_rx = s["vsc85x2_rx"].as_array()?; - let v_mac_valid = s["vsc85x2_mac_valid"].as_base()?.as_bool().unwrap(); - let value = |v: &Struct, s: &str| v[s].as_base().unwrap().as_u16().unwrap(); + let v_tx = s.field::<[_; 2]>("vsc85x2_tx")?; + let v_rx = s.field::<[_; 2]>("vsc85x2_rx")?; + let v_mac_valid = s.field::("vsc85x2_mac_valid")?; + let value = |v: &Struct, s: &str| v.field::(s).unwrap(); let mut v_mac_tx = [0; 2]; let mut v_mac_rx = [0; 2]; let mut v_media_tx = [0; 2]; let mut v_media_rx = [0; 2]; for port in 0..2 { - let v_tx = v_tx[port].as_struct()?; - let v_rx = v_rx[port].as_struct()?; + let v_tx = &v_tx[port]; + let v_rx = &v_rx[port]; if v_mac_valid { v_mac_tx[port] = value(v_tx, "mac_good"); v_mac_rx[port] = value(v_rx, "mac_good"); diff --git a/humility-core/src/reflect.rs b/humility-core/src/reflect.rs index 3654fe454..093b5583f 100644 --- a/humility-core/src/reflect.rs +++ b/humility-core/src/reflect.rs @@ -139,7 +139,7 @@ impl Value { Value::Struct(s) => { v = s.get(f).ok_or_else(|| { anyhow!( - "could not field field `{f}`; \ + "could not find field `{f}`; \ available fields are {:?}", s.members .keys() @@ -529,6 +529,14 @@ impl Struct { { self.members.get(name).map(Box::as_ref) } + + /// Loads the field with name `name` if it exists + pub fn field(&self, name: &str) -> Result { + let Some(v) = self.members.get(name) else { + bail!("no such field {name}"); + }; + T::from_value(v) + } } /// Allows access to struct members using `s["foo"]`. @@ -592,6 +600,14 @@ impl Tuple { pub fn name(&self) -> &str { self.0.as_str() } + + /// Loads the field with index `i` if it exists + pub fn field(&self, index: usize) -> Result { + let Some(v) = self.1.get(index) else { + bail!("field {index} is not in range 0..{}", self.1.len()); + }; + T::from_value(v) + } } /// Allows the tuple to be treated like a slice, e.g. with `len()` and indexing. @@ -1035,6 +1051,36 @@ impl Load for Enum { } } +impl Load for Struct { + fn from_value(v: &Value) -> Result { + if let Value::Struct(v) = v { + Ok(v.clone()) + } else { + bail!("expected struct, got {v:?}"); + } + } +} + +impl Load for Tuple { + fn from_value(v: &Value) -> Result { + if let Value::Tuple(v) = v { + Ok(v.clone()) + } else { + bail!("expected tuple, got {v:?}"); + } + } +} + +impl Load for Base { + fn from_value(v: &Value) -> Result { + if let Value::Base(v) = v { + Ok(v.clone()) + } else { + bail!("expected base, got {v:?}"); + } + } +} + impl Load for bool { fn from_value(v: &Value) -> Result { v.as_base()?.as_bool().ok_or_else(|| anyhow!("not a bool: {:?}", v)) @@ -1071,6 +1117,15 @@ impl Load for f32 { } } +impl Load for () { + fn from_value(v: &Value) -> Result { + match v.as_base()? { + Base::U0 => Ok(()), + b => bail!("expected U0, got base {b:?}"), + } + } +} + impl Load for Array { fn from_value(v: &Value) -> Result { if let Value::Array(v) = v { From 2740bf73c99726a9c6d5ca6fb663d97cbff109a3 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 23 Apr 2026 16:43:57 -0400 Subject: [PATCH 2/3] Better spd_data unpacking --- humility-spd/src/lib.rs | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/humility-spd/src/lib.rs b/humility-spd/src/lib.rs index f88cb0f23..111864b3c 100644 --- a/humility-spd/src/lib.rs +++ b/humility-spd/src/lib.rs @@ -114,24 +114,8 @@ pub fn spd_lookup( out } Value::Struct(s) => { - let Some(Value::Array(a)) = s.get("spd_data") else { - bail!("expected `spd_data` to be an array"); - }; - let mut out = Vec::with_capacity(a.len()); - for a in a.iter() { - let Value::Array(a) = a else { - bail!("expected array-of-arrays"); - }; - let mut chunk = Vec::with_capacity(a.len()); - for v in a.iter() { - let Value::Base(Base::U8(b)) = v else { - bail!("expected `u8` array"); - }; - chunk.push(*b); - } - out.push(SpdData(chunk)) - } - out + let out = s.field::>>("spd_data")?; + out.into_iter().map(SpdData).collect() } _ => bail!("expected `spd_data` to be an array or struct"), }; From 97b54a29fb526eda079464853528f82844d5e446 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 20 May 2026 18:41:32 -0400 Subject: [PATCH 3/3] Minor cleanups --- cmd/monorail/src/lib.rs | 7 +--- cmd/net/src/lib.rs | 26 ++++++------ cmd/tasks/src/lib.rs | 15 +++---- humility-core/src/reflect.rs | 78 +++++++++++++++++++----------------- humility-doppel/src/lib.rs | 5 +-- humility-spd/src/lib.rs | 12 +++--- 6 files changed, 69 insertions(+), 74 deletions(-) diff --git a/cmd/monorail/src/lib.rs b/cmd/monorail/src/lib.rs index 898f182ba..13c678b4a 100644 --- a/cmd/monorail/src/lib.rs +++ b/cmd/monorail/src/lib.rs @@ -939,11 +939,8 @@ fn monorail_mac_table( if let Ok(r) = r { let s = r.as_struct()?; assert_eq!(s.name(), "MacTableEntry"); - let port = s["port"].as_base().unwrap().as_u16().unwrap(); - let mut mac = [0; 6]; - for (i, m) in s["mac"].as_array().unwrap().iter().enumerate() { - mac[i] = m.as_base().unwrap().as_u8().unwrap() - } + let port = s.field::("port").unwrap(); + let mac = s.field::<[u8; 6]>("mac").unwrap(); if mac == [0; 6] && port == 0xFFFF { println!("Skipping empty MAC address"); } else { diff --git a/cmd/net/src/lib.rs b/cmd/net/src/lib.rs index 3271fd5cc..d30a96445 100644 --- a/cmd/net/src/lib.rs +++ b/cmd/net/src/lib.rs @@ -224,7 +224,7 @@ fn net_mac_table( if let Ok(r) = r { let s = r.as_struct()?; assert_eq!(s.name(), "KszMacTableEntry"); - let port = s.field::("port").unwrap(); + let port = s.field::("port")?; let mac = s.field::<[u8; 6]>("mac")?; if mac == [0; 6] && port == 0xFFFF { humility::msg!("Skipping empty MAC address"); @@ -372,7 +372,7 @@ fn net_counters_table(s: &Struct) -> Result<()> { let k_tx = s.field::<[_; 3]>("ksz8463_tx")?; let k_rx = s.field::<[_; 3]>("ksz8463_rx")?; let value = |k: &Struct, s: &str| { - let k = k[s].as_base().unwrap().as_u32().unwrap(); + let k = k.field::(s).unwrap(); let out = format!("{:>6}", k); if k > 0 { if s.contains("ERR") { out.red() } else { out.green() } @@ -466,9 +466,8 @@ fn net_counters_table(s: &Struct) -> Result<()> { } fn net_counters_diagram(s: &Struct) -> Result<()> { - let k_tx = s.field::<[_; 3]>("ksz8463_tx")?; - let k_rx = s.field::<[_; 3]>("ksz8463_rx")?; - let value = |k: &Struct, s: &str| k.field::(s).unwrap(); + let k_tx = s.field::<[Struct; 3]>("ksz8463_tx")?; + let k_rx = s.field::<[Struct; 3]>("ksz8463_rx")?; let mut ksz_tx = [0; 3]; let mut ksz_rx = [0; 3]; @@ -476,15 +475,14 @@ fn net_counters_diagram(s: &Struct) -> Result<()> { let k_tx = &k_tx[port]; let k_rx = &k_rx[port]; for t in ["unicast", "broadcast", "multicast"] { - ksz_tx[port] += value(k_tx, t); - ksz_rx[port] += value(k_rx, t); + ksz_tx[port] += k_tx.field::(t)?; + ksz_rx[port] += k_rx.field::(t)?; } } - let v_tx = s.field::<[_; 2]>("vsc85x2_tx")?; - let v_rx = s.field::<[_; 2]>("vsc85x2_rx")?; + let v_tx = s.field::<[Struct; 2]>("vsc85x2_tx")?; + let v_rx = s.field::<[Struct; 2]>("vsc85x2_rx")?; let v_mac_valid = s.field::("vsc85x2_mac_valid")?; - let value = |v: &Struct, s: &str| v.field::(s).unwrap(); let mut v_mac_tx = [0; 2]; let mut v_mac_rx = [0; 2]; @@ -494,11 +492,11 @@ fn net_counters_diagram(s: &Struct) -> Result<()> { let v_tx = &v_tx[port]; let v_rx = &v_rx[port]; if v_mac_valid { - v_mac_tx[port] = value(v_tx, "mac_good"); - v_mac_rx[port] = value(v_rx, "mac_good"); + v_mac_tx[port] = v_tx.field::("mac_good")?; + v_mac_rx[port] = v_rx.field::("mac_good")?; } - v_media_tx[port] = value(v_tx, "media_good"); - v_media_rx[port] = value(v_rx, "media_good"); + v_media_tx[port] = v_tx.field::("media_good")?; + v_media_rx[port] = v_rx.field::("media_good")?; } let mac = |i: u16| { diff --git a/cmd/tasks/src/lib.rs b/cmd/tasks/src/lib.rs index fc9db75d0..596baad88 100644 --- a/cmd/tasks/src/lib.rs +++ b/cmd/tasks/src/lib.rs @@ -565,18 +565,13 @@ fn stack_guess<'a>( let reflect::Value::Struct(s) = &task_value else { bail!("invalid type for task_value") }; - let Some(reflect::Value::Struct(save)) = s.get("save") else { - bail!("invalid type for save") - }; - let Some(reflect::Value::Base(reflect::Base::U32(addr))) = save.get("r7") - else { - bail!("invalid type for r7") - }; - let pc = core.read_word_32(*addr + 4)? & !1; + let save = s.field::("save")?; + let addr = save.field::("r7")?; + let pc = core.read_word_32(addr + 4)? & !1; let mut regs = BTreeMap::new(); - regs.insert(ARMRegister::R7, *addr); - regs.insert(ARMRegister::LR, *addr); + regs.insert(ARMRegister::R7, addr); + regs.insert(ARMRegister::LR, addr); // Provide a dummy stack value to pick the // correct memory region diff --git a/humility-core/src/reflect.rs b/humility-core/src/reflect.rs index 093b5583f..a8d34ffa1 100644 --- a/humility-core/src/reflect.rs +++ b/humility-core/src/reflect.rs @@ -137,29 +137,13 @@ impl Value { for f in f.split(".") { match v { Value::Struct(s) => { - v = s.get(f).ok_or_else(|| { - anyhow!( - "could not find field `{f}`; \ - available fields are {:?}", - s.members - .keys() - .map(|s| s.as_str()) - .collect::>() - .join(", ") - ) - })?; + v = s.get(f)?; } Value::Tuple(t) => { let index: usize = f.parse().with_context(|| { format!("could not parse tuple index from `{f}`") })?; - v = t.1.get(index).ok_or_else(|| { - anyhow!( - "could not get field {index} from tuple \ - with {} elements", - t.1.len() - ) - })?; + v = t.get(index)?; } _ => bail!( "expected a struct or tuple when getting field `{f}`, \ @@ -490,7 +474,7 @@ impl Format for Base { #[derive(Clone, Debug, Default)] pub struct Struct { name: String, - members: IndexMap>, + members: IndexMap, } impl Struct { @@ -507,7 +491,7 @@ impl Struct { } pub fn iter(&self) -> impl Iterator { - self.members.iter().map(|(s, v)| (s.as_str(), &**v)) + self.members.iter().map(|(s, v)| (s.as_str(), v)) } /// Verifies that the struct contains _at least_ members with the given @@ -521,20 +505,34 @@ impl Struct { Ok(()) } - /// Returns a reference to the value of the member named `name`, if it - /// exists, or `None`, if no member with that name exists. - pub fn get(&self, name: &Q) -> Option<&Value> + /// Returns a reference to the value of the member named `name` + /// + /// If the name is not found, then an informative error is returned. + pub fn get(&self, name: &Q) -> Result<&Value> where - Q: std::hash::Hash + indexmap::Equivalent + ?Sized, + Q: std::hash::Hash + + indexmap::Equivalent + + ?Sized + + std::fmt::Display, { - self.members.get(name).map(Box::as_ref) + self.members.get(name).ok_or_else(|| { + anyhow!( + "could not find field `{}` in struct `{}`; \ + available fields are {:?}", + name, + self.name, + self.members + .keys() + .map(|s| s.as_str()) + .collect::>() + .join(", ") + ) + }) } /// Loads the field with name `name` if it exists pub fn field(&self, name: &str) -> Result { - let Some(v) = self.members.get(name) else { - bail!("no such field {name}"); - }; + let v = self.get(name)?; T::from_value(v) } } @@ -601,11 +599,22 @@ impl Tuple { self.0.as_str() } - /// Loads the field with index `i` if it exists - pub fn field(&self, index: usize) -> Result { + /// Returns a reference to a value in the tuple, by index + pub fn get(&self, index: usize) -> Result<&Value> { let Some(v) = self.1.get(index) else { - bail!("field {index} is not in range 0..{}", self.1.len()); + bail!( + "could not get field {} in tuple `{}` with {} elements", + index, + self.0, + self.1.len() + ); }; + Ok(v) + } + + /// Loads the field with index `i` if it exists + pub fn field(&self, index: usize) -> Result { + let v = self.get(index)?; T::from_value(v) } } @@ -919,10 +928,7 @@ pub fn load_struct( for m in &ty.members { let maddr = addr + m.offset; let mty = hubris.lookup_type(m.goff)?; - s.members.insert( - m.name.clone(), - Box::new(load_value(hubris, buf, mty, maddr)?), - ); + s.members.insert(m.name.clone(), load_value(hubris, buf, mty, maddr)?); } Ok(s) @@ -1291,7 +1297,7 @@ fn deserialize_struct<'a>( for m in &ty.members { let mty = hubris.lookup_type(m.goff)?; let out = deserialize_value(hubris, buf, mty)?; - s.members.insert(m.name.clone(), Box::new(out.0)); + s.members.insert(m.name.clone(), out.0); buf = out.1; } diff --git a/humility-doppel/src/lib.rs b/humility-doppel/src/lib.rs index f5e32e080..ab812ddec 100644 --- a/humility-doppel/src/lib.rs +++ b/humility-doppel/src/lib.rs @@ -414,9 +414,7 @@ impl humility::reflect::Load for MaybeUninit { "expected MaybeUninit, got {:?}", v_struct.name() ); - let value = v_struct - .get("value") - .ok_or_else(|| anyhow!("missing `value` member"))?; + let value = v_struct.get("value")?; T::from_value(value).map(|value| Self { value }) } } @@ -463,6 +461,7 @@ impl humility::reflect::Load for CountedRingbuf { // but don't compile in the actual ringbufs. let ringbuf = rb_struct .get("ringbuf") + .ok() .map(|value| { let cell = StaticCell::from_value(value)?; let ringbuf = Ringbuf::from_value(&cell.cell.value)?; diff --git a/humility-spd/src/lib.rs b/humility-spd/src/lib.rs index 111864b3c..c4784d74f 100644 --- a/humility-spd/src/lib.rs +++ b/humility-spd/src/lib.rs @@ -9,7 +9,7 @@ use humility::{ }; use humility_doppel as doppel; -use anyhow::{Result, bail}; +use anyhow::{Context, Result, bail}; /// Name of the packrat buffer which contains SPD data static PACKRAT_BUF_NAME: &str = "task_packrat::main::BUFS"; @@ -78,15 +78,15 @@ pub fn spd_lookup( let Value::Struct(packrat_bufs) = &as_static_cell.cell.value else { bail!("expected {PACKRAT_BUF_NAME} to be a struct"); }; - let Some(Value::Struct(compute_sled_bufs)) = packrat_bufs + let Ok(Value::Struct(compute_sled_bufs)) = packrat_bufs .get("gimlet_bufs") - .or_else(|| packrat_bufs.get("cosmo_bufs")) + .or_else(|_e| packrat_bufs.get("cosmo_bufs")) else { bail!("could not find `gimlet_bufs` or `cosmo_bufs`"); }; - let Some(spd_data) = compute_sled_bufs.get("spd_data") else { - bail!("could not find `spd_data` in sled-specific packrat bufs"); - }; + let spd_data = compute_sled_bufs.get("spd_data").context( + "could not find `spd_data` in sled-specific packrat bufs", + )?; // We have multiple versions of SPD data. In older firmwares (Gimlet // only), it's a single `[u8; 8192]` buffer; in newer firmwares, it's a