fix(rp2040): wait for delayed Windows CDC handoff#1095
Merged
Conversation
📝 WalkthroughWalkthroughRP2040 deployment now uses board-family application CDC timing, polls with injectable clocks, performs final catalogue-aware timeout handling, and reports detailed diagnostics. Board timing tables, documentation, and tests were updated for delayed CDC enumeration. ChangesRP2040 CDC recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Rp2040Deployer
participant CdcWatcher
participant BoardCatalogue
Rp2040Deployer->>CdcWatcher: Wait for runtime CDC port
CdcWatcher->>BoardCatalogue: Poll catalogue candidates
BoardCatalogue-->>CdcWatcher: Matching port or timeout candidates
CdcWatcher-->>Rp2040Deployer: Port or timeout diagnostics
Rp2040Deployer->>Rp2040Deployer: Resolve confirmed or unconfirmed flash
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.rs`:
- Around line 1127-1140: Update both timeout return paths in the RP2040
post-flash CDC handling: the CdcWaitError::Timeout(diagnostics) arm with
flash_confirmed should append diagnostics.diagnostics() to the
PostFlashCdc::Unconfirmed message, and the other timeout arm should append it to
the FbuildError::DeployFailed message. Keep the existing timeout guidance intact
while exposing the diagnostics to daemon/API clients.
- Around line 2194-2197: Update the test
rp2040_post_flash_cdc_policy_exceeds_the_legacy_15_second_window so it does not
depend on the FBUILD_RP2040_POST_DEPLOY_TIMEOUT environment variable: exercise
the timeout resolution with an injected None override or a pure
default-resolution helper, and retain the assertion that the default is 30
seconds.
🪄 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
Run ID: 2787d374-d618-4bd0-8a07-3624987a973b
📒 Files selected for processing (3)
crates/fbuild-deploy/src/rp2040.rscrates/fbuild-serial/src/boards.rsdocs/usb-cdc-control-line-matrix.md
Comment on lines
+1127
to
+1140
| Err(CdcWaitError::Timeout(diagnostics)) if flash_confirmed => { | ||
| tracing::warn!(diagnostics = %diagnostics.diagnostics(), "RP2040 runtime CDC did not return before the confirmed-flash deadline"); | ||
| Ok(PostFlashCdc::Unconfirmed(format!( | ||
| "the firmware was flashed and accepted, but the runtime CDC port did not reappear within {}s; first-plug driver installation can exceed this window — the board is likely healthy (extend the window with {POST_DEPLOY_TIMEOUT_ENV})", | ||
| window.as_secs() | ||
| ))), | ||
| Err(CdcWaitError::Timeout) => Err(FbuildError::DeployFailed(format!( | ||
| ))) | ||
| } | ||
| Err(CdcWaitError::Timeout(diagnostics)) => { | ||
| tracing::warn!(diagnostics = %diagnostics.diagnostics(), "RP2040 runtime CDC did not return before the unconfirmed-flash deadline"); | ||
| Err(FbuildError::DeployFailed(format!( | ||
| "RP2040 firmware was transferred, but no catalogue-identified runtime CDC port appeared within {}s; verify that the firmware enables USB serial and that FastLED/boards USB data is current (extend the window with {POST_DEPLOY_TIMEOUT_ENV})", | ||
| window.as_secs() | ||
| ))), | ||
| ))) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include timeout diagnostics in the returned result.
Lines 1127-1140 only write elapsed time, prior port/serial, and candidates to tracing. Daemon/API clients receiving the returned Unconfirmed note or DeployFailed error lose the required diagnostics. Append diagnostics.diagnostics() to both returned messages.
🤖 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 1127 - 1140, Update both
timeout return paths in the RP2040 post-flash CDC handling: the
CdcWaitError::Timeout(diagnostics) arm with flash_confirmed should append
diagnostics.diagnostics() to the PostFlashCdc::Unconfirmed message, and the
other timeout arm should append it to the FbuildError::DeployFailed message.
Keep the existing timeout guidance intact while exposing the diagnostics to
daemon/API clients.
Comment on lines
+2194
to
+2197
| #[test] | ||
| fn rp2040_post_flash_cdc_policy_exceeds_the_legacy_15_second_window() { | ||
| assert_eq!(rp2040_post_deploy_timeout(), Duration::from_secs(30)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the default-timeout test independent of the environment.
rp2040_post_deploy_timeout() honors FBUILD_RP2040_POST_DEPLOY_TIMEOUT, so this exact 30-second assertion fails when that supported override is present. Test an injected None override or extract a pure default-resolution helper.
🤖 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 2194 - 2197, Update the test
rp2040_post_flash_cdc_policy_exceeds_the_legacy_15_second_window so it does not
depend on the FBUILD_RP2040_POST_DEPLOY_TIMEOUT environment variable: exercise
the timeout resolution with an injected None override or a pure
default-resolution helper, and retain the assertion that the default is 30
seconds.
Source: Coding guidelines
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.
Fixes #1074
Restores the normal post-flash success path for delayed Windows CDC enumeration: wait up to 30 seconds using the existing boards catalogue and serial selector, perform a final catalogue scan at the deadline, and retain #1083's confirmed-flash fallback with detailed timeout diagnostics.
Validation:
Summary by CodeRabbit
Bug Fixes
Documentation