diff --git a/crates/fbuild-daemon/Cargo.toml b/crates/fbuild-daemon/Cargo.toml index 11524394..1c039caa 100644 --- a/crates/fbuild-daemon/Cargo.toml +++ b/crates/fbuild-daemon/Cargo.toml @@ -15,12 +15,10 @@ name = "fbuild_daemon" path = "src/lib.rs" [features] -# Issue #66 spike. Forwards to `fbuild-deploy/espflash-native` so -# `FBUILD_USE_ESPFLASH_VERIFY=1` / `FBUILD_USE_ESPFLASH_WRITE=1` can -# actually route through espflash at runtime. Default-off: the daemon -# binary we ship keeps the conservative esptool-subprocess path unless -# the CI/release pipeline flips this on explicitly. -default = [] +# Compile native espflash-backed ESP32 verify/write support into the daemon +# by default. Runtime fallback still routes through esptool automatically if +# the native path fails on a board/host combination. +default = ["espflash-native"] espflash-native = ["fbuild-deploy/espflash-native"] [dependencies] @@ -58,7 +56,7 @@ tempfile = { workspace = true } socket2 = "0.6" # libc::kill(pid, 0) is used as a probe to detect dead daemon PIDs at -# startup (stale-PID cleanup). Unix-only — Windows uses a manual +# startup (stale-PID cleanup). Unix-only; Windows uses a manual # OpenProcess FFI in main.rs. [target.'cfg(unix)'.dependencies] libc = "0.2" diff --git a/crates/fbuild-daemon/src/handlers/operations.rs b/crates/fbuild-daemon/src/handlers/operations.rs index deb2adcb..d7f45ecf 100644 --- a/crates/fbuild-daemon/src/handlers/operations.rs +++ b/crates/fbuild-daemon/src/handlers/operations.rs @@ -18,20 +18,13 @@ use std::sync::Arc; /// pre-checks through the native [`espflash`] crate (issue #66) instead /// of the Python `esptool` subprocess. /// -/// Controlled by the `FBUILD_USE_ESPFLASH_VERIFY` environment variable -/// (set to `1`, `true`, `yes`, or `on` to enable — case-insensitive). -/// Any other value — including unset — keeps the default esptool path, -/// so users on unusual setups retain the existing escape hatch until -/// the native path has bench time on every ESP32 family member. +/// Controlled by the `FBUILD_USE_ESPFLASH_VERIFY` environment variable. +/// Native verify is enabled by default when compiled in. +/// Set this variable to `0`, `false`, `no`, or `off` (case-insensitive) +/// to force esptool. #[cfg(feature = "espflash-native")] pub(crate) fn native_verify_enabled() -> bool { - match std::env::var("FBUILD_USE_ESPFLASH_VERIFY") { - Ok(v) => matches!( - v.trim().to_ascii_lowercase().as_str(), - "1" | "true" | "yes" | "on" - ), - Err(_) => false, - } + env_default_enabled("FBUILD_USE_ESPFLASH_VERIFY") } /// Returns `true` when the daemon should trust an in-memory firmware @@ -137,20 +130,23 @@ pub(crate) fn compute_esp32_image_hash( /// through the native [`espflash`] crate (issue #66) instead of the /// Python `esptool` subprocess. /// -/// Controlled by the `FBUILD_USE_ESPFLASH_WRITE` environment variable -/// (set to `1`, `true`, `yes`, or `on` to enable — case-insensitive). -/// Independent of `FBUILD_USE_ESPFLASH_VERIFY` — either toggle can be -/// flipped without the other. Default off so esptool remains the safe -/// fallback while the native write path accumulates bench time across -/// every ESP32 family member. +/// Controlled by the `FBUILD_USE_ESPFLASH_WRITE` environment variable. +/// Native write is enabled by default when compiled in. Independent of +/// `FBUILD_USE_ESPFLASH_VERIFY`. Set it to `0`, `false`, `no`, or `off` +/// (case-insensitive) to force esptool. #[cfg(feature = "espflash-native")] pub(crate) fn native_write_enabled() -> bool { - match std::env::var("FBUILD_USE_ESPFLASH_WRITE") { - Ok(v) => matches!( + env_default_enabled("FBUILD_USE_ESPFLASH_WRITE") +} + +#[cfg(feature = "espflash-native")] +fn env_default_enabled(name: &str) -> bool { + match std::env::var(name) { + Ok(v) => !matches!( v.trim().to_ascii_lowercase().as_str(), - "1" | "true" | "yes" | "on" + "0" | "false" | "no" | "off" ), - Err(_) => false, + Err(_) => true, } } @@ -1260,14 +1256,11 @@ pub async fn deploy( } else { deployer }; - // Issue #66: opt-in native `verify-flash` + `write-flash` - // via the `espflash` crate. Only compiled in when the - // daemon is built with `--features espflash-native`; - // default builds keep the esptool-subprocess path and - // skip pulling espflash + its ~30 transitive deps. At - // runtime, further gated by the `FBUILD_USE_ESPFLASH_*` - // env vars so operators can still fall back to esptool - // on an espflash-native build. + // Issue #66: native `verify-flash` + `write-flash` via + // the `espflash` crate. This is compiled in by default; + // `FBUILD_USE_ESPFLASH_*` are opt-out switches, and the + // deployer falls back to esptool automatically when a + // native operation fails. #[cfg(feature = "espflash-native")] let deployer = deployer .with_native_verify(native_verify_enabled()) @@ -2243,6 +2236,47 @@ mod deploy_message_tests { } } +#[cfg(all(test, feature = "espflash-native"))] +mod espflash_env_tests { + use super::{native_verify_enabled, native_write_enabled}; + + // This lock only serializes these unit tests while they mutate the + // process environment. Production callers and any other tests reading + // the same env vars remain unsynchronized. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + #[test] + fn native_verify_defaults_on_and_allows_opt_out() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + std::env::remove_var("FBUILD_USE_ESPFLASH_VERIFY"); + assert!(native_verify_enabled()); + + std::env::set_var("FBUILD_USE_ESPFLASH_VERIFY", "0"); + assert!(!native_verify_enabled()); + + std::env::set_var("FBUILD_USE_ESPFLASH_VERIFY", "false"); + assert!(!native_verify_enabled()); + + std::env::set_var("FBUILD_USE_ESPFLASH_VERIFY", "1"); + assert!(native_verify_enabled()); + std::env::remove_var("FBUILD_USE_ESPFLASH_VERIFY"); + } + + #[test] + fn native_write_defaults_on_and_allows_opt_out() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + std::env::remove_var("FBUILD_USE_ESPFLASH_WRITE"); + assert!(native_write_enabled()); + + std::env::set_var("FBUILD_USE_ESPFLASH_WRITE", "off"); + assert!(!native_write_enabled()); + + std::env::set_var("FBUILD_USE_ESPFLASH_WRITE", "yes"); + assert!(native_write_enabled()); + std::env::remove_var("FBUILD_USE_ESPFLASH_WRITE"); + } +} + #[cfg(test)] mod image_hash_memo_tests { //! Memo-cache correctness for [`compute_esp32_image_hash`]: the diff --git a/crates/fbuild-deploy/Cargo.toml b/crates/fbuild-deploy/Cargo.toml index eb7406a5..34852071 100644 --- a/crates/fbuild-deploy/Cargo.toml +++ b/crates/fbuild-deploy/Cargo.toml @@ -7,13 +7,10 @@ rust-version.workspace = true license.workspace = true [features] -# Default is the conservative esptool-subprocess path. The native -# espflash-backed verify/write paths compile in only when the caller -# explicitly opts in via this feature — keeps the default dep graph slim -# (no espflash/strum/deku/miette/... pulled in for consumers that don't -# need it) and gives issue #66's spike a clean kill-switch while it -# accumulates bench time on every ESP32 family member. -default = [] +# Compile the native espflash-backed verify/write paths by default. The +# ESP32 deployer keeps the esptool subprocess path as an automatic runtime +# fallback when native connect/verify/write fails. +default = ["espflash-native"] espflash-native = ["dep:espflash", "dep:md-5"] [dependencies] @@ -31,15 +28,12 @@ tracing = { workspace = true } async-trait = { workspace = true } object = { workspace = true } # Native ESP32 flasher protocol (issue #66). The `serialport` feature -# gates the `Flasher` type (the real transport entry point) so we enable -# it; default features stay off to keep the CLI/TUI surface out of our -# dependency graph. Optional so default builds don't pull espflash + -# its ~30 transitive deps (strum, deku, miette, ...); enabled via the -# `espflash-native` feature. +# gates the `Flasher` type (the real transport entry point), so it is +# enabled while espflash's CLI/TUI defaults stay off. espflash = { version = "4", default-features = false, features = ["serialport"], optional = true } # Local MD5 of firmware regions for comparison against espflash's on-chip # `FLASH_MD5SUM` command. Optional and gated behind `espflash-native` -# since it's only used by the native verify path. +# since it is only used by the native verify path. md-5 = { version = "0.10", optional = true } [dev-dependencies] diff --git a/crates/fbuild-deploy/README.md b/crates/fbuild-deploy/README.md index 497ae987..d370fd2f 100644 --- a/crates/fbuild-deploy/README.md +++ b/crates/fbuild-deploy/README.md @@ -1,29 +1,29 @@ # fbuild-deploy -Firmware deployment to embedded devices via platform-specific upload tools (esptool, avrdude, teensy_loader_cli), and device reset sequences. +Firmware deployment to embedded devices via platform-specific upload tools (espflash/esptool, avrdude, teensy_loader_cli), and device reset sequences. ## Key Types - `Deployer` -- trait for platform-specific firmware upload (`deploy` method) - `DeploymentResult` -- success/failure with message and optional port -- `Esp32Deployer` -- esptool-based deployer with chip-specific flash offsets and modes +- `Esp32Deployer` -- ESP32 deployer with native espflash fast path and esptool fallback - `AvrDeployer` -- avrdude-based deployer for Arduino boards - `TeensyDeployer` -- teensy_loader_cli-based deployer via USB HID ## Modules - **esp32** -- `Esp32Deployer`, `EsptoolParams`; handles bootloader/partitions/firmware offsets per MCU -- **esp32_native** -- native espflash-backed `verify-flash` and `write-flash` (issue #66); opt-in via `FBUILD_USE_ESPFLASH_VERIFY=1` / `FBUILD_USE_ESPFLASH_WRITE=1` (independent toggles), falls back to esptool subprocess by default +- **esp32_native** -- native espflash-backed `verify-flash` and `write-flash` (issue #66); enabled by default when compiled in, with automatic esptool subprocess fallback - **avr** -- `AvrDeployer`, `AvrdudeParams`; flashes firmware.hex via serial - **teensy** -- `TeensyDeployer`, `TeensyLoaderParams`; flashes firmware.hex via USB - **reset** -- `reset_device`, `detect_platform_for_reset`; DTR/RTS toggle sequences per platform Skip-redeploy is handled authoritatively by the daemon's device-side `verify-flash` pre-check (see `handlers/operations.rs`), which asks the ESP32 stub flasher for a per-region MD5 via `FLASH_MD5SUM` before writing. The previous client-side `FirmwareLedger` was removed (issue #18) because it could not detect flashes performed outside fbuild. -### Native verify-flash and write-flash (issue #66, opt-in) +### Native verify-flash and write-flash (issue #66) -Setting `FBUILD_USE_ESPFLASH_VERIFY=1` routes the ESP32 verify pre-check through the native [`espflash`](https://crates.io/crates/espflash) crate instead of the Python `esptool` subprocess, avoiding ~1 s of interpreter startup and ~0.5 s of subprocess spawn per invocation. Setting `FBUILD_USE_ESPFLASH_WRITE=1` routes `write-flash` through the same in-process path — both the full deploy and the selective post-verify-mismatch rewrite. The two toggles are independent, so you can opt into native verify without native write (or vice versa). The daemon pre-empts any active serial monitor via `SharedSerialManager::preempt_for_deploy` before opening the port, so the native path never contends with the existing lease. +The ESP32 verify pre-check uses the native [`espflash`](https://crates.io/crates/espflash) crate by default instead of the Python `esptool` subprocess, avoiding ~1 s of interpreter startup and ~0.5 s of subprocess spawn per invocation. `write-flash` uses the same in-process path for both full deploys and selective post-verify-mismatch rewrites. If native verify/write fails, the deployer logs a warning and retries the same operation through esptool. Set `FBUILD_USE_ESPFLASH_VERIFY=0` and/or `FBUILD_USE_ESPFLASH_WRITE=0` to force esptool for either phase. -Progress from espflash's `ProgressCallbacks` is bridged into `tracing` and throttled to roughly one log line per 10% of each region, which the daemon's existing log broadcaster surfaces. Structured WebSocket progress frames on the deploy channel are a follow-up. +The daemon pre-empts any active serial monitor via `SharedSerialManager::preempt_for_deploy` before opening the port, so neither path contends with the existing lease. Progress from espflash's `ProgressCallbacks` is bridged into `tracing` and throttled to roughly one log line per 10% of each region, which the daemon's existing log broadcaster surfaces. Structured WebSocket progress frames on the deploy channel are a follow-up. See `docs/architecture/deploy-preemption.md` for architecture details. diff --git a/crates/fbuild-deploy/src/esp32.rs b/crates/fbuild-deploy/src/esp32.rs index 66281496..30d0a66f 100644 --- a/crates/fbuild-deploy/src/esp32.rs +++ b/crates/fbuild-deploy/src/esp32.rs @@ -50,28 +50,53 @@ pub struct Esp32Deployer { /// instead of the Python `esptool` subprocess. /// /// The daemon sets this from the `FBUILD_USE_ESPFLASH_VERIFY` env - /// var. Default `false` keeps esptool as the fallback so users on - /// unusual setups aren't forced onto the native path until it has - /// bench time on every ESP32 family member. + /// var. When enabled, the deployer still falls back to esptool if + /// the native path fails on a given board or host. /// /// Only compiled in when the `espflash-native` cargo feature is - /// enabled (issue #66 spike). Without the feature the struct layout - /// doesn't even carry the flag, so default builds pay zero cost for - /// the native path. + /// enabled. #[cfg(feature = "espflash-native")] use_native_verify: bool, /// Route `write-flash` through the native [`espflash`] crate /// instead of the Python `esptool` subprocess (issue #66). /// /// The daemon sets this from the `FBUILD_USE_ESPFLASH_WRITE` env - /// var, independently of `use_native_verify`. Default `false` keeps - /// esptool as the safe fallback. + /// var, independently of `use_native_verify`. When enabled, the + /// deployer still falls back to esptool if the native path fails. /// /// Feature-gated — see `use_native_verify`. #[cfg(feature = "espflash-native")] use_native_write: bool, } +#[cfg(feature = "espflash-native")] +fn native_write_or_fallback(port: &str, label: &str, native: F) -> Option +where + F: FnOnce() -> Result, +{ + match native() { + Ok(result) if result.success => Some(result), + Ok(result) => { + tracing::warn!( + port, + "native {} failed ({}); falling back to esptool", + label, + result.message + ); + None + } + Err(e) => { + tracing::warn!( + port, + "native {} failed ({}); falling back to esptool", + label, + e + ); + None + } + } +} + impl Esp32Deployer { #[allow(clippy::too_many_arguments)] pub fn new( @@ -114,10 +139,11 @@ impl Esp32Deployer { } /// Opt this deployer into the native espflash-based write-flash - /// path (issue #66). Independent of `with_native_verify`. On opt-in - /// both [`Deployer::deploy`] and [`Esp32Deployer::deploy_regions`] - /// route through the in-process espflash `Flasher`, skipping the - /// ~1.5 s Python/esptool startup per flash. + /// path (issue #66). Independent of `with_native_verify`. When + /// enabled, both [`Deployer::deploy`] and + /// [`Esp32Deployer::deploy_regions`] route through the in-process + /// espflash `Flasher`, skipping the ~1.5 s Python/esptool startup + /// per flash unless they need to fall back. /// /// Only present when the `espflash-native` cargo feature is enabled. #[cfg(feature = "espflash-native")] @@ -235,8 +261,26 @@ impl Esp32Deployer { pub fn try_verify_deployment(&self, firmware_path: &Path, port: &str) -> Result { #[cfg(feature = "espflash-native")] if self.use_native_verify { - return self.try_verify_deployment_native(firmware_path, port); + match self.try_verify_deployment_native(firmware_path, port) { + Ok(outcome) => return Ok(outcome), + Err(e) => { + tracing::warn!( + port, + "native verify-flash failed ({}); falling back to esptool", + e + ); + } + } } + + self.try_verify_deployment_esptool(firmware_path, port) + } + + fn try_verify_deployment_esptool( + &self, + firmware_path: &Path, + port: &str, + ) -> Result { let args = self.build_verify_flash_args(firmware_path, port); let args_ref: Vec<&str> = args.iter().map(|s| s.as_str()).collect(); @@ -1126,7 +1170,11 @@ impl Esp32Deployer { #[cfg(feature = "espflash-native")] if self.use_native_write { - return self.try_deploy_regions_native(firmware_path, port, regions); + if let Some(result) = native_write_or_fallback(port, "selective write-flash", || { + self.try_deploy_regions_native(firmware_path, port, regions) + }) { + return Ok(result); + } } let args = self.build_write_flash_args(firmware_path, port, Some(regions)); @@ -1197,7 +1245,13 @@ impl Deployer for Esp32Deployer { #[cfg(feature = "espflash-native")] if self.use_native_write { - return self.try_deploy_native(firmware_path, port); + if let Some(result) = + native_write_or_fallback(port, "write-flash", || { + self.try_deploy_native(firmware_path, port) + }) + { + return Ok(result); + } } let args = self.build_write_flash_args(firmware_path, port, None);