esp32: default to native espflash with esptool fallback (#197)

* esp32: default to native espflash with esptool fallback

* esp32: address CodeRabbit review feedback

Author: zackees

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"

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

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]

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.

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<F>(port: &str, label: &str, native: F) -> Option<DeploymentResult>
+where
+    F: FnOnce() -> Result<DeploymentResult>,
+{
+    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<VerifyOutcome> {
         #[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<VerifyOutcome> {
         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);