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: driver and client implementations (with Click CLI), README, examples, Sphinx docs, packaging (pyproject), workspace registration, and inclusion in jumpstarter-all; no existing public APIs were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "j esp32 (CLI)"
participant Client as "ESP32Client"
participant Driver as "ESP32 Driver (RPC)"
participant ESP as "ESP32 Device"
User->>CLI: j esp32 flash --address 0x1000 firmware.bin
CLI->>Client: flash_firmware_file(filepath, address)
Client->>Client: validate path & address
Client->>Driver: call("flash_firmware", handle, address)
activate Driver
Driver->>Driver: stream src → TemporaryFilename (async/thread)
Driver->>ESP: esptool write_flash(address, tempFile)
ESP-->>Driver: status
Driver->>ESP: hard reset
deactivate Driver
Driver-->>Client: "Flashed at 0x1000"
Client-->>CLI: result
CLI-->>User: print status
sequenceDiagram
autonumber
actor User
participant CLI as "j esp32 (CLI)"
participant Client as "ESP32Client"
participant Driver as "ESP32 Driver (RPC)"
participant ESP as "ESP32 Device"
User->>CLI: j esp32 info
CLI->>Client: chip_info()
Client->>Driver: call("chip_info")
Driver->>ESP: connect + read efuse/MAC
ESP-->>Driver: chip data
Driver-->>Client: dict(chip_type, mac, revision, ...)
Client-->>CLI: info dict
CLI-->>User: print key=value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests
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 Pre-merge checks✅ Passed checks (3 passed)
|
![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.
packages/jumpstarter-driver-esp32/jumpstarter_driver_esp32/client.py
Outdated
Show resolved
Hide resolved
| 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
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.
Signed-off-by: Benny Zlotnik <[email protected]>
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.
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
bennyz
force-pushed
the
esp32-driver
branch
from
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
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
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 |
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
Examples