feat: add Ctrl-] x3 hotkey to power cycle board from serial console#791
Open
evakhoni wants to merge 3 commits into
Open
feat: add Ctrl-] x3 hotkey to power cycle board from serial console#791evakhoni wants to merge 3 commits into
evakhoni wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds reconnect and power-cycle support to the pyserial interactive console. ChangesSerial Console Power-Cycle and Reconnect
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
LuuOW
reviewed
Jun 14, 2026
LuuOW
left a comment
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: Implementation verified for architectural consistency and engineering integrity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py`:
- Around line 135-142: The `_search_power()` method only checks the children of
the provided client node but skips checking if the client node itself is
power-capable. This causes power-driver discovery to fail when the root node has
power capabilities. Modify the method to first check if the current `client`
parameter has a `cycle` method or both `on` and `off` methods before recursing
into `client.children`, ensuring the root node is included in the power-driver
discovery process.
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py`:
- Around line 17-37: The slave_file opened via os.fdopen() in the
_start_console() function is never explicitly closed, causing file descriptor
leaks across multiple test runs. Ensure proper cleanup by either returning
slave_file as part of the function's return tuple so the caller can close it
after the thread completes, or by adding a cleanup mechanism that closes
slave_file after the console.run() thread finishes executing. The key is to
guarantee that the file handle opened for slave_fd is properly closed to prevent
descriptor exhaustion.
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.py`:
- Around line 47-52: The exception re-raising in the except* ConsoleStreamDrop
and except EndOfStream blocks violates Ruff B904 linting rules by not using
explicit exception chaining. For each exception handler block that raises an
exception (the except* ConsoleStreamDrop block that raises ConsoleStreamDrop()
and the except EndOfStream block that raises ConsoleStreamDrop()), capture the
caught exception as a variable in the except clause (e.g., except*
ConsoleStreamDrop as exc) and use the from keyword when raising to provide
explicit chaining (e.g., raise ConsoleStreamDrop() from exc). Alternatively, use
make lint-fix to automatically apply the necessary fixes to satisfy B904
requirements.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c042f0b-f525-40fa-9c8d-e51bfe0279df
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.pypython/packages/jumpstarter/jumpstarter/client/client.py
- Add on_power_cycle callback to Console; Ctrl-] x3 triggers it - Add root back-reference to DriverClient, stamped during tree construction - Auto-discover power client from DUT siblings in PySerialClient - Prefer cycle() when available, fall back to off()+on() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ConsoleStreamDrop exception to signal a dropped serial stream - Add retry loop in start_console to reconnect on stream drop - Use object.__setattr__ to stamp root back-reference without polluting pydantic schema - Remove root field from DriverClient; it is now a dynamic attribute Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
evakhoni
force-pushed
the
feat/console-power-hotkey
branch
2 times, most recently
from
0242c3e to
67ebd74
Compare
- Ctrl-B x3 exits cleanly - Ctrl-] x3 triggers on_power_cycle and console stays alive - Ctrl-] x3 without a power client does nothing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
evakhoni
force-pushed
the
feat/console-power-hotkey
branch
from
67ebd74 to
beac1ee
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py (1)
8-33: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConsolidate new driver-package tests into
driver_test.pyper repository policy.Both files add comprehensive driver-package test coverage, but not in the required module location.
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py#L8-L33: move/merge the power discovery and power-cycle tests intojumpstarter_driver_pyserial/driver_test.py.python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py#L40-L91: move/merge the Ctrl-B/Ctrl-] integration tests intojumpstarter_driver_pyserial/driver_test.py.As per coding guidelines,
**/jumpstarter-driver-*/jumpstarter_driver_*/*_test.py: Add comprehensive tests indriver_test.pyfile within the driver package.🤖 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 `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py` around lines 8 - 33, Move the power discovery and power-cycle tests (test_find_power_client_no_root, test_find_power_client_with_cycle, test_make_power_cycle_calls_cycle) from python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py lines 8-33 into python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.py, and similarly move the Ctrl-B/Ctrl-] integration tests from python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py lines 40-91 into the same driver_test.py file. Per the coding guidelines, comprehensive driver-package tests must be consolidated in the driver_test.py file within the driver package rather than scattered across individual module test files.Source: Coding guidelines
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py (1)
24-33: ⚡ Quick winAdd coverage for the
_make_power_cyclefallback branch.
test_make_power_cycle_calls_cyclevalidates only thecycle()path; theoff()→sleep(2)→on()path inpython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pyis still untested.💡 Suggested test addition
+def test_make_power_cycle_calls_off_on_when_cycle_missing(): + called_off = threading.Event() + called_on = threading.Event() + power = MagicMock(spec=["off", "on"]) + power.off = MagicMock(side_effect=lambda: called_off.set()) + power.on = MagicMock(side_effect=lambda: called_on.set()) + + with serve(PySerial(url="loop://")) as client: + cycle_fn = client._make_power_cycle(power) + client.portal.call(cycle_fn) + + assert called_off.is_set() + assert called_on.is_set()🤖 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 `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py` around lines 24 - 33, The test_make_power_cycle_calls_cycle test only covers the cycle() code path in the _make_power_cycle method, but the fallback branch that uses off() followed by sleep(2) followed by on() remains untested. Add a new test case that mocks the power object to trigger the fallback path (either by not providing a cycle method or having it unavailable), then verify that the fallback sequence of off(), sleep(), and on() method calls is executed properly when the cycle function is invoked through client.portal.call().python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py (1)
44-45: ⚡ Quick winReplace fixed sleeps with deterministic readiness signaling.
Line 44, Line 64, and Line 81 use
time.sleep(0.1)to assume console readiness; this can cause intermittent CI flakes.Also applies to: 64-65, 81-83
🤖 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 `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py` around lines 44 - 45, Replace the three instances of time.sleep(0.1) calls with deterministic readiness signaling instead of relying on fixed delays to assume console readiness. For each location where time.sleep(0.1) precedes an os.write operation, implement a proper synchronization mechanism such as using select, poll, or event-based signaling to confirm the file descriptor is actually ready for writing before proceeding, eliminating the intermittent CI flakes caused by timing assumptions.
🤖 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.
Outside diff comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py`:
- Around line 8-33: Move the power discovery and power-cycle tests
(test_find_power_client_no_root, test_find_power_client_with_cycle,
test_make_power_cycle_calls_cycle) from
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py
lines 8-33 into
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.py,
and similarly move the Ctrl-B/Ctrl-] integration tests from
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py
lines 40-91 into the same driver_test.py file. Per the coding guidelines,
comprehensive driver-package tests must be consolidated in the driver_test.py
file within the driver package rather than scattered across individual module
test files.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py`:
- Around line 24-33: The test_make_power_cycle_calls_cycle test only covers the
cycle() code path in the _make_power_cycle method, but the fallback branch that
uses off() followed by sleep(2) followed by on() remains untested. Add a new
test case that mocks the power object to trigger the fallback path (either by
not providing a cycle method or having it unavailable), then verify that the
fallback sequence of off(), sleep(), and on() method calls is executed properly
when the cycle function is invoked through client.portal.call().
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py`:
- Around line 44-45: Replace the three instances of time.sleep(0.1) calls with
deterministic readiness signaling instead of relying on fixed delays to assume
console readiness. For each location where time.sleep(0.1) precedes an os.write
operation, implement a proper synchronization mechanism such as using select,
poll, or event-based signaling to confirm the file descriptor is actually ready
for writing before proceeding, eliminating the intermittent CI flakes caused by
timing assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb30cdcc-eb35-42ef-8a00-b909cd4dc09f
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py
mangelajo
requested changes
Jun 15, 2026
|
|
||
| return bytes_read, bytes_sent | ||
|
|
||
| def _find_power_client(self): |
Member
There was a problem hiding this comment.
I think we should provide a config entry to let admins specify the power device via a "ref". While I think that finding it automatically is cool, and will work out of the box in most cases, imagine environments where you have multiple power controls , and the power controls could not be what you are expecting, or the reset mechanism is different.
Even in some cases the method to be called could be "reset()", i.e. in the esp32 controller.
So I would take a config with "ref" + method in the serial config
Member
There was a problem hiding this comment.
i.e.
export:
serial:
type:....
config:
....
reset_control:
device:
ref: "esp32"
commands:
- "reset()"
export:
serial:
type:....
config:
....
reset_control:
device:
ref: "power"
commands:
- "on()"
- "sleep 2"
- "off()"
or something like this.
Member
Author
There was a problem hiding this comment.
yeah config sounds good. but if unset let it fallback to the auto discovery WDYT? otherwise I guess the user base would be rather small..
+1 on the reset(), i thought I seen it somewhere but forgot it was esp32.
Member
There was a problem hiding this comment.
IMHO auto detect continues to be risky, it could pick up the wrong power device if you had multiple doing different things in your setup. We could provide a flag for auto-detection, but explain very clearly what it does, and should be disabled by default.
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
3 participants
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
cycle()oron()/off()How it works
The root client is back-referenced onto every driver client after tree construction (
object.__setattr__is used to avoid adding a field to the pydantic schema).PySerialClientuses this to walk the tree and find the first power-capable driver at console start. The discovered callback is passed intoConsole, which intercepts the Ctrl-] sequence and invokes it. If the serial stream subsequently drops,ConsoleStreamDropsignalsstart_consoleto retry the connection.Test plan
🤖 Generated with Claude Code