diff --git a/Cargo.lock b/Cargo.lock index 81dacf2d..2f44c811 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -951,7 +951,7 @@ checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" [[package]] name = "fbuild-bench-fastled-examples" -version = "2.5.16" +version = "2.5.17" dependencies = [ "fbuild-core", "fbuild-library-select", @@ -965,7 +965,7 @@ dependencies = [ [[package]] name = "fbuild-build" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "blake3", @@ -1002,7 +1002,7 @@ dependencies = [ [[package]] name = "fbuild-build-arm" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "blake3", @@ -1036,7 +1036,7 @@ dependencies = [ [[package]] name = "fbuild-build-engine" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "blake3", @@ -1069,7 +1069,7 @@ dependencies = [ [[package]] name = "fbuild-build-esp" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "blake3", @@ -1103,7 +1103,7 @@ dependencies = [ [[package]] name = "fbuild-build-mcu" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "blake3", @@ -1137,7 +1137,7 @@ dependencies = [ [[package]] name = "fbuild-cli" -version = "2.5.16" +version = "2.5.17" dependencies = [ "blake3", "clap", @@ -1170,7 +1170,7 @@ dependencies = [ [[package]] name = "fbuild-config" -version = "2.5.16" +version = "2.5.17" dependencies = [ "fbuild-core", "fbuild-paths", @@ -1185,7 +1185,7 @@ dependencies = [ [[package]] name = "fbuild-core" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "fs2", @@ -1206,7 +1206,7 @@ dependencies = [ [[package]] name = "fbuild-daemon" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "axum", @@ -1246,7 +1246,7 @@ dependencies = [ [[package]] name = "fbuild-deploy" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "espflash", @@ -1275,7 +1275,7 @@ dependencies = [ [[package]] name = "fbuild-header-scan" -version = "2.5.16" +version = "2.5.17" dependencies = [ "criterion", "fbuild-paths", @@ -1286,7 +1286,7 @@ dependencies = [ [[package]] name = "fbuild-library" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "axum", @@ -1316,7 +1316,7 @@ dependencies = [ [[package]] name = "fbuild-library-select" -version = "2.5.16" +version = "2.5.17" dependencies = [ "bincode", "blake3", @@ -1336,7 +1336,7 @@ dependencies = [ [[package]] name = "fbuild-packages" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "axum", @@ -1368,7 +1368,7 @@ dependencies = [ [[package]] name = "fbuild-packages-fetch" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "axum", @@ -1397,7 +1397,7 @@ dependencies = [ [[package]] name = "fbuild-paths" -version = "2.5.16" +version = "2.5.17" dependencies = [ "fbuild-core", "serde", @@ -1408,7 +1408,7 @@ dependencies = [ [[package]] name = "fbuild-python" -version = "2.5.16" +version = "2.5.17" dependencies = [ "base64", "fbuild-core", @@ -1430,7 +1430,7 @@ dependencies = [ [[package]] name = "fbuild-serial" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "base64", @@ -1454,7 +1454,7 @@ dependencies = [ [[package]] name = "fbuild-test-support" -version = "2.5.16" +version = "2.5.17" dependencies = [ "fbuild-config", "fbuild-core", @@ -1474,7 +1474,7 @@ dependencies = [ [[package]] name = "fbuild-toolchain" -version = "2.5.16" +version = "2.5.17" dependencies = [ "async-trait", "axum", diff --git a/agents/docs/deploy-architecture.md b/agents/docs/deploy-architecture.md index 90417583..f9052e9c 100644 --- a/agents/docs/deploy-architecture.md +++ b/agents/docs/deploy-architecture.md @@ -92,17 +92,29 @@ The follow-up issues track the full polymorphic version: `crates/fbuild-deploy/src/rp2040.rs` (`--transport picotool|uf2`, 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, +- **Shared reset ladder.** After the pre-touch scan and 1200-bps CDC touch, + fbuild waits for BOOTSEL. If none appears and the selected runtime endpoint + supplied an exact VID, PID, and non-empty USB serial, it asks the Pico SDK + application reset interface to enter BOOTSEL and waits again. On Windows, + fbuild maps the selected CDC identity to one exact healthy WinUSB reset + interface and sends the Pico class control request directly. Other hosts use + managed `picotool reboot -u --vid ... --pid ... -f`; picotool derives the + runtime serial from the opened application device because that interface + cannot be selected reliably with `--ser`. The fallback is attempted only + after fbuild resolves one exact runtime identity, and picotool refuses a + forced command when the VID/PID is ambiguous. A missing or ambiguous identity + skips this layer, so an unscoped forced command is impossible. Deployment + results name the application reset-interface reboot when it ran successfully. +- **`picotool` (default).** After the shared reset ladder and UF2 preparation, 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; + The ROM load does not use `-f`; forced application reset is the separately + scoped ladder step above. 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 @@ -129,6 +141,14 @@ image, a quiet runtime-CDC window no longer fails the deploy — it reports re-flash a healthy board whose first-plug driver install outlived the window. A genuine port-enumeration error still fails. +On Windows, the daemon retains the last non-empty `LocationPaths` observed +for a runtime endpoint. An unidentified `VID_0000&PID_0002` Code 43 node is +eligible for the explicit/admin-gated exact-child restart only when one and +only one matching RP history has the same normalized physical USB path. The +elevated helper re-queries that path before acting. A different location or +multiple matches fail closed; fbuild never cycles a hub or changes the +host-wide selective-suspend policy. + ## Worked example — "agent ports a new RP2350 board" The right sequence today: diff --git a/ci/check_usb_vidpid_literals.py b/ci/check_usb_vidpid_literals.py index 8ef2ec03..62305ae2 100644 --- a/ci/check_usb_vidpid_literals.py +++ b/ci/check_usb_vidpid_literals.py @@ -50,7 +50,7 @@ r"(?i)[\[(]\s*0x([0-9a-f]{4})\s*,\s*0x([0-9a-f]{4})\s*[\])]" ) NAMED_LITERAL_RE = re.compile( - r"(?i)\b(?:vid|pid|[a-z0-9]+_(?:vid|pid))\b\s*" + r"(?i)\b(?:vid|pid|[a-z0-9_]+_(?:vid|pid))\b\s*" r"(?::\s*[a-z_][a-z0-9_:<>]*)?\s*(?:==|!=|=|:)\s*" r"(?:Some\(\s*)?\b(0x[0-9a-f]{4}|[1-9][0-9]{0,4})\b" ) @@ -65,6 +65,16 @@ ) CFG_TEST_RE = re.compile(r"#\s*\[\s*cfg\s*\(\s*test\s*\)\s*\]") +# This is a Windows PnP protocol sentinel, not a board identity. Keep the +# exception exact in path, spelling, type, and value so it cannot become a +# second production VID/PID catalogue. +IDENTITY_LITERAL_EXCEPTIONS = { + ( + "crates/fbuild-core/src/usb/recovery.rs", + "pub const WINDOWS_DESCRIPTOR_FAILURE_PID: u16 = 2;", + ) +} + @dataclass(frozen=True) class Finding: @@ -210,6 +220,12 @@ def scan_text(path: str, source: str) -> list[Finding]: for pattern, reason in checks: if not pattern.search(line): continue + if ( + reason == "named VID/PID literal" + and (path.replace("\\", "/"), stripped) + in IDENTITY_LITERAL_EXCEPTIONS + ): + continue key = (line_number, reason) if key not in seen: findings.append(Finding(path, line_number, reason, stripped[:160])) diff --git a/ci/test_check_usb_vidpid_literals.py b/ci/test_check_usb_vidpid_literals.py index 3d8ae5ee..08cd5ad3 100644 --- a/ci/test_check_usb_vidpid_literals.py +++ b/ci/test_check_usb_vidpid_literals.py @@ -28,6 +28,23 @@ def test_named_decimal_literal_is_rejected(self): reasons = self.reasons("crates/demo/src/device.rs", "const DEVICE_VID: u16 = 4660;") self.assertIn("named VID/PID literal", reasons) + def test_exact_windows_descriptor_failure_sentinel_is_allowed_only_in_core_contract(self): + declaration = "pub const WINDOWS_DESCRIPTOR_FAILURE_PID: u16 = 2;" + self.assertEqual( + self.reasons("crates/fbuild-core/src/usb/recovery.rs", declaration), [] + ) + self.assertIn( + "named VID/PID literal", + self.reasons("crates/demo/src/device.rs", declaration), + ) + self.assertIn( + "named VID/PID literal", + self.reasons( + "crates/fbuild-core/src/usb/recovery.rs", + "pub const WINDOWS_DESCRIPTOR_FAILURE_PID: u16 = 3;", + ), + ) + def test_board_json_separate_fields_are_rejected(self): findings = guard.scan_text( "crates/fbuild-config/assets/boards/json/demo.json", diff --git a/crates/fbuild-cli/src/cli/deploy.rs b/crates/fbuild-cli/src/cli/deploy.rs index a44f70d9..61ffe5a1 100644 --- a/crates/fbuild-cli/src/cli/deploy.rs +++ b/crates/fbuild-cli/src/cli/deploy.rs @@ -732,6 +732,7 @@ mod tests { instance_id: None, parent_instance_id: None, ancestor_instance_ids: Vec::new(), + location_paths: Vec::new(), } } diff --git a/crates/fbuild-cli/src/cli/port_doctor.rs b/crates/fbuild-cli/src/cli/port_doctor.rs index 39910e5e..89c93959 100644 --- a/crates/fbuild-cli/src/cli/port_doctor.rs +++ b/crates/fbuild-cli/src/cli/port_doctor.rs @@ -665,6 +665,7 @@ mod tests { location: Some("Port_#0014.Hub_#0001".into()), behind_external_hub: Some(false), device_class: None, + location_paths: Vec::new(), parent_instance_id: None, }]; let out = render_report(&[diag(Some(false), None)], &problems); @@ -735,6 +736,7 @@ Power Scheme GUID: 381b4222-f694-41f0-9685-ff5bb260df2e (Balanced) location: Some("Port_#0014.Hub_#0001".into()), behind_external_hub: Some(false), device_class: None, + location_paths: Vec::new(), parent_instance_id: None, }]; let report = build_json_report(&[diag(Some(false), None)], &problems, Some(true)); diff --git a/crates/fbuild-cli/src/cli/port_scan.rs b/crates/fbuild-cli/src/cli/port_scan.rs index fb709c72..6c0d6eac 100644 --- a/crates/fbuild-cli/src/cli/port_scan.rs +++ b/crates/fbuild-cli/src/cli/port_scan.rs @@ -833,6 +833,7 @@ mod tests { behind_external_hub: Some(true), parent_instance_id: None, device_class: None, + location_paths: Vec::new(), }]; let warning = format_usb_problem_warning(&devices); assert!(warning.contains("problem code 43")); @@ -852,6 +853,7 @@ mod tests { behind_external_hub: Some(false), parent_instance_id: None, device_class: None, + location_paths: Vec::new(), }]; let warning = format_usb_problem_warning(&devices); assert!(warning.contains("Unknown USB device")); diff --git a/crates/fbuild-cli/src/cli/usb_recovery.rs b/crates/fbuild-cli/src/cli/usb_recovery.rs index 6a2dbb86..2197aa16 100644 --- a/crates/fbuild-cli/src/cli/usb_recovery.rs +++ b/crates/fbuild-cli/src/cli/usb_recovery.rs @@ -473,6 +473,8 @@ mod tests { expected_vid: 0x2e8a, expected_pid: 0x000a, expected_serial: Some("serial".to_string()), + descriptor_failure_at_location: false, + expected_location_path: None, problem_code: Some(43), flash_completed: true, }, diff --git a/crates/fbuild-core/src/usb/mod.rs b/crates/fbuild-core/src/usb/mod.rs index 5ce6062f..2c235953 100644 --- a/crates/fbuild-core/src/usb/mod.rs +++ b/crates/fbuild-core/src/usb/mod.rs @@ -33,7 +33,8 @@ pub use data::{ pub use embedded::vendor_name as embedded_vendor_name; pub use recovery::{ UNCLASSED_DEVICE_CLASS, UsbRecoveryHealth, UsbRecoveryOperation, UsbRecoveryPolicy, - UsbRecoveryRequest, UsbRecoveryResult, + UsbRecoveryRequest, UsbRecoveryResult, is_windows_descriptor_failure_identity, + normalize_physical_location, }; #[cfg(test)] pub use resolver::resolve_bundled; diff --git a/crates/fbuild-core/src/usb/recovery.rs b/crates/fbuild-core/src/usb/recovery.rs index d71322d3..437d1beb 100644 --- a/crates/fbuild-core/src/usb/recovery.rs +++ b/crates/fbuild-core/src/usb/recovery.rs @@ -48,6 +48,36 @@ pub enum UsbRecoveryOperation { /// revalidation so an absent class is an exact-match fact, not a wildcard. pub const UNCLASSED_DEVICE_CLASS: &str = "(none)"; +/// Windows' USB descriptor-request-failure identity. This is an operating- +/// system protocol sentinel, not a board VID/PID record; board identities +/// remain sourced exclusively from the verified FastLED/boards catalogue. +pub const WINDOWS_DESCRIPTOR_FAILURE_VID: u16 = 0; +pub const WINDOWS_DESCRIPTOR_FAILURE_PID: u16 = 2; + +pub fn is_windows_descriptor_failure_identity(vid: u16, pid: u16) -> bool { + vid == WINDOWS_DESCRIPTOR_FAILURE_VID && pid == WINDOWS_DESCRIPTOR_FAILURE_PID +} + +/// Normalize one Windows USB `LocationPaths` value to its physical-device +/// path, removing a trailing composite-interface component when present. +/// +/// Non-USB and empty values are rejected so callers that correlate an +/// identity-lost device cannot accidentally treat an arbitrary PnP location +/// as a physical USB socket. +pub fn normalize_physical_location(path: &str) -> Option { + let upper = path.trim().to_ascii_uppercase(); + if upper.is_empty() || !upper.contains("#USB(") { + return None; + } + Some( + upper + .rsplit_once("#USBMI(") + .and_then(|(physical, interface)| interface.ends_with(')').then_some(physical)) + .unwrap_or(&upper) + .to_string(), + ) +} + /// Host health observed before or after a recovery operation. /// /// This is intentionally independent of `fbuild_serial::PortHealth` so the @@ -81,6 +111,16 @@ pub struct UsbRecoveryRequest { pub expected_pid: u16, /// Required when the board profile supplied a serial number. pub expected_serial: Option, + /// True only when Windows reported a descriptor-failed USB node that was + /// correlated to one historical board by an exact physical location. + /// The helper still revalidates the node's observed VID/PID and location. + #[serde(default)] + pub descriptor_failure_at_location: bool, + /// Normalized physical USB location that must still match when recovering + /// a descriptor-failed node whose current VID/PID cannot identify the + /// board. `None` for ordinary identity-bound recovery requests. + #[serde(default)] + pub expected_location_path: Option, /// Problem code observed by the normal process, if Windows supplied one. pub problem_code: Option, /// Distinguishes preflight recovery from post-flash recovery-only flow. @@ -102,6 +142,15 @@ impl UsbRecoveryRequest { }) } + let location_bound_shape_is_safe = if self.descriptor_failure_at_location { + is_windows_descriptor_failure_identity(self.expected_vid, self.expected_pid) + && self.expected_location_path.is_some() + && self.expected_serial.is_none() + && self.problem_code == Some(43) + } else { + self.expected_vid != 0 && self.expected_location_path.is_none() + }; + canonical_pnp_id(&self.operation_id) && canonical_pnp_id(&self.instance_id) && canonical_pnp_id(&self.expected_class) @@ -112,6 +161,15 @@ impl UsbRecoveryRequest { && self.expected_serial.as_deref().map_or(true, |serial| { !serial.is_empty() && serial.len() <= 256 && !serial.chars().any(char::is_control) }) + && self.expected_location_path.as_deref().map_or(true, |path| { + !path.is_empty() + && path.len() <= 1024 + && !path.chars().any(|character| { + character.is_control() + || matches!(character, '"' | '\'' | '\n' | '\r' | '\t') + }) + }) + && location_bound_shape_is_safe } } @@ -143,6 +201,8 @@ mod tests { expected_vid: 0x2e8a, expected_pid: 0x000a, expected_serial: Some("5303284720C4641C".to_string()), + descriptor_failure_at_location: false, + expected_location_path: None, problem_code: Some(43), flash_completed: true, } @@ -179,4 +239,61 @@ mod tests { bad_class.expected_class = "Ports\nUSB".to_string(); assert!(!bad_class.has_canonical_identity()); } + + #[test] + fn location_bound_request_requires_descriptor_failure_shape() { + let mut location_bound = request(); + location_bound.instance_id = "USB\\VID_0000&PID_0002\\descriptor-failed".to_string(); + location_bound.expected_class = "USB".to_string(); + location_bound.expected_vid = 0; + location_bound.expected_pid = 2; + location_bound.expected_serial = None; + location_bound.descriptor_failure_at_location = true; + location_bound.expected_location_path = Some("PCIROOT(0)#USBROOT(0)#USB(4)".to_string()); + location_bound.problem_code = Some(43); + assert!(location_bound.has_canonical_identity()); + + let mut missing_code = location_bound.clone(); + missing_code.problem_code = None; + assert!(!missing_code.has_canonical_identity()); + + let mut wrong_identity = location_bound.clone(); + wrong_identity.expected_vid = 0x2e8a; + assert!(!wrong_identity.has_canonical_identity()); + + let mut wrong_descriptor_failure_pid = location_bound.clone(); + wrong_descriptor_failure_pid.expected_pid = 3; + assert!(!wrong_descriptor_failure_pid.has_canonical_identity()); + + let mut missing_descriptor_failure_fact = location_bound.clone(); + missing_descriptor_failure_fact.descriptor_failure_at_location = false; + assert!(!missing_descriptor_failure_fact.has_canonical_identity()); + + let mut unexpected_serial = location_bound; + unexpected_serial.expected_serial = Some("not-authoritative".to_string()); + assert!(!unexpected_serial.has_canonical_identity()); + + let mut missing_location = request(); + missing_location.expected_vid = 0; + missing_location.expected_pid = 2; + missing_location.expected_serial = None; + missing_location.descriptor_failure_at_location = true; + missing_location.problem_code = Some(43); + assert!(!missing_location.has_canonical_identity()); + } + + #[test] + fn physical_location_normalization_is_shared_and_usb_only() { + assert_eq!( + normalize_physical_location(" pciroot(0)#usbroot(0)#usb(10)#usb(4)#usbmi(2) ") + .as_deref(), + Some("PCIROOT(0)#USBROOT(0)#USB(10)#USB(4)") + ); + assert_eq!( + normalize_physical_location("PCIROOT(0)#USBROOT(0)#USB(4)").as_deref(), + Some("PCIROOT(0)#USBROOT(0)#USB(4)") + ); + assert_eq!(normalize_physical_location("PCIROOT(0)#PCI(1)"), None); + assert_eq!(normalize_physical_location(" "), None); + } } diff --git a/crates/fbuild-daemon/src/device_manager.rs b/crates/fbuild-daemon/src/device_manager.rs index 2ce4291f..54cc7f9d 100644 --- a/crates/fbuild-daemon/src/device_manager.rs +++ b/crates/fbuild-daemon/src/device_manager.rs @@ -118,6 +118,10 @@ pub struct DeviceState { /// Canonical Plug and Play identity when the host exposes one. pub instance_id: Option, pub parent_instance_id: Option, + /// Last non-empty physical USB location paths observed for this endpoint. + /// Retained across a later phantom refresh for descriptor-failure + /// correlation; never used to make a serial endpoint selectable. + pub location_paths: Vec, pub previous_port: Option, pub exclusive_lease: Option, pub monitor_leases: HashMap, @@ -173,6 +177,7 @@ struct DiscoveredDevice { port_health: fbuild_serial::ports::PortHealth, instance_id: Option, parent_instance_id: Option, + location_paths: Vec, } /// Thread-safe device manager. @@ -250,6 +255,7 @@ impl DeviceManager { let port_health = detected.health.clone(); let instance_id = detected.instance_id.clone(); let parent_instance_id = detected.parent_instance_id.clone(); + let location_paths = detected.location_paths.clone(); let port_info = detected.info; let (vid, pid, fallback_desc) = match &port_info.port_type { serialport::SerialPortType::UsbPort(usb) => ( @@ -303,6 +309,7 @@ impl DeviceManager { port_health, instance_id, parent_instance_id, + location_paths, } }) .collect(); @@ -359,6 +366,9 @@ impl DeviceManager { state.port_health = device.port_health; state.instance_id = device.instance_id; state.parent_instance_id = device.parent_instance_id; + if !device.location_paths.is_empty() { + state.location_paths = device.location_paths; + } if let Some(previous_port) = state.previous_port.clone() { self.recent_port_moves .lock() @@ -389,6 +399,7 @@ impl DeviceManager { port_health: device.port_health.clone(), instance_id: device.instance_id.clone(), parent_instance_id: device.parent_instance_id.clone(), + location_paths: device.location_paths.clone(), previous_port: None, exclusive_lease: None, monitor_leases: HashMap::new(), @@ -411,6 +422,9 @@ impl DeviceManager { entry.port_health = device.port_health; entry.instance_id = device.instance_id; entry.parent_instance_id = device.parent_instance_id; + if !device.location_paths.is_empty() { + entry.location_paths = device.location_paths; + } } // Stamp `last_disconnect_at` for every device that went from @@ -730,6 +744,7 @@ impl DeviceManager { port_health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, + location_paths: Vec::new(), previous_port: None, exclusive_lease: None, monitor_leases: HashMap::new(), diff --git a/crates/fbuild-daemon/src/device_manager/tests.rs b/crates/fbuild-daemon/src/device_manager/tests.rs index 4df29e6e..a83a981a 100644 --- a/crates/fbuild-daemon/src/device_manager/tests.rs +++ b/crates/fbuild-daemon/src/device_manager/tests.rs @@ -193,6 +193,7 @@ fn tracked_serial_lease_moves_to_new_port_on_refresh() { port_health: fbuild_serial::ports::PortHealth::HealthyPresent, instance_id: Some(r"USB\VID_1234&PID_5678\TEST-SERIAL".to_string()), parent_instance_id: Some(r"USB\VID_1234&PID_5678\PARENT".to_string()), + location_paths: vec!["PCIROOT(0)#USBROOT(0)#USB(3)".to_string()], }]); assert!(mgr.get_device_status("COM3").is_none()); @@ -229,6 +230,43 @@ fn tracked_serial_lease_moves_to_new_port_on_refresh() { ); } +#[test] +fn phantom_refresh_retains_last_nonempty_usb_location_paths() { + let mgr = DeviceManager::new(); + let discovered = |health, location_paths| DiscoveredDevice { + port: "COM18".to_string(), + device_id: "2e8a:f00f".to_string(), + description: "Raspberry Pi Pico 2 W".to_string(), + vid: Some(0x2e8a), + pid: Some(0xf00f), + vendor_name: Some("Raspberry Pi".to_string()), + product_name: Some("Pico 2".to_string()), + is_cdc: Some(true), + serial_number: Some("2DCB876B587EA334".to_string()), + port_health: health, + instance_id: Some("USB\\VID_2E8A&PID_F00F\\2DCB876B587EA334".to_string()), + parent_instance_id: Some("USB\\ROOT_HUB30\\parent".to_string()), + location_paths, + }; + let location = "PCIROOT(0)#USBROOT(0)#USB(10)#USB(4)#USBMI(0)".to_string(); + mgr.refresh_from_discovered(vec![discovered( + fbuild_serial::ports::PortHealth::HealthyPresent, + vec![location.clone()], + )]); + mgr.refresh_from_discovered(vec![discovered( + fbuild_serial::ports::PortHealth::Phantom { + problem_code: None, + status: None, + }, + Vec::new(), + )]); + + assert_eq!( + mgr.get_device_status("COM18").unwrap().location_paths, + vec![location] + ); +} + #[test] fn untracked_serial_lease_stays_on_old_disconnected_port() { let mgr = make_manager_with_device("COM3"); @@ -251,6 +289,7 @@ fn untracked_serial_lease_stays_on_old_disconnected_port() { }, instance_id: Some(r"USB\VID_1234&PID_5678\TEST-SERIAL".to_string()), parent_instance_id: Some(r"USB\VID_1234&PID_5678\PARENT".to_string()), + location_paths: Vec::new(), }]); let old = mgr.get_device_status("COM3").unwrap(); diff --git a/crates/fbuild-daemon/src/handlers/operations/deploy_port.rs b/crates/fbuild-daemon/src/handlers/operations/deploy_port.rs index e9608a2c..fd7168b5 100644 --- a/crates/fbuild-daemon/src/handlers/operations/deploy_port.rs +++ b/crates/fbuild-daemon/src/handlers/operations/deploy_port.rs @@ -391,6 +391,7 @@ mod tests { port_health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, + location_paths: Vec::new(), previous_port: None, exclusive_lease: None, monitor_leases: HashMap::new(), diff --git a/crates/fbuild-daemon/src/handlers/operations/recovery_request.rs b/crates/fbuild-daemon/src/handlers/operations/recovery_request.rs index f1af4718..91dd682a 100644 --- a/crates/fbuild-daemon/src/handlers/operations/recovery_request.rs +++ b/crates/fbuild-daemon/src/handlers/operations/recovery_request.rs @@ -9,8 +9,12 @@ //! single allowlisted PnP operation. use crate::device_manager::DeviceState; -use fbuild_core::usb::{UNCLASSED_DEVICE_CLASS, UsbRecoveryRequest}; +use fbuild_core::usb::{ + UNCLASSED_DEVICE_CLASS, UsbRecoveryRequest, is_windows_descriptor_failure_identity, + normalize_physical_location, +}; use fbuild_serial::ports::UsbProblemDevice; +use std::collections::BTreeSet; /// Pick the exact-device recovery target from fresh scan facts. /// @@ -51,6 +55,8 @@ pub(super) fn compose_rp2040_recovery_request( .clone() .unwrap_or_else(|| UNCLASSED_DEVICE_CLASS.to_string()), expected_serial: serial_from_matching_parent(&device.instance_id, &parent), + descriptor_failure_at_location: false, + expected_location_path: None, parent_instance_id: Some(parent), expected_vid: vid, expected_pid: pid, @@ -58,6 +64,81 @@ pub(super) fn compose_rp2040_recovery_request( flash_completed, }); } + + // A descriptor-failed node has lost the board VID/PID and serial, so it + // is never safe to associate by proximity, hub ancestry, or timing. The + // sole exception is a unique equality between the node's current + // LocationPaths and a matching RP runtime endpoint's retained healthy + // LocationPaths. Interface suffixes identify USB functions rather than + // physical sockets and are normalized away on both sides. + let mut location_matches = BTreeSet::new(); + for historical in devices { + let (Some(vid), Some(pid), Some(serial)) = ( + historical.vid, + historical.pid, + historical.serial_number.as_deref(), + ) else { + continue; + }; + if !runtime_match(vid, pid) { + continue; + } + for historical_path in &historical.location_paths { + let Some(physical_path) = normalize_physical_location(historical_path) else { + continue; + }; + for problem in problem_devices { + let Some((problem_vid, problem_pid)) = parse_usb_vid_pid(&problem.instance_id) + else { + continue; + }; + if !is_windows_descriptor_failure_identity(problem_vid, problem_pid) + || problem.problem_code != 43 + || problem.parent_instance_id.is_none() + { + continue; + } + if problem.location_paths.iter().any(|candidate| { + normalize_physical_location(candidate).as_deref() + == Some(physical_path.as_str()) + }) { + location_matches.insert(( + historical.port.clone(), + serial.to_ascii_uppercase(), + historical.instance_id.clone().unwrap_or_default(), + problem.instance_id.clone(), + problem_vid, + problem_pid, + physical_path.clone(), + )); + } + } + } + } + if let Some((_, _, _, problem_instance, problem_vid, problem_pid, physical_path)) = + exactly_one(location_matches.into_iter()) + { + let problem = problem_devices + .iter() + .find(|device| device.instance_id.eq_ignore_ascii_case(&problem_instance))?; + let parent = problem.parent_instance_id.clone()?; + return Some(UsbRecoveryRequest { + operation_id: operation_id.to_string(), + instance_id: problem.instance_id.clone(), + expected_class: problem + .device_class + .clone() + .unwrap_or_else(|| UNCLASSED_DEVICE_CLASS.to_string()), + parent_instance_id: Some(parent), + expected_vid: problem_vid, + expected_pid: problem_pid, + expected_serial: None, + descriptor_failure_at_location: true, + expected_location_path: Some(physical_path), + problem_code: Some(problem.problem_code), + flash_completed, + }); + } for device in devices { if !device.port_health.is_known_unhealthy() { continue; @@ -84,6 +165,8 @@ pub(super) fn compose_rp2040_recovery_request( expected_vid: vid, expected_pid: pid, expected_serial: device.serial_number.clone(), + descriptor_failure_at_location: false, + expected_location_path: None, problem_code: device.port_health.problem_code(), flash_completed, }); @@ -91,6 +174,11 @@ pub(super) fn compose_rp2040_recovery_request( None } +fn exactly_one(mut values: impl Iterator) -> Option { + let first = values.next()?; + values.next().is_none().then_some(first) +} + fn is_composite_interface(instance_id: &str) -> bool { instance_id.to_ascii_uppercase().contains("&MI_") } @@ -131,6 +219,9 @@ mod tests { const BOOTSEL_INTERFACE: &str = "USB\\VID_2E8A&PID_0003&MI_01\\8&22CF742D&0&0001"; const BOOTSEL_COMPOSITE: &str = "USB\\VID_2E8A&PID_0003\\E0C9125B0D9B"; const PHANTOM_CDC: &str = "USB\\VID_2E8A&PID_000A\\5303284720C4641C"; + const RP_LOCATION: &str = "PCIROOT(0)#PCI(0103)#PCI(0000)#USBROOT(0)#USB(10)#USB(4)#USBMI(0)"; + const RP_PHYSICAL_LOCATION: &str = "PCIROOT(0)#PCI(0103)#PCI(0000)#USBROOT(0)#USB(10)#USB(4)"; + const UNKNOWN_CODE43: &str = "USB\\VID_0000&PID_0002\\6&3AF0F9CE&0&4"; fn phantom_cdc_device() -> DeviceState { DeviceState { @@ -149,6 +240,7 @@ mod tests { }, instance_id: Some(PHANTOM_CDC.to_string()), parent_instance_id: Some("USB\\ROOT_HUB30\\5&23f8e3f5&0&0".to_string()), + location_paths: vec![RP_LOCATION.to_string()], previous_port: None, exclusive_lease: None, monitor_leases: HashMap::new(), @@ -168,9 +260,117 @@ mod tests { behind_external_hub: Some(false), parent_instance_id: Some(BOOTSEL_COMPOSITE.to_string()), device_class: None, + location_paths: Vec::new(), } } + fn unidentified_code43(location: &str) -> UsbProblemDevice { + UsbProblemDevice { + instance_id: UNKNOWN_CODE43.to_string(), + problem_code: 43, + friendly_name: Some("Unknown USB Device".to_string()), + location: Some("Port_#0004.Hub_#0005".to_string()), + behind_external_hub: Some(true), + parent_instance_id: Some("USB\\ROOT_HUB30\\5&23f8e3f5&0&0".to_string()), + device_class: Some("USB".to_string()), + location_paths: vec![location.to_string()], + } + } + + #[test] + fn unidentified_code43_requires_unique_exact_physical_location() { + let request = compose_rp2040_recovery_request( + &[phantom_cdc_device()], + &[unidentified_code43(RP_PHYSICAL_LOCATION)], + "deploy-location", + false, + |_, _| false, + |vid, pid| (vid, pid) == (0x2e8a, 0x000a), + ) + .expect("the exact historical RP location must correlate uniquely"); + + assert_eq!(request.instance_id, UNKNOWN_CODE43); + assert_eq!((request.expected_vid, request.expected_pid), (0, 2)); + assert!(request.descriptor_failure_at_location); + assert_eq!( + request.expected_location_path.as_deref(), + Some(RP_PHYSICAL_LOCATION) + ); + } + + #[test] + fn unidentified_code43_mismatch_and_ambiguity_fail_closed() { + let mut historical = phantom_cdc_device(); + historical.port_health = PortHealth::HealthyPresent; + historical.is_connected = false; + let different = "PCIROOT(0)#PCI(0103)#PCI(0000)#USBROOT(0)#USB(14)"; + assert_eq!( + compose_rp2040_recovery_request( + std::slice::from_ref(&historical), + &[unidentified_code43(different)], + "deploy-mismatch", + false, + |_, _| false, + |vid, pid| (vid, pid) == (0x2e8a, 0x000a), + ), + None + ); + let mut wrong_sentinel_pid = unidentified_code43(RP_PHYSICAL_LOCATION); + wrong_sentinel_pid.instance_id = "USB\\VID_0000&PID_0003\\6&3AF0F9CE&0&4".to_string(); + assert_eq!( + compose_rp2040_recovery_request( + std::slice::from_ref(&historical), + &[wrong_sentinel_pid], + "deploy-wrong-sentinel-pid", + false, + |_, _| false, + |vid, pid| (vid, pid) == (0x2e8a, 0x000a), + ), + None + ); + let mut identified_problem = unidentified_code43(RP_PHYSICAL_LOCATION); + identified_problem.instance_id = "USB\\VID_1234&PID_0002\\6&3AF0F9CE&0&4".to_string(); + assert_eq!( + compose_rp2040_recovery_request( + std::slice::from_ref(&historical), + &[identified_problem], + "deploy-identified-problem", + false, + |_, _| false, + |vid, pid| (vid, pid) == (0x2e8a, 0x000a), + ), + None + ); + let mut second_historical = historical.clone(); + second_historical.port = "COM19".to_string(); + second_historical.serial_number = Some("SECOND-RP".to_string()); + second_historical.instance_id = Some("USB\\VID_2E8A&PID_000A\\SECOND-RP".to_string()); + assert_eq!( + compose_rp2040_recovery_request( + &[historical.clone(), second_historical], + &[unidentified_code43(RP_PHYSICAL_LOCATION)], + "deploy-ambiguous-history", + false, + |_, _| false, + |vid, pid| (vid, pid) == (0x2e8a, 0x000a), + ), + None + ); + let mut second_problem = unidentified_code43(RP_PHYSICAL_LOCATION); + second_problem.instance_id = "USB\\VID_0000&PID_0002\\6&3AF0F9CE&0&5".to_string(); + assert_eq!( + compose_rp2040_recovery_request( + &[historical], + &[unidentified_code43(RP_PHYSICAL_LOCATION), second_problem,], + "deploy-ambiguous", + false, + |_, _| false, + |vid, pid| (vid, pid) == (0x2e8a, 0x000a), + ), + None + ); + } + #[test] fn bootsel_problem_interface_is_preferred_over_phantom_cdc() { let request = compose_rp2040_recovery_request( diff --git a/crates/fbuild-deploy/src/rp2040.rs b/crates/fbuild-deploy/src/rp2040.rs index b578a90d..853a35bc 100644 --- a/crates/fbuild-deploy/src/rp2040.rs +++ b/crates/fbuild-deploy/src/rp2040.rs @@ -2,6 +2,7 @@ use std::collections::{BTreeSet, HashMap}; use std::fs; +use std::future::Future; use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::sync::{Mutex, OnceLock}; @@ -190,6 +191,104 @@ fn picotool_identity_required_error() -> FbuildError { ) } +/// Decide whether the Pico SDK USB reset interface may be used as the middle +/// recovery layer between the CDC touch and ROM transports. A serial number +/// plus the selected endpoint's observed VID/PID are mandatory: without all +/// three facts, a reset request could act on the wrong attached RP board. +fn application_reboot_target( + bootsel_present: bool, + runtime_target: Option<&target::RequestedRuntimeTarget>, +) -> Option { + if bootsel_present { + return None; + } + let runtime_target = runtime_target?; + let serial = runtime_target.serial_number.as_deref()?; + if serial.is_empty() || runtime_target.vendor_id == 0 || runtime_target.product_id == 0 { + return None; + } + Some(picotool::PicotoolTarget::new( + serial, + &format!("{:04x}", runtime_target.vendor_id), + &format!("{:04x}", runtime_target.product_id), + )) +} + +struct ApplicationRebootRecovery { + volume: Option, + volume_discovery_error: Option, + application_reboot_succeeded: bool, +} + +/// Run the application-mode recovery transition behind injectable boundaries +/// so its ordering and failure semantics are covered without invoking a real +/// USB device from unit tests. The production closures remain the managed +/// reset-interface transport and the normal BOOTSEL watcher. +async fn run_application_reboot_recovery( + target: Option, + volume: Option, + mut volume_discovery_error: Option, + bootloader_timeout: Duration, + reboot: Reboot, + discover_bootsel: Discover, +) -> Result +where + Reboot: FnOnce(picotool::PicotoolTarget) -> RebootFuture, + RebootFuture: Future>, + Discover: FnOnce() -> DiscoverFuture, + DiscoverFuture: Future>>, +{ + let Some(target) = target else { + return Ok(ApplicationRebootRecovery { + volume, + volume_discovery_error, + application_reboot_succeeded: false, + }); + }; + + let earlier_failure = volume_discovery_error + .take() + .map(|error| error.to_string()) + .unwrap_or_else(|| "initial BOOTSEL discovery found no target".to_string()); + tracing::warn!( + cdc_or_bootsel_failure = %earlier_failure, + "RP-series CDC/BOOTSEL transition failed; trying target-bound application reset-interface reboot" + ); + + match reboot(target).await { + Ok(_) => { + tracing::info!( + "RP-series recovery layer succeeded: target-bound application reset-interface reboot" + ); + let discovered = discover_bootsel().await.map_err(|error| { + FbuildError::DeployFailed(format!( + "{earlier_failure}; target-bound application reset-interface reboot succeeded, but BOOTSEL rediscovery failed: {error}" + )) + })?; + let volume_discovery_error = if discovered.is_some() { + None + } else { + Some(FbuildError::DeployFailed(format!( + "{earlier_failure}; target-bound application reset-interface reboot succeeded, but no RP2040 BOOTSEL volume mounted within {}s", + bootloader_timeout.as_secs() + ))) + }; + Ok(ApplicationRebootRecovery { + volume: discovered, + volume_discovery_error, + application_reboot_succeeded: true, + }) + } + Err(error) => Ok(ApplicationRebootRecovery { + volume, + volume_discovery_error: Some(FbuildError::DeployFailed(format!( + "{earlier_failure}; target-bound application reset-interface reboot also failed: {error}" + ))), + application_reboot_succeeded: false, + }), + } +} + /// 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. @@ -1286,11 +1385,15 @@ impl Rp2040Deployer { struct PicoCdcPort { name: String, serial_number: Option, + vendor_id: u16, + product_id: u16, health: fbuild_serial::ports::PortHealth, instance_id: Option, parent_instance_id: Option, } +type PicoResetInterface = fbuild_serial::ports::UsbResetInterface; + fn catalogue_pico_cdc_ports(expected_family: u32) -> Result> { let ports = fbuild_serial::ports::available_ports().map_err(|error| { FbuildError::SerialError(format!( @@ -1309,6 +1412,8 @@ fn catalogue_pico_cdc_ports(expected_family: u32) -> Result> { matches_family.then(|| PicoCdcPort { name: port.info.port_name, serial_number: usb.serial_number.clone(), + vendor_id: usb.vid, + product_id: usb.pid, health: port.health, instance_id: port.instance_id, parent_instance_id: port.parent_instance_id, @@ -1319,6 +1424,99 @@ fn catalogue_pico_cdc_ports(expected_family: u32) -> Result> { Ok(matches) } +fn catalogue_pico_reset_interfaces(expected_family: u32) -> Vec { + let mut matches: Vec<_> = fbuild_serial::ports::present_usb_reset_interfaces() + .into_iter() + .filter(|interface| { + fbuild_core::usb::profiles::profiles_for(interface.vid, interface.pid) + .iter() + .any(|profile| profile_matches_family(profile, expected_family)) + }) + .collect(); + matches.sort_by(|left, right| left.instance_id.cmp(&right.instance_id)); + matches +} + +/// Resolve an exact healthy application reset interface when the selected CDC +/// endpoint itself is not usable. A serial selector may recover even after the +/// COM devnode disappears; a COM selector must still match a retained, +/// catalogue-identified unhealthy record before its serial can be trusted. +fn resolve_reset_only_target( + selector: &str, + cdc_candidates: &[PicoCdcPort], + reset_interfaces: &[PicoResetInterface], +) -> Result> { + let (serial, expected_vid_pid) = if let Some(serial) = serial_selector(selector) { + (serial, None) + } else { + let matching_cdc: Vec<_> = cdc_candidates + .iter() + .filter(|candidate| candidate.name == selector) + .collect(); + let candidate = match matching_cdc.as_slice() { + [] => return Ok(None), + [only] if only.health.is_known_unhealthy() => *only, + [only] => { + return Err(FbuildError::DeployFailed(format!( + "RP2040 runtime selector {selector:?} matched healthy {}, but normal CDC target resolution failed", + only.name + ))); + } + many => { + return Err(FbuildError::DeployFailed(format!( + "RP2040 runtime selector {selector:?} is ambiguous across {} CDC records", + many.len() + ))); + } + }; + let Some(serial) = candidate.serial_number.as_deref() else { + return Ok(None); + }; + (serial, Some((candidate.vendor_id, candidate.product_id))) + }; + + let matching_reset: Vec<_> = reset_interfaces + .iter() + .filter(|interface| interface.serial_number.eq_ignore_ascii_case(serial)) + .filter(|interface| { + expected_vid_pid.map_or(true, |(vid, pid)| { + (interface.vid, interface.pid) == (vid, pid) + }) + }) + .collect(); + match matching_reset.as_slice() { + [] => Ok(None), + [only] => Ok(Some((*only).clone())), + many => Err(FbuildError::DeployFailed(format!( + "RP2040 reset-interface selector {selector:?} is ambiguous across {} exact serial/VID/PID matches; fbuild refuses an unscoped reset", + many.len() + ))), + } +} + +fn reset_interface_for_runtime_target( + target: &target::RequestedRuntimeTarget, + reset_interfaces: &[PicoResetInterface], +) -> Result> { + let Some(serial) = target.serial_number.as_deref() else { + return Ok(None); + }; + let matches: Vec<_> = reset_interfaces + .iter() + .filter(|interface| interface.serial_number.eq_ignore_ascii_case(serial)) + .filter(|interface| (interface.vid, interface.pid) == (target.vendor_id, target.product_id)) + .collect(); + match matches.as_slice() { + [] => Ok(None), + [only] => Ok(Some((*only).clone())), + many => Err(FbuildError::DeployFailed(format!( + "RP2040 runtime target {} is ambiguous across {} exact reset-interface matches; fbuild refuses an unscoped reset", + target.port, + many.len() + ))), + } +} + /// CDC-wait failure classification: a quiet window (`Timeout`) is /// recoverable once the flash itself is confirmed, while an enumeration or /// selection error (`Enumeration`) always fails the deploy. @@ -1694,17 +1892,62 @@ impl Deployer for Rp2040Deployer { pretouch_volume_policy(mounted, can_attribute)? } }; - let requested_serial = selector.and_then(serial_selector).map(str::to_string); - let runtime_target = if volume_before_reset.is_none() { - selector + let reset_interfaces = if volume_before_reset.is_none() { + let family_id = self.family_id; + tokio::task::spawn_blocking(move || catalogue_pico_reset_interfaces(family_id)) + .await + .map_err(|error| { + FbuildError::DeployFailed(format!( + "RP2040 reset-interface snapshot task failed: {error}" + )) + })? + } else { + Vec::new() + }; + let (runtime_target, reset_only_target) = if volume_before_reset.is_none() { + match selector .map(|value| resolve_requested_runtime_target(value, ¤t_ports)) - .transpose()? + .transpose() + { + Ok(target) => (target, None), + Err(cdc_error) => { + let reset_target = selector + .map(|value| { + resolve_reset_only_target(value, ¤t_ports, &reset_interfaces) + }) + .transpose()? + .flatten(); + let Some(reset_target) = reset_target else { + return Err(cdc_error); + }; + tracing::warn!( + selector, + serial = %reset_target.serial_number, + instance_id = %reset_target.instance_id, + cdc_failure = %cdc_error, + "selected RP-series CDC endpoint is unusable; using its exact healthy application reset interface" + ); + (None, Some(reset_target)) + } + } } else { - None + (None, None) }; - let picotool_target = runtime_target - .as_ref() - .and_then(|target| target.serial_number.as_deref()) + let requested_serial = selector + .and_then(serial_selector) + .map(str::to_string) + .or_else(|| { + runtime_target + .as_ref() + .and_then(|target| target.serial_number.clone()) + }) + .or_else(|| { + reset_only_target + .as_ref() + .map(|target| target.serial_number.clone()) + }); + let picotool_target = requested_serial + .as_deref() .map(|serial| picotool_target_for_family(serial, self.family_id)) .transpose()?; // Capture topology before the 1200-bps touch: once the board resets @@ -1748,6 +1991,80 @@ impl Deployer for Rp2040Deployer { Err(error) => (None, Some(error)), } }; + let application_target = if volume.is_some() { + None + } else if let Some(target) = reset_only_target.as_ref() { + Some(picotool::PicotoolTarget::new( + &target.serial_number, + &format!("{:04x}", target.vid), + &format!("{:04x}", target.pid), + )) + } else { + application_reboot_target(false, runtime_target.as_ref()) + }; + let native_reset_target = if cfg!(windows) && volume.is_none() { + if let Some(target) = reset_only_target.clone() { + Some(target) + } else if let Some(target) = runtime_target.as_ref() { + reset_interface_for_runtime_target(target, &reset_interfaces)? + } else { + None + } + } else { + None + }; + let timeout = self.bootloader_timeout; + let stale_volumes = volumes_before.clone(); + let application_recovery = run_application_reboot_recovery( + application_target, + volume, + volume_discovery_error, + self.bootloader_timeout, + move |target| async move { + if let Some(reset_interface) = native_reset_target { + let instance_id = reset_interface.instance_id.clone(); + tokio::task::spawn_blocking(move || { + fbuild_serial::ports::reset_usb_interface_to_bootsel(&reset_interface) + }) + .await + .map_err(|error| { + FbuildError::DeployFailed(format!( + "native Pico reset-interface task failed: {error}" + )) + })? + .map_err(|error| { + FbuildError::DeployFailed(format!( + "native Pico reset-interface request for {instance_id} failed: {error}" + )) + })?; + return Ok(picotool::PicotoolLoad { + stdout: format!( + "requested BOOTSEL through native Pico reset interface {instance_id}" + ), + stderr: String::new(), + }); + } + picotool::reboot_runtime_to_bootsel( + project_dir, + &target, + PICOTOOL_INFO_PROBE_TIMEOUT, + ) + .await + }, + move || async move { + tokio::task::spawn_blocking(move || find_uf2_volume_until(timeout, &stale_volumes)) + .await + .map_err(|error| { + FbuildError::DeployFailed(format!( + "RP2040 post-picotool volume watcher failed: {error}" + )) + })? + }, + ) + .await?; + let volume = application_recovery.volume; + let volume_discovery_error = application_recovery.volume_discovery_error; + let application_reboot_succeeded = application_recovery.application_reboot_succeeded; let firmware = firmware_path.to_path_buf(); let family_id = self.family_id; let (artifact, uf2_target) = @@ -1938,6 +2255,11 @@ impl Deployer for Rp2040Deployer { } } }; + let transfer_method = if application_reboot_succeeded { + format!("{transfer_method} after target-bound application reset-interface reboot") + } else { + transfer_method.to_string() + }; if let Some(volume_for_wait) = transfer_volume.clone() { let post_timeout = self.post_deploy_timeout; let eject_result = tokio::task::spawn_blocking(move || { @@ -1962,7 +2284,7 @@ impl Deployer for Rp2040Deployer { if uf2_target == Uf2Target::Ram { return Ok(ram_load_result( transfer_volume.as_deref(), - transfer_method, + &transfer_method, transfer_stdout, transfer_stderr, )); @@ -2763,12 +3085,250 @@ mod tests { PicoCdcPort { name: name.to_string(), serial_number: serial.map(str::to_string), + vendor_id: 0x2e8a, + product_id: 0xf00f, health, instance_id: Some(format!("USB\\VID_2E8A&PID_000A\\{name}")), parent_instance_id: None, } } + fn reset_candidate(instance: &str, serial: &str) -> PicoResetInterface { + PicoResetInterface { + instance_id: instance.to_string(), + parent_instance_id: format!("USB\\VID_2E8A&PID_F00F\\{serial}"), + serial_number: serial.to_string(), + vid: 0x2e8a, + pid: 0xf00f, + device_path: format!("\\\\?\\{instance}"), + interface_number: 2, + location_paths: vec!["PCIROOT(0)#USBROOT(0)#USB(1)#USBMI(2)".to_string()], + } + } + + #[test] + fn unhealthy_cdc_can_select_its_exact_live_reset_interface() { + let cdc = cdc_candidate( + "COM18", + Some("2DCB876B587EA334"), + fbuild_serial::ports::PortHealth::PresentProblem { + problem_code: 31, + status: Some(0), + }, + ); + let reset = reset_candidate( + "USB\\VID_2E8A&PID_F00F&MI_02\\8&20C14328&0&0002", + "2DCB876B587EA334", + ); + + let selected = resolve_reset_only_target("COM18", &[cdc], std::slice::from_ref(&reset)) + .unwrap() + .expect("the exact live reset interface must recover the stale COM selector"); + + assert_eq!(selected, reset); + } + + #[test] + fn serial_selector_can_recover_after_the_cdc_devnode_disappears() { + let reset = reset_candidate( + "USB\\VID_2E8A&PID_F00F&MI_02\\8&20C14328&0&0002", + "2DCB876B587EA334", + ); + + let selected = + resolve_reset_only_target("SER=2DCB876B587EA334", &[], std::slice::from_ref(&reset)) + .unwrap(); + + assert_eq!(selected, Some(reset)); + } + + #[test] + fn reset_only_target_fails_closed_on_wrong_or_multiple_devices() { + let first = reset_candidate("USB\\VID_2E8A&PID_F00F&MI_02\\FIRST", "2DCB876B587EA334"); + let wrong = reset_candidate("USB\\VID_2E8A&PID_F00F&MI_02\\WRONG", "OTHER"); + assert_eq!( + resolve_reset_only_target("SER=2DCB876B587EA334", &[], std::slice::from_ref(&wrong)) + .unwrap(), + None + ); + + let duplicate = reset_candidate("USB\\VID_2E8A&PID_F00F&MI_02\\SECOND", "2DCB876B587EA334"); + let error = resolve_reset_only_target("SER=2DCB876B587EA334", &[], &[first, duplicate]) + .unwrap_err(); + assert!(error.to_string().contains("ambiguous")); + assert!(error.to_string().contains("refuses an unscoped reset")); + } + + #[test] + fn runtime_target_maps_to_one_exact_native_reset_interface() { + let runtime = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + let reset = reset_candidate( + "USB\\VID_2E8A&PID_F00F&MI_02\\8&20C14328&0&0002", + "2DCB876B587EA334", + ); + assert_eq!( + reset_interface_for_runtime_target(&runtime, std::slice::from_ref(&reset)).unwrap(), + Some(reset) + ); + } + + #[test] + fn failed_cdc_without_bootsel_plans_target_bound_application_reboot() { + let runtime = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + + assert!(application_reboot_target(false, Some(&runtime)).is_some()); + } + + #[test] + fn application_reboot_is_never_forced_without_exact_identity() { + let no_serial = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: None, + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + + assert!(application_reboot_target(false, Some(&no_serial)).is_none()); + let zero_vid = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0, + product_id: 0xf00f, + }; + assert!(application_reboot_target(false, Some(&zero_vid)).is_none()); + assert!(application_reboot_target(false, None).is_none()); + } + + #[test] + fn already_visible_bootsel_skips_application_reboot() { + let runtime = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + + assert!(application_reboot_target(true, Some(&runtime)).is_none()); + } + + #[tokio::test] + async fn failed_cdc_without_bootsel_reboots_and_reacquires_bootsel() { + let runtime = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + let expected_volume = PathBuf::from("RPI-RP2"); + + let outcome = run_application_reboot_recovery( + application_reboot_target(false, Some(&runtime)), + None, + Some(FbuildError::DeployFailed( + "CDC touch failed and initial BOOTSEL discovery timed out".to_string(), + )), + Duration::from_secs(10), + |_target| async { + Ok(picotool::PicotoolLoad { + stdout: "rebooted".to_string(), + stderr: String::new(), + }) + }, + || async { Ok(Some(PathBuf::from("RPI-RP2"))) }, + ) + .await + .unwrap(); + + assert_eq!(outcome.volume, Some(expected_volume)); + assert!(outcome.volume_discovery_error.is_none()); + assert!(outcome.application_reboot_succeeded); + } + + #[tokio::test] + async fn bootsel_rediscovery_error_preserves_layered_failure() { + let runtime = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + + let result = run_application_reboot_recovery( + application_reboot_target(false, Some(&runtime)), + None, + Some(FbuildError::DeployFailed( + "CDC touch failed and initial BOOTSEL discovery timed out".to_string(), + )), + Duration::from_secs(10), + |_target| async { + Ok(picotool::PicotoolLoad { + stdout: "rebooted".to_string(), + stderr: String::new(), + }) + }, + || async { + Err(FbuildError::DeployFailed( + "volume enumeration failed".to_string(), + )) + }, + ) + .await; + let error = match result { + Err(error) => error.to_string(), + Ok(_) => panic!("BOOTSEL rediscovery failure must fail the recovery layer"), + }; + + assert!(error.contains("CDC touch failed and initial BOOTSEL discovery timed out")); + assert!(error.contains("reset-interface reboot succeeded")); + assert!(error.contains("volume enumeration failed")); + } + + #[tokio::test] + async fn application_picotool_timeout_preserves_layered_failure() { + let runtime = target::RequestedRuntimeTarget { + port: "COM18".to_string(), + serial_number: Some("2DCB876B587EA334".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, + }; + + let outcome = run_application_reboot_recovery( + application_reboot_target(false, Some(&runtime)), + None, + Some(FbuildError::DeployFailed( + "initial BOOTSEL discovery timed out".to_string(), + )), + Duration::from_secs(10), + |_target| async { + Err(FbuildError::DeployFailed( + "managed picotool timed out after 2s".to_string(), + )) + }, + || async { panic!("BOOTSEL discovery must not run after a failed application reboot") }, + ) + .await + .unwrap(); + + assert!(outcome.volume.is_none()); + assert!(!outcome.application_reboot_succeeded); + let error = outcome + .volume_discovery_error + .expect("the layered recovery failure must remain actionable") + .to_string(); + assert!(error.contains("initial BOOTSEL discovery timed out")); + assert!(error.contains("managed picotool timed out after 2s")); + } + #[test] fn cdc_timeout_without_flash_confirmation_is_an_actionable_failure() { let wait = wait_for_cdc_port_with_clock( @@ -2929,6 +3489,8 @@ mod tests { candidates: vec![PicoCdcPort { name: "COM27".to_string(), serial_number: Some("5303284720C4641C".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, @@ -2993,6 +3555,8 @@ mod tests { vec![PicoCdcPort { name: "COM27".to_string(), serial_number: Some("5303284720C4641C".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, @@ -3023,6 +3587,8 @@ mod tests { Ok(vec![PicoCdcPort { name: "COM27".to_string(), serial_number: Some("5303284720C4641C".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, @@ -3055,6 +3621,8 @@ mod tests { PicoCdcPort { name: "COM12".to_string(), serial_number: None, + vendor_id: 0x2e8a, + product_id: 0xf00f, health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, @@ -3062,6 +3630,8 @@ mod tests { PicoCdcPort { name: "COM13".to_string(), serial_number: None, + vendor_id: 0x2e8a, + product_id: 0xf00f, health: fbuild_serial::ports::PortHealth::Unknown, instance_id: None, parent_instance_id: None, diff --git a/crates/fbuild-deploy/src/rp2040_picotool.rs b/crates/fbuild-deploy/src/rp2040_picotool.rs index 7878cd2e..af51fbd6 100644 --- a/crates/fbuild-deploy/src/rp2040_picotool.rs +++ b/crates/fbuild-deploy/src/rp2040_picotool.rs @@ -95,6 +95,35 @@ pub(super) async fn probe_picotool_info( Ok(()) } +/// Ask a cooperative runtime application to enter USB BOOTSEL. Unlike a +/// 1200-bps touch this uses the Pico SDK reset interface, so it still works +/// when the selected board's CDC endpoint cannot be opened. The caller must +/// supply the exact runtime VID/PID and USB serial. Windows callers use the +/// native WinUSB reset-interface path when it can be resolved exactly; this +/// libusb fallback remains target-filtered for other hosts. +pub(super) async fn reboot_runtime_to_bootsel( + 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 = reboot_to_bootsel_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() { + return Err(FbuildError::DeployFailed(format!( + "managed picotool application-mode reboot error: {}", + combined_tool_output(output.stdout.trim(), output.stderr.trim()) + ))); + } + Ok(PicotoolLoad { + stdout: output.stdout, + stderr: output.stderr, + }) +} + /// Ask the already-installed managed picotool for its most recent UF2 /// diagnostic. This must never trigger a package download; it shares the /// caller's bounded subprocess timeout (FastLED/fbuild#1245). @@ -165,13 +194,16 @@ fn combined_tool_output(stdout: &str, stderr: &str) -> String { } fn append_target_selection(args: &mut Vec, target: &PicotoolTarget) { + append_target_vid_pid(args, target); + args.extend(["--ser".to_string(), target.serial_number.to_string()]); +} + +fn append_target_vid_pid(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(), ]); } @@ -186,6 +218,27 @@ fn load_args(executable: &Path, artifact: &Path, target: &PicotoolTarget) -> Vec args } +fn reboot_to_bootsel_args(executable: &Path, target: &PicotoolTarget) -> Vec { + let mut args = vec![ + executable.to_string_lossy().to_string(), + "reboot".to_string(), + "-u".to_string(), + ]; + // pico-quick-toolchain's pinned picotool uses an order-sensitive + // CLIPP grammar: reboot-type options precede device selectors, and `-f` + // is the final option in the selector group. Keep the application-mode + // VID/PID selectors. Do not pass + // `--ser` here: current picotool applies it while opening the application + // device, where Arduino-Pico's reset function does not expose the BOOTSEL + // serial. When omitted, picotool reads the application device descriptor + // and tracks that serial automatically across the reboot. fbuild has + // already resolved one exact healthy reset interface, and picotool itself + // refuses a forced command if the VID/PID matches multiple devices. + append_target_vid_pid(&mut args, target); + args.push("-f".to_string()); + args +} + 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); @@ -292,6 +345,25 @@ mod tests { ); } + #[test] + fn forced_application_reboot_is_bound_to_the_runtime_target() { + let target = PicotoolTarget::new("2DCB876B587EA334", "2e8a", "f00f"); + let args = reboot_to_bootsel_args(Path::new("managed/picotool"), &target); + assert_eq!( + args, + [ + "managed/picotool", + "reboot", + "-u", + "--vid", + "0x2e8a", + "--pid", + "0xf00f", + "-f", + ] + ); + } + #[test] fn info_probe_uses_the_same_bootsel_target() { let args = info_probe_args(Path::new("managed/picotool"), &rp2350_target()); diff --git a/crates/fbuild-deploy/src/rp2040_preflight.rs b/crates/fbuild-deploy/src/rp2040_preflight.rs index 47fad1ac..7e0ae23d 100644 --- a/crates/fbuild-deploy/src/rp2040_preflight.rs +++ b/crates/fbuild-deploy/src/rp2040_preflight.rs @@ -134,6 +134,7 @@ mod tests { behind_external_hub: Some(false), parent_instance_id: Some(BOOTSEL_COMPOSITE.to_string()), device_class: None, + location_paths: Vec::new(), } } diff --git a/crates/fbuild-deploy/src/rp2040_target.rs b/crates/fbuild-deploy/src/rp2040_target.rs index 1ff55bc7..45b91477 100644 --- a/crates/fbuild-deploy/src/rp2040_target.rs +++ b/crates/fbuild-deploy/src/rp2040_target.rs @@ -10,6 +10,8 @@ use super::PicoCdcPort; pub(super) struct RequestedRuntimeTarget { pub(super) port: String, pub(super) serial_number: Option, + pub(super) vendor_id: u16, + pub(super) product_id: u16, } pub(super) fn serial_selector(selector: &str) -> Option<&str> { @@ -67,6 +69,8 @@ pub(super) fn resolve_requested_runtime_target( [only] => Ok(RequestedRuntimeTarget { port: only.name.clone(), serial_number: only.serial_number.clone(), + vendor_id: only.vendor_id, + product_id: only.product_id, }), [] => Err(FbuildError::DeployFailed(format!( "RP2040 runtime selector {selector:?} did not match a catalogue-identified CDC port" @@ -151,6 +155,8 @@ mod tests { PicoCdcPort { name: name.to_string(), serial_number: serial_number.map(str::to_string), + vendor_id: 0x2e8a, + product_id: 0xf00f, health, instance_id: Some(format!("USB\\VID_2E8A&PID_000A\\{name}")), parent_instance_id: None, @@ -186,10 +192,23 @@ mod tests { RequestedRuntimeTarget { port: "COM13".to_string(), serial_number: Some("PICO-2".to_string()), + vendor_id: 0x2e8a, + product_id: 0xf00f, } ); } + #[test] + fn selected_runtime_target_retains_exact_picotool_identity() { + let target = + resolve_requested_runtime_target("SER=PICO-2", &[cdc("COM13", Some("PICO-2"))]) + .unwrap(); + + assert_eq!(target.vendor_id, 0x2e8a); + assert_eq!(target.product_id, 0xf00f); + assert_eq!(target.serial_number.as_deref(), Some("PICO-2")); + } + #[test] fn multiple_new_cdc_ports_are_rejected() { let error = select_cdc_candidate( diff --git a/crates/fbuild-serial/Cargo.toml b/crates/fbuild-serial/Cargo.toml index 67edf9bd..cbba3de5 100644 --- a/crates/fbuild-serial/Cargo.toml +++ b/crates/fbuild-serial/Cargo.toml @@ -33,7 +33,11 @@ regex = { workspace = true } windows-sys = { version = "0.52", features = [ "Win32_Devices_DeviceAndDriverInstallation", "Win32_Devices_Properties", + "Win32_Devices_Usb", "Win32_Foundation", + "Win32_Security", + "Win32_Storage_FileSystem", + "Win32_System_IO", "Win32_System_Registry", ] } diff --git a/crates/fbuild-serial/src/ports.rs b/crates/fbuild-serial/src/ports.rs index 98b1d3ff..55951990 100644 --- a/crates/fbuild-serial/src/ports.rs +++ b/crates/fbuild-serial/src/ports.rs @@ -104,6 +104,9 @@ pub struct DetectedPort { /// hub — anything reasoning about hub-level policy (power management, /// topology) needs the whole chain, not just one hop. pub ancestor_instance_ids: Vec, + /// Windows physical USB location paths. Empty when unavailable. These are + /// identity history only and never make a phantom endpoint selectable. + pub location_paths: Vec, } impl DetectedPort { @@ -114,6 +117,7 @@ impl DetectedPort { instance_id: None, parent_instance_id: None, ancestor_instance_ids: Vec::new(), + location_paths: Vec::new(), } } } @@ -238,6 +242,71 @@ pub struct UsbProblemDevice { /// Windows device class (e.g. `Ports`, `USB`); `None` for driverless /// devnodes that never got a class assigned. pub device_class: Option, + /// Windows physical USB location paths for exact device-local + /// correlation. Human-readable `location` is not stable enough for this. + pub location_paths: Vec, +} + +/// A healthy, present Pico SDK application-mode USB reset interface. +/// +/// Arduino-Pico exposes this WinUSB function when `ENABLE_PICOTOOL_USB` is +/// enabled. It remains independently addressable when the sibling CDC +/// interface is missing or unusable, which lets the RP deployer recover the +/// exact application device without opening a stale COM endpoint. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct UsbResetInterface { + pub instance_id: String, + pub parent_instance_id: String, + pub vid: u16, + pub pid: u16, + pub serial_number: String, + /// WinUSB device-interface path for the fixed Pico SDK reset GUID. + pub device_path: String, + /// USB interface number carried by the composite `MI_xx` devnode. + pub interface_number: u8, + pub location_paths: Vec, +} + +fn is_picotool_reset_compatible_id(value: &str) -> bool { + value.eq_ignore_ascii_case("USB\\Class_ff&SubClass_00&Prot_01") +} + +/// Best-effort enumeration of healthy Pico SDK application reset interfaces. +/// +/// Windows exposes the function as the standard Raspberry Pi reset-interface +/// compatible ID. Other hosts currently return an empty list; their normal +/// libusb/picotool path remains unchanged. +pub fn present_usb_reset_interfaces() -> Vec { + #[cfg(windows)] + { + imp::present_usb_reset_interfaces() + } + #[cfg(not(windows))] + { + Vec::new() + } +} + +/// Ask one exact Pico SDK WinUSB reset interface to enter BOOTSEL mode. +/// +/// The interface must come from [`present_usb_reset_interfaces`], which binds +/// the live device path to its USB serial and VID/PID before this request is +/// issued. The board may disconnect before Windows reports completion; that +/// is the normal successful shape of the no-data control transfer, so the +/// deployer confirms success by waiting for the target BOOTSEL transport. +pub fn reset_usb_interface_to_bootsel(interface: &UsbResetInterface) -> std::io::Result<()> { + #[cfg(windows)] + { + imp::reset_usb_interface_to_bootsel(interface) + } + #[cfg(not(windows))] + { + let _ = interface; + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "the native Pico reset interface is currently implemented only on Windows", + )) + } } /// Best-effort enumeration of present USB devnodes with a non-zero Windows @@ -383,32 +452,70 @@ mod health_tests { .is_known_unhealthy() ); } + + #[test] + fn recognizes_only_the_pico_sdk_reset_interface_protocol() { + assert!(is_picotool_reset_compatible_id( + "usb\\class_FF&subclass_00&prot_01" + )); + assert!(!is_picotool_reset_compatible_id( + "USB\\Class_02&SubClass_02&Prot_01" + )); + assert!(!is_picotool_reset_compatible_id( + "USB\\Class_ff&SubClass_00" + )); + } } #[cfg(windows)] mod imp { - use super::{DetectedPort, PnpObservation, UsbProblemDevice, health_for_endpoint}; - use std::collections::HashSet; + use super::{ + DetectedPort, PnpObservation, UsbProblemDevice, UsbResetInterface, health_for_endpoint, + is_picotool_reset_compatible_id, + }; + use std::collections::{HashMap, HashSet}; + use std::io; use std::ptr; use serialport::{SerialPortInfo, SerialPortType, UsbPortInfo}; use windows_sys::Win32::Devices::DeviceAndDriverInstallation::{ CM_Get_DevNode_Status, CM_Get_Device_IDW, CM_Get_Parent, CR_NO_SUCH_DEVINST, CR_SUCCESS, - DICS_FLAG_GLOBAL, DIGCF_ALLCLASSES, DIGCF_PRESENT, DIREG_DEV, HDEVINFO, MAX_DEVICE_ID_LEN, - SP_DEVINFO_DATA, SPDRP_CLASS, SPDRP_FRIENDLYNAME, SPDRP_HARDWAREID, + DICS_FLAG_GLOBAL, DIGCF_ALLCLASSES, DIGCF_DEVICEINTERFACE, DIGCF_PRESENT, DIREG_DEV, + HDEVINFO, MAX_DEVICE_ID_LEN, SP_DEVICE_INTERFACE_DATA, SP_DEVICE_INTERFACE_DETAIL_DATA_W, + SP_DEVINFO_DATA, SPDRP_CLASS, SPDRP_COMPATIBLEIDS, SPDRP_FRIENDLYNAME, SPDRP_HARDWAREID, SPDRP_LOCATION_INFORMATION, SPDRP_MFG, SetupDiClassGuidsFromNameW, - SetupDiDestroyDeviceInfoList, SetupDiEnumDeviceInfo, SetupDiGetClassDevsW, - SetupDiGetDeviceInstanceIdW, SetupDiGetDeviceRegistryPropertyW, SetupDiOpenDevRegKey, + SetupDiDestroyDeviceInfoList, SetupDiEnumDeviceInfo, SetupDiEnumDeviceInterfaces, + SetupDiGetClassDevsW, SetupDiGetDeviceInstanceIdW, SetupDiGetDeviceInterfaceDetailW, + SetupDiGetDevicePropertyW, SetupDiGetDeviceRegistryPropertyW, SetupDiOpenDevRegKey, + }; + use windows_sys::Win32::Devices::Properties::{ + DEVPKEY_Device_LocationPaths, DEVPROP_TYPE_STRING_LIST, + }; + use windows_sys::Win32::Devices::Usb::{ + WINUSB_INTERFACE_HANDLE, WINUSB_SETUP_PACKET, WinUsb_ControlTransfer, WinUsb_Free, + WinUsb_Initialize, + }; + use windows_sys::Win32::Foundation::{ + CloseHandle, FALSE, FILETIME, GENERIC_READ, GENERIC_WRITE, INVALID_HANDLE_VALUE, MAX_PATH, + }; + use windows_sys::Win32::Storage::FileSystem::{ + CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_FLAG_OVERLAPPED, FILE_SHARE_READ, + FILE_SHARE_WRITE, OPEN_EXISTING, }; - use windows_sys::Win32::Foundation::{FALSE, FILETIME, INVALID_HANDLE_VALUE, MAX_PATH}; use windows_sys::Win32::System::Registry::{ - HKEY, HKEY_LOCAL_MACHINE, KEY_READ, REG_SZ, RegCloseKey, RegEnumValueW, RegOpenKeyExW, - RegQueryInfoKeyW, RegQueryValueExW, + HKEY, HKEY_LOCAL_MACHINE, KEY_READ, REG_MULTI_SZ, REG_SZ, RegCloseKey, RegEnumValueW, + RegOpenKeyExW, RegQueryInfoKeyW, RegQueryValueExW, }; use windows_sys::core::GUID; const CONNECTOR_PUNCTUATION_SELECTION: &[char] = &[':', '_', '\u{ff3f}']; + const PICO_RESET_INTERFACE_GUID: GUID = GUID::from_u128(0xbc7398c1_73cd_4cb7_98b8_913a8fca7bf6); + const RESET_REQUEST_BOOTSEL: u8 = 0x01; + // USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE. This exactly + // matches picotool's reset-interface request; the endpoint is vendor + // class, but the control request itself is class-scoped. + const RESET_REQUEST_TYPE: u8 = 0x21; fn as_utf16(utf8: &str) -> Vec { utf8.encode_utf16().chain(Some(0)).collect() @@ -796,6 +903,51 @@ mod imp { })) } + fn location_paths_from_info(hdi: HDEVINFO, info: &SP_DEVINFO_DATA) -> Vec { + let mut property_type = 0u32; + let mut required_bytes = 0u32; + // First call obtains the required byte count. SetupAPI reports + // insufficient buffer here, so the return value itself is not the + // success signal; a non-zero required size is. + unsafe { + SetupDiGetDevicePropertyW( + hdi, + info, + &DEVPKEY_Device_LocationPaths, + &mut property_type, + std::ptr::null_mut(), + 0, + &mut required_bytes, + 0, + ) + }; + if required_bytes < 2 { + return Vec::new(); + } + let mut buffer = vec![0u16; (required_bytes as usize).div_ceil(2)]; + let ok = unsafe { + SetupDiGetDevicePropertyW( + hdi, + info, + &DEVPKEY_Device_LocationPaths, + &mut property_type, + buffer.as_mut_ptr().cast(), + required_bytes, + &mut required_bytes, + 0, + ) + }; + if ok == FALSE || property_type != DEVPROP_TYPE_STRING_LIST { + return Vec::new(); + } + buffer + .split(|unit| *unit == 0) + .take_while(|segment| !segment.is_empty()) + .map(String::from_utf16_lossy) + .filter(|path| !path.is_empty()) + .collect() + } + pub(super) fn present_usb_problem_devices() -> Vec { // Enumerate by the `USB` *enumerator* with DIGCF_ALLCLASSES, not by // the USB *setup class*: a driverless devnode (e.g. a BOOTSEL @@ -853,6 +1005,7 @@ mod imp { behind_external_hub: classify_usb_ancestry(info.DevInst), parent_instance_id: ancestor_ids(info.DevInst).into_iter().next(), device_class: property_from_info(hdi, &info, SPDRP_CLASS), + location_paths: location_paths_from_info(hdi, &info), }); } unsafe { @@ -861,6 +1014,256 @@ mod imp { devices } + pub(super) fn present_usb_reset_interfaces() -> Vec { + let device_paths = pico_reset_interface_paths(); + let enumerator: Vec = "USB".encode_utf16().chain(Some(0)).collect(); + let hdi = unsafe { + SetupDiGetClassDevsW( + std::ptr::null(), + enumerator.as_ptr(), + 0, + DIGCF_PRESENT | DIGCF_ALLCLASSES, + ) + }; + if hdi == INVALID_HANDLE_VALUE { + return Vec::new(); + } + + let mut devices = Vec::new(); + let mut index = 0u32; + loop { + let mut info = SP_DEVINFO_DATA { + cbSize: std::mem::size_of::() as u32, + ClassGuid: GUID::from_u128(0), + DevInst: 0, + Reserved: 0, + }; + if unsafe { SetupDiEnumDeviceInfo(hdi, index, &mut info) } == FALSE { + break; + } + index += 1; + + let compatible_ids = string_list_property_from_info(hdi, &info, SPDRP_COMPATIBLEIDS); + if !compatible_ids + .iter() + .any(|value| is_picotool_reset_compatible_id(value)) + { + continue; + } + let mut status = 0u32; + let mut problem_code = 0u32; + if unsafe { CM_Get_DevNode_Status(&mut status, &mut problem_code, info.DevInst, 0) } + != CR_SUCCESS + || problem_code != 0 + { + continue; + } + + let Some(instance_id) = device_instance_id_from_info(hdi, &info) else { + continue; + }; + let Some(parent_instance_id) = ancestor_ids(info.DevInst).into_iter().next() else { + continue; + }; + let Some(identity) = parse_usb_port_info(&instance_id, Some(&parent_instance_id)) + else { + continue; + }; + let Some(serial_number) = identity.serial_number else { + continue; + }; + let Some(interface_number) = identity.interface else { + continue; + }; + let Some(device_path) = device_paths.get(&instance_id.to_ascii_uppercase()) else { + continue; + }; + devices.push(UsbResetInterface { + instance_id, + parent_instance_id, + vid: identity.vid, + pid: identity.pid, + serial_number, + device_path: device_path.clone(), + interface_number, + location_paths: location_paths_from_info(hdi, &info), + }); + } + unsafe { + SetupDiDestroyDeviceInfoList(hdi); + } + devices.sort_by(|left, right| left.instance_id.cmp(&right.instance_id)); + devices + } + + fn pico_reset_interface_paths() -> HashMap { + let hdi = unsafe { + SetupDiGetClassDevsW( + &PICO_RESET_INTERFACE_GUID, + std::ptr::null(), + 0, + DIGCF_PRESENT | DIGCF_DEVICEINTERFACE, + ) + }; + if hdi == INVALID_HANDLE_VALUE { + return HashMap::new(); + } + + let mut paths = HashMap::new(); + let mut index = 0u32; + loop { + let mut interface = SP_DEVICE_INTERFACE_DATA { + cbSize: std::mem::size_of::() as u32, + InterfaceClassGuid: GUID::from_u128(0), + Flags: 0, + Reserved: 0, + }; + if unsafe { + SetupDiEnumDeviceInterfaces( + hdi, + std::ptr::null(), + &PICO_RESET_INTERFACE_GUID, + index, + &mut interface, + ) + } == FALSE + { + break; + } + index += 1; + + let mut required_bytes = 0u32; + unsafe { + SetupDiGetDeviceInterfaceDetailW( + hdi, + &interface, + std::ptr::null_mut(), + 0, + &mut required_bytes, + std::ptr::null_mut(), + ) + }; + if required_bytes < std::mem::size_of::() as u32 { + continue; + } + + // `Vec` provides pointer alignment suitable for the + // variable-sized SetupAPI detail record while still letting the + // API state its required byte count exactly. + let units = (required_bytes as usize).div_ceil(std::mem::size_of::()); + let mut storage = vec![0usize; units]; + let detail = storage + .as_mut_ptr() + .cast::(); + unsafe { + (*detail).cbSize = std::mem::size_of::() as u32; + } + let mut info = SP_DEVINFO_DATA { + cbSize: std::mem::size_of::() as u32, + ClassGuid: GUID::from_u128(0), + DevInst: 0, + Reserved: 0, + }; + if unsafe { + SetupDiGetDeviceInterfaceDetailW( + hdi, + &interface, + detail, + required_bytes, + &mut required_bytes, + &mut info, + ) + } == FALSE + { + continue; + } + let Some(instance_id) = device_instance_id_from_info(hdi, &info) else { + continue; + }; + let path_ptr = unsafe { std::ptr::addr_of!((*detail).DevicePath).cast::() }; + let path_offset = std::mem::offset_of!(SP_DEVICE_INTERFACE_DETAIL_DATA_W, DevicePath); + let max_units = (required_bytes as usize).saturating_sub(path_offset) / 2; + let path_units = unsafe { std::slice::from_raw_parts(path_ptr, max_units) }; + let length = path_units + .iter() + .position(|unit| *unit == 0) + .unwrap_or(path_units.len()); + if length != 0 { + paths.insert( + instance_id.to_ascii_uppercase(), + String::from_utf16_lossy(&path_units[..length]), + ); + } + } + unsafe { + SetupDiDestroyDeviceInfoList(hdi); + } + paths + } + + pub(super) fn reset_usb_interface_to_bootsel(interface: &UsbResetInterface) -> io::Result<()> { + let path = as_utf16(&interface.device_path); + let device = unsafe { + CreateFileW( + path.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + std::ptr::null(), + OPEN_EXISTING, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED, + 0, + ) + }; + if device == INVALID_HANDLE_VALUE { + return Err(io::Error::last_os_error()); + } + + let mut winusb: WINUSB_INTERFACE_HANDLE = 0; + if unsafe { WinUsb_Initialize(device, &mut winusb) } == FALSE { + let error = io::Error::last_os_error(); + unsafe { + CloseHandle(device); + } + return Err(error); + } + + let setup = WINUSB_SETUP_PACKET { + RequestType: RESET_REQUEST_TYPE, + Request: RESET_REQUEST_BOOTSEL, + Value: 0, + Index: u16::from(interface.interface_number), + Length: 0, + }; + let mut transferred = 0u32; + let transfer_ok = unsafe { + WinUsb_ControlTransfer( + winusb, + setup, + std::ptr::null_mut(), + 0, + &mut transferred, + std::ptr::null(), + ) + }; + let transfer_error = (transfer_ok == FALSE).then(io::Error::last_os_error); + unsafe { + WinUsb_Free(winusb); + CloseHandle(device); + } + if let Some(error) = transfer_error { + // The reset handler does not return. Windows can therefore report + // the expected disconnect as a failed zero-length transfer even + // though the request was accepted. The deployer performs the + // authoritative BOOTSEL wait immediately after this call. + tracing::debug!( + instance_id = %interface.instance_id, + %error, + "Pico reset interface disconnected while handling the BOOTSEL request" + ); + } + Ok(()) + } + fn device_instance_id_from_info(hdi: HDEVINFO, info: &SP_DEVINFO_DATA) -> Option { let mut buffer = [0u16; MAX_DEVICE_ID_LEN as usize]; let mut required = 0u32; @@ -912,6 +1315,50 @@ mod imp { (!value.is_empty()).then_some(value) } + fn string_list_property_from_info( + hdi: HDEVINFO, + info: &SP_DEVINFO_DATA, + property_id: u32, + ) -> Vec { + let mut value_type = 0u32; + let mut required_bytes = 0u32; + unsafe { + SetupDiGetDeviceRegistryPropertyW( + hdi, + info, + property_id, + &mut value_type, + std::ptr::null_mut(), + 0, + &mut required_bytes, + ) + }; + if required_bytes < 2 { + return Vec::new(); + } + let mut buffer = vec![0u16; (required_bytes as usize).div_ceil(2)]; + let ok = unsafe { + SetupDiGetDeviceRegistryPropertyW( + hdi, + info, + property_id, + &mut value_type, + buffer.as_mut_ptr().cast(), + required_bytes, + &mut required_bytes, + ) + }; + if ok == FALSE || value_type != REG_MULTI_SZ { + return Vec::new(); + } + buffer + .split(|unit| *unit == 0) + .take_while(|segment| !segment.is_empty()) + .map(String::from_utf16_lossy) + .filter(|value| !value.is_empty()) + .collect() + } + /// COM ports listed under `HKLM\HARDWARE\DEVICEMAP\SERIALCOMM` that the /// "Ports" class walk did not surface (parity with upstream serialport). fn get_registry_com_ports() -> HashSet { @@ -1043,6 +1490,10 @@ mod imp { instance_id, parent_instance_id, ancestor_instance_ids, + location_paths: location_paths_from_info( + port_device.hdi, + &port_device.devinfo_data, + ), }); } } diff --git a/crates/fbuild-serial/src/usb_recovery.rs b/crates/fbuild-serial/src/usb_recovery.rs index a0d1cc06..e1754b3d 100644 --- a/crates/fbuild-serial/src/usb_recovery.rs +++ b/crates/fbuild-serial/src/usb_recovery.rs @@ -9,6 +9,7 @@ use fbuild_core::usb::{ UsbRecoveryHealth, UsbRecoveryOperation, UsbRecoveryRequest, UsbRecoveryResult, + normalize_physical_location, }; /// A PnP devnode observed directly by the recovery backend. @@ -21,6 +22,7 @@ pub struct UsbPnpDevice { pub pid: u16, pub serial: Option, pub health: UsbRecoveryHealth, + pub location_paths: Vec, } /// Narrow host boundary used by the elevated helper and deterministic tests. @@ -193,6 +195,23 @@ fn validate_target_identity( return Err("serial-mismatch"); } } + if let Some(expected_location) = request.expected_location_path.as_deref() { + match (request.problem_code, &target.health) { + (Some(expected_problem_code), UsbRecoveryHealth::PresentProblem { problem_code }) + if *problem_code == expected_problem_code => {} + (Some(_), UsbRecoveryHealth::PresentProblem { .. }) => { + return Err("problem-code-mismatch"); + } + _ => return Err("location-target-not-present-problem"), + } + if !target + .location_paths + .iter() + .any(|path| normalize_physical_location(path).as_deref() == Some(expected_location)) + { + return Err("location-path-mismatch"); + } + } if let (Some(expected_problem_code), UsbRecoveryHealth::PresentProblem { problem_code }) = (request.problem_code, &target.health) { @@ -266,7 +285,10 @@ mod windows { CM_Locate_DevNodeW, CM_Reenumerate_DevNode, CR_NO_SUCH_DEVINST, CR_NO_SUCH_VALUE, CR_SUCCESS, MAX_DEVICE_ID_LEN, }; - use windows_sys::Win32::Devices::Properties::{DEVPKEY_Device_Class, DEVPROP_TYPE_STRING}; + use windows_sys::Win32::Devices::Properties::{ + DEVPKEY_Device_Class, DEVPKEY_Device_LocationPaths, DEVPROP_TYPE_STRING, + DEVPROP_TYPE_STRING_LIST, + }; /// Windows implementation is added below the common security ladder so /// tests can exercise every allowlist decision without a privileged host. @@ -317,6 +339,7 @@ mod windows { let parent_instance_id = parent_id(devinst)?; let device_class = device_class(devinst)?; let health = device_health(devinst); + let location_paths = device_location_paths(devinst); let (vid, pid, serial) = parse_usb_identity(&actual_instance_id, parent_instance_id.as_deref()).ok_or_else( || "device does not expose a canonical USB VID/PID identity".to_string(), @@ -330,6 +353,7 @@ mod windows { pid, serial, health, + location_paths, }) } @@ -465,6 +489,44 @@ mod windows { } } + fn device_location_paths(devinst: u32) -> Vec { + let mut property_type = 0u32; + let mut byte_len = 0u32; + unsafe { + CM_Get_DevNode_PropertyW( + devinst, + &DEVPKEY_Device_LocationPaths, + &mut property_type, + std::ptr::null_mut(), + &mut byte_len, + 0, + ) + }; + if byte_len < 2 { + return Vec::new(); + } + let mut buffer = vec![0u16; (byte_len as usize).div_ceil(2)]; + let result = unsafe { + CM_Get_DevNode_PropertyW( + devinst, + &DEVPKEY_Device_LocationPaths, + &mut property_type, + buffer.as_mut_ptr().cast(), + &mut byte_len, + 0, + ) + }; + if result != CR_SUCCESS || property_type != DEVPROP_TYPE_STRING_LIST { + return Vec::new(); + } + buffer + .split(|unit| *unit == 0) + .take_while(|segment| !segment.is_empty()) + .map(String::from_utf16_lossy) + .filter(|path| !path.is_empty()) + .collect() + } + fn parse_usb_identity( instance_id: &str, parent_instance_id: Option<&str>, @@ -568,11 +630,23 @@ mod tests { expected_vid: 0x2e8a, expected_pid: 0x000a, expected_serial: Some("serial".to_string()), + descriptor_failure_at_location: false, + expected_location_path: None, problem_code: Some(43), flash_completed: true, } } + fn descriptor_failure_request(location: Option<&str>) -> UsbRecoveryRequest { + let mut request = request(); + request.expected_vid = 0; + request.expected_pid = 2; + request.expected_serial = None; + request.descriptor_failure_at_location = true; + request.expected_location_path = location.map(str::to_string); + request + } + fn device(health: UsbRecoveryHealth) -> UsbPnpDevice { UsbPnpDevice { instance_id: request().instance_id, @@ -582,6 +656,7 @@ mod tests { pid: 0x000a, serial: Some("serial".to_string()), health, + location_paths: Vec::new(), } } @@ -595,6 +670,7 @@ mod tests { pid: 0x000a, serial: Some("serial".to_string()), health: UsbRecoveryHealth::HealthyPresent, + location_paths: Vec::new(), }; let mut backend = FakePnp::with_observations(vec![ device(UsbRecoveryHealth::Phantom { @@ -688,6 +764,74 @@ mod tests { } } + #[test] + fn descriptor_failure_recovery_revalidates_normalized_location_path() { + let request = descriptor_failure_request(Some("PCIROOT(0)#USBROOT(0)#USB(10)#USB(4)")); + let mut target = device(UsbRecoveryHealth::PresentProblem { problem_code: 43 }); + target.vid = 0; + target.pid = 2; + target.serial = None; + target.location_paths = vec!["pciroot(0)#usbroot(0)#usb(10)#usb(4)#usbmi(0)".to_string()]; + assert_eq!(validate_target_identity(&request, &target), Ok(())); + + target.location_paths = vec!["PCIROOT(0)#USBROOT(0)#USB(14)".to_string()]; + assert_eq!( + validate_target_identity(&request, &target), + Err("location-path-mismatch") + ); + } + + #[test] + fn descriptor_failure_that_became_phantom_never_reenumerates_parent() { + let mut request = descriptor_failure_request(Some("PCIROOT(0)#USBROOT(0)#USB(10)#USB(4)")); + request.instance_id = "USB\\VID_0000&PID_0002\\descriptor-failed".to_string(); + request.expected_class = "USB".to_string(); + + let mut target = device(UsbRecoveryHealth::Phantom { + problem_code: Some(43), + }); + target.instance_id = request.instance_id.clone(); + target.device_class = request.expected_class.clone(); + target.vid = 0; + target.pid = 2; + target.serial = None; + target.location_paths = vec!["PCIROOT(0)#USBROOT(0)#USB(10)#USB(4)".to_string()]; + let mut backend = FakePnp::with_observations(vec![target]); + + let result = execute_recovery(&request, "nonce".to_string(), &mut backend); + + assert!(!result.success); + assert_eq!( + result.error_code.as_deref(), + Some("location-target-not-present-problem") + ); + assert_eq!( + backend.calls, + vec!["inspect:USB\\VID_0000&PID_0002\\descriptor-failed:true"] + ); + } + + #[test] + fn descriptor_failure_without_location_is_rejected_before_inspection() { + let mut request = descriptor_failure_request(None); + request.instance_id = "USB\\VID_0000&PID_0002\\descriptor-failed".to_string(); + request.expected_class = "USB".to_string(); + request.problem_code = Some(43); + let mut backend = + FakePnp::with_observations(vec![device(UsbRecoveryHealth::PresentProblem { + problem_code: 43, + })]); + + let result = execute_recovery(&request, "nonce".to_string(), &mut backend); + + assert!(!result.success); + assert_eq!( + result.error_code.as_deref(), + Some("invalid-request-identity") + ); + assert!(backend.calls.is_empty()); + } + #[test] fn unhealthy_result_after_success_remains_advisory_not_a_port() { let mut backend = FakePnp::with_observations(vec![ @@ -717,6 +861,8 @@ mod tests { expected_vid: 0x2e8a, expected_pid: 0x0003, expected_serial: Some("E0C9125B0D9B".to_string()), + descriptor_failure_at_location: false, + expected_location_path: None, problem_code: Some(28), flash_completed: false, } @@ -731,6 +877,7 @@ mod tests { pid: 0x0003, serial: Some("E0C9125B0D9B".to_string()), health, + location_paths: Vec::new(), } } @@ -743,6 +890,7 @@ mod tests { pid: 0x0003, serial: Some("E0C9125B0D9B".to_string()), health, + location_paths: Vec::new(), } } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index b3efffc9..49052a14 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -85,11 +85,20 @@ Common options include `--port`, `--clean`, `--monitor`, `--timeout`, `--output-dir`. `--transport picotool|uf2` selects the RP2040/RP2350 deploy transport -(FastLED/fbuild#1162). `picotool` is the default: fbuild tries the PICOBOOT -vendor interface first (Windows preflight checks for a missing WinUSB driver -before attempting it, then a bounded `picotool info` probe, then -`picotool load -f -x`) and falls back to BOOTSEL mass-storage on any -failure. `uf2` preserves the historical mass-storage-first order with +(FastLED/fbuild#1162). Before either transport runs, fbuild tries the normal +1200-bps CDC touch. If no BOOTSEL endpoint appears and the selected runtime +port supplied an exact VID, PID, and USB serial, fbuild asks the Pico SDK +application reset interface to enter BOOTSEL, then reacquires it. Windows uses +the exact healthy WinUSB reset interface mapped to the selected CDC identity; +other hosts fall back to managed +`picotool reboot -u --vid 0x --pid 0x -f`. Picotool derives the +runtime serial from the opened application device, and refuses a forced command +when that VID/PID is ambiguous. fbuild never emits an unscoped reset request. + +`picotool` is the default transport: fbuild tries the target-bound PICOBOOT +vendor interface first (Windows preflight checks for a missing WinUSB driver, +then a bounded `picotool info` probe and `picotool load -x`) and falls back to +BOOTSEL mass-storage. `uf2` preserves the mass-storage-first order with picotool as the fallback. The picotool load timeout defaults to 60s and is configurable with `FBUILD_RP2040_PICOTOOL_TIMEOUT_SECS`. diff --git a/uv.lock b/uv.lock index 9aac6b95..a92a052b 100644 --- a/uv.lock +++ b/uv.lock @@ -4,7 +4,7 @@ requires-python = ">=3.10" [[package]] name = "fbuild" -version = "2.5.6" +version = "2.5.17" source = { editable = "." } [package.dev-dependencies]