introduce pi pico driver#367
Merged
mangelajo merged 1 commit intojumpstarter-dev:mainfrom Mar 23, 2026
Merged
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds a new jumpstarter-driver-pi-pico package with a UF2-flashing PiPico driver, mount-detection helpers, GPIO and 1200‑baud serial BOOTSEL entry flows, CLI client, docs/examples, tests, and workspace/meta-package registration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as PiPicoClient
participant Driver as PiPico Driver
participant BootMount as BootloaderMount
participant GPIO as GPIO Reset
participant Serial as Serial Touch
participant USB as BOOTSEL USB MSC
User->>Client: flash(source.uf2)
Client->>Driver: enter_bootloader()
alt GPIO children present
Driver->>GPIO: toggle BOOTSEL & RUN pins
GPIO-->>Driver: pins toggled
else Serial child present
Driver->>Serial: perform 1200 baud DTR touch
Serial-->>Driver: serial touch complete
end
Driver->>BootMount: find_all_bootloader_mounts()
BootMount-->>Driver: mount paths
alt single mount found
Driver->>USB: copy UF2 -> mount (write + fsync)
USB-->>Driver: write persisted
Driver-->>Client: flash complete
Client->>User: success message
else none or multiple mounts
Driver-->>Client: raise FileNotFoundError
Client->>User: error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py (1)
17-17: Consider adding type annotation forcallsfield.- calls: list = field(default_factory=list) + calls: list[str] = field(default_factory=list)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py` at line 17, Add an explicit type annotation for the dataclass field "calls" (currently declared as calls: list = field(default_factory=list)) so the type checker knows the element types; update it to something like calls: List[Any] or more specific calls: List[Tuple[str, Any]] and import the necessary typing symbols (from typing import List, Any, Tuple) so the field(default_factory=list) declaration remains unchanged but now has a concrete type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py`:
- Around line 20-30: The parser _parse_proc_mounts_mount_points currently only
replaces the space escape \040; update it to decode all octal escapes in the
mount-point field (e.g., \040, \011, \012, \134 and any \NNN) by replacing
occurrences of backslash followed by three octal digits with the corresponding
character. Locate where mnt = parts[1].replace("\\040", " ") and instead run a
small unescape routine (e.g., use a regex r'\\([0-7]{3})' and replace with
chr(int(octal, 8))) to produce the correct Path(mnt) for any standard octal
escape in /proc/mounts.
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.py`:
- Around line 50-60: The CLI nested function flash is recursively calling
self.flash (the same Click command) causing infinite recursion; replace that
call with the parent flasher implementation by invoking the inherited
FlasherClient.flash (e.g., call super().flash or FlasherClient.flash(self, ...))
and pass the same arguments (target=dest_name, compression=compression); keep
the surrounding enter_bootloader and click.echo calls unchanged and ensure
imports remain as-is.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py`:
- Line 17: Add an explicit type annotation for the dataclass field "calls"
(currently declared as calls: list = field(default_factory=list)) so the type
checker knows the element types; update it to something like calls: List[Any] or
more specific calls: List[Tuple[str, Any]] and import the necessary typing
symbols (from typing import List, Any, Tuple) so the field(default_factory=list)
declaration remains unchanged but now has a concrete type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51f2b873-d4b2-4c8b-89ef-826ec067d5ae
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py (1)
79-130: Prefer server/client invocation for more of these behavior tests.Several cases assert behavior via direct
PiPico()calls instead of going throughserve(PiPico()). Where feasible, route these through the RPC path to keep the suite aligned with driver-level end-to-end expectations.As per coding guidelines, Driver package tests should focus on end-to-end testing with server and client, using mocks only when dealing with system dependencies, services, or macOS/Linux compatibility issues.
Also applies to: 159-171, 173-229, 259-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py` around lines 79 - 130, Update the tests to exercise PiPico over the RPC path using serve(PiPico()) and the client API instead of constructing PiPico() directly: replace direct PiPico() calls in test_drivers_pi_pico_multiple_boot_volumes_raises, test_drivers_pi_pico_auto_discover_single, and test_drivers_pi_pico_enter_bootloader_via_serial with with serve(PiPico()) as client and invoke the public client methods (e.g., client.bootloader_info(), client.enter_bootloader() or the RPC-exposed equivalent of _resolve_mount_path) while keeping existing monkeypatches for jumpstarter_driver_pi_pico.driver.find_all_bootloader_mounts and jumpstarter_driver_pi_pico.driver.serial_for_url so system interactions remain mocked; ensure the assertions still expect the same exceptions or returned values when called via the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py`:
- Around line 79-130: Update the tests to exercise PiPico over the RPC path
using serve(PiPico()) and the client API instead of constructing PiPico()
directly: replace direct PiPico() calls in
test_drivers_pi_pico_multiple_boot_volumes_raises,
test_drivers_pi_pico_auto_discover_single, and
test_drivers_pi_pico_enter_bootloader_via_serial with with serve(PiPico()) as
client and invoke the public client methods (e.g., client.bootloader_info(),
client.enter_bootloader() or the RPC-exposed equivalent of _resolve_mount_path)
while keeping existing monkeypatches for
jumpstarter_driver_pi_pico.driver.find_all_bootloader_mounts and
jumpstarter_driver_pi_pico.driver.serial_for_url so system interactions remain
mocked; ensure the assertions still expect the same exceptions or returned
values when called via the client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0e7831f-2ff3-4dcd-bcfa-5741a93e3124
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (8)
- python/docs/source/reference/package-apis/drivers/pi-pico.md
- python/packages/jumpstarter-driver-pi-pico/.gitignore
- python/docs/source/reference/package-apis/drivers/index.md
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yaml
- python/packages/jumpstarter-driver-pi-pico/README.md
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yaml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.py
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- python/packages/jumpstarter-all/pyproject.toml
- python/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py
mangelajo
reviewed
Mar 21, 2026
|
|
||
| 1. **GPIO reset** — wire the Pico's BOOTSEL pad and RUN pin to host GPIO | ||
| lines. | ||
| 2. **1200-baud serial touch** — uses a USB CDC serial child. Only works when |
Member
There was a problem hiding this comment.
ahh cool I was going to ask for this :-D
Member
Author
There was a problem hiding this comment.
so far it only worked with CircuitPython, I couldn't test the GPIO because I'd need to solder the pico's test pad 😅
Member
|
This is so cool :) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py (1)
17-18: Import ordering: relative imports should follow absolute imports.Per PEP 8 and typical linter expectations, absolute imports should precede relative imports.
♻️ Proposed reorder
-from .bootloader_mount import find_all_bootloader_mounts from jumpstarter.driver import Driver, export + +from .bootloader_mount import find_all_bootloader_mounts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py` around lines 17 - 18, Reorder the import statements so the absolute import comes before the relative import: move "from jumpstarter.driver import Driver, export" to appear above "from .bootloader_mount import find_all_bootloader_mounts" to satisfy PEP 8 import ordering and linter expectations.python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.py (1)
52-57: Consider logging the suppressed exception for debugging.The broad
except Exceptionallows flashing to proceed when automatic BOOTSEL entry fails, which is good UX. However, completely swallowing the exception makes debugging difficult when the fallback doesn't work.♻️ Proposed enhancement to log the exception
try: click.echo("Entering BOOTSEL mode...") self.enter_bootloader() - except Exception: + except Exception as e: click.echo("Could not enter BOOTSEL automatically. " "Ensure the Pico is in BOOTSEL mode (hold BOOTSEL while plugging USB).") + click.echo(f"(Reason: {e})", err=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.py` around lines 52 - 57, The except block around self.enter_bootloader() swallows all exceptions, losing useful debug info; update the except Exception handler in the client code that calls enter_bootloader() to log the caught exception details (stack trace or message) in addition to the user-facing click.echo fallback message—use the module/class logger or click.echo with the exception info to record the exception from enter_bootloader() so debugging the fallback path is possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.py`:
- Around line 52-57: The except block around self.enter_bootloader() swallows
all exceptions, losing useful debug info; update the except Exception handler in
the client code that calls enter_bootloader() to log the caught exception
details (stack trace or message) in addition to the user-facing click.echo
fallback message—use the module/class logger or click.echo with the exception
info to record the exception from enter_bootloader() so debugging the fallback
path is possible.
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py`:
- Around line 17-18: Reorder the import statements so the absolute import comes
before the relative import: move "from jumpstarter.driver import Driver, export"
to appear above "from .bootloader_mount import find_all_bootloader_mounts" to
satisfy PEP 8 import ordering and linter expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f69e1d0-f86f-44e0-bb13-afd3b3d93e77
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (10)
- python/packages/jumpstarter-driver-pi-pico/.gitignore
- python/docs/source/reference/package-apis/drivers/index.md
- python/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.py
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yaml
- python/packages/jumpstarter-driver-pi-pico/README.md
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yaml
- python/docs/source/reference/package-apis/drivers/pi-pico.md
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yaml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-all/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/pyproject.toml
d13dc9a to
c2f2bc3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py (2)
35-36: ConsiderPiPicoFlasherfor naming consistency.Per coding guidelines, flasher drivers should use the
*Flashersuffix. Consider renamingPiPicotoPiPicoFlasherfor consistency with other drivers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py` around lines 35 - 36, The class is named PiPico but should follow the flasher naming convention; rename the dataclass class PiPico to PiPicoFlasher and update all references/imports and type usages (e.g., any instantiations, annotations, registrations) to the new symbol; ensure the class still inherits FlasherInterface and Driver and keep the `@dataclass`(kw_only=True) decorator, and update any module exports or factory mappings that referenced PiPico so they use PiPicoFlasher.
174-178: Redundant mount discovery after enter_bootloader.
enter_bootloader()already waits for and validates the mount exists before returning, but line 177 immediately re-queriesfind_all_bootloader_mounts(). Consider havingenter_bootloader()return the discovered mount path to avoid the redundant filesystem scan.♻️ Suggested refactor to avoid redundant mount discovery
`@export` - def enter_bootloader(self): + def enter_bootloader(self) -> Path | None: """Request BOOTSEL mode using the best available method. Priority: already mounted > GPIO reset (bootsel + run children) > 1200-baud serial touch > error. + + Returns the discovered mount path, or None if already mounted. """ - if find_all_bootloader_mounts(): - return + mounts = find_all_bootloader_mounts() + if mounts: + return self._resolve_mounts(mounts) if self._has_gpio_children: self._gpio_bootsel_reset() - self._wait_for_bootloader_mount() - return + return self._wait_for_bootloader_mount() if "serial" in self.children: self.logger.info("Requesting Pico BOOTSEL over serial %s", self._serial.url) self._touch_serial_for_bootloader() - self._wait_for_bootloader_mount() - return + return self._wait_for_bootloader_mount()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py` around lines 174 - 178, The current code redundantly calls find_all_bootloader_mounts() after awaiting enter_bootloader(); change enter_bootloader to return the discovered mount path(s) (or a list) instead of None, then update the caller here to capture that return value and pass it to _resolve_mounts instead of re-querying the filesystem; specifically modify enter_bootloader to perform the wait/validation and return the mount(s), and replace the second find_all_bootloader_mounts() call in this block so mount = self._resolve_mounts(mounts_or_mount_returned_by_enter_bootloader).python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py (1)
51-64: Consider renaming for consistency with return type.The function is named
iter_bootloader_mount_candidatesbut returnslist[Path]rather than an iterator. Consider renaming toget_bootloader_mount_candidatesorlist_bootloader_mount_candidatesto better reflect the return type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py` around lines 51 - 64, The function iter_bootloader_mount_candidates currently returns a concrete list[Path] but its name suggests an iterator; rename iter_bootloader_mount_candidates to get_bootloader_mount_candidates (or list_bootloader_mount_candidates) and update all references/calls to that symbol, plus any import/export entries (e.g., __all__) so the API and tests reflect the new name; keep the implementation and return type unchanged but change the function definition and usages to the new identifier.python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py (1)
1-11: Import block may need reordering.Similar to
driver.py, the imports mix relative and absolute imports. The relative import.driver(line 8) appears before the absolutejumpstarterimports (lines 9-10). Runmake lintto verify andmake lint-fixif needed.🔧 Proposed fix for import ordering
import asyncio from dataclasses import dataclass, field from unittest.mock import MagicMock import pytest from jumpstarter_driver_pyserial.driver import PySerial -from .driver import PiPico from jumpstarter.common.utils import serve from jumpstarter.driver import Driver + +from .driver import PiPico🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py` around lines 1 - 11, Reorder the import block to follow standard grouping: first standard library (asyncio, dataclasses, unittest.mock), then third-party (pytest, jumpstarter_driver_pyserial.driver.PySerial), then absolute project imports (jumpstarter.common.utils.serve and jumpstarter.driver.Driver), and finally the local relative import (.driver PiPico); update the file's import order so PiPico is imported after the absolute jumpstarter imports and run make lint / make lint-fix (or isort) to apply/verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py`:
- Around line 3-19: The import block in driver.py is not sorted and fails
linting; reorder imports to follow project/PEP8 grouping and alphabetical order
(standard library, third-party, local) so imports like os, shutil, tempfile,
time, contextmanager, dataclass come first, then third-party names such as
to_thread, FileWriteStream, serial_for_url, FlasherInterface, and finally local
package imports like Driver, export and find_all_bootloader_mounts; after
reordering run make lint-fix to apply formatting and ensure the file passes the
linter.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py`:
- Around line 51-64: The function iter_bootloader_mount_candidates currently
returns a concrete list[Path] but its name suggests an iterator; rename
iter_bootloader_mount_candidates to get_bootloader_mount_candidates (or
list_bootloader_mount_candidates) and update all references/calls to that
symbol, plus any import/export entries (e.g., __all__) so the API and tests
reflect the new name; keep the implementation and return type unchanged but
change the function definition and usages to the new identifier.
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py`:
- Around line 1-11: Reorder the import block to follow standard grouping: first
standard library (asyncio, dataclasses, unittest.mock), then third-party
(pytest, jumpstarter_driver_pyserial.driver.PySerial), then absolute project
imports (jumpstarter.common.utils.serve and jumpstarter.driver.Driver), and
finally the local relative import (.driver PiPico); update the file's import
order so PiPico is imported after the absolute jumpstarter imports and run make
lint / make lint-fix (or isort) to apply/verify the change.
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py`:
- Around line 35-36: The class is named PiPico but should follow the flasher
naming convention; rename the dataclass class PiPico to PiPicoFlasher and update
all references/imports and type usages (e.g., any instantiations, annotations,
registrations) to the new symbol; ensure the class still inherits
FlasherInterface and Driver and keep the `@dataclass`(kw_only=True) decorator, and
update any module exports or factory mappings that referenced PiPico so they use
PiPicoFlasher.
- Around line 174-178: The current code redundantly calls
find_all_bootloader_mounts() after awaiting enter_bootloader(); change
enter_bootloader to return the discovered mount path(s) (or a list) instead of
None, then update the caller here to capture that return value and pass it to
_resolve_mounts instead of re-querying the filesystem; specifically modify
enter_bootloader to perform the wait/validation and return the mount(s), and
replace the second find_all_bootloader_mounts() call in this block so mount =
self._resolve_mounts(mounts_or_mount_returned_by_enter_bootloader).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abdda21b-6aac-41bc-b014-07dc08f164bc
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (10)
- python/packages/jumpstarter-driver-pi-pico/.gitignore
- python/docs/source/reference/package-apis/drivers/pi-pico.md
- python/docs/source/reference/package-apis/drivers/index.md
- python/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.py
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yaml
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yaml
- python/packages/jumpstarter-driver-pi-pico/README.md
- python/packages/jumpstarter-driver-pi-pico/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-all/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py (1)
34-35: Consider adopting the*Flashersuffix per coding guidelines.The driver implements
FlasherInterface, and the coding guidelines specify flasher drivers should use a*Flashersuffix (e.g.,PiPicoFlasherinstead ofPiPico).♻️ Proposed rename
-class PiPico(FlasherInterface, Driver): +class PiPicoFlasher(FlasherInterface, Driver):Also update the client reference in
client():- return "jumpstarter_driver_pi_pico.client.PiPicoClient" + return "jumpstarter_driver_pi_pico.client.PiPicoFlasherClient"As per coding guidelines: "Driver class names should be in CamelCase and use appropriate suffixes based on functionality: *Flasher for flasher drivers"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py` around lines 34 - 35, The class PiPico implements FlasherInterface but does not follow the naming guideline; rename the class PiPico to PiPicoFlasher and update all internal references accordingly (including its class declaration that currently lists FlasherInterface and Driver and any factory/client code that constructs or type-checks PiPico such as the client() function), ensure imports, type annotations, and any subclass/instance checks are updated to PiPicoFlasher, and run tests/linters to validate there are no remaining references to the old PiPico symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py`:
- Around line 34-35: The class PiPico implements FlasherInterface but does not
follow the naming guideline; rename the class PiPico to PiPicoFlasher and update
all internal references accordingly (including its class declaration that
currently lists FlasherInterface and Driver and any factory/client code that
constructs or type-checks PiPico such as the client() function), ensure imports,
type annotations, and any subclass/instance checks are updated to PiPicoFlasher,
and run tests/linters to validate there are no remaining references to the old
PiPico symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee18d0bb-e260-43fb-b6f7-674669f96726
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (9)
- python/packages/jumpstarter-driver-pi-pico/.gitignore
- python/docs/source/reference/package-apis/drivers/pi-pico.md
- python/docs/source/reference/package-apis/drivers/index.md
- python/packages/jumpstarter-all/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yaml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.py
- python/packages/jumpstarter-driver-pi-pico/README.md
- python/packages/jumpstarter-driver-pi-pico/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- python/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yaml
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-pi-pico/pyproject.toml (1)
30-34: Consider specifying the coverage source package.The
--covoption without a source argument will track coverage for all code executed during tests, which may include dependencies. For accurate per-package coverage, specify the module explicitly.♻️ Proposed fix
[tool.pytest.ini_options] -addopts = "--cov --cov-report=html --cov-report=xml" +addopts = "--cov=jumpstarter_driver_pi_pico --cov-report=html --cov-report=xml" log_cli = true log_cli_level = "INFO" testpaths = ["jumpstarter_driver_pi_pico"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/pyproject.toml` around lines 30 - 34, The pytest configuration uses a generic --cov flag in the addopts setting which collects coverage for everything; update the addopts value in the [tool.pytest.ini_options] block to specify the package under test (e.g., use --cov=jumpstarter_driver_pi_pico) so coverage is limited to the jumpstarter_driver_pi_pico package; modify the addopts string (referencing the addopts key) to include the explicit --cov=<package> argument while keeping the other --cov-report options intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter-driver-pi-pico/pyproject.toml`:
- Around line 30-34: The pytest configuration uses a generic --cov flag in the
addopts setting which collects coverage for everything; update the addopts value
in the [tool.pytest.ini_options] block to specify the package under test (e.g.,
use --cov=jumpstarter_driver_pi_pico) so coverage is limited to the
jumpstarter_driver_pi_pico package; modify the addopts string (referencing the
addopts key) to include the explicit --cov=<package> argument while keeping the
other --cov-report options intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d3164f3-9f78-4897-9a35-dda29606b21a
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (9)
- python/packages/jumpstarter-driver-pi-pico/.gitignore
- python/docs/source/reference/package-apis/drivers/pi-pico.md
- python/docs/source/reference/package-apis/drivers/index.md
- python/packages/jumpstarter-all/pyproject.toml
- python/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.py
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yaml
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yaml
- python/packages/jumpstarter-driver-pi-pico/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yaml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py (1)
79-97: Consider clarifying the GPIO reset sequence documentation.The docstring mentions "assert BOOTSEL low" and "pulse RUN low", but the code calls
on()andoff(). This works correctly when the GPIO children are configured withactive_low=True(which inverts the logic), but could confuse readers who expecton()to mean "high" andoff()to mean "low".Consider adding a brief note that the semantics depend on
active_lowconfiguration of the GPIO children.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py` around lines 79 - 97, Update the _gpio_bootsel_reset docstring to clarify that the calls to bootsel.on()/off() and run.on()/off() depend on the GPIO child's active_low setting (i.e., on() may drive the physical pin low when active_low=True), and explicitly state that the routine asserts the BOOTSEL signal level as required by the hardware (via the children "bootsel" and "run") then pulses RUN and releases BOOTSEL; keep the existing sequence description but add one sentence about the active_low semantics so readers understand why on()/off() correspond to "assert low" in this code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.py`:
- Around line 79-97: Update the _gpio_bootsel_reset docstring to clarify that
the calls to bootsel.on()/off() and run.on()/off() depend on the GPIO child's
active_low setting (i.e., on() may drive the physical pin low when
active_low=True), and explicitly state that the routine asserts the BOOTSEL
signal level as required by the hardware (via the children "bootsel" and "run")
then pulses RUN and releases BOOTSEL; keep the existing sequence description but
add one sentence about the active_low semantics so readers understand why
on()/off() correspond to "assert low" in this code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8af4e803-7da8-48b5-ba75-92bab4314cc3
📒 Files selected for processing (16)
python/docs/source/reference/package-apis/drivers/index.mdpython/docs/source/reference/package-apis/drivers/pi-pico.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/.gitignorepython/packages/jumpstarter-driver-pi-pico/README.mdpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yamlpython/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yamlpython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/__init__.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver.pypython/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/driver_test.pypython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (10)
- python/docs/source/reference/package-apis/drivers/pi-pico.md
- python/packages/jumpstarter-all/pyproject.toml
- python/pyproject.toml
- python/docs/source/reference/package-apis/drivers/index.md
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount_test.py
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter.yaml
- python/packages/jumpstarter-driver-pi-pico/README.md
- python/packages/jumpstarter-driver-pi-pico/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/.gitignore
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/bootloader_mount.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-gpio.yaml
- python/packages/jumpstarter-driver-pi-pico/examples/pico-exporter-standalone.yaml
- python/packages/jumpstarter-driver-pi-pico/jumpstarter_driver_pi_pico/client.py
mangelajo
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pi Pico UF2 flashing