Uh oh!
There was an error while loading. Please reload this page.
feat(rp2040): make PICOBOOT/picotool the primary deploy transport (#1162, #1163) - #1195
Conversation
, #1163) - --transport picotool|uf2 (default picotool) plumbed CLI -> daemon -> Rp2040Deployer, mirroring the CH32V --protocol pattern. - Picotool-primary flow: 1200-baud touch -> best-effort BOOTSEL volume wait -> Windows WinUSB preflight -> bounded `picotool info` probe -> `picotool load <uf2> -f -x` (FBUILD_RP2040_PICOTOOL_TIMEOUT_SECS, default 60s) -> mass-storage NEW.UF2 fallback on any failure. - --transport uf2 preserves the historical mass-storage-first order; the picotool fallback also gains -f and the configurable timeout. - Windows preflight (rp2040_preflight.rs): classifies the VID_2E8A&PID_0003 composite-interface devnode's CM problem code; the CM_PROB_FAILED_INSTALL family (28/1/31) skips picotool outright with actionable WinUSB guidance naming the exact devnode. - flash_confirmed is now typed instead of a transfer-method string prefix check; combined transport failures are direction-aware. Closes#1162. Closes#1163. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRP2040 deployment now supports selectable ChangesRP2040 transport deployment
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Daemon
participant RP2040Deployer
participant Picotool
participant BOOTSEL
CLI->>Daemon: Send DeployRequest with transport
Daemon->>RP2040Deployer: Apply transport configuration
RP2040Deployer->>Picotool: Run preflight, info, and load
Picotool-->>RP2040Deployer: Return success or failure
RP2040Deployer->>BOOTSEL: Transfer UF2 as fallback
BOOTSEL-->>RP2040Deployer: Confirm ROM acceptance
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-deploy/src/rp2040_preflight.rs`:
- Around line 24-28: Replace the production default matching on PICOBOOT_VID_PID
in the RP2040 preflight logic with VID/PID extraction from the instance ID,
following the existing parsing approach in fbuild-serial’s ports handling.
Resolve bootloader-role profiles through
fbuild_core::usb::profiles::profiles_for and use
rp_bootloader_profiles_match_generation with Rp2040Deployer::family_id so RP2040
and RP2350 generations are classified correctly; retain
COMPOSITE_INTERFACE_MARKER for interface discrimination and keep any hardcoded
identifiers test-only.
In `@crates/fbuild-deploy/src/rp2040.rs`:
- Around line 1741-1747: Update the picotool-primary success tuple in
transfer_volume to return None for transfer_volume instead of volume.clone().
Remove the now-unnecessary clone while preserving the existing Err-arm volume
handling and picotool_confirmed state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dbddb4bb-cc2a-4657-9680-f7499f1e1a3e
📒 Files selected for processing (12)
agents/docs/deploy-architecture.mdcrates/fbuild-cli/src/cli/args.rscrates/fbuild-cli/src/cli/deploy.rscrates/fbuild-cli/src/cli/dispatch.rscrates/fbuild-cli/src/daemon_client/types.rscrates/fbuild-cli/src/mcp/tools.rscrates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-daemon/src/models.rscrates/fbuild-deploy/src/rp2040.rscrates/fbuild-deploy/src/rp2040_picotool.rscrates/fbuild-deploy/src/rp2040_preflight.rsdocs/reference/cli.md
| /// 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_"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Hardcoded PICOBOOT VID/PID; also misses RP2350.
PICOBOOT_VID_PID embeds a real device USB VID/PID in production code and uses it as the runtime matching default, rather than resolving it through the FastLED/boards registry — the same registry rp2040.rs and the daemon's recovery-request composer already consult via fbuild_core::usb::profiles::profiles_for(vid, pid).
Functional consequence: substring-matching a single PID means RP2350 boards (and any other RP-series bootloader PID) never classify as DriverMissing, so preflight always returns Ready and the fast-path this module exists for never fires on those boards. Rp2040Deployer already carries family_id, and rp_bootloader_profiles_match_generation exists for exactly this generation gating.
Suggested direction: parse VID/PID out of the instance ID (fbuild-serial's ports.rs already does this around line 348) and gate on bootloader-role profiles from the registry, keeping &MI_ as the composite-interface discriminator. The test constants can stay as test-only fixtures.
As per coding guidelines, "Never hardcode or embed board/device USB VID/PID records in fbuild code, production tests, generated Rust tables, or release artifacts. VID/PIDs must come from the published FastLED/boards registry through its normal ingestion pipeline; synthetic or copied IDs are allowed only in test-only fixtures and must not become runtime defaults."
Also applies to: 58-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-deploy/src/rp2040_preflight.rs` around lines 24 - 28, Replace
the production default matching on PICOBOOT_VID_PID in the RP2040 preflight
logic with VID/PID extraction from the instance ID, following the existing
parsing approach in fbuild-serial’s ports handling. Resolve bootloader-role
profiles through fbuild_core::usb::profiles::profiles_for and use
rp_bootloader_profiles_match_generation with Rp2040Deployer::family_id so RP2040
and RP2350 generations are classified correctly; retain
COMPOSITE_INTERFACE_MARKER for interface discrimination and keep any hardcoded
identifiers test-only.
Source: Coding guidelines
| Ok(loaded) => ( | ||
| loaded.stdout, | ||
| loaded.stderr, | ||
| "PICOBOOT (managed picotool)", | ||
| volume.clone(), | ||
| true, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
transfer_volume should be None on the picotool-primary success path.
volume is the BOOTSEL mass-storage volume discovered before the transport attempt; picotool never wrote to it. Setting it as transfer_volume has two downstream effects:
- Line 1805 then runs
wait_for_volume_disappearance(volume, post_deploy_timeout)with??, so a lingering/cached mount turns a fully successful PICOBOOT load into a hard deploy failure reporting"did not eject after NEW.UF2"— an eject that never applied here. - Line 1848
describe_transfer_locationreports the drive path instead of"PICOBOOT vendor interface", contradicting the"PICOBOOT (managed picotool)"method string in the same tuple.
picotool_confirmed = true already carries flash_confirmed at line 1847, so None loses nothing. Note the Uf2 arm's fallback branch correctly uses Some(failure.volume) there because that volume was written to.
🐛 Proposed fix
Ok(loaded) => (
loaded.stdout,
loaded.stderr,
"PICOBOOT (managed picotool)",
- volume.clone(),+ None,
true,
),volume is still moved by the Err arm below, so the clone() can go away with it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Ok(loaded) => ( | |
| loaded.stdout, | |
| loaded.stderr, | |
| "PICOBOOT (managed picotool)", | |
| volume.clone(), | |
| true, | |
| ), | |
| Ok(loaded) => ( | |
| loaded.stdout, | |
| loaded.stderr, | |
| "PICOBOOT (managed picotool)", | |
| None, | |
| true, | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-deploy/src/rp2040.rs` around lines 1741 - 1747, Update the
picotool-primary success tuple in transfer_volume to return None for
transfer_volume instead of volume.clone(). Remove the now-unnecessary clone
while preserving the existing Err-arm volume handling and picotool_confirmed
state.
Implements the two software sub-issues of meta #1166 (from the #1161 mass-storage write-stall investigation).
#1162 — PICOBOOT/picotool as first-class primary transport
--transport picotool|uf2deploy flag (defaultpicotool), plumbed CLI → HTTP → daemon →Rp2040Deployerexactly like the CH32V--protocolflag.picotool infoprobe (10 s) →picotool load <uf2> -f -x(-fadded so an application-mode board can be vendor-reset) with configurableFBUILD_RP2040_PICOTOOL_TIMEOUT_SECS(default 60 s; replaces the hardcoded 30 s) → on success theRPI-RP2volume is never written.NEW.UF2path (retries + fix(rp2040): cancel timed-out UF2 writes #1167 watchdog).--transport uf2restores the historical order exactly.flash_confirmedis now a typed bool rather than a transfer-method string-prefix check; combined "both transports failed" errors are direction-aware.#1163 — Windows WinUSB preflight + code-28 classification
fbuild_serial::ports::present_usb_problem_devices()(fix(windows): enumerate problem devices by USB enumerator, not setup class #1157 machinery): aVID_2E8A&PID_0003composite-interface (&MI_) devnode with aCM_PROB_FAILED_INSTALL-family problem code (28/1/31) ⇒DriverMissing— picotool is skipped outright (no probe/load timeout burn) and the guidance names the exact devnode instance ID + problem code with the Raspberry Pi-documented fix (bind WinUSB to "RP2 Boot (Interface 1)"), explicitly noting it is not a board fault. Other problem codes warn and continue (the probe is authoritative). No-op off Windows / healthy hosts.Validation (local)
soldr cargo test -p fbuild-deploy→ 260 passed (18 new transport/ordering tests + 7 preflight tests);-p fbuild-cli→ 210 passed;-p fbuild-daemon→ 215 passedsoldr cargo clippy -p fbuild-deploy -p fbuild-cli -p fbuild-daemon --all-targets -- -D warnings→ clean;fmt --checkcleanbash testrun before mergeHardware validation of the flip against the pinned artifact is #1165 (gated on the physical board).
Closes#1162. Closes#1163.
🤖 Generated with Claude Code
Summary by CodeRabbit
--transport picotool|uf2to RP2040/RP2350 deployments.picotoolby default, with automatic BOOTSEL/UF2 fallback.picotooltimeout support throughFBUILD_RP2040_PICOTOOL_TIMEOUT_SECS.