From 58242f212d40ca06b0956dec252ab2cc64430696 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 5 May 2026 16:53:49 -0400 Subject: [PATCH 1/5] Make some fields non-optional --- cmd/host/src/lib.rs | 12 ++++-------- cmd/manifest/src/lib.rs | 8 ++++---- cmd/spd/src/lib.rs | 4 ++-- humility-core/src/hubris.rs | 30 +++++++++++++++--------------- 4 files changed, 25 insertions(+), 29 deletions(-) diff --git a/cmd/host/src/lib.rs b/cmd/host/src/lib.rs index 32d78fbf..1b6e0da8 100644 --- a/cmd/host/src/lib.rs +++ b/cmd/host/src/lib.rs @@ -334,15 +334,11 @@ fn print_panic(d: Vec) -> Result<()> { /// Print a warning message if the archive is not for a `cosmo` board fn check_post_code_target(hubris: &HubrisArchive) { - if !hubris.manifest.board.as_ref().is_some_and(|b| b.contains("cosmo")) { + if !hubris.manifest.board.contains("cosmo") { humility::warn!( - "POST code buffer is only present on 'cosmo' hardware{}; \ - hiffy may fail and time out", - if let Some(board) = &hubris.manifest.board { - format!(" but this is a '{board}'") - } else { - String::new() - } + "POST code buffer is only present on 'cosmo' hardware \ + but this is a '{}'; hiffy may fail and time out", + hubris.manifest.board, ) } } diff --git a/cmd/manifest/src/lib.rs b/cmd/manifest/src/lib.rs index 5b1d8b6f..11ea5c2c 100644 --- a/cmd/manifest/src/lib.rs +++ b/cmd/manifest/src/lib.rs @@ -74,7 +74,7 @@ fn manifestcmd(context: &mut ExecutionContext) -> Result<()> { let size = |task| hubris.lookup_module(task).unwrap().memsize; - print("version", manifest.version.as_deref().unwrap_or("")); + print("version", manifest.version.as_str()); print("git rev", manifest.gitrev.as_deref().unwrap_or("")); println!( @@ -88,10 +88,10 @@ fn manifestcmd(context: &mut ExecutionContext) -> Result<()> { }, ); - print("board", manifest.board.as_deref().unwrap_or("")); - print("name", manifest.name.as_deref().unwrap_or("")); + print("board", manifest.board.as_str()); + print("name", manifest.name.as_str()); print("image", manifest.image.as_deref().unwrap_or("")); - print("target", manifest.target.as_deref().unwrap_or("")); + print("target", manifest.target.as_str()); print("features", &manifest.features.join(", ")); let ttl = hubris.modules().fold(0, |ttl, m| ttl + m.memsize); diff --git a/cmd/spd/src/lib.rs b/cmd/spd/src/lib.rs index 1376114a..707fac51 100644 --- a/cmd/spd/src/lib.rs +++ b/cmd/spd/src/lib.rs @@ -372,10 +372,10 @@ fn dump_ddr4_over_i2c( subargs: &SpdArgs, ) -> Result<()> { // Warn the user that we probably can't talk to DDR4s on non-Gimlet hardware - if !hubris.manifest.target.as_ref().is_some_and(|t| t.contains("gimlet")) { + if !hubris.manifest.target.contains("gimlet") { humility::warn!( "trying to talk to DDR4 SPDs on an invalid target `{}`", - hubris.manifest.target.as_deref().unwrap_or("") + hubris.manifest.target, ); }; diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 11287f7f..4f1ff38e 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -42,13 +42,13 @@ const MAX_HUBRIS_VERSION: u32 = 11; #[derive(Debug, Serialize)] pub struct HubrisManifest { - pub version: Option, + pub version: String, pub gitrev: Option, pub features: Vec, - pub board: Option, + pub board: String, pub image: Option, - pub name: Option, - pub target: Option, + pub name: String, + pub target: String, pub task_features: HashMap>, pub task_irqs: HashMap>, pub task_notifications: HashMap>, @@ -73,9 +73,9 @@ impl HubrisManifest { config: &HubrisConfig, rev: HubrisManifestRev, ) -> Result { - let board = Some(config.board.clone()); - let name = Some(config.name.clone()); - let target = Some(config.target.clone()); + let board = config.board.clone(); + let name = config.name.clone(); + let target = config.target.clone(); let features = match config.kernel.features { Some(ref features) => features.clone(), None => vec![], @@ -539,7 +539,7 @@ impl<'a> IntoIterator for &'a HubrisI2cBusList { /// Portions of the [`HubrisManifest`] that are loaded from archive files #[derive(Default)] pub struct HubrisManifestRev { - pub version: Option, + pub version: String, pub gitrev: Option, pub image: Option, } @@ -1365,7 +1365,7 @@ impl HubrisArchive { } manifest_rev.version = - Some(format!("hubris build archive v{}", archive_version)); + format!("hubris build archive v{}", archive_version); // Load the main manifest config file let app = hubris.extract_file("app.toml")?; @@ -2749,12 +2749,12 @@ impl HubrisArchive { // always 8-byte aligned; if we have our 17 floating point registers // here, we also have an unstored pad.) // - let (nregs_fp, align) = - if self.manifest.target.as_ref().unwrap() == "thumbv6m-none-eabi" { - (0, 0) - } else { - (17, 1) - }; + let (nregs_fp, align) = if self.manifest.target == "thumbv6m-none-eabi" + { + (0, 0) + } else { + (17, 1) + }; let nregs_frame: usize = NREGS_CORE + nregs_fp + align; From 5af988eeefb2d12081d61a40f5da8830c088a3f7 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 5 May 2026 16:56:22 -0400 Subject: [PATCH 2/5] Make imageid non-optional as well --- cmd/discover/src/lib.rs | 2 +- cmd/dump/src/lib.rs | 8 ++--- cmd/hydrate/src/lib.rs | 6 ++-- cmd/manifest/src/lib.rs | 11 +----- humility-core/src/hubris.rs | 61 +++++++++++++++------------------- humility-dump-agent/src/udp.rs | 4 +-- humility-hiffy/src/lib.rs | 6 ++-- humility-net-core/src/lib.rs | 7 +--- 8 files changed, 38 insertions(+), 67 deletions(-) diff --git a/cmd/discover/src/lib.rs b/cmd/discover/src/lib.rs index 4d3f1932..8d981ba5 100644 --- a/cmd/discover/src/lib.rs +++ b/cmd/discover/src/lib.rs @@ -315,7 +315,7 @@ fn discover_run(context: &mut ExecutionContext) -> Result<()> { let subargs = DiscoverArgs::try_parse_from(&context.cli.cmd)?; let hubris = &context.cli.try_archive()?; - let image_id = hubris.as_ref().and_then(|h| h.image_id()); + let image_id = hubris.as_ref().map(|h| h.image_id()); if image_id.is_none() { humility::warn!("no archive provided; not checking for compatibility"); } diff --git a/cmd/dump/src/lib.rs b/cmd/dump/src/lib.rs index ff4ea66a..b2de98c6 100644 --- a/cmd/dump/src/lib.rs +++ b/cmd/dump/src/lib.rs @@ -65,7 +65,7 @@ //! ``` //! -use anyhow::{Result, anyhow, bail}; +use anyhow::{Result, bail}; use clap::{ArgGroup, CommandFactory, Parser}; use humility::core::Core; use humility::hubris::*; @@ -325,11 +325,7 @@ fn get_dump_agent<'a>( { humility::msg!("using UDP dump agent"); - let imageid = &hubris - .imageid - .as_ref() - .ok_or_else(|| anyhow!("missing image ID"))? - .1; + let imageid = hubris.image_id(); Ok(Box::new(UdpDumpAgent::new(core, imageid)?)) } else { diff --git a/cmd/hydrate/src/lib.rs b/cmd/hydrate/src/lib.rs index ab416592..ccf4804b 100644 --- a/cmd/hydrate/src/lib.rs +++ b/cmd/hydrate/src/lib.rs @@ -17,7 +17,7 @@ //! By default, this writes to `hubris.core.TASK_NAME.N` (where `N` is the //! lowest available value); use `--out` to specify a different path name. -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Context, Result, bail}; use clap::{ArgGroup, CommandFactory, Parser}; use humility::hubris::HubrisFlashMap; use humility_arch_arm::ARMRegister; @@ -173,9 +173,7 @@ fn run(context: &mut ExecutionContext) -> Result<()> { // compare archive ID let archive = &context.cli.archive()?; - let expected_id = archive - .image_id() - .ok_or_else(|| anyhow!("missing image ID in archive"))?; + let expected_id = archive.image_id(); if archive_id != expected_id { bail!( "image ID mismatch: archive ID is {expected_id:02x?}, \ diff --git a/cmd/manifest/src/lib.rs b/cmd/manifest/src/lib.rs index 11ea5c2c..4178ab75 100644 --- a/cmd/manifest/src/lib.rs +++ b/cmd/manifest/src/lib.rs @@ -77,16 +77,7 @@ fn manifestcmd(context: &mut ExecutionContext) -> Result<()> { print("version", manifest.version.as_str()); print("git rev", manifest.gitrev.as_deref().unwrap_or("")); - println!( - "{:>12} => {}", - "image id", - match &hubris.imageid { - Some(s) => { - format!("{:x?}", s.1) - } - None => "".to_string(), - }, - ); + println!("{:>12} => {:x?}", "image id", hubris.image_id()); print("board", manifest.board.as_str()); print("name", manifest.name.as_str()); diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 4f1ff38e..5187cc3a 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -1156,7 +1156,7 @@ pub struct HubrisArchive { pub manifest: HubrisManifest, // image ID - pub imageid: Option<(u32, Vec)>, + imageid: (u32, Vec), // loaded regions loaded: BTreeMap, @@ -1531,9 +1531,12 @@ impl HubrisArchive { loader.structs_byname.insert(name.clone(), *goff); } + let Some(imageid) = loader.imageid else { + bail!("missing image id"); + }; Ok(Self { hubris_archive: hubris, - imageid: loader.imageid, + imageid, manifest, loaded: loader.loaded, task_dump, @@ -2092,42 +2095,30 @@ impl HubrisArchive { return Ok(()); } - // // To validate that what we're running on the target matches what // we have in the archive, we are going to check the image ID, an - // identifer created for this purpose. If we don't have an image ID, - // we check the legacy mechanism of the .hubris_app_table; if we - // don't have either of these, we don't have a way of validating the - // archive and we fail. - // - if let Some(imageid) = &self.imageid { - let addr = imageid.0; - let nbytes = imageid.1.len(); - assert!(nbytes > 0); - - let mut id = vec![0; nbytes]; - core.read_8(addr, &mut id[0..nbytes]).with_context(|| { - format!( - "failed to read image ID at 0x{:x}; board mismatch?", - addr - ) - })?; + // identifer created for this purpose. + let addr = self.imageid.0; + let nbytes = self.imageid.1.len(); + assert!(nbytes > 0); + + let mut id = vec![0; nbytes]; + core.read_8(addr, &mut id[0..nbytes]).with_context(|| { + format!("failed to read image ID at 0x{:x}; board mismatch?", addr) + })?; - let deltas = id - .iter() - .zip(imageid.1.iter()) - .filter(|&(lhs, rhs)| lhs != rhs) - .count(); + let deltas = id + .iter() + .zip(self.imageid.1.iter()) + .filter(|&(lhs, rhs)| lhs != rhs) + .count(); - if deltas > 0 || id.len() != imageid.1.len() { - bail!( + if deltas > 0 || id.len() != self.imageid.1.len() { + bail!( "image ID in archive ({:x?}) does not equal \ ID at 0x{:x} ({:x?})", - imageid.1, imageid.0, id, + self.imageid.1, self.imageid.0, id, ); - } - } else { - bail!("could not find HUBRIS_IMAGE_ID"); } if criteria == HubrisValidate::ArchiveMatch { @@ -2331,12 +2322,12 @@ impl HubrisArchive { Ok(()) } - pub fn image_id_addr(&self) -> Option { - self.imageid.as_ref().map(|i| i.0) + pub fn image_id_addr(&self) -> u32 { + self.imageid.0 } - pub fn image_id(&self) -> Option<&[u8]> { - self.imageid.as_ref().map(|i| i.1.as_slice()) + pub fn image_id(&self) -> &[u8] { + &self.imageid.1 } pub fn member_offset( diff --git a/humility-dump-agent/src/udp.rs b/humility-dump-agent/src/udp.rs index 5d479bb6..851d0503 100644 --- a/humility-dump-agent/src/udp.rs +++ b/humility-dump-agent/src/udp.rs @@ -11,7 +11,7 @@ pub struct UdpDumpAgent<'a> { } impl<'a> UdpDumpAgent<'a> { - pub fn new(core: &'a mut dyn Core, image_id: &Vec) -> Result { + pub fn new(core: &'a mut dyn Core, image_id: &[u8]) -> Result { let mut udp_dump = Self { core }; udp_dump.check_imageid(image_id)?; @@ -83,7 +83,7 @@ impl<'a> UdpDumpAgent<'a> { Ok(reply) } - fn check_imageid(&mut self, image_id: &Vec) -> Result<()> { + fn check_imageid(&mut self, image_id: &[u8]) -> Result<()> { let r = self.dump_remote_action(humpty::udp::Request::GetImageId)?; match r { diff --git a/humility-hiffy/src/lib.rs b/humility-hiffy/src/lib.rs index 8c16599c..503fc1e8 100644 --- a/humility-hiffy/src/lib.rs +++ b/humility-hiffy/src/lib.rs @@ -651,7 +651,7 @@ impl<'a> HiffyContext<'a> { let socket = workspace.socket.as_ref().unwrap(); let mut buf = vec![0u8; socket.tx.bytes]; - let image_id = hubris.image_id().unwrap(); + let image_id = hubris.image_id(); let header = RpcHeader { image_id: U64::from_bytes(image_id.try_into().unwrap()), @@ -821,7 +821,7 @@ impl<'a> HiffyContext<'a> { .lookup_variant_by_tag(Tag::from(buf[0])) { Some(e) => { - let image_id = self.hubris.image_id().unwrap(); + let image_id = self.hubris.image_id(); let msg = format!("RPC error: {}", e.name); if e.name == "BadImageId" { bail!( @@ -1043,7 +1043,7 @@ impl<'a> HiffyContext<'a> { } HiffyImpl::NetHiffy { vars, ops, errs } => { let image_id = u64::from_le_bytes( - self.hubris.image_id().unwrap().try_into().unwrap(), + self.hubris.image_id().try_into().unwrap(), ); let buf_size = self .hubris diff --git a/humility-net-core/src/lib.rs b/humility-net-core/src/lib.rs index 59637ff0..fa08b854 100644 --- a/humility-net-core/src/lib.rs +++ b/humility-net-core/src/lib.rs @@ -96,12 +96,7 @@ impl NetCore { hiffy_socket, flash: HubrisFlashMap::new(hubris)?, ram: None, // filled in below - imageid: hubris - .imageid - .as_ref() - .ok_or_else(|| anyhow!("missing image ID"))? - .1 - .clone(), + imageid: hubris.image_id().to_owned(), }; // Check for the existence of the DumpAgent.dump_task_region API, which From 33794fe3278f49075d32b977ffa7969ce69eca10 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 6 May 2026 10:43:13 -0400 Subject: [PATCH 3/5] Make constructor infallible --- humility-core/src/hubris.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 5187cc3a..d1f1b046 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -1424,7 +1424,7 @@ impl HubrisArchive { // forward slash: regardless of platform, paths within a ZIP archive // use the forward slash as a separator. // - let mut loader = HubrisObjectLoader::new(0)?; + let mut loader = HubrisObjectLoader::new(0); loader.load_object( "kernel", HubrisTask::Kernel, @@ -1479,7 +1479,7 @@ impl HubrisArchive { .into_par_iter() .map(|(id, name, buf)| { let id: u32 = id.try_into().unwrap(); - let mut loader = HubrisObjectLoader::new(id + 1)?; + let mut loader = HubrisObjectLoader::new(id + 1); loader.load_object(&name, HubrisTask::Task(id), &buf)?; Ok(loader) }) @@ -3718,8 +3718,8 @@ struct HubrisObjectLoader { } impl HubrisObjectLoader { - fn new(object_id: u32) -> Result { - Ok(Self { + fn new(object_id: u32) -> Self { + Self { object_id, imageid: None, arrays: HashMap::new(), @@ -3748,7 +3748,7 @@ impl HubrisObjectLoader { structs_byname: MultiMap::new(), subprograms: HashMap::new(), syscall_pushes: HashMap::new(), - }) + } } fn merge(&mut self, loader: HubrisObjectLoader) -> Result<()> { From eb7dea6970a0ae15a25428a01e8b24b50ebbb214 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 6 May 2026 14:21:27 -0400 Subject: [PATCH 4/5] Remove Default for HubrisManifestRev --- humility-core/src/hubris.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index d1f1b046..5f155c5a 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -537,7 +537,6 @@ impl<'a> IntoIterator for &'a HubrisI2cBusList { } /// Portions of the [`HubrisManifest`] that are loaded from archive files -#[derive(Default)] pub struct HubrisManifestRev { pub version: String, pub gitrev: Option, @@ -1353,19 +1352,23 @@ impl HubrisArchive { } // First, we'll load aspects of configuration. - let mut manifest_rev = HubrisManifestRev::default(); - if let Ok(git_rev) = hubris.extract_file("git-rev") { - manifest_rev.gitrev = - Some(std::str::from_utf8(&git_rev)?.to_string()); - } + let gitrev = if let Ok(git_rev) = hubris.extract_file("git-rev") { + Some(std::str::from_utf8(&git_rev)?.to_string()) + } else { + None + }; - if let Ok(image_name) = hubris.extract_file("image-name") { - manifest_rev.image = - Some(std::str::from_utf8(&image_name)?.to_string()); - } + let image = if let Ok(image_name) = hubris.extract_file("image-name") { + Some(std::str::from_utf8(&image_name)?.to_string()) + } else { + None + }; - manifest_rev.version = - format!("hubris build archive v{}", archive_version); + let manifest_rev = HubrisManifestRev { + version: format!("hubris build archive v{archive_version}"), + gitrev, + image, + }; // Load the main manifest config file let app = hubris.extract_file("app.toml")?; From 57d5f1c37120ff14ae0e0091242b1cbd23ead7ac Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Thu, 7 May 2026 07:39:01 -0400 Subject: [PATCH 5/5] Remove loaded() and take_raw_archive() functions --- humility-core/src/hubris.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 5f155c5a..7610fdda 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -1613,11 +1613,6 @@ impl HubrisArchive { flash.chip } - /// Destroys the `HubrisArchive`, returning the raw archive data - pub fn take_raw_archive(self) -> Vec { - self.hubris_archive.zip - } - /// Helper function to load a dump into a [`RawHubrisArchive`] /// /// Returns a tuple of `(raw archive, dump task)`; the second field is @@ -1687,10 +1682,6 @@ impl HubrisArchive { Ok((archive, task_dump)) } - pub fn loaded(&self) -> bool { - !self.modules.is_empty() - } - /// /// Takes a list of potentially similar types and deduplicates the list. ///