feat(python): add reset_device() to SerialMonitor#52
Merged
Conversation
Adds a reset_device(board=None) method that calls the daemon's POST /api/reset endpoint to perform platform-specific DTR/RTS reset sequences. Works without __enter__ (uses HTTP, not WebSocket). Closes #50 Co-Authored-By: Claude Opus 4.6 (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 47 minutes and 30 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Client as Python Client
participant SM as SerialMonitor
participant HTTP as HTTP Client
participant Daemon as fbuild Daemon
participant Device as ESP32 Device
Client->>SM: reset_device(board="esp32s3")
activate SM
SM->>SM: Build endpoint URL from daemon_url
SM->>HTTP: POST /api/reset with {port, board}
activate HTTP
HTTP->>Daemon: HTTP POST request
activate Daemon
Daemon->>Device: Toggle DTR/send reset signal
Device-->>Daemon: Device reboots
Daemon-->>HTTP: JSON response {success: true}
deactivate Daemon
HTTP-->>SM: Response parsed
deactivate HTTP
SM->>SM: Extract success field (default false)
SM-->>Client: return bool
deactivate SM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (2)
tests/test_serial_reset_integration.py (1)
55-64: Nit: usewithinstead of explicit__enter__/__exit__.The explicit dunder calls work but lose the guarantees a
withblock provides (and make exception paths harder to reason about vs the current try/finally). Minor readability nit.♻️ Suggested change
- mon2 = SerialMonitor(port="COM13", baud_rate=115200) - mon2.__enter__() - try: - lines = mon2.read_lines(timeout=5.0) - assert len(lines) > 0, ( - "Device should produce serial output after reset. " - "Got 0 lines — device may not have rebooted." - ) - finally: - mon2.__exit__(None, None, None) + with SerialMonitor(port="COM13", baud_rate=115200) as mon2: + lines = mon2.read_lines(timeout=5.0) + assert len(lines) > 0, ( + "Device should produce serial output after reset. " + "Got 0 lines — device may not have rebooted." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_serial_reset_integration.py` around lines 55 - 64, Replace the manual context management for SerialMonitor with a with-block: instead of calling mon2 = SerialMonitor(...); mon2.__enter__(); try: ... finally: mon2.__exit__(...), instantiate SerialMonitor in a with SerialMonitor(...) as mon2: block and call mon2.read_lines(timeout=5.0); this ensures proper enter/exit semantics and exception safety around the SerialMonitor class and the read_lines call.crates/fbuild-python/src/lib.rs (1)
477-488: Silent failure on reset: surface daemon diagnostics.When the daemon returns
success: false, this method discards themessage(and anystdout) fields and just returnsfalse. Users will see a failed reset with no way to know why (timeout? USB busy? platform detection failed?). The existingsend_ophelper at lines 730–738 already establishes a pattern of loggingmessage/stderrto stderr on failure — consider mirroring that here for parity.♻️ Suggested change
- Ok(body - .get("success") - .and_then(|v| v.as_bool()) - .unwrap_or(false)) + let success = body + .get("success") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + if !success { + if let Some(msg) = body.get("message").and_then(|v| v.as_str()) { + eprintln!("[fbuild] reset_device failed: {}", msg); + } + if let Some(stdout) = body.get("stdout").and_then(|v| v.as_str()) { + if !stdout.is_empty() { + eprintln!("[fbuild] reset stdout:\n{}", stdout); + } + } + } + Ok(success)🤖 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 477 - 488, The reset handler currently parses the JSON response into `body` and returns the boolean success but discards diagnostic fields; update the reset method (the block that reads `let body: serde_json::Value = resp.json()...` and returns the boolean) to mirror `send_op`'s behavior: when `body.get("success").and_then(|v| v.as_bool())` is false, extract `body.get("message")` and `body.get("stderr")` (or `stdout` if present) and log them to stderr (e.g., via eprintln! or the same logging used in `send_op`) before returning false so daemon diagnostics are surfaced to the user. Ensure extraction handles missing/non-string fields safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_serial_reset_integration.py`:
- Around line 19-26: The _port_exists function currently opens the serial port
(pyserial.Serial) which can conflict with a running daemon (SharedSerialManager)
that already holds the port; change _port_exists to detect the port via
enumeration instead of opening it—use pyserial.tools.list_ports.comports() to
look for the target port name (e.g., "COM13") and return True if present, False
otherwise, leaving _daemon_running and any daemon checks untouched.
- Around line 29-37: The _daemon_running function currently hardcodes
"http://127.0.0.1:8765/health", which can diverge from the daemon URL resolution
used by reset_device; replace this check to use the Daemon API from
fbuild._native so it resolves the same port/URL (e.g., import Daemon from
fbuild._native and call the Daemon availability method on it rather than
hardcoding the URL). Ensure you reference and invoke the same Daemon resolution
used by reset_device (use the Daemon class/method provided by fbuild._native to
check reachability) and return True/False based on that result.
---
Nitpick comments:
In `@crates/fbuild-python/src/lib.rs`:
- Around line 477-488: The reset handler currently parses the JSON response into
`body` and returns the boolean success but discards diagnostic fields; update
the reset method (the block that reads `let body: serde_json::Value =
resp.json()...` and returns the boolean) to mirror `send_op`'s behavior: when
`body.get("success").and_then(|v| v.as_bool())` is false, extract
`body.get("message")` and `body.get("stderr")` (or `stdout` if present) and log
them to stderr (e.g., via eprintln! or the same logging used in `send_op`)
before returning false so daemon diagnostics are surfaced to the user. Ensure
extraction handles missing/non-string fields safely.
In `@tests/test_serial_reset_integration.py`:
- Around line 55-64: Replace the manual context management for SerialMonitor
with a with-block: instead of calling mon2 = SerialMonitor(...);
mon2.__enter__(); try: ... finally: mon2.__exit__(...), instantiate
SerialMonitor in a with SerialMonitor(...) as mon2: block and call
mon2.read_lines(timeout=5.0); this ensures proper enter/exit semantics and
exception safety around the SerialMonitor class and the read_lines call.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd698377-20b4-4d31-91f4-dcaf5485f9d0
📒 Files selected for processing (3)
crates/fbuild-python/src/lib.rstests/test_serial_reset.pytests/test_serial_reset_integration.py
- Use port enumeration instead of opening port (avoids daemon conflict) - Use `with` context manager instead of explicit __enter__/__exit__ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d reviews When enabled, CodeRabbit submits REQUEST_CHANGES reviews instead of just comments. PRs cannot be merged until all review comments are acknowledged and CodeRabbit auto-approves. Co-Authored-By: Claude Opus 4.6 (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
reset_device(board=None)method toSerialMonitorPyO3 bindingPOST /api/resetendpoint for platform-specific DTR/RTS sequences__enter__— uses HTTP, not WebSocketWhy
FastLED's autoresearch tool needs to reboot an ESP32 that's already booted and idle. Currently it bypasses fbuild with raw pyserial DTR toggling, which fights the daemon for the serial port. This gives fbuild a proper API for device reset.
Test plan
test_serial_monitor_has_reset_device— method exists on classtest_serial_monitor_reset_device_is_callable— method is callabletest_reset_esp32s3_and_read_output— resets real ESP32-S3, verifies boot outputtest_reset_device_without_enter— works without__enter__Closes #50
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
reset_device()method to SerialMonitor, enabling users to reset connected devices with optional board specification.Tests