Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new jumpstarter-driver-noyito-relay Python package including serial and HID Noyito relay drivers, a NoyitoPowerClient with CLI status, package metadata (pyproject), docs and examples, tests, and a .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Driver as NoyitoPowerSerial<br/>or<br/>NoyitoPowerHID
participant HW as SerialPort<br/>or HIDDevice
Client->>Driver: on()/off()/cycle()
activate Driver
Driver->>Driver: _build_command(channel, state)
Driver->>Driver: _channels()
Driver->>HW: _send_command(bytes)
HW-->>Driver: ack/complete
deactivate Driver
Driver-->>Client: return
Client->>Driver: status()
activate Driver
Driver->>HW: _query_status()
HW-->>Driver: response bytes/text
Driver->>Driver: parse -> dict of CHx:ON/OFF
deactivate Driver
Driver-->>Client: "on"/"off"/"partial"
Client->>Driver: read()
activate Driver
Driver-->>Client: Generator[PowerReading] (NotImplemented)
deactivate Driver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
c717a9b to
10ce9f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-noyito-relay/.gitignore (1)
1-3: LGTM! Appropriate ignore patterns for Python package.The patterns correctly exclude Python bytecode caches and test coverage artifacts, which aligns with the test suite described in the PR.
Optionally, you may consider adding additional common Python patterns if they're not already covered by a root
.gitignore:
.pytest_cache/*.egg-info/dist/build/htmlcov/However, this is perfectly adequate if those are handled at the repository level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/.gitignore` around lines 1 - 3, Current .gitignore correctly excludes __pycache__, .coverage and coverage.xml; optionally add common Python ignore patterns to the same .gitignore (or confirm they're in the root) by including entries for .pytest_cache/, *.egg-info/, dist/, build/, and htmlcov/ so build artifacts and pytest caches are also ignored; update the file named .gitignore in the package (python/packages/jumpstarter-driver-noyito-relay/.gitignore) to add those lines if the repo root does not already cover them.python/packages/jumpstarter-driver-noyito-relay/pyproject.toml (1)
4-4: Package description undersells current functionality.The description mentions only the 2-channel serial board, but this package also ships HID 4/8-channel support. Updating this helps avoid confusion in package indexes.
💡 Proposed tweak
-description = "Jumpstarter driver for the NOYITO 5V 2-Channel USB Relay Module" +description = "Jumpstarter driver for NOYITO USB relay boards (serial 1/2-channel and HID 4/8-channel)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/pyproject.toml` at line 4, Update the package description value in pyproject.toml (the description field) to reflect all supported hardware: change the current "Jumpstarter driver for the NOYITO 5V 2-Channel USB Relay Module" to a broader string that mentions both the 2-channel serial board and HID 4/8-channel support (e.g., include "2-channel serial and HID 4/8-channel" or similar wording) so package indexes correctly describe the package capabilities.python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py (1)
217-312: Add a regression test for HID/status capability mismatch.Please add one integration-style test that verifies
statusis unavailable (or fails with a clear error) forNoyitoPowerHID. It would lock in expected behavior and guard against CLI/client drift.Based on learnings Add comprehensive tests and update documentation in the driver package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py` around lines 217 - 312, Add an integration test that asserts the status capability is not present for NoyitoPowerHID: create a new test function (e.g., test_hid_status_unavailable) in driver_test.py that patches hid.Device (reuse _make_hid_mock and the `@patch`("hid.Device") pattern), opens the device with serve(NoyitoPowerHID(...)) as client, and then assert that either getattr(client, "status", None) is None or that calling client.status() raises a clear exception (AttributeError or NotImplementedError) to lock in expected behavior; reference NoyitoPowerHID, serve, and _make_hid_mock when locating where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/docs/source/reference/package-apis/drivers/noyito-relay.md`:
- Line 1: The docs page "noyito-relay.md" is not included in any Sphinx toctree;
open the drivers toctree index (the drivers reference index used for package
driver docs) and add an entry for "noyito-relay.md" to the toctree so Sphinx
will include it in the generated reference docs. Ensure the toctree entry
matches the document name "noyito-relay.md" (or the same relative doc path used
by other driver entries) and update the drivers index file so the page is
discoverable in navigation.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/conftest.py`:
- Around line 16-20: Replace the direct call to item.runtest() with a pytest
hookwrapper: add `@pytest.hookimpl`(hookwrapper=True) to pytest_runtest_call,
yield to let pytest run the test, capture the outcome via outcome = yield, then
call outcome.get_result() inside a try/except that catches
serial.SerialException and calls pytest.skip("Serial device not available");
ensure serial is imported and remove the manual item.runtest() invocation so
pytest's runner and plugins handle execution and reporting correctly.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`:
- Around line 118-120: The client() classmethod on NoyitoPowerHID incorrectly
returns the status-capable
"jumpstarter_driver_noyito_relay.client.NoyitoPowerClient" causing CLI to expose
a status command for HID; update NoyitoPowerHID.client() to return the proper
HID-only client (or the module/class that does not export status) — e.g. point
it at the HID-specific client class name (the one that does not implement
status) instead of NoyitoPowerClient, or alternatively implement/export a safe
status() on NoyitoPowerHID if status behavior is required.
---
Nitpick comments:
In `@python/packages/jumpstarter-driver-noyito-relay/.gitignore`:
- Around line 1-3: Current .gitignore correctly excludes __pycache__, .coverage
and coverage.xml; optionally add common Python ignore patterns to the same
.gitignore (or confirm they're in the root) by including entries for
.pytest_cache/, *.egg-info/, dist/, build/, and htmlcov/ so build artifacts and
pytest caches are also ignored; update the file named .gitignore in the package
(python/packages/jumpstarter-driver-noyito-relay/.gitignore) to add those lines
if the repo root does not already cover them.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py`:
- Around line 217-312: Add an integration test that asserts the status
capability is not present for NoyitoPowerHID: create a new test function (e.g.,
test_hid_status_unavailable) in driver_test.py that patches hid.Device (reuse
_make_hid_mock and the `@patch`("hid.Device") pattern), opens the device with
serve(NoyitoPowerHID(...)) as client, and then assert that either
getattr(client, "status", None) is None or that calling client.status() raises a
clear exception (AttributeError or NotImplementedError) to lock in expected
behavior; reference NoyitoPowerHID, serve, and _make_hid_mock when locating
where to add the test.
In `@python/packages/jumpstarter-driver-noyito-relay/pyproject.toml`:
- Line 4: Update the package description value in pyproject.toml (the
description field) to reflect all supported hardware: change the current
"Jumpstarter driver for the NOYITO 5V 2-Channel USB Relay Module" to a broader
string that mentions both the 2-channel serial board and HID 4/8-channel support
(e.g., include "2-channel serial and HID 4/8-channel" or similar wording) so
package indexes correctly describe the package capabilities.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
python/docs/source/reference/package-apis/drivers/noyito-relay.mdpython/packages/jumpstarter-driver-noyito-relay/.gitignorepython/packages/jumpstarter-driver-noyito-relay/README.mdpython/packages/jumpstarter-driver-noyito-relay/examples/exporter.yamlpython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/__init__.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/client.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/conftest.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.pypython/packages/jumpstarter-driver-noyito-relay/pyproject.toml
| @@ -0,0 +1 @@ | |||
| ../../../../../packages/jumpstarter-driver-noyito-relay/README.md No newline at end of file | |||
There was a problem hiding this comment.
Docs page is currently orphaned from Sphinx navigation.
The docs pipeline warning shows this document is not included in any toctree, so it won’t be discoverable in generated reference docs. Please add it to the appropriate drivers toctree index.
🧰 Tools
🪛 GitHub Actions: Documentation
[warning] 1-1: Sphinx warning: document isn't included in any toctree
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/docs/source/reference/package-apis/drivers/noyito-relay.md` at line 1,
The docs page "noyito-relay.md" is not included in any Sphinx toctree; open the
drivers toctree index (the drivers reference index used for package driver docs)
and add an entry for "noyito-relay.md" to the toctree so Sphinx will include it
in the generated reference docs. Ensure the toctree entry matches the document
name "noyito-relay.md" (or the same relative doc path used by other driver
entries) and update the drivers index file so the page is discoverable in
navigation.
| def pytest_runtest_call(item): | ||
| try: | ||
| item.runtest() | ||
| except serial.SerialException: | ||
| pytest.skip("Serial device not available") # ty: ignore[call-non-callable] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual conftest.py file
cat -n python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/conftest.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 871
🏁 Script executed:
# Run the verification script from the review comment
#!/bin/bash
set -euo pipefail
echo "Custom pytest_runtest_call hooks:"
rg -n --type=py 'def pytest_runtest_call\('
echo
echo "Direct calls to item.runtest() (should be absent in custom hooks):"
rg -n --type=py 'item\.runtest\s*\('
echo
echo "All pytest hook declarations:"
rg -n --type=py 'def pytest_' python/packages/jumpstarter-driver-noyito-relay/Repository: jumpstarter-dev/jumpstarter
Length of output: 1013
🌐 Web query:
pytest_runtest_call hook semantics documentation
💡 Result:
pytest_runtest_call(item) is a runtest-phase hook that pytest calls to execute the “call” phase of a single test item (i.e., the actual test body, after setup and before teardown). Its default implementation is essentially to run the test via item.runtest(). [1]
Where it fits in the per-item runtest protocol:
pytest_runtest_logstart(...)happens before setup/call/teardown. [2]pytest_runtest_setup(item)runs beforepytest_runtest_call(item). [2]pytest_runtest_call(item)runs the test body (call phase). [1]pytest_runtest_teardown(item, nextitem)runs teardown after the call phase. [1]pytest_runtest_logfinish(...)happens after setup/call/teardown. [2]
Implementation/usage notes:
- You can implement this hook in
conftest.py(subject to the usual conftest visibility rules). [1] - Unlike most non-
pytest_runtest_*hooks, runtest hooks are allowed to raise exceptions as part of reporting test failures/errors. [3]
Docs: see the pytest “API Reference” entry for pytest_runtest_call. [1]
Sources:
[1] pytest docs – API Reference (pytest_runtest_call) (docs.pytest.org)
[2] pytest docs – runtest hook order (logstart, setup, call, teardown, logfinish) (docs.pytest.org)
[3] pytest docs – hook-writing notes (exception rules; runtest hooks special) (docs.pytest.org)
Citations:
- 1: https://docs.pytest.org/en/stable/reference/reference.html?utm_source=openai
- 2: https://docs.pytest.org/en/4.6.x/reference.html?utm_source=openai
- 3: https://docs.pytest.org/en/7.1.x/how-to/writing_hook_functions.html?utm_source=openai
Implement pytest_runtest_call using the hookwrapper pattern to properly integrate with pytest's test execution protocol.
The current implementation directly calls item.runtest() without using @pytest.hookimpl(hookwrapper=True) and yield. This violates pytest's runtest hook semantics, which expects hooks to yield control back to pytest's runner rather than manually executing the test. Improper exception handling also prevents integration with pytest's error reporting and other plugins.
💡 Proposed fix
+@pytest.hookimpl(hookwrapper=True)
def pytest_runtest_call(item):
- try:
- item.runtest()
- except serial.SerialException:
- pytest.skip("Serial device not available") # ty: ignore[call-non-callable]
+ outcome = yield
+ if outcome.excinfo and issubclass(outcome.excinfo[0], serial.SerialException):
+ pytest.skip("Serial device not available")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/conftest.py`
around lines 16 - 20, Replace the direct call to item.runtest() with a pytest
hookwrapper: add `@pytest.hookimpl`(hookwrapper=True) to pytest_runtest_call,
yield to let pytest run the test, capture the outcome via outcome = yield, then
call outcome.get_result() inside a try/except that catches
serial.SerialException and calls pytest.skip("Serial device not available");
ensure serial is imported and remove the manual item.runtest() invocation so
pytest's runner and plugins handle execution and reporting correctly.
| @classmethod | ||
| def client(cls) -> str: | ||
| return "jumpstarter_driver_noyito_relay.client.NoyitoPowerClient" |
There was a problem hiding this comment.
NoyitoPowerHID points to a status-capable client, but HID has no status export.
NoyitoPowerHID.client() currently returns NoyitoPowerClient, which adds status()/status CLI command. That command can be invoked for HID and then fail at runtime because NoyitoPowerHID does not implement/export status.
💡 Proposed fix
`@classmethod`
def client(cls) -> str:
- return "jumpstarter_driver_noyito_relay.client.NoyitoPowerClient"
+ return "jumpstarter_driver_power.client.PowerClient"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`
around lines 118 - 120, The client() classmethod on NoyitoPowerHID incorrectly
returns the status-capable
"jumpstarter_driver_noyito_relay.client.NoyitoPowerClient" causing CLI to expose
a status command for HID; update NoyitoPowerHID.client() to return the proper
HID-only client (or the module/class that does not export status) — e.g. point
it at the HID-specific client class name (the one that does not implement
status) instead of NoyitoPowerClient, or alternatively implement/export a safe
status() on NoyitoPowerHID if status behavior is required.
There was a problem hiding this comment.
Ahh, the status part is missing in this variant of the driver, can it be implemented?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
NOYITO USB HID relay board status query support
💡 Result:
Many “NOYITO USB HID relay” boards are re-branded LCUS/CH-style HID relays that use the simple 4‑byte 0xA0 command protocol (often sent to the device’s HID endpoint / hidraw). A Linux user report for LCUS_HID_X1 shows the NOYITO/LCUS HID variant accepts the same A0 … on/off frames over HID. [2]
Does it support relay status queries?
Yes—if your NOYITO board uses this 0xA0 protocol, it supports a query operation where the 3rd byte is 0x02 (“query status”), and the device returns text like CH1:ON / CH1:OFF. [1]
Per-channel status query
Send:
A0 <CH> 02 <CHK>
Examples:
- CH1 status:
A0 01 02 A3→ response likeCH1:ON(ASCII) [1] - CH2 status:
A0 02 02 A4→ response likeCH2:ON(ASCII) [1]
All-channel status query
Send:
A0 0F 02 B1→ returns multiple lines (CH1:...,CH2:..., etc.) [1]
Checksum
CHK = (0xA0 + address + operation) & 0xFF (matches the examples above). [1]
Quick way to confirm on your system
If your board is supported by common HID-relay tooling, the usbrelay utility can query and print relay state (with no action it prints all relays). [3]
Sources: Diustou relay protocol + query/response examples [1], LCUS_HID_X1 controlled via A0 ... frames over HID (hidraw) [2], usbrelay tool supports querying HID relay state [3].
@mangelajo Yes, status can be implemented for the HID variant!
The web search confirms that NOYITO HID relay boards using the 0xA0 protocol support status queries:
Protocol for status query:
- Send:
0xA0 <channel> 0x02 <checksum>where checksum =(0xA0 + channel + 0x02) & 0xFF - For single channel:
0xA0 0x01 0x02 0xA3→ returns ASCII"CH1:ON"or"CH1:OFF" - For all channels:
0xA0 0x0F 0x02 0xB1→ returns multiple lines (CH1:...,CH2:..., etc.)
Implementation approach:
The existing code already has most pieces needed:
- The
_build_command(channel, state)helper can be used withstate=0x02for queries - The response format is identical to the Serial variant (ASCII text like
"CH1:ON") - You can adapt the
_query_status()andstatus()methods fromNoyitoPowerSerial(lines 51-96)
The main difference is sending/receiving via HID instead of serial. The _send_command method in NoyitoPowerHID would need a companion _query_status() that:
- Opens the HID device
- Sends the status query command
- Reads the HID response (ASCII text)
- Parses it the same way as the Serial variant
This would also resolve the original issue—once status() is exported on NoyitoPowerHID, the NoyitoPowerClient client becomes appropriate for both driver variants.
Would you like me to plan the implementation of the status query for the HID driver?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
just a couple of warnings from the bot, and from docs compilation. |
| from jumpstarter_driver_power.client import PowerClient | ||
|
|
||
|
|
||
| class NoyitoPowerClient(PowerClient): |
There was a problem hiding this comment.
I would go ahead and add status and cli to all PowerClients, defaulting to "not implemented", then we can add it to others as well. WDYT?
There was a problem hiding this comment.
Saying because we have been talking about that sometimes. ... anyway, we can do that later TBH, not really this PR, please ignore :D
There was a problem hiding this comment.
@mangelajo yeah, I think this makes sense, let's just throw NotImplemented for this driver for now.
|
|
||
| @export | ||
| def read(self) -> Generator[PowerReading, None, None]: | ||
| yield PowerReading(voltage=0.0, current=0.0) |
There was a problem hiding this comment.
Here I think we should throw a non implemented exception
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py (2)
82-85: Inconsistentread()behavior between Serial and HID drivers.
NoyitoPowerSerial.read()raisesNotImplementedError, whileNoyitoPowerHID.read()(line 187-188) yields a placeholderPowerReading(voltage=0.0, current=0.0). For consistent API behavior, both should handle "not supported" the same way.Consider either:
- Both raise
NotImplementedError, or- Both yield a placeholder reading
♻️ Option: Make Serial match HID's placeholder approach
`@export` def read(self) -> Generator[PowerReading, None, None]: - # Power reading not supported - raise NotImplementedError + # Power reading not supported; yield placeholder + yield PowerReading(voltage=0.0, current=0.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py` around lines 82 - 85, The NoyitoPowerSerial.read implementation currently raises NotImplementedError while NoyitoPowerHID.read yields a placeholder PowerReading; make them consistent by changing NoyitoPowerSerial.read to also yield a placeholder PowerReading(voltage=0.0, current=0.0) (i.e., return a Generator that yields the same placeholder) so both drivers present the same API behavior; update any docstring/comments for NoyitoPowerSerial.read to note that power reading is not supported and a zero-value placeholder is emitted.
135-147: Consider extracting macOS library path setup to avoid duplication.The DYLD_FALLBACK_LIBRARY_PATH handling in
_send_command(lines 139-144) and_query_status(lines 162-167) is identical. Extracting this to a helper would improve maintainability.♻️ Proposed refactor
+def _ensure_hidapi_path() -> None: + """Extend DYLD_FALLBACK_LIBRARY_PATH for Homebrew hidapi on macOS.""" + if sys.platform != "darwin": + return + _brew_lib = os.path.join(os.environ.get("HOMEBREW_PREFIX", "/opt/homebrew"), "lib") + if os.path.isdir(_brew_lib): + _fallback = os.environ.get("DYLD_FALLBACK_LIBRARY_PATH", "") + if _brew_lib not in _fallback.split(":"): + os.environ["DYLD_FALLBACK_LIBRARY_PATH"] = _brew_lib + (":" + _fallback if _fallback else "") + + `@dataclass`(kw_only=True) class NoyitoPowerHID(PowerInterface, Driver): ... def _send_command(self, cmd: bytes) -> None: - # On Apple Silicon Macs, Homebrew installs hidapi to /opt/homebrew/lib - # which is not in ctypes's default search path. Extend - # DYLD_FALLBACK_LIBRARY_PATH before the first import so dlopen finds it. - if sys.platform == "darwin": - _brew_lib = os.path.join(os.environ.get("HOMEBREW_PREFIX", "/opt/homebrew"), "lib") - if os.path.isdir(_brew_lib): - _fallback = os.environ.get("DYLD_FALLBACK_LIBRARY_PATH", "") - if _brew_lib not in _fallback.split(":"): - os.environ["DYLD_FALLBACK_LIBRARY_PATH"] = _brew_lib + (":" + _fallback if _fallback else "") + _ensure_hidapi_path() import hid # noqa: PLC0415 ... def _query_status(self) -> dict[str, str]: - if sys.platform == "darwin": - _brew_lib = os.path.join(os.environ.get("HOMEBREW_PREFIX", "/opt/homebrew"), "lib") - if os.path.isdir(_brew_lib): - _fallback = os.environ.get("DYLD_FALLBACK_LIBRARY_PATH", "") - if _brew_lib not in _fallback.split(":"): - os.environ["DYLD_FALLBACK_LIBRARY_PATH"] = _brew_lib + (":" + _fallback if _fallback else "") + _ensure_hidapi_path() import hid # noqa: PLC0415 ...Also applies to: 161-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py` around lines 135 - 147, Duplicate macOS DYLD_FALLBACK_LIBRARY_PATH setup appears in _send_command and _query_status; extract that logic into a small helper (e.g., _ensure_macos_hid_library_path or extend_dyld_fallback) that checks sys.platform == "darwin", computes HOMEBREW_PREFIX + "/lib", verifies the directory exists and that it is not already in DYLD_FALLBACK_LIBRARY_PATH, then updates the environment variable; call this helper from both _send_command and _query_status immediately before importing hid and remove the duplicated block in each method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py`:
- Around line 100-110: The test fails because NoyitoPowerSerial.read currently
raises NotImplementedError while test_read expects a PowerReading with
voltage=0.0 and current=0.0; fix by implementing NoyitoPowerSerial.read in
driver.py to match HID behavior: return/iterate a reading placeholder (e.g.,
yield a PowerReading or equivalent object with voltage=0.0 and current=0.0)
instead of raising, ensuring the method signature and return type match existing
tests and the HID implementation so test_read (which calls list(client.read()))
receives one reading; alternatively, if you prefer to keep NotImplementedError,
update the test_read to assert that client.read() raises NotImplementedError
(reference NoyitoPowerSerial.read and test_read).
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`:
- Around line 82-85: The NoyitoPowerSerial.read implementation currently raises
NotImplementedError while NoyitoPowerHID.read yields a placeholder PowerReading;
make them consistent by changing NoyitoPowerSerial.read to also yield a
placeholder PowerReading(voltage=0.0, current=0.0) (i.e., return a Generator
that yields the same placeholder) so both drivers present the same API behavior;
update any docstring/comments for NoyitoPowerSerial.read to note that power
reading is not supported and a zero-value placeholder is emitted.
- Around line 135-147: Duplicate macOS DYLD_FALLBACK_LIBRARY_PATH setup appears
in _send_command and _query_status; extract that logic into a small helper
(e.g., _ensure_macos_hid_library_path or extend_dyld_fallback) that checks
sys.platform == "darwin", computes HOMEBREW_PREFIX + "/lib", verifies the
directory exists and that it is not already in DYLD_FALLBACK_LIBRARY_PATH, then
updates the environment variable; call this helper from both _send_command and
_query_status immediately before importing hid and remove the duplicated block
in each method.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
python/docs/source/reference/package-apis/drivers/index.mdpython/packages/jumpstarter-driver-noyito-relay/README.mdpython/packages/jumpstarter-driver-noyito-relay/examples/exporter.yamlpython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter-driver-noyito-relay/README.md
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py (2)
312-316:_encode_statuscan return overlength payloads, which masks report-size bugs.When
textexceeds 32 bytes, the helper returns all bytes instead of enforcing a fixed report size. That makes mocks less realistic and can hide truncation issues.💡 Proposed fix
-def _encode_status(text: str) -> list[int]: - """Encode an ASCII status string into a 32-byte HID read buffer.""" - raw = list(text.encode("ascii")) - return raw + [0] * (32 - len(raw)) +def _encode_status(text: str, report_size: int = 64) -> list[int]: + """Encode ASCII status into a fixed-size HID read buffer.""" + raw = list(text.encode("ascii")) + if len(raw) > report_size: + raise ValueError( + f"status payload ({len(raw)} bytes) exceeds report_size={report_size}" + ) + return raw + [0] * (report_size - len(raw))Based on learnings: Each driver package must include comprehensive tests in
driver_test.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py` around lines 312 - 316, The helper _encode_status currently returns the full encoded bytes when text is longer than 32 bytes which masks report-size bugs; change it so the function enforces a fixed 32-byte HID report by truncating the ASCII-encoded bytes to at most 32 bytes and then padding with zeros to exactly 32 elements (or alternatively raise a clear error if you prefer strictness), ensuring _encode_status always returns a list[int] of length 32 and still uses text.encode("ascii") for encoding.
342-363: Add an 8-channel HID status test to cover advertised support.Status parsing is only exercised for 4-channel boards right now. Adding an 8-channel case will protect the most payload-heavy path.
💡 Proposed test addition
+@patch("hid.Device") +def test_hid_status_all_on_8ch(mock_hid_cls): + mock_dev = _make_hid_mock() + mock_dev.read.return_value = _encode_status( + "CH1:ON\r\nCH2:ON\r\nCH3:ON\r\nCH4:ON\r\nCH5:ON\r\nCH6:ON\r\nCH7:ON\r\nCH8:ON\r\n" + ) + mock_hid_cls.return_value = mock_dev + + with serve(NoyitoPowerHID(num_channels=8, all_channels=True)) as client: + assert client.status() == "on"Based on learnings: Each driver package must include comprehensive tests in
driver_test.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py` around lines 342 - 363, Add two tests mirroring the 4-channel cases to exercise 8-channel parsing: create test_hid_status_all_on_8ch and test_hid_status_partial_8ch that use _make_hid_mock() and set mock_dev.read.return_value = _encode_status("CH1:ON\r\n...CH8:ON\r\n") (and a mixed ON/OFF string for the partial case), patch hid.Device the same way, instantiate with serve(NoyitoPowerHID(num_channels=8, all_channels=True)), assert client.status() returns "on" or "partial" respectively, and assert mock_dev.write was called with the same report prefix (b"\x00") plus the correct bytes payload for the 8-channel query (use the same write-assert pattern as the 4ch tests but adjust the channel mask/payload to match 8-channel behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py`:
- Around line 100-107: Update the test_read unit test to assert the specific
exception type instead of the broad Exception: when calling list(client.read())
inside the serve(NoyitoPowerSerial(...)) context, replace
pytest.raises(Exception) with pytest.raises(NotImplementedError) so the test
expects NotImplementedError from NoyitoPowerSerial.read; keep the existing
serial mock setup (_make_serial_mock and patched
jumpstarter_driver_noyito_relay.driver.serial.Serial) unchanged.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`:
- Around line 170-184: The HID read is too small (32 bytes) causing truncated
payloads in the status() flow; update the read logic in the method that calls
_build_command to either request the full maximum report length (e.g., change
device.read(32, timeout_ms=2000) to device.read(72, timeout_ms=2000) to cover
8-channel boards) or implement a loop that repeatedly calls device.read(...) and
appends results until an empty/timeout read signals end-of-report, then decode
the concatenated bytes as before; ensure you keep references to _build_command
and status (or the enclosing method) so the change is made in the correct place.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py`:
- Around line 312-316: The helper _encode_status currently returns the full
encoded bytes when text is longer than 32 bytes which masks report-size bugs;
change it so the function enforces a fixed 32-byte HID report by truncating the
ASCII-encoded bytes to at most 32 bytes and then padding with zeros to exactly
32 elements (or alternatively raise a clear error if you prefer strictness),
ensuring _encode_status always returns a list[int] of length 32 and still uses
text.encode("ascii") for encoding.
- Around line 342-363: Add two tests mirroring the 4-channel cases to exercise
8-channel parsing: create test_hid_status_all_on_8ch and
test_hid_status_partial_8ch that use _make_hid_mock() and set
mock_dev.read.return_value = _encode_status("CH1:ON\r\n...CH8:ON\r\n") (and a
mixed ON/OFF string for the partial case), patch hid.Device the same way,
instantiate with serve(NoyitoPowerHID(num_channels=8, all_channels=True)),
assert client.status() returns "on" or "partial" respectively, and assert
mock_dev.write was called with the same report prefix (b"\x00") plus the correct
bytes payload for the 8-channel query (use the same write-assert pattern as the
4ch tests but adjust the channel mask/payload to match 8-channel behavior).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py
| @patch("jumpstarter_driver_noyito_relay.driver.serial.Serial") | ||
| def test_read(mock_serial_cls): | ||
| mock_ser = _make_serial_mock() | ||
| mock_serial_cls.return_value = mock_ser | ||
|
|
||
| with serve(NoyitoPowerSerial(port="/dev/ttyUSB0", channel=1)) as client: | ||
| with pytest.raises(Exception): | ||
| list(client.read()) |
There was a problem hiding this comment.
Use a specific exception type in pytest.raises (Ruff B017 failure).
This currently fails lint/pipeline. Catch NotImplementedError explicitly instead of Exception.
💡 Proposed fix
-@patch("jumpstarter_driver_noyito_relay.driver.serial.Serial")
-def test_read(mock_serial_cls):
+@patch("jumpstarter_driver_noyito_relay.driver.serial.Serial")
+def test_read_not_supported(mock_serial_cls):
mock_ser = _make_serial_mock()
mock_serial_cls.return_value = mock_ser
with serve(NoyitoPowerSerial(port="/dev/ttyUSB0", channel=1)) as client:
- with pytest.raises(Exception):
+ with pytest.raises(NotImplementedError):
list(client.read())🧰 Tools
🪛 GitHub Actions: Linters
[error] 106-106: Ruff: B017 Do not assert blind exception: Exception.
🪛 GitHub Check: lint-python
[failure] 106-106: ruff (B017)
python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py:106:14: B017 Do not assert blind exception: Exception
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py`
around lines 100 - 107, Update the test_read unit test to assert the specific
exception type instead of the broad Exception: when calling list(client.read())
inside the serve(NoyitoPowerSerial(...)) context, replace
pytest.raises(Exception) with pytest.raises(NotImplementedError) so the test
expects NotImplementedError from NoyitoPowerSerial.read; keep the existing
serial mock setup (_make_serial_mock and patched
jumpstarter_driver_noyito_relay.driver.serial.Serial) unchanged.
| cmd = _build_command(0x0F, 0x02) # 0x0F = all-channels status query pseudo-channel | ||
| with hid.Device(self.vendor_id, self.product_id) as device: | ||
| device.write(b"\x00" + cmd) | ||
| raw = device.read(32, timeout_ms=2000) | ||
|
|
||
| text = bytes(raw).decode("ascii", errors="replace") | ||
| result: dict[str, str] = {} | ||
| for line in text.split("\n"): | ||
| line = line.strip("\r").strip() | ||
| if ":" in line: | ||
| key, _, value = line.partition(":") | ||
| result[key.strip()] = value.strip() | ||
| if not result: | ||
| raise ValueError(f"Unexpected status response: {text!r}") | ||
| return result |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and display the _query_status method implementation
echo "=== Finding _query_status method ==="
rg -n "_query_status" python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py -A 15
echo ""
echo "=== Checking device.read calls ==="
rg -n "device\.read\(" python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py
echo ""
echo "=== Checking _build_command(0x0F) ==="
rg -n "_build_command\(0x0F" python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py
echo ""
echo "=== Calculating status payload sizes ==="
python - <<'PY'
samples = {
"4ch_on": "".join(f"CH{i}:ON\r\n" for i in range(1, 5)),
"4ch_off": "".join(f"CH{i}:OFF\r\n" for i in range(1, 5)),
"8ch_on": "".join(f"CH{i}:ON\r\n" for i in range(1, 9)),
"8ch_off": "".join(f"CH{i}:OFF\r\n" for i in range(1, 9)),
}
for name, payload in samples.items():
print(f"{name}: {len(payload)} bytes")
print(f" Content: {repr(payload)}")
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 3482
🏁 Script executed:
# Check the full driver.py file to understand context and lines 170-184
echo "=== Reading driver.py to find exact implementation ==="
wc -l python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 219
🏁 Script executed:
# Display lines 160-200 to see full context
echo "=== Lines 160-200 for context ==="
sed -n '160,200p' python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py | cat -nRepository: jumpstarter-dev/jumpstarter
Length of output: 2154
🏁 Script executed:
# Check the status() method to see how it uses _query_status result
echo "=== Finding status() method ==="
rg -n "def status" python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py -A 20Repository: jumpstarter-dev/jumpstarter
Length of output: 1461
HID status read is undersized and causes truncation for 4-channel OFF states and all 8-channel boards.
Line 173 reads exactly 32 bytes once. The 4-channel OFF status payload is 36 bytes and 8-channel payloads range from 64-72 bytes, causing truncation that drops channel keys and triggers ValueError: Channel X not found in status response failures in the status() method.
💡 Proposed fix
cmd = _build_command(0x0F, 0x02) # 0x0F = all-channels status query pseudo-channel
with hid.Device(self.vendor_id, self.product_id) as device:
device.write(b"\x00" + cmd)
- raw = device.read(32, timeout_ms=2000)
+ raw = b""
+ for _ in range(4):
+ chunk = bytes(device.read(64, timeout_ms=500))
+ if not chunk:
+ break
+ raw += chunk
+ decoded = raw.replace(b"\x00", b"").decode("ascii", errors="replace")
+ if all(f"CH{i}:" in decoded for i in range(1, self.num_channels + 1)):
+ break
- text = bytes(raw).decode("ascii", errors="replace")
+ text = raw.replace(b"\x00", b"").decode("ascii", errors="replace")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`
around lines 170 - 184, The HID read is too small (32 bytes) causing truncated
payloads in the status() flow; update the read logic in the method that calls
_build_command to either request the full maximum report length (e.g., change
device.read(32, timeout_ms=2000) to device.read(72, timeout_ms=2000) to cover
8-channel boards) or implement a loop that repeatedly calls device.read(...) and
appends results until an empty/timeout read signals end-of-report, then decode
the concatenated bytes as before; ensure you keep references to _build_command
and status (or the enclosing method) so the change is made in the correct place.
There was a problem hiding this comment.
@coderabbitai, about your proposal, wouldn't it make more sense to try to read exactly what we expect it to be based on the number of channels and then set a lower timeout so reads are fast?
There was a problem hiding this comment.
@mangelajo, that's a much cleaner approach! Since we know num_channels ahead of time, we can compute the exact worst-case byte count:
- Per channel:
"CH{n}:OFF\r\n"= 9 bytes (worst case, when all channels are OFF) - Max for 4-ch:
4 × 9 = 36 bytes - Max for 8-ch:
8 × 9 = 72 bytes
So the formula max_bytes = self.num_channels * 9 gives a tight upper bound, and we can pair it with a shorter timeout:
cmd = _build_command(0x0F, 0x02) # 0x0F = all-channels status query pseudo-channel
with hid.Device(self.vendor_id, self.product_id) as device:
device.write(b"\x00" + cmd)
- raw = device.read(32, timeout_ms=2000)
+ raw = device.read(self.num_channels * 9, timeout_ms=500)
- text = bytes(raw).decode("ascii", errors="replace")
+ text = bytes(raw).decode("ascii", errors="replace")This is simpler and more deterministic — no loop, no early-exit logic, and the timeout can be kept low since we know the exact payload size to expect. The only caveat is that hid.Device.read() returns one HID report at a time (typically ≤ 64 bytes), so if the firmware splits the 8-channel response across two reports we'd still need two reads. If the hardware is known to fit its response into a single report, the single-read approach is perfectly fine.
|
The linter complains about Exception handling |
…annels simultaneously
- Fix Ruff B017: use pytest.raises(NotImplementedError) instead of pytest.raises(Exception) - Change NoyitoPowerHID.read() to raise NotImplementedError (matching Serial driver behavior, as agreed with reviewers) - Update _query_status to use exact byte count (num_channels * 9) with a shorter 500ms timeout instead of fixed 32-byte / 2000ms read - Update tests to match new behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6d2993a to
6558931
Compare
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@kirkbrauer I asked ambient to rebase and address the comments, please feel free to drop the last commit if it doesn't make sense. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py (1)
314-359: Please cover the 8-channel HID status path.The HID status tests stop at
num_channels=4, so the only branch that expands the status-read size for 8-channel boards is still untested. Adding onenum_channels=8status case here would protect the longest parsing path and pin the expectedread(72, timeout_ms=500)call.🧪 Suggested test shape
`@patch`("hid.Device") +def test_hid_status_all_on_8ch(mock_hid_cls): + mock_dev = _make_hid_mock() + mock_dev.read.return_value = _encode_status( + "CH1:ON\r\nCH2:ON\r\nCH3:ON\r\nCH4:ON\r\n" + "CH5:ON\r\nCH6:ON\r\nCH7:ON\r\nCH8:ON\r\n" + ) + mock_hid_cls.return_value = mock_dev + + with serve(NoyitoPowerHID(num_channels=8, all_channels=True)) as client: + assert client.status() == "on" + + mock_dev.read.assert_called_once_with(72, timeout_ms=500) + mock_dev.write.assert_called_once_with(b"\x00" + bytes([0xA0, 0x0F, 0x02, 0xB1]))As per coding guidelines
python/packages/jumpstarter-driver-*/jumpstarter_driver_*/driver_test.py: Each driver package must include comprehensive tests indriver_test.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py` around lines 314 - 359, Add a new test covering the 8-channel HID status path: create a test (e.g., test_hid_status_all_on_8ch) that patches hid.Device, uses _make_hid_mock and sets mock_dev.read.return_value = _encode_status with 8 channel lines (e.g., "CH1:ON\r\n...CH8:ON\r\n"), instantiate serve(NoyitoPowerHID(num_channels=8, all_channels=True)) and assert client.status() == "on", and assert mock_dev.write.assert_called_once_with the same request bytes used in the other tests (b"\x00" + bytes([0xA0, 0x0F, 0x02, 0xB1])); this will exercise the 8-channel parsing branch and pin the larger read size behavior.python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py (1)
30-31: Don’t promise simultaneous switching here.
all_channelsis implemented as one command per channel, so the current “simultaneously/high-current” wording overstates what the driver can guarantee. Either switch through a single device-level broadcast command or document this as sequential best-effort.Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py` around lines 30 - 31, The comment overstates that setting all_channels=True in the exporter config will switch channels simultaneously; update driver.py to either (A) implement a true device-level broadcast by calling the device's broadcast API (e.g. use the device-level broadcast/send_broadcast_command routine instead of per-channel commands) so all channels are switched in one atomic command, or (B) if broadcast is not available, change the wording in the exporter config docs (and any log/messages) to state that all_channels triggers sequential, best-effort per-channel switches (implemented via the existing per-channel loop/function that sends individual switch commands), and add a short note referencing that this is not atomic/high-current safe; ensure you update both occurrences referencing all_channels (the doc string around Set ``all_channels=True`` and the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`:
- Around line 71-85: The methods on, off, and read in NoyitoPowerSerial and
NoyitoPowerHID currently are synchronous but must match the PowerInterface async
signatures; change their definitions to async def on(self), async def off(self),
and async def read(self) (returning an async generator/AsyncGenerator for
PowerReading), and inside await any asynchronous helpers (e.g., await
self._send_command(_build_command(ch, 1/0)) and await any async iteration over
self._channels() or async I/O in read) so the implementations of on/off/read in
those classes conform to the interface and pass static typing.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.py`:
- Around line 314-359: Add a new test covering the 8-channel HID status path:
create a test (e.g., test_hid_status_all_on_8ch) that patches hid.Device, uses
_make_hid_mock and sets mock_dev.read.return_value = _encode_status with 8
channel lines (e.g., "CH1:ON\r\n...CH8:ON\r\n"), instantiate
serve(NoyitoPowerHID(num_channels=8, all_channels=True)) and assert
client.status() == "on", and assert mock_dev.write.assert_called_once_with the
same request bytes used in the other tests (b"\x00" + bytes([0xA0, 0x0F, 0x02,
0xB1])); this will exercise the 8-channel parsing branch and pin the larger read
size behavior.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`:
- Around line 30-31: The comment overstates that setting all_channels=True in
the exporter config will switch channels simultaneously; update driver.py to
either (A) implement a true device-level broadcast by calling the device's
broadcast API (e.g. use the device-level broadcast/send_broadcast_command
routine instead of per-channel commands) so all channels are switched in one
atomic command, or (B) if broadcast is not available, change the wording in the
exporter config docs (and any log/messages) to state that all_channels triggers
sequential, best-effort per-channel switches (implemented via the existing
per-channel loop/function that sends individual switch commands), and add a
short note referencing that this is not atomic/high-current safe; ensure you
update both occurrences referencing all_channels (the doc string around Set
``all_channels=True`` and the other occurrence).
🪄 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: 48775e87-7d94-41e7-81ff-42f1d04bf322
📒 Files selected for processing (11)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/noyito-relay.mdpython/packages/jumpstarter-driver-noyito-relay/.gitignorepython/packages/jumpstarter-driver-noyito-relay/README.mdpython/packages/jumpstarter-driver-noyito-relay/examples/exporter.yamlpython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/__init__.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/client.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/conftest.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.pypython/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver_test.pypython/packages/jumpstarter-driver-noyito-relay/pyproject.toml
✅ Files skipped from review due to trivial changes (5)
- python/packages/jumpstarter-driver-noyito-relay/.gitignore
- python/docs/source/reference/package-apis/drivers/noyito-relay.md
- python/docs/source/reference/package-apis/drivers/index.md
- python/packages/jumpstarter-driver-noyito-relay/README.md
- python/packages/jumpstarter-driver-noyito-relay/examples/exporter.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/conftest.py
- python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/client.py
- python/packages/jumpstarter-driver-noyito-relay/pyproject.toml
| def on(self) -> None: | ||
| for ch in self._channels(): | ||
| self.logger.info("Relay channel %d ON", ch) | ||
| self._send_command(_build_command(ch, 1)) | ||
|
|
||
| @export | ||
| def off(self) -> None: | ||
| for ch in self._channels(): | ||
| self.logger.info("Relay channel %d OFF", ch) | ||
| self._send_command(_build_command(ch, 0)) | ||
|
|
||
| @export | ||
| def read(self) -> Generator[PowerReading, None, None]: | ||
| raise NotImplementedError | ||
| yield # makes this a generator function |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import ast
from pathlib import Path
def dump_methods(path_str: str, class_name: str) -> None:
path = Path(path_str)
tree = ast.parse(path.read_text())
cls = next(
node for node in tree.body
if isinstance(node, ast.ClassDef) and node.name == class_name
)
print(f"== {class_name} ({path}) ==")
for name in ("on", "off", "read"):
method = next(
node for node in cls.body
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name
)
kind = "async def" if isinstance(method, ast.AsyncFunctionDef) else "def"
print(f"{name}: {kind}")
print()
dump_methods(
"python/packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py",
"PowerInterface",
)
dump_methods(
"python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py",
"NoyitoPowerSerial",
)
dump_methods(
"python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py",
"NoyitoPowerHID",
)
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 500
Fix async method signatures to match PowerInterface contract.
Both NoyitoPowerSerial and NoyitoPowerHID inherit from PowerInterface but implement on(), off(), and read() as sync methods. The interface declares these as async def:
PowerInterface:on: async def,off: async def,read: async defNoyitoPowerSerial:on: def,off: def,read: defNoyitoPowerHID:on: def,off: def,read: def
Change to async def and await internal calls to align with the interface contract. This mismatch will fail static type checking (make ty-pkg-jumpstarter-driver-noyito-relay).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-noyito-relay/jumpstarter_driver_noyito_relay/driver.py`
around lines 71 - 85, The methods on, off, and read in NoyitoPowerSerial and
NoyitoPowerHID currently are synchronous but must match the PowerInterface async
signatures; change their definitions to async def on(self), async def off(self),
and async def read(self) (returning an async generator/AsyncGenerator for
PowerReading), and inside await any asynchronous helpers (e.g., await
self._send_command(_build_command(ch, 1/0)) and await any async iteration over
self._channels() or async I/O in read) so the implementations of on/off/read in
those classes conform to the interface and pass static typing.
Add
jumpstarter-driver-noyito-relay— NOYITO USB Relay Power DriverSummary
Adds a new driver package supporting the full NOYITO USB relay board family
across two distinct hardware series:
NoyitoPowerSerial— 1/2-channel boards using a CH340 USB-to-serial chip(9600 baud). Supports the
0xFFstatus query to read channel state.Controlled via
pyserial.NoyitoPowerHID— 4/8-channel "HID Drive-free" boards that enumerate asUSB HID devices (no serial port). Status query is not available on this
hardware. Controlled via the
hidPython package.Both classes share the same 4-byte command protocol
(
0xA0+ channel + state + checksum) via a common_build_commandhelper.Key configuration parameters
NoyitoPowerSerialport,channel(1–2),dualdual=Truefires both contacts simultaneously for high-current wiringNoyitoPowerHIDnum_channels(4 or 8),channel,all_channels,vendor_id,product_idall_channels=Truefires every channel simultaneouslymacOS / Apple Silicon note
The
hidpackage requires the nativehidapishared library(
brew install hidapi). On Apple Silicon, Homebrew installs to/opt/homebrew/libwhich is not indlopen's default search path.NoyitoPowerHID._send_commandautomatically prepends the Homebrew prefix toDYLD_FALLBACK_LIBRARY_PATHbefore the firstimport hid, so no manualenvironment setup is needed on Intel or Apple Silicon Macs.
Files added
jumpstarter_driver_noyito_relay/driver.pyNoyitoPowerSerial+NoyitoPowerHIDdriver classesjumpstarter_driver_noyito_relay/client.pyNoyitoPowerClient(thinPowerClientsubclass)jumpstarter_driver_noyito_relay/driver_test.pyjumpstarter_driver_noyito_relay/conftest.pyhidinsys.modulesso tests run without nativehidapiexamples/exporter.yamlREADME.mdpyproject.tomlpyserial>=3.5,hid>=1.0.4deps; registers both entry pointspython/uv.lockhid 1.0.9from pypi.orgTest plan
hid.Devicemocked (no hardware needed):make pkg-test-jumpstarter-driver-noyito-relayruff checkpasses cleanon/off/status/cycleon/off/cycle,confirm
statusis not available