From b903fd105ee9353b6fe2b407a7320ae9a20c8cb4 Mon Sep 17 00:00:00 2001 From: zackees Date: Wed, 5 Aug 2026 21:48:36 -0700 Subject: [PATCH] fix(rp): bind picotool deploy to selected device --- Cargo.lock | 42 ++-- Cargo.toml | 2 +- agents/docs/deploy-architecture.md | 30 +-- crates/fbuild-core/src/usb/profiles.rs | 23 ++ crates/fbuild-deploy/src/rp2040.rs | 249 ++++++++++++++++--- crates/fbuild-deploy/src/rp2040_picotool.rs | 106 ++++++-- crates/fbuild-deploy/src/rp2040_preflight.rs | 94 +++++-- pyproject.toml | 2 +- 8 files changed, 440 insertions(+), 108 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5d5297a82..4f63b5cfd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -951,7 +951,7 @@ checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" [[package]] name = "fbuild-bench-fastled-examples" -version = "2.5.13" +version = "2.5.14" dependencies = [ "fbuild-core", "fbuild-library-select", @@ -965,7 +965,7 @@ dependencies = [ [[package]] name = "fbuild-build" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "blake3", @@ -1002,7 +1002,7 @@ dependencies = [ [[package]] name = "fbuild-build-arm" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "blake3", @@ -1036,7 +1036,7 @@ dependencies = [ [[package]] name = "fbuild-build-engine" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "blake3", @@ -1069,7 +1069,7 @@ dependencies = [ [[package]] name = "fbuild-build-esp" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "blake3", @@ -1103,7 +1103,7 @@ dependencies = [ [[package]] name = "fbuild-build-mcu" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "blake3", @@ -1137,7 +1137,7 @@ dependencies = [ [[package]] name = "fbuild-cli" -version = "2.5.13" +version = "2.5.14" dependencies = [ "blake3", "clap", @@ -1170,7 +1170,7 @@ dependencies = [ [[package]] name = "fbuild-config" -version = "2.5.13" +version = "2.5.14" dependencies = [ "fbuild-core", "fbuild-paths", @@ -1185,7 +1185,7 @@ dependencies = [ [[package]] name = "fbuild-core" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "fs2", @@ -1206,7 +1206,7 @@ dependencies = [ [[package]] name = "fbuild-daemon" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "axum", @@ -1246,7 +1246,7 @@ dependencies = [ [[package]] name = "fbuild-deploy" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "espflash", @@ -1275,7 +1275,7 @@ dependencies = [ [[package]] name = "fbuild-header-scan" -version = "2.5.13" +version = "2.5.14" dependencies = [ "criterion", "fbuild-paths", @@ -1286,7 +1286,7 @@ dependencies = [ [[package]] name = "fbuild-library" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "axum", @@ -1316,7 +1316,7 @@ dependencies = [ [[package]] name = "fbuild-library-select" -version = "2.5.13" +version = "2.5.14" dependencies = [ "bincode", "blake3", @@ -1336,7 +1336,7 @@ dependencies = [ [[package]] name = "fbuild-packages" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "axum", @@ -1368,7 +1368,7 @@ dependencies = [ [[package]] name = "fbuild-packages-fetch" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "axum", @@ -1397,7 +1397,7 @@ dependencies = [ [[package]] name = "fbuild-paths" -version = "2.5.13" +version = "2.5.14" dependencies = [ "fbuild-core", "serde", @@ -1408,7 +1408,7 @@ dependencies = [ [[package]] name = "fbuild-python" -version = "2.5.13" +version = "2.5.14" dependencies = [ "base64", "fbuild-core", @@ -1430,7 +1430,7 @@ dependencies = [ [[package]] name = "fbuild-serial" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "base64", @@ -1454,7 +1454,7 @@ dependencies = [ [[package]] name = "fbuild-test-support" -version = "2.5.13" +version = "2.5.14" dependencies = [ "fbuild-config", "fbuild-core", @@ -1474,7 +1474,7 @@ dependencies = [ [[package]] name = "fbuild-toolchain" -version = "2.5.13" +version = "2.5.14" dependencies = [ "async-trait", "axum", diff --git a/Cargo.toml b/Cargo.toml index 0116f00b2..b5377a717 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,7 +69,7 @@ exclude = [ libraries = [{ path = "dylints/*" }] [workspace.package] -version = "2.5.13" +version = "2.5.14" edition = "2021" rust-version = "1.94.1" license = "MIT OR Apache-2.0" diff --git a/agents/docs/deploy-architecture.md b/agents/docs/deploy-architecture.md index bdd29b6df..90417583b 100644 --- a/agents/docs/deploy-architecture.md +++ b/agents/docs/deploy-architecture.md @@ -93,19 +93,21 @@ The follow-up issues track the full polymorphic version: FastLED/fbuild#1162) selects which stock transport is tried first: - **`picotool` (default).** After the unchanged pre-touch/1200-bps-touch/ - UF2-preparation steps and a best-effort bounded BOOTSEL volume wait - (mounting is not required — failure to mount only affects the - mass-storage fallback), fbuild runs a Windows-only PICOBOOT driver - preflight (`crates/fbuild-deploy/src/rp2040_preflight.rs`, - FastLED/fbuild#1163: classifies `present_usb_problem_devices()` for a - `VID_2E8A&PID_0003` composite-interface devnode stuck at a - `CM_PROB_FAILED_INSTALL`-family Config Manager code), then a bounded - `picotool info` probe, then `picotool load -f -x`. A driver-missing - preflight result skips picotool outright (no probe/load timeout burned) - and names the exact devnode + WinUSB (Zadig) guidance. Any picotool - failure — preflight skip, probe failure, or load failure — falls back to - the BOOTSEL mass-storage path below; if that also fails, the combined - error names both transports' failures. + UF2-preparation steps and a best-effort bounded BOOTSEL volume wait, + fbuild derives one exact BOOTSEL VID:PID from the verified FastLED/boards + profile for the selected RP family and binds each picotool operation to + that identity plus the selected runtime USB serial: + `--vid 0x --pid 0x --ser `. It then + runs a Windows-only PICOBOOT driver preflight for that same composite + interface, a bounded `picotool info` probe, and `picotool load -x`. + It intentionally does **not** use `-f`: a failed 1200-bps transition must + not let picotool reset some other compatible RP board. A missing runtime + serial or ambiguous/missing registry BOOTSEL identity disables picotool; + fbuild uses only an explicitly identified BOOTSEL mass-storage volume. A + Windows driver problem, including Code 43, also skips picotool rather than + spending its timeout. Any remaining picotool failure falls back to the + BOOTSEL mass-storage path; if that also fails, the combined error names + both transports' failures. - **`uf2`.** Preserves the historical order exactly: BOOTSEL mass-storage first (with bounded transfer retries across fresh enumerations), managed picotool as the fallback. @@ -118,7 +120,7 @@ else logs a warning and keeps the default: | `FBUILD_RP2040_BOOTLOADER_TIMEOUT_SECS` | 10 s | BOOTSEL volume discovery after the 1200-bps touch (and re-discovery between transfer retries) | | `FBUILD_RP2040_UF2_WRITE_TIMEOUT_SECS` | 60 s | Per-attempt watchdog on the `NEW.UF2` write; a timed-out write feeds the normal retry/picotool-fallback path | | `FBUILD_RP2040_POST_DEPLOY_TIMEOUT_SECS` | 15 s | Eject watch after the write, and the runtime-CDC reappearance wait | -| `FBUILD_RP2040_PICOTOOL_TIMEOUT_SECS` | 60 s | `picotool load -f -x` budget, both as the picotool-primary load and the picotool-fallback load | +| `FBUILD_RP2040_PICOTOOL_TIMEOUT_SECS` | 60 s | Target-bound `picotool load -x` budget, both as the picotool-primary load and the picotool-fallback load | Outcome note: once the eject watch (mass-storage path) or a successful picotool load (primary or fallback) has confirmed the ROM accepted the diff --git a/crates/fbuild-core/src/usb/profiles.rs b/crates/fbuild-core/src/usb/profiles.rs index d4f05605a..fbf16b8c4 100644 --- a/crates/fbuild-core/src/usb/profiles.rs +++ b/crates/fbuild-core/src/usb/profiles.rs @@ -135,6 +135,26 @@ pub fn profiles_for(vid: u16, pid: u16) -> Vec { profiles } +/// Return every verified USB transport profile, ordered by descending +/// publisher priority. Callers that need to correlate a runtime endpoint with +/// a bootloader endpoint must derive both identities from this registry rather +/// than carrying a built-in VID/PID table. +pub fn all_profiles() -> Vec { + let Ok(guard) = INSTALLED.read() else { + return Vec::new(); + }; + let Some(installed) = guard.as_ref() else { + return Vec::new(); + }; + let mut profiles: Vec<_> = installed + .identities + .iter() + .map(|entry| entry.profile.clone()) + .collect(); + profiles.sort_by(|left, right| right.priority.cmp(&left.priority)); + profiles +} + fn identity_pid_matches(entry: &IndexedProfile, candidate: u16) -> bool { let Some(expected) = entry.pid else { return true; @@ -540,6 +560,9 @@ mod tests { assert_eq!(profiles.len(), 1); assert_eq!(profiles[0].role, UsbDeviceRole::RuntimeCdc); assert_eq!(profiles[0].family.as_deref(), Some("synthetic-family")); + let installed = all_profiles(); + assert_eq!(installed.len(), 1); + assert_eq!(installed[0].identity_match.vid, "feed"); let board = board_profile("synthetic-alias").unwrap(); assert_eq!(board.board_id, "synthetic-board"); assert_eq!(board.primary_compile_identity, Some((0xfeed, 0xc0de))); diff --git a/crates/fbuild-deploy/src/rp2040.rs b/crates/fbuild-deploy/src/rp2040.rs index dada37327..b578a90d8 100644 --- a/crates/fbuild-deploy/src/rp2040.rs +++ b/crates/fbuild-deploy/src/rp2040.rs @@ -64,7 +64,7 @@ const UF2_WRITE_TIMEOUT_ENV: &str = "FBUILD_RP2040_UF2_WRITE_TIMEOUT_SECS"; const DEFAULT_UF2_WRITE_TIMEOUT: Duration = Duration::from_secs(60); const CDC_POLL_INTERVAL: Duration = Duration::from_millis(100); const WATCHDOG_CANCELLATION_GRACE: Duration = Duration::from_millis(250); -/// Timeout for `picotool load -f -x`, used both as the picotool-primary +/// Timeout for target-bound `picotool load -x`, used both as the picotool-primary /// load budget (FastLED/fbuild#1162) and the historical picotool-fallback /// budget (previously a hardcoded 30s in `rp2040_picotool.rs`). const PICOTOOL_LOAD_TIMEOUT_ENV: &str = "FBUILD_RP2040_PICOTOOL_TIMEOUT_SECS"; @@ -108,13 +108,88 @@ fn should_attempt_picotool_first( ) -> bool { match transport { Rp2040Transport::Uf2 => false, - Rp2040Transport::Picotool => !matches!( - preflight, - Some(preflight::PicobootPreflight::DriverMissing { .. }) - ), + Rp2040Transport::Picotool => { + matches!(preflight, None | Some(preflight::PicobootPreflight::Ready)) + } + } +} + +fn rp_family_name(family_id: u32) -> &'static str { + if family_id == RP2350_FAMILY_ID { + "rp2350" + } else { + "rp2040" } } +fn bootloader_profile_matches_family( + profile: &fbuild_core::usb::profiles::UsbTransportProfile, + family_id: u32, +) -> bool { + use fbuild_core::usb::profiles::{UsbDeviceRole, UsbPurpose}; + + profile.purpose == UsbPurpose::Bootloader + && profile.role == UsbDeviceRole::BootloaderUf2 + && profile.family.as_deref() == Some(rp_family_name(family_id)) + && profile.identity_match.pid.is_some() +} + +fn picotool_target_for_family( + serial_number: &str, + family_id: u32, +) -> Result { + let profiles = fbuild_core::usb::profiles::all_profiles(); + picotool_target_from_profiles(serial_number, family_id, &profiles) +} + +fn picotool_target_from_profiles( + serial_number: &str, + family_id: u32, + profiles: &[fbuild_core::usb::profiles::UsbTransportProfile], +) -> Result { + let mut candidates: BTreeSet<_> = profiles + .iter() + .filter(|profile| bootloader_profile_matches_family(profile, family_id)) + .filter_map(|profile| { + profile + .identity_match + .pid + .as_ref() + .map(|pid| (profile.identity_match.vid.clone(), pid.clone())) + }) + .collect(); + match candidates.len() { + 0 => Err(FbuildError::DeployFailed(format!( + "FastLED/boards USB profiles do not define an exact {} BOOTSEL identity; fbuild refuses an unscoped picotool command", + rp_family_name(family_id) + ))), + 1 => { + let (vendor_id, product_id) = candidates.pop_first().ok_or_else(|| { + FbuildError::DeployFailed( + "FastLED/boards USB profile identity disappeared while resolving PICOBOOT" + .to_string(), + ) + })?; + Ok(picotool::PicotoolTarget::new( + serial_number, + &vendor_id, + &product_id, + )) + } + _ => Err(FbuildError::DeployFailed(format!( + "FastLED/boards USB profiles define multiple {} BOOTSEL identities; fbuild refuses to guess which one picotool should use", + rp_family_name(family_id) + ))), + } +} + +fn picotool_identity_required_error() -> FbuildError { + FbuildError::DeployFailed( + "RP-series picotool deployment requires the selected runtime USB serial so fbuild can bind the BOOTSEL operation to one board; use a healthy USB CDC port selector or an explicit UF2=BOOTSEL-volume recovery path" + .to_string(), + ) +} + /// Parse an env-supplied stage timeout. Accepts integer seconds in 1..=600; /// an unset variable is silently the default, anything else warns and falls /// back to the default. @@ -1489,8 +1564,8 @@ fn ensure_verified_usb_profiles() -> Result<()> { } } -/// Best-effort Windows PICOBOOT preflight + bounded `picotool info` probe + -/// `picotool load -f -x`, all folded into a single string error so the +/// Best-effort Windows PICOBOOT preflight + bounded target-bound `picotool +/// info` probe + `picotool load -x`, all folded into a single string error so the /// caller can compose the final combined-transport message if the /// mass-storage fallback also fails (FastLED/fbuild#1162/#1163). Off /// Windows, `present_usb_problem_devices` is a no-op empty vec, so preflight @@ -1498,6 +1573,7 @@ fn ensure_verified_usb_profiles() -> Result<()> { async fn attempt_picotool_primary( project_dir: &Path, artifact: &Path, + target: &picotool::PicotoolTarget, load_timeout: Duration, ) -> std::result::Result { let preflight_result = if cfg!(windows) { @@ -1505,45 +1581,27 @@ async fn attempt_picotool_primary( tokio::task::spawn_blocking(fbuild_serial::ports::present_usb_problem_devices) .await .unwrap_or_default(); - Some(preflight::classify_picoboot_preflight(&devices)) + Some(preflight::classify_picoboot_preflight(&devices, target)) } else { None }; if !should_attempt_picotool_first(Rp2040Transport::Picotool, preflight_result.as_ref()) { - let Some(preflight::PicobootPreflight::DriverMissing { - instance_id, - problem_code, - }) = preflight_result - else { - unreachable!("should_attempt_picotool_first only skips picotool on DriverMissing"); - }; - let message = preflight::driver_missing_message(&instance_id, problem_code); - tracing::warn!( - instance_id = %instance_id, - problem_code, - "{message}" - ); + let preflight = preflight_result + .as_ref() + .expect("picotool preflight can only block when a problem was found"); + let message = preflight::problem_message(preflight); + tracing::warn!("{message}"); return Err(message); } - if let Some(preflight::PicobootPreflight::OtherProblem { - instance_id, - problem_code, - }) = &preflight_result - { - tracing::warn!( - instance_id = %instance_id, - problem_code, - "RP2040 PICOBOOT devnode reports a Windows Config Manager problem; attempting picotool anyway" - ); - } if let Err(probe_error) = - picotool::probe_picotool_info(project_dir, PICOTOOL_INFO_PROBE_TIMEOUT).await + picotool::probe_picotool_info(project_dir, target, PICOTOOL_INFO_PROBE_TIMEOUT).await { return Err(format!("picotool info probe failed: {probe_error}")); } picotool::load_with_managed_picotool( project_dir, artifact, + target, None, load_timeout, picotool::PicotoolMode::Primary, @@ -1644,6 +1702,11 @@ impl Deployer for Rp2040Deployer { } else { None }; + let picotool_target = runtime_target + .as_ref() + .and_then(|target| target.serial_number.as_deref()) + .map(|serial| picotool_target_for_family(serial, self.family_id)) + .transpose()?; // Capture topology before the 1200-bps touch: once the board resets // into BOOTSEL the runtime CDC devnode this looks up disappears. // A join failure on this purely-diagnostic task must never fail the @@ -1695,6 +1758,20 @@ impl Deployer for Rp2040Deployer { "RP2040 UF2 preparation task failed: {error}" )) })??; + // A PICOBOOT command without a serial-number selector may choose any + // attached RP board. If serial identity is unavailable, retain the + // safe BOOTSEL mass-storage path but never issue an unscoped picotool + // probe or force-reset command. + let transport = if picotool_target.is_none() { + if self.transport == Rp2040Transport::Picotool { + tracing::warn!( + "RP-series runtime USB serial unavailable; skipping picotool-primary and using BOOTSEL mass-storage only" + ); + } + Rp2040Transport::Uf2 + } else { + self.transport + }; let ( transfer_stdout, transfer_stderr, @@ -1702,7 +1779,7 @@ impl Deployer for Rp2040Deployer { transfer_volume, picotool_confirmed, prior_transport_failure, - ) = match self.transport { + ) = match transport { Rp2040Transport::Uf2 => { if let Some(volume) = volume { let transfer = run_mass_storage_transfer( @@ -1730,9 +1807,13 @@ impl Deployer for Rp2040Deployer { ), topology.as_deref(), ); + let target = picotool_target + .as_ref() + .ok_or_else(picotool_identity_required_error)?; let loaded = picotool::load_with_managed_picotool( project_dir, &artifact, + target, Some(&context), self.picotool_timeout, picotool::PicotoolMode::Fallback, @@ -1755,9 +1836,13 @@ impl Deployer for Rp2040Deployer { .to_string(), topology.as_deref(), ); + let target = picotool_target + .as_ref() + .ok_or_else(picotool_identity_required_error)?; let loaded = picotool::load_with_managed_picotool( project_dir, &artifact, + target, Some(&context), self.picotool_timeout, picotool::PicotoolMode::Fallback, @@ -1774,7 +1859,16 @@ impl Deployer for Rp2040Deployer { } } Rp2040Transport::Picotool => { - match attempt_picotool_primary(project_dir, &artifact, self.picotool_timeout).await + let target = picotool_target + .as_ref() + .ok_or_else(picotool_identity_required_error)?; + match attempt_picotool_primary( + project_dir, + &artifact, + target, + self.picotool_timeout, + ) + .await { Ok(loaded) => ( loaded.stdout, @@ -2547,6 +2641,87 @@ mod tests { assert!(!profile_matches_family(&profile, RP2350_FAMILY_ID)); } + #[test] + fn picotool_target_uses_the_single_registry_bootsel_identity() { + use fbuild_core::usb::profiles::{ + UsbDeviceRole, UsbIdentityMatch, UsbProfileProvenance, UsbPurpose, UsbTransportProfile, + }; + + let bootloader = UsbTransportProfile { + identity_match: UsbIdentityMatch { + vid: "feed".to_string(), + pid: Some("c0de".to_string()), + pid_mask: None, + }, + purpose: UsbPurpose::Bootloader, + role: UsbDeviceRole::BootloaderUf2, + transport: "usb".to_string(), + reset: "touch-1200".to_string(), + handoff: "bootloader".to_string(), + platform: Some("synthetic".to_string()), + family: Some("rp2350".to_string()), + generation: Some("synthetic".to_string()), + interface: Some("msc".to_string()), + provenance: UsbProfileProvenance { + source_url: "test://fixture".to_string(), + source_revision: "a".repeat(40), + source_class: "test".to_string(), + }, + priority: 100, + allow_ambiguous: false, + }; + let target = picotool_target_from_profiles( + "SERIAL", + RP2350_FAMILY_ID, + &[bootloader.clone(), bootloader], + ) + .unwrap(); + assert!(target.matches_usb_instance("USB\\VID_FEED&PID_C0DE&MI_01\\SERIAL")); + assert!(!target.matches_usb_instance("USB\\VID_FEED&PID_BEEF&MI_01\\SERIAL")); + } + + #[test] + fn picotool_target_refuses_ambiguous_registry_bootsel_identities() { + use fbuild_core::usb::profiles::{ + UsbDeviceRole, UsbIdentityMatch, UsbProfileProvenance, UsbPurpose, UsbTransportProfile, + }; + + let profile = |pid: &str| UsbTransportProfile { + identity_match: UsbIdentityMatch { + vid: "feed".to_string(), + pid: Some(pid.to_string()), + pid_mask: None, + }, + purpose: UsbPurpose::Bootloader, + role: UsbDeviceRole::BootloaderUf2, + transport: "usb".to_string(), + reset: "touch-1200".to_string(), + handoff: "bootloader".to_string(), + platform: Some("synthetic".to_string()), + family: Some("rp2350".to_string()), + generation: Some("synthetic".to_string()), + interface: Some("msc".to_string()), + provenance: UsbProfileProvenance { + source_url: "test://fixture".to_string(), + source_revision: "a".repeat(40), + source_class: "test".to_string(), + }, + priority: 100, + allow_ambiguous: false, + }; + let error = picotool_target_from_profiles( + "SERIAL", + RP2350_FAMILY_ID, + &[profile("c0de"), profile("beef")], + ) + .unwrap_err(); + assert!( + error + .to_string() + .contains("multiple rp2350 BOOTSEL identities") + ); + } + #[test] fn successful_rom_transfer_waits_for_marker_disappearance() { let volume = tempdir().unwrap(); @@ -3371,12 +3546,12 @@ mod tests { } #[test] - fn picotool_transport_still_attempts_on_other_problem() { + fn picotool_transport_skips_picotool_on_an_unusable_bootsel_device() { let preflight = preflight::PicobootPreflight::OtherProblem { instance_id: "USB\\VID_2E8A&PID_0003&MI_01\\x".to_string(), problem_code: 43, }; - assert!(should_attempt_picotool_first( + assert!(!should_attempt_picotool_first( Rp2040Transport::Picotool, Some(&preflight) )); diff --git a/crates/fbuild-deploy/src/rp2040_picotool.rs b/crates/fbuild-deploy/src/rp2040_picotool.rs index 1246ada96..7878cd2e9 100644 --- a/crates/fbuild-deploy/src/rp2040_picotool.rs +++ b/crates/fbuild-deploy/src/rp2040_picotool.rs @@ -8,6 +8,32 @@ use std::time::Duration; use fbuild_core::{FbuildError, Result}; use fbuild_packages::Package; +/// BOOTSEL identity selected before fbuild invokes picotool. The runtime CDC +/// port is only a control path; the serial number and ROM PID bind the +/// subsequent PICOBOOT operation to the board that was touched. +#[derive(Debug)] +pub(super) struct PicotoolTarget { + serial_number: String, + vendor_id: String, + product_id: String, +} + +impl PicotoolTarget { + pub(super) fn new(serial_number: &str, vendor_id: &str, product_id: &str) -> Self { + Self { + serial_number: serial_number.to_string(), + vendor_id: vendor_id.to_string(), + product_id: product_id.to_string(), + } + } + + pub(super) fn matches_usb_instance(&self, instance_id: &str) -> bool { + let upper = instance_id.to_ascii_uppercase(); + upper.contains(&format!("VID_{}", self.vendor_id.to_ascii_uppercase())) + && upper.contains(&format!("PID_{}", self.product_id.to_ascii_uppercase())) + } +} + pub(super) struct PicotoolLoad { pub stdout: String, pub stderr: String, @@ -49,11 +75,15 @@ pub(super) enum PriorTransportFailure { /// reachable before committing to the (longer) load timeout. Failure here is /// classified as a transport/device failure by the caller, which falls back /// to mass-storage. -pub(super) async fn probe_picotool_info(project_dir: &Path, timeout: Duration) -> Result<()> { +pub(super) async fn probe_picotool_info( + project_dir: &Path, + target: &PicotoolTarget, + timeout: Duration, +) -> Result<()> { let package = fbuild_packages::toolchain::Rp2040Picotool::new(project_dir); Package::ensure_installed(&package).await?; let executable = package.executable(); - let args = info_probe_args(&executable); + let args = info_probe_args(&executable, target); let args_ref: Vec<&str> = args.iter().map(String::as_str).collect(); let output = fbuild_core::subprocess::run_command(&args_ref, None, None, Some(timeout)).await?; if !output.success() { @@ -94,6 +124,7 @@ pub(super) async fn probe_uf2_rejection_info( pub(super) async fn load_with_managed_picotool( project_dir: &Path, artifact: &Path, + target: &PicotoolTarget, mass_storage_error: Option<&str>, timeout: Duration, mode: PicotoolMode, @@ -101,7 +132,7 @@ pub(super) async fn load_with_managed_picotool( let package = fbuild_packages::toolchain::Rp2040Picotool::new(project_dir); Package::ensure_installed(&package).await?; let executable = package.executable(); - let args = load_args(&executable, artifact); + let args = load_args(&executable, artifact, target); let args_ref: Vec<&str> = args.iter().map(String::as_str).collect(); let output = fbuild_core::subprocess::run_command(&args_ref, None, None, Some(timeout)).await?; if !output.success() { @@ -133,18 +164,32 @@ fn combined_tool_output(stdout: &str, stderr: &str) -> String { .join("\n") } -fn load_args(executable: &Path, artifact: &Path) -> Vec { - vec![ +fn append_target_selection(args: &mut Vec, target: &PicotoolTarget) { + args.extend([ + "--vid".to_string(), + format!("0x{}", target.vendor_id), + "--pid".to_string(), + format!("0x{}", target.product_id), + "--ser".to_string(), + target.serial_number.to_string(), + ]); +} + +fn load_args(executable: &Path, artifact: &Path, target: &PicotoolTarget) -> Vec { + let mut args = vec![ executable.to_string_lossy().to_string(), "load".to_string(), artifact.to_string_lossy().to_string(), - "-f".to_string(), "-x".to_string(), - ] + ]; + append_target_selection(&mut args, target); + args } -fn info_probe_args(executable: &Path) -> Vec { - vec![executable.to_string_lossy().to_string(), "info".to_string()] +fn info_probe_args(executable: &Path, target: &PicotoolTarget) -> Vec { + let mut args = vec![executable.to_string_lossy().to_string(), "info".to_string()]; + append_target_selection(&mut args, target); + args } fn uf2_info_args(executable: &Path) -> Vec { @@ -219,19 +264,50 @@ pub(super) fn format_eject_failure( mod tests { use super::*; + fn rp2350_target() -> PicotoolTarget { + PicotoolTarget::new("2DCB876B587EA334", "2e8a", "000f") + } + #[test] - fn load_uses_managed_executable_force_flag_and_reboots_after_success() { - let args = load_args(Path::new("managed/picotool"), Path::new("firmware.uf2")); + fn load_is_bound_to_the_bootsel_target_and_reboots_after_success() { + let args = load_args( + Path::new("managed/picotool"), + Path::new("firmware.uf2"), + &rp2350_target(), + ); assert_eq!( args, - ["managed/picotool", "load", "firmware.uf2", "-f", "-x"] + [ + "managed/picotool", + "load", + "firmware.uf2", + "-x", + "--vid", + "0x2e8a", + "--pid", + "0x000f", + "--ser", + "2DCB876B587EA334", + ] ); } #[test] - fn info_probe_uses_managed_executable() { - let args = info_probe_args(Path::new("managed/picotool")); - assert_eq!(args, ["managed/picotool", "info"]); + fn info_probe_uses_the_same_bootsel_target() { + let args = info_probe_args(Path::new("managed/picotool"), &rp2350_target()); + assert_eq!( + args, + [ + "managed/picotool", + "info", + "--vid", + "0x2e8a", + "--pid", + "0x000f", + "--ser", + "2DCB876B587EA334", + ] + ); } #[test] diff --git a/crates/fbuild-deploy/src/rp2040_preflight.rs b/crates/fbuild-deploy/src/rp2040_preflight.rs index 8d78faece..47fad1ac3 100644 --- a/crates/fbuild-deploy/src/rp2040_preflight.rs +++ b/crates/fbuild-deploy/src/rp2040_preflight.rs @@ -10,6 +10,8 @@ use fbuild_serial::ports::UsbProblemDevice; +use super::picotool::PicotoolTarget; + /// `CM_PROB_FAILED_INSTALL`: Windows Config Manager could not install a /// driver for the devnode. const CM_PROB_FAILED_INSTALL: u32 = 28; @@ -21,10 +23,6 @@ const CM_PROB_NOT_CONFIGURED: u32 = 1; /// part of the FAILED_INSTALL family. const CM_PROB_FAILED_ADD: u32 = 31; -/// The RP2040 ROM bootloader's USB identity. `&MI_` in the instance ID marks -/// the composite-interface devnode (as opposed to the parent composite -/// device), which is what picotool needs a working driver on. -const PICOBOOT_VID_PID: &str = "VID_2E8A&PID_0003"; const COMPOSITE_INTERFACE_MARKER: &str = "&MI_"; /// Classification of the PICOBOOT devnode's driver health, computed purely @@ -44,20 +42,20 @@ pub(super) enum PicobootPreflight { problem_code: u32, }, /// The PICOBOOT interface devnode reports some other nonzero problem - /// code. Not treated as fatal at preflight time — picotool is still - /// attempted, since the probe step's own failure is authoritative. + /// code. Windows cannot provide a usable vendor interface, so picotool + /// is skipped in favor of BOOTSEL mass-storage. OtherProblem { instance_id: String, problem_code: u32, }, } -/// True for the PICOBOOT composite-interface devnode: a `USB\VID_2E8A&PID_0003` -/// instance ID that also names a specific interface (`&MI_xx`), as opposed to -/// the parent composite device or an unrelated USB node. -fn is_picoboot_interface_devnode(instance_id: &str) -> bool { +/// True for the registry-selected PICOBOOT composite-interface devnode. The +/// `&MI_` marker distinguishes the interface picotool opens from the parent +/// composite device. +fn is_picoboot_interface_devnode(instance_id: &str, target: &PicotoolTarget) -> bool { let upper = instance_id.to_ascii_uppercase(); - upper.contains(PICOBOOT_VID_PID) && upper.contains(COMPOSITE_INTERFACE_MARKER) + target.matches_usb_instance(instance_id) && upper.contains(COMPOSITE_INTERFACE_MARKER) } fn is_driver_missing_family(problem_code: u32) -> bool { @@ -70,9 +68,12 @@ fn is_driver_missing_family(problem_code: u32) -> bool { /// Pure classification (FastLED/fbuild#1163): given a snapshot of present /// USB problem devnodes, decide whether the PICOBOOT interface has a /// driver-missing problem that should skip the picotool attempt outright. -pub(super) fn classify_picoboot_preflight(devices: &[UsbProblemDevice]) -> PicobootPreflight { +pub(super) fn classify_picoboot_preflight( + devices: &[UsbProblemDevice], + target: &PicotoolTarget, +) -> PicobootPreflight { for device in devices { - if !is_picoboot_interface_devnode(&device.instance_id) { + if !is_picoboot_interface_devnode(&device.instance_id, target) { continue; } return if is_driver_missing_family(device.problem_code) { @@ -99,11 +100,28 @@ pub(super) fn driver_missing_message(instance_id: &str, problem_code: u32) -> St ) } +pub(super) fn problem_message(preflight: &PicobootPreflight) -> String { + match preflight { + PicobootPreflight::Ready => String::new(), + PicobootPreflight::DriverMissing { + instance_id, + problem_code, + } => driver_missing_message(instance_id, *problem_code), + PicobootPreflight::OtherProblem { + instance_id, + problem_code, + } => format!( + "RP-series PICOBOOT interface {instance_id} reports Windows Config Manager problem code {problem_code}; skipping picotool because Windows cannot provide a usable vendor interface. fbuild will use the BOOTSEL mass-storage fallback if it appears." + ), + } +} + #[cfg(test)] mod tests { use super::*; const BOOTSEL_INTERFACE: &str = "USB\\VID_2E8A&PID_0003&MI_01\\8&22CF742D&0&0001"; + const RP2350_BOOTSEL_INTERFACE: &str = "USB\\VID_2E8A&PID_000F&MI_01\\8&22CF742D&0&0001"; const BOOTSEL_COMPOSITE: &str = "USB\\VID_2E8A&PID_0003\\E0C9125B0D9B"; const UNRELATED: &str = "USB\\VID_25A7&PID_2510\\receiver"; @@ -119,16 +137,27 @@ mod tests { } } + fn rp2040_target() -> PicotoolTarget { + PicotoolTarget::new("test", "2e8a", "0003") + } + + fn rp2350_target() -> PicotoolTarget { + PicotoolTarget::new("test", "2e8a", "000f") + } + #[test] fn empty_snapshot_is_ready() { - assert_eq!(classify_picoboot_preflight(&[]), PicobootPreflight::Ready); + assert_eq!( + classify_picoboot_preflight(&[], &rp2040_target()), + PicobootPreflight::Ready + ); } #[test] fn problem_code_28_is_driver_missing() { let devices = [device(BOOTSEL_INTERFACE, 28)]; assert_eq!( - classify_picoboot_preflight(&devices), + classify_picoboot_preflight(&devices, &rp2040_target()), PicobootPreflight::DriverMissing { instance_id: BOOTSEL_INTERFACE.to_string(), problem_code: 28, @@ -141,7 +170,7 @@ mod tests { for code in [1, 31] { let devices = [device(BOOTSEL_INTERFACE, code)]; assert_eq!( - classify_picoboot_preflight(&devices), + classify_picoboot_preflight(&devices, &rp2040_target()), PicobootPreflight::DriverMissing { instance_id: BOOTSEL_INTERFACE.to_string(), problem_code: code, @@ -154,7 +183,7 @@ mod tests { fn other_problem_code_is_other_problem() { let devices = [device(BOOTSEL_INTERFACE, 43)]; assert_eq!( - classify_picoboot_preflight(&devices), + classify_picoboot_preflight(&devices, &rp2040_target()), PicobootPreflight::OtherProblem { instance_id: BOOTSEL_INTERFACE.to_string(), problem_code: 43, @@ -166,7 +195,7 @@ mod tests { fn unrelated_devices_are_ignored() { let devices = [device(UNRELATED, 28)]; assert_eq!( - classify_picoboot_preflight(&devices), + classify_picoboot_preflight(&devices, &rp2040_target()), PicobootPreflight::Ready ); } @@ -177,7 +206,23 @@ mod tests { // devnode picotool needs a driver on; only the interface node counts. let devices = [device(BOOTSEL_COMPOSITE, 28)]; assert_eq!( - classify_picoboot_preflight(&devices), + classify_picoboot_preflight(&devices, &rp2040_target()), + PicobootPreflight::Ready + ); + } + + #[test] + fn rp2350_bootloader_problem_is_not_mistaken_for_rp2040() { + let devices = [device(RP2350_BOOTSEL_INTERFACE, 43)]; + assert_eq!( + classify_picoboot_preflight(&devices, &rp2350_target()), + PicobootPreflight::OtherProblem { + instance_id: RP2350_BOOTSEL_INTERFACE.to_string(), + problem_code: 43, + } + ); + assert_eq!( + classify_picoboot_preflight(&devices, &rp2040_target()), PicobootPreflight::Ready ); } @@ -191,4 +236,15 @@ mod tests { assert!(message.contains("RP2 Boot (Interface 1)")); assert!(message.contains("not a board fault")); } + + #[test] + fn other_problem_message_skips_picotool_without_claiming_a_driver_fix() { + let message = problem_message(&PicobootPreflight::OtherProblem { + instance_id: RP2350_BOOTSEL_INTERFACE.to_string(), + problem_code: 43, + }); + assert!(message.contains("43")); + assert!(message.contains("skipping picotool")); + assert!(message.contains("mass-storage fallback")); + } } diff --git a/pyproject.toml b/pyproject.toml index 2bc3d62d6..de5ffdb8c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "fbuild" -version = "2.5.13" +version = "2.5.14" description = "PlatformIO-compatible embedded build tool (Rust implementation)" readme = "README.md" requires-python = ">=3.10"