Yepkit ykush driver #249
Yepkit ykush driver #249
Conversation
|
Warning Rate limit exceeded@mangelajo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
WalkthroughThe changes update the driver scaffold creation process by rebranding from the "shell" driver to a new "yepkit" (Ykush) driver. The command in the contribution instructions and the file creation paths have been modified accordingly. The Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Hook as Pytest Hook
participant Driver as Ykush Driver
participant USB as USB Device
Test->>Hook: Execute test (pytest_runtest_call)
alt File/USB Error
Hook->>Test: Skip test with appropriate message
else Normal Execution
Test->>Driver: Initialize and call on() method
Driver->>USB: Send power on command
USB-->>Driver: Acknowledge power on
Test->>Driver: Call off() method to power off
Driver->>USB: Send power off command
USB-->>Driver: Acknowledge power off
end
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
CONTRIB.md (1)
31-35: Sync Log Output Inconsistency – Update Package NameThe sync log output (lines 31–35) still references
jumpstarter-driver-shelland paths undercontrib/drivers/shell. Since the scaffold now targets the Yepkit driver, these references should be updated to reflect the new package name (jumpstarter-driver-yepkit). This discrepancy might confuse users reviewing the logs.A potential diff for correction:
- Built jumpstarter-driver-shell @ file:///Users/ajo/work/jumpstarter/contrib/drivers/shell + Built jumpstarter-driver-yepkit @ file:///Users/ajo/work/jumpstarter-driver-yepkit ... - ~ jumpstarter-driver-shell==0.1.0 (from file:///Users/ajo/work/jumpstarter/contrib/drivers/shell) + ~ jumpstarter-driver-yepkit==0.1.0 (from file:///Users/ajo/work/jumpstarter-driver-yepkit)Please update these lines to ensure consistency across the documentation.
🧹 Nitpick comments (9)
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py (3)
36-36: Fix the “intefaces” spelling error.
Replace "intefaces" with "interfaces" to conform to correct spelling.-# static shared array of usb devices, intefaces on same device cannot be claimed multiple times +# static shared array of usb devices, interfaces on same device cannot be claimed multiple times🧰 Tools
🪛 GitHub Check: typos
[warning] 36-36:
"intefaces" should be "interfaces".🪛 GitHub Actions: Spell Check
[warning] 36-36: "intefaces" should be "interfaces".
[error] 36-36:
intefacesshould beinterfaces.
67-72: Clarify device selection logic for multiple connections.
When multiple Ykush devices are present and no serial is provided, the code picks the first device found. This might lead to unpredictable device selection if multiple are connected. Consider prompting the user or raising an exception to ensure the correct device is addressed.
149-150: Finish or remove the unimplementedreadmethod.
Thereadmethod currently raisesNotImplementedError. If reading power states is not planned, consider removing this method or at least providing a stub implementation that returns a placeholder.Would you like me to propose a basic partial implementation for
readin a new comment?packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver_test.py (1)
8-10: Add assertions to verify driver functionality.
Activating and deactivating the driver is tested, but there are no checks to confirm actual behavior. Consider adding verification steps (oncereador another validation mechanism is available) to ensure the driver responds as expected.packages/jumpstarter-driver-yepkit/examples/exporter.yaml (1)
1-27: Exporter YAML Configuration – Verify Placeholder Values and ConsistencyThis new configuration file clearly defines the exporter settings for the Ykush driver. The API version, kind, endpoint, and token fields are well set up. A few points to consider:
- The token on line 4 (
"<token>") is a placeholder; ensure that in production environments a valid token value is provided.- The export configurations for
power,power2,power3, andallare consistent with our expected settings. In particular, note that only thepower2configuration includes aserialparameter. If the driver requires a serial number for correct operation in all cases (or if auto-detection is expected when omitted), please confirm this behavior.__templates__/create_driver.sh (1)
24-24: Updated Driver Directory NamingThe assignment for
DRIVER_DIRECTORYon line 24 now uses hyphenated naming (jumpstarter-driver-${DRIVER_NAME}) instead of underscores. This aligns with the new naming convention.One minor note: the
MODULE_DIRECTORY(line 25) still uses underscores (jumpstarter_driver_${DRIVER_NAME}). This difference is likely intentional since Python module names conventionally use underscores. Just ensure this mixing of naming styles is clearly documented.docs/source/api-reference/drivers/yepkit.md (3)
11-25: Driver Configuration Example ClarityThe configuration snippet (lines 11–25) is well-formatted and provides clear examples for different export scenarios (including variations with and without the
serialsetting).Ensure that the configurations here match the expectations outlined in
exporter.yaml(e.g., if auto-detection for the serial number should occur when omitted, it might be worth a brief note).
26-33: Configuration Parameters TableThe table (lines 26–33) comprehensively lists the driver parameters. It might be helpful to clarify in the description for the
serialfield that leaving it empty triggers auto-detection if that is indeed the case.
34-43: PowerClient API DocumentationThe auto-documentation block (lines 34–43) is correctly set up to document the
PowerClientAPI. Note that a static analysis hint flagged a possible missing preposition, but the current RST directive appears standard. If you decide to refine the wording, consider reviewing the formatting of the directive options.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing preposition found.
Context: ... :members: on, off### Examples Powering on and off a device{testcode} clien...(AI_HYDRA_LEO_MISSING_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CONTRIB.md(1 hunks)__templates__/create_driver.sh(2 hunks)docs/source/api-reference/drivers/yepkit.md(1 hunks)packages/jumpstarter-driver-yepkit/README.md(1 hunks)packages/jumpstarter-driver-yepkit/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/conftest.py(1 hunks)packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py(1 hunks)packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver_test.py(1 hunks)packages/jumpstarter-driver-yepkit/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/jumpstarter-driver-yepkit/README.md
- packages/jumpstarter-driver-yepkit/pyproject.toml
🧰 Additional context used
🪛 LanguageTool
docs/source/api-reference/drivers/yepkit.md
[uncategorized] ~43-~43: Possible missing preposition found.
Context: ... :members: on, off ### Examples Powering on and off a device{testcode} clien...
(AI_HYDRA_LEO_MISSING_OF)
🪛 GitHub Check: typos
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py
[warning] 36-36:
"intefaces" should be "interfaces".
🪛 GitHub Actions: Spell Check
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py
[warning] 36-36: "intefaces" should be "interfaces".
[error] 36-36: intefaces should be interfaces.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/conftest.py (1)
5-13: Graceful skip logic looks good!
The approach to skip tests when the hardware or backend is unavailable is clean and straightforward.__templates__/create_driver.sh (2)
33-33: Consolidated Template File Paths in Module LoopThe template file path on line 33 has been updated to use the consolidated
__templates__directory (__templates__/driver/jumpstarter_driver/${f}.tmpl). This change simplifies the path and is consistent with our new directory structure.
38-38: Streamlined Template File Paths for Additional FilesSimilarly, the template path on line 38 in the second loop now points to
__templates__/driver/${f}.tmpl, which is in line with the new reorganization of the template files.docs/source/api-reference/drivers/yepkit.md (2)
1-10: Driver Introduction and Overview DocumentationThe introduction (lines 1–10) effectively presents the Yepkit (Ykush) driver, including a clear header and a brief product description with a relevant link to the Ykush USB switch.
44-75: CLI Examples and Usage InstructionsThe CLI access section (lines 44–75) offers practical examples for interacting with the driver. The provided command outputs (warning and info messages) appear realistic; however, verify that the messages will match the final driver behavior. For instance, double-check whether the sample output in the CLI snippet reflects the correct state transitions (ON/OFF messages) as intended.
CONTRIB.md (2)
16-18: Updated Driver Scaffold Creation CommandThe instructions for creating a new driver scaffold now correctly use the new parameters:
$ ./__templates__/create_driver.sh yepkit Ykush "Miguel Angel Ajo" "[email protected]"This update aligns with the rebranding effort for the Yepkit driver.
19-27: New File Creaton Output VerificationThe output lines (19–27) accurately show the creation of all required files in the new directory structure (
packages/jumpstarter_driver_yepkit/...). This reflects the changes in the directory layout.
mangelajo
force-pushed
the
yepkit-ykush-driver
branch
2 times, most recently
from
fb9161d to
f6a6645
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py (4)
36-38: Fix typo in comment.The comment contains a typo: "intefaces" should be "interfaces".
-# static shared array of usb devices, intefaces on same device cannot be claimed multiple times +# static shared array of usb devices, interfaces on same device cannot be claimed multiple times🧰 Tools
🪛 GitHub Check: typos
[warning] 36-36:
"intefaces" should be "interfaces".🪛 GitHub Actions: Spell Check
[warning] 36-36: "intefaces" should be "interfaces".
[error] 36-36:
intefacesshould beinterfaces.
54-90: Consider enhancing error handling.While the error handling is generally good, consider adding more specific error types and handling USB-related exceptions that might occur during device discovery.
+from usb.core import USBError + def __post_init__(self): if hasattr(super(), "__post_init__"): super().__post_init__() @@ -75,16 +77,20 @@ self.dev = _USB_DEVS[self.serial] return - for dev in usb.core.find(idVendor=VID, idProduct=PID, find_all=True): - serial = usb.util.get_string(dev, dev.iSerialNumber, 0) - if serial == self.serial or self.serial is None: - _USB_DEVS[serial] = dev - if self.serial is None: - self.logger.warning( - f"No serial number provided for ykush, using the first one found: {serial}") - self.serial = serial - self.dev = dev - return + try: + for dev in usb.core.find(idVendor=VID, idProduct=PID, find_all=True): + try: + serial = usb.util.get_string(dev, dev.iSerialNumber, 0) + if serial == self.serial or self.serial is None: + _USB_DEVS[serial] = dev + if self.serial is None: + self.logger.warning( + f"No serial number provided for ykush, using the first one found: {serial}") + self.serial = serial + self.dev = dev + return + except USBError as e: + self.logger.warning(f"Failed to get serial number from device: {e}") + except USBError as e: + raise RuntimeError(f"Failed to enumerate USB devices: {e}") from e raise FileNotFoundError("failed to find ykush device")
91-126: Consider parameterizing USB timeout and improving error handling.The USB timeout is hardcoded and error handling could be more robust.
+ USB_TIMEOUT_MS = 2000 # Class constant for USB timeout + def _send_cmd(self, cmd, report_size=64): - out_ep, in_ep = self._get_endpoints(self.dev) - out_buf = [0x00] * report_size - out_buf[0] = cmd # YKUSH command + try: + out_ep, in_ep = self._get_endpoints(self.dev) + out_buf = [0x00] * report_size + out_buf[0] = cmd # YKUSH command - # Write to the OUT endpoint - out_ep.write(out_buf) + # Write to the OUT endpoint + out_ep.write(out_buf) - # Read from the IN endpoint - in_buf = in_ep.read(report_size, timeout=2000) - return list(in_buf) + # Read from the IN endpoint + in_buf = in_ep.read(report_size, timeout=self.USB_TIMEOUT_MS) + return list(in_buf) + except USBError as e: + raise RuntimeError(f"Failed to send command to device: {e}") from e
148-151: Document power reading limitation.Since power readings are not supported, consider adding a more descriptive error message explaining why.
@export def read(self) -> AsyncGenerator[PowerReading, None]: - raise NotImplementedError + raise NotImplementedError("Power readings are not supported by the Ykush device")
🛑 Comments failed to post (1)
packages/jumpstarter-driver-yepkit/README.md (1)
1-1:
⚠️ Potential issueComplete the README header.
The header appears to be incomplete. Consider updating it to properly describe the driver.
-# Jumpstarter Driver for ... +# Jumpstarter Driver for Yepkit Ykush USB Hub📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.-# Jumpstarter Driver for ... +# Jumpstarter Driver for Yepkit Ykush USB Hub🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Trailing punctuation in heading
Punctuation: ' ...'(MD026, no-trailing-punctuation)
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/conftest.py
Outdated
Show resolved
Hide resolved
mangelajo
force-pushed
the
yepkit-ykush-driver
branch
from
f6a6645 to
be1e70b
Compare
|
Updated :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-yepkit/examples/exporter.yaml (1)
8-30: Well-structured exporter configuration
The export mappings forpower,power2,power3, andallare clearly defined and correctly reference thejumpstarter_driver_yepkit.driver.Ykushdriver type. Note that theserialfield is provided only for thepower2configuration. If this omission for the other entries is intentional (to use auto-detection, for example), it may help to add a comment or documentation note clarifying this behavior.docs/source/api-reference/drivers/yepkit.md (1)
34-42: Effective PowerClient API documentation
The section describing thePowerClientAPI is succinct and leverages the reStructuredText directive (autoclass) to automatically generate documentation for theonandoffmethods. Although a static analysis hint flagged a possible missing preposition, it appears to be a false positive linked to this directive block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CONTRIB.md(1 hunks)__templates__/create_driver.sh(2 hunks)docs/source/api-reference/drivers/yepkit.md(1 hunks)packages/jumpstarter-driver-yepkit/README.md(1 hunks)packages/jumpstarter-driver-yepkit/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/conftest.py(1 hunks)packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py(1 hunks)packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver_test.py(1 hunks)packages/jumpstarter-driver-yepkit/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver_test.py
- packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/conftest.py
- packages/jumpstarter-driver-yepkit/README.md
- templates/create_driver.sh
- packages/jumpstarter-driver-yepkit/pyproject.toml
- packages/jumpstarter-driver-yepkit/jumpstarter_driver_yepkit/driver.py
- CONTRIB.md
🧰 Additional context used
🪛 LanguageTool
docs/source/api-reference/drivers/yepkit.md
[uncategorized] ~43-~43: Possible missing preposition found.
Context: ... :members: on, off ### Examples Powering on and off a device{testcode} clien...
(AI_HYDRA_LEO_MISSING_OF)
🔇 Additional comments (6)
packages/jumpstarter-driver-yepkit/examples/exporter.yaml (1)
1-7: Clear configuration metadata and API version
The metadata section, including the API version (jumpstarter.dev/v1alpha1),kind, andmetadata(name and namespace), is clearly defined. Theendpointandtokenfields are provided appropriately—with the token as a placeholder—which makes it easy to adapt for real deployments.docs/source/api-reference/drivers/yepkit.md (5)
1-9: Concise introduction and driver overview
The introduction clearly presents the Yepkit driver, specifying its association with the Ykush USB switch and the driver class (jumpstarter_driver_yepkit.driver.Ykush). The content is clear and sets the stage for the configuration details that follow.
10-25: Clear driver configuration example with minor discrepancy
The YAML configuration sample is well-documented and serves as a useful guide for users setting up the driver. One minor point: in this sample, theserialfield is provided for bothpowerandpower2, whereas in the corresponding exporter configuration file, thepowerentry omits theserial. Please verify that this discrepancy is intentional (to demonstrate both auto-detection and explicit configuration) and, if so, consider clarifying this in the documentation.
26-32: Well-documented configuration parameters
The parameters table clearly lists the available configuration options with their descriptions, types, and required status. It might be beneficial to include an example default value in the “Default” column where applicable, but overall the table is informative and easy to follow.
43-50: Clear usage example for power control
The provided test code snippet clearly demonstrates how to use thePowerClientmethods (onandoff) with an appropriate delay. This serves as a practical example for users and reinforces the documentation's intent.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing preposition found.
Context: ... :members: on, off### Examples Powering on and off a device{testcode} clien...(AI_HYDRA_LEO_MISSING_OF)
51-74: Informative CLI access documentation with a potential naming consideration
The CLI instructions and sample outputs effectively guide the user through running the exporter, including both the configuration file path and sample log messages. However, note that the CLI command still referencesshell(i.e.,jmp exporter shell), while the overall PR objective mentioned rebranding to the Yepkit driver. Please verify if the CLI command should be updated to reflect the new driver name or ifshellremains valid in this context.
This USB Hub enables power control of the ports, making it useful to control the power of different targets as long as those targets can be USB powered.
mangelajo
force-pushed
the
yepkit-ykush-driver
branch
from
be1e70b to
f933e09
Compare
Summary by CodeRabbit
New Features
Documentation
Chores