esp32: introduce ESP32 driver #613
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new jumpstarter-driver-esp32 package providing an ESP32 driver and client with flashing, erase, reset, bootloader, and read operations; CLI and examples; documentation pages and README; package metadata and workspace integration; and an exporter YAML example. No existing public signatures were modified. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as ESP32Client (CLI)
participant Client as ESP32Client
participant Driver as ESP32 Driver
participant Storage as Opendal / File Stream
participant Tool as esptool
CLI->>Client: flash <firmware> --address
Client->>Storage: open via OpendalAdapter (operator, path)
Storage-->>Client: stream handle / data
Client->>Driver: flash_firmware(operator, path, address)
activate Driver
Driver->>Storage: stream data to temp file (async write)
Storage-->>Driver: temp file ready
Driver->>Tool: connect(port, baudrate)
activate Tool
Driver->>Tool: flash_firmware(temp_file, address)
Tool-->>Driver: flash complete
Driver->>Tool: hard_reset()
Tool-->>Driver: reset done
deactivate Tool
Driver->>Driver: close port
Driver-->>Client: status message
deactivate Driver
Client-->>CLI: print result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 11
🧹 Nitpick comments (14)
pyproject.toml (1)
15-15: Workspace source entry looks good. Consider source list ordering for churn minimization.No issues with adding the ESP32 package. To reduce future diff noise, consider keeping
[tool.uv.sources]keys alphabetized.packages/jumpstarter-driver-esp32/pyproject.toml (1)
25-30: pytest config: consider surfacing failures early.Optional: add
-rA -qor--maxfail=1for faster feedback locally.packages/jumpstarter-driver-esp32/examples/exporter.yaml (1)
13-15: Clarify hardware pin support status.Docs imply hardware reset/boot are available, but driver methods are TODOs. Add a short comment noting current no-op status to avoid confusion.
Apply this diff:
- # Optional GPIO pins for hardware control + # Optional GPIO pins for hardware control (currently no-op/TODO in driver)packages/jumpstarter-driver-esp32/README.md (2)
57-75: Fix markdown lint: add language to fenced block.Add a language identifier to satisfy MD040 and improve rendering.
Apply this diff:
-``` +```text ... -``` +```
40-44: Set expectations on hardware control.Since
_hardware_reset/_hardware_bootloaderare TODOs, add a brief note that GPIO-based control is not implemented yet.packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (5)
29-33: Remove shadow import.
import osis already at module top. Avoid re-importing inside__post_init__.Apply this diff:
if self.check_present and not self.port.startswith(("/dev/null", "loop://")): - import os if not os.path.exists(self.port): self.logger.warning(f"Serial port {self.port} does not exist")
144-148: Avoid KeyError and improve message clarity.Use
.get()and prefer chip description/type for the message.Apply this diff:
- return f"Entered bootloader mode successfully. Connected to {info['chip_revision']}" + return ( + "Entered bootloader mode successfully. Connected to " + f"{info.get('chip_revision') or info.get('chip_type')}" + )
181-191: Hardware control methods are stubs.Consider returning
NotImplementedErroruntil GPIO control is implemented, or clearly document as no-op. Also, decide on a GPIO backend (gpiod vs. RPi vs. sysfs) and add it as an optional extra.I can wire a minimal gpiod-based implementation behind an optional dependency (e.g.,
jumpstarter-driver-esp32[gpio]).Also applies to: 192-201
43-54: Avoid non‑portable efuse access; normalize fieldsFile: packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py Lines: 43-54
Replace esp.read_efuse(0) with a safe lookup and make chip_id optional (only format as hex when int); use esp.get_chip_description() directly for chip_revision.
- chip_id = esp.read_efuse(0) + # Avoid non-portable efuse access across ESP variants + chip_id = getattr(esp, "chip_id", None) mac = esp.read_mac() mac_str = ":".join([f"{b:02x}" for b in mac]) return { "chip_type": self.chip, "port": self.port, "baudrate": self.baudrate, "mac_address": mac_str, - "chip_id": f"0x{chip_id:08x}", - "chip_revision": f"Chip is {esp.get_chip_description()}" + "chip_id": f"0x{chip_id:08x}" if isinstance(chip_id, int) else None, + # Keep key name for API stability; value is the description + "chip_revision": esp.get_chip_description(), }
96-106: Pass filename (path) to esptool.cmds.write_flash instead of an open file handle.esptool v5+ examples use path tuples and the v5 public API targets filename/bytes/file-like inputs — passing the filename string is more portable across esptool versions (repo depends on esptool>=5.0.2). (developer.espressif.com)
- with open(filename, 'rb') as f: - esptool.cmds.write_flash( - esp, - [(address, f)], - flash_freq="keep", - flash_mode="keep", - flash_size="detect", - compress=True, - no_progress=False, - erase_all=False - ) + esptool.cmds.write_flash( + esp, + [(address, filename)], + flash_freq="keep", + flash_mode="keep", + flash_size="detect", + compress=True, + no_progress=False, + erase_all=False, + )
- Add a small test that mocks esptool.cmds.write_flash and verifies it's called with the filename string.
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (4)
82-82: Prefer click.echo over print for consistent CLI behavior.Switching to
click.echohandles encoding, piping, and testing better.Apply:
- print(f"{key}: {value}") + click.echo(f"{key}: {value}") @@ - print(f"Chip Type: {info.get('chip_type', 'Unknown')}") + click.echo(f"Chip Type: {info.get('chip_type', 'Unknown')}") + if 'chip_id' in info: + click.echo(f"Chip ID: {info['chip_id']}") if 'mac_address' in info: - print(f"MAC Address: {info['mac_address']}") + click.echo(f"MAC Address: {info['mac_address']}") if 'chip_revision' in info: - print(f"Chip Revision: {info['chip_revision']}") + click.echo(f"Chip Revision: {info['chip_revision']}") @@ - result = self.reset() - print(result) + result = self.reset() + click.echo(result) @@ - print("Erasing flash... this may take a while") - result = self.erase_flash() - print(result) + click.echo("Erasing flash... this may take a while") + result = self.erase_flash() + click.echo(result) @@ - print(f"Reading {size} bytes from address 0x{address:x}...") + click.echo(f"Reading {size} bytes from address 0x{address:x}...") @@ - with open(output, 'wb') as f: - f.write(data) - print(f"Data written to {output}") + with open(output, 'wb') as f: + f.write(data) + click.echo(f"Data written to {output}") @@ - print(f"0x{addr_offset:08x}: {line}") + click.echo(f"0x{addr_offset:08x}: {line}") @@ - result = self.enter_bootloader() - print(result) + result = self.enter_bootloader() + click.echo(result)Also applies to: 88-93, 97-98, 103-105, 121-124, 149-156, 160-163, 167-168
84-93: chip-id command omits the actual chip_id field.The command name suggests showing the numeric ID. Include
chip_id.Included in the echo diff above:
+ if 'chip_id' in info: + click.echo(f"Chip ID: {info['chip_id']}")
29-30: Guard logger usage on client.
DriverClientmay not expose.logger. AvoidAttributeError.Apply:
- self.logger.info("Erasing flash... this may take a while") + if getattr(self, "logger", None): + self.logger.info("Erasing flash... this may take a while")
53-65: LGTM with sane validation.Non-negative address and positive size checks are appropriate. Consider an upper bound (e.g., against detected flash size) later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-esp32/README.md(1 hunks)packages/jumpstarter-driver-esp32/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py(1 hunks)packages/jumpstarter-driver-esp32/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (2)
Driver(56-269)resource(225-252)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (7)
chip_info(19-21)chip_id(85-92)erase_flash(27-30)info(78-82)flash_firmware(32-44)read_flash(53-65)enter_bootloader(67-69)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (4)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
OpendalAdapter(60-86)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (6)
client(35-36)chip_info(39-57)erase_flash(68-77)flash_firmware(80-123)read_flash(126-136)enter_bootloader(139-148)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-99)call(36-46)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ArgumentError(54-57)
🪛 markdownlint-cli2 (0.17.2)
packages/jumpstarter-driver-esp32/README.md
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py
[failure] 176-176: Ruff (B904)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py:176:13: B904 Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
[failure] 167-167: Ruff (B904)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py:167:13: B904 Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
[failure] 120-120: Ruff (E722)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py:120:21: E722 Do not use bare except
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
[failure] 71-71: Ruff (C901)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py:71:9: C901 cli is too complex (20 > 10)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
[error] 71-71: Ruff: C901 'cli' is too complex (20 > 10).
⏰ 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: e2e
- 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: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
🔇 Additional comments (6)
packages/jumpstarter-driver-esp32/pyproject.toml (2)
21-24: Verify source_archive URL template.
...github.com/jumpstarter-dev/repo/...looks placeholder-ish. Confirm the correct repo slug (likelyjumpstarter).Do you want me to open a follow-up to align this with the actual repository name?
31-34: Build backend/tooling alignment check.You require
hatch-pin-jumpstarter. Confirm this hook is present in the workspace (root listshatch-pin-jumpstarter). LGTM otherwise.packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (1)
150-159: esptool API usage—double-check kwargs across versions.
esptool.cmds.detect_chip(..., connect_mode='default_reset', connect_attempts=7, trace_enabled=False)may vary by version. Verify these kwargs exist for your pinned range (esptool>=5.0.2).If needed, we can branch on
hasattr(esptool.cmds, "detect_chip")and fall back toesptool.ESPLoader.detect_chip(...).packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (3)
19-21: LGTM: thin wrapper over driver call.The
chip_infoclient method is straightforward and matches the driver API.
23-26: LGTM: reset proxy.Matches the exported driver method name.
43-45: Incorrect — no change needed: OpendalAdapter is already wrapped for sync use.OpendalAdapter is decorated with @Blocking and @asynccontextmanager (packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py), so using
with OpendalAdapter(...)is valid; DriverClient.call_async exists at packages/jumpstarter/jumpstarter/client/core.py:79.Likely an incorrect or invalid review comment.
| absolute = Path(filepath).resolve() | ||
| if not absolute.exists(): | ||
| raise ArgumentError(f"File not found: {filepath}") | ||
| return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden local file handling and FS operator pathing.
- Accepts directories; should enforce regular file.
- Using root="/" with an absolute path can be brittle across backends. Prefer root=parent, path=name.
Apply:
- absolute = Path(filepath).resolve()
- if not absolute.exists():
- raise ArgumentError(f"File not found: {filepath}")
- return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address)
+ absolute = Path(filepath).resolve()
+ if not absolute.exists() or not absolute.is_file():
+ raise ArgumentError(f"File not found or not a file: {filepath}")
+ op = Operator("fs", root=str(absolute.parent))
+ return self.flash_firmware(operator=op, path=absolute.name, address=address)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| absolute = Path(filepath).resolve() | |
| if not absolute.exists(): | |
| raise ArgumentError(f"File not found: {filepath}") | |
| return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) | |
| absolute = Path(filepath).resolve() | |
| if not absolute.exists() or not absolute.is_file(): | |
| raise ArgumentError(f"File not found or not a file: {filepath}") | |
| op = Operator("fs", root=str(absolute.parent)) | |
| return self.flash_firmware(operator=op, path=absolute.name, address=address) |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py around
lines 48 to 51, the code currently accepts directories and passes an absolute
path with Operator(root="/"), which is brittle; change it to verify the resolved
path is a regular file (not a directory or symlink to a directory) and raise
ArgumentError if not, then build the FS operator using the file's parent
directory as root and pass only the file name (or relative path) as the path
argument to flash_firmware so backends get a portable operator/root + relative
path pairing.
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
Outdated
Show resolved
Hide resolved
| async def flash_firmware(self, src: str, address: int = 0x1000) -> str: | ||
| """Flash firmware to the ESP32""" | ||
| address = int(address) | ||
|
|
There was a problem hiding this comment.
Fix default flash address and validate input.
Default should be 0x10000 (matches README/client and common ESP32 app offset). Add non-negative check.
Apply this diff:
- async def flash_firmware(self, src: str, address: int = 0x1000) -> str:
+ async def flash_firmware(self, src: str, address: int = 0x10000) -> str:
"""Flash firmware to the ESP32"""
address = int(address)
+ if address < 0:
+ raise ValueError("Flash address must be non-negative")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def flash_firmware(self, src: str, address: int = 0x1000) -> str: | |
| """Flash firmware to the ESP32""" | |
| address = int(address) | |
| async def flash_firmware(self, src: str, address: int = 0x10000) -> str: | |
| """Flash firmware to the ESP32""" | |
| address = int(address) | |
| if address < 0: | |
| raise ValueError("Flash address must be non-negative") |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py around
lines 80 to 83, the flash_firmware default address is incorrect and lacks
validation: change the function signature default from 0x1000 to 0x10000, ensure
address is converted to int, and add a non-negative check that raises a
ValueError (with a clear message) if the provided address is negative before
proceeding.
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py
Outdated
Show resolved
Hide resolved
| **Flash firmware:** | ||
| ```{testcode} | ||
| esp32client.flash_firmware("/path/to/firmware.bin", address=0x10000) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Default flash address inconsistent with driver.
README (0x10000) conflicts with driver default (0x1000). Align to 0x10000 to match typical ESP32 app offset and your client docs.
I’ve proposed a driver fix below; once merged, README will be consistent.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-esp32/README.md around lines 79 to 83, the
example flash address must match the driver's default; update the README example
so the address used is 0x10000 (the standard ESP32 app offset) and ensure any
surrounding text or examples refer to 0x10000 as the default; after the driver
fix is merged, verify the README matches the driver defaults and adjust any
other docs or examples that reference the old 0x1000 value.
bennyz
force-pushed
the
esp32-driver
branch
from
1cb401a to
23b19f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (5)
149-195: Ruff C901: cli is too complex — quick unblock and longer-term extractionAdd noqa to unblock. Prefer extracting subcommands to module-level fns to actually reduce complexity.
Apply quick fix:
- def cli(self): + def cli(self): # noqa: C901Longer-term:
- Move each command callback to module-level def cmd_xxx(client: ESP32Client, ...).
- In cli(), set ctx.obj = self and register commands.
- Reuse _parse_int to eliminate branching in callbacks.
32-39: Unify default flash address with driver (client 0x10000 vs driver 0x1000)Client and README default to 0x10000 for app partition; driver.flash_firmware currently defaults to 0x1000. This mismatch risks flashing the wrong partition.
Apply in driver.py:
--- a/packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py +++ b/packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py @@ -async def flash_firmware(self, src: str, address: int = 0x1000) -> str: +async def flash_firmware(self, src: str, address: int = 0x10000) -> str: """Flash firmware to the ESP32"""
46-51: Harden local file handling and FS operator pathingReject directories and use the file’s parent as the FS root to avoid brittle absolute-path usage across backends.
Apply:
def flash_firmware_file(self, filepath: str, address: int = 0x10000) -> str: """Flash a local firmware file to the ESP32""" absolute = Path(filepath).resolve() - if not absolute.exists(): - raise ArgumentError(f"File not found: {filepath}") - return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) + if not absolute.exists() or not absolute.is_file(): + raise ArgumentError(f"File not found or not a file: {filepath}") + op = Operator("fs", root=str(absolute.parent)) + return self.flash_firmware(operator=op, path=absolute.name, address=address)
97-116: Fail fast on invalid integers; avoid int(float(...)) and silent defaultsCurrent parsing accepts “1e5” and silently falls back (dangerous for flash ops). Parse with base-0 ints and raise Click errors.
Apply:
def _parse_address(self, address): """Parse address string to integer""" - try: - if isinstance(address, str) and address.startswith("0x"): - return int(address, 16) - else: - return int(float(address)) - except (ValueError, TypeError): - return 0x10000 # Default fallback + return _parse_int(address, param_hint="--address") @@ def _parse_size(self, size): """Parse size string to integer""" - try: - if size.startswith("0x"): - return int(size, 16) - else: - return int(float(size)) - except (ValueError, TypeError): - return 1024 # Default fallback + return _parse_int(size, param_hint="SIZE")Add helper near imports:
def _parse_int(value: str | int, *, param_hint: str) -> int: try: if isinstance(value, str): return int(value, 0) # auto-detect 0x/0o/0b/decimal return int(value) except Exception: raise click.BadParameter(f"invalid integer: {value}", param_hint=param_hint)
124-131: read-flash: fail fast on invalid/negative sizeSurface errors early and avoid defaulting silently.
Apply:
def _read_flash_command(self, address, size, output): """Read flash contents""" address = self._parse_address(address) - size = self._parse_size(size) + size = self._parse_size(size) + if size <= 0: + raise click.BadParameter("size must be positive", param_hint="SIZE") - print(f"Reading {size} bytes from address 0x{address:x}...") + click.echo(f"Reading {size} bytes from address 0x{address:x}...")
🧹 Nitpick comments (6)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (6)
71-96: Prefer click.echo over print for CLI outputUse click.echo for consistent CLI I/O, proper encoding, and testability.
Apply:
- for key, value in chip_info.items(): - print(f"{key}: {value}") + for key, value in chip_info.items(): + click.echo(f"{key}: {value}") @@ - print(f"Chip Type: {info.get('chip_type', 'Unknown')}") + click.echo(f"Chip Type: {info.get('chip_type', 'Unknown')}") @@ - print(f"MAC Address: {info['mac_address']}") + click.echo(f"MAC Address: {info['mac_address']}") @@ - print(f"Chip Revision: {info['chip_revision']}") + click.echo(f"Chip Revision: {info['chip_revision']}") @@ - result = self.reset() - print(result) + result = self.reset() + click.echo(result) @@ - print("Erasing flash...") + click.echo("Erasing flash...") result = self.erase_flash() - print(result) + click.echo(result)
77-85: chip-id command: include chip_id in outputThe command omits chip_id despite being available in chip_info.
Apply:
info = self.chip_info() - click.echo(f"Chip Type: {info.get('chip_type', 'Unknown')}") + click.echo(f"Chip Type: {info.get('chip_type', 'Unknown')}") + if "chip_id" in info: + click.echo(f"Chip ID: {info['chip_id']}")
117-123: flash: echo status and rely on strict parsingUse click.echo and rely on the stricter parser to avoid silent defaults.
Apply:
def _flash_command(self, firmware_file, address): """Flash firmware to the device""" address = self._parse_address(address) - print(f"Flashing {firmware_file} to address 0x{address:x}...") + click.echo(f"Flashing {firmware_file} to address 0x{address:x}...") result = self.flash_firmware_file(firmware_file, address) - print(result) + click.echo(result)
132-143: Prefer click.echo and minor hexdump polishUse click.echo; logic for address offset is fine.
Apply:
if output: with open(output, "wb") as f: f.write(data) - print(f"Data written to {output}") + click.echo(f"Data written to {output}") else: # Print as hex hex_data = data.hex() for i in range(0, len(hex_data), 32): addr_offset = address + i // 2 line = hex_data[i : i + 32] - print(f"0x{addr_offset:08x}: {line}") + click.echo(f"0x{addr_offset:08x}: {line}")
144-148: Use click.echo for consistencyAligns with other CLI outputs.
Apply:
def _bootloader_command(self): """Enter bootloader mode""" result = self.enter_bootloader() - print(result) + click.echo(result)
175-178: CLI arg: disallow directories for firmware_fileTighten Click param to fail on directories at parse time.
Apply:
- @click.argument("firmware_file", type=click.Path(exists=True)) + @click.argument("firmware_file", type=click.Path(exists=True, dir_okay=False))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/jumpstarter-driver-esp32/README.md(1 hunks)packages/jumpstarter-driver-esp32/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py(1 hunks)packages/jumpstarter-driver-esp32/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-driver-esp32/pyproject.toml
- pyproject.toml
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py
- packages/jumpstarter-driver-esp32/README.md
- packages/jumpstarter-driver-esp32/examples/exporter.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (4)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
OpendalAdapter(60-86)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (6)
client(36-37)chip_info(40-58)erase_flash(69-78)flash_firmware(81-124)read_flash(127-141)enter_bootloader(144-153)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-99)call(36-46)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ArgumentError(54-57)
⏰ 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: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (3)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (3)
19-22: chip_info: straightforward pass-through looks goodDirect delegation to driver via self.call("chip_info") is clear and consistent with the base client pattern.
53-66: read_flash: sane argument validation and delegationNon-negative address and positive size checks here are appropriate; remaining logic correctly defers to the driver.
43-45: Confirm OpendalAdapter can be used with a synchronous “with”If OpendalAdapter is an async context manager (@asynccontextmanager), using it with “with” (not “async with”) will break at runtime. Ensure there’s a sync wrapper or adapt to a blocking wrapper.
Run to verify:
#!/bin/bash set -euo pipefail rg -nC2 'def OpendalAdapter' packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py || true rg -n '@asynccontextmanager' packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py || true rg -nC2 'resource_async|resource\(' packages/jumpstarter/jumpstarter/client || trueIf OpendalAdapter is async-only, either:
- expose a blocking wrapper (e.g., client.portal.run_cm(OpendalAdapter(...))) or
- provide a sync variant that uses client.resource(...) internally.
|
very cool!, make the jumpstarte-all depend on this one! :D |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-esp32/README.md (1)
49-51: Align default flash address with driver/docs and fix markdownlint MD014Prior feedback noted 0x10000 as the default app offset; this example still shows 0x1000. Also remove leading “$ ” (MD014).
-```shell -$ j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x1000 +```console +j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x10000</blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>packages/jumpstarter-driver-esp32/README.md (2)</summary><blockquote> `24-29`: **YAML indentation consistency for the serial exporter** Tighten indentation to match the esp32 block; current mix may confuse readers. ```diff serial: - type: jumpstarter_driver_pyserial.driver.PySerial - config: - url: "/dev/ttyUSB0" - baudrate: 115200 + type: jumpstarter_driver_pyserial.driver.PySerial + config: + url: "/dev/ttyUSB0" + baudrate: 115200
31-39: Document check_present to match APIDocs page lists check_present; README table omits it. Add for parity.
| reset_pin | GPIO pin number for hardware reset (if connected) | int | no | null | | boot_pin | GPIO pin number for boot mode control (if connected) | int | no | null | +| check_present| Check if the serial port exists during exporter initialization | bool | no | true |docs/source/reference/package-apis/drivers/esp32.md (1)
51-68: Add fence language to CLI help block (MD040)Specify a language to satisfy markdownlint and improve rendering.
-``` +```text jumpstarter ⚡ local ➤ j esp32 Usage: j esp32 [OPTIONS] COMMAND [ARGS]... ESP32 client @@ reset Reset the device</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 23b19f55b55283ef4d441320a4b295ccd81f517a and 32a8ffbcc0c03dc64a3f65457b7ae77b906204b5. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `docs/source/reference/package-apis/drivers/esp32.md` (1 hunks) * `docs/source/reference/package-apis/drivers/index.md` (2 hunks) * `packages/jumpstarter-all/pyproject.toml` (1 hunks) * `packages/jumpstarter-driver-esp32/README.md` (1 hunks) * `packages/jumpstarter-driver-esp32/examples/example.py` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * docs/source/reference/package-apis/drivers/index.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>packages/jumpstarter-driver-esp32/examples/example.py (1)</summary><blockquote> <details> <summary>packages/jumpstarter/jumpstarter/utils/env.py (1)</summary> * `env` (41-50) </details> </blockquote></details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>docs/source/reference/package-apis/drivers/esp32.md</summary> 51-51: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>packages/jumpstarter-driver-esp32/README.md</summary> 50-50: Dollar signs used before commands without showing output (MD014, commands-show-output) </details> </details> </details> <details> <summary>⏰ 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)</summary> * 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.13) * GitHub Check: e2e * GitHub Check: pytest-matrix (macos-15, 3.12) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.12) * GitHub Check: pytest-matrix (macos-15, 3.11) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.11) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.13) </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>packages/jumpstarter-driver-esp32/examples/example.py (1)</summary><blockquote> `2-2`: **Verify import path for env()** The function lives at jumpstarter.utils.env in this repo. If there’s no re-export from jumpstarter.common.utils, switch to the canonical import. ```diff -from jumpstarter.common.utils import env +from jumpstarter.utils.env import envIf both exist, prefer the latter for directness.
docs/source/reference/package-apis/drivers/esp32.md (1)
42-45: Sphinx autodoc target should not include parenthesesThe object path must be dotted import, no “()”. This can break the docs build.
-```{eval-rst} -.. autoclass:: jumpstarter_driver_esp32.client.ESP32Client() - :members: chip_info, reset, erase_flash, flash_firmware, flash_firmware_file, read_flash, enter_bootloader -``` +```{eval-rst} +.. autoclass:: jumpstarter_driver_esp32.client.ESP32Client + :members: chip_info, reset, erase_flash, flash_firmware, flash_firmware_file, read_flash, enter_bootloader +```⛔ Skipped due to learnings
Learnt from: mangelajo PR: jumpstarter-dev/jumpstarter#526 File: packages/jumpstarter-driver-power/README.md:27-30 Timestamp: 2025-06-18T13:07:24.108Z Learning: In the jumpstarter project, the Sphinx autoclass directive requires parentheses after the class name (e.g., `.. autoclass:: jumpstarter_driver_power.client.PowerClient()`) to work properly in their documentation setup.
| ```{code-block} console | ||
| # Get device information | ||
| $ j esp32 info | ||
| # Flash firmware to default app partition (0x10000) | ||
| $ j esp32 flash firmware.bin | ||
| # Flash firmware to specific address | ||
| $ j esp32 flash firmware.bin --address 0x1000 | ||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check the default flash address matches the driver constant
This page states 0x10000 as the default (Line 125) and shows an explicit 0x1000 example (Line 129). Ensure the driver’s default matches what we document as “default”.
🏁 Script executed:
#!/bin/bash
# Verify any hardcoded/default flash address in code and docs
rg -nI -C2 -S -e '0x1000\b|0x10000\b|default.*address' \
packages/jumpstarter-driver-esp32 jumpstarter docs packages | sed -n '1,200p'Length of output: 5104
Align default flash address between docs and driver (0x1000 vs 0x10000)
Docs declare default 0x10000 (docs/source/reference/package-apis/drivers/esp32.md, ~lines 123–130) but the codebase contains conflicting defaults: an async implementation uses "async def flash_firmware(self, src: str, address: int = 0x1000)" while other functions/CLI/fallbacks use 0x10000 (e.g., "def flash_firmware(..., address: int = 0x10000)", "flash_firmware_file(..., address: int = 0x10000)", CLI --address default "0x10000", _parse_size fallback 0x10000). Decide the correct default, update the discrepant implementation or the docs to match, and centralize the value into a single constant used by code, CLI, and docs.
🤖 Prompt for AI Agents
In docs/source/reference/package-apis/drivers/esp32.md around lines 121–130 the
README shows a default flash address of 0x10000 but the codebase contains mixed
defaults (some functions use 0x1000 and others 0x10000). Pick the correct
canonical default (use 0x10000 to match CLI and most implementations unless
product/team decides otherwise), update the async flash_firmware signature to
use that value (or update the docs if you choose 0x1000), and centralize the
default by adding a single constant (e.g., ESP32_DEFAULT_FLASH_ADDRESS) in the
driver module used by all flash_firmware variants, flash_firmware_file, CLI
default parsing, and _parse_size fallback; then update docs to reference that
constant/default so code and docs stay consistent.
32a8ffb to
2f595da
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/jumpstarter-driver-esp32/README.md (1)
49-51: Align flash address example with the driver’s default (0x10000).The CLI example uses 0x1000, which conflicts with the documented/default 0x10000 elsewhere. Update the example to avoid user confusion.
-```shell -$ j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x1000 -``` +```shell +$ j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x10000 +```docs/source/reference/package-apis/drivers/esp32.md (1)
121-130: Ensure the documented default flash address matches the driver constant (0x10000).Docs show 0x10000 as default and a 0x1000 explicit example; confirm the driver’s default and CLI default flag value are 0x10000 to avoid drift.
🧹 Nitpick comments (5)
packages/jumpstarter-driver-esp32/README.md (3)
24-29: YAML indentation/style consistency for the serial exporter block.Indent children of
serial:by 2 spaces (likeesp32:) for consistency/readability.serial: - type: jumpstarter_driver_pyserial.driver.PySerial - config: - url: "/dev/ttyUSB0" - baudrate: 115200 + type: jumpstarter_driver_pyserial.driver.PySerial + config: + url: "/dev/ttyUSB0" + baudrate: 115200
7-10: Fix markdownlint MD014 by removing shell prompts from command-only blocks.These blocks don’t show output; drop the leading
$to satisfy MD014.```{code-block} console :substitutions: -$ pip3 install --extra-index-url {{index_url}} jumpstarter-driver-esp32 +pip3 install --extra-index-url {{index_url}} jumpstarter-driver-esp32--- `31-39`: **Document `check_present` config parameter to match implementation/docs.** Add the missing row so README matches the driver and the reference page. ```diff | boot_pin | GPIO pin number for boot mode control (if connected) | int | no | null | +| check_present | Check if the serial port exists during exporter initialization | bool | no | True |docs/source/reference/package-apis/drivers/esp32.md (2)
42-45: Remove parentheses from autoclass target; Sphinx expects a class path, not a call.This can break autodoc resolution in some setups.
```{eval-rst} -.. autoclass:: jumpstarter_driver_esp32.client.ESP32Client() +.. autoclass:: jumpstarter_driver_esp32.client.ESP32Client :members: chip_info, reset, erase_flash, flash_firmware, flash_firmware_file, read_flash, enter_bootloader--- `51-68`: **Specify language for CLI block to satisfy MD040 and render consistently.** Use MyST’s code-block directive with `console`. ```diff -``` +```{code-block} console jumpstarter ⚡ local ➤ j esp32 Usage: j esp32 [OPTIONS] COMMAND [ARGS]... ESP32 client Options: --help Show this message and exit. Commands: bootloader Enter bootloader mode chip-id Get chip ID information erase Erase the entire flash flash Flash firmware to the device info Get device information read-flash Read flash contents reset Reset the device</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 32a8ffbcc0c03dc64a3f65457b7ae77b906204b5 and 2f595da902e978fb3fd230b78c92026f969a7d55. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `docs/source/reference/package-apis/drivers/esp32.md` (1 hunks) * `docs/source/reference/package-apis/drivers/index.md` (2 hunks) * `packages/jumpstarter-all/pyproject.toml` (1 hunks) * `packages/jumpstarter-driver-esp32/README.md` (1 hunks) * `packages/jumpstarter-driver-esp32/examples/example.py` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * packages/jumpstarter-all/pyproject.toml * docs/source/reference/package-apis/drivers/index.md * packages/jumpstarter-driver-esp32/examples/example.py </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>docs/source/reference/package-apis/drivers/esp32.md</summary> 51-51: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>packages/jumpstarter-driver-esp32/README.md</summary> 50-50: Dollar signs used before commands without showing output (MD014, commands-show-output) </details> </details> </details> <details> <summary>⏰ 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)</summary> * 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.13) * GitHub Check: pytest-matrix (macos-15, 3.12) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.13) * GitHub Check: pytest-matrix (macos-15, 3.11) * GitHub Check: e2e * GitHub Check: pytest-matrix (ubuntu-24.04, 3.12) * GitHub Check: pytest-matrix (ubuntu-24.04, 3.11) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
bennyz
force-pushed
the
esp32-driver
branch
from
2f595da to
153674a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/source/reference/package-apis/drivers/esp32.md (1)
121-129: Align default flash address across docs and examples (use 0x10000).You state default app partition is 0x10000 (Line 125) but show a 0x1000 example (Line 129). Update the example to 0x10000. Also ensure the driver/CLI default is sourced from a single constant.
-# Flash firmware to specific address -$ j esp32 flash firmware.bin --address 0x1000 +# Flash firmware to specific address +$ j esp32 flash firmware.bin --address 0x10000
🧹 Nitpick comments (6)
packages/jumpstarter-driver-esp32/README.md (3)
33-40: Add missing config parameter row: check_present.README’s table omits check_present, which is documented elsewhere. Add it for consistency.
| boot_pin | GPIO pin number for boot mode control (if connected) | int | no | null | +| check_present| Check if the serial port exists during exporter initialization | bool | no | True |
16-29: Clarify YAML example: optional serial exporter is unrelated to ESP32 config.As written, readers may think ESP32 depends on the “serial” export. Either remove it or label it as an independent, optional exporter.
export: esp32: type: jumpstarter_driver_esp32.driver.ESP32 config: port: "/dev/ttyUSB0" baudrate: 115200 chip: "esp32" - serial: + # Optional: independent serial exporter (not required by ESP32 driver) + serial: type: jumpstarter_driver_pyserial.driver.PySerial config: url: "/dev/ttyUSB0" baudrate: 115200
49-51: Satisfy markdownlint MD014 (no leading “$” without output).Use a console fence and drop the prompt character to avoid MD014.
-```shell -$ j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x1000 +```console +j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x10000</blockquote></details> <details> <summary>docs/source/reference/package-apis/drivers/esp32.md (3)</summary><blockquote> `42-45`: **Remove parentheses from autoclass target.** Sphinx autodoc expects the class path without “()”; parentheses can break linking. ```diff -```{eval-rst} -.. autoclass:: jumpstarter_driver_esp32.client.ESP32Client() +```{eval-rst} +.. autoclass:: jumpstarter_driver_esp32.client.ESP32Client :members: chip_info, reset, erase_flash, flash_firmware, flash_firmware_file, read_flash, enter_bootloader--- `51-68`: **Specify language for fenced block to fix MD040.** Add a language to the CLI usage block. ```diff -``` +```console jumpstarter ⚡ local ➤ j esp32 Usage: j esp32 [OPTIONS] COMMAND [ARGS]... ESP32 client ...--- `121-142`: **Optional: avoid leading “$” prompts or include outputs.** To appease MD014 and improve copy-paste, either drop the “$” or show sample outputs. ```diff -$ j esp32 info +j esp32 info ... -# Flash firmware to default app partition (0x10000) -$ j esp32 flash firmware.bin +# Flash firmware to default app partition (0x10000) +j esp32 flash firmware.bin
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/source/reference/package-apis/drivers/esp32.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)packages/jumpstarter-driver-esp32/README.md(1 hunks)packages/jumpstarter-driver-esp32/examples/example.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-esp32/examples/example.py
- docs/source/reference/package-apis/drivers/index.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/source/reference/package-apis/drivers/esp32.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
packages/jumpstarter-driver-esp32/README.md
50-50: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🪛 GitHub Actions: documentation
docs/source/reference/package-apis/drivers/esp32.md
[warning] 74-74: Sphinx: Unknown directive type: 'testcode-skip' [myst.directive_unknown]
[warning] 83-83: Sphinx: Unknown directive type: 'testcode-skip' [myst.directive_unknown]
[warning] 97-97: Sphinx: Unknown directive type: 'testcode-skip' [myst.directive_unknown]
[warning] 105-105: Sphinx: Unknown directive type: 'testcode-skip' [myst.directive_unknown]
⏰ 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: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- 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.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
bennyz
force-pushed
the
esp32-driver
branch
2 times, most recently
from
8a07b34 to
302dd46
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-driver-esp32/README.md (1)
49-51: Fix flash address (use 0x10000) and fence language; remove leading “$”.Using 0x1000 risks clobbering the bootloader; examples should default to the ESP32 app offset 0x10000. Also switch to "console" and drop "$" to satisfy markdownlint.
-```shell -$ j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x1000 -``` +```console +j esp32 flash ESP32_GENERIC-20250911-v1.26.1.bin -a 0x10000 +```To ensure consistency across code/docs, run:
#!/bin/bash # Verify defaults and examples for flash offset across the repo rg -nP -C2 '(0x1000|0x10000)\b' --type=md --type=py
🧹 Nitpick comments (3)
packages/jumpstarter-driver-esp32/README.md (3)
24-29: Normalize YAML indentation under serial for readability.Indent levels are inconsistent vs. the esp32 block. Use 2 spaces per level.
- serial: - type: jumpstarter_driver_pyserial.driver.PySerial - config: - url: "/dev/ttyUSB0" - baudrate: 115200 + serial: + type: jumpstarter_driver_pyserial.driver.PySerial + config: + url: "/dev/ttyUSB0" + baudrate: 115200
41-45: Document the client API alongside the driver.Expose ESP32Client in the README to align with the CLI users will touch.
```{autoclass} jumpstarter_driver_esp32.driver.ESP32 :members:+
{autoclass} jumpstarter_driver_esp32.client.ESP32Client +:members: +--- `47-47`: **Use a secondary heading for “Examples”.** Keep a single H1 per page. ```diff -# Examples +## Examples
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/source/reference/package-apis/drivers/esp32.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)packages/jumpstarter-driver-esp32/README.md(1 hunks)packages/jumpstarter-driver-esp32/examples/example.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/source/reference/package-apis/drivers/index.md
- docs/source/reference/package-apis/drivers/esp32.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-esp32/examples/example.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/jumpstarter-driver-esp32/README.md
51-51: 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). (5)
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
bennyz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (1)
81-84: Default flash address is wrong and missing validation — risk of overwriting bootloader.ESP32 app offset is typically 0x10000. Defaulting to 0x1000 risks flashing over the bootloader. Also add a non‑negative check. This was flagged earlier; still present.
- async def flash_firmware(self, src: str, address: int = 0x1000) -> str: + async def flash_firmware(self, src: str, address: int = 0x10000) -> str: """Flash firmware to the ESP32""" - address = int(address) + address = int(address) + if address < 0: + raise ValueError("Flash address must be non-negative")#!/bin/bash # Verify no other defaults use 0x1000 for flash address rg -nP --hidden --no-ignore '(flash_firmware\(.+address:\s*int\s*=\s*0x[0-9a-fA-F]+\))|(--address[^\\n]*0x[0-9a-fA-F]+)' -g '!**/dist/**' -g '!**/build/**'packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (4)
32-45: Unify default flash address to 0x10000 (matches common app offset and CLI default).Keep programmatic and CLI defaults consistent and safe.
- def flash_firmware(self, operator: Operator, path: str, address: int = 0x1000) -> str: + def flash_firmware(self, operator: Operator, path: str, address: int = 0x10000) -> str: @@ - if address < 0: - raise ArgumentError("Flash address must be non-negative") + if address < 0: + raise ArgumentError("Flash address must be non-negative")
46-52: Harden local file handling and OpenDAL pathing.Reject directories and construct the FS Operator with the parent directory as root; pass the filename as path. Aligns with backend expectations and avoids brittle absolute-path + root="/" pairing.
- def flash_firmware_file(self, filepath: str, address: int = 0x1000) -> str: + def flash_firmware_file(self, filepath: str, address: int = 0x10000) -> str: @@ - absolute = Path(filepath).resolve() - if not absolute.exists(): - raise ArgumentError(f"File not found: {filepath}") - return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) + absolute = Path(filepath).resolve() + if not absolute.exists() or not absolute.is_file(): + raise ArgumentError(f"File not found or not a file: {filepath}") + op = Operator("fs", root=str(absolute.parent)) + return self.flash_firmware(operator=op, path=absolute.name, address=address)
101-110: CLI integer parsing accepts floats/scientific notation and silently falls back — fail fast.int(float(...)) will accept "1e5"; silent fallback to 0x10000 is dangerous. Parse with base autodetection and raise a Click error on invalid input.
- def _parse_address(self, address): - """Parse address string to integer""" - try: - if isinstance(address, str) and address.startswith("0x"): - return int(address, 16) - else: - return int(float(address)) - except (ValueError, TypeError): - return 0x10000 # Default fallback + def _parse_address(self, address): + """Parse address string to integer""" + return _parse_int(address, param_hint="--address")Add helper near the imports (outside this hunk):
def _parse_int(value: str | int, *, param_hint: str) -> int: try: if isinstance(value, str): return int(value, 0) # supports 0x/0o/0b/decimal return int(value) except Exception: raise click.BadParameter(f"invalid integer: {value}", param_hint=param_hint)
111-120: Same for size parsing — reject invalid values instead of defaulting to 1024.- def _parse_size(self, size): - """Parse size string to integer""" - try: - if size.startswith("0x"): - return int(size, 16) - else: - return int(float(size)) - except (ValueError, TypeError): - return 1024 # Default fallback + def _parse_size(self, size): + """Parse size string to integer""" + return _parse_int(size, param_hint="SIZE")
🧹 Nitpick comments (5)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (3)
25-27: Call base post_init directly; the hasattr(super(), ...) guard is unnecessary.Driver defines post_init, so just call it. The current hasattr(super(), "post_init") is unusual and can be dropped.
- def __post_init__(self): - if hasattr(super(), "__post_init__"): - super().__post_init__() + def __post_init__(self): + super().__post_init__()
29-33: Avoid redundant import and limit path existence checks to POSIX serial paths.You already import os at top; the inner import is redundant. Also, COM ports on Windows won't exist as filesystem paths—this would log a spurious warning. Guard by platform and drop loop:// exemption duplication.
- if self.check_present and not self.port.startswith(("/dev/null", "loop://")): - import os - - if not os.path.exists(self.port): + if self.check_present and os.name != "nt" and not self.port.startswith(("loop://",)): + if not os.path.exists(self.port): self.logger.warning(f"Serial port {self.port} does not exist")
153-163: enter_bootloader does not actually enter bootloader without hardware pins.When boot_pin is None, this only calls chip_info and claims success. Either implement a software path (e.g., toggling DTR/RTS or esptool reset-to-bootloader) or adjust messaging to avoid false success.
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (2)
153-199: Ruff C901: cli() likely exceeds complexity threshold — quick unblock with noqa; plan refactor.Short-term: add # noqa: C901 on def line. Longer-term: extract command callbacks to module-level functions to reduce complexity.
- def cli(self): + def cli(self): # noqa: C901
75-100: Prefer click.echo over print for CLI output.Provides better stdout/stderr handling and unicode safety across platforms.
Examples:
- print(f"{key}: {value}") + click.echo(f"{key}: {value}")Apply similarly to other print calls in this section and below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (4)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
OpendalAdapter(60-86)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (6)
client(36-37)chip_info(40-58)erase_flash(69-78)flash_firmware(81-124)read_flash(127-150)enter_bootloader(153-162)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-99)call(36-46)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ArgumentError(54-57)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (2)
Driver(56-269)resource(225-252)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (6)
chip_info(19-21)erase_flash(27-30)info(160-162)flash_firmware(32-44)read_flash(53-69)enter_bootloader(71-73)
⏰ 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: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
bennyz
force-pushed
the
esp32-driver
branch
from
b1bc760 to
e204945
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (3)
153-199: Ruff C901: cli too complex; quick unblock or extract commands.Add
# noqa: C901or split callbacks into functions to reduce complexity.- def cli(self): + def cli(self): # noqa: C901Longer-term: move command callbacks to module-level helpers and register with
base.add_command(...).
46-52: Harden local file path handling and align default address.Enforce regular file, build FS operator from parent dir, and switch default to 0x10000.
- def flash_firmware_file(self, filepath: str, address: int = 0x1000) -> str: + def flash_firmware_file(self, filepath: str, address: int = 0x10000) -> str: """Flash a local firmware file to the ESP32""" absolute = Path(filepath).resolve() - if not absolute.exists(): - raise ArgumentError(f"File not found: {filepath}") - return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) + if not absolute.exists() or not absolute.is_file(): + raise ArgumentError(f"File not found or not a file: {filepath}") + op = Operator("fs", root=str(absolute.parent)) + return self.flash_firmware(operator=op, path=absolute.name, address=address)
101-111: CLI parsing: fail fast and parse ints robustly; set default to 0x10000.Avoid
int(float(...))and silent fallbacks. Use base-0 parsing and raise click.BadParameter. Also update CLI default.- def _parse_address(self, address): - """Parse address string to integer""" - try: - if isinstance(address, str) and address.startswith("0x"): - return int(address, 16) - else: - return int(float(address)) - except (ValueError, TypeError): - return 0x1000 + def _parse_int(self, value, *, param_hint: str) -> int: + """Parse integer from str or int with base auto-detection""" + try: + if isinstance(value, str): + return int(value, 0) + return int(value) + except Exception: + raise click.BadParameter(f"invalid integer: {value}", param_hint=param_hint) @@ - address = self._parse_address(address) + address = self._parse_int(address, param_hint="--address") @@ - @click.option("--address", "-a", default="0x1000", type=str, help="Flash address (hex or decimal)") + @click.option("--address", "-a", default="0x10000", type=str, help="Flash address (hex like 0x10000 or decimal)")And in read-flash:
- address = self._parse_address(address) - size = self._parse_size(size) + address = self._parse_int(address, param_hint="ADDRESS") + size = self._parse_int(size, param_hint="SIZE") + if size <= 0: + raise click.BadParameter("size must be positive", param_hint="SIZE")Also applies to: 121-127, 179-183
docs/source/reference/package-apis/drivers/esp32.md (2)
120-125: Avoid suggesting 0x1000 in CLI examples.To prevent bricking, prefer 0x10000 as explicit address; show “specific address” with a safe alternative (e.g., 0x20000) if needed.
-# Flash firmware to specific address -$ j esp32 flash firmware.bin --address 0x1000 +# Flash firmware to specific address (example: second app slot) +$ j esp32 flash firmware.bin --address 0x20000
84-89: Use safe default flash address (0x10000) in example.Example currently shows 0x1000, which targets the bootloader region on typical ESP32 layouts.
-result = esp32.flash_firmware_file("firmware.bin", address=0x1000) +result = esp32.flash_firmware_file("firmware.bin", address=0x10000)
🧹 Nitpick comments (4)
packages/jumpstarter-driver-esp32/README.md (1)
31-39: Document the check_present option for parity with driver/docs.README table omits check_present (bool, default: True). Add it to avoid config drift.
| chip | The ESP32 chip type (esp32, esp32s2, esp32s3, esp32c3, etc.) | str | no | esp32 | | reset_pin | GPIO pin number for hardware reset (if connected) | int | no | null | | boot_pin | GPIO pin number for boot mode control (if connected) | int | no | null | +| check_present| Check if the serial port exists during exporter initialization | bool | no | True |docs/source/reference/package-apis/drivers/esp32.md (1)
51-68: Add code block language for CLI usage.Specify a language to satisfy MD040 and improve rendering.
-``` +```console jumpstarter ⚡ local ➤ j esp32 Usage: j esp32 [OPTIONS] COMMAND [ARGS]... ...</blockquote></details> <details> <summary>packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (2)</summary><blockquote> `111-120`: **Remove silent size fallback; validate explicitly.** Defaulting to 1024 on parse errors hides user mistakes during flash reads. ```diff - def _parse_size(self, size): - """Parse size string to integer""" - try: - if size.startswith("0x"): - return int(size, 16) - else: - return int(float(size)) - except (ValueError, TypeError): - return 1024 # Default fallback + # handled via _parse_int(...) and explicit >0 check in callers
75-100: Use click.echo for consistent CLI output.Improves testability and respects Click’s output redirection.
- for key, value in chip_info.items(): - print(f"{key}: {value}") + for key, value in chip_info.items(): + click.echo(f"{key}: {value}") @@ - print(result) + click.echo(result) @@ - print("Erasing flash...") + click.echo("Erasing flash...") @@ - print(f"Flashing {firmware_file} to address 0x{address:x}...") + click.echo(f"Flashing {firmware_file} to address 0x{address:x}...") @@ - print(f"Reading {size} bytes from address 0x{address:x}...") + click.echo(f"Reading {size} bytes from address 0x{address:x}...") @@ - print(f"Data written to {output}") + click.echo(f"Data written to {output}") @@ - print(f"0x{addr_offset:08x}: {line}") + click.echo(f"0x{addr_offset:08x}: {line}") @@ - print(result) + click.echo(result)Also applies to: 121-147, 148-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/source/reference/package-apis/drivers/esp32.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)packages/jumpstarter-driver-esp32/README.md(1 hunks)packages/jumpstarter-driver-esp32/examples/example.py(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-esp32/examples/example.py
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (4)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
OpendalAdapter(60-86)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (6)
client(36-37)chip_info(40-58)erase_flash(69-78)flash_firmware(81-124)read_flash(127-150)enter_bootloader(153-162)packages/jumpstarter/jumpstarter/client/base.py (2)
DriverClient(19-99)call(36-46)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
ArgumentError(54-57)
🪛 markdownlint-cli2 (0.17.2)
docs/source/reference/package-apis/drivers/esp32.md
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
packages/jumpstarter-driver-esp32/README.md
50-50: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ 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). (9)
- 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 (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (3)
docs/source/reference/package-apis/drivers/index.md (2)
76-76: LGTM: ESP32 driver entry added to list.Entry reads clearly and matches package name.
90-90: LGTM: toctree includes esp32.md.Ensures the page is built and discoverable.
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (1)
32-32: Incorrect: repository defaults use 0x1000 (client, driver, README).All occurrences use 0x1000 — matches found at:
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py:81 — async def flash_firmware(... address: int = 0x1000)
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py:32 — def flash_firmware(... address: int = 0x1000)
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py:46 — def flash_firmware_file(... address: int = 0x1000)
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py:109 — return 0x1000
- packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py:181 — click.option("--address", default="0x1000")
- packages/jumpstarter-driver-esp32/README.md:50 — example uses
-a 0x1000No change required; if you want to standardize on 0x10000, update the locations above.
Likely an incorrect or invalid review comment.
| def flash_firmware(self, operator: Operator, path: str, address: int = 0x1000) -> str: | ||
| """Flash firmware to the ESP32 | ||
| Args: | ||
| operator: OpenDAL operator for file access | ||
| path: Path to firmware file | ||
| address: Flash address | ||
| """ | ||
| if address < 0: | ||
| raise ArgumentError("Flash address must be non-negative") | ||
|
|
||
| with OpendalAdapter(client=self, operator=operator, path=path) as handle: | ||
| return self.call("flash_firmware", handle, address) | ||
|
|
There was a problem hiding this comment.
Blocking: async context manager misused; this will fail at runtime.
OpendalAdapter is async; using with ... in a sync method will raise. Run the async context inside the portal and call the async RPC.
- def flash_firmware(self, operator: Operator, path: str, address: int = 0x1000) -> str:
+ def flash_firmware(self, operator: Operator, path: str, address: int = 0x10000) -> str:
"""Flash firmware to the ESP32
@@
- if address < 0:
+ if address < 0:
raise ArgumentError("Flash address must be non-negative")
- with OpendalAdapter(client=self, operator=operator, path=path) as handle:
- return self.call("flash_firmware", handle, address)
+ async def _run():
+ async with OpendalAdapter(client=self, operator=operator, path=path) as handle:
+ return await self.call_async("flash_firmware", handle, address)
+ return self.portal.call(_run)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def flash_firmware(self, operator: Operator, path: str, address: int = 0x1000) -> str: | |
| """Flash firmware to the ESP32 | |
| Args: | |
| operator: OpenDAL operator for file access | |
| path: Path to firmware file | |
| address: Flash address | |
| """ | |
| if address < 0: | |
| raise ArgumentError("Flash address must be non-negative") | |
| with OpendalAdapter(client=self, operator=operator, path=path) as handle: | |
| return self.call("flash_firmware", handle, address) | |
| def flash_firmware(self, operator: Operator, path: str, address: int = 0x10000) -> str: | |
| """Flash firmware to the ESP32 | |
| Args: | |
| operator: OpenDAL operator for file access | |
| path: Path to firmware file | |
| address: Flash address | |
| """ | |
| if address < 0: | |
| raise ArgumentError("Flash address must be non-negative") | |
| async def _run(): | |
| async with OpendalAdapter(client=self, operator=operator, path=path) as handle: | |
| return await self.call_async("flash_firmware", handle, address) | |
| return self.portal.call(_run) |
bennyz
changed the title
| --help Show this message and exit. | ||
| Commands: | ||
| bootloader Enter bootloader mode |
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
it looks great, just some comments about fragmentation of interfaces, IMHO at least all the small MCU embedded should follow the same pattern, I am happy to change probe-rs as I am sure it has no users yet but me.
| self.logger.info("Erasing flash... this may take a while") | ||
| return self.call("erase_flash") | ||
|
|
||
| def flash_firmware(self, operator: Operator, path: str, address: int = 0x1000) -> str: |
There was a problem hiding this comment.
let's make it follow an extended https://github.com/jumpstarter-dev/jumpstarter/blob/main/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py#L582 flash interface
I should do the same in probe-rs myself
| raise ArgumentError(f"File not found: {filepath}") | ||
| return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) | ||
|
|
||
| def read_flash(self, address: int, size: int) -> bytes: |
There was a problem hiding this comment.
under flash interface this is "dump"
Perhaps we cannot follow exactly the same interface, but implement on top of this? (as this gives bytes, and of course can be useful)
I should do the same here:
Whatever we end up implementing in this esp32 patch, I will unify on the probe-rs :) perhaps we should document it so all similar drivers aren't fragmented.
|
@bennyz I'd say, let's merge it, and let's consider esp32 and probers interfaces not stable, then we can unify them whenever we have time. WDYT? |
I have made some changes, to the interface as well, I need to fix an issue with the dump method and i'll post it |
bennyz
force-pushed
the
esp32-driver
branch
2 times, most recently
from
c66ea93 to
812cca0
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com>
Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com>
Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com>
Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/jumpstarter-all/pyproject.toml (1)
9-44: Normalize ESP32 driver dependency name (lowercase for consistency/tooling)The dependency here should use the normalized, all‑lowercase package name to match the rest of the list and avoid resolver/pinning quirks.
- "Jumpstarter-driver-esp32", + "jumpstarter-driver-esp32",packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py (3)
46-51: Harden local firmware file handling & FS operator rootThis method currently only checks
.exists()and then mounts an FS operator at root/with an absolute path, which is brittle across backends and allows directories.Consider tightening it:
def flash_firmware_file(self, filepath: str, address: int = 0x10000) -> str: """Flash a local firmware file to the ESP32""" absolute = Path(filepath).resolve() - if not absolute.exists(): - raise ArgumentError(f"File not found: {filepath}") - return self.flash_firmware(operator=Operator("fs", root="/"), path=str(absolute), address=address) + if not absolute.exists() or not absolute.is_file(): + raise ArgumentError(f"File not found or not a file: {filepath}") + + op = Operator("fs", root=str(absolute.parent)) + return self.flash_firmware(operator=op, path=absolute.name, address=address)This enforces that the path is a regular file and uses a portable
(root, relative-path)pair with the FS operator. Based on patterns used in the opendal driver.Confirm recommended patterns for constructing `Operator("fs", ...)` roots and paths in the opendal Python bindings, to ensure this root/relative-path usage is aligned.
32-45: Fix OpendalAdapter usage: don’t use async adapter with a syncwithIf
OpendalAdapteris an async context manager (as in the opendal driver), using it with a plainwithinside a synchronous method will raise at runtime.Adapt
flash_firmwareto run the async context and RPC via the client’s portal, similar to the existing opendal driver’s flash interface:- def flash_firmware(self, operator: Operator, path: str, address: int = 0x10000) -> str: + def flash_firmware(self, operator: Operator, path: str, address: int = 0x10000) -> str: """Flash firmware to the ESP32 @@ - if address < 0: - raise ArgumentError("Flash address must be non-negative") - - with OpendalAdapter(client=self, operator=operator, path=path) as handle: - return self.call("flash_firmware", handle, address) + if address < 0: + raise ArgumentError("Flash address must be non-negative") + + async def _run() -> str: + async with OpendalAdapter(client=self, operator=operator, path=path) as handle: + return await self.call_async("flash_firmware", handle, address) + + # Run the async operation in the client portal + return self.portal.call(_run)Please double‑check this against the actual
DriverClient/OpendalAdapterAPIs so the pattern matches the existing opendal driver. Based on prior discussions about the opendal flash interface.Check how `OpendalAdapter` is defined in `jumpstarter_driver_opendal.adapter` and how it is used in the existing opendal driver’s flash APIs, to confirm it is an async context manager and that `call_async`/`portal.call` are the right integration points.
97-115: Make CLI parsing strict and explicit instead ofint(float(...))with silent fallbacksThe current CLI parsers:
- Accept float/“1e5”‑style values via
int(float(...)).- On any parse error silently fall back to
0x10000(address) or1024(size).For flash/erase operations this is risky: an invalid
--addressor SIZE might quietly perform a dangerous write with unexpected parameters.Refactor to a shared strict parser that raises
click.BadParameteron invalid input:-from typing import Any, Dict +from typing import Any, Dict, Union @@ import click @@ from jumpstarter.common.exceptions import ArgumentError @@ +def _parse_int(value: Union[str, int], *, param_hint: str) -> int: + """Parse an integer from CLI input, with clear errors.""" + try: + if isinstance(value, str): + # base=0 supports 0x..., 0o..., 0b..., and decimal + return int(value, 0) + return int(value) + except Exception: + raise click.BadParameter(f"invalid integer: {value}", param_hint=param_hint) @@ def _parse_address(self, address): """Parse address string to integer""" - try: - if isinstance(address, str) and address.startswith("0x"): - return int(address, 16) - else: - return int(float(address)) - except (ValueError, TypeError): - return 0x10000 # Default fallback + return _parse_int(address, param_hint="--address") @@ def _parse_size(self, size): """Parse size string to integer""" - try: - if size.startswith("0x"): - return int(size, 16) - else: - return int(float(size)) - except (ValueError, TypeError): - return 1024 # Default fallback + value = _parse_int(size, param_hint="SIZE") + if value <= 0: + raise click.BadParameter("size must be positive", param_hint="SIZE") + return valueWith this change, invalid inputs fail fast with a clear CLI error instead of silently falling back to a default that might not match the user’s intent. (CLI defaults for
--addresscan remain unchanged.)Check Click’s latest docs for `BadParameter` and `param_hint` usage to ensure this error pattern is still idiomatic in the current version.Also applies to: 117-127
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py (1)
80-85: Add defensive validation for flash address inflash_firmwareUnlike
read_flash, this method doesn’t validate theaddressafter casting toint. A negative address (or otherwise invalid value) would be passed straight intoesptool.write_flash, which is undesirable for a low‑level flash operation.Consider mirroring the read_flash checks:
@export async def flash_firmware(self, src: str, address: int = 0x1000) -> str: """Flash firmware to the ESP32""" address = int(address) + if address < 0: + raise ValueError(f"Flash address must be non-negative, got {address}")This keeps the server‑side API safe even when called outside the Python client. (Default value choice can follow whatever you decide in the docs/CLI discussion.)
docs/source/reference/package-apis/drivers/esp32.md (1)
72-117: Skip hardware‑dependent doctest blocks to fix failing docs testsThese
{testcode}examples call real firmware/flash operations (e.g.,flash_firmware_file,read_flash,erase_flash) and currently fail in CI (missingfirmware.bin, no ESP32 connected).To keep them as executable examples but avoid requiring hardware in the docs build, mark them as skipped:
-```{testcode} +```{testcode} +:skipif: True info = esp32.chip_info() @@ -```{testcode} +```{testcode} +:skipif: True # Flash firmware from a local file @@ -```{testcode} +```{testcode} +:skipif: True # Read 1024 bytes from address 0x0 @@ -```{testcode} +```{testcode} +:skipif: True # Reset the deviceRepeat
:skipif: Truefor each hardware‑touching{testcode}block in this file. This keeps the examples close to the real API while unblocking the doctest pipeline. (Based on pipeline failures and Sphinx/doctest behavior.)
🧹 Nitpick comments (2)
packages/jumpstarter-driver-esp32/README.md (1)
16-29: Tighten YAML indentation in example for clarityThe
serialblock is currently less indented than itsconfigchildren, which can be a bit confusing when copying the snippet. Consider aligning it with theesp32block:export: esp32: type: jumpstarter_driver_esp32.driver.ESP32 config: port: "/dev/ttyUSB0" baudrate: 115200 chip: "esp32" - serial: - type: jumpstarter_driver_pyserial.driver.PySerial - config: - url: "/dev/ttyUSB0" - baudrate: 115200 + serial: + type: jumpstarter_driver_pyserial.driver.PySerial + config: + url: "/dev/ttyUSB0" + baudrate: 115200docs/source/reference/package-apis/drivers/esp32.md (1)
81-92: Clarify and align default flash address across driver, client, and docsHere the narrative and examples assume a default app partition at
0x10000, while other parts of the PR (README and driver behavior for MicroPython) intentionally use0x1000as the typical MicroPython firmware address. There’s also a mismatch between:
- Driver default:
ESP32.flash_firmware(..., address=0x1000)- CLI default:
j esp32 flash ...→--addressdefault"0x10000"- Docs here: claim default 0x10000, then show an explicit 0x1000 example.
It would be good to either:
- Pick a single canonical default (and document MicroPython as a special case), or
- Explicitly document that the CLI targets the common ESP-IDF
0x10000app partition by default, while MicroPython users should pass-a 0x1000, and ensure code defaults match that story.Right now it’s easy for users to be confused about which address is “the default”, especially when mixing MicroPython and standard ESP32 apps. Based on learnings about MicroPython using 0x1000 by default.
Also applies to: 121-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/source/reference/package-apis/drivers/esp32.md(1 hunks)docs/source/reference/package-apis/drivers/index.md(2 hunks)packages/jumpstarter-all/pyproject.toml(1 hunks)packages/jumpstarter-driver-esp32/README.md(1 hunks)packages/jumpstarter-driver-esp32/examples/example.py(1 hunks)packages/jumpstarter-driver-esp32/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py(1 hunks)packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py(1 hunks)packages/jumpstarter-driver-esp32/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/reference/package-apis/drivers/index.md
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- packages/jumpstarter-driver-esp32/examples/exporter.yaml
- packages/jumpstarter-driver-esp32/pyproject.toml
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-esp32/examples/example.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-all/pyproject.toml
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
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.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-esp32/examples/example.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.pypackages/jumpstarter-all/pyproject.tomldocs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-09-14T10:01:56.723Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 613
File: packages/jumpstarter-driver-esp32/README.md:49-51
Timestamp: 2025-09-14T10:01:56.723Z
Learning: MicroPython firmware for ESP32 is correctly flashed at address 0x1000, not 0x10000. This is different from regular ESP-IDF applications which typically use 0x10000. MicroPython includes its own bootloader and uses 0x1000 as the standard address for firmware deployment on original ESP32 devices.
Applied to files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.pypackages/jumpstarter-driver-esp32/README.md
📚 Learning: 2025-11-05T13:31:39.304Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:100-101
Timestamp: 2025-11-05T13:31:39.304Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the `flash()` method and its CLI command intentionally have different defaults for `fls_version`: the method defaults to `""` (empty string) while the CLI defaults to `"0.1.5"`. This is by design to provide CLI users with a convenient default while keeping the programmatic API ready for when FLS is included in the flasher images.
Applied to files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/driver.pypackages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.pydocs/source/reference/package-apis/drivers/esp32.md
📚 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-driver-esp32/jumpstarter_driver_esp32/driver.pypackages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.pydocs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-11-05T13:33:24.741Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:476-486
Timestamp: 2025-11-05T13:33:24.741Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the FLS flasher download in _flash_with_fls() hardcodes "aarch64-linux" architecture because the flasher always targets ARM devices currently.
Applied to files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.pydocs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-03-14T16:05:14.577Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 339
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:349-349
Timestamp: 2025-03-14T16:05:14.577Z
Learning: The `_upload_artifact` method in `BaseFlasherClient` class should call `write_from_path` with the calculated filename parameter: `storage.write_from_path(filename, path, operator=operator)` instead of `storage.write_from_path(path, operator=operator)`.
Applied to files:
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-all/pyproject.tomlpackages/jumpstarter-driver-esp32/README.md
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Core packages must depend on `jumpstarter-protocol`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py : Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with `*Power`, Network drivers with `*Network`, Flasher drivers with `*Flasher`, Console drivers with `*Console`, Server drivers with `*Server`
Applied to files:
packages/jumpstarter-all/pyproject.toml
📚 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-all/pyproject.toml
📚 Learning: 2025-02-10T20:39:47.893Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter PR: 271
File: docs/source/conf.py:53-53
Timestamp: 2025-02-10T20:39:47.893Z
Learning: In Sphinx configuration, setting `doctest_test_doctest_blocks = ""` specifically disables automatic testing of standard reStructuredText doctest blocks (those using >>> syntax without explicit directives), while still allowing testing of blocks explicitly marked with .. doctest::, .. testcode::, or other doctest directives.
Applied to files:
docs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Each package must have its own README.md documentation
Applied to files:
packages/jumpstarter-driver-esp32/README.md
🧬 Code graph analysis (1)
packages/jumpstarter-driver-esp32/examples/example.py (1)
packages/jumpstarter/jumpstarter/utils/env.py (1)
env(42-51)
🪛 GitHub Actions: Run Tests
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
[error] 1-1: ESP32: Failed to connect to ESP32: fileno
docs/source/reference/package-apis/drivers/esp32.md
[error] 165-165: Doctest failure in ESP32 example: flash_firmware_file("firmware.bin", address=0x10000) -> File not found: firmware.bin
[error] 193-193: Doctest failure in ESP32 example: read_flash(address=0x0, size=1024) -> DriverError: Failed to connect to ESP32: fileno
[error] 209-209: Doctest failure in ESP32 example: reset/enter_bootloader/erase_flash sequence -> DriverError: Failed to connect to ESP32: fileno
🪛 LanguageTool
docs/source/reference/package-apis/drivers/esp32.md
[style] ~36-~36: This phrase is redundant (‘N’ stands for ‘number’). Consider using “pin”.
Context: ... | esp32 | | reset_pin | GPIO pin number for hardware reset (if connected) ...
(PIN_NUMBER)
[style] ~37-~37: This phrase is redundant (‘N’ stands for ‘number’). Consider using “pin”.
Context: ... | null | | boot_pin | GPIO pin number for boot mode control (if connected) ...
(PIN_NUMBER)
packages/jumpstarter-driver-esp32/README.md
[style] ~38-~38: This phrase is redundant (‘N’ stands for ‘number’). Consider using “pin”.
Context: ... | esp32 | | reset_pin | GPIO pin number for hardware reset (if connected) ...
(PIN_NUMBER)
[style] ~39-~39: This phrase is redundant (‘N’ stands for ‘number’). Consider using “pin”.
Context: ... | null | | boot_pin | GPIO pin number for boot mode control (if connected) ...
(PIN_NUMBER)
🔇 Additional comments (1)
packages/jumpstarter-driver-esp32/examples/example.py (1)
1-15: Example script looks correct and aligned with the client APIThe usage of
env()andclient.esp32.chip_info()matches the driver/client design and makes for a clear minimal example.
Signed-off-by: Benny Zlotnik <bzlotnik@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/source/reference/package-apis/drivers/esp32.md (2)
36-37: Simplify redundant "pin number" phrasing in parameter descriptions.The descriptions use "GPIO pin number" which is redundant (the column label "Parameter" already clarifies it's a pin). Simplify to "GPIO pin for hardware reset/boot mode control (if connected)" for clarity.
- | reset_pin | GPIO pin number for hardware reset (if connected) | int | no | null | - | boot_pin | GPIO pin number for boot mode control (if connected) | int | no | null | + | reset_pin | GPIO pin for hardware reset (if connected) | int | no | null | + | boot_pin | GPIO pin for boot mode control (if connected) | int | no | null |
51-68: Specify a language for the fenced code block.The CLI output code block at line 51 lacks a language specifier. Use
```consoleto denote this as console/shell output, which improves syntax highlighting and clarity.-``` +```console jumpstarter ⚡ local ➤ j esp32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/reference/package-apis/drivers/esp32.md(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Core packages must depend on `jumpstarter-protocol`
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.
📚 Learning: 2025-11-05T13:31:39.304Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:100-101
Timestamp: 2025-11-05T13:31:39.304Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the `flash()` method and its CLI command intentionally have different defaults for `fls_version`: the method defaults to `""` (empty string) while the CLI defaults to `"0.1.5"`. This is by design to provide CLI users with a convenient default while keeping the programmatic API ready for when FLS is included in the flasher images.
Applied to files:
docs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-11-05T13:33:24.741Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:476-486
Timestamp: 2025-11-05T13:33:24.741Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the FLS flasher download in _flash_with_fls() hardcodes "aarch64-linux" architecture because the flasher always targets ARM devices currently.
Applied to files:
docs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-02-10T20:39:47.893Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter PR: 271
File: docs/source/conf.py:53-53
Timestamp: 2025-02-10T20:39:47.893Z
Learning: In Sphinx configuration, setting `doctest_test_doctest_blocks = ""` specifically disables automatic testing of standard reStructuredText doctest blocks (those using >>> syntax without explicit directives), while still allowing testing of blocks explicitly marked with .. doctest::, .. testcode::, or other doctest directives.
Applied to files:
docs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
docs/source/reference/package-apis/drivers/esp32.md
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
docs/source/reference/package-apis/drivers/esp32.md
🪛 LanguageTool
docs/source/reference/package-apis/drivers/esp32.md
[style] ~36-~36: This phrase is redundant (‘N’ stands for ‘number’). Consider using “pin”.
Context: ... | esp32 | | reset_pin | GPIO pin number for hardware reset (if connected) ...
(PIN_NUMBER)
[style] ~37-~37: This phrase is redundant (‘N’ stands for ‘number’). Consider using “pin”.
Context: ... | null | | boot_pin | GPIO pin number for boot mode control (if connected) ...
(PIN_NUMBER)
🪛 markdownlint-cli2 (0.18.1)
docs/source/reference/package-apis/drivers/esp32.md
51-51: 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.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (2)
docs/source/reference/package-apis/drivers/esp32.md (2)
1-142: Excellent documentation coverage for the ESP32 driver.The documentation is comprehensive, well-structured, and provides clear examples covering configuration, API, CLI, and usage patterns. Installation instructions, configuration parameters with defaults, and practical examples are all well-presented.
However, please verify one clarification with the team:
Note on PR objectives mention of "dump method": The PR summary mentions an issue with the "dump method" that needs fixing, but the documentation shows
read_flash. Please confirm:
- Is there a separate
dumpmethod in the driver that should be documented?- Or is the PR note referring to
read_flashand should use consistent terminology?This will ensure alignment between the implementation and documentation terminology.
85-85: The mixed flash addresses in the documentation are correct and intentional.The examples showing both
0x10000and0x1000reflect different firmware types: MicroPython firmware uses address0x1000(which includes a bootloader), while ESP-IDF applications use0x10000(without a bootloader). This is not an inconsistency to be resolved with a centralized constant; it is by design. The documentation should clarify which address applies to each firmware type if this distinction is not already evident.Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.