Rework gpio driver with adafruit-blinka #227
Conversation
WalkthroughThis pull request introduces comprehensive updates to the Raspberry Pi driver package, focusing on enhancing digital I/O functionality and documentation. The changes span multiple files, including Dockerfiles, documentation, and the driver implementation. The primary modifications involve consolidating digital input/output classes, adding new power-related classes, updating client interfaces, and expanding documentation for Raspberry Pi drivers. Changes
Poem
✨ Finishing Touches
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: 1
🧹 Nitpick comments (4)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py (2)
11-18: Simplify mapping fromDriveModeto integer valuesInstead of using a
matchstatement to mapDriveModeenum values to integers, you can directly use the.valueattribute of the enum. This simplifies the code and reduces the possibility of mapping errors.Apply this diff to simplify the mapping:
def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: - match drive_mode: - case DriveMode.PUSH_PULL: - drive_mode = 0 - case DriveMode.OPEN_DRAIN: - drive_mode = 1 - case _: - raise ValueError("unrecognized drive_mode") + if drive_mode not in (DriveMode.PUSH_PULL, DriveMode.OPEN_DRAIN): + raise ValueError("unrecognized drive_mode") + drive_mode = drive_mode.value self.call("switch_to_output", value, drive_mode)
21-30: Simplify mapping fromPullto integer valuesSimilarly, you can simplify the mapping of
Pullenum values by using their.valueattributes. This makes the code more concise and easier to maintain.Apply this diff to simplify the mapping:
def switch_to_input(self, pull: Pull | None = None) -> None: - match pull: - case None: - pull = 0 - case Pull.UP: - pull = 1 - case Pull.DOWN: - pull = 2 - case _: - raise ValueError("unrecognized pull") + if pull is None or pull in (Pull.UP, Pull.DOWN): + pull = 0 if pull is None else pull.value + else: + raise ValueError("unrecognized pull") self.call("switch_to_input", pull)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (2)
30-36: Simplify mapping from integers toDriveModeenumsInstead of using a
matchstatement to map integers toDriveModeenums, you can directly construct the enum using the integer. This leverages the enum's built-in functionality and simplifies error handling.Apply this diff to simplify the mapping:
def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: - match drive_mode: - case 0: - drive_mode = DriveMode.PUSH_PULL - case 1: - drive_mode = DriveMode.OPEN_DRAIN - case _: - raise ValueError("unrecognized drive_mode") + try: + drive_mode = DriveMode(drive_mode) + except ValueError: + raise ValueError("unrecognized drive_mode") self.device.switch_to_output(value=value, drive_mode=drive_mode)
42-51: Simplify mapping from integers toPullenumsYou can simplify the mapping by directly constructing the
Pullenum from the integer value or handlingNoneexplicitly. This makes the code cleaner and relies on the enum's inherent error checking.Apply this diff to simplify the mapping:
def switch_to_input(self, pull: Pull | None = None) -> None: - match pull: - case 0: - pull = None - case 1: - pull = Pull.UP - case 2: - pull = Pull.DOWN - case _: - raise ValueError("unrecognized pull") + if pull == 0: + pull = None + else: + try: + pull = Pull(pull) + except ValueError: + raise ValueError("unrecognized pull") self.device.switch_to_input(pull=pull)
📜 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 (4)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)contrib/drivers/raspberrypi/pyproject.toml(1 hunks)
🔇 Additional comments (3)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (1)
12-15: Verify the sequence of input value assertionsThe sequence of assertions in the input test assumes that
client.valuetoggles betweenTrueandFalse. Please verify if this behavior is expected in your test environment. Ifclient.valuedoes not change without external influence, these assertions may produce inconsistent results.contrib/drivers/raspberrypi/pyproject.toml (2)
15-15: Verify GPIO functionality with adafruit-blinka.Since this is a major change from gpiozero to adafruit-blinka, ensure that all GPIO functionality is maintained.
Consider documenting the following in the PR description or README:
- Rationale for switching from gpiozero to adafruit-blinka
- Any differences in functionality or performance
- Migration guide for users
15-15: Verify adafruit-blinka version and consider version pinning.Using
>=without an upper bound could lead to compatibility issues if breaking changes are introduced in future versions. Consider using a more specific version range.Let's verify the latest stable version and check for any security advisories:
Consider updating to:
- "adafruit-blinka>=8.51.0", + "adafruit-blinka>=8.51.0,<9.0.0",✅ Verification successful
Version constraint is appropriate, but could be more specific
The current version requirement (
>=8.51.0) matches the latest stable release. While no security vulnerabilities were found, it's recommended to add an upper bound to prevent potential breaking changes from major version updates:- "adafruit-blinka>=8.51.0", + "adafruit-blinka>=8.51.0,<9.0.0",🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check PyPI for latest versions and release dates curl -s https://pypi.org/pypi/adafruit-blinka/json | jq -r '.releases | to_entries[] | "\(.key) \(.value[0].upload_time)"' | sort -V # Check for any known vulnerabilities gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "adafruit-blinka") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 7910
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (1)
21-22:⚠️ Potential issueFix incorrect assertion after setting value to False
The test asserts
client.valueis True after setting it to False.client.value = False - assert client.value + assert not client.value
🧹 Nitpick comments (4)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (4)
22-23: Consider adding input validation for pin nameThe pin name is directly used with
getattr(board, self.pin)without validation. If an invalid pin name is provided, this will raise an AttributeError with a generic message.Consider adding explicit validation:
def __post_init__(self): super().__post_init__() + if not hasattr(board, self.pin): + raise ValueError(f"Invalid pin name: {self.pin}") # Defaults to input with no pull self.device = DigitalInOut(pin=getattr(board, self.pin))
31-40: Improve drive mode handling with enum comparisonThe current implementation uses integer comparison for drive modes which is fragile and less maintainable.
Consider using enum comparison for better type safety:
def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: match drive_mode: - case 0: + case DriveMode.PUSH_PULL: drive_mode = DriveMode.PUSH_PULL - case 1: + case DriveMode.OPEN_DRAIN: drive_mode = DriveMode.OPEN_DRAIN case _: raise ValueError("unrecognized drive_mode")
43-54: Improve pull configuration handling with enum comparisonSimilar to drive mode handling, pull configuration uses integer comparison.
Consider using enum comparison:
def switch_to_input(self, pull: Pull | None = None) -> None: match pull: - case 0: + case None: pull = None - case 1: + case Pull.UP: pull = Pull.UP - case 2: + case Pull.DOWN: pull = Pull.DOWN case _: raise ValueError("unrecognized pull")
73-73: Remove debug print statementDebug print statements should not be present in production code.
- print(self.value, self.drive_mode, getattr(DriveMode, self.drive_mode))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)
🔇 Additional comments (1)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (1)
13-15: Add delay between value reads for toggle inputThe consecutive value reads might be too fast for the hardware to actually toggle. This could lead to flaky tests.
Consider adding small delays between reads:
+ from time import sleep + with serve(DigitalIO(pin="Dx_INPUT_TOGGLE")) as client: client.switch_to_input(pull=Pull.UP) assert client.value + sleep(0.1) # Add delay to allow for hardware toggle assert not client.value + sleep(0.1) # Add delay to allow for hardware toggle assert client.value
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (2)
17-22:⚠️ Potential issueCorrect the test assertion after setting
client.value = FalseAfter setting
client.value = False, the subsequent assertionassert client.valuewill fail becauseclient.valueshould now beFalse.client.value = True assert client.value client.value = False - assert client.value + assert not client.value
25-36: 🛠️ Refactor suggestionAdd assertions for power state changes
The power tests only call methods without verifying the state changes.
with serve(DigitalPowerSwitch(pin="Dx_OUTPUT", drive_mode="PUSH_PULL")) as client: client.off() + assert not client.get_value() client.on() + assert client.get_value() with serve(DigitalPowerButton(pin="Dx_OUTPUT", drive_mode="PUSH_PULL", off_press_seconds=1)) as client: client.off() + assert not client.get_value() client.on() + assert client.get_value()contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (1)
77-82:⚠️ Potential issueFix return type hints for on/off methods.
The methods declare a return type of
strbut don't return anything.@export - def on(self) -> str: + def on(self) -> None: self.device.value = True @export - def off(self) -> str: + def off(self) -> None: self.device.value = False
🧹 Nitpick comments (3)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (1)
11-15: Add delays between input reads to prevent race conditions.Reading the input value in rapid succession without delays could lead to missed state changes or race conditions. Consider adding small delays between reads to ensure reliable state detection.
with serve(DigitalIO(pin="Dx_INPUT_TOGGLE")) as client: client.switch_to_input(pull=Pull.UP) assert client.value + sleep(0.1) # Add delay to ensure stable reading assert not client.value + sleep(0.1) # Add delay to ensure stable reading assert client.valuecontrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (2)
32-41: Replace magic numbers with enum values and add documentation.The method uses magic numbers for drive modes and lacks documentation explaining the parameters and behavior.
@export def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: + """Configure the pin as a digital output. + + Args: + value: Initial output value (default: False) + drive_mode: Output drive mode (default: PUSH_PULL) + DriveMode.PUSH_PULL (0): Standard push-pull output + DriveMode.OPEN_DRAIN (1): Open-drain output + + Raises: + ValueError: If drive_mode is not recognized + """ match drive_mode: - case 0: + case DriveMode.PUSH_PULL: drive_mode = DriveMode.PUSH_PULL - case 1: + case DriveMode.OPEN_DRAIN: drive_mode = DriveMode.OPEN_DRAIN case _: raise ValueError("unrecognized drive_mode")
44-55: Replace magic numbers with enum values and add documentation.Similar to switch_to_output, this method uses magic numbers for pull modes and lacks documentation.
@export def switch_to_input(self, pull: Pull | None = None) -> None: + """Configure the pin as a digital input. + + Args: + pull: Input pull configuration (default: None) + None (0): No pull + Pull.UP (1): Pull-up enabled + Pull.DOWN (2): Pull-down enabled + + Raises: + ValueError: If pull is not recognized + """ match pull: - case 0: + case None: pull = None - case 1: + case Pull.UP: pull = Pull.UP - case 2: + case Pull.DOWN: pull = Pull.DOWN case _: raise ValueError("unrecognized pull")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)
🔇 Additional comments (4)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (1)
4-9: LGTM! Good test setup.Setting BLINKA_OS_AGNOSTIC=1 is good practice for testing GPIO functionality in isolation.
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (3)
1-15: LGTM! Well-structured class definition.Good use of dataclass and type hints for clean, maintainable code.
85-86: Document NotImplementedError in read method.The read method raises NotImplementedError without explanation.
@export def read(self) -> AsyncGenerator[PowerReading, None]: + """ + Not implemented for DigitalPowerSwitch as it only supports binary states. + Use get_value() instead to read the current state. + + Raises: + NotImplementedError: This method is not supported. + """ raise NotImplementedError
108-117: LGTM! Methods use configured press durations correctly.
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (1)
58-63: Add mode validation in value getters/setters.The
set_valueandget_valuemethods should verify that the device is in the correct mode before operation.@export def set_value(self, value: bool) -> None: + if not self.device.direction == Direction.OUTPUT: + raise RuntimeError("Device must be in output mode to set value") self.device.value = value @export def get_value(self) -> bool: + if not self.device.direction == Direction.INPUT: + raise RuntimeError("Device must be in input mode to get value") return self.device.value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)
🔇 Additional comments (3)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (3)
23-24: Add error handling for invalid pin names.The
getattr(board, self.pin)call will raise AttributeError if the pin name is invalid. Consider adding explicit error handling.
85-86: Implement or document NotImplementedError.The
read()method raises NotImplementedError without explanation.
101-105: Add validation and error handling in press method.The press method should validate the duration and handle potential device failures.
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (3)
35-44: Replace magic numbers with enum values in drive_mode parameter.The method accepts an integer parameter but should use the DriveMode enum directly for better type safety and readability.
- def switch_to_output(self, value: bool = False, drive_mode: int = 0) -> None: + def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: match drive_mode: - case 0: - drive_mode = DriveMode.PUSH_PULL - case 1: - drive_mode = DriveMode.OPEN_DRAIN + case DriveMode.PUSH_PULL: + pass + case DriveMode.OPEN_DRAIN: + pass case _: raise ValueError("unrecognized drive_mode")
47-58: Replace magic numbers with enum values in pull parameter.Similar to drive_mode, the pull parameter should use the Pull enum directly.
- def switch_to_input(self, pull: int = 0) -> None: + def switch_to_input(self, pull: Pull | None = None) -> None: match pull: - case 0: + case None: pull = None - case 1: + case Pull.UP: pull = Pull.UP - case 2: + case Pull.DOWN: pull = Pull.DOWN case _: raise ValueError("unrecognized pull")
91-92: Document why read method is not implemented.Add docstring explaining why this method raises NotImplementedError.
@export def read(self) -> AsyncGenerator[PowerReading, None]: + """ + Not implemented for DigitalPowerSwitch as it only supports binary states. + Use get_value() instead to read the current state. + + Raises: + NotImplementedError: This method is not supported. + """ raise NotImplementedError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)
🔇 Additional comments (6)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py (2)
13-27: LGTM! Good error handling for invalid pin names.The initialization code properly validates pin names and provides clear error messages.
110-114: 🛠️ Refactor suggestionAdd validation and error handling in press method.
The press method should validate the duration and handle potential device failures.
def press(self, seconds: int) -> None: + if seconds <= 0: + raise ValueError("Press duration must be positive") + + try: self.device.value = self.value self.device.value = not self.value sleep(seconds) self.device.value = self.value + except Exception as e: + raise RuntimeError(f"Failed to control power button: {e}") from eLikely invalid or redundant comment.
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (4)
21-23:⚠️ Potential issueFix incorrect assertion after setting value to False.
The comment "Dx_OUTPUT is always True" contradicts the test logic. If
client.value = Falseis called, thenassert client.valueshould fail.Either:
- Remove the assertion if Dx_OUTPUT is truly always True, or
- Update the assertion to match the expected behavior:
client.value = False - # Dx_OUTPUT is always True - assert client.value + assert not client.valueLikely invalid or redundant comment.
31-33: 🛠️ Refactor suggestionAdd assertions for power switch state.
The test only calls methods without verifying the state changes.
with serve(DigitalPowerSwitch(pin="Dx_OUTPUT", drive_mode="PUSH_PULL")) as client: client.off() + assert not client.get_value() client.on() + assert client.get_value()Likely invalid or redundant comment.
35-37: 🛠️ Refactor suggestionAdd assertions for power button press.
Similar to the power switch test, add assertions to verify the button press effects.
with serve(DigitalPowerButton(pin="Dx_OUTPUT", drive_mode="PUSH_PULL", off_press_seconds=1)) as client: client.off() + assert not client.get_value() client.on() + assert client.get_value()Likely invalid or redundant comment.
11-15: Verify the toggle behavior assertions.The consecutive assertions
assert client.value→assert not client.value→assert client.valueseem to expect the value to automatically toggle, which might not be the intended behavior. Please verify if this is the expected behavior of theDx_INPUT_TOGGLEpin.
|
Since adafruit/Adafruit_Blinka@b277397, sysv_ipc is installed unconditionally, which is very annoying since it has c extensions to be built from source. |
|
Looks great! We need to add gcc to the container images building the wheels, (see the container build error) also, awesome if we can include documentation https://github.com/jumpstarter-dev/jumpstarter/tree/main/docs/source/api-reference on the PR :) Thank you! |
let's just add gcc to the builds and let's live with it. |
Problem is sysv_ipc is built when installing the wheels, not when building the wheels, so it would be part of the final product. |
Ohhh, right!, thanks for explaining It is the same problem as with gRPC, i.e. for RV64 it did not have precompiled wheels and it needed to compile on install, but they have pre-compiled wheels for most architectures though... |
NickCao
force-pushed
the
gpio-opendrain
branch
from
4caf6af to
aa5a151
Compare
|
Let's leave gcc there and call it a day, will surely find a solution in the long term. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py (4)
11-18: Replace magic numbers with constants for drive mode mapping.The drive mode mapping uses magic numbers which reduces code maintainability. Consider defining constants or an IntEnum for these values.
+from enum import IntEnum + +class DriveModeBits(IntEnum): + PUSH_PULL = 0 + OPEN_DRAIN = 1 def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: match drive_mode: case DriveMode.PUSH_PULL: - drive_mode = 0 + drive_mode = DriveModeBits.PUSH_PULL case DriveMode.OPEN_DRAIN: - drive_mode = 1 + drive_mode = DriveModeBits.OPEN_DRAIN case _: raise ValueError("unrecognized drive_mode")
21-30: Replace magic numbers with constants for pull resistor mapping.Similar to the drive mode mapping, the pull resistor mapping uses magic numbers which reduces code maintainability.
+class PullBits(IntEnum): + NONE = 0 + UP = 1 + DOWN = 2 def switch_to_input(self, pull: Pull | None = None) -> None: match pull: case None: - pull = 0 + pull = PullBits.NONE case Pull.UP: - pull = 1 + pull = PullBits.UP case Pull.DOWN: - pull = 2 + pull = PullBits.DOWN case _: raise ValueError("unrecognized pull")
9-38: Add docstrings to document the class and methods.The class and its methods lack documentation. Add docstrings to explain:
- The purpose of the class
- Parameters and their valid values
- Return values
- Possible exceptions
- Examples of usage
Example docstring for the class:
@dataclass(kw_only=True) class DigitalIOClient(DriverClient): """Client for controlling GPIO pins in both input and output modes. This class provides methods to configure and control GPIO pins, supporting both input and output modes with various configuration options like pull resistors and drive modes. """Example docstring for switch_to_output:
def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: """Configure the pin as a digital output. Args: value: Initial output value (True=high, False=low) drive_mode: Output drive mode (PUSH_PULL or OPEN_DRAIN) Raises: ValueError: If drive_mode is not recognized """
32-38: Add error handling for RPC call failures.The value property methods should handle potential RPC call failures gracefully.
@property def value(self) -> bool: - return self.call("get_value") + try: + return self.call("get_value") + except Exception as e: + raise RuntimeError(f"Failed to get pin value: {e}") from e @value.setter def value(self, value: bool) -> None: - self.call("set_value", value) + try: + self.call("set_value", value) + except Exception as e: + raise RuntimeError(f"Failed to set pin value: {e}") from e
📜 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 (5)
Dockerfile(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)contrib/drivers/raspberrypi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contrib/drivers/raspberrypi/pyproject.toml
- contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py
- contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/driver.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (1)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py (1)
3-3: LGTM! Good choice of dependency.The switch to
adafruit-blinka's digitalio module aligns well with the PR objectives and provides a more standardized GPIO interface.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.devfile/Containerfile (1)
16-16: Consider adding build-essential for comprehensive build support.While adding
gccandpython3.12-develis good, you might want to consider addingbuild-essential(or its RHEL equivalent) to ensure all necessary build tools are available. This could help avoid potential issues with other Python extensions that require additional build dependencies.Also, since this change is related to supporting
adafruit-blinkaand its dependency onsysv_ipc, consider documenting this requirement in a comment..devfile/Containerfile.client (1)
27-27: Consider caching pip dependencies and documenting build requirements.The changes look good but consider:
The
sysv_ipcpackage (required by adafruit-blinka) will need to be compiled during wheel installation. Consider pre-building this dependency in the builder stage to avoid runtime compilation.Since this is a client container, document the gcc requirement and its relationship to adafruit-blinka's dependencies.
Here's a potential optimization for the builder stage:
FROM --platform=$BUILDPLATFORM registry.access.redhat.com/ubi9/ubi:latest AS builder -RUN dnf install -y make git && \ +RUN dnf install -y make git gcc python3.12-devel && \ dnf clean all && \ rm -rf /var/cache/dnf COPY --from=uv /uv /uvx /bin/ ADD . /src -RUN make -C /src build +RUN make -C /src build && \ + # Pre-build sysv_ipc to avoid runtime compilation + python3.12 -m pip install --no-cache-dir sysv_ipc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devfile/Containerfile(1 hunks).devfile/Containerfile.client(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
Documented DigitalIOClient |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py (4)
9-13: Enhance class documentation with usage examples.While the documentation provides a basic description, it would be helpful to include:
- Example usage of the class
- Description of available methods and properties
- Explanation of the different drive modes and pull configurations
15-27: Improve drive mode mapping and error handling.Consider the following improvements:
- Replace magic numbers with named constants for better maintainability
- Include the actual drive_mode value in the error message
+# At module level +DRIVE_MODE_PUSH_PULL = 0 +DRIVE_MODE_OPEN_DRAIN = 1 def switch_to_output(self, value: bool = False, drive_mode: DriveMode = DriveMode.PUSH_PULL) -> None: match drive_mode: case DriveMode.PUSH_PULL: - drive_mode = 0 + drive_mode = DRIVE_MODE_PUSH_PULL case DriveMode.OPEN_DRAIN: - drive_mode = 1 + drive_mode = DRIVE_MODE_OPEN_DRAIN case _: - raise ValueError("unrecognized drive_mode") + raise ValueError(f"unrecognized drive_mode: {drive_mode}")
29-43: Apply similar improvements to pull configuration mapping.For consistency with the previous suggestion, consider:
- Using named constants instead of magic numbers
- Including the actual pull value in the error message
+# At module level +PULL_NONE = 0 +PULL_UP = 1 +PULL_DOWN = 2 def switch_to_input(self, pull: Pull | None = None) -> None: match pull: case None: - pull = 0 + pull = PULL_NONE case Pull.UP: - pull = 1 + pull = PULL_UP case Pull.DOWN: - pull = 2 + pull = PULL_DOWN case _: - raise ValueError("unrecognized pull") + raise ValueError(f"unrecognized pull: {pull}")
45-55: Enhance property documentation.The current documentation could be more detailed. Consider adding:
- Possible exceptions that might be raised
- Valid value ranges
- Behavior when reading/writing in different modes (input vs output)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/raspberrypi.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/source/api-reference/drivers/index.md
- docs/source/api-reference/drivers/raspberrypi.md
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (1)
contrib/drivers/raspberrypi/jumpstarter_driver_raspberrypi/client.py (1)
45-55: Consider adding mode validation in value property.The value property might benefit from runtime checks to ensure the pin is in the correct mode:
- Reading value should verify pin is configured as input
- Setting value should verify pin is configured as output
This would prevent potential runtime errors from incorrect usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/source/api-reference/drivers/raspberrypi.md (5)
1-4: Enhance the introduction section.Consider expanding the introduction to:
- Mention the transition to Adafruit Blinka
- Explain the benefits of using Blinka over gpiozero
- Add links to Blinka documentation
# Raspberry Pi drivers -Raspberry Pi drivers are a set of drivers for the various peripherals on Pi and similar single board computers. +Raspberry Pi drivers provide a set of drivers for various peripherals on Pi and similar single board computers. These drivers are built on top of [Adafruit Blinka](https://github.com/adafruit/Adafruit_Blinka), which provides a CircuitPython compatibility layer for Linux boards. + +Key features: +- CircuitPython compatibility through Blinka +- Support for GPIO, I2C, SPI, and UART +- Cross-platform compatibility with other CircuitPython boards
5-13: Expand the configuration examples section.The current example is minimal. Consider adding:
- Multiple configuration examples showing different use cases
- Explanation of the
typefield- Examples for power-related classes mentioned in the PR summary (
DigitalPowerSwitch,DigitalPowerButton)## Driver configuration + +The drivers can be configured through YAML configuration files. The following examples demonstrate various use cases: + +### Basic Digital IO ```yaml export: my_serial: type: "jumpstarter_driver_raspberrypi.driver.DigitalIO" config: pin: "D3"
+### Power Switch Example
+```yaml
+export:
- power_switch:
- type: "jumpstarter_driver_raspberrypi.driver.DigitalPowerSwitch"
- config:
pin: "D4"+
+ +### Power Button Example +yaml
+export:
- power_button:
- type: "jumpstarter_driver_raspberrypi.driver.DigitalPowerButton"
- config:
pin: "D5"+```
--- `20-24`: **Add manual API documentation overview.** While autodoc is useful, consider adding a manual overview of key methods and their use cases before the auto-generated documentation. ```diff ## DigitalIOClient API + +The `DigitalIOClient` class provides the following key functionality: + +- `switch_to_output()`: Configure pin as output with optional initial value and drive mode +- `switch_to_input()`: Configure pin as input with optional pull resistor +- `value`: Get or set the current pin state + ```{eval-rst} .. autoclass:: jumpstarter_driver_raspberrypi.client.DigitalIOClient :members:--- `27-27`: **Fix hyphenation in "push-pull".** The term "push-pull" should be hyphenated. ```diff -Switch pin to push pull output and set output to high +Switch pin to push-pull output and set output to high🧰 Tools
🪛 LanguageTool
[misspelling] ~27-~27: This word is normally spelled with a hyphen.
Context: ...members:## Examples Switch pin to push pull output and set output to high{testc...(EN_COMPOUNDS_PUSH_PULL)
26-37: Enhance code examples section.Consider adding:
- Error handling examples
- Examples for power-related classes
- Shell command examples (as requested in past review comments)
## Examples Switch pin to push-pull output and set output to high ```{testcode} digitalioclient.switch_to_output(value=False, drive_mode=digitalio.DriveMode.PUSH_PULL) # default to low digitalioclient.value = TrueSwitch pin to input with pull up and read value
digitalioclient.switch_to_input(pull=digitalio.Pull.UP) print(digitalioclient.value)
+### Error Handling
+```{testcode}
+try:
- digitalioclient.switch_to_output(drive_mode="INVALID_MODE")
+except ValueError as e:- print(f"Configuration error: {e}")
+```+### Shell Commands
+bash +# Get current pin state +jumpstarter-client my_serial value + +# Set pin high +jumpstarter-client my_serial value --set true +<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [misspelling] ~27-~27: This word is normally spelled with a hyphen. Context: ...members: ``` ## Examples Switch pin to push pull output and set output to high ```{testc... (EN_COMPOUNDS_PUSH_PULL) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between da18d1890358cc16ef75e3398057ae64d60e8835 and dd6263758918782422aedfcf831b6c206361e65b. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/source/api-reference/drivers/raspberrypi.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/source/api-reference/drivers/raspberrypi.md</summary> [misspelling] ~27-~27: This word is normally spelled with a hyphen. Context: ...members: ``` ## Examples Switch pin to push pull output and set output to high ```{testc... (EN_COMPOUNDS_PUSH_PULL) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (3)</summary> * GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client) * GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile) * GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
NickCao
force-pushed
the
gpio-opendrain
branch
from
dd62637 to
f5ec76f
Compare
|
Rebased on current main. |
NickCao
force-pushed
the
gpio-opendrain
branch
from
f5ec76f to
eb32bdf
Compare
|
Thanks, testing it, will merge once validated on a rpi4 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (4)
8-12: Class docstring enhancement
Consider further expanding or refining the docstring to clarify the purpose and usage constraints (e.g., supported hardware or platform requirements).
14-17: Document parameter types
While the function docstring is concise, it does not detail the parametervaluein the description. Including parameter descriptions can improve clarity.
28-31: Docstring consistency
Similar toswitch_to_output, it may help to explicitly describe thepullparameter.
44-50: Property getter usage
Fetching the pin value throughself.call("get_value")is straightforward. Exception handling or timeouts for hardware calls may be considered in the future if hardware misbehaves.packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver.py (2)
59-64: Value setters/getters
Using dedicated methods for setting and retrieving the pin value is standard for hardware drivers. Potential expansions could include error handling for I/O.
93-124: DigitalPowerButton press logic
The toggle-sleep-toggle sequence is a clear approach for simulating a hardware button press. Future improvement could include error-handling or states to ensure consistent press durations if the device is in an unexpected state.packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (2)
18-31: Output test logic
The test code sets and checks the pin’s value. Yet line 31’s comment indicates the hardware is “always True.” It’s not entirely clear from the test if this is intended or if it’s highlighting a driver limitation. Adding a comment or docstring clarifying this quirk might help maintainers.
34-45: Power interface coverage
TestingDigitalPowerSwitchandDigitalPowerButtonwith the same environment variable approach is consistent. The test is straightforward, but it doesn’t assert any final states after callingoff()oron(). Consider checking the final pin state for better coverage.docs/source/api-reference/drivers/raspberrypi.md (1)
7-12: Enhance the configuration example.The example shows a serial driver but uses
DigitalIOtype, which seems inconsistent. Consider:
- Using a more appropriate example name (e.g.,
my_gpioinstead ofmy_serial)- Adding more configuration examples for different use cases
export: - my_serial: + my_gpio: type: "jumpstarter_driver_raspberrypi.driver.DigitalIO" config: pin: "D3"
📜 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)
.devfile/Containerfile(1 hunks).devfile/Containerfile.client(1 hunks)Dockerfile(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/raspberrypi.md(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)packages/jumpstarter-driver-raspberrypi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/source/api-reference/drivers/index.md
- .devfile/Containerfile.client
- Dockerfile
- .devfile/Containerfile
🧰 Additional context used
🪛 LanguageTool
docs/source/api-reference/drivers/raspberrypi.md
[misspelling] ~27-~27: This word is normally spelled with a hyphen.
Context: ...members: ## Examples Switch pin to push pull output and set output to high{testc...
(EN_COMPOUNDS_PUSH_PULL)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (19)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (4)
3-3: Imports look good
No issues found with these imported entities.
19-27: Good use of match-case
This approach correctly translates theDriveModeenum to integer constants for the remote call. The error handling is explicit and clear.
33-42: Pull configuration
MappingPullto numeric codes is logical and consistent. The error handling for unsupported values is well-structured.
52-54: Property setter usage
Setting the pin value via the remote call is consistent with the design. No issues found.packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver.py (9)
1-6: Imports organization
All imports appear valid. No flagged security or maintainability issues here.
8-8: External dependency
jumpstarter_driver_power.driverintroduction looks fine. Ensure that thePowerInterfaceis properly tracked in your distribution for consistent usage.
12-14: Initialization clarity
DigitalIOclearly declares the pin name and the underlyingDigitalInOutdevice. This fosters a neat separation of hardware detail from the driver logic.
18-18: Client method reference
Returning the fully qualified path for the client fosters discoverability. This is good practice.
22-26: Robust error handling for pin
Raising aValueErrorwhen the pin name is invalid is clean and explicit. This is a solid approach for avoiding silent failures.
30-30: Device cleanup
Callingdeinit()ensures resources are freed correctly. No issues here.
33-43: drive_mode match logic
The match-case mapping from integer toDriveModeis consistent with the logic in the client. This is good for a uniform contract.
45-57: pull match logic
Similar toswitch_to_output, the numeric mapping frompullto thePullenum is neatly handled. Error conditions are explicit.
68-87: DigitalPowerSwitch design
These methods (on(),off(),read()) provide a simple, high-level power switch interface. Markingread()asNotImplementedErroris appropriate until the functionality is supported.packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py (3)
4-7: Possible contradictory assertions
Lines 13, 14, and 15 (all in the same context) toggle between assertingclient.valueisTrue, thenFalse, thenTrueagain, without intermediate changes. This might be intentional if mocking is applied, but it's unclear how the state changes. Consider clarifying or adding comments to explain the expected hardware or mock behavior.
9-9: Selective import
ImportingDigitalIOinline is valid for test isolation. No issues found.
11-15: Context manager usage
Usingserve(...) as clientensures clean setup and teardown. Combined with the inline environment variable (BLINKA_OS_AGNOSTIC), this test approach is consistent across multiple test methods.packages/jumpstarter-driver-raspberrypi/pyproject.toml (2)
15-16: LGTM! Dependencies align with the driver rework.The switch from gpiozero to adafruit-blinka aligns with the PR objectives. The addition of jumpstarter-driver-power supports the new power-related functionality.
16-16: Verify adafruit-blinka version compatibility.The specified version (>=8.51.0) includes the sysv_ipc dependency issue mentioned in the PR comments. Consider pinning to a specific version or documenting the build requirements.
docs/source/api-reference/drivers/raspberrypi.md (1)
16-18: Add missing configuration parameters.The parameters table is incomplete. Based on the examples shown below, it's missing parameters for:
value(initial_value)drive_modepull
| ```{testcode} | ||
| digitalioclient.switch_to_output(value=False, drive_mode=digitalio.DriveMode.PUSH_PULL) # default to low | ||
| digitalioclient.value = True | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add import statements to the example.
The example uses undefined symbols (digitalioclient, digitalio). Include the necessary import statements and client initialization.
+from jumpstarter.client import Client
+from digitalio import DriveMode
+
+client = Client()
+digitalioclient = client.get_driver("my_gpio")
+
-digitalioclient.switch_to_output(value=False, drive_mode=digitalio.DriveMode.PUSH_PULL) # default to low
+digitalioclient.switch_to_output(value=False, drive_mode=DriveMode.PUSH_PULL) # default to low
digitalioclient.value = True| ```{testcode} | ||
| digitalioclient.switch_to_input(pull=digitalio.Pull.UP) | ||
| print(digitalioclient.value) | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add import statements and fix undefined symbols.
Similar to the previous example, this one needs proper imports and initialization.
+from jumpstarter.client import Client
+from digitalio import Pull
+
+client = Client()
+digitalioclient = client.get_driver("my_gpio")
+
-digitalioclient.switch_to_input(pull=digitalio.Pull.UP)
+digitalioclient.switch_to_input(pull=Pull.UP)
print(digitalioclient.value)
|
I still need to test with https://pypi.org/project/rpi-lgpio/ in fedora, sorry for the delay. Now there are a few conflicts |
NickCao
force-pushed
the
gpio-opendrain
branch
from
eb32bdf to
ec50c44
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py (5)
10-13: Enhance the docstring with more details.The docstring could be more descriptive by including:
- Purpose of the class
- Available methods and their high-level functionality
- Example usage
- """DigitalIO (Digital GPIO) client class - - Client methods for the DigitalIO driver. - """ + """DigitalIO (Digital GPIO) client class for controlling GPIO pins. + + This class provides methods to: + - Configure pins as input or output with various modes + - Read and write digital values + - Control pin drive modes and pull resistors + + Example: + client = DigitalIOClient(...) + client.switch_to_output(value=True, drive_mode=DriveMode.PUSH_PULL) + client.value = False + """
15-27: Enhance the docstring with parameter details.The docstring should document the parameters and their valid values.
- """ - Switch pin to output mode with given default value and drive mode - """ + """Switch pin to output mode with given default value and drive mode. + + Args: + value: Initial output value (True=high, False=low). + drive_mode: Output drive mode (DriveMode.PUSH_PULL or DriveMode.OPEN_DRAIN). + + Raises: + ValueError: If drive_mode is not recognized. + """
29-43: Enhance the docstring with parameter details.The docstring should document the parameter and its valid values.
- """ - Switch pin to input mode with given pull up/down mode - """ + """Switch pin to input mode with given pull up/down mode. + + Args: + pull: Pull resistor configuration (None=floating, Pull.UP=pull-up, + Pull.DOWN=pull-down). + + Raises: + ValueError: If pull is not recognized. + """
45-51: Enhance the docstring with return value details.The docstring should document the return value and its meaning.
- """ - Current value of the pin - """ + """Get the current value of the pin. + + Returns: + bool: True if the pin is high, False if the pin is low. + """
53-55: Add docstring to the value setter.The property setter should have a docstring documenting the parameter.
@value.setter def value(self, value: bool) -> None: + """Set the value of the pin. + + Args: + value: True to set the pin high, False to set it low. + """ self.call("set_value", value)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver.py (3)
13-66: Add docstrings to the class and methods.The class and its methods should have descriptive docstrings.
class DigitalIO(Driver): + """Driver class for digital GPIO pins using Adafruit Blinka. + + This class provides low-level access to GPIO pins through the Adafruit Blinka + library, supporting both input and output modes with various configurations. + + Args: + pin: Name of the GPIO pin in Adafruit Blinka format (e.g., "D4", "GPIO18"). + """ pin: str device: DigitalInOut = field(init=False) @classmethod def client(cls) -> str: + """Get the client class name for this driver. + + Returns: + str: Fully qualified name of the client class. + """ return "jumpstarter_driver_raspberrypi.client.DigitalIOClient" def __post_init__(self): + """Initialize the GPIO pin. + + Raises: + ValueError: If the pin name is invalid. + """ if hasattr(super(), "__post_init__"): super().__post_init__() # Defaults to input with no pull try: self.device = DigitalInOut(pin=getattr(board, self.pin)) except AttributeError as err: raise ValueError(f"Invalid pin name: {self.pin}") from err def close(self): + """Release the GPIO pin.""" if hasattr(self, "device"): self.device.deinit() @export def switch_to_output(self, value: bool = False, drive_mode: int = 0) -> None: + """Switch the pin to output mode. + + Args: + value: Initial output value (True=high, False=low). + drive_mode: Drive mode (0=PUSH_PULL, 1=OPEN_DRAIN). + + Raises: + ValueError: If drive_mode is not recognized. + """ match drive_mode: case 0: drive_mode = DriveMode.PUSH_PULL case 1: drive_mode = DriveMode.OPEN_DRAIN case _: raise ValueError("unrecognized drive_mode") self.device.switch_to_output(value, drive_mode) @export def switch_to_input(self, pull: int = 0) -> None: + """Switch the pin to input mode. + + Args: + pull: Pull resistor mode (0=None, 1=UP, 2=DOWN). + + Raises: + ValueError: If pull is not recognized. + """ match pull: case 0: pull = None case 1: pull = Pull.UP case 2: pull = Pull.DOWN case _: raise ValueError("unrecognized pull") self.device.switch_to_input(pull) @export def set_value(self, value: bool) -> None: + """Set the pin value. + + Args: + value: True to set the pin high, False to set it low. + """ self.device.value = value @export def get_value(self) -> bool: + """Get the pin value. + + Returns: + bool: True if the pin is high, False if the pin is low. + """ return self.device.value
70-93: Add docstrings to the class and methods.The class and its methods should have descriptive docstrings.
class DigitalPowerSwitch(PowerInterface, DigitalIO): + """Driver class for GPIO-controlled power switches. + + This class implements a power switch interface using a GPIO pin, allowing + control of power to external devices. + + Args: + pin: Name of the GPIO pin in Adafruit Blinka format. + value: Initial output value (True=high, False=low). + drive_mode: Drive mode ("PUSH_PULL" or "OPEN_DRAIN"). + """ value: bool = False drive_mode: str = "PUSH_PULL" def __post_init__(self): + """Initialize the power switch. + + Raises: + ValueError: If the drive mode is invalid. + """ if hasattr(super(), "__post_init__"): super().__post_init__() try: self.device.switch_to_output(value=self.value, drive_mode=getattr(DriveMode, self.drive_mode)) except AttributeError as err: raise ValueError(f"Invalid drive mode: {self.drive_mode}") from err @export def on(self) -> None: + """Turn the power switch on.""" self.device.value = True @export def off(self) -> None: + """Turn the power switch off.""" self.device.value = False @export def read(self) -> AsyncGenerator[PowerReading, None]: + """Read power measurements. + + Not implemented for this class. + + Raises: + NotImplementedError: Always raised. + """ raise NotImplementedError
97-128: Add docstrings to the class and methods.The class and its methods should have descriptive docstrings.
class DigitalPowerButton(PowerInterface, DigitalIO): + """Driver class for GPIO-controlled power buttons. + + This class implements a power button interface using a GPIO pin, simulating + button presses with configurable durations for on/off actions. + + Args: + pin: Name of the GPIO pin in Adafruit Blinka format. + value: Initial output value (True=high, False=low). + drive_mode: Drive mode ("PUSH_PULL" or "OPEN_DRAIN"). + on_press_seconds: Duration of button press for power on (seconds). + off_press_seconds: Duration of button press for power off (seconds). + """ value: bool = False drive_mode: str = "OPEN_DRAIN" on_press_seconds: int = 1 off_press_seconds: int = 5 def __post_init__(self): + """Initialize the power button. + + Raises: + ValueError: If the drive mode is invalid. + """ if hasattr(super(), "__post_init__"): super().__post_init__() try: self.device.switch_to_output(value=self.value, drive_mode=getattr(DriveMode, self.drive_mode)) except AttributeError as err: raise ValueError(f"Invalid drive mode: {self.drive_mode}") from err def press(self, seconds: int) -> None: + """Simulate a button press for the specified duration. + + Args: + seconds: Duration of the button press in seconds. + """ self.device.value = self.value self.device.value = not self.value sleep(seconds) self.device.value = self.value @export def on(self) -> None: + """Simulate a power-on button press.""" self.press(self.on_press_seconds) @export def off(self) -> None: + """Simulate a power-off button press.""" self.press(self.off_press_seconds) @export def read(self) -> AsyncGenerator[PowerReading, None]: + """Read power measurements. + + Not implemented for this class. + + Raises: + NotImplementedError: Always raised. + """ raise NotImplementedError
📜 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)
.devfile/Containerfile(1 hunks).devfile/Containerfile.client(1 hunks)Dockerfile(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/raspberrypi.md(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/client.py(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver.py(1 hunks)packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py(1 hunks)packages/jumpstarter-driver-raspberrypi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/source/api-reference/drivers/index.md
- .devfile/Containerfile.client
- .devfile/Containerfile
- Dockerfile
- packages/jumpstarter-driver-raspberrypi/pyproject.toml
- packages/jumpstarter-driver-raspberrypi/jumpstarter_driver_raspberrypi/driver_test.py
🧰 Additional context used
🪛 LanguageTool
docs/source/api-reference/drivers/raspberrypi.md
[misspelling] ~27-~27: This word is normally spelled with a hyphen.
Context: ...members: ## Examples Switch pin to push pull output and set output to high{testc...
(EN_COMPOUNDS_PUSH_PULL)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (2)
docs/source/api-reference/drivers/raspberrypi.md (2)
6-18: Fix the configuration example and expand the parameter table.The configuration example shows
my_serialbut should use a more appropriate name for a GPIO driver. The parameter table is missing configuration options for power-related classes.```yaml export: - my_serial: + my_gpio: type: "jumpstarter_driver_raspberrypi.driver.DigitalIO" config: pin: "D3" + value: false + drive_mode: "PUSH_PULL"Config parameters
Parameter Description Type Required Default pin Name of the GPIO pin to connect to, in Adafruit Blinka format str yes + value Initial state of the pin when configured as output bool no + drive_mode Drive mode for output pins (PUSH_PULL or OPEN_DRAIN) str no + pull Pull resistor configuration for input pins (UP, DOWN, or None) str no + on_press_seconds Duration of button press for power on (DigitalPowerButton only) int no + off_press_seconds Duration of button press for power off (DigitalPowerButton only) int no --- `28-37`: **Fix the examples with proper imports and initialization.** The examples use undefined symbols and need proper imports. ```diff +from jumpstarter.client import Client +from digitalio import DriveMode, Pull + +client = Client() +digitalioclient = client.get_driver("my_gpio") + -digitalioclient.switch_to_output(value=False, drive_mode=digitalio.DriveMode.PUSH_PULL) # default to low +digitalioclient.switch_to_output(value=False, drive_mode=DriveMode.PUSH_PULL) # default to low digitalioclient.value = True -digitalioclient.switch_to_input(pull=digitalio.Pull.UP) +digitalioclient.switch_to_input(pull=Pull.UP) print(digitalioclient.value)
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements
Dependencies
gpiozeroand adding Adafruit Blinka and power driver support.