Conversation
…or boot output (#68) Extends `SerialMonitor.reset_device` with two keyword args: - `wait_for_output: bool = False` — when True, block until the device produces any serial output after reset. - `timeout: f64 = 5.0` — upper bound for the wait. Behavior: - `wait_for_output=False` (default): unchanged — returns immediately after the DTR/RTS reset succeeds. - `wait_for_output=True`: sleeps 300ms for USB re-enumeration, then polls for output. Two paths: * WebSocket fast path (when `__enter__` has been called): reuses the existing WS read loop via `read_lines_inner(0.2s)` and returns `true` on the first non-empty batch. * HTTP fallback (no WS session): polls `/api/serial/output?port=...&limit=1` at 100ms intervals. - Returns `false` when the deadline expires with no output, or when the daemon-side reset itself failed. Lets Python callers replace `time.sleep(3.0)` after reset with `wait_for_output=True`, typically returning in <500ms on ESP32-S3 USB-CDC instead of the conservative fixed 3s guess. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR extends the Changes
Sequence Diagram(s)sequenceDiagram
participant Python as Python Client
participant Monitor as SerialMonitor
participant HTTP as HTTP API
participant WS as WebSocket Stream
Python->>Monitor: reset_device(board, wait_for_output=true, timeout=5.0)
Monitor->>HTTP: POST /api/reset
HTTP-->>Monitor: {success: true}
alt wait_for_output enabled
Monitor->>Monitor: Sleep for USB re-enumeration
alt WebSocket available
loop Poll until output or timeout
Monitor->>WS: read_lines_inner()
WS-->>Monitor: output lines or empty
alt Output detected
Monitor-->>Python: Ok(true)
end
end
else WebSocket unavailable
loop Poll until output or timeout
Monitor->>HTTP: GET /api/serial/output?port=...&limit=1
HTTP-->>Monitor: output or empty
alt Output detected
Monitor-->>Python: Ok(true)
end
end
end
alt Timeout expired
Monitor-->>Python: Ok(false)
end
else wait_for_output disabled
Monitor-->>Python: Ok(true)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
When wait_for_output=True, reset_device() polls for serial output after the DTR reset instead of returning immediately. Returns as soon as any output is detected (device rebooted) or after timeout. - WebSocket path: polls read_lines_inner() if __enter__ was called - HTTP fallback: polls daemon's serial output API if no WebSocket - Tests: unit test for parameter signature + integration tests Closes #68 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/fbuild-python/src/lib.rs (1)
544-558: Hoist thereqwest::blocking::Clientout of the poll loop.A new client (with its own connection pool) is constructed every 100 ms. Build it once before the loop and reuse it.
+ let http = reqwest::blocking::Client::new(); while std::time::Instant::now() < deadline { - if let Ok(resp) = reqwest::blocking::Client::new() + if let Ok(resp) = http .get(&output_url) .timeout(std::time::Duration::from_millis(500)) .send()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-python/src/lib.rs` around lines 544 - 558, Currently a new reqwest::blocking::Client is constructed on each poll iteration; create the client once before the while loop and reuse it to avoid rebuilding connection pools. Move the call to reqwest::blocking::Client::new() into a local variable (e.g., let client = reqwest::blocking::Client::new();) before the loop that checks deadline, then replace the in-loop construction with client.get(&output_url).timeout(...).send() so the rest of the logic (reading resp.text(), checking body.len() > 10, sleeping) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-python/src/lib.rs`:
- Around line 516-517: Rustfmt expects the deadline binding to be a single line;
collapse the multi-line let binding for deadline into one line so it reads: let
deadline = std::time::Instant::now() +
std::time::Duration::from_secs_f64(timeout); (update the binding that defines
variable `deadline`).
- Around line 440-461: The crate contains two duplicated rustdoc blocks for the
function reset_device which will be concatenated by rustdoc; remove the
stale/older doc block (the first occurrence) so only a single doc comment
remains for reset_device, or replace the old block with the new one; ensure the
remaining doc comment describes arguments wait_for_output, timeout and the
return boolean and is adjacent to the reset_device function definition.
- Around line 539-558: The current HTTP fallback in the wait loop returns
Ok(true) based on body.len() > 10 which matches empty daemon JSON and stale
buffered lines; change the logic in the reqwest::blocking::Client GET loop that
constructs output_url to parse the response JSON (e.g., into a struct with
lines/count or serde_json::Value) and check the "lines" array explicitly
(lines.len() > 0) and/or compare the daemon-reported count/index against a
snapshot taken before the reset so only strictly new lines after the deadline
start count as device output; also prefer using the daemon's cursor/“since”
parameter if available instead of relying on limit=1 to avoid matching pre-reset
buffered lines.
- Around line 522-535: The WS fast-path times out because the daemon never
notifies clients of port preemption/reconnect and doesn't replay buffered
output; modify the daemon to (1) broadcast a Preempted frame to connected WS
sessions (manager.rs before closing the serial broadcaster) so existing WS
handlers see preemption instead of silently closing, (2) reopen the port and
broadcast a Reconnected frame after clearing preemption (and after websockets.rs
acknowledges/reattaches), and (3) replay the buffered serial output to existing
WS clients on reconnect so read_lines_inner / the Python binding's fast path
(self.runtime, self.ws_read, read_lines_inner, deadline) can immediately receive
post-reset data. Ensure the websockets handler (websockets.rs) forwards these
special frames to clients rather than just tearing down the session so
auto_reconnect=true in the Python binding is triggered.
---
Nitpick comments:
In `@crates/fbuild-python/src/lib.rs`:
- Around line 544-558: Currently a new reqwest::blocking::Client is constructed
on each poll iteration; create the client once before the while loop and reuse
it to avoid rebuilding connection pools. Move the call to
reqwest::blocking::Client::new() into a local variable (e.g., let client =
reqwest::blocking::Client::new();) before the loop that checks deadline, then
replace the in-loop construction with
client.get(&output_url).timeout(...).send() so the rest of the logic (reading
resp.text(), checking body.len() > 10, sleeping) remains unchanged.
🪄 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: 8336943d-9263-4197-a5d3-6555acbce84e
📒 Files selected for processing (1)
crates/fbuild-python/src/lib.rs
- Remove duplicate doc comment block on reset_device - Add note about WebSocket auto-reconnect after preemption - Replace unreliable HTTP output polling with conservative 1s sleep when no WebSocket is connected (simpler, no false positives) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Extends
SerialMonitor.reset_devicewithwait_for_output: bool = Falseandtimeout: f64 = 5.0keyword args. Whenwait_for_output=True, the method blocks until the device produces any serial output after the DTR/RTS reset, so Python callers can replacetime.sleep(3.0)with an event-driven wait that typically returns in under 500ms on ESP32-S3 USB-CDC.Behavior
wait_for_output=False(default): unchanged — returns immediately after the reset succeeds.wait_for_output=True:__enter__has been called): reusesread_lines_inner(0.2s)in a loop and returnstrueon the first non-empty batch./api/serial/output?port=...&limit=1at 100ms intervals.falseif the deadline expires with no output or the daemon-side reset itself failed.Context
Closes #68. Parent: FastLED/FastLED#2265 — replace sleep-based reset waits with spin-poll validation.
Test plan
uv run cargo clippy --workspace --all-targets -- -D warningscleanuv run cargo test -p fbuild-python --lib— 8 passed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features