From 13e9bdd93eb96973374f96714fe59ee3c016c7c3 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 3 Jun 2026 11:10:57 -0400 Subject: [PATCH 1/5] Working on a `humility-flash` crate --- Cargo.lock | 13 +- Cargo.toml | 4 +- cmd/flash/Cargo.toml | 3 +- cmd/flash/src/lib.rs | 195 +++++----------------------- humility-flash/Cargo.toml | 13 ++ humility-flash/src/lib.rs | 266 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 329 insertions(+), 165 deletions(-) create mode 100644 humility-flash/Cargo.toml create mode 100644 humility-flash/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index fdaa8c61a..1d48bb7c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1904,9 +1904,9 @@ version = "0.1.0" dependencies = [ "anyhow", "clap", - "humility-auxflash", "humility-cli", "humility-core", + "humility-flash", "humility-probes-core", "parse_int", ] @@ -2635,6 +2635,17 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "humility-flash" +version = "0.1.0" +dependencies = [ + "anyhow", + "humility-auxflash", + "humility-core", + "humility-probes-core", + "thiserror 2.0.18", +] + [[package]] name = "humility-hexdump" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 582a9a39e..bec7aa652 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,7 @@ members = [ "humility-core", "humility-doppel", "humility-dump-agent", + "humility-flash", "humility-hexdump", "humility-hiffy", "humility-i2c", @@ -78,7 +79,7 @@ members = [ "cmd/validate", "cmd/vpd", "cmd/writeword", - "xtask", "humility-log", + "xtask", ] resolver = "2" @@ -119,6 +120,7 @@ humility-cortex = { path = "./humility-arch-cortex" } humility-cli = { path = "./humility-cli", default-features = false } humility-doppel = { path = "./humility-doppel" } humility-dump-agent = { path = "./humility-dump-agent" } +humility-flash = { path = "./humility-flash" } humility-hexdump = { path = "./humility-hexdump" } humility-hiffy = { path = "./humility-hiffy" } humility-i2c = { path = "./humility-i2c" } diff --git a/cmd/flash/Cargo.toml b/cmd/flash/Cargo.toml index d3ce3d99e..965e0e941 100644 --- a/cmd/flash/Cargo.toml +++ b/cmd/flash/Cargo.toml @@ -8,7 +8,8 @@ description = "flash archive onto attached device" humility = { workspace = true } humility-cli = { workspace = true } humility-probes-core = { workspace = true } -humility-auxflash = { workspace = true } +humility-flash = { workspace = true } + anyhow = { workspace = true } clap = { workspace = true } parse_int = { workspace = true } diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index e7e6b5530..08bf85325 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -26,8 +26,8 @@ use humility::{ hubris::*, log::{Logger, info}, }; -use humility_auxflash::{AuxFlashHandler, AuxFlashWriter}; use humility_cli::{ExecutionContext, humility_cmd}; +use humility_flash::{get_image_state, program_auxflash}; #[derive(Parser, Debug)] #[clap(name = "flash", about = env!("CARGO_PKG_DESCRIPTION"))] @@ -85,13 +85,19 @@ fn validate( subargs: &FlashArgs, log: &Logger, ) -> Result<()> { - let r = get_image_state( - hubris, - core, - subargs.verify || subargs.check, - subargs.verbose, - log, - ); + let check_type = if subargs.verify || subargs.check { + humility_flash::ImageCheckType::ImageIdAndFlash { + check_every_byte: subargs.verbose, + } + } else { + humility_flash::ImageCheckType::ImageId + }; + // Collapse both Humility-level errors and mismatch errors into `Err(..)` + let r = match get_image_state(hubris, core, check_type, log) { + Ok(humility_flash::ImageStateResult::Matches) => Ok(()), + Ok(humility_flash::ImageStateResult::DoesNotMatch(e)) => Err(e.into()), + Err(e) => Err(e.into()), + }; if subargs.check { core.run()?; return r; @@ -124,82 +130,6 @@ fn validate( } } -/// Checks the image and auxiliary flash -/// -/// Returns `Ok(())` if the image matches; returns an error otherwise. -/// -/// The core is halted when this function exits. -fn get_image_state( - hubris: &HubrisArchive, - core: &mut dyn humility::core::Core, - full_check: bool, - verbose: bool, - log: &Logger, -) -> Result<()> { - core.halt()?; - - // First pass: check only the image ID - hubris - .validate(core, HubrisValidate::ArchiveMatch) - .context("flash/archive mismatch")?; - - // More rigorous checks if requested - if full_check { - hubris.verify(core, verbose, log).with_context(|| { - let mut s = "image IDs match, but flash contents do not \ - match archive contents" - .to_owned(); - if !verbose { - s += ". Add `--verbose` to see all byte mismatches." - } - s - })?; - } - - if hubris.read_auxflash_data()?.is_some() { - // The core must be running for us to check the auxflash slot. - // - // However, we want it halted before we exit this function, which - // requires careful handling of functions that could bail out. - core.run()?; - - // Note that we only run this check if we pass the image ID check; - // otherwise, the Idol / hiffy memory maps are unknown. - let mut worker = match AuxFlashHandler::new( - hubris, - core, - std::time::Duration::from_millis(15_000), - log, - ) { - Ok(w) => w, - Err(e) => { - // Halt the core before returning! - core.halt()?; - return Err(e); - } - }; - let r = match worker.active_slot() { - Ok(Some(s)) => { - info!(log, "verified auxflash in slot {s}"); - Ok(()) - } - Ok(None) => Err(anyhow!("no active auxflash slot")), - Err(e) => Err(anyhow!("failed to check auxflash slot: {e}")), - }; - - // Halt the core before returning results - core.halt()?; - - r.context(if full_check { - "image ID and flash contents match, but auxflash is not loaded" - } else { - "image IDs match, but auxflash is not loaded" - }) - } else { - Ok(()) - } -} - fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { let hubris = &context.cli.archive()?; let log = context.log(); @@ -277,89 +207,30 @@ fn try_program_auxflash( core: &mut humility_probes_core::ProbeCore, log: &Logger, ) -> Result<()> { + use humility_flash::ProgramAuxflashSuccess; match hubris.read_auxflash_data()? { - Some(auxflash) => { - match program_auxflash(hubris, core, &auxflash, log) { - Ok(_) => { - info!(log, "done with auxiliary flash"); - Ok(()) + Some(auxflash) => match program_auxflash(hubris, core, &auxflash, log) { + Ok(r) => { + match r { + ProgramAuxflashSuccess::AlreadyProgrammed { slot } => { + info!( + log, + "auxiliary flash data is already loaded in slot \ + {slot}; skipping programming", + ) + } + ProgramAuxflashSuccess::Success => { + info!(log, "successfully programmed auxiliary flash") + } } - Err(e) => bail!( - "failed to program auxflash: {e:?}; \ - your system may not be functional!", - ), + Ok(()) } - } + Err(e) => Err(e).context( + "failed to program auxflash; your system may not be functional", + ), + }, None => Ok(()), } } -fn program_auxflash( - hubris: &HubrisArchive, - core: &mut humility_probes_core::ProbeCore, - data: &[u8], - log: &Logger, -) -> Result<()> { - let mut worker = AuxFlashWriter::new( - hubris, - core, - std::time::Duration::from_millis(15_000), - log, - )?; - - // At this point, we've already rebooted into the new image. - // - // If the Hubris auxflash task has picked up an active slot, then we're all - // set: our target image was already loaded (or was unchanged). - match worker.active_slot() { - Ok(Some(i)) => { - info!( - log, - "auxiliary flash data is already loaded in slot {i}; \ - skipping programming", - ); - return Ok(()); - } - Ok(None) => (), - Err(e) => { - info!(log, "Got error while checking active slot: {e:?}"); - } - }; - - // Otherwise, we need to pick a slot. This is tricky, because we don't - // actually know whether there's an image on the B partition that's using - // auxiliary data. We'll prioritize picking an empty (even) slot, and will - // otherwise pick slot 0 arbitrarily. - let slot_count = worker.slot_count()?; - let mut target_slot = 0; - for i in 0..slot_count { - if i % 2 == 0 && matches!(worker.slot_status(i), Ok(None)) { - target_slot = i; - break; - } - } - - worker.auxflash_write(target_slot, data, false)?; - - // After a reset, two things will happen: - // - The SP `auxflash` task will automatically mirror from the even slot to - // the odd slot, since the even slot will have valid data and the odd slot - // will not. - // - The SP will recognize the programmed slot as valid and choose it as the - // active slot. - worker.reset()?; - - // Give the SP plenty of time to do its mirroring operation - info!(log, "resetting the SP, please wait..."); - std::thread::sleep(std::time::Duration::from_secs(5)); - - match worker.active_slot() { - Ok(Some(..)) => Ok(()), - Ok(None) => bail!("No active auxflash slot, even after programming"), - Err(e) => { - bail!("Could not check auxflash slot after programming: {:?}", e) - } - } -} - humility_cmd!(FlashArgs, flashcmd); diff --git a/humility-flash/Cargo.toml b/humility-flash/Cargo.toml new file mode 100644 index 000000000..7339f45f5 --- /dev/null +++ b/humility-flash/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "humility-flash" +version = "0.1.0" +edition.workspace = true +rust-version.workspace = true + +[dependencies] +humility.workspace = true +humility-probes-core.workspace = true +humility-auxflash.workspace = true + +anyhow.workspace = true # wrapping other errors +thiserror.workspace = true # error types defined in this crate diff --git a/humility-flash/src/lib.rs b/humility-flash/src/lib.rs new file mode 100644 index 000000000..14959baa1 --- /dev/null +++ b/humility-flash/src/lib.rs @@ -0,0 +1,266 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Functions to check flash state and reprogram a processor +#![warn(missing_docs)] + +use humility::{ + core::Core, + hubris::{HubrisArchive, HubrisValidate}, + log::{Logger, info}, +}; +use humility_auxflash::{AuxFlashHandler, AuxFlashWriter}; +use humility_probes_core::ProbeCore; + +/// Result of checking image state +#[must_use] +pub enum ImageStateResult { + /// Image state check succeeded + Matches, + /// Image state does not match + DoesNotMatch(ImageStateMismatch), +} + +/// Error type when checking image state +#[derive(Debug, thiserror::Error)] +pub enum ImageStateError { + /// Could not read auxflash from archive + #[error("could not read auxflash from archive")] + CouldNotReadAuxflash(#[source] anyhow::Error), + + /// Could not call [`Core::run`] + #[error("run failed")] + CouldNotRun(#[source] anyhow::Error), + + /// Could not call [`Core::halt`] + #[error("halt failed")] + CouldNotHalt(#[source] anyhow::Error), + + /// Could not build an [`AuxFlashHandler`] + #[error("could not build auxflash handler")] + CouldNotBuildHandler(#[source] anyhow::Error), + + /// [`AuxFlashHandler::active_slot`] returned an error + #[error("could not get active slot")] + CouldNotGetAuxflashSlot(#[source] anyhow::Error), +} + +/// Type indicating a mismatch when checking image state +#[derive(Debug, thiserror::Error)] +pub enum ImageStateMismatch { + /// Could not halt core + #[error("flash/archive mismatch")] + FlashArchiveMismatch(#[source] anyhow::Error), + + /// Call to [`HubrisArchive::verify`] failed + #[error("flash contents do not match archive contents")] + VerifyFailed(#[source] anyhow::Error), + + /// The processor does not have an active auxflash slot + #[error( + "archive has auxflash data but processor does \ + not have an active auxflash slot" + )] + NoActiveAuxflashSlot, +} + +/// Type of image checks that can be run +pub enum ImageCheckType { + /// Check that the image ID matches + ImageId, + /// Check that the image ID matches and that flash data matches + ImageIdAndFlash { + /// Check every single byte, rather than bailing on the first mismatch + check_every_byte: bool, + }, +} + +/// Checks the image and auxiliary flash against a Hubris archive +/// +/// Returns `Ok(r)` if the function succeeded; `r` may indicate that the check +/// itself is either successful or unsuccessful. +/// +/// Returns `Err(..)` on a Humility-level error. +/// +/// The core is halted when this function exits. +pub fn get_image_state( + hubris: &HubrisArchive, + core: &mut dyn Core, + check_type: ImageCheckType, + log: &Logger, +) -> Result { + core.halt().map_err(ImageStateError::CouldNotHalt)?; + + // First pass: check only the image ID + if let Err(e) = hubris.validate(core, HubrisValidate::ArchiveMatch) { + return Ok(ImageStateResult::DoesNotMatch( + ImageStateMismatch::FlashArchiveMismatch(e), + )); + } + + // More rigorous checks if requested + if let ImageCheckType::ImageIdAndFlash { check_every_byte } = check_type + && let Err(e) = hubris.verify(core, check_every_byte, log) + { + return Ok(ImageStateResult::DoesNotMatch( + ImageStateMismatch::VerifyFailed(e), + )); + } + + if hubris + .read_auxflash_data() + .map_err(ImageStateError::CouldNotReadAuxflash)? + .is_some() + { + // The core must be running for us to check the auxflash slot. + // + // However, we want it halted before we exit this function, which + // requires careful handling of functions that could bail out. + core.run().map_err(ImageStateError::CouldNotRun)?; + + // Note that we only run this check if we pass the image ID check; + // otherwise, the Idol / hiffy memory maps are unknown. + let mut worker = match AuxFlashHandler::new( + hubris, + core, + std::time::Duration::from_millis(15_000), + log, + ) { + Ok(w) => w, + Err(e) => { + // Halt the core before returning! + core.halt().map_err(ImageStateError::CouldNotHalt)?; + return Err(ImageStateError::CouldNotBuildHandler(e)); + } + }; + let r = match worker.active_slot() { + Ok(Some(s)) => { + info!(log, "verified auxflash in slot {s}"); + Ok(ImageStateResult::Matches) + } + Ok(None) => Ok(ImageStateResult::DoesNotMatch( + ImageStateMismatch::NoActiveAuxflashSlot, + )), + Err(e) => Err(ImageStateError::CouldNotGetAuxflashSlot(e)), + }; + + // Halt the core before returning results + core.halt().map_err(ImageStateError::CouldNotHalt)?; + r + } else { + Ok(ImageStateResult::Matches) + } +} + +/// Error type when programming auxflash +#[derive(Debug, thiserror::Error)] +pub enum ProgramAuxflashError { + /// Could not build an [`AuxFlashHandler`] + #[error("could not build auxflash handler")] + CouldNotBuildHandler(#[source] anyhow::Error), + + /// [`AuxFlashHandler::active_slot`] returned an error + #[error("could not get active slot {} programming", + if *before { "before" } else { "after" })] + CouldNotGetAuxflashSlot { + /// Original error + #[source] + err: anyhow::Error, + /// Whether this occurred before or after programming auxflash + before: bool, + }, + + /// [`AuxFlashHandler::slot_count`] returned an error + #[error("could not get slot_count")] + CouldNotGetSlotCount(#[source] anyhow::Error), + + /// [`AuxFlashWriter::auxflash_write`] returned an error + #[error("could not write auxflash")] + WriteFailed(#[source] anyhow::Error), + + /// [`ProbeCore::reset`] returned an error + #[error("could not reset processor")] + ResetFailed(#[source] anyhow::Error), + + /// No active auxflash slot after programming + #[error("no active auxflash slot, even after programming")] + NoActiveSlot, +} + +/// Success type for programming auxflash +pub enum ProgramAuxflashSuccess { + /// The image successfully booted, meaning our data was already programmed + AlreadyProgrammed { + /// Which slot the auxflash is using + slot: u32, + }, + /// We successfully programmed the auxflash + Success, +} + +/// Writes the given data to an auxflash slot +pub fn program_auxflash( + hubris: &HubrisArchive, + core: &mut ProbeCore, + data: &[u8], + log: &Logger, +) -> Result { + let mut worker = AuxFlashWriter::new( + hubris, + core, + std::time::Duration::from_millis(15_000), + log, + ) + .map_err(ProgramAuxflashError::CouldNotBuildHandler)?; + + // At this point, we've already rebooted into the new image. + // + // If the Hubris auxflash task has picked up an active slot, then we're all + // set: our target image was already loaded (or was unchanged). + if let Some(i) = worker.active_slot().map_err(|err| { + ProgramAuxflashError::CouldNotGetAuxflashSlot { err, before: true } + })? { + return Ok(ProgramAuxflashSuccess::AlreadyProgrammed { slot: i }); + } + + // Otherwise, we need to pick a slot. This is tricky, because we don't + // actually know whether there's an image on the B partition that's using + // auxiliary data. We'll prioritize picking an empty (even) slot, and will + // otherwise pick slot 0 arbitrarily. + let slot_count = worker + .slot_count() + .map_err(ProgramAuxflashError::CouldNotGetSlotCount)?; + let mut target_slot = 0; + for i in 0..slot_count { + if i % 2 == 0 && matches!(worker.slot_status(i), Ok(None)) { + target_slot = i; + break; + } + } + + worker + .auxflash_write(target_slot, data, false) + .map_err(ProgramAuxflashError::WriteFailed)?; + + // After a reset, two things will happen: + // - The SP `auxflash` task will automatically mirror from the even slot to + // the odd slot, since the even slot will have valid data and the odd slot + // will not. + // - The SP will recognize the programmed slot as valid and choose it as the + // active slot. + worker.reset().map_err(ProgramAuxflashError::ResetFailed)?; + + // Give the SP plenty of time to do its mirroring operation + info!(log, "resetting the SP, please wait..."); + std::thread::sleep(std::time::Duration::from_secs(5)); + + match worker.active_slot() { + Ok(Some(..)) => Ok(ProgramAuxflashSuccess::Success), + Ok(None) => Err(ProgramAuxflashError::NoActiveSlot), + Err(err) => Err(ProgramAuxflashError::CouldNotGetAuxflashSlot { + err, + before: false, + }), + } +} From 3f8cdb6ef1eef561616ebba420956476d4f0450c Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 10 Jun 2026 14:01:39 -0400 Subject: [PATCH 2/5] Split `validate` into two functions --- cmd/flash/src/lib.rs | 131 ++++++++++++++++---------------------- humility-flash/src/lib.rs | 2 +- 2 files changed, 56 insertions(+), 77 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index 08bf85325..0da77f32b 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -19,7 +19,7 @@ //! will be programmed after the image is written. See RFD 311 for more //! information about auxiliary flash management. -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Context, Result, bail}; use clap::Parser; use humility::{ core::Core, @@ -36,19 +36,6 @@ pub struct FlashArgs { #[clap(long, short = 'F')] force: bool, - /// if using OpenOCD, do not actually flash, but show commands and retain - /// any temporary files - #[clap(long = "dry-run", short = 'n')] - dryrun: bool, - - /// retain any temporary files - #[clap(long = "retain-temporaries", short = 'R')] - retain: bool, - - /// force usage of OpenOCD - #[clap(long = "force-openocd", short = 'O')] - force_openocd: bool, - /// reset delay #[clap( long = "reset-delay", short = 'd', @@ -70,98 +57,61 @@ pub struct FlashArgs { verbose: bool, } -/// Validates the image and auxiliary flash against our subcommand -/// -/// If `subargs.check` is true, returns `Ok(())` on a clean check and `Err(..)` -/// otherwise; the core is left running. -/// -/// If `subargs.check` is false, returns `Ok(())` if the check _fails_ (meaning -/// we should reflash), and `Err(..)` if all checks pass (meaning we should -/// _not_ reflash). The core is left halted if we should reflash and is running -/// otherwise. -fn validate( +fn flash_check( hubris: &HubrisArchive, core: &mut humility_probes_core::ProbeCore, + verbose: bool, + log: &Logger, +) -> Result<()> { + let check_type = humility_flash::ImageCheckType::ImageIdAndFlash { + check_every_byte: verbose, + }; + let out = match get_image_state(hubris, core, check_type, log)? { + humility_flash::ImageStateResult::Matches => Ok(()), + humility_flash::ImageStateResult::DoesNotMatch(e) => Err(e.into()), + }; + core.run()?; + out +} + +fn flash_program( + hubris: &HubrisArchive, + core: &mut humility_probes_core::ProbeCore, + config: &HubrisFlashConfig, subargs: &FlashArgs, log: &Logger, ) -> Result<()> { - let check_type = if subargs.verify || subargs.check { + let check_type = if subargs.verify { humility_flash::ImageCheckType::ImageIdAndFlash { check_every_byte: subargs.verbose, } } else { humility_flash::ImageCheckType::ImageId }; - // Collapse both Humility-level errors and mismatch errors into `Err(..)` - let r = match get_image_state(hubris, core, check_type, log) { - Ok(humility_flash::ImageStateResult::Matches) => Ok(()), - Ok(humility_flash::ImageStateResult::DoesNotMatch(e)) => Err(e.into()), - Err(e) => Err(e.into()), - }; - if subargs.check { - core.run()?; - return r; - } - match r { - Ok(()) => { + match get_image_state(hubris, core, check_type, log)? { + humility_flash::ImageStateResult::Matches => { if subargs.force { info!( log, "archive appears to be already flashed; forcing re-flash" ); - Ok(()) } else { core.run()?; - Err(anyhow!(if subargs.verify { + bail!(if subargs.verify { "archive is already flashed on attached device; use -F \ (\"--force\") to force re-flash" } else { "archive appears to be already flashed on attached \ device; use -F (\"--force\") to force re-flash or -V \ (\"--verify\") to verify contents" - })) + }) } } - Err(e) => { + humility_flash::ImageStateResult::DoesNotMatch(e) => { info!(log, "{e}; reflashing"); - Ok(()) } } -} - -fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { - let hubris = &context.cli.archive()?; - let log = context.log(); - - let config = hubris.load_flash_config()?; - - if subargs.force_openocd { - bail!("openocd no longer supported"); - } - - let probe = match &context.cli.probe { - Some(p) => p, - None => "auto", - }; - - let chip = match config.chip { - Some(c) => c, - None => bail!("Archive is very old and missing a chip"), - }; - - info!(log, "attaching with chip set to {chip:x?}"); - let core = &mut humility_probes_core::attach_for_flashing( - probe, - &chip, - context.cli.speed, - log, - )?; - - validate(hubris, core, &subargs, log)?; - if subargs.check { - return Ok(()); - } // // Load the flash image. If that fails, we're in a world of hurt: we @@ -202,6 +152,35 @@ fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { Ok(()) } +fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { + let hubris = &context.cli.archive()?; + let log = context.log(); + + let config = hubris.load_flash_config()?; + let probe = match &context.cli.probe { + Some(p) => p, + None => "auto", + }; + + let Some(chip) = &config.chip else { + bail!("Archive is very old and missing a chip") + }; + + info!(log, "attaching with chip set to {chip:x?}"); + let core = &mut humility_probes_core::attach_for_flashing( + probe, + chip, + context.cli.speed, + log, + )?; + + if subargs.check { + flash_check(hubris, core, subargs.verbose, log) + } else { + flash_program(hubris, core, &config, &subargs, log) + } +} + fn try_program_auxflash( hubris: &HubrisArchive, core: &mut humility_probes_core::ProbeCore, diff --git a/humility-flash/src/lib.rs b/humility-flash/src/lib.rs index 14959baa1..c2e66b390 100644 --- a/humility-flash/src/lib.rs +++ b/humility-flash/src/lib.rs @@ -49,7 +49,7 @@ pub enum ImageStateError { /// Type indicating a mismatch when checking image state #[derive(Debug, thiserror::Error)] pub enum ImageStateMismatch { - /// Could not halt core + /// Failed at the archive ID check #[error("flash/archive mismatch")] FlashArchiveMismatch(#[source] anyhow::Error), From 9ba599acd49613f709aa83312948735622c0b864 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 10 Jun 2026 14:29:00 -0400 Subject: [PATCH 3/5] Move loading into library crate as well --- cmd/flash/src/lib.rs | 98 +++++++++--------------------- humility-cli/src/lib.rs | 2 +- humility-core/src/hubris.rs | 50 +++++++++------- humility-flash/src/lib.rs | 102 ++++++++++++++++++++++++++++++-- humility-probes-core/src/lib.rs | 4 +- 5 files changed, 154 insertions(+), 102 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index 0da77f32b..a05b80aa9 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -19,7 +19,7 @@ //! will be programmed after the image is written. See RFD 311 for more //! information about auxiliary flash management. -use anyhow::{Context, Result, bail}; +use anyhow::{Result, bail}; use clap::Parser; use humility::{ core::Core, @@ -27,7 +27,7 @@ use humility::{ log::{Logger, info}, }; use humility_cli::{ExecutionContext, humility_cmd}; -use humility_flash::{get_image_state, program_auxflash}; +use humility_flash::{ProgramAuxflashSuccess, get_image_state, program_image}; #[derive(Parser, Debug)] #[clap(name = "flash", about = env!("CARGO_PKG_DESCRIPTION"))] @@ -113,56 +113,43 @@ fn flash_program( } } - // - // Load the flash image. If that fails, we're in a world of hurt: we - // really don't want to run the core for fear of masking the initial - // error. (It will hopefully be pretty clear to the user that a - // half-flashed part is going to be in an ill-defined state!) - // - core.load(&config.elf)?; - - // - // On Gimlet Rev B, the BOOT0 pin is unstrapped -- and during a flash, - // it seems to float high enough to bounce the part onto the wrong - // image (that is, the BOOT1 image -- which by default is the ST - // bootloader). This seems to only be true when resetting immediately - // after flashing the part: if there is a delay on the order of ~35 - // milliseconds or more, the BOOT0 pin is seen as low when the part - // resets. Because this delay is (more or less) harmless, we do it on - // all platforms, and further make it tunable. - // - let delay = subargs.reset_delay; - - if delay != 0 { - std::thread::sleep(std::time::Duration::from_millis(delay)); + let reset_delay = if subargs.reset_delay == 0 { + None + } else { + Some(std::time::Duration::from_millis(subargs.reset_delay)) + }; + match program_image(hubris, core, reset_delay, log) { + Ok(s) => { + match s.auxflash { + Some(ProgramAuxflashSuccess::AlreadyProgrammed { slot }) => { + info!( + log, + "auxiliary flash data is already loaded in slot \ + {slot}; skipping programming", + ) + } + Some(ProgramAuxflashSuccess::Success) => { + info!(log, "successfully programmed auxiliary flash") + } + None => (), + } + info!(log, "flashing done"); + Ok(()) + } + Err(e) => Err(e.into()), } - - // Reset, using the handoff token if present in the archive - core.reset_with_handoff(hubris, log)?; - - // At this point, we can attempt to program the auxiliary flash. This has - // to happen *after* the image is flashed and the core is reset, because it - // uses hiffy calls to the `auxflash` task to actually do the programming; - // because we have no knowledge of the archive previously flashed onto the - // chip, we couldn't do hiffy calls before flashing. - // - // This is called out in RFD 311 as a weakness of our approach! - try_program_auxflash(hubris, core, log)?; - info!(log, "flashing done"); - Ok(()) } fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { let hubris = &context.cli.archive()?; let log = context.log(); - let config = hubris.load_flash_config()?; let probe = match &context.cli.probe { Some(p) => p, None => "auto", }; - let Some(chip) = &config.chip else { + let Some(chip) = &hubris.chip()? else { bail!("Archive is very old and missing a chip") }; @@ -181,35 +168,4 @@ fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { } } -fn try_program_auxflash( - hubris: &HubrisArchive, - core: &mut humility_probes_core::ProbeCore, - log: &Logger, -) -> Result<()> { - use humility_flash::ProgramAuxflashSuccess; - match hubris.read_auxflash_data()? { - Some(auxflash) => match program_auxflash(hubris, core, &auxflash, log) { - Ok(r) => { - match r { - ProgramAuxflashSuccess::AlreadyProgrammed { slot } => { - info!( - log, - "auxiliary flash data is already loaded in slot \ - {slot}; skipping programming", - ) - } - ProgramAuxflashSuccess::Success => { - info!(log, "successfully programmed auxiliary flash") - } - } - Ok(()) - } - Err(e) => Err(e).context( - "failed to program auxflash; your system may not be functional", - ), - }, - None => Ok(()), - } -} - humility_cmd!(FlashArgs, flashcmd); diff --git a/humility-cli/src/lib.rs b/humility-cli/src/lib.rs index 09f4ff6c6..ac90773cc 100644 --- a/humility-cli/src/lib.rs +++ b/humility-cli/src/lib.rs @@ -219,7 +219,7 @@ impl Cli { hubris: Option<&HubrisArchive>, ) -> Result { let probe = self.probe.as_deref().unwrap_or("auto"); - let chip = hubris.and_then(|h| h.chip()); + let chip = hubris.map(|h| h.chip()).transpose()?.flatten(); humility_probes_core::attach_to_chip( probe, chip.as_deref(), diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index e0085ff11..437b79c56 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -1645,7 +1645,31 @@ impl HubrisArchive { }) } + /// Returns a [`HubrisFlashConfig`] from data in the archive pub fn load_flash_config(&self) -> Result { + let metadata = self.load_flash_meta()?; + + // This is incredibly ugly! It also gives us backwards compatibility! + let chip: Option = match metadata.chip { + Some(ref chip) => Some(chip.to_string()), + None => bail!("must specify a chip in your config"), + }; + + Ok(HubrisFlashConfig { + metadata, + elf: self.hubris_archive.extract_file("img/final.elf")?, + chip, + }) + } + + /// Returns the ELF file data to be programmed + pub fn load_flash_elf(&self) -> Result> { + let data = self.hubris_archive.extract_file("img/final.elf")?; + Ok(data) + } + + /// Returns [`HubrisFlashMeta`] from the archive + pub fn load_flash_meta(&self) -> Result { let flash_ron = self .hubris_archive .extract_file("img/flash.ron") @@ -1659,30 +1683,12 @@ impl HubrisArchive { let config: HubrisFlashMeta = ron::options::Options::default() .with_default_extension(ron::extensions::Extensions::IMPLICIT_SOME) .from_bytes(&flash_ron)?; - - // This is incredibly ugly! It also gives us backwards compatibility! - let chip: Option = match config.chip { - Some(ref chip) => Some(chip.to_string()), - None => bail!("must specify a chip in your config"), - }; - - Ok(HubrisFlashConfig { - metadata: config, - elf: self.hubris_archive.extract_file("img/final.elf")?, - chip, - }) + Ok(config) } - pub fn chip(&self) -> Option { - // It turns out the easiest way right now to get the chip is via the - // flash config. Long term we may want to fix this - // - - let flash = match self.load_flash_config() { - Ok(f) => f, - Err(_) => return None, - }; - flash.chip + /// Returns the chip name (to be used for flashing) + pub fn chip(&self) -> Result> { + self.load_flash_meta().map(|m| m.chip) } /// Helper function to load a dump into a [`RawHubrisArchive`] diff --git a/humility-flash/src/lib.rs b/humility-flash/src/lib.rs index c2e66b390..36e174528 100644 --- a/humility-flash/src/lib.rs +++ b/humility-flash/src/lib.rs @@ -31,11 +31,11 @@ pub enum ImageStateError { /// Could not call [`Core::run`] #[error("run failed")] - CouldNotRun(#[source] anyhow::Error), + RunFailed(#[source] anyhow::Error), /// Could not call [`Core::halt`] #[error("halt failed")] - CouldNotHalt(#[source] anyhow::Error), + HaltFailed(#[source] anyhow::Error), /// Could not build an [`AuxFlashHandler`] #[error("could not build auxflash handler")] @@ -90,7 +90,7 @@ pub fn get_image_state( check_type: ImageCheckType, log: &Logger, ) -> Result { - core.halt().map_err(ImageStateError::CouldNotHalt)?; + core.halt().map_err(ImageStateError::HaltFailed)?; // First pass: check only the image ID if let Err(e) = hubris.validate(core, HubrisValidate::ArchiveMatch) { @@ -117,7 +117,7 @@ pub fn get_image_state( // // However, we want it halted before we exit this function, which // requires careful handling of functions that could bail out. - core.run().map_err(ImageStateError::CouldNotRun)?; + core.run().map_err(ImageStateError::RunFailed)?; // Note that we only run this check if we pass the image ID check; // otherwise, the Idol / hiffy memory maps are unknown. @@ -130,7 +130,7 @@ pub fn get_image_state( Ok(w) => w, Err(e) => { // Halt the core before returning! - core.halt().map_err(ImageStateError::CouldNotHalt)?; + core.halt().map_err(ImageStateError::HaltFailed)?; return Err(ImageStateError::CouldNotBuildHandler(e)); } }; @@ -146,7 +146,7 @@ pub fn get_image_state( }; // Halt the core before returning results - core.halt().map_err(ImageStateError::CouldNotHalt)?; + core.halt().map_err(ImageStateError::HaltFailed)?; r } else { Ok(ImageStateResult::Matches) @@ -264,3 +264,93 @@ pub fn program_auxflash( }), } } + +/// Success type returned from [`program_image`] +pub struct ProgramImageSuccess { + /// Result of programming the auxiliary flash (if relevant) + pub auxflash: Option, +} + +/// Error type returned from [`program_image`] +#[derive(Debug, thiserror::Error)] +pub enum ProgramImageError { + /// Could not call [`Core::halt`] + #[error("halt failed")] + HaltFailed(#[source] anyhow::Error), + + /// Could not call [`ProbeCore::load`] + #[error("load failed")] + CouldNotLoad(#[source] humility_probes_core::LoadError), + + /// Failed to call [`ProbeCore::reset`] + #[error("reset failed")] + ResetFailed(#[source] anyhow::Error), + + /// Could not read auxflash data from archive + #[error("could not read auxflash data from archive")] + CouldNotReadAuxflashData(#[source] anyhow::Error), + + /// Could not read ELF data using [`HubrisArchive::load_flash_elf`] + #[error("could not read image ELF data from archive")] + CouldNotReadElfData(#[source] anyhow::Error), + + /// Error while programming auxflash + #[error("failed to program auxflash; your system may not be functional")] + Auxflash(#[from] ProgramAuxflashError), +} + +/// Write an image (including auxflash) to a particular probe +pub fn program_image( + hubris: &HubrisArchive, + core: &mut ProbeCore, + reset_delay: Option, + log: &Logger, +) -> Result { + // The core may already be halted, but this is idempotent + core.halt().map_err(ProgramImageError::HaltFailed)?; + + // + // Load the flash image. If that fails, we're in a world of hurt: we + // really don't want to run the core for fear of masking the initial + // error. (It will hopefully be pretty clear to the user that a + // half-flashed part is going to be in an ill-defined state!) + // + let elf = hubris + .load_flash_elf() + .map_err(ProgramImageError::CouldNotReadElfData)?; + core.load(&elf).map_err(ProgramImageError::CouldNotLoad)?; + + // + // On Gimlet Rev B, the BOOT0 pin is unstrapped -- and during a flash, + // it seems to float high enough to bounce the part onto the wrong + // image (that is, the BOOT1 image -- which by default is the ST + // bootloader). This seems to only be true when resetting immediately + // after flashing the part: if there is a delay on the order of ~35 + // milliseconds or more, the BOOT0 pin is seen as low when the part + // resets. Because this delay is (more or less) harmless, we do it on + // all platforms, and further make it tunable. + // + if let Some(d) = reset_delay { + std::thread::sleep(d); + } + + // Reset, using the handoff token if present in the archive + core.reset_with_handoff(hubris, log) + .map_err(ProgramImageError::ResetFailed)?; + + // At this point, we can attempt to program the auxiliary flash. This has + // to happen *after* the image is flashed and the core is reset, because it + // uses hiffy calls to the `auxflash` task to actually do the programming; + // because we have no knowledge of the archive previously flashed onto the + // chip, we couldn't do hiffy calls before flashing. + // + // This is called out in RFD 311 as a weakness of our approach! + let auxflash = hubris + .read_auxflash_data() + .map_err(ProgramImageError::CouldNotReadAuxflashData)? + .map(|auxflash| program_auxflash(hubris, core, &auxflash, log)) + .transpose() + .map_err(ProgramImageError::Auxflash)?; + + Ok(ProgramImageSuccess { auxflash }) +} diff --git a/humility-probes-core/src/lib.rs b/humility-probes-core/src/lib.rs index e837f59d7..b2d837a60 100644 --- a/humility-probes-core/src/lib.rs +++ b/humility-probes-core/src/lib.rs @@ -16,7 +16,7 @@ use anyhow::{Result, anyhow, bail}; mod probe_rs; mod unattached; -pub use probe_rs::ProbeCore; +pub use probe_rs::{LoadError, ProbeCore}; pub use unattached::UnattachedCore; fn parse_probe(probe: &str) -> (&str, Option) { @@ -258,7 +258,7 @@ impl HubrisAttach for humility::hubris::HubrisArchive { log: &Logger, ) -> Result { let mut core = - attach_to_chip(probe, self.chip().as_deref(), None, log)?; + attach_to_chip(probe, self.chip()?.as_deref(), None, log)?; self.validate( &mut core, From 84a140f13b4faaedc5c6b4f0ddeb13b2fe789e36 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 10 Jun 2026 14:59:22 -0400 Subject: [PATCH 4/5] Clean up `HubrisArchive::chip` --- cmd/flash/src/lib.rs | 10 +++------- cmd/reset/src/lib.rs | 18 ++++++++--------- humility-cli/src/lib.rs | 2 +- humility-core/src/hubris.rs | 34 ++++++--------------------------- humility-flash/src/lib.rs | 4 ++-- humility-probes-core/src/lib.rs | 3 +-- 6 files changed, 22 insertions(+), 49 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index a05b80aa9..a24e01e9d 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -77,7 +77,6 @@ fn flash_check( fn flash_program( hubris: &HubrisArchive, core: &mut humility_probes_core::ProbeCore, - config: &HubrisFlashConfig, subargs: &FlashArgs, log: &Logger, ) -> Result<()> { @@ -149,14 +148,11 @@ fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { None => "auto", }; - let Some(chip) = &hubris.chip()? else { - bail!("Archive is very old and missing a chip") - }; - + let chip = hubris.chip()?; info!(log, "attaching with chip set to {chip:x?}"); let core = &mut humility_probes_core::attach_for_flashing( probe, - chip, + &chip, context.cli.speed, log, )?; @@ -164,7 +160,7 @@ fn flashcmd(subargs: FlashArgs, context: &mut ExecutionContext) -> Result<()> { if subargs.check { flash_check(hubris, core, subargs.verbose, log) } else { - flash_program(hubris, core, &config, &subargs, log) + flash_program(hubris, core, &subargs, log) } } diff --git a/cmd/reset/src/lib.rs b/cmd/reset/src/lib.rs index 719b41b7a..58b4b7083 100644 --- a/cmd/reset/src/lib.rs +++ b/cmd/reset/src/lib.rs @@ -48,7 +48,6 @@ fn reset(subargs: ResetArgs, context: &mut ExecutionContext) -> Result<()> { None => { // Detect bogus archives by looking at the chip member if let Some(hubris) = &hubris - && hubris.chip().is_some() && hubris.wants_reset_handoff_token() { Behavior::ResetWithHandoff(hubris) @@ -58,9 +57,7 @@ fn reset(subargs: ResetArgs, context: &mut ExecutionContext) -> Result<()> { } Some(false) => Behavior::Reset, Some(true) => { - if let Some(hubris) = &hubris - && hubris.chip().is_some() - { + if let Some(hubris) = &hubris { if !hubris.wants_reset_handoff_token() { anyhow::bail!( "--use-token=true was specified, but the archive \ @@ -80,11 +77,14 @@ fn reset(subargs: ResetArgs, context: &mut ExecutionContext) -> Result<()> { let r = if subargs.soft_reset || matches!(behavior, Behavior::Halt | Behavior::ResetWithHandoff(..)) { - let chip = hubris.as_ref().and_then(|h| h.chip()).ok_or_else(|| { - anyhow::anyhow!( - "Need a chip to do a soft reset, halt after reset, or handoff" - ) - })?; + let chip = hubris.as_ref().map(|h| h.chip()).transpose()?.ok_or_else( + || { + anyhow::anyhow!( + "Need an archive and chip to do a soft reset, \ + halt after reset, or handoff" + ) + }, + )?; let mut c = humility_probes_core::attach_to_chip( probe, Some(&chip), diff --git a/humility-cli/src/lib.rs b/humility-cli/src/lib.rs index ac90773cc..ee82b4695 100644 --- a/humility-cli/src/lib.rs +++ b/humility-cli/src/lib.rs @@ -219,7 +219,7 @@ impl Cli { hubris: Option<&HubrisArchive>, ) -> Result { let probe = self.probe.as_deref().unwrap_or("auto"); - let chip = hubris.map(|h| h.chip()).transpose()?.flatten(); + let chip = hubris.map(|h| h.chip()).transpose()?; humility_probes_core::attach_to_chip( probe, chip.as_deref(), diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index 437b79c56..913964952 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -1124,15 +1124,6 @@ pub struct HubrisFlashMeta { pub chip: Option, } -// -// Flash information pulled from the archive -// -pub struct HubrisFlashConfig { - pub metadata: HubrisFlashMeta, - pub elf: Vec, - pub chip: Option, -} - #[derive(Copy, Clone, Debug)] struct NamespaceId(usize); @@ -1645,23 +1636,6 @@ impl HubrisArchive { }) } - /// Returns a [`HubrisFlashConfig`] from data in the archive - pub fn load_flash_config(&self) -> Result { - let metadata = self.load_flash_meta()?; - - // This is incredibly ugly! It also gives us backwards compatibility! - let chip: Option = match metadata.chip { - Some(ref chip) => Some(chip.to_string()), - None => bail!("must specify a chip in your config"), - }; - - Ok(HubrisFlashConfig { - metadata, - elf: self.hubris_archive.extract_file("img/final.elf")?, - chip, - }) - } - /// Returns the ELF file data to be programmed pub fn load_flash_elf(&self) -> Result> { let data = self.hubris_archive.extract_file("img/final.elf")?; @@ -1687,8 +1661,12 @@ impl HubrisArchive { } /// Returns the chip name (to be used for flashing) - pub fn chip(&self) -> Result> { - self.load_flash_meta().map(|m| m.chip) + pub fn chip(&self) -> Result { + self.load_flash_meta().and_then(|m| { + m.chip.ok_or_else(|| { + anyhow!("archive is missing a chip in flash metadata") + }) + }) } /// Helper function to load a dump into a [`RawHubrisArchive`] diff --git a/humility-flash/src/lib.rs b/humility-flash/src/lib.rs index 36e174528..8199b434c 100644 --- a/humility-flash/src/lib.rs +++ b/humility-flash/src/lib.rs @@ -280,7 +280,7 @@ pub enum ProgramImageError { /// Could not call [`ProbeCore::load`] #[error("load failed")] - CouldNotLoad(#[source] humility_probes_core::LoadError), + LoadFailed(#[source] humility_probes_core::LoadError), /// Failed to call [`ProbeCore::reset`] #[error("reset failed")] @@ -318,7 +318,7 @@ pub fn program_image( let elf = hubris .load_flash_elf() .map_err(ProgramImageError::CouldNotReadElfData)?; - core.load(&elf).map_err(ProgramImageError::CouldNotLoad)?; + core.load(&elf).map_err(ProgramImageError::LoadFailed)?; // // On Gimlet Rev B, the BOOT0 pin is unstrapped -- and during a flash, diff --git a/humility-probes-core/src/lib.rs b/humility-probes-core/src/lib.rs index b2d837a60..8ff350432 100644 --- a/humility-probes-core/src/lib.rs +++ b/humility-probes-core/src/lib.rs @@ -257,8 +257,7 @@ impl HubrisAttach for humility::hubris::HubrisArchive { probe: &str, log: &Logger, ) -> Result { - let mut core = - attach_to_chip(probe, self.chip()?.as_deref(), None, log)?; + let mut core = attach_to_chip(probe, Some(&self.chip()?), None, log)?; self.validate( &mut core, From db23c596984fc517a9da591d96b8792a15bb6f55 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 10 Jun 2026 15:53:35 -0400 Subject: [PATCH 5/5] Copilot suggestions --- cmd/flash/src/lib.rs | 7 ++++--- cmd/reset/src/lib.rs | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/flash/src/lib.rs b/cmd/flash/src/lib.rs index a24e01e9d..2d2d040a3 100644 --- a/cmd/flash/src/lib.rs +++ b/cmd/flash/src/lib.rs @@ -66,9 +66,10 @@ fn flash_check( let check_type = humility_flash::ImageCheckType::ImageIdAndFlash { check_every_byte: verbose, }; - let out = match get_image_state(hubris, core, check_type, log)? { - humility_flash::ImageStateResult::Matches => Ok(()), - humility_flash::ImageStateResult::DoesNotMatch(e) => Err(e.into()), + let out = match get_image_state(hubris, core, check_type, log) { + Ok(humility_flash::ImageStateResult::Matches) => Ok(()), + Ok(humility_flash::ImageStateResult::DoesNotMatch(e)) => Err(e.into()), + Err(e) => Err(e.into()), }; core.run()?; out diff --git a/cmd/reset/src/lib.rs b/cmd/reset/src/lib.rs index 58b4b7083..9db9d5342 100644 --- a/cmd/reset/src/lib.rs +++ b/cmd/reset/src/lib.rs @@ -46,7 +46,6 @@ fn reset(subargs: ResetArgs, context: &mut ExecutionContext) -> Result<()> { } else { match subargs.use_token { None => { - // Detect bogus archives by looking at the chip member if let Some(hubris) = &hubris && hubris.wants_reset_handoff_token() {