fix(rp2040): consume Windows port health in deploy selection and reacquisition - #1153
Merged
Conversation
…quisition A retained CM_PROB_PHANTOM devnode whose serial still matches the board could be selected pre-flash, touched at 1200 baud, and returned as the post-flash deploy port. Health from #1146/#1150 now gates every step: - rp2040_target: select_cdc_candidate returns a health-eligible PicoCdcPort (phantom/present-problem records are never selected); resolve_requested_runtime_target fails an explicit selector that matches a known-unhealthy record, with health/problem/instance detail - rp2040: the post-flash wait adds a bounded openability probe seam; timeout diagnostics carry the last open error and phantom-aware BOOT/RESET recovery guidance; Deployer::owns_post_flash_port_discovery marks the RP2040 port as authoritative - daemon: recovery/monitor port resolution no longer substitutes the requested pre-flash name when the deployer owns port discovery (deploy_port_str.or(result.port) inverted the trust order); flash success with an unrecovered CDC now surfaces the deployer's recovery diagnostic instead of discarding it; choose_deploy_port partitions known-unhealthy RP2040 records out of auto-selection with a diagnostic warning Closes #1147 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe deployer contract now identifies post-flash port discovery ownership. RP2040 and Raspberry Pi CDC selection excludes known-unhealthy records, probes endpoint openability, reports richer diagnostics, and prevents monitoring from using stale pre-flash ports. ChangesRP2040 CDC recovery and deploy selection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeployHandler
participant Rp2040Deployer
participant CDCWait
participant Monitor
DeployHandler->>Rp2040Deployer: deploy and recover runtime CDC
Rp2040Deployer->>CDCWait: select and probe CDC candidate
CDCWait-->>Rp2040Deployer: confirmed endpoint or recovery diagnostics
Rp2040Deployer-->>DeployHandler: deployment result and recovery port
DeployHandler->>Monitor: monitor recovered endpoint or skip monitoring
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/dylint.ymlTraceback (most recent call last): 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-daemon/src/handlers/operations/deploy.rs`:
- Around line 901-913: Update the recovery cleanup around resolve_recovery_port
and clear_preemption so deploy_port_str is cleared first when present, before
falling back to clearing recovery_port. Preserve the existing recovery-port
behavior for cases without a deploy port, ensuring owned post-flash recovery
releases the original pre-flash preemption even when the port is renumbered.
🪄 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: a93469ac-0311-4871-8888-96b0c319bbbf
📒 Files selected for processing (5)
crates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-daemon/src/handlers/operations/deploy_port.rscrates/fbuild-deploy/src/lib.rscrates/fbuild-deploy/src/rp2040.rscrates/fbuild-deploy/src/rp2040_target.rs
Comment on lines
+901
to
913
| let port_discovery_owned = deployer_for_recovery | ||
| .as_ref() | ||
| .is_some_and(|deployer| deployer.owns_post_flash_port_discovery()); | ||
| let recovery_port = resolve_recovery_port( | ||
| port_discovery_owned, | ||
| deploy_port_str.clone(), | ||
| deploy_result | ||
| .as_ref() | ||
| .ok() | ||
| .and_then(|result| result.port.clone()) | ||
| }); | ||
| .and_then(|result| result.port.clone()), | ||
| ); | ||
| if let Some(ref p) = recovery_port { | ||
| ctx.serial_manager.clear_preemption(p).await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust -C4 '(fn\s+clear_preemption|fn\s+preempt_for_deploy|last_disconnect|remove.*preempt)' crates/fbuild-daemon/srcRepository: FastLED/fbuild
Length of output: 12569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deploy.rs relevant sections =="
sed -n '400,470p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
sed -n '850,930p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
sed -n '1210,1265p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
echo "== serial_manager references in deploy and manager =="
rg -n --type=rust -C3 'serial_manager|preempt|clear_preemption|preemption' crates/fbuild-daemon/src/handlers/operations/deploy.rs crates/fbuild-daemon/src h 2>/dev/null || true
echo "== locate serial_manager type definition/mocks/tests =="
rg -n --type=rust -C3 'struct .*Serial|serial_manager|preempt_for_deploy|clear_preemption|MonitorLease|preemption' crates/fbuild-daemon/srcRepository: FastLED/fbuild
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared serial manager files =="
fd -i 'serial' crates/fbuild-daemon/src -t f
echo "== serial manager outline and relevant preemption API =="
for f in $(fd -i 'serial' crates/fbuild-daemon/src -t f); do
echo "--- $f"
rg -n 'pub struct |mod |impl .*Serial|clear_preemption|preempt_for_deploy|has_clients|notify_port_renumbered|rebind_port_session' "$f" -C4
done
echo "== focused read files =="
sed -n '1,360p' crates/fbuild-daemon/src/handlers/operations/monitor.rs
sed -n '700,950p' crates/fbuild-daemon/src/handlers/operations/deploy.rs
sed -n '1220,1260p' crates/fbuild-daemon/src/handlers/operations/deploy.rsRepository: FastLED/fbuild
Length of output: 26436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files that define SharedSerialManager =="
rg -n --type=rust 'SharedSerialManager|impl .*SharedSerialManager|struct SharedSerialManager' crates -S
echo "== imports and usages with clearer boundary =="
rg -n --type=rust 'serial_manager|SerialManager|Monit' crates/fbuild-daemon/src/context.rs crates/fbuild-daemon/src/handlers/operations/deploy.rs crates/fbuild-daemon/src/handlers/operations/reset.rs crates/fbuild-daemon/src/handlers/operations/monitor.rs crates/fbuild-daemon/src/lib.rs crates/fbuild-daemon/src/main.rs -S
echo "== repository-wide serial manager exact matches =="
rg -n 'struct .*SerialManager|enum .*SerialManager|impl .*SerialManager|clear_preemption|preempt_for_deploy|notify_port_renumbered' crates -SRepository: FastLED/fbuild
Length of output: 12021
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== manager preemption, client tracking, and renumber APIs =="
sed -n '50,140p' crates/fbuild-serial/src/manager.rs
sed -n '440,520p' crates/fbuild-serial/src/manager.rs
sed -n '640,730p' crates/fbuild-serial/src/manager.rs
sed -n '840,945p' crates/fbuild-serial/src/manager.rs
sed -n '1020,1105p' crates/fbuild-serial/src/manager.rs
echo "== monitor attach/detach/close preemption handling =="
rg -n --type=rust -C5 'attach_reader|detach_reader|release_writer|close_port_after_grace_if_idle|has_clients' crates/fbuild-serial/src/manager.rs
echo "== local behavioral verifier for preemption mapping logic =="
python3 - <<'PY'
requested = "COM12"
deployer_port = "COM27"
old_recovered = requested or deployer_port
new_recovered = deployer_port
preempted = requested
cleared_by_new = set(p for p in [deployer_port] if p)
print("preempted:", preempted)
print("ownership: requested", requested, "deployer_port", deployer_port)
print("old_recovery_port:", old_recovered)
print("new_recovery_port:", new_recovered)
print("cleared_preempted", preempted in cleared_by_new)
PYRepository: FastLED/fbuild
Length of output: 22357
Release the pre-flash preemption after owned post-flash recovery. Owned deployers resolve recovery_port to the new port, so a renumbered pre-flash endpoint like COM12 never has its preempt_for_deploy() lease cleared when clear_preemption only runs on the recovered COM27 port. Clear deploy_port_str before falling back to recovery-port clearing.
🔒 Proposed fix
let recovery_port = resolve_recovery_port(
port_discovery_owned,
deploy_port_str.clone(),
deploy_result
.as_ref()
.ok()
.and_then(|result| result.port.clone()),
);
+ // Always release the preemption taken on the pre-flash port,
+ // even when owned discovery moved recovery to a renumbered endpoint.
+ if let Some(ref pre) = deploy_port_str {
+ if recovery_port.as_deref() != Some(pre.as_str()) {
+ ctx.serial_manager.clear_preemption(pre).await;
+ }
+ }
if let Some(ref p) = recovery_port {
ctx.serial_manager.clear_preemption(p).await;📝 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.
Suggested change
| let port_discovery_owned = deployer_for_recovery | |
| .as_ref() | |
| .is_some_and(|deployer| deployer.owns_post_flash_port_discovery()); | |
| let recovery_port = resolve_recovery_port( | |
| port_discovery_owned, | |
| deploy_port_str.clone(), | |
| deploy_result | |
| .as_ref() | |
| .ok() | |
| .and_then(|result| result.port.clone()) | |
| }); | |
| .and_then(|result| result.port.clone()), | |
| ); | |
| if let Some(ref p) = recovery_port { | |
| ctx.serial_manager.clear_preemption(p).await; | |
| let port_discovery_owned = deployer_for_recovery | |
| .as_ref() | |
| .is_some_and(|deployer| deployer.owns_post_flash_port_discovery()); | |
| let recovery_port = resolve_recovery_port( | |
| port_discovery_owned, | |
| deploy_port_str.clone(), | |
| deploy_result | |
| .as_ref() | |
| .ok() | |
| .and_then(|result| result.port.clone()), | |
| ); | |
| // Always release the preemption taken on the pre-flash port, | |
| // even when owned discovery moved recovery to a renumbered endpoint. | |
| if let Some(ref pre) = deploy_port_str { | |
| if recovery_port.as_deref() != Some(pre.as_str()) { | |
| ctx.serial_manager.clear_preemption(pre).await; | |
| } | |
| } | |
| if let Some(ref p) = recovery_port { | |
| ctx.serial_manager.clear_preemption(p).await; |
🤖 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-daemon/src/handlers/operations/deploy.rs` around lines 901 -
913, Update the recovery cleanup around resolve_recovery_port and
clear_preemption so deploy_port_str is cleared first when present, before
falling back to clearing recovery_port. Preserve the existing recovery-port
behavior for cases without a deploy port, ensuring owned post-flash recovery
releases the original pre-flash preemption even when the port is renumbered.
… gate The Formatting and Check workflows have been red on main since #1145: - four files (two from this branch, two pre-existing: board/loaders.rs, ch32v_core.rs) carry diffs under the pinned 1.94.1 formatter. Local cargo-fmt --all silently skips the daemon handler files on this checkout, so they were formatted with direct invocations. - the #1150 health-classification helpers in fbuild-serial have no non-Windows, non-test caller, which RUSTFLAGS=-D warnings promotes to a hard error on the ubuntu/macos Check jobs. Annotate them cfg_attr(not(windows), allow(dead_code)) — they are fed by the Windows PnP enumeration and unit-tested cross-platform. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
board_hint, vcom_for_env, and family_for_vid_pid resolve through the FastLED/boards registry at runtime; on doctest hosts (local and CI) that cache is absent, so the three examples panicked on every Check job since the vendored-table migration. no_run keeps them compile-checked without requiring a populated registry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ci/find_direct_subprocess.py --fail has flagged the ch32v multilib probe since it landed, keeping the Check jobs red. The spawn is a short synchronous -print-multi-directory capability probe; mark it allow-direct-spawn rather than routing a one-flag probe through the subprocess facade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The USB VID/PID catalogue-literal guard has flagged this user-facing string since the wchisp deployer landed, keeping Check (ubuntu) red. Identity data belongs to the FastLED/boards registry; point the user at 'fbuild port scan' instead of embedding 4348/1a86 literals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Dylint crate-formatting gate (nightly style edition) has wanted these two use-statement reorders since the lint landed; apply them so the gate goes green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… dylint Same nightly-2026-03-26 style-edition reorders as ban_raw_subprocess; verified with the crate-pinned formatter that both format-checked dylint crates are now clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lint Companion to the Python-lint marker: the workspace dylint pass wants its own allowlist.txt entry for the same short synchronous -print-multi-directory capability probe. ci/check_dylint_allowlists.py passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ix_compare WalkDir-derived paths are stripped against the exact root spelling they were enumerated from (same-normalized by construction) and feed a content hash, not a path-keyed cache lookup — the lint's own stated exemption. Pre-existing debt unmasked once the earlier Dylint gate failures were fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… edit The previous round still flagged the freshly-allowlisted esp32 framework-library cache: the workspace dylint step served a compiled lint from the restored build cache, ignoring the allowlist-only edit. Touch the lint source (documenting the trap) so the fingerprint moves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cargo-dylint loads the <name>@<toolchain> library copy, but the alias loop only created it when absent. With target caching, a restored stale alias therefore pinned every lint to its first-ever compiled allowlist: rebuilt bare libraries (allowlist/source edits) were never loaded, which is why ban_raw_path_prefix_compare kept flagging the freshly-allowlisted esp32 framework-library cache. Overwrite the alias when the bare library is newer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1147 (tracked by meta #1149).
What changed
RP2040 deployment never treats a stale Windows CDC history record as a usable runtime port. Health from #1146 (PR #1150) now gates every step of the state machine, and the daemon no longer re-blesses the requested pre-flash COM name.
Files / state transitions
crates/fbuild-deploy/src/rp2040_target.rsselect_cdc_candidatefilters known-unhealthy (Phantom/PresentProblem) records before serial/previous-port/new-port matching and returns the full health-carryingPicoCdcPortinstead of a bare name.Unknownhealth (non-Windows) stays eligible.resolve_requested_runtime_targetfails an explicit--port COMx/SER=selector that matches a known-unhealthy record, printing health label, problem code, and instance ID instead of touching the stale handle (no 1200-baud touch on it).crates/fbuild-deploy/src/rp2040.rswait_for_cdc_port_with_clockgains an injectable bounded openability probe: a selected candidate that failsopen()is never returned; the loop keeps polling and the last open error lands in the timeout diagnostics.CdcWaitTimeoutcarrieslast_open_errorand emits phantom-aware recovery guidance (stale devnode identity + exact BOOT/RESET → RPI-RP2 steps + direct-port/cable advice).resolve_post_flash_cdcconfirmed-flash timeout note now includes the full diagnostics and guidance — flash success and CDC recovery are reported separately (port: None).Rp2040Deployer::owns_post_flash_port_discovery() == true.crates/fbuild-deploy/src/lib.rs— new default-falseDeployer::owns_post_flash_port_discovery(); legacy deployers are unchanged.crates/fbuild-daemon/src/handlers/operations/deploy.rsdeploy_port_str.or_else(|| deploy_result.port)is replaced byresolve_recovery_port(...): when the deployer owns port discovery, only the freshly catalogued, health-eligible, open-verifiedDeploymentResult.portis used forclear_preemption/post_deploy_recovery/ monitor attach;Nonemeans nothing is touched or monitored (monitor was skippedresponse instead of/dev/ttyUSB0fallback).FBUILD_DEPLOY_PORT=stays empty (never stale) when no port was recovered, and the deployer's recovery diagnostic — previously discarded on success — is appended to the response message.crates/fbuild-daemon/src/handlers/operations/deploy_port.rschoose_deploy_port(RaspberryPi arm) partitions known-unhealthy device records out of auto-selection viapartition_rp_candidates+rp_deploy_choice; they are reported in a warning (health/problem/instance,fbuild port scanpointer) and deploy continues on the BOOTSEL-volume path.Deterministic tests added
rp2040_target.rs: phantom serial match never selected; present-problem previous port never reselected; healthy record wins over phantom history with the same serial; unhealthy new port creates no ambiguity; explicit selector on phantom fails with health details;Unknownhealth stays eligible.rp2040.rs: open-probe failure never returned + lands in diagnostics; candidate that stays phantom is never returned (probe unreachable); confirmed flash + phantom history yields instance/health/problem/last-open-error/BOOTSEL guidance; existing #1095 delayed-handoff and renumbering tests updated for the probe seam.deploy.rs(daemon): owned port discovery never substitutes the requested pre-flash port; legacy deployers keep requested-first order.deploy_port.rs(daemon): phantom RP2040 CDC never auto-selected (with diagnostic warning); healthy selected while phantom is reported; present-problem excluded; unknown health eligible.Validation
Note:
fbuild-serialboards.rsdoctests (board_hint,family_for_vid_pid,vcom_for_env) fail locally without a populated FastLED/boards registry; this is identical onorigin/mainin this environment (this PR does not touchfbuild-serialor its dependencies) and green in CI.Callers intentionally left on raw/lossy enumeration
choose_deploy_port— generic platforms keep existing VID/PID-only behavior; extending the health gate beyond RP2040 is test(usb): validate port scanning parity on macOS and Linux #1091-adjacent follow-up scope.Deployer::post_deploy_recoverydefault and RP2040 override — advisory-only open-poll; with the daemon anchor fixed, a stale name can no longer reach it, so it was left untouched.DeviceInfo/DeviceStatusResponsemirrors still omitport_health/instance IDs — response-surface plumbing belongs to the feat(windows): recover RP2040 phantom/BOOTSEL USB state through scoped admin PnP flow #1152 typed-recovery field work.Hardware
Not touched by this PR. The attached board (serial
5303284720C4641C) is deliberately preserved in the reproduced wedge (BOOTSELRPI-RP2onG:, phantomCOM12, PICOBOOT MI_01CM_PROB_FAILED_INSTALL) for the fbuild-only closeout run, which follows with #1152's--adminorchestration per the plan in #1152 (comment). Next child after merge: #1152.🤖 Generated with Claude Code
Summary by CodeRabbit
fbuild port scanfor identifying the USB-ISP device.