Uh oh!
There was an error while loading. Please reload this page.
Feat/433 comprehensive teensy deployer - #449
Merged
Merged
Conversation
Replaces the bare `teensy_loader_cli` wrapper with a state machine that addresses the wedged-Windows-host failure modes catalogued in #433: * `soft_reboot.rs` — baud-134 trigger so a running user firmware can be flashed unattended (no program-button press); also fixes#432 (`Soft reboot is not implemented for Win32`). * `halfkay_probe.rs` — confirms the CDC port left the bus after the baud-134 trigger. * `flash.rs` — bounded retry+backoff around `teensy_loader_cli` for the WinUSB "error writing to Teensy" quirk that fires 6/10 cycles. * `port_discovery.rs` — pre-flash port snapshot + post-flash new-CDC-ACM detection; surfaces the freshly enumerated port through `DeploymentResult.port`. * `first_byte_probe.rs` — advisory probe that warns when a successful flash produces zero serial bytes (typical `setup()`-hang signature). * `usb_type.rs` — best-effort read of the build's `usb_type` and advisory when no Serial endpoint exists (`USB_RAWHID`, …). * Two-tier timeouts (`flash_timeout_secs` vs `wait_for_halfkay_timeout_secs`) so first-flash button-press scenarios don't kill the loader at 60s. * Env escape hatches: `FBUILD_TEENSY_FLASH_RETRIES`, `FBUILD_TEENSY_FIRST_BYTE_TIMEOUT_SECS`, `FBUILD_TEENSY_DISABLE_BAUD_134_TRIGGER`. Daemon's `/api/deploy` now forwards `DeploymentResult.port` to the post-deploy monitor (failure mode #7) so re-enumeration to a new COM name doesn't leave the monitor on the dead pre-flash port. Public API of `TeensyDeployer::new` / `from_board_config` is unchanged; `TeensyLoaderParams` gains opt-in fields with sensible defaults so the daemon dispatch site needs no code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Two-tier retry timeout: first attempt gets `wait_for_halfkay_timeout_secs`
(180 s default — covers the "user walks up and presses the program
button" case); subsequent retries get only `flash_timeout_secs` (30 s
default) so a wedged device can't burn 15 min of subprocess time before
the structured diagnostic surfaces. `flash::run_with_retry` takes both
timeouts explicitly.
* Normalize the caller's explicit port once: `Some("")` becomes `None`
so an empty string never leaks into `DeploymentResult.port` (which the
daemon would forward verbatim to the monitor).
* On post-flash port-discovery timeout, fall back to the resolved
trigger port (the PJRC device we actually flashed) instead of `None`
so the same-port-after-flash case still produces a usable port name.
* Serialize the three env-var-mutating tests via a shared
`TEST_ENV_LOCK` so parallel `cargo test` execution can't race the
global process environment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>📝 WalkthroughWalkthroughThis PR replaces a simplistic Teensy deployer wrapper with a robust state machine that handles USB re-enumeration, flash retry, and post-deploy diagnostics. The implementation spans seven support modules ( ChangesTeensy USB Deployment State Machine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🧹 Nitpick comments (1)
crates/fbuild-deploy/src/teensy/flash.rs (1)
1-249: ⚡ Quick winConsider testing the UTF-8 boundary fix with multi-byte characters.
Once the UTF-8 slicing fix is applied, add a test case to verify correct handling of multi-byte characters near the truncation boundary.
📝 Suggested test case
#[test]fnshort_one_line_handles_multibyte_chars_near_boundary(){// Build a string with multi-byte chars around the 120-byte marklet base = "x".repeat(115);let emoji = "😀";// 4-byte emojilet long = format!("{}{}{}", base, emoji,"y".repeat(10));// Should truncate without panickinglet result = short_one_line(&long);assert!(result.len() <= 120 + '…'.len_utf8());assert!(result.ends_with('…'));}🤖 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/teensy/flash.rs` around lines 1 - 249, Add a unit test to exercise short_one_line with multi-byte UTF-8 characters near the 120-byte truncation boundary: create a string composed of ASCII filler up to just before 120 bytes, insert a multi-byte glyph (e.g. "😀") and more ASCII, call short_one_line, and assert the returned string length is <= 120 + '…'.len_utf8() and that it ends with the ellipsis; place this test in the existing tests module alongside the other short_one_line tests and name it short_one_line_handles_multibyte_chars_near_boundary to ensure truncation handles UTF-8 boundaries without panicking.
🤖 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/teensy/flash.rs`:
- Around line 178-190: short_one_line currently slices the string by byte index
(&first_line[..max]) which can panic on multi-byte UTF-8 characters; update
short_one_line to truncate at a valid UTF-8 character boundary instead of raw
byte slicing (for example, iterate char_indices on first_line to find the
largest index <= max and slice at that boundary or build the prefix by taking
chars until the encoded length reaches max), then append the ellipsis when
truncated; keep function name short_one_line and preserve the existing behavior
for empty input and the 120-character limit.
In `@crates/fbuild-deploy/src/teensy/README.md`:
- Around line 6-7: The markdown link to issue `#433` is being broken so that
“#433” is parsed as a heading: replace the broken link text with a proper inline
URL (for example wrap the full issue URL in angle brackets like
<https://github.com/FastLED/fbuild/issues/433> or remove the stray line break)
so the issue number is not treated as a heading, and add a language identifier
to the fenced code block that contains the pre-snapshot diagram (replace the
opening ``` fence with ```text) to satisfy MD040; update the README.md
occurrences around the issue link and the fenced block (the link referencing
issue `#433` and the fenced block beginning with the “pre-snapshot → …” diagram)
accordingly.
---
Nitpick comments:
In `@crates/fbuild-deploy/src/teensy/flash.rs`:
- Around line 1-249: Add a unit test to exercise short_one_line with multi-byte
UTF-8 characters near the 120-byte truncation boundary: create a string composed
of ASCII filler up to just before 120 bytes, insert a multi-byte glyph (e.g.
"😀") and more ASCII, call short_one_line, and assert the returned string length
is <= 120 + '…'.len_utf8() and that it ends with the ellipsis; place this test
in the existing tests module alongside the other short_one_line tests and name
it short_one_line_handles_multibyte_chars_near_boundary to ensure truncation
handles UTF-8 boundaries without panicking.
🪄 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: 809a6b69-dac4-4ded-8d62-4dc367a48207
📒 Files selected for processing (13)
crates/fbuild-build/src/symbol_analyzer.rscrates/fbuild-core/src/symbol_analysis/mod.rscrates/fbuild-core/src/symbol_analysis/tests.rscrates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-deploy/src/teensy.rscrates/fbuild-deploy/src/teensy/README.mdcrates/fbuild-deploy/src/teensy/first_byte_probe.rscrates/fbuild-deploy/src/teensy/flash.rscrates/fbuild-deploy/src/teensy/halfkay_probe.rscrates/fbuild-deploy/src/teensy/mod.rscrates/fbuild-deploy/src/teensy/port_discovery.rscrates/fbuild-deploy/src/teensy/soft_reboot.rscrates/fbuild-deploy/src/teensy/usb_type.rs
💤 Files with no reviewable changes (2)
- crates/fbuild-core/src/symbol_analysis/mod.rs
- crates/fbuild-deploy/src/teensy.rs
Comment on lines
+178
to
+190
| fn short_one_line(s: &str) -> String { | ||
| let trimmed = s.trim(); | ||
| if trimmed.is_empty() { | ||
| return "<no stderr>".to_string(); | ||
| } | ||
| let first_line = trimmed.lines().next().unwrap_or(""); | ||
| let max = 120; | ||
| if first_line.len() > max { | ||
| format!("{}…", &first_line[..max]) | ||
| } else { | ||
| first_line.to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential panic from UTF-8 boundary slicing in short_one_line.
Line 186 performs byte-index slicing &first_line[..max] where max = 120. If first_line contains multi-byte UTF-8 characters (emoji, non-ASCII text), slicing at byte index 120 can split a character boundary and panic at runtime.
For example, if a 4-byte emoji character spans bytes 119-122, the slice [..120] would attempt to split it and panic with "byte index 120 is not a char boundary".
🐛 Proposed fix using char_indices to find a valid UTF-8 boundary
fn short_one_line(s: &str) -> String {
let trimmed = s.trim();
if trimmed.is_empty() {
return "<no stderr>".to_string();
}
let first_line = trimmed.lines().next().unwrap_or("");
let max = 120;
if first_line.len() > max {
- format!("{}…", &first_line[..max])+ // Find the last valid char boundary at or before max bytes+ let truncate_at = first_line+ .char_indices()+ .take_while(|(idx, _)| *idx < max)+ .last()+ .map(|(idx, ch)| idx + ch.len_utf8())+ .unwrap_or(0);+ format!("{}…", &first_line[..truncate_at])
} else {
first_line.to_string()
}
}📝 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
| fn short_one_line(s:&str) -> String{ | |
| let trimmed = s.trim(); | |
| if trimmed.is_empty(){ | |
| return"<no stderr>".to_string(); | |
| } | |
| let first_line = trimmed.lines().next().unwrap_or(""); | |
| let max = 120; | |
| if first_line.len() > max { | |
| format!("{}…",&first_line[..max]) | |
| }else{ | |
| first_line.to_string() | |
| } | |
| } | |
| fn short_one_line(s:&str) -> String{ | |
| let trimmed = s.trim(); | |
| if trimmed.is_empty(){ | |
| return"<no stderr>".to_string(); | |
| } | |
| let first_line = trimmed.lines().next().unwrap_or(""); | |
| let max = 120; | |
| if first_line.len() > max { | |
| // Find the last valid char boundary at or before max bytes | |
| let truncate_at = first_line | |
| .char_indices() | |
| .take_while(|(idx, _)| *idx < max) | |
| .last() | |
| .map(|(idx, ch)| idx + ch.len_utf8()) | |
| .unwrap_or(0); | |
| format!("{}…",&first_line[..truncate_at]) | |
| }else{ | |
| first_line.to_string() | |
| } | |
| } |
🤖 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/teensy/flash.rs` around lines 178 - 190,
short_one_line currently slices the string by byte index (&first_line[..max])
which can panic on multi-byte UTF-8 characters; update short_one_line to
truncate at a valid UTF-8 character boundary instead of raw byte slicing (for
example, iterate char_indices on first_line to find the largest index <= max and
slice at that boundary or build the prefix by taking chars until the encoded
length reaches max), then append the ellipsis when truncated; keep function name
short_one_line and preserve the existing behavior for empty input and the
120-character limit.
Comment on lines
+6
to
+7
| Implements the design from [issue | ||
| #433](https://github.com/FastLED/fbuild/issues/433) (which supersedes #432). The |
There was a problem hiding this comment.
Fix markdownlint violations in link wrapping and code fence language.
Line 7 breaks the issue link such that #433 is parsed like a heading token, and the state-machine fence at Line 38 should declare a language for MD040 compliance.
Suggested doc-only patch
-Implements the design from [issue-#433](https://github.com/FastLED/fbuild/issues/433) (which supersedes `#432`). The+Implements the design from [issue `#433`](https://github.com/FastLED/fbuild/issues/433) (which supersedes `#432`). The
@@
-```+```text
pre-snapshot → (CDC at port? → baud-134 trigger) → wait-for-HalfKay
→ flash with retry → wait-for-new-CDC → first-byte probe
→ DeploymentResult { port: Some(new_port), … }</details>
Also applies to: 38-42
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 7-7: No space after hash on atx style heading
(MD018, no-missing-space-atx)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/teensy/README.md around lines 6 - 7, The markdown
link to issue #433 is being broken so that “#433” is parsed as a heading:
replace the broken link text with a proper inline URL (for example wrap the full
issue URL in angle brackets like #433
or remove the stray line break) so the issue number is not treated as a heading,
and add a language identifier to the fenced code block that contains the
pre-snapshot diagram (replace the opening fence withtext) to satisfy
MD040; update the README.md occurrences around the issue link and the fenced
block (the link referencing issue #433 and the fenced block beginning with the
“pre-snapshot → …” diagram) accordingly.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- cr-comment:v1:26d538b9c7a7393fbeafcd25 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
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 freeto 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.
Summary by CodeRabbit
New Features
Documentation