Add firmware diagnostics and recovery workbench - #23
Conversation
📝 WalkthroughWalkthroughThe PR adds firmware probing and raw flash backup, profile backup and restore, serial-console controls, USB identity validation, operation logging, and PyInstaller serial-port packaging support. ChangesDevice operations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DevicePage
participant MainWindow
participant FirmwareController
participant firmware
DevicePage->>MainWindow: Request probe or flash backup
MainWindow->>FirmwareController: Start operation after USB release
FirmwareController->>firmware: Validate USB and run esptool
firmware-->>FirmwareController: Return logs and completion
FirmwareController-->>MainWindow: Emit operation result
MainWindow-->>DevicePage: Update UI and reconnect state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/meshchat/services/firmware.py (2)
359-371: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winesptool output capture relies on replacing the process-wide standard streams with a minimal writer. Both sites follow from one design choice:
_run_esptoolswapssys.stdoutandsys.stderrfor a_LineWriterthat implements onlywriteandflush. That makes the capture visible to every thread in the process, and it breaks any consumer that queries a normal file attribute.
src/meshchat/services/firmware.py#L359-L371: confirm that no other thread prints during a firmware operation, or replace the global redirect with an esptool-specific output hook.src/meshchat/services/firmware.py#L300-L317: addisatty()and anencodingattribute to_LineWriterso attribute queries do not raiseAttributeErrorinside the redirect.🤖 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 359 - 371, The _run_esptool flow uses process-wide stream redirection; confirm firmware operations prevent other threads from printing, or replace redirect_stdout/redirect_stderr with an esptool-specific output hook. In _LineWriter, add isatty() and an encoding attribute so redirected consumers can query normal stream properties without AttributeError. Apply the changes at src/meshchat/services/firmware.py lines 359-371 and 300-317.
359-371: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffAvoid process-wide stream redirection for concurrent output. The application has no runtime
print()calls outside--version, and its logging handler retains the originalsys.stderr. Ifesptoolor another dependency writes from another thread,_LineWriterreceives that output through the global streams without synchronization. Use an esptool-specific output hook when concurrent output is supported.🤖 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 359 - 371, The firmware invocation around esptool.main currently redirects process-wide stdout and stderr, allowing concurrent writes from unrelated threads to reach _LineWriter unsafely. Replace redirect_stdout/redirect_stderr with esptool’s supported instance-specific output hook or equivalent, wiring it to writer while preserving the existing arguments, exception conversion, and final flush behavior.src/meshchat/ui/main_window.py (2)
666-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
_release_for_firmwarein the other USB handlers.
_on_firmware_flash_requestedat lines 655-664 and_on_serial_console_requestedat lines 688-696 repeat the connection check, the_flash_portassignment, the_supervisor.cancel()call, and the status message that_release_for_firmwarealready implements. Route both through the helper to keep one definition of the release sequence.♻️ Proposed change
def _on_firmware_flash_requested(self, bundle, full_install: bool, expected_usb) -> None: - snapshot = self._device_snapshot - if not self._is_connected or 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() + if self._release_for_firmware("Releasing the USB port for firmware flashing…"): + self._pending_flash = (bundle, full_install, expected_usb) + self._controller.disconnect()def _on_serial_console_requested(self, port: str, baud: int) -> None: snapshot = self._device_snapshot - if not self._is_connected or snapshot is None or snapshot.serial_port != port: - QMessageBox.warning(self, "USB Radio Required", "Reconnect the selected USB radio first.") - return - self._pending_serial_console = (port, baud) - self._supervisor.cancel() - self._status_bar.showMessage("Releasing USB for read-only serial console…") - self._controller.disconnect() + if snapshot is not None and snapshot.serial_port != port: + QMessageBox.warning(self, "USB Radio Required", "Reconnect the selected USB radio first.") + return + if self._release_for_firmware("Releasing USB for read-only serial console…"): + self._pending_serial_console = (port, baud) + 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 666 - 696, Route _on_firmware_flash_requested and _on_serial_console_requested through _release_for_firmware instead of duplicating connection validation, _flash_port assignment, supervisor cancellation, and status updates. Preserve each handler’s existing pending-operation assignment and subsequent controller disconnect, using the helper’s success result to guard those steps.
624-645: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against a second queued USB operation.
The
if/elifchain runs at most one pending operation per disconnect._on_firmware_probe_requested,_on_firmware_backup_requested, and_on_serial_console_requestedeach set a different field, so two requests issued before the firstdisconnectedsignal arrive as two pending entries. Only the first branch runs, and the second operation is silently dropped with no message to the user.Reject a new request while any pending operation exists.
♻️ Proposed guard in `_release_for_firmware`
def _release_for_firmware(self, detail: str) -> bool: snapshot = self._device_snapshot + if any(( + self._pending_flash, self._pending_probe, + self._pending_flash_backup, self._pending_serial_console, + )): + QMessageBox.warning( + self, "Operation In Progress", + "Wait for the current USB operation to finish.", + ) + return False if not self._is_connected or snapshot is None or not snapshot.serial_port:Apply the same guard in
_on_serial_console_requested.🤖 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 624 - 645, Guard each firmware operation request handler, including _on_firmware_probe_requested, _on_firmware_backup_requested, and _on_serial_console_requested, by rejecting the request when any pending operation field is already set. Preserve the existing pending-operation scheduling and clear the new request without overwriting the queued one; apply the equivalent guard in _release_for_firmware as requested.src/meshchat/controllers/firmware_controller.py (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
Pathimport to module scope.
backupimportsPathinside the method body. The other worker methods use module-level imports. Move the import to the top of the file for consistency.♻️ Proposed change
`@Slot`(str, str, object) def backup(self, port: str, destination: str, expected_usb) -> None: try: - from pathlib import Path backup_flash(port, Path(destination), expected_usb, lambda line: self.log.emit(line))Add at the top of the file:
from pathlib import Path🤖 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 71 - 76, Move the pathlib.Path import from inside the backup method to module scope, alongside the existing imports, and keep backup using the same Path symbol when calling backup_flash.src/meshchat/ui/device/device_page.py (1)
331-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the hardcoded tab count with an explicit reference to the Firmware tab.
range(min(3, self._tabs.count()))depends on the Firmware tab being the fourth tab. If a tab is added or reordered, this loop disables the wrong tabs. Store the Firmware tab index when you create it in_build_firmware_tab, then skip that index here.♻️ Proposed change
In
_build_firmware_tab:- self._tabs.addTab(page, "Firmware") + self._firmware_tab_index = self._tabs.addTab(page, "Firmware")In
set_connected:- for index in range(min(3, self._tabs.count())): - self._tabs.setTabEnabled(index, connected) + for index in range(self._tabs.count()): + if index != self._firmware_tab_index: + self._tabs.setTabEnabled(index, connected)🤖 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 331 - 341, Update _build_firmware_tab to store the created Firmware tab’s index, then revise set_connected to iterate over the actual tab count while skipping that stored index; remove the hardcoded min(3, ...) assumption and continue applying the existing enabled state to all other tabs.tests/test_firmware_service.py (1)
341-348: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the expected partial path in the assertion.
backup_flashcallsdestination.resolve()before it derives the partial path. The test compares against the unresolvedtmp_path. On platforms where the temporary directory is a symbolic link, the two strings differ and the assertion fails. Resolve the destination in the test to remove that dependency.♻️ Proposed change
- destination = tmp_path / "radio.bin" + destination = (tmp_path / "radio.bin").resolve()🤖 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 341 - 348, Update the expected command assertion in the backup_flash test to derive the partial path from destination.resolve(), matching the path resolution performed by backup_flash while preserving the existing command arguments and assertions.
🤖 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 `@src/meshchat/services/device_profile.py`:
- Around line 24-29: Update the profile backup flow around the temporary
“.cfg.partial” path to create a randomized temporary file inside
destination.parent with owner-only permissions, avoiding predictable names and
symlink races; write the raw configuration to that securely created file, then
atomically replace destination and clean up the temporary file.
- Line 16: Update src/meshchat/services/device_profile.py:16 and the
backup/restore logic at src/meshchat/services/device_profile.py:64-84 to use
APIs and DeviceProfile fields supported by the declared Meshtastic minimum
version, or raise that minimum version to one providing export_profile,
canned_messages, ringtone, and fixed_position. Update
tests/test_device_profile.py:10-12 and tests/test_device_profile.py:22-28 to
match the selected supported API and schema.
In `@src/meshchat/services/firmware.py`:
- Around line 398-410: Update probe_device so get-security-info is optional for
unsupported chips, allowing FirmwareError from that command without aborting the
probe. Ensure the radio reset is always performed by moving the final hard-reset
invocation into a finally block, while preserving the existing read-mac and
flash-id commands and completion output.
In `@src/meshchat/ui/device/device_page.py`:
- Around line 502-508: Update _save_text to catch OSError around opening and
writing the selected report file, and show the same user-facing warning used by
MainWindow._export_nodes. Keep the existing filename extension handling and
successful-write behavior unchanged.
- Around line 551-553: Update _serial_error to report every error other than
NoError without requiring self._serial.isOpen(). For fatal serial errors such as
ResourceError, also reset the console controls by enabling _console_start and
disabling _console_stop, matching the stopped state.
In `@tests/test_firmware_service.py`:
- Around line 351-362: Update
test_installed_esptool_accepts_commands_used_by_flasher so each esptool.main
help invocation is wrapped in pytest.raises(SystemExit), asserting the captured
exit code is 0 or None while preserving the existing command coverage and
arguments.
---
Nitpick comments:
In `@src/meshchat/controllers/firmware_controller.py`:
- Around line 71-76: Move the pathlib.Path import from inside the backup method
to module scope, alongside the existing imports, and keep backup using the same
Path symbol when calling backup_flash.
In `@src/meshchat/services/firmware.py`:
- Around line 359-371: The _run_esptool flow uses process-wide stream
redirection; confirm firmware operations prevent other threads from printing, or
replace redirect_stdout/redirect_stderr with an esptool-specific output hook. In
_LineWriter, add isatty() and an encoding attribute so redirected consumers can
query normal stream properties without AttributeError. Apply the changes at
src/meshchat/services/firmware.py lines 359-371 and 300-317.
- Around line 359-371: The firmware invocation around esptool.main currently
redirects process-wide stdout and stderr, allowing concurrent writes from
unrelated threads to reach _LineWriter unsafely. Replace
redirect_stdout/redirect_stderr with esptool’s supported instance-specific
output hook or equivalent, wiring it to writer while preserving the existing
arguments, exception conversion, and final flush behavior.
In `@src/meshchat/ui/device/device_page.py`:
- Around line 331-341: Update _build_firmware_tab to store the created Firmware
tab’s index, then revise set_connected to iterate over the actual tab count
while skipping that stored index; remove the hardcoded min(3, ...) assumption
and continue applying the existing enabled state to all other tabs.
In `@src/meshchat/ui/main_window.py`:
- Around line 666-696: Route _on_firmware_flash_requested and
_on_serial_console_requested through _release_for_firmware instead of
duplicating connection validation, _flash_port assignment, supervisor
cancellation, and status updates. Preserve each handler’s existing
pending-operation assignment and subsequent controller disconnect, using the
helper’s success result to guard those steps.
- Around line 624-645: Guard each firmware operation request handler, including
_on_firmware_probe_requested, _on_firmware_backup_requested, and
_on_serial_console_requested, by rejecting the request when any pending
operation field is already set. Preserve the existing pending-operation
scheduling and clear the new request without overwriting the queued one; apply
the equivalent guard in _release_for_firmware as requested.
In `@tests/test_firmware_service.py`:
- Around line 341-348: Update the expected command assertion in the backup_flash
test to derive the partial path from destination.resolve(), matching the path
resolution performed by backup_flash while preserving the existing command
arguments and assertions.
🪄 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: b3fd122a-b170-4be1-b0b6-84940dcecb8e
📒 Files selected for processing (10)
packaging/orcmesh.specsrc/meshchat/controllers/firmware_controller.pysrc/meshchat/controllers/meshtastic_controller.pysrc/meshchat/services/device_profile.pysrc/meshchat/services/firmware.pysrc/meshchat/ui/device/device_page.pysrc/meshchat/ui/main_window.pytests/test_device_page.pytests/test_device_profile.pytests/test_firmware_service.py
|
|
||
|
|
||
| def save_profile(interface, destination: Path) -> str: | ||
| from meshtastic.__main__ import export_profile |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the repository declaration that selects the Meshtastic version.
fd -HI -t f . | rg '/?(pyproject\.toml|requirements[^/]*\.txt|poetry\.lock|uv\.lock|Pipfile(\.lock)?)$' |
while IFS= read -r file; do
rg -n -i 'meshtastic' "$file" || true
done
# Inspect Meshtastic 2.5.0 without importing or executing package code.
wheel="$(mktemp)"
curl -fsSL \
'https://files.pythonhosted.org/packages/py3/m/meshtastic/meshtastic-2.5.0-py3-none-any.whl' \
-o "$wheel"
echo '--- exported profile functions ---'
unzip -p "$wheel" meshtastic/__main__.py |
rg -n '^def (export_profile|export_config)\b' || true
echo '--- DeviceProfile fields ---'
unzip -p "$wheel" meshtastic/protobuf/clientonly_pb2.py |
rg -n 'fixed_position|ringtone|canned_messages|DeviceProfile' || true
rm -f "$wheel"Repository: hardcoreerik/OrcMesh
Length of output: 1295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- dependency declarations ---'
rg -n -i -C 2 'meshtastic' pyproject.toml requirements\*.txt 2>/dev/null || true
echo '--- device profile service ---'
cat -n src/meshchat/services/device_profile.py | sed -n '1,120p'
echo '--- device profile tests ---'
cat -n tests/test_device_profile.py | sed -n '1,180p'
echo '--- profile API references ---'
rg -n -C 2 'export_profile|export_config|fixed_position|ringtone|canned_messages' src testsRepository: hardcoreerik/OrcMesh
Length of output: 17426
Raise the Meshtastic minimum version or remove unsupported profile APIs.
meshtastic>=2.5.0,<3 permits 2.5.0, which defines export_config, not export_profile. Its DeviceProfile schema lacks canned_messages, ringtone, and fixed_position. Backup, restore, and the first two profile tests fail with this supported dependency version.
📍 Affects 2 files
src/meshchat/services/device_profile.py#L16-L16(this comment)src/meshchat/services/device_profile.py#L64-L84tests/test_device_profile.py#L10-L12tests/test_device_profile.py#L22-L28
🤖 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_profile.py` at line 16, Update
src/meshchat/services/device_profile.py:16 and the backup/restore logic at
src/meshchat/services/device_profile.py:64-84 to use APIs and DeviceProfile
fields supported by the declared Meshtastic minimum version, or raise that
minimum version to one providing export_profile, canned_messages, ringtone, and
fixed_position. Update tests/test_device_profile.py:10-12 and
tests/test_device_profile.py:22-28 to match the selected supported API and
schema.
| partial = destination.with_suffix(".cfg.partial") | ||
| try: | ||
| partial.write_bytes(raw) | ||
| os.replace(partial, destination) | ||
| finally: | ||
| partial.unlink(missing_ok=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Protect profile backups from disclosure and path races.
Lines 24-29 write sensitive radio configuration to a predictable partial path with the process default permissions. If the selected directory is shared, another local user can read the partial file or replace it with a symlink before write_bytes() runs.
Create a random temporary file with owner-only permissions in destination.parent. Then atomically replace the destination.
Proposed fix
+import tempfile
+
- partial = destination.with_suffix(".cfg.partial")
+ fd, partial_name = tempfile.mkstemp(
+ dir=destination.parent,
+ prefix=f".{destination.stem}.",
+ suffix=".cfg.partial",
+ )
+ partial = Path(partial_name)
try:
- partial.write_bytes(raw)
+ with os.fdopen(fd, "wb") as profile_file:
+ profile_file.write(raw)
os.replace(partial, destination)
finally:
partial.unlink(missing_ok=True)📝 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.
| partial = destination.with_suffix(".cfg.partial") | |
| try: | |
| partial.write_bytes(raw) | |
| os.replace(partial, destination) | |
| finally: | |
| partial.unlink(missing_ok=True) | |
| import tempfile | |
| fd, partial_name = tempfile.mkstemp( | |
| dir=destination.parent, | |
| prefix=f".{destination.stem}.", | |
| suffix=".cfg.partial", | |
| ) | |
| partial = Path(partial_name) | |
| try: | |
| with os.fdopen(fd, "wb") as profile_file: | |
| profile_file.write(raw) | |
| os.replace(partial, destination) | |
| finally: | |
| partial.unlink(missing_ok=True) |
🤖 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_profile.py` around lines 24 - 29, Update the
profile backup flow around the temporary “.cfg.partial” path to create a
randomized temporary file inside destination.parent with owner-only permissions,
avoiding predictable names and symlink races; write the raw configuration to
that securely created file, then atomically replace destination and clean up the
temporary file.
| def probe_device( | ||
| port: str, | ||
| expected_usb: tuple[int | None, int | None, str | None] | None = None, | ||
| output: Callable[[str], None] = lambda _line: None, | ||
| ) -> None: | ||
| _verify_usb(port, expected_usb) | ||
| port = _bootloader_port("auto", port, expected_usb, output, True) | ||
| for command in ("read-mac", "flash-id"): | ||
| _run_esptool("auto", port, [command], output, "no-reset") | ||
| _run_esptool( | ||
| "auto", port, ["get-security-info"], output, "no-reset", "hard-reset" | ||
| ) | ||
| output("Read-only device probe completed.") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the supported platform-to-chip map and probe tests.
set -euo pipefail
rg -n -A10 '_CHIP_BY_PLATFORM\s*=' src/meshchat/services/firmware.py
rg -n -C5 'get-security-info' src testsRepository: hardcoreerik/OrcMesh
Length of output: 2661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- firmware helpers and probe ---'
sed -n '1,90p;250,420p' src/meshchat/services/firmware.py
printf '%s\n' '--- completion reconnect path ---'
rg -n -C8 '_on_firmware_completed|probe_device|hard-reset|_run_esptool' src/meshchat tests/test_firmware_service.py
printf '%s\n' '--- dependency and esptool version declarations ---'
rg -n -C3 'esptool|ESP32|esp32s3|platform' pyproject.toml requirements*.txt setup.cfg setup.py src tests 2>/dev/null || trueRepository: hardcoreerik/OrcMesh
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("src/meshchat/services/firmware.py")
tree = ast.parse(path.read_text())
probe = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "probe_device"
)
calls = []
for node in ast.walk(probe):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name):
if node.func.id == "_run_esptool":
args = [ast.unparse(arg) for arg in node.args]
calls.append((node.lineno, args))
print("probe calls in source order:")
for item in sorted(calls):
print(item)
print("_CHIP_BY_PLATFORM:", next(
ast.literal_eval(node.value)
for node in tree.body
if isinstance(node, ast.Assign)
and any(isinstance(target, ast.Name) and target.id == "_CHIP_BY_PLATFORM"
for target in node.targets)
))
PYRepository: hardcoreerik/OrcMesh
Length of output: 383
🌐 Web query:
esptool 5 get-security-info command supported chips ESP32 original ESP32
💡 Result:
The get-security-info command in esptool is not supported on the original ESP32 [1]. This command was introduced for later Espressif chips (beginning with the ESP32-S2) and is specifically designed to interact with the ROM bootloader's security features [2][1][3]. Official esptool documentation and source code confirm that it is supported on ESP32-S2, ESP32-S3, ESP32-C3, ESP32-C6, and other newer devices, but explicitly excludes the original ESP32 and ESP8266 [4][1][5]. Attempting to run this command on an original ESP32 will result in an error, as the ROM on that chip does not implement the necessary command (0x14) [2][6].
Citations:
- 1: https://github.com/espressif/esptool/blob/e132f6f7/test/test_esptool.py
- 2: https://github.com/espressif/esptool/blob/4698b396730b23fb4aab023c5fb1744db957fc4c/esptool.py
- 3: https://github.com/espressif/esptool/blob/28b65d6e5ba6eb90ea5e7dd7eccd69c6dfd90c36/esptool.py
- 4: https://github.com/espressif/esptool/blob/v5.0.2/esptool/cmds.py
- 5: https://github.com/espressif/esptool/blob/master/esptool/cmds.py
- 6: https://github.com/espressif/esptool/blob/e132f6f7/esptool/cmds.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
tree = ast.parse(Path("src/meshchat/services/firmware.py").read_text())
probe = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "probe_device"
)
calls = []
for node in ast.walk(probe):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name):
if node.func.id == "_run_esptool":
calls.append((node.lineno, [ast.unparse(arg) for arg in node.args]))
print("probe calls in source order:")
for item in sorted(calls):
print(item)
mapping = next(
ast.literal_eval(node.value)
for node in tree.body
if isinstance(node, ast.Assign)
and any(
isinstance(target, ast.Name) and target.id == "_CHIP_BY_PLATFORM"
for target in node.targets
)
)
print("_CHIP_BY_PLATFORM:", mapping)
PYRepository: hardcoreerik/OrcMesh
Length of output: 383
Always reset the radio after the probe
get-security-info is unsupported on the original ESP32. Because --chip auto allows this path, _run_esptool raises FirmwareError before the configured hard-reset, leaving the radio in download mode. Treat this command as optional and put the final reset in a finally block.
🤖 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 398 - 410, Update
probe_device so get-security-info is optional for unsupported chips, allowing
FirmwareError from that command without aborting the probe. Ensure the radio
reset is always performed by moving the final hard-reset invocation into a
finally block, while preserving the existing read-mac and flash-id commands and
completion output.
| def _save_text(self, title: str, suggested: str, text: str) -> None: | ||
| destination, _ = QFileDialog.getSaveFileName(self, title, suggested, "Text files (*.txt)") | ||
| if destination: | ||
| if not destination.lower().endswith(".txt"): | ||
| destination += ".txt" | ||
| with open(destination, "w", encoding="utf-8") as output: | ||
| output.write(text) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle OSError when the report file is written.
_save_text calls open() without error handling. If the selected path is not writable, if the disk is full, or if the file is locked, OSError propagates out of a Qt slot. PySide6 then reports an unhandled exception, and the user sees no explanation. MainWindow._export_nodes in src/meshchat/ui/main_window.py already catches OSError and shows a warning; apply the same handling here.
The static analysis path-traversal hint does not apply. The path comes from a local QFileDialog choice, not from a request.
🛡️ Proposed fix
def _save_text(self, title: str, suggested: str, text: str) -> None:
destination, _ = QFileDialog.getSaveFileName(self, title, suggested, "Text files (*.txt)")
- if destination:
- if not destination.lower().endswith(".txt"):
- destination += ".txt"
- with open(destination, "w", encoding="utf-8") as output:
- output.write(text)
+ if not destination:
+ return
+ if not destination.lower().endswith(".txt"):
+ destination += ".txt"
+ try:
+ with open(destination, "w", encoding="utf-8") as output:
+ output.write(text)
+ except OSError as exc:
+ QMessageBox.warning(self, title, f"Could not write the file:\n{exc}")📝 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 _save_text(self, title: str, suggested: str, text: str) -> None: | |
| destination, _ = QFileDialog.getSaveFileName(self, title, suggested, "Text files (*.txt)") | |
| if destination: | |
| if not destination.lower().endswith(".txt"): | |
| destination += ".txt" | |
| with open(destination, "w", encoding="utf-8") as output: | |
| output.write(text) | |
| def _save_text(self, title: str, suggested: str, text: str) -> None: | |
| destination, _ = QFileDialog.getSaveFileName(self, title, suggested, "Text files (*.txt)") | |
| if not destination: | |
| return | |
| if not destination.lower().endswith(".txt"): | |
| destination += ".txt" | |
| try: | |
| with open(destination, "w", encoding="utf-8") as output: | |
| output.write(text) | |
| except OSError as exc: | |
| QMessageBox.warning(self, title, f"Could not write the file:\n{exc}") |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 506-506: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(destination, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 502 - 508, Update
_save_text to catch OSError around opening and writing the selected report file,
and show the same user-facing warning used by MainWindow._export_nodes. Keep the
existing filename extension handling and successful-write behavior unchanged.
Source: Linters/SAST tools
| def _serial_error(self, error) -> None: | ||
| if error != QSerialPort.SerialPortError.NoError and self._serial.isOpen(): | ||
| self._serial_log.append(f"\nSERIAL ERROR: {self._serial.errorString()}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report serial errors even after Qt closes the port.
_serial_error only appends a message when self._serial.isOpen() is true. When the radio is unplugged, Qt reports ResourceError and can close the device before the slot runs. The user then sees no message, and the console stays in the started state because _console_stop remains enabled and _console_start remains disabled. Report every non-NoError value, and reset the console state on a fatal error.
🐛 Proposed fix
def _serial_error(self, error) -> None:
- if error != QSerialPort.SerialPortError.NoError and self._serial.isOpen():
- self._serial_log.append(f"\nSERIAL ERROR: {self._serial.errorString()}")
+ if error == QSerialPort.SerialPortError.NoError:
+ return
+ self._serial_log.append(f"\nSERIAL ERROR: {self._serial.errorString()}")
+ if error == QSerialPort.SerialPortError.ResourceError:
+ self._stop_serial_console()📝 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 _serial_error(self, error) -> None: | |
| if error != QSerialPort.SerialPortError.NoError and self._serial.isOpen(): | |
| self._serial_log.append(f"\nSERIAL ERROR: {self._serial.errorString()}") | |
| def _serial_error(self, error) -> None: | |
| if error == QSerialPort.SerialPortError.NoError: | |
| return | |
| self._serial_log.append(f"\nSERIAL ERROR: {self._serial.errorString()}") | |
| if error == QSerialPort.SerialPortError.ResourceError: | |
| self._stop_serial_console() |
🤖 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 551 - 553, Update
_serial_error to report every error other than NoError without requiring
self._serial.isOpen(). For fatal serial errors such as ResourceError, also reset
the console controls by enabling _console_start and disabling _console_stop,
matching the stopped state.
| 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"]) | ||
| prefix = [ | ||
| "--verbose", "--chip", "esp32s3", "--port", "COM8", "--baud", "115200", | ||
| "--before", "default-reset", "--after", "no-reset", | ||
| ] | ||
| for command in ( | ||
| "chip-id", "read-mac", "flash-id", "get-security-info", | ||
| "read-flash", "erase-flash", "write-flash", | ||
| ): | ||
| esptool.main([*prefix, command, "--help"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does esptool 5.0 main() raise SystemExit when passed --help?
💡 Result:
Yes, esptool 5.0's main function raises a SystemExit exception when passed the --help argument [1][2]. In esptool 5.0, the main function processes command-line arguments using the argparse library [2]. By default, argparse handles --help by printing the help message and calling sys.exit [2]. In the esptool codebase, this behavior propagates as a SystemExit exception [1]. Because of this, invoking esptool.main(['--help']) within a Python script will terminate the parent process unless the exception is explicitly caught by the caller [3][1]. This behavior has been a known point of frustration for users integrating esptool programmatically, leading to pull requests seeking to prevent this exit [3]. While some users have requested changes to stop this process exit, it remains standard behavior in the 5.0 release [3].
Citations:
- 1: https://github.com/espressif/esptool/blob/master/esptool/__init__.py
- 2: https://github.com/espressif/esptool/blob/749d1adaf55f574cbbc241ca9a10c56cc34b5bd0/esptool/__init__.py
- 3: fix: stop exit 0 when being called programmatically espressif/esptool#1108
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(tests/test_firmware_service\.py|.*firmware.*service.*|.*esptool.*)$' || true
printf '%s\n' '--- relevant symbols and references ---'
rg -n -C 5 'def test_installed_esptool_accepts_commands_used_by_flasher|esptool\.main|SystemExit|_run_esptool|version\("esptool"\)' . --glob '!node_modules' --glob '!build' --glob '!dist'Repository: hardcoreerik/OrcMesh
Length of output: 10735
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- esptool dependency constraints ---'
rg -n -C 3 'esptool|pytest' pyproject.toml requirements*.txt setup.cfg tox.ini .github 2>/dev/null || true
printf '%s\n' '--- relevant implementation ---'
sed -n '341,372p' src/meshchat/services/firmware.py
printf '%s\n' '--- test imports and nearby fixtures ---'
sed -n '1,35p' tests/test_firmware_service.py
sed -n '345,366p' tests/test_firmware_service.py
printf '%s\n' '--- standalone argparse behavior probe ---'
python3 - <<'PY'
import argparse
import sys
parser = argparse.ArgumentParser(prog="esptool")
parser.add_argument("--verbose", action="store_true")
try:
parser.parse_args(["--help"])
except SystemExit as exc:
print(f"argparse --help raises SystemExit with code={exc.code!r}")
print(f"SystemExit subclasses BaseException={isinstance(exc, BaseException)!r}")
PYRepository: hardcoreerik/OrcMesh
Length of output: 5909
Catch SystemExit from esptool help commands
esptool.main(..., "--help") raises SystemExit(0). Wrap each call with pytest.raises(SystemExit) and assert that the exit code is 0 or None.
🤖 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 351 - 362, Update
test_installed_esptool_accepts_commands_used_by_flasher so each esptool.main
help invocation is wrapped in pytest.raises(SystemExit), asserting the captured
exit code is 0 or None while preserving the existing command coverage and
arguments.
1 participant
Summary
Safety
Verification
.venv\\Scripts\\python.exe -m pytest -q-> 441 passed.venv\\Scripts\\python.exe -m ruff check src tests packaging-> passed.venv\\Scripts\\python.exe -m mypy src-> no issues in 68 source filesOrcMesh.exe,Qt6SerialPort.dll, and ESP32-S3 esptool stub verified presentSummary by CodeRabbit
New Features
Bug Fixes