Skip to content

Add guarded USB device controls and firmware flashing - #22

Merged
hardcoreerik merged 4 commits into
mainfrom
codex/usb-device-controls
Aug 11, 2026
Merged

Add guarded USB device controls and firmware flashing#22
hardcoreerik merged 4 commits into
mainfrom
codex/usb-device-controls

Conversation

@hardcoreerik

@hardcoreerik hardcoreerik commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • add a USB-only Device page for typed Meshtastic radio/module configuration, identity, channels, fixed position, maintenance, and guarded resets
  • keep passwords, PINs, PSKs, and cryptographic key material write-only or non-displayed
  • discover and download firmware only from official Meshtastic GitHub releases, validating the release SHA-256, target/hardware metadata, image hashes, partition offsets, USB identity, and ESP32 chip ID
  • support settings-preserving update flashes and typed-confirmation full erase/install through bundled esptool
  • release/reacquire the COM port around flashing and provide BOOT/RESET recovery guidance

Hardware verification

  • read-only sync against COM8 confirmed tbeam-s3-core / LILYGO_TBEAM_S3_CORE / ESP32-S3 USB 303a:1001
  • 22 configuration sections and 8 channels normalized
  • all write-only secret and cryptographic values remained blank
  • no configuration write, reboot, erase, or firmware flash was performed

Tests

  • .venv\Scripts\python.exe -m pytest -q (425 passed)
  • .venv\Scripts\ruff.exe check src tests (passed)
  • git diff --check (passed)
  • .\scripts\build.ps1 -SkipTests (passed)
  • packaged OrcMesh.exe hidden startup smoke test (remained running after 8 seconds)

Summary by CodeRabbit

  • New Features
    • Added a Device page for managing connected radios, including identity, channels, configuration, position, reboot, reset, and shutdown controls.
    • Added official firmware discovery, verification, download, preparation, and ESP32 flashing with update-only and full-install options.
    • Added USB identification, chip checks, progress tracking, and recovery guidance for firmware operations.
  • Security
    • Protected write-only credentials and prevented editing cryptographic key fields.
  • Documentation
    • Documented device controls, firmware management, flashing warnings, and verification behavior.
    • Updated the roadmap to reflect shipped and deferred features.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e8d79f11-05c9-4453-ba7a-ac2913f0251c

📥 Commits

Reviewing files that changed from the base of the PR and between 554cddf and ce0ff4d.

📒 Files selected for processing (2)
  • src/meshchat/services/firmware.py
  • tests/test_firmware_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/meshchat/services/firmware.py

📝 Walkthrough

Walkthrough

The application adds connected-radio configuration and maintenance controls, verified ESP32 firmware discovery and flashing, threaded controller workflows, a device page, esptool packaging support, documentation updates, and tests.

Changes

Device Controls and Firmware Management

Layer / File(s) Summary
Device control models and validation
src/meshchat/models/device_control.py, src/meshchat/services/device_config.py, tests/test_device_config.py
Adds immutable control models and secret-safe configuration discovery, coercion, validation, and persistence.
Verified firmware discovery and flashing
src/meshchat/services/firmware.py, tests/test_firmware_service.py, pyproject.toml, requirements.txt, packaging/orcmesh.spec, THIRD_PARTY_LICENSES.md, README.md, ROADMAP.md
Adds manifest-based release discovery, cached download verification, bundle validation, USB and chip checks, update-only and full-install flashing, esptool packaging, tests, and documentation.
Controller workflows and radio operations
src/meshchat/controllers/firmware_controller.py, src/meshchat/controllers/meshtastic_controller.py, tests/test_firmware_service.py
Adds threaded firmware operations and queued radio configuration, identity, channel, position, reboot, shutdown, reset, snapshot, progress, log, completion, and error handling.
Device page and main-window integration
src/meshchat/ui/device/*, src/meshchat/ui/main_window.py, tests/test_device_page.py
Adds device configuration, channel, maintenance, and firmware controls. MainWindow wires navigation, controllers, USB disconnect/reconnect handling, and cleanup.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant DevicePage
  participant MainWindow
  participant FirmwareController
  participant FirmwareService
  participant ESP32Radio

  Operator->>DevicePage: Select firmware operation
  DevicePage->>MainWindow: Emit firmware request
  MainWindow->>FirmwareController: Queue discovery or preparation
  FirmwareController->>FirmwareService: Validate release and build bundle
  FirmwareService-->>DevicePage: Report progress and verified bundle
  DevicePage->>MainWindow: Confirm flash with USB identity
  MainWindow->>ESP32Radio: Disconnect active serial connection
  MainWindow->>FirmwareController: Queue flashing
  FirmwareController->>FirmwareService: Flash verified bundle
  FirmwareService->>ESP32Radio: Run esptool commands
  FirmwareService-->>DevicePage: Return logs and completion status
  MainWindow->>ESP32Radio: Reconnect after flashing
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the pull request's main changes: guarded USB device controls and firmware flashing.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/usb-device-controls

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Nitpick comments (5)
src/meshchat/services/device_config.py (1)

192-196: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Validate section_name against _WRITABLE_SECTIONS before the write.

hasattr accepts any attribute name on the config message. For a scalar attribute such as version, or for a bound method name, line 197 raises AttributeError instead of the intended ValueError. The read path already restricts sections to _WRITABLE_SECTIONS (line 120). Apply the same allowlist on the write path so both directions share one contract.

♻️ Proposed fix
 def apply_section(node, section_name: str, changes: dict[str, Any]) -> None:
+    if section_name not in _WRITABLE_SECTIONS:
+        raise ValueError(f"Unknown configuration section: {section_name}")
     parent = node.localConfig if hasattr(node.localConfig, section_name) else node.moduleConfig
     if not hasattr(parent, section_name):
         raise ValueError(f"Unknown configuration section: {section_name}")
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/meshchat/services/device_config.py` around lines 192 - 196, Update
apply_section to validate section_name against _WRITABLE_SECTIONS before
selecting or accessing the configuration parent, raising ValueError for any
disallowed section. Keep the existing parent selection and valid-section write
behavior unchanged, matching the read path’s allowlist contract.
tests/test_firmware_service.py (1)

119-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add one test that runs against the installed esptool parser.

Every flashing test replaces sys.modules["esptool"] with a stub, so the asserted command names are never checked against a real esptool. This is why the v4/v5 command-name mismatch reached review. Add a guard test that asserts the installed esptool accepts chip-id, erase-flash, and write-flash.

💚 Proposed test
def test_installed_esptool_accepts_the_command_names_used_by_flash_bundle():
    esptool = pytest.importorskip("esptool")
    with pytest.raises(SystemExit) as exit_info:
        esptool.main(["--help"])
    assert exit_info.value.code == 0
    # Fails on esptool 4.x, where the commands are chip_id / erase_flash / write_flash.
    assert tuple(int(part) for part in esptool.__version__.split(".")[:1]) >= (5,)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_firmware_service.py` around lines 119 - 143, Add a separate test
near test_flash_update_uses_only_verified_update_offset that uses
pytest.importorskip("esptool") without mocking sys.modules, invokes the
installed parser through esptool.main(["--help"]) and verifies successful exit,
then assert the installed esptool major version supports the hyphenated commands
chip-id, erase-flash, and write-flash. Keep existing flashing tests and stubs
unchanged.
src/meshchat/controllers/firmware_controller.py (1)

22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log worker exceptions in addition to emitting completed.

Each handler converts the exception to str(exc) and discards the traceback. Firmware failures are hard to diagnose from a one-line message. The rest of the codebase uses log.exception for the same pattern, for example in MeshtasticWorker.

Add a module logger and call log.exception(...) in each except block before emitting completed.

Also applies to: 34-35, 45-46

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/meshchat/controllers/firmware_controller.py` around lines 22 - 23, Add a
module-level logger and update every exception handler in the firmware
controller, including the handlers around discover and the other referenced
operations, to call log.exception(...) before emitting completed. Preserve each
handler’s existing completion signal and error text.
src/meshchat/ui/device/device_page.py (1)

230-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

set_connected clears _snapshot as a side effect, which set_snapshot then relies on ordering to survive.

set_snapshot assigns self._snapshot = snapshot at Line 240, then calls set_connected(...) at Line 241. If snapshot.serial_port is None, set_connected(False) resets self._snapshot back to None, but the following lines still populate the summary, identity fields, sections, and channels from that snapshot. The page then shows device data while self._snapshot is None.

The current behavior is safe because the tabs are disabled and every action guards on self._snapshot is None. The coupling is fragile. Move the _snapshot reset out of set_connected and clear it explicitly in the disconnect path.

♻️ Proposed refactor
     def set_connected(self, connected: bool) -> None:
         self._tabs.setEnabled(connected)
         self._refresh.setEnabled(connected)
         if not connected:
-            self._snapshot = None
             self._summary.setText("Connect a Meshtastic radio over USB / Serial to manage it.")
             self._flash_update.setEnabled(False)
             self._flash_full.setEnabled(False)

Then clear the snapshot where disconnect is handled, for example in MainWindow._on_disconnected before set_connected(False), or add a small clear() method on DevicePage.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/meshchat/ui/device/device_page.py` around lines 230 - 241, Remove the
_snapshot reset from DevicePage.set_connected so set_snapshot preserves the
assigned snapshot while updating the connection state. Clear the device page
snapshot explicitly in the disconnect flow, preferably in
MainWindow._on_disconnected before calling set_connected(False), or through a
dedicated DevicePage clear method.
tests/test_device_page.py (1)

5-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the destructive and parsing paths of DevicePage.

The current tests cover tab labels and the summary text. The highest-risk logic is untested:

  • _confirm_flash typed-confirmation gating, including the case where the typed phrase does not match and no signal is emitted.
  • _confirm_factory_reset for both full=False and full=True.
  • _read_widget parsing of int, float, and repeated fields, including the ValueError path that _save_current_section converts into a warning.
  • _load_channel clearing _channel_psk when the user switches channels, which is the guarantee that a PSK is never carried across channels.

The snapshot factory also omits sections and channels, so _rebuild_sections and _rebuild_channels currently run against empty input only.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_device_page.py` around lines 5 - 20, Expand tests around
DevicePage to cover _confirm_flash’s matching and non-matching typed
confirmation behavior, _confirm_factory_reset with both full=False and
full=True, and _read_widget parsing for integer, float, repeated fields, and
invalid input through _save_current_section’s warning path. Add representative
sections and channels to _snapshot, and verify _load_channel clears _channel_psk
when switching channels.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packaging/orcmesh.spec`:
- Around line 83-85: Update the esptool packaging setup near hiddenimports to
collect its non-Python data files with collect_data_files, ensuring the call is
made while datas is in scope and preserves the existing collect_submodules
behavior. Include the esptool target stub JSON files in the frozen build.

In `@pyproject.toml`:
- Line 18: Update the esptool dependency constraint in pyproject.toml to require
version 5.0 or newer while preserving the existing upper bound, so firmware.py’s
v5-only hyphenated commands remain supported.

In `@requirements.txt`:
- Line 12: Update the esptool version constraint in requirements.txt to use the
minimum version that provides the v5 command names, matching the consolidated
constraint defined for pyproject.toml. Preserve the existing upper bound unless
the consolidated requirement specifies otherwise.

In `@src/meshchat/controllers/firmware_controller.py`:
- Around line 84-86: Update the firmware worker around its flash/prepare
operation handlers and shutdown() to track whether an operation is in flight,
then ensure shutdown waits until that operation completes before returning. Do
not allow the thread to be destroyed while work remains active; if
MainWindow.closeEvent participates in shutdown, preserve the existing close flow
only after the worker is no longer busy.

In `@src/meshchat/controllers/meshtastic_controller.py`:
- Around line 952-962: The successful write path in set_owner must refresh the
device-control snapshot. After setOwner succeeds, call _emit_device_controls()
before or alongside emitting device_operation_completed, matching the behavior
of apply_device_section and update_channel; leave the existing error handling
unchanged.
- Around line 993-1002: Update _run_local_node_action to accept separate
display-label and verb-phrase parameters, using the label for the success
message and the verb phrase for the error message. Update every caller,
including reset_nodedb, set_fixed_position, and remove_fixed_position, to
provide grammatically correct values while preserving the existing callback and
error-handling behavior.

In `@src/meshchat/services/firmware.py`:
- Around line 296-301: Update the expected-chip validation around expected_chip
so every declared release platform is covered by an explicit mapping; reject any
platform absent from that mapping before flashing, rather than skipping
validation when expected_chip is None. Preserve the existing case-insensitive
chip-identity mismatch error for mapped platforms and ensure both write paths
remain gated by this preflight.
- Around line 56-63: Update both _json and _download to validate each URL before
calling urllib.request.urlopen: require HTTPS and restrict the hostname to the
known GitHub hosts used by the API and release downloads. Reject any other
scheme or host, including values from browser_download_url and the manifest
response, before fetching.
- Around line 271-285: Update the local esptool invocation in run so it no
longer uses contextlib.redirect_stdout or redirect_stderr, which mutate
process-global streams from the QThread. Pass an explicit output/error stream
supported by esptool, or use a thread-local capture mechanism, while preserving
writer.text collection and the existing FirmwareError handling.
- Around line 303-309: Update the esptool command names used by the firmware
flashing flow, including the preflight command and both branches of the flash
logic around full_install, to use the underscore-form names supported by the
pinned esptool v4 dependency (chip_id, erase_flash, and write_flash). Keep the
existing arguments, offsets, and execution order unchanged.

In `@src/meshchat/ui/device/device_page.py`:
- Around line 368-373: Update the enum and channel-role widget initialization
near the non-repeated enum branch and the corresponding logic around
_read_widget so an unknown field value is preserved rather than coerced to index
0. Only select a combo-box choice when findData returns a valid index; otherwise
retain the original value and ensure saving does not overwrite it with the first
choice.
- Around line 120-131: Update the button handler mapping in the device-page
button setup so the non-full “Factory Reset” action invokes
_confirm_factory_reset with full=False through a wrapper that ignores
QPushButton.clicked’s boolean argument; preserve the existing lambda for “Full
Factory Reset” and other handlers.

In `@src/meshchat/ui/main_window.py`:
- Around line 606-619: Clear _device_snapshot in the disconnect handling path,
specifically _on_disconnected, so stale serial-port data cannot authorize a
flash after the radio is gone. Update _on_firmware_flash_requested to detect an
already-disconnected controller and proceed with the pending flash handoff
directly instead of relying on disconnect to emit an event; preserve the
existing connected flow through self._controller.disconnect().

---

Nitpick comments:
In `@src/meshchat/controllers/firmware_controller.py`:
- Around line 22-23: Add a module-level logger and update every exception
handler in the firmware controller, including the handlers around discover and
the other referenced operations, to call log.exception(...) before emitting
completed. Preserve each handler’s existing completion signal and error text.

In `@src/meshchat/services/device_config.py`:
- Around line 192-196: Update apply_section to validate section_name against
_WRITABLE_SECTIONS before selecting or accessing the configuration parent,
raising ValueError for any disallowed section. Keep the existing parent
selection and valid-section write behavior unchanged, matching the read path’s
allowlist contract.

In `@src/meshchat/ui/device/device_page.py`:
- Around line 230-241: Remove the _snapshot reset from DevicePage.set_connected
so set_snapshot preserves the assigned snapshot while updating the connection
state. Clear the device page snapshot explicitly in the disconnect flow,
preferably in MainWindow._on_disconnected before calling set_connected(False),
or through a dedicated DevicePage clear method.

In `@tests/test_device_page.py`:
- Around line 5-20: Expand tests around DevicePage to cover _confirm_flash’s
matching and non-matching typed confirmation behavior, _confirm_factory_reset
with both full=False and full=True, and _read_widget parsing for integer, float,
repeated fields, and invalid input through _save_current_section’s warning path.
Add representative sections and channels to _snapshot, and verify _load_channel
clears _channel_psk when switching channels.

In `@tests/test_firmware_service.py`:
- Around line 119-143: Add a separate test near
test_flash_update_uses_only_verified_update_offset that uses
pytest.importorskip("esptool") without mocking sys.modules, invokes the
installed parser through esptool.main(["--help"]) and verifies successful exit,
then assert the installed esptool major version supports the hyphenated commands
chip-id, erase-flash, and write-flash. Keep existing flashing tests and stubs
unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fafdc3cb-3931-4454-aee6-aa71d1920f74

📥 Commits

Reviewing files that changed from the base of the PR and between ab75322 and 43c8eb9.

📒 Files selected for processing (17)
  • README.md
  • ROADMAP.md
  • THIRD_PARTY_LICENSES.md
  • packaging/orcmesh.spec
  • pyproject.toml
  • requirements.txt
  • src/meshchat/controllers/firmware_controller.py
  • src/meshchat/controllers/meshtastic_controller.py
  • src/meshchat/models/device_control.py
  • src/meshchat/services/device_config.py
  • src/meshchat/services/firmware.py
  • src/meshchat/ui/device/__init__.py
  • src/meshchat/ui/device/device_page.py
  • src/meshchat/ui/main_window.py
  • tests/test_device_config.py
  • tests/test_device_page.py
  • tests/test_firmware_service.py

Comment thread packaging/orcmesh.spec
Comment thread pyproject.toml Outdated
Comment thread requirements.txt Outdated
Comment thread src/meshchat/controllers/firmware_controller.py Outdated
Comment thread src/meshchat/controllers/meshtastic_controller.py
Comment thread src/meshchat/services/firmware.py Outdated
Comment on lines +303 to +309
if full_install:
run(["erase-flash"])
run(["write-flash", "0x0", str(bundle.factory_image)])
run(["write-flash", bundle.ota_offset, str(bundle.ota_image)])
run(["write-flash", bundle.filesystem_offset, str(bundle.filesystem_image)])
else:
run(["write-flash", "0x10000", str(bundle.update_image)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

The hyphenated esptool commands require esptool v5.

chip-id, erase-flash, and write-flash are the v5 command names. esptool v5 renamed every command from _ to -, and the v4 parser accepts only the underscore names. The dependency pin allows 4.8. With esptool 4.x the preflight at line 288 fails in argparse, so every flash attempt aborts. The unit tests replace esptool with a stub, so they cannot catch this.

Details and the cross-file fix are in the consolidated comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/meshchat/services/firmware.py` around lines 303 - 309, Update the esptool
command names used by the firmware flashing flow, including the preflight
command and both branches of the flash logic around full_install, to use the
underscore-form names supported by the pinned esptool v4 dependency (chip_id,
erase_flash, and write_flash). Keep the existing arguments, offsets, and
execution order unchanged.

Comment thread src/meshchat/ui/device/device_page.py
Comment thread src/meshchat/ui/device/device_page.py Outdated
Comment on lines +606 to +619
def _on_device_controls_updated(self, snapshot) -> None:
self._device_snapshot = snapshot
self._device_page.set_snapshot(snapshot)

def _on_firmware_flash_requested(self, bundle, full_install: bool, expected_usb) -> None:
snapshot = self._device_snapshot
if snapshot is None or not snapshot.serial_port:
QMessageBox.warning(self, "USB Radio Required", "Reconnect the radio over USB before flashing.")
return
self._flash_port = snapshot.serial_port
self._pending_flash = (bundle, full_install, expected_usb)
self._supervisor.cancel()
self._status_bar.showMessage("Releasing the USB port for firmware flashing…")
self._controller.disconnect()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

A stale _device_snapshot can start a flash handoff that never resumes.

_device_snapshot is only assigned in _on_device_controls_updated. It is never cleared on disconnect, while DevicePage.set_connected(False) does clear the page state. Two consequences follow:

  1. If the radio is already disconnected, _on_firmware_flash_requested still passes the snapshot.serial_port guard at Line 612. It sets _pending_flash and _flash_port, then calls self._controller.disconnect(). MeshtasticWorker.disconnect returns immediately when the state is already DISCONNECTED, so disconnected is never emitted. _on_disconnected never runs, _pending_flash stays set, and the flash never starts.
  2. DevicePage._confirm_flash has already disabled both flash buttons. firmware_completed never fires, so the buttons are never re-enabled. The user has no feedback and no way to retry.

Clear _device_snapshot on disconnect, and start the flash directly when the controller is already disconnected.

🐛 Proposed fix
     def _on_disconnected(self, reason: str) -> None:
         ...
         self._device_page.set_connected(False)
+        self._device_snapshot = None
         self._status_bar.showMessage(f"Disconnected — {reason}")
     def _on_firmware_flash_requested(self, bundle, full_install: bool, expected_usb) -> None:
         snapshot = self._device_snapshot
         if snapshot is None or not snapshot.serial_port:
             QMessageBox.warning(self, "USB Radio Required", "Reconnect the radio over USB before flashing.")
             return
         self._flash_port = snapshot.serial_port
-        self._pending_flash = (bundle, full_install, expected_usb)
         self._supervisor.cancel()
+        if not self._is_connected:
+            # disconnect() is a no-op when already disconnected, so
+            # `disconnected` would never fire to resume the handoff.
+            self._firmware_controller.flash(bundle, self._flash_port, full_install, expected_usb)
+            return
+        self._pending_flash = (bundle, full_install, expected_usb)
         self._status_bar.showMessage("Releasing the USB port for firmware flashing…")
         self._controller.disconnect()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/meshchat/ui/main_window.py` around lines 606 - 619, Clear
_device_snapshot in the disconnect handling path, specifically _on_disconnected,
so stale serial-port data cannot authorize a flash after the radio is gone.
Update _on_firmware_flash_requested to detect an already-disconnected controller
and proceed with the pending flash handoff directly instead of relying on
disconnect to emit an event; preserve the existing connected flow through
self._controller.disconnect().

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_firmware_service.py`:
- Around line 240-245: Update
test_installed_esptool_accepts_commands_used_by_flasher to wrap each
esptool.main([command, "--help"]) call in handling for SystemExit, asserting the
caught exception has code zero while allowing normal help termination. Preserve
the existing command iteration and version check.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d11c06e7-7e28-41bf-b991-4ac98a10726f

📥 Commits

Reviewing files that changed from the base of the PR and between 437e4b2 and 554cddf.

📒 Files selected for processing (12)
  • packaging/orcmesh.spec
  • pyproject.toml
  • requirements.txt
  • src/meshchat/controllers/firmware_controller.py
  • src/meshchat/controllers/meshtastic_controller.py
  • src/meshchat/services/device_config.py
  • src/meshchat/services/firmware.py
  • src/meshchat/ui/device/device_page.py
  • src/meshchat/ui/main_window.py
  • tests/test_device_config.py
  • tests/test_device_page.py
  • tests/test_firmware_service.py
🚧 Files skipped from review as they are similar to previous changes (7)
  • packaging/orcmesh.spec
  • tests/test_device_config.py
  • pyproject.toml
  • src/meshchat/ui/device/device_page.py
  • src/meshchat/ui/main_window.py
  • src/meshchat/controllers/meshtastic_controller.py
  • src/meshchat/services/device_config.py

Comment on lines +240 to +245
def test_installed_esptool_accepts_commands_used_by_flasher():
esptool = pytest.importorskip("esptool")
assert int(version("esptool").split(".", 1)[0]) >= 5
for command in ("chip-id", "erase-flash", "write-flash"):
esptool.main([command, "--help"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python - <<'PY'
import esptool

for command in ("chip-id", "erase-flash", "write-flash"):
    try:
        esptool.main([command, "--help"])
    except SystemExit as exc:
        assert exc.code == 0, (command, exc.code)
    else:
        raise AssertionError(f"{command} did not exit from --help")
PY

Repository: hardcoreerik/OrcMesh

Length of output: 278


🏁 Script executed:

set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(tests/test_firmware_service\.py|pyproject\.toml|requirements[^/]*|setup\.cfg|tox\.ini|.*\.ya?ml)$' | head -80
printf '%s\n' '--- test context ---'
sed -n '228,252p' tests/test_firmware_service.py
printf '%s\n' '--- esptool references ---'
rg -n -i 'esptool|pytest.importorskip' . --glob '!*.lock' --glob '!package-lock.json' | head -120

Repository: hardcoreerik/OrcMesh

Length of output: 3176


🏁 Script executed:

set -euo pipefail
printf '%s\n' '--- dependency and test configuration ---'
sed -n '1,80p' pyproject.toml
printf '%s\n' '--- production esptool invocation ---'
sed -n '268,300p' src/meshchat/services/firmware.py
printf '%s\n' '--- esptool version-5 references in repository ---'
rg -n -C 3 'esptool\.main|SystemExit|--help' tests src pyproject.toml requirements.txt

Repository: hardcoreerik/OrcMesh

Length of output: 5385


🌐 Web query:

espressif esptool v5.0.0 esptool.main standalone_mode SystemExit --help source

💡 Result:

In esptool v5.0.0 and subsequent versions, calling esptool.main to run commands programmatically often causes the Python process to exit because the underlying implementation is designed for CLI execution [1][2]. The behavior you are encountering is rooted in how Python command-line interfaces are typically built using the click library, where standalone_mode=True is the default [3]. In this mode, the application handles its own exceptions and automatically calls sys.exit upon completion or when displaying help messages [3]. Key points regarding this behavior in esptool v5.0.0: 1. Process Termination: When you call esptool.main with arguments (e.g., ['--chip', 'esp32', 'read_mac']), it executes as a standalone command, which forces the Python interpreter to terminate with a SystemExit exception (or simply exit the process) once the operation finishes [1][2]. 2. Programmatic Alternatives: To avoid this behavior when integrating esptool into your own scripts or applications, avoid calling esptool.main directly [4][2]. Instead, use the updated Python API provided in v5.0.0, which has been refactored for better programmatic access [5][6]. The official esptool documentation recommends using functions within the esptool.cmds module (e.g., esptool.cmds.connect_esp or esptool.cmds.detect_chip) for stable, non-terminating integration [4]. 3. Migration Context: Esptool v5.0.0 introduced significant breaking changes, including the refactoring of internal command dependencies and the removal of the.py suffix for console scripts [7][5]. If your workflow relies on legacy patterns, consult the official v5 Migration Guide for the recommended way to invoke tools and use the module interface [5][6]. If you must capture output or control execution flow without the process exiting, one workaround is to invoke the esptool script as a subprocess via subprocess.Popen rather than importing it as a module [2]. This isolates the exit behavior to the subprocess, allowing your main script to continue running [2].

Citations:


🌐 Web query:

site:github.com/espressif/esptool/blob/v5.0.0/esptool/__init__.py def main standalone_mode

💡 Result:

In esptool v5.0.0, main is defined at line 974:

def main(argv: list[str] | None = None, esp: ESPLoader | None = None):
    args = expand_file_arguments(argv or sys.argv[1:])
    cli(args=args, esp=esp)

There is no standalone_mode parameter in this function; it invokes the Click command via cli(...). [1]


Handle the normal help exit from esptool.main().

Each --help invocation raises SystemExit(0) through Click's standalone CLI mode. Catch the expected exit and assert that its code is zero.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_firmware_service.py` around lines 240 - 245, Update
test_installed_esptool_accepts_commands_used_by_flasher to wrap each
esptool.main([command, "--help"]) call in handling for SystemExit, asserting the
caught exception has code zero while allowing normal help termination. Preserve
the existing command iteration and version check.

@hardcoreerik
hardcoreerik merged commit b580b89 into main Aug 11, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant