feat: ble write notify driver: added basic ble write and notify stream driver and cli console#726
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new BLE driver package Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Client as BleClient
participant Driver as BleWriteNotifyStream
participant Device as BleakClient
participant Console
CLI->>Client: start-console / info
Client->>Driver: connect() (exportstream)
alt successful connect
Driver->>Device: BleakClient.connect()
Driver->>Device: discover & validate service/characteristics
Note right of Driver `#DDEEF7`: register notify handler → enqueue to notify_queue
Driver->>Client: yield AsyncBleWrapper (stream)
Client->>Console: run(stream)
par IO
Console->>Driver: stream.send(data) → Device.write_gatt_char
and
Device->>Driver: notify(data) → enqueue
Driver->>Console: notify_queue → Console writes to stdout
end
else failure
Driver->>Client: raise BleakError / disconnect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py (1)
1-1: Address the TODO: Add test coverage for the BLE driver.Test coverage is missing for the new BLE driver functionality. Consider adding tests for connection, streaming, and error handling scenarios.
Would you like me to generate a test suite template or open an issue to track this task?
packages/jumpstarter-driver-ble/README.md (1)
48-60: Add language identifier to the code block.The CLI usage example should specify a language identifier (e.g.,
consoleortext) for proper rendering.Apply this diff:
-``` +```console jumpstarter ⚡ local ➤ j ble Usage: j ble [OPTIONS] COMMAND [ARGS]...packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (4)
14-20: Use proper logging instead of print statement.The notification handler uses
print()for warnings. While this is a callback function without direct logger access, consider passing a logger instance to the handler for consistent logging across the driver.Example approach - modify the handler signature:
-def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue): +def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue, logger=None): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: - print("Warning: Data queue is full, dropping message") + if logger: + logger.warning("Data queue is full, dropping message") + else: + print("Warning: Data queue is full, dropping message")Then update the handler creation at line 115-116 to pass the logger.
22-34: Use @DataClass for consistency.
AsyncBleConfigis a simple data holder that could benefit from the@dataclassdecorator for consistency withAsyncBleWrapperbelow.+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
114-120: Remove unnecessary truthiness check.Line 114 checks if
notify_char_uuidis truthy, but it's a required field in the class definition (line 63). This check is unnecessary unless you intend to make this field optional.If
notify_char_uuidshould always be present, remove the check:- # register notification handler if notify_char_uuid is provided - if self.notify_char_uuid: - notify_handler = partial( - _ble_notify_handler, data_queue=self.notify_queue) - await client.start_notify(self.notify_char_uuid, notify_handler) - self.logger.info( - "Setting up notification handler for characteristic UUID: %s", self.notify_char_uuid) + # register notification handler + notify_handler = partial( + _ble_notify_handler, data_queue=self.notify_queue) + await client.start_notify(self.notify_char_uuid, notify_handler) + self.logger.info( + "Setting up notification handler for characteristic UUID: %s", self.notify_char_uuid)Alternatively, if the field should be optional, add a type hint and default value to make that explicit.
128-128: Remove commented-out code.Line 128 contains commented-out code for
read_char_uuid. Remove it or implement the functionality if needed.config=AsyncBleConfig( address=self.address, service_uuid=self.service_uuid, write_char_uuid=self.write_char_uuid, - # read_char_uuid=self.read_char_uuid, notify_char_uuid=self.notify_char_uuid, ),
📜 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)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-all/pyproject.toml
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
PexpectAdapter(11-19)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
BleConsole(16-65)run(20-22)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-54)command(90-102)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(20-22)info(52-54)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(73-74)receive(45-46)send(42-43)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py
[error] 1-1: Ruff formatting check failed. Import block is un-sorted or un-formatted. (I001)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py
[failure] 1-11: Ruff (I001)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py:1:1: I001 Import block is un-sorted or un-formatted
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py
[failure] 91-91: Ruff (C400)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py:91:30: C400 Unnecessary generator (rewrite as a list comprehension)
[failure] 11-11: Ruff (F811)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py:11:19: F811 Redefinition of unused BleakClient from line 9
[failure] 1-14: Ruff (I001)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py:1:1: I001 Import block is un-sorted or un-formatted
🪛 markdownlint-cli2 (0.18.1)
packages/jumpstarter-driver-ble/README.md
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-driver-ble/pyproject.toml (1)
1-37: LGTM: Package configuration follows repository standards.The pyproject.toml configuration is well-structured and consistent with other driver packages in the repository. Dependencies are appropriate for a BLE driver implementation.
packages/jumpstarter-all/pyproject.toml (1)
15-15: LGTM: BLE driver correctly added to meta-package.The new BLE driver has been properly added to the meta-package dependencies in alphabetical order.
pyproject.toml (1)
10-10: LGTM: BLE driver workspace member configured correctly.The workspace member entry is properly positioned and follows the repository's workspace configuration pattern.
packages/jumpstarter-driver-ble/examples/exporter.yaml (1)
1-16: LGTM: Example configuration is clear and complete.The exporter configuration provides a good example with all required BLE parameters (address, service UUID, characteristic UUIDs).
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (1)
11-64: LGTM: Client implementation follows driver patterns correctly.The BleClient class properly extends DriverClient and implements the expected interface patterns:
- Method delegation via
self.call()- Pexpect adapter integration
- CLI command structure using driver_click_group decorator
Once the import order is fixed, this implementation is ready.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
16-65: LGTM: Console implementation is well-structured with proper async I/O handling.The BleConsole class correctly:
- Manages terminal raw mode with proper cleanup
- Uses AnyIO task groups for concurrent bidirectional streaming
- Implements triple Ctrl-B exit mechanism
- Handles screen clearing on exit
52-65: Verify Ctrl-B forwarding behavior to remote device.The current implementation sends Ctrl-B characters to the remote device before checking if three consecutive presses occurred. This means the remote device receives all Ctrl-B bytes before the console exits. Verify this is the intended behavior, as users might expect Ctrl-B to be consumed locally without being forwarded.
whati001
force-pushed
the
feat/ble_console
branch
from
bbf7ba3 to
95018c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/jumpstarter-driver-ble/README.md (1)
48-60: Add language identifier to the fenced code block.The code block should specify a language for proper syntax highlighting and rendering.
Apply this diff:
-``` +```console jumpstarter ⚡ local ➤ j ble Usage: j ble [OPTIONS] COMMAND [ARGS]...As per static analysis hints.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
13-18: Use proper logging instead of print statement.Library code should use logging instead of print() to allow users to control log output and levels.
Consider passing a logger to the notification handler:
-def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue): +def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue, logger=None): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: - print("Warning: Data queue is full, dropping message") + if logger: + logger.warning("Data queue is full, dropping message")Then update the partial call at line 116-117 to include the logger:
notify_handler = partial( _ble_notify_handler, data_queue=notify_queue, logger=self.logger)
21-32: Consider using dataclass for consistency and clarity.
AsyncBleConfigcould be a dataclass likeAsyncBleWrapperfor consistency and to benefit from automatic__init__,__repr__, and type annotations.Apply this diff:
+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
122-131: Remove commented-out code.Line 129 contains commented-out code that references a non-existent
read_char_uuidattribute. This should be removed.Apply this diff:
async with AsyncBleWrapper( client=client, notify_queue=notify_queue, config=AsyncBleConfig( address=self.address, service_uuid=self.service_uuid, write_char_uuid=self.write_char_uuid, - # read_char_uuid=self.read_char_uuid, notify_char_uuid=self.notify_char_uuid, ), ) as stream:
📜 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)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-all/pyproject.toml
- packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py
- pyproject.toml
- packages/jumpstarter-driver-ble/examples/exporter.yaml
- packages/jumpstarter-driver-ble/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(69-70)receive(44-45)send(41-42)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(21-23)info(53-55)
🪛 markdownlint-cli2 (0.18.1)
packages/jumpstarter-driver-ble/README.md
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (1)
82-145: LGTM! Core BLE driver implementation is solid.The connection management, characteristic validation, and error handling are well-implemented. Previous critical issues from earlier reviews have been properly addressed:
- Queue is created fresh per connection (line 107)
- Service validation now properly errors if service is missing (lines 96-97)
- Imports are clean and properly ordered
The async context manager pattern is correctly used, and the BLE stream wrapper appropriately implements the
ObjectStreaminterface.packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (1)
16-65: LGTM! Clean console implementation with proper terminal handling.The console implementation demonstrates good practices:
- Proper terminal state save/restore with cleanup (lines 24-33)
- Binary-safe I/O using
bufferfor BLE data- Clear user communication about the exit sequence (line 38)
- Appropriate use of task groups for concurrent I/O
- Exception group handling with
except*for clean exitThe triple Ctrl-B detection mechanism is user-friendly and the raw mode handling ensures all keypresses are captured correctly.
whati001
force-pushed
the
feat/ble_console
branch
from
95018c1 to
306fd44
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (1)
1-10: Fix import order to resolve pipeline failure.The imports remain incorrectly ordered, causing the ruff check to fail. Standard library imports must come first, followed by third-party imports (click, pexpect), then first-party imports (jumpstarter, jumpstarter_driver_network), and finally local imports.
Apply this diff to fix the import order:
from contextlib import contextmanager import click -from jumpstarter_driver_network.adapters import PexpectAdapter from pexpect.fdpexpect import fdspawn +from jumpstarter_driver_network.adapters import PexpectAdapter -from .console import BleConsole from jumpstarter.client import DriverClient from jumpstarter.client.decorators import driver_click_group + +from .console import BleConsole
🧹 Nitpick comments (4)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (1)
24-33: Consider adding TTY check before terminal manipulation.If stdin is not a TTY (e.g., piped input or non-interactive session),
termios.tcgetattr()will raise anOSError. While the console is intended for interactive use, adding a guard would make the error message clearer.Consider this enhancement:
@contextmanager def setraw(self): + if not sys.stdin.isatty(): + raise RuntimeError("BLE console requires an interactive terminal (stdin is not a TTY)") original = termios.tcgetattr(sys.stdin.fileno()) try:packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
13-18: Consider using logging module for warnings.The warning message uses
loggingmodule directly.Apply this diff:
+import logging + +_logger = logging.getLogger(__name__) + def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: - print("Warning: Data queue is full, dropping message") + _logger.warning("Data queue is full, dropping message")
21-32: Consider using @DataClass for consistency.
AsyncBleConfigmanually implements__init__, while other classes in the file (AsyncBleWrapper,Ble) use the@dataclassdecorator. Using@dataclasshere would improve consistency and reduce boilerplate.Apply this diff:
+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
122-133: Remove commented-out parameter.Line 129 contains a commented-out
read_char_uuidparameter that should be removed to clean up the code.Apply this diff:
async with AsyncBleWrapper( client=client, notify_queue=notify_queue, config=AsyncBleConfig( address=self.address, service_uuid=self.service_uuid, write_char_uuid=self.write_char_uuid, - # read_char_uuid=self.read_char_uuid, notify_char_uuid=self.notify_char_uuid, ), ) as stream:
📜 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)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-ble/examples/exporter.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-ble/pyproject.toml
- pyproject.toml
- packages/jumpstarter-all/pyproject.toml
- packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (5)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
PexpectAdapter(11-19)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
BleConsole(16-65)run(20-22)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
client(69-70)info(73-80)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-54)command(90-102)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(69-70)receive(44-45)send(41-42)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(21-23)info(53-55)
🪛 markdownlint-cli2 (0.18.1)
packages/jumpstarter-driver-ble/README.md
48-48: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (17)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (4)
21-23: LGTM!The method correctly delegates to the driver's exported
info()method.
25-33: LGTM!The method correctly uses the stack context manager to open a pexpect session, consistent with other Jumpstarter drivers.
35-44: LGTM!The context manager correctly provides a PexpectAdapter for BLE interactions.
46-65: LGTM!The CLI implementation correctly follows Jumpstarter patterns and properly integrates the BLE console with clear user instructions.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (6)
1-10: LGTM!Import order correctly follows Python conventions (stdlib → third-party → first-party).
12-13: LGTM!Custom exception appropriately used for console exit signaling with AnyIO exception groups.
20-22: LGTM!Correctly uses the portal pattern to bridge synchronous and asynchronous execution contexts.
35-43: LGTM!Properly uses AnyIO task groups for concurrent bidirectional I/O and exception group handling.
45-50: LGTM!The implementation correctly handles binary-safe streaming from BLE to stdout with appropriate flushing for interactive console use.
52-65: LGTM!The stdin-to-BLE forwarding correctly implements the triple Ctrl-B exit sequence with proper counter management. Byte-by-byte reading ensures immediate detection of the escape sequence.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (7)
1-11: LGTM!Import order now correctly follows conventions (stdlib → third-party → first-party) with no duplicates.
35-52: LGTM!
AsyncBleWrappercorrectly implements theObjectStreaminterface for BLE characteristics, withsend_eof()appropriately implemented as a no-op for BLE.
55-66: LGTM!Class declaration and initialization follow Jumpstarter Driver patterns correctly, with proper docstring syntax.
68-70: LGTM!Correctly returns the client class path as required by the Driver base class.
72-80: LGTM!The exported
info()method correctly provides BLE connection details in a readable format.
82-97: LGTM!Service and characteristic validation is now complete, properly raising errors when required UUIDs are not found on the device.
99-145: LGTM!The
connect()method correctly implements the BLE connection lifecycle with proper error handling, characteristic validation, and per-connection queue initialization. All previously identified issues have been addressed.
whati001
force-pushed
the
feat/ble_console
branch
from
306fd44 to
d231a7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (1)
1-10: Fix import order to resolve linter failure.The imports are still not correctly ordered. Third-party imports (
pexpecton line 5) must come before first-party imports (jumpstarter_driver_networkon line 4).Apply this diff to fix the import order:
from contextlib import contextmanager import click -from jumpstarter_driver_network.adapters import PexpectAdapter from pexpect.fdpexpect import fdspawn -from .console import BleConsole +from jumpstarter_driver_network.adapters import PexpectAdapter from jumpstarter.client import DriverClient from jumpstarter.client.decorators import driver_click_group + +from .console import BleConsole
🧹 Nitpick comments (5)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
13-18: Replaceprint()with proper logging.Using
print()for warnings in driver code bypasses the logging framework and doesn't respect log level configuration. The Ble driver class already hasself.loggeravailable.Apply this diff to use logging:
-def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue): +def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue, logger=None): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: - print("Warning: Data queue is full, dropping message") + if logger: + logger.warning("Data queue is full, dropping message")Then update the handler creation at line 116-117:
if self.notify_char_uuid: notify_handler = partial( - _ble_notify_handler, data_queue=notify_queue) + _ble_notify_handler, data_queue=notify_queue, logger=self.logger)
21-32: Consider using@dataclassfor consistency.The file uses
@dataclassforAsyncBleWrapperandBle, butAsyncBleConfigis a plain class. Using a dataclass would reduce boilerplate and maintain consistency.Apply this diff:
+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
122-132: Remove commented-out code.Line 129 contains a commented-out parameter
read_char_uuidthat should either be implemented or removed to avoid confusion.Apply this diff to remove the commented line:
async with AsyncBleWrapper( client=client, notify_queue=notify_queue, config=AsyncBleConfig( address=self.address, service_uuid=self.service_uuid, write_char_uuid=self.write_char_uuid, - # read_char_uuid=self.read_char_uuid, notify_char_uuid=self.notify_char_uuid, ), ) as stream:packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
45-50: Add error handling for stream closure.The method has no handling for stream closure or disconnection. If the BLE device disconnects,
stream.receive()may raise an exception, causing the task to crash and potentially leaving the terminal in an inconsistent state.Apply this diff to handle stream closure gracefully:
async def __ble_to_stdout(self, stream): stdout = FileWriteStream(sys.stdout.buffer) - while True: - data = await stream.receive() - await stdout.send(data) - sys.stdout.flush() + try: + while True: + data = await stream.receive() + await stdout.send(data) + sys.stdout.flush() + except Exception: + # Stream closed or error - exit console + raise BleConsoleExit
52-65: Add error handling for stream closure.Similar to
__ble_to_stdout, this method lacks error handling for stream closure. If the BLE device disconnects,stream.send()may fail, causing the task to crash.Apply this diff:
async def __stdin_to_ble(self, stream): stdin = FileReadStream(sys.stdin.buffer) ctrl_b_count = 0 - while True: - data = await stdin.receive(max_bytes=1) - if not data: - continue - if data == b"\x02": # Ctrl-B - ctrl_b_count += 1 - if ctrl_b_count == 3: - raise BleConsoleExit - else: - ctrl_b_count = 0 - await stream.send(data) + try: + while True: + data = await stdin.receive(max_bytes=1) + if not data: + continue + if data == b"\x02": # Ctrl-B + ctrl_b_count += 1 + if ctrl_b_count == 3: + raise BleConsoleExit + else: + ctrl_b_count = 0 + await stream.send(data) + except Exception: + # Stream closed or error - exit console + raise BleConsoleExit
📜 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)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter-driver-ble/examples/exporter.yaml
- packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py
- packages/jumpstarter-driver-ble/pyproject.toml
- packages/jumpstarter-all/pyproject.toml
- pyproject.toml
- packages/jumpstarter-driver-ble/README.md
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(69-70)receive(44-45)send(41-42)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(21-23)info(53-55)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
PexpectAdapter(11-19)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
BleConsole(16-65)run(20-22)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
whati001
force-pushed
the
feat/ble_console
branch
from
d231a7e to
cdc37c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (1)
1-9: Import order still not matching standard conventions.The imports are not fully sorted according to standard Python conventions (stdlib → third-party → first-party → local). Lines 4-5 show
jumpstarter_driver_network(first-party) beforepexpect(third-party), and the local import.consolecomes before other first-partyjumpstarterimports.The past review suggested this ordering:
from contextlib import contextmanager import click from pexpect.fdpexpect import fdspawn from jumpstarter_driver_network.adapters import PexpectAdapter from jumpstarter.client import DriverClient from jumpstarter.client.decorators import driver_click_group from .console import BleConsole
🧹 Nitpick comments (4)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
13-18: Use logger instead of print for dropped messages.The notification handler uses
print()to warn about dropped messages. In a driver/library context, this should use the driver's logger for consistency with the rest of the codebase.Consider passing the logger to the handler and logging the warning:
def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue, logger=None): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: if logger: logger.warning("Data queue is full, dropping message")Then update the partial at line 114-115 to include the logger:
notify_handler = partial( _ble_notify_handler, data_queue=notify_queue, logger=self.logger)
21-32: Consider using @DataClass for consistency.
AsyncBleConfigmanually defines__init__, whileAsyncBleWrapper(line 35) uses the@dataclassdecorator. For consistency and reduced boilerplate, consider converting this class to a dataclass.Apply this diff to use @DataClass:
+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
127-127: Remove commented code.Line 127 contains commented-out code for
read_char_uuid. This should be removed to keep the codebase clean.Apply this diff:
config=AsyncBleConfig( address=self.address, service_uuid=self.service_uuid, write_char_uuid=self.write_char_uuid, - # read_char_uuid=self.read_char_uuid, notify_char_uuid=self.notify_char_uuid, ),packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (1)
57-63: Consider clarifying the exit instruction message.Line 61 says "exit with CTRL+B x 3 times" which is slightly informal. For consistency with the console message (console.py line 39), consider using clearer phrasing.
Apply this diff for clearer messaging:
@base.command() def start_console(): """Start BLE console""" click.echo( - "\nStarting ble console ... exit with CTRL+B x 3 times\n") + "\nStarting BLE console. Press Ctrl-B three times to exit.\n") console = BleConsole(ble_client=self) console.run()
📜 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)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-ble/examples/exporter.yaml
- pyproject.toml
- packages/jumpstarter-driver-ble/README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-all/pyproject.toml
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(69-70)receive(44-45)send(41-42)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(21-23)info(53-55)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
PexpectAdapter(11-19)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
BleConsole(16-66)run(20-22)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-54)command(90-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (18)
packages/jumpstarter-all/pyproject.toml (1)
14-16: Dependency placement is alphabetically correct.The new BLE driver dependency is properly positioned within the alphabetically-ordered list and follows the established pattern.
packages/jumpstarter-driver-ble/pyproject.toml (2)
23-27: Pytest configuration looks good.The pytest setup with coverage reporting, log handling, and testpaths follows a standard pattern for test automation within the Jumpstarter framework.
15-21: VCS versioning and build tooling align with existing patterns.The Hatch configuration for version control, metadata URLs, and build system requirements is consistent with other driver packages in the repository.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (6)
1-9: LGTM!The imports are well-organized and appropriate for the terminal-based BLE console implementation.
12-13: LGTM!Clean exception definition for handling console exit signals.
16-22: LGTM!The initialization and run method correctly bridge synchronous and asynchronous execution using the portal pattern.
24-33: LGTM!The terminal raw mode handling is correct, with proper attribute saving/restoration and screen cleanup on exit.
35-44: LGTM!The async streaming logic is well-structured with proper exception group handling. The connection message now correctly uses
\r\nfor raw mode display.
46-66: LGTM!The bidirectional I/O streaming logic is well-implemented with proper exit detection. The Ctrl-B bytes are forwarded to the device before exit detection (line 66), which means all user input is transmitted - this appears intentional for a transparent console session.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (6)
1-10: LGTM!The imports are properly organized following Python conventions (stdlib, third-party, local) with no duplicates.
35-52: LGTM!The
AsyncBleWrappercorrectly implements theObjectStreaminterface with appropriate send/receive/aclose methods. The no-opsend_eofis appropriate since BLE characteristics lack explicit EOF signaling.
55-66: LGTM!The driver class is properly structured with required BLE configuration fields and correct initialization chaining.
68-78: LGTM!The client locator and info methods are correctly implemented with clean formatting.
80-95: LGTM!The characteristic validation properly checks for service existence and required characteristics, raising descriptive errors when components are missing.
135-143: LGTM!The error handling properly logs connection failures and raises descriptive exceptions.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (3)
12-23: LGTM!The class documentation is clear, and the
infomethod correctly delegates to the driver.
25-44: LGTM!The pexpect integration correctly uses
PexpectAdapterand follows established patterns from other drivers in the codebase.
46-65: LGTM!The CLI implementation correctly uses the driver_click_group pattern and provides appropriate commands for BLE interaction.
| if self.notify_char_uuid: | ||
| notify_handler = partial( | ||
| _ble_notify_handler, data_queue=notify_queue) | ||
| await client.start_notify(self.notify_char_uuid, notify_handler) | ||
| self.logger.info( | ||
| "Setting up notification handler for characteristic UUID: %s", self.notify_char_uuid) | ||
|
|
||
| async with AsyncBleWrapper( | ||
| client=client, | ||
| notify_queue=notify_queue, |
There was a problem hiding this comment.
Potential hang if receive() is called without notification handler.
When notify_char_uuid is falsy (line 113), no notification handler is registered, but the AsyncBleWrapper is still created with the notify_queue. If a consumer calls receive() on the stream without notifications being enabled, it will block indefinitely waiting for data that will never arrive.
Consider one of these approaches:
- Document that
receive()should only be called whennotify_char_uuidis configured - Make
notify_char_uuida required field if receiving is a core feature - Have
receive()raise a descriptive error if called without notification support
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py around lines
113-122, creating AsyncBleWrapper with a notify_queue even when
self.notify_char_uuid is falsy can cause receive() to block forever; fix by (1)
only create and pass notify_queue to AsyncBleWrapper when self.notify_char_uuid
is truthy (avoid creating an unused queue), and (2) add a clear guard in the
stream.receive() implementation to raise a descriptive exception if
notifications are not enabled (e.g., raise RuntimeError/ValueError with
guidance) so callers get immediate, actionable feedback instead of hanging.
cdc37c3 to
3e3d05e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
13-18: Use logger instead of print() for warnings.The
print()statement on line 18 bypasses the driver's logging infrastructure. This warning won't appear in structured logs and can't be filtered by log level.Since this is a module-level function without access to a logger instance, consider passing a logger as a parameter:
-def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue): +def _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue, logger): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: - print("Warning: Data queue is full, dropping message") + logger.warning("Data queue is full, dropping message")Then update the handler registration in
connect()around line 113:notify_handler = partial( - _ble_notify_handler, data_queue=notify_queue) + _ble_notify_handler, data_queue=notify_queue, logger=self.logger)
21-32: Consider using @DataClass for consistency.The
AsyncBleConfigclass uses manual__init__, while other classes in this file (AsyncBleWrapper,Ble) use the@dataclassdecorator. Using@dataclasswould reduce boilerplate and maintain consistency.Apply this diff:
+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
📜 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)
packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-driver-ble/pyproject.toml
- pyproject.toml
- packages/jumpstarter-driver-ble/README.md
- packages/jumpstarter-all/pyproject.toml
- packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(21-23)info(53-55)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (5)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
PexpectAdapter(11-19)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
BleConsole(16-66)run(20-22)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
client(69-70)info(73-78)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-54)command(90-102)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(69-70)receive(44-45)send(41-42)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (15)
packages/jumpstarter-driver-ble/examples/exporter.yaml (1)
1-16: LGTM! Example configuration is well-structured.The ExporterConfig manifest correctly defines the BLE driver type and all required configuration fields (address, service_uuid, write_char_uuid, notify_char_uuid) that align with the
Bledriver class implementation.packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (4)
35-52: LGTM! Stream wrapper implementation is correct.The
AsyncBleWrapperproperly implements theObjectStreaminterface with appropriate BLE-specific behavior:
send()writes to the GATT characteristicreceive()retrieves from the notification queuesend_eof()is correctly a no-op (BLE characteristics lack explicit EOF)aclose()handles disconnection
55-78: LGTM! Driver class structure and info() method are well-implemented.The
Bledriver class correctly:
- Defines all required configuration fields
- Implements the required
client()classmethod- Provides a clear
info()method that returns formatted configuration details- Properly chains
__post_init__to parent class
80-95: LGTM! Characteristic validation is thorough.The
_check_ble_characteristicsmethod properly validates:
- Service UUID existence on the device
- Write characteristic UUID presence in the service
- Notify characteristic UUID presence in the service
All error cases raise descriptive
BleakErrorexceptions.
97-142: Verify whether notify characteristic should be optional for write-only use cases.The current implementation requires
notify_char_uuid(line 62, typed asstrnotOptional[str]) and always registers a notification handler (line 115). While_check_ble_characteristicsvalidates its existence, this design assumes all BLE interactions need bidirectional communication.For write-only BLE devices (e.g., controlling actuators without feedback), requiring a notify characteristic may be overly restrictive.
Consider whether
notify_char_uuidshould be optional:notify_char_uuid: str | None = NoneThen conditionally register the notification handler:
if self.notify_char_uuid: notify_handler = partial( _ble_notify_handler, data_queue=notify_queue) await client.start_notify(self.notify_char_uuid, notify_handler) self.logger.info( "Setting up notification handler for characteristic UUID: %s", self.notify_char_uuid)If bidirectional communication is always required by your use case, the current design is appropriate and you can disregard this suggestion.
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (5)
1-14: LGTM! Imports and exception definition are appropriate.The imports cover terminal control (termios, tty), async operations (anyio), and the custom
BleConsoleExitexception is correctly defined for graceful exit signaling.
16-23: LGTM! Console initialization and run flow are correct.The
BleConsoleproperly stores the client reference and orchestrates the raw terminal mode setup with the async run loop via the portal.
24-33: LGTM! Terminal raw mode handling is correct.The
setraw()context manager properly:
- Saves original terminal attributes
- Sets raw mode for direct character input
- Restores attributes on exit
- Clears the screen with ANSI escape sequences
35-44: LGTM! Async run flow correctly orchestrates bidirectional I/O.The
__run()method properly:
- Opens the BLE stream connection
- Prints the connection message with correct line endings for raw mode (
\r\n)- Sets up concurrent tasks for bidirectional data flow
- Handles
BleConsoleExitgracefully with exception groups
46-51: LGTM! BLE-to-stdout streaming is correctly implemented.The
__ble_to_stdout()method properly receives data from the BLE stream and writes it to stdout with immediate flushing for responsive output.packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (5)
1-19: LGTM! Imports and documentation are well-organized.The imports follow proper ordering (stdlib, third-party, local), and the class docstring clearly explains the BLE client's communication model using write and notify characteristics.
21-23: LGTM! Info method correctly delegates to driver.The
info()method properly proxies the call to the driver'sinfo()method.
25-33: LGTM! Pexpect session management is correct.The
open()method properly manages the pexpect session lifecycle usingstack.enter_context()and includes helpful documentation referencing pexpect docs.
35-44: LGTM! Pexpect adapter context manager is correctly implemented.The
pexpect()method properly wraps thePexpectAdapterin a context manager, passing the client instance appropriately.
46-65: LGTM! CLI commands are well-implemented and user-friendly.The
cli()method provides:
- An
infocommand that displays target information- A
start_consolecommand that launches the interactive BLE console- Clear user messaging about exit behavior (Ctrl-B × 3)
The Click integration follows the driver framework patterns.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
It looks great in general, just a couple of nit comments, the only important one now is the class name so we leave room for additional functionality in the driver down the line.
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class Ble(Driver): |
There was a problem hiding this comment.
| class Ble(Driver): | |
| class BleWriteNotifyStream(Driver): |
What do you think about this name (same for the client BleWriteNotifyStreamClient )
A bit lenghty...
but would allow room for creating more types of ble drivers (for other purposes) over time in this package.
There was a problem hiding this comment.
True, I have changed the name for the driver and client to include WriteNotify.
Personally, I would prefer to have a generic BLE stream driver, which simply exposes all characteristics as stream. This driver could be used by other drivers as base to build things like this console. But because I am not sure if jumpstarter supports multistage drivers (driver depends on driver), I went with this impl yet.
| from jumpstarter.client import DriverClient | ||
|
|
||
|
|
||
| class BleConsoleExit(Exception): |
There was a problem hiding this comment.
I would (in a later PR, or... also feel free to do it now, ..), move the pyserial console functionality here under "console.py" https://github.com/jumpstarter-dev/jumpstarter/tree/main/packages/jumpstarter/jumpstarter/streams and, then reuse it from both places pyserial, and your driver. It is the same code, we can maintain it just once :)
The console could receive a init parameter for the banner: BLE Console, vs Serial Console , vs... something else.
Happy to help with that later, no worries.
There was a problem hiding this comment.
Indeed, removing the duplicate code is a good idea. I can work on this later
There was a problem hiding this comment.
I am not sure how smart the AI (coderabbitai) is, but maybe it complains because the BLE driver does not have a EOF read function (has a pass).
In addition, the read blocks without a timeout via queue.get().
Hence, to receive an empty data set is not possible.
Should I remove the if and code block?
There was a problem hiding this comment.
don't worry, always take the comments from coderabbitai with a grain of salt, it is not perfect. If it's working for you keep it as it is, we can refactor code later.
| async def __run(self): | ||
| async with self.ble_client.stream_async(method="connect") as stream: | ||
| try: | ||
| print( |
There was a problem hiding this comment.
btw, you hace this message also on the CLI :)
There was a problem hiding this comment.
I know, this was on purpose, to inform the user how to exit the console.
To stay aligned with the serial console, I have removed it.
| $ pip3 install --extra-index-url {{index_url}} jumpstarter-driver-ble | ||
| ``` | ||
|
|
||
| ## Configuration |
There was a problem hiding this comment.
You need to add a reference in:
Also above in the that index.md under the "communication interfaces"
And also a symbolic link from docs/source/reference/package-apis/drivers/ble.md to this README.md
There was a problem hiding this comment.
whati001
force-pushed
the
feat/ble_console
branch
from
3e3d05e to
d0426a0
Compare
whati001
changed the title
whati001
changed the title
whati001
force-pushed
the
feat/ble_console
branch
from
d0426a0 to
f6cb9dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py (1)
1-1: Add test coverage for the BLE driver.The TODO indicates missing tests. Consider adding tests for:
- BLE connection establishment and disconnection
- Characteristic validation (_check_ble_characteristics)
- Stream send/receive operations
- Error handling for missing services/characteristics
Would you like me to generate a test suite for the BLE driver, or open an issue to track this task?
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
13-18: Consider using the logging module for consistent logging.The warning message uses
print(), which bypasses the driver's logging configuration. Consider importing theloggingmodule and using a logger instance, or pass the logger as a parameter to maintain consistent log formatting and levels.Apply this diff:
import asyncio +import logging from contextlib import asynccontextmanager from dataclasses import dataclass from functools import partialdef _ble_notify_handler(_sender: BleakGATTCharacteristic, data: bytearray, data_queue: asyncio.Queue): """Notification handler that puts received data into a queue.""" try: data_queue.put_nowait(data) except asyncio.QueueFull: - print("Warning: Data queue is full, dropping message") + logging.getLogger(__name__).warning("Data queue is full, dropping message")
21-32: Simplify with dataclass decorator.
AsyncBleConfigis a simple data holder that would benefit from the@dataclassdecorator for cleaner, more maintainable code.Apply this diff:
+@dataclass class AsyncBleConfig(): - def __init__( - self, - address: str, - service_uuid: str, - write_char_uuid: str, - notify_char_uuid: str, - ): - self.address = address - self.service_uuid = service_uuid - self.write_char_uuid = write_char_uuid - self.notify_char_uuid = notify_char_uuid + address: str + service_uuid: str + write_char_uuid: str + notify_char_uuid: str
📜 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 (11)
docs/source/reference/package-apis/drivers/ble.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-ble/README.md(1 hunks)packages/jumpstarter-driver-ble/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py(1 hunks)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver_test.py(1 hunks)packages/jumpstarter-driver-ble/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/reference/package-apis/drivers/ble.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-ble/README.md
- packages/jumpstarter-driver-ble/examples/exporter.yaml
- packages/jumpstarter-all/pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T13:45:58.271Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:15-15
Timestamp: 2025-11-05T13:45:58.271Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, pexpect is intentionally used as a transitive dependency through the jumpstarter-driver-pyserial package. The flashers package does not declare pexpect as a direct dependency because the pyserial driver package is intended to control the pexpect version.
Applied to files:
packages/jumpstarter-driver-ble/pyproject.toml
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (2)
info(21-23)info(53-55)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
PexpectAdapter(11-19)packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
BleConsole(16-64)run(20-22)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)packages/jumpstarter/jumpstarter/client/decorators.py (2)
driver_click_group(13-54)command(90-102)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (2)
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (3)
client(74-75)receive(44-45)send(41-42)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-105)call(42-52)
🪛 LanguageTool
docs/source/reference/package-apis/drivers/index.md
[uncategorized] ~33-~33: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... (jumpstarter-driver-ble) - Bluetooth Low Energy communication * CAN (`jum...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
🔇 Additional comments (6)
docs/source/reference/package-apis/drivers/index.md (1)
33-33: LGTM! Documentation addition is consistent and properly integrated.The BLE driver entry is correctly added to the Communication Drivers section (line 33) and the toctree (line 88) in alphabetical order, following the same pattern as existing drivers. The changes integrate seamlessly with the existing documentation structure.
Note: The static analysis tool flagged "Bluetooth Low Energy" as requiring hyphenation, but this is the proper technical name and not being used as a compound adjective, so this is a false positive.
Also applies to: 88-88
pyproject.toml (1)
10-10: LGTM!The workspace member addition is correctly placed in alphabetical order and follows the established pattern for driver packages.
packages/jumpstarter-driver-ble/pyproject.toml (1)
1-37: LGTM!The package configuration follows repository conventions:
- External dependencies (
anyio,bleak) have appropriate version constraints- Internal
jumpstarterdependencies are correctly unversioned per repository pattern- Build and test configuration aligns with other driver packages
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/driver.py (1)
55-147: LGTM!The BLE driver implementation is well-structured:
- Queue is correctly created per connection (line 110), avoiding stale data issues
- Service and characteristic validation properly raises errors for missing UUIDs
- Error handling appropriately catches and logs
BleakError- Stream lifecycle management follows proper async context manager patterns
packages/jumpstarter-driver-ble/jumpstarter_driver_ble/client.py (1)
1-65: LGTM!The client implementation correctly:
- Uses
PexpectAdapterfor pexpect session management- Provides both programmatic (
open(),pexpect()) and CLI (cli()) interfaces- Properly integrates with the
BleConsolefor interactive sessionspackages/jumpstarter-driver-ble/jumpstarter_driver_ble/console.py (1)
1-64: LGTM!The console implementation follows the established pattern from the serial console driver:
- Terminal raw mode handling with proper restoration
- Bidirectional I/O bridging using
FileReadStreamandFileWriteStream- Triple Ctrl-B exit mechanism
- Clean exception handling with
BleConsoleExit
| from jumpstarter.client import DriverClient | ||
|
|
||
|
|
||
| class BleConsoleExit(Exception): |
There was a problem hiding this comment.
don't worry, always take the comments from coderabbitai with a grain of salt, it is not perfect. If it's working for you keep it as it is, we can refactor code later.
| notify_queue: asyncio.Queue | ||
|
|
||
| async def send(self, data: bytes): | ||
| await self.client.write_gatt_char(self.config.write_char_uuid, data) |
There was a problem hiding this comment.
just a question for later improvement,
Is there a limit in the number of bytes you can send via gatt?
May be above MTU may need chunking, or just refusing to send it (I guess we would get that via an exception anyway) :)
There was a problem hiding this comment.
bleak does the chunking for us.
| $ pip3 install --extra-index-url {{index_url}} jumpstarter-driver-ble | ||
| ``` | ||
|
|
||
| ## Configuration |
There was a problem hiding this comment.
|
Only some tests are missing.. we can add that later, but let's not forget :) Is there any standard-ish ble device I can test this with? |
True, I will check bleak if there is a way to emulate a ble device. |
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
Tests