pyserial driver: add pipe command on cli #781
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI (pipe)"
participant Client as "PySerialClient"
participant SerialTask as "_serial_to_output task"
participant InputTask as "_stdin_to_serial task"
participant Serial as "Serial stream"
participant Output as "File / stdout"
participant Stdin as "stdin"
User->>CLI: invoke `pipe` command
CLI->>Client: call _pipe_serial(output_file, input_enabled, append)
Client->>Client: start task group
alt read flow
Client->>SerialTask: start
end
alt input enabled
Client->>InputTask: start
end
par Bidirectional data flow
Serial->>SerialTask: data available
SerialTask->>Output: write data
and
Stdin->>InputTask: user/piped input
InputTask->>Serial: write to serial
end
User->>CLI: Ctrl+C / KeyboardInterrupt
CLI->>Client: propagate interrupt
Client->>SerialTask: cancel/close
Client->>InputTask: cancel/close
Client->>Serial: close stream
Client->>Output: flush/close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (2)
43-70: Consider adding clarifying comment and return type hint.The infinite loop at lines 68-69 keeps the task group alive after stdin EOF, allowing serial output monitoring to continue. This design is intentional but not immediately obvious.
Apply this diff to add clarity:
async def _pipe_serial( self, output_file: Optional[str] = None, input_enabled: bool = False, append: bool = False, -): +) -> None: """ Pipe serial port data to stdout or a file, optionally reading from stdin. Args: output_file: Path to output file. If None, writes to stdout. input_enabled: If True, also pipe stdin to serial port. append: If True, append to file instead of overwriting. """ async with self.stream_async(method="connect") as stream: async with create_task_group() as tg: # Output task: serial -> file/stdout tg.start_soon(self._serial_to_output, stream, output_file, append) # Input task: stdin -> serial (optional) if input_enabled: tg.start_soon(self._stdin_to_serial, stream) - # Keep running until interrupted (Ctrl+C) - # When input is enabled, this continues even after stdin EOF + # Keep task group alive until interrupted (Ctrl+C) + # After stdin EOF, serial output continues until user interrupts while True: await sleep(1)
71-92: Good exception handling!The exception handling for
EndOfStreamandBrokenResourceErrorcorrectly addresses the past review comment. The implementation properly handles both file and stdout output with appropriate error messages.Optional: Consider adding return type hint for consistency:
-async def _serial_to_output(self, stream, output_file: Optional[str], append: bool): +async def _serial_to_output(self, stream, output_file: Optional[str], append: bool) -> None:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.py(1 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
🧠 Learnings (4)
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
📚 Learning: 2025-11-05T13:45:58.271Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:15-15
Timestamp: 2025-11-05T13:45:58.271Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, pexpect is intentionally used as a transitive dependency through the jumpstarter-driver-pyserial package. The flashers package does not declare pexpect as a direct dependency because the pyserial driver package is intended to control the pexpect version.
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.py (1)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (3)
cli(107-214)_serial_to_output(71-91)open(22-30)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (2)
packages/jumpstarter/jumpstarter/client/base.py (2)
stream(72-82)call(42-52)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (4)
receive(48-51)receive(75-76)send(26-30)send(72-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (7)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (3)
1-7: LGTM!The new imports are appropriate for the async piping functionality being added.
107-213: Excellent CLI implementation!The
pipecommand is well-structured with:
- Proper option validation (lines 171-175)
- Smart stdin auto-detection (line 178)
- Comprehensive user-friendly status messages (lines 189-207)
- Clean KeyboardInterrupt handling (lines 211-212)
The complexity is justified given the feature-rich behavior and clear user feedback.
93-106: Add return type hint for consistency and clarity.The method correctly handles stdin EOF (returns on empty data or EndOfStream exception). Add
-> Nonereturn type annotation:-async def _stdin_to_serial(self, stream): +async def _stdin_to_serial(self, stream) -> None:packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/cli_test.py (4)
1-28: LGTM!The test fixtures are well-structured. Using
loop://URL for the PySerial instance is an excellent approach for testing without requiring actual hardware.
30-92: Comprehensive option validation tests!The tests properly verify CLI option constraints and argument passing to the underlying implementation. Good use of mocking to isolate CLI logic.
93-227: Excellent coverage of CLI behavior!The tests thoroughly verify:
- Flag behavior and interactions
- Stdin auto-detection (leveraging CliRunner's non-TTY behavior)
- Status message output for different modes
- KeyboardInterrupt handling
The test strategy is sound and comprehensive.
229-347: Thorough testing of exception handling!The async tests properly verify the exception handling that was highlighted in past review comments. Testing both stdout and file output paths with
EndOfStreamandBrokenResourceErrorensures robust error handling.The data reception test (lines 329-347) is particularly valuable for verifying the happy path works before exceptions occur.
|
Successfully created backport PR for |
Summary by CodeRabbit
New Features
pipecommand to the PySerial driver CLI for streaming serial data, optional file output, and controllable bidirectional input.Documentation
pipeandstart-consoleusage, options, and behavior notes.Tests
pipebehavior, option validation, mode detection, and startup help.✏️ Tip: You can customize this high-level summary in your review settings.