Nvdemux serial support #798
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a new NVDemuxSerial driver, a singleton DemuxerManager coordinating a shared NVIDIA TCU demuxer subprocess, README documentation, and comprehensive unit tests for registration, device resolution, lifecycle, readiness signaling, pts path distribution, and recovery. Changes
Sequence Diagram(s)sequenceDiagram
participant D as Driver (NVDemuxSerial)
participant M as DemuxerManager
participant P as Demuxer Process
participant S as Serial Port (/dev/pts/...)
D->>M: register_driver(driver_id, demuxer_path, device, chip, target, poll_interval)
activate M
M->>M: resolve device (glob/direct)
M->>P: start demuxer process (if not running)
deactivate M
P-->>M: stdout lines (target -> pts_path)
M->>M: parse output, update pts_map & ready_targets
M->>D: invoke callback(driver_id -> pts_path)
D->>M: get_pts_path(driver_id)
M-->>D: pts_path
D->>S: open serial connection (pts_path, baudrate)
D-->>D: wrap streams in AsyncSerial (apply cps if set)
D->>S: read/write data
D->>M: unregister_driver(driver_id)
M->>M: decrement refcount, stop process if no drivers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Files:
🧠 Learnings (11)📚 Learning: 2025-11-27T09:58:55.346ZApplied to files:
📚 Learning: 2025-11-27T09:58:55.346ZApplied to files:
📚 Learning: 2025-11-27T09:58:55.346ZApplied to files:
📚 Learning: 2025-11-27T09:58:41.875ZApplied to files:
📚 Learning: 2025-11-27T09:58:55.346ZApplied to files:
📚 Learning: 2025-11-27T09:58:41.875ZApplied to files:
📚 Learning: 2025-11-27T09:58:41.875ZApplied to files:
📚 Learning: 2025-11-05T13:45:58.271ZApplied to files:
📚 Learning: 2025-05-07T08:29:38.418ZApplied to files:
📚 Learning: 2025-05-07T08:29:38.418ZApplied to files:
📚 Learning: 2025-02-07T10:16:05.684ZApplied to files:
🧬 Code graph analysis (1)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py (2)
⏰ 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). (10)
✏️ Tip: You can disable this entire section by setting 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: 3
🤖 Fix all issues with AI agents
In
`@packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py`:
- Around line 7-13: The code imports private anyio backend classes
StreamReaderWrapper and StreamWriterWrapper; remove that private import and
either (a) stop using those internal types in nvdemux/driver.py and refactor
AsyncSerial/DemuxerManager usage to rely on public anyio/asyncio stream
primitives or on serial_asyncio.open_serial_connection directly, or (b) if you
cannot refactor now, document and enforce a strict anyio version pin (in
pyproject/requirements) that is known to expose those private classes and add a
runtime guard that raises a clear error if the import fails; update references
to StreamReaderWrapper/StreamWriterWrapper accordingly so code no longer depends
on anyio._backends._asyncio internals.
In
`@packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py`:
- Around line 113-170: The register_driver path calls _notify_if_ready while
holding self._lock which can deadlock if the callback calls back into the
manager; change the code to capture any state needed (e.g., pts_path via
self._pts_map.get(target)) and the callback reference while under the lock, then
release the lock and invoke callback outside the lock (alternatively switch
self._lock to a threading.RLock and ensure all callback invocations happen
without holding the lock). Update register_driver to not call _notify_if_ready
while locked (or make _notify_if_ready not require the lock) so callbacks are
always executed with the lock released.
- Around line 339-378: In _read_demuxer_output the driver_info.callback(target,
pts_path) is invoked while holding self._lock which risks deadlock; modify the
loop so that when a pts_path/target is parsed you update self._pts_map and
self._ready_targets under the lock but instead of calling callbacks there,
collect matching callbacks (e.g., append (driver_id, driver_info.callback) or
just the callback) into a local list while still holding self._lock, release the
lock, then iterate that collected list and call each callback (with target,
pts_path) outside the lock and log exceptions; ensure you still break after
finding the one driver per target and preserve the existing error logging
behavior.
🧹 Nitpick comments (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver_test.py (1)
68-94: Test validates mock setup but not driver's actual pts retrieval.The test calls
mock_manager.get_pts_path()directly on the mock rather than verifying that the driver'sconnect()method properly retrieves the pts path. The comment acknowledges this limitation. Consider adding an integration test or documenting this coverage gap.packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (1)
380-395: Redundant state clearing in _cleanup_demuxer_process.
_read_demuxer_output(lines 376-378) already clears_pts_mapand_ready_targetswhen the output loop exits._cleanup_demuxer_process(lines 393-395) clears them again. This is harmless but redundant.♻️ Remove redundant clearing
def _cleanup_demuxer_process(self): """Clean up the demuxer process and clear ready state.""" if self._process: try: self._process.wait(timeout=1.0) except subprocess.TimeoutExpired: self._process.kill() self._process.wait() exit_code = self._process.returncode logger.info("Demuxer process exited with code %s", exit_code) self._process = None - - with self._lock: - self._pts_map.clear() - self._ready_targets.clear()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-pyserial/README.mdpackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/__init__.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/nvdemux/driver_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py
🧠 Learnings (8)
📚 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/nvdemux/driver_test.pypackages/jumpstarter-driver-pyserial/README.mdpackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_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: After driver creation, implement driver logic in `driver.py`, add tests in `driver_test.py`, update README.md with specific documentation, and test the driver
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver_test.pypackages/jumpstarter-driver-pyserial/README.md
📚 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 `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver_test.pypackages/jumpstarter-driver-pyserial/README.mdpackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.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-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver_test.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.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-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-driver-pyserial/README.mdpackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.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/README.md
📚 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/nvdemux/driver.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_*/driver.py : Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with `*Power`, Network drivers with `*Network`, Flasher drivers with `*Flasher`, Console drivers with `*Console`, Server drivers with `*Server`
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.py (1)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (5)
DemuxerManager(52-395)_has_glob_chars(19-21)_resolve_device(24-40)reset_instance(88-93)get_instance(79-85)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (1)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.py (4)
terminate(84-85)wait(81-82)kill(87-88)readline(67-79)
🪛 LanguageTool
packages/jumpstarter-driver-pyserial/README.md
[grammar] ~46-~46: Use a hyphen to join words.
Context: ... device. ### Configuration #### Single channel example: ```yaml export: ccpl...
(QB_NEW_EN_HYPHEN)
⏰ 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: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
🔇 Additional comments (16)
packages/jumpstarter-driver-pyserial/README.md (1)
36-133: Documentation for NVDemuxSerial is comprehensive and well-structured.The documentation covers all essential aspects: configuration parameters, multi-instance support, auto-detection patterns, and validation requirements. The examples for single and multiple channel configurations are clear and practical.
Minor: Static analysis suggests hyphenating "single-channel" and "multiple-channels" in the section headers (lines 46, 58) for grammatical correctness, but this is optional.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver_test.py (3)
10-36: LGTM! Test correctly verifies driver registration with DemuxerManager.The test properly validates that the driver registers with all expected parameters (driver_id, demuxer_path, device, chip, target) and includes proper cleanup in a finally block.
38-66: LGTM! Callback readiness test is well-designed.The test correctly verifies that the callback provided during registration properly sets the driver's internal
_readyevent.
96-184: LGTM! Remaining tests provide good coverage.The tests properly cover unregistration, timeout behavior, default values, error propagation, and client class. The error propagation test (lines 162-179) works correctly; using
pytest.raiseswould be slightly more idiomatic but the current approach is valid.packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.py (7)
11-19: LGTM! Glob character detection tests are comprehensive.Tests cover asterisk, question mark, bracket patterns, and negative cases with concrete device paths.
34-46: LGTM! Glob pattern resolution test is well-structured.The test creates multiple files, uses a glob pattern, and correctly verifies that the first sorted match is returned.
54-89: MockPopen implementation is adequate for testing.The mock simulates demuxer output behavior well. Note that
wait()ignores the timeout parameter (always returns immediately), which differs from realPopen.wait(). This is acceptable for these tests since the mock process doesn't actually do work.
105-143: LGTM! Single driver registration test is comprehensive.The test properly verifies callback invocation, pts path retrieval, and cleanup. The
time.sleep(0.5)for synchronization is pragmatic for this type of integration test.
145-213: LGTM! Multi-driver sharing test validates key behavior.The test correctly verifies that multiple drivers share a single demuxer process (popen called once), each receives their respective callback, and all pts paths are correctly mapped.
215-377: LGTM! Configuration validation tests are thorough.Tests properly cover all validation scenarios: demuxer path mismatch, device mismatch, chip mismatch, and duplicate target rejection. Each test verifies the expected ValueError message content.
379-484: LGTM! Reference counting and immediate notification tests complete the coverage.The reference counting test validates process lifecycle management (start on first driver, stop on last unregister). The immediate notification test verifies that late-registering drivers for already-ready targets receive callbacks immediately.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py (2)
19-80: Driver initialization with blocking wait is acceptable but worth noting.The
__post_init__blocks up totimeoutseconds waiting for the demuxer to become ready (line 78). This is intentional behavior per the documentation, but callers should be aware that driver instantiation can be slow. The warning log on timeout (line 79) is appropriate since the driver can still function if the device appears later.
104-141: Connect implementation is well-structured.The async context manager correctly:
- Waits for demuxer readiness with proper timeout handling
- Validates pts_path availability
- Opens serial connection with explicit buffer configuration
- Wraps in AsyncSerial for consistent streaming interface
The
limit=1onopen_serial_connection(line 133) creates a minimal read buffer. This pattern is consistently used across both the base serial driver and the demux driver, followed by explicit write buffer limits, indicating intentional design for low-latency serial communication.packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (3)
18-41: Helper functions for device resolution are correct.
_has_glob_charscorrectly identifies glob metacharacters._resolve_deviceproperly handles both glob patterns and direct paths, with appropriate logging for multiple matches.
78-94: Singleton implementation is thread-safe.The double-checked locking pattern is correctly implemented. The
reset_instancemethod properly cleans up resources before nullifying the instance, which is important for testing.
252-298: Monitor loop and device wait implementation are robust.The monitor loop properly handles exceptions with retry and respects shutdown signals. The device wait loop correctly uses
_shutdown.wait()for interruptible polling. The flow through_run_demuxer_cycleensures proper cleanup on each iteration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py
Show resolved
Hide resolved
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py
Show resolved
Hide resolved
Co-authored-by: Claude <[email protected]>
Co-authored-by: GPT 5.2 Codex
mangelajo
force-pushed
the
nvdemux-serial
branch
from
11f1169 to
52b2eb2
Compare
mangelajo
force-pushed
the
nvdemux-serial
branch
from
26d8464 to
e7c4056
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py`:
- Around line 304-320: In _start_demuxer_process change how stderr is handled to
avoid pipe deadlock: either set stderr=subprocess.DEVNULL if you do not need
error output, or set stderr=subprocess.STDOUT to merge stderr into stdout (so
the existing stdout reader handles both), or keep stderr=subprocess.PIPE but
spawn a dedicated background reader thread that drains self._process.stderr
(e.g., start a thread that reads lines from self._process.stderr and logs or
discards them) to ensure the pipe never fills; update the subprocess.Popen call
and, if adding a thread, ensure the thread is started and references
self._process so it stops when the process exits.
🧹 Nitpick comments (2)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (2)
78-85: Double-checked locking pattern has a race condition.The singleton implementation reads
cls._instanceoutside the lock (line 81), which can race with another thread setting it. In Python, this is usually safe due to the GIL, but for correctness and clarity, the outer check should be kept but the pattern should be understood as an optimization, not a guarantee.The current implementation is acceptable for CPython but may have issues in other Python implementations without a GIL.
177-190: Monitor thread continues running even when all drivers unregister.The comment on line 189 indicates this is intentional, but the monitor thread and demuxer process will continue running indefinitely even if all drivers have unregistered. This could be a resource concern if drivers are frequently registered/unregistered.
Consider adding logic to stop the monitor after a grace period when no drivers remain, or document this as expected behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager_test.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/nvdemux/driver.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py
🧠 Learnings (7)
📚 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 `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.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/nvdemux/driver.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_**/*.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/nvdemux/driver.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-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.pypackages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.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-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.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_*/driver.py : Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with `*Power`, Network drivers with `*Network`, Flasher drivers with `*Flasher`, Console drivers with `*Console`, Server drivers with `*Server`
Applied to files:
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.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/nvdemux/driver.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py (1)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (5)
DemuxerManager(52-403)get_instance(79-85)register_driver(121-175)unregister_driver(177-189)get_pts_path(191-204)
⏰ 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). (5)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: build
🔇 Additional comments (17)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py (10)
19-41: LGTM!The helper functions correctly handle glob pattern detection and device resolution with appropriate logging for edge cases.
43-50: LGTM!Clean dataclass definition for driver registration information.
62-77: LGTM!Proper initialization of instance state with appropriate thread synchronization primitives.
95-111: LGTM!Configuration validation properly prevents mismatched demuxer configurations and duplicate target registrations.
121-175: LGTM!The registration logic correctly addresses the previously identified deadlock risk by invoking callbacks outside the lock. State updates are properly protected while allowing safe callback invocation.
227-248: Thread join may hang ifreadlineblocks indefinitely.The monitor thread calls
self._process.stdout.readline()which blocks until data is available or EOF. If_stop_monitoris called whilereadlineis blocking and the process doesn't terminate cleanly, the thread may not respond to the shutdown event until the process finally exits.The current implementation handles this by terminating the process first (lines 232-240), which should cause
readlineto return EOF. However, if process termination fails or the file descriptor isn't properly closed, the 2-second join timeout (line 244) would be reached and the thread may continue running.
256-269: LGTM!The monitor loop has proper error handling with state cleanup and interruptible waiting using
shutdown.wait().
322-341: LGTM!The parser correctly handles tab-separated demuxer output and identifies pts paths and targets with appropriate format detection.
343-386: LGTM!The output reading logic correctly addresses the previously identified deadlock risk. Callbacks are now invoked outside the lock, and state is properly managed.
388-403: LGTM!Process cleanup handles both graceful and forceful termination with appropriate logging.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/driver.py (7)
1-17: LGTM!Imports are appropriate. The private anyio import (
StreamReaderWrapper,StreamWriterWrapper) was previously discussed and accepted as an established pattern in this project.
19-55: LGTM!Well-structured dataclass with appropriate defaults, clear documentation, and proper separation of public configuration fields from internal state.
56-79: LGTM!The initialization correctly registers with the DemuxerManager and handles registration failures. The initial ready wait is appropriately non-blocking with a warning on timeout.
85-93: LGTM!The callback correctly sets the ready event. The design properly handles reconnection scenarios since
connect()always fetches the current pts_path from the manager.
95-102: LGTM!Proper cleanup with idempotent unregistration and base class cleanup.
104-145: LGTM with a minor observation.The
connect()implementation correctly handles both readiness waiting and pts_path retrieval with appropriate timeouts.Minor note: The first wait loop uses a fixed 0.1s sleep interval (line 121) while the second uses
self.poll_interval(line 131). This inconsistency is acceptable since the first loop is checking an in-memory event (fast) while the second involves manager queries, but consider using a consistent approach for clarity.
137-144: LGTM!Serial connection setup with minimal buffering (
limit=1) and explicit write buffer limits is appropriate for real-time serial communication. TheAsyncSerialwrapper properly handles optional CPS throttling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/nvdemux/manager.py
Show resolved
Hide resolved
* Do not wait for ready during driver init * Be more defensive on demuxer process cleanup * Move many log messages to debug * Wait for poll_interval when restarting demuxer always to avoid busy loops
|
@cursor can you review this? |
|
Please finish setting up background agents. Go to Cursor |
| poll_interval: float = field(default=1.0) | ||
|
|
||
| # Internal state (not init params) | ||
| _ready: threading.Event = field(init=False, default_factory=threading.Event) |
There was a problem hiding this comment.
anyio event should be used instead: https://anyio.readthedocs.io/en/stable/synchronization.html#events
| manager = DemuxerManager.get_instance() | ||
| pts_start = time.monotonic() | ||
| pts_path = manager.get_pts_path(str(self.uuid)) | ||
| while not pts_path: |
There was a problem hiding this comment.
How come ready is already set but pts is still not available?
There was a problem hiding this comment.
Hehe, that was not obvious for Claude either, I had to add this.
Because the manager can be ready (it stays up, ready to launch, start, restart the muxer), but the PTS could not be ready (the muxer just went offline, so no ports...) :)
This became important when I started powering off... trying to connect
ir power cycling and trying to connect too quickly, now it just wait until the muxer is up again and the PTS is ready :)
There was a problem hiding this comment.
You were actually right, I was confused by previous implementation, the ready event, and this was redundant, in the end I removed the callback and just kept the polling mechanism of this loop.
github-project-automation
bot
moved this from In review
to Done
in release-0.8 tracking (plus bugs and backports)
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.