Conversation
📝 WalkthroughWalkthroughAdds Improv Wi‑Fi provisioning support: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant ImprovDialog
participant ImprovManager
participant SerialPort
participant Device as "Device\n(via Serial)"
User->>MainWindow: Click "Improv WiFi"
MainWindow->>MainWindow: Stop SerialReader thread
MainWindow->>ImprovDialog: Create & show modal dialog
ImprovDialog->>ImprovManager: Init with open serial port
ImprovManager->>ImprovManager: Start background reader thread
ImprovDialog->>ImprovManager: request_current_state()
ImprovManager->>SerialPort: Send CURRENT_STATE RPC packet
SerialPort->>Device: Transmit packet
Device->>SerialPort: Respond with state packet
SerialPort->>ImprovManager: Receive & parse packet
ImprovManager->>ImprovDialog: Emit state_changed
User->>ImprovDialog: Trigger WiFi scan
ImprovDialog->>ImprovManager: request_wifi_networks()
ImprovManager->>SerialPort: Send WiFi scan RPC
Device->>SerialPort: Send network list packets
SerialPort->>ImprovManager: Parse networks
ImprovManager->>ImprovDialog: Emit wifi_networks_received
User->>ImprovDialog: Enter creds & click "Provision"
ImprovDialog->>ImprovManager: send_wifi_settings(ssid,password)
ImprovManager->>SerialPort: Send provisioning RPC (TLV)
Device->>SerialPort: Send provision result
SerialPort->>ImprovManager: Parse result
ImprovManager->>ImprovDialog: Emit provisioned / error
User->>ImprovDialog: Close dialog
ImprovDialog->>ImprovManager: stop()
ImprovManager->>ImprovManager: Stop reader thread
ImprovDialog->>MainWindow: Return control
MainWindow->>MainWindow: Restart SerialReader on same port
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esp_flasher/gui.py`:
- Around line 48-50: The scan button triggers _scan_networks which spawns daemon
threads while ImprovManager.request_wifi_networks reuses the same
accumulator/event pair, allowing results to overlap; fix by preventing
re-entrant scans: in the GUI disable the Scan Networks button (or set a
scan_in_progress flag) at start of _scan_networks and re-enable it when that
scan thread completes, and in esp_flasher/improv.py change request_wifi_networks
to allocate a fresh accumulator and threading.Event (do not reuse any shared
accumulator/event), or protect shared state with a lock and return immediately
if a scan is already running; update references in _scan_networks and
ImprovManager.request_wifi_networks accordingly so each request uses its own
accumulator/event or is serialized.
In `@esp_flasher/improv.py`:
- Around line 143-146: The _rpc_command field is set but never used to match
incoming TYPE_RPC_RESULT packets, so concurrent RPCs (e.g.,
ImprovDialog._on_state_changed() triggering request_device_info() while
send_wifi_settings() is waiting) can race and wake the wrong waiter; change
_send_rpc() and related RPC code paths (references: _rpc_command, _rpc_event,
_rpc_result, _rpc_error, and the _send_rpc method) to serialize RPC calls
(single outstanding RPC) and on receiving TYPE_RPC_RESULT verify the result
correlates to the pending _rpc_command before satisfying the waiter—if the
incoming result does not match, either queue it for the matching command or
ignore it and leave the current waiter blocked; apply the same fix patterns to
the other RPC usage blocks noted (around the sections corresponding to lines
198-215 and 324-349) so only the matching command wakes its waiter.
- Around line 175-188: request_wifi_networks (and _send_rpc used by
request_device_info) can block forever because _write() only logs failures and
callers still wait on events; modify the call sites so write errors are
propagated instead of silently logging: have _write return success/failure (or
raise) and in request_wifi_networks check the result of _write(pkt) and, on
failure, set/notify _wifi_scan_done and return an empty list or raise a specific
exception; similarly update _send_rpc to detect failed writes when sending the
RPC (use the same return/exception behavior) so request_device_info won't block;
reference symbols: request_wifi_networks, _wifi_scan_done, _wifi_networks,
CMD_REQUEST_WIFI_NETWORKS, _send_rpc, and _write.
- Around line 281-285: The else branch incorrectly sets self._is_improv to None
which resumes header scanning immediately; change it to enter “skip-line” mode
by setting self._is_improv = False (keeping self._line = []), so the state
machine will ignore the rest of the current console line until a newline is
seen; update the else block in improv.py where _line and _is_improv are reset
accordingly.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d86c072-1b44-4107-92d8-7d93dd9c8c90
📒 Files selected for processing (3)
esp_flasher/gui.pyesp_flasher/improv.pyesp_flasher/serial_console.py
There was a problem hiding this comment.
♻️ Duplicate comments (4)
esp_flasher/gui.py (1)
48-50:⚠️ Potential issue | 🟠 MajorPrevent overlapping Wi‑Fi scans from the dialog.
Line 196 launches a new scan thread on every click, and scan completion is handled through shared state; rapid re-clicks can interleave results and completion handling.
🛠️ Proposed fix
- scan_btn = QPushButton("Scan Networks") - scan_btn.clicked.connect(self._scan_networks) - net_layout.addWidget(scan_btn) + self.scan_btn = QPushButton("Scan Networks") + self.scan_btn.clicked.connect(self._scan_networks) + net_layout.addWidget(self.scan_btn) + self._scan_in_progress = False @@ def _scan_networks(self): + if self._scan_in_progress: + return + self._scan_in_progress = True + self.scan_btn.setEnabled(False) self.network_list.clear() self.status_label.setText("Scanning WiFi networks...") self.progress.setVisible(True) threading.Thread(target=self._scan_bg, daemon=True).start() @@ def _update_network_list(self, networks): + self._scan_in_progress = False + self.scan_btn.setEnabled(True) self.network_list.clear() self.progress.setVisible(False)Also applies to: 196-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/gui.py` around lines 48 - 50, The scan button currently starts a new thread each click via _scan_networks which can cause overlapping scans and race conditions when updating shared state; modify _scan_networks / the scan worker logic to prevent concurrent scans by tracking a boolean/in-progress flag (e.g., self._scan_in_progress) or using a threading.Lock, disable the scan_btn at start of a scan and re-enable it only in the scan completion handler (e.g., _on_scan_complete) after moving GUI updates to the main thread via signals/slots, and ensure any shared state used to collect results is accessed/updated thread-safely so rapid clicks cannot interleave results.esp_flasher/improv.py (3)
305-309:⚠️ Potential issue | 🟠 MajorKeep skip-line mode after a 9-byte header mismatch.
At Line 308, resetting to
Noneresumes scanning too early; remaining bytes on the same non-Improv line can be misparsed as a packet start.🛠️ Proposed fix
else: # Not an Improv header — discard and go back to scanning # (JS does: isImprov = false, line = []) self._line = [] - self._is_improv = None + self._is_improv = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 305 - 309, The code resets self._is_improv to None after a 9-byte header mismatch, which resumes scanning too early and can misparse remaining bytes; change the reset behavior so that after discarding the tentative header you clear the buffer (self._line = []) but set self._is_improv = False (retain "skip-line" mode) so the parser continues skipping the rest of that non‑Improv line until a newline resets state.
142-147:⚠️ Potential issue | 🟠 MajorSerialize RPC calls and correlate
TYPE_RPC_RESULTwith the pending command.
_rpc_commandis assigned (Line 204) but not enforced when unblocking waiters. Concurrent RPCs can wake the wrong waiter and return mismatched results.🛠️ Proposed fix
class ImprovManager(QObject): @@ self._rpc_event = threading.Event() self._rpc_result = None self._rpc_error = None self._rpc_command = None + self._rpc_call_lock = threading.Lock() + self._rpc_state_lock = threading.Lock() @@ def _send_rpc(self, command, payload=b"", timeout=None): @@ - self._rpc_event.clear() - self._rpc_result = None - self._rpc_error = None - self._rpc_command = command - - pkt = _build_rpc(command, payload) - self._write(pkt) - - if self._rpc_event.wait(timeout=timeout): - if self._rpc_error is not None and self._rpc_error != ERROR_NONE: - return None - return self._rpc_result + with self._rpc_call_lock: + with self._rpc_state_lock: + self._rpc_event.clear() + self._rpc_result = None + self._rpc_error = None + self._rpc_command = command + + pkt = _build_rpc(command, payload) + if not self._write(pkt): + with self._rpc_state_lock: + self._rpc_command = None + return None + + if self._rpc_event.wait(timeout=timeout): + with self._rpc_state_lock: + rpc_error = self._rpc_error + rpc_result = self._rpc_result + self._rpc_command = None + if rpc_error is not None and rpc_error != ERROR_NONE: + return None + return rpc_result + + with self._rpc_state_lock: + self._rpc_command = None self.log_message.emit("RPC timeout") self.error_received.emit(ERROR_TIMEOUT) return None @@ elif msg_type == TYPE_RPC_RESULT: @@ - if command == CMD_REQUEST_INFO: + with self._rpc_state_lock: + pending = self._rpc_command + if pending is not None and command != pending: + return + + if command == CMD_REQUEST_INFO:Also applies to: 198-215, 347-376
161-167:⚠️ Potential issue | 🟠 MajorDon’t wait indefinitely when writes fail or shutdown begins.
Line 217
_write()only logs failures; callers still block on events (Lines 187, 209). Also,stop()(Lines 161-167) does not release those waiters.🛠️ Proposed fix
def stop(self): """Stop the Improv receiver thread.""" self._running = False + self._wifi_scan_done.set() + self._rpc_error = ERROR_TIMEOUT + self._rpc_event.set() if self._thread: self._thread.join(timeout=3.0) self._thread = None @@ def request_wifi_networks(self): @@ self._wifi_networks = [] self._wifi_scan_done.clear() pkt = _build_rpc(CMD_REQUEST_WIFI_NETWORKS) - self._write(pkt) + if not self._write(pkt): + return [] self._wifi_scan_done.wait() return list(self._wifi_networks) @@ def _write(self, data): """Write data to the serial port.""" try: if self._port and self._port.is_open: self._port.write(data) self._port.flush() + return True except Exception as e: logger.error("Improv write error: %s", e) self.log_message.emit(f"Write error: {e}") + return FalseAlso applies to: 180-188, 198-208, 217-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 161 - 167, The _write() method currently only logs failures so callers that wait on Events remain blocked, and stop() only clears _running and joins the thread without releasing those waiters; update _write() to set/notify any Event(s) or condition variables it uses (so callers unblock) when a write fails or when _running is False, and modify stop() to explicitly set/notify those same Event(s) or conditions before joining the thread (and ensure any blocking waits use a timeout or check _running to avoid indefinite blocking). Locate the relevant synchronization primitives referenced by _write() and other waiters and ensure both failure paths and stop() trigger them (names: _write, stop, and the Event/Condition attributes used by the class) so no callers remain stuck.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@esp_flasher/gui.py`:
- Around line 48-50: The scan button currently starts a new thread each click
via _scan_networks which can cause overlapping scans and race conditions when
updating shared state; modify _scan_networks / the scan worker logic to prevent
concurrent scans by tracking a boolean/in-progress flag (e.g.,
self._scan_in_progress) or using a threading.Lock, disable the scan_btn at start
of a scan and re-enable it only in the scan completion handler (e.g.,
_on_scan_complete) after moving GUI updates to the main thread via
signals/slots, and ensure any shared state used to collect results is
accessed/updated thread-safely so rapid clicks cannot interleave results.
In `@esp_flasher/improv.py`:
- Around line 305-309: The code resets self._is_improv to None after a 9-byte
header mismatch, which resumes scanning too early and can misparse remaining
bytes; change the reset behavior so that after discarding the tentative header
you clear the buffer (self._line = []) but set self._is_improv = False (retain
"skip-line" mode) so the parser continues skipping the rest of that non‑Improv
line until a newline resets state.
- Around line 161-167: The _write() method currently only logs failures so
callers that wait on Events remain blocked, and stop() only clears _running and
joins the thread without releasing those waiters; update _write() to set/notify
any Event(s) or condition variables it uses (so callers unblock) when a write
fails or when _running is False, and modify stop() to explicitly set/notify
those same Event(s) or conditions before joining the thread (and ensure any
blocking waits use a timeout or check _running to avoid indefinite blocking).
Locate the relevant synchronization primitives referenced by _write() and other
waiters and ensure both failure paths and stop() trigger them (names: _write,
stop, and the Event/Condition attributes used by the class) so no callers remain
stuck.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f62e0cf6-d7fc-4a68-b26f-a7164342cb4b
📒 Files selected for processing (3)
esp_flasher/gui.pyesp_flasher/improv.pyesp_flasher/serial_console.py
🚧 Files skipped from review as they are similar to previous changes (1)
- esp_flasher/serial_console.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
esp_flasher/gui.py (2)
590-600: Redundant thread join afterstop().The
stop()method already callsself.thread.join(timeout=1.0)internally (peresp_flasher/serial_console.pyLines 43-49). The additional join on Lines 593-594 is redundant and may reference aNonethread sincestop()setsself.thread = None.♻️ Proposed simplification
if self._serial_reader: self._serial_reader.stop() # sets _muted=True and running=False, joins thread - # Verify the thread is truly dead before handing the port to Improv - if self._serial_reader.thread and self._serial_reader.thread.is_alive(): - self._serial_reader.thread.join(timeout=2.0) self._serial_reader.line_received.disconnect(self.append_log_line) self._serial_reader.error_occurred.disconnect(self.handle_serial_error) # Flush any already-queued cross-thread events so they are discarded from PyQt6.QtWidgets import QApplication QApplication.processEvents() self._serial_reader = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/gui.py` around lines 590 - 600, The extra join after calling self._serial_reader.stop() is redundant and unsafe because SerialReader.stop() already joins and sets self.thread = None; remove the block that checks self._serial_reader.thread and calls join(timeout=2.0), and simply call self._serial_reader.stop(), then disconnect the signals (self._serial_reader.line_received.disconnect and self._serial_reader.error_occurred.disconnect) and flush events with QApplication.processEvents() before setting self._serial_reader = None to avoid referencing a None thread.
103-107: Consider narrowing the exception type for serial port operations.The broad
Exceptioncatch is flagged by static analysis (Ruff BLE001). While serial operations can fail in various ways, narrowing to more specific exception types improves debuggability.♻️ Proposed fix
try: self._serial_port.reset_input_buffer() - except Exception as e: + except (OSError, serial.SerialException) as e: self.status_label.setText(f"Port error: {e}") returnThis requires adding
import serialat the top of the file or using a local import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/gui.py` around lines 103 - 107, The try/except around self._serial_port.reset_input_buffer() is catching a broad Exception; narrow it to serial-specific errors by importing the serial module (add `import serial` at top or do a local import) and catch serial.SerialException (and optionally serial.SerialTimeoutException) instead, then update the status_label.setText to include the caught exception message for context while returning. Ensure the exception handler references the same method name self._serial_port.reset_input_buffer() so the change is localized.esp_flasher/improv.py (1)
244-247: Consider logging exceptions when setting serial timeout.The static analysis (Ruff S110) flags the silent
try-except-passpatterns. While these are intentional fallbacks, logging at debug level would aid troubleshooting.♻️ Proposed fix
try: self._port.timeout = 0.1 - except Exception: - pass + except Exception as e: + logger.debug("Could not set serial timeout: %s", e)Apply the same pattern to the finally block (Lines 272-275).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 244 - 247, The silent try/except around setting self._port.timeout should capture the exception as e and log it at debug level instead of passing; update the block that sets self._port.timeout (and the symmetric timeout-reset in the finally block) to use "except Exception as e: logger.debug(...)" (or the module/class logger in this file) so any failure to adjust the serial timeout is recorded for troubleshooting while preserving the fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@esp_flasher/gui.py`:
- Around line 590-600: The extra join after calling self._serial_reader.stop()
is redundant and unsafe because SerialReader.stop() already joins and sets
self.thread = None; remove the block that checks self._serial_reader.thread and
calls join(timeout=2.0), and simply call self._serial_reader.stop(), then
disconnect the signals (self._serial_reader.line_received.disconnect and
self._serial_reader.error_occurred.disconnect) and flush events with
QApplication.processEvents() before setting self._serial_reader = None to avoid
referencing a None thread.
- Around line 103-107: The try/except around
self._serial_port.reset_input_buffer() is catching a broad Exception; narrow it
to serial-specific errors by importing the serial module (add `import serial` at
top or do a local import) and catch serial.SerialException (and optionally
serial.SerialTimeoutException) instead, then update the status_label.setText to
include the caught exception message for context while returning. Ensure the
exception handler references the same method name
self._serial_port.reset_input_buffer() so the change is localized.
In `@esp_flasher/improv.py`:
- Around line 244-247: The silent try/except around setting self._port.timeout
should capture the exception as e and log it at debug level instead of passing;
update the block that sets self._port.timeout (and the symmetric timeout-reset
in the finally block) to use "except Exception as e: logger.debug(...)" (or the
module/class logger in this file) so any failure to adjust the serial timeout is
recorded for troubleshooting while preserving the fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 732d592a-fb99-4a3b-8e11-37edcffd9c38
📒 Files selected for processing (2)
esp_flasher/gui.pyesp_flasher/improv.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
esp_flasher/improv.py (1)
365-389:⚠️ Potential issue | 🟠 MajorDon’t drop Wi‑Fi scan packets behind the global RPC mismatch guard.
Lines 372-377 return early for any
TYPE_RPC_RESULTwhose command does not equal_rpc_command. Wi-Fi scan results use the same packet type, so a concurrentCMD_REQUEST_INFOorCMD_SEND_WIFI_SETTINGSmakes the scan stream look “mismatched” andrequest_wifi_networks()can sit until timeout.ImprovDialog._on_state_changed()auto-requests device info, so this is reachable in the current GUI flow.Patch suggestion
elif msg_type == TYPE_RPC_RESULT: if not data: return command = data[0] strings = _parse_tlv_strings(data) + if command == CMD_REQUEST_WIFI_NETWORKS: + if not strings: + self.log_message.emit( + f"WiFi scan complete: {len(self._wifi_networks)} networks" + ) + self._wifi_scan_done.set() + elif len(strings) >= 3: + try: + rssi = int(strings[1]) + except (ValueError, IndexError): + rssi = 0 + self._wifi_networks.append( + (strings[0], rssi, strings[2] == "YES") + ) + return + # Verify result matches pending RPC command to prevent race conditions if self._rpc_command is not None and command != self._rpc_command: logger.warning( "Ignoring RPC result for command %d (expected %d)", command, self._rpc_command ) return @@ - elif command == CMD_REQUEST_WIFI_NETWORKS: - if not strings: - # Empty result = scan complete (matches JS: receivedData done) - self.log_message.emit(f"WiFi scan complete: {len(self._wifi_networks)} networks") - self._wifi_scan_done.set() - else: - if len(strings) >= 3: - try: - rssi = int(strings[1]) - except (ValueError, IndexError): - rssi = 0 - self._wifi_networks.append( - (strings[0], rssi, strings[2] == "YES") - ) - elif command == CMD_SEND_WIFI_SETTINGS:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 365 - 389, The early return in the TYPE_RPC_RESULT branch is too aggressive: it ignores any RPC result whose command != self._rpc_command, which drops interleaved Wi‑Fi scan packets; modify the handler in improv.py so CMD_REQUEST_WIFI_NETWORKS is processed regardless of self._rpc_command (i.e., check and handle command == CMD_REQUEST_WIFI_NETWORKS before the global mismatch guard or whitelist it), ensuring you still keep the mismatch warning for other commands; preserve existing behavior of appending to self._wifi_networks, emitting log_message, setting self._wifi_scan_done, and only set self._rpc_result/_rpc_event when command == self._rpc_command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esp_flasher/gui.py`:
- Around line 265-278: The background threads (_request_info_bg, _scan_bg,
_provision_bg) currently dereference self._improv after being started, which can
race with the dialog teardown that clears self._improv; fix this by capturing a
stable reference to self._improv before starting each thread and using that
captured reference inside the worker instead of accessing self._improv directly
(e.g., read improv = self._improv and pass it into the thread via args or wrap
the worker in a closure that closes over improv); apply this pattern to the
Thread starts that invoke _request_info_bg, _scan_bg, and _provision_bg so the
detached workers never read the attribute after it may have been cleared.
- Around line 215-219: The except clause in the reset_input_buffer handling
references serial.SerialException but serial is only imported locally inside
MainWindow.connect_to_port(); add a top-level import for the serial module
(e.g., import serial) at module scope so serial.SerialException is defined when
the except runs, or alternatively import SerialException into the module
namespace and remove the local import in MainWindow.connect_to_port(); update
references accordingly (target symbols: serial.SerialException,
MainWindow.connect_to_port, self._serial_port.reset_input_buffer).
In `@esp_flasher/improv.py`:
- Around line 184-192: The request_wifi_networks caller (and other RPCs like
request_device_info) can block forever if the receiver thread dies because
waiters (_wifi_scan_done and others) are never woken and subsequent RPCs queue
behind a locked _rpc_lock; to fix this, add a shared "receiver_dead" flag/state
that the read/receiver loop sets when it exits on a closed port or read error,
and ensure the receiver exit path also sets this flag and signals/wakes all
pending waiters (e.g., call _wifi_scan_done.set() and any other
condition/events) and releases/notifies any RPC waiters so they immediately
fail; then update request_wifi_networks (and other request_* methods that wait)
to check the receiver_dead flag early and raise a clear exception (e.g.,
IOError/RuntimeError) instead of blocking, and ensure the _rpc_lock acquisition
path respects the receiver_dead state so queued RPCs fail fast.
---
Duplicate comments:
In `@esp_flasher/improv.py`:
- Around line 365-389: The early return in the TYPE_RPC_RESULT branch is too
aggressive: it ignores any RPC result whose command != self._rpc_command, which
drops interleaved Wi‑Fi scan packets; modify the handler in improv.py so
CMD_REQUEST_WIFI_NETWORKS is processed regardless of self._rpc_command (i.e.,
check and handle command == CMD_REQUEST_WIFI_NETWORKS before the global mismatch
guard or whitelist it), ensuring you still keep the mismatch warning for other
commands; preserve existing behavior of appending to self._wifi_networks,
emitting log_message, setting self._wifi_scan_done, and only set
self._rpc_result/_rpc_event when command == self._rpc_command.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d09edcf-6d99-44e8-8aaa-6ae564e59ecb
📒 Files selected for processing (2)
esp_flasher/gui.pyesp_flasher/improv.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
esp_flasher/improv.py (1)
311-314: Consider logging the exception when restoring serial timeout.The
try-except-passpattern here is acceptable for defensive cleanup, but logging could help with debugging rare edge cases.♻️ Optional: Add debug logging
try: self._port.timeout = old_timeout - except Exception: - pass + except Exception as e: + logger.debug("Could not restore serial timeout: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 311 - 314, Replace the silent except in the cleanup that restores self._port.timeout so failures are logged for debugging: change the except to "except Exception as e" and emit a debug/exception log that includes the old_timeout and the caught exception. Use the class logger if available (e.g., self.logger or self._logger), otherwise use the module logger via logging.exception or logging.debug, and preserve the original behavior by not re-raising the exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esp_flasher/gui.py`:
- Around line 329-345: _guard against using a None _improv in network
scan/provision flows by checking self._improv before launching background work:
in _scan_networks (and similarly in _provision) return early and reset UI state
(re-enable scan_btn, hide progress, set status_label) if self._improv is None or
_start_improv failed; do not start the thread calling _scan_bg when self._improv
is None so _scan_bg never receives None and avoids calling
improv.request_wifi_networks(); keep using the _scan_finished signal to update
the UI when a valid improv exists.
---
Nitpick comments:
In `@esp_flasher/improv.py`:
- Around line 311-314: Replace the silent except in the cleanup that restores
self._port.timeout so failures are logged for debugging: change the except to
"except Exception as e" and emit a debug/exception log that includes the
old_timeout and the caught exception. Use the class logger if available (e.g.,
self.logger or self._logger), otherwise use the module logger via
logging.exception or logging.debug, and preserve the original behavior by not
re-raising the exception.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38deaffe-b44a-4815-b946-3e0371798615
📒 Files selected for processing (2)
esp_flasher/gui.pyesp_flasher/improv.py
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
esp_flasher/improv.py (1)
187-201:⚠️ Potential issue | 🟠 MajorWi‑Fi scans still bypass the serialized RPC path.
_handle_packet()now drops anyTYPE_RPC_RESULTwhose command does not match_rpc_command, butrequest_wifi_networks()never acquires_rpc_lockor claims_rpc_command. If the automatic info request is still in flight, scan results are ignored and the scan times out even though the device replied.Also applies to: 403-409, 417-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 187 - 201, request_wifi_networks currently sends the RPC without reserving the serialized RPC slot so responses can be dropped by _handle_packet; fix by acquiring the _rpc_lock, set self._rpc_command = CMD_REQUEST_WIFI_NETWORKS before calling _write(_build_rpc(...)), then use try/finally to clear self._rpc_command and release the lock after the wait (preserving behavior of _wifi_scan_done and _wifi_networks); apply the same locking/preset-then-clear pattern to the other similar scan/info methods referenced (the blocks at the other occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esp_flasher/gui.py`:
- Around line 343-348: Wrap calls to ImprovManager methods (e.g.,
improv.request_wifi_networks(), improv.request_device_info(),
improv.send_wifi_settings()) inside the worker thread functions (_scan_bg and
the other background methods referenced) with try/except that catches exceptions
raised when the receiver dies; on exception, log or record the error and always
emit the UI-reset signal used by the main thread (for _scan_bg emit
self._scan_finished with an empty list or an error sentinel, and in the other
background functions emit their respective completion/reset signals) so the
dialog unblocks and controls are re-enabled even if the Improv call fails.
In `@esp_flasher/improv.py`:
- Around line 182-185: request_device_info (and other auto-probe callers in the
211-247 range) can block indefinitely inside _send_rpc while holding _rpc_lock,
preventing later send_wifi_settings from sending; change _send_rpc (or its
callers) so the lock is only held while accessing the transport to send the
request and register the response waiter, then release _rpc_lock before waiting
for the response (or enforce a bounded timeout and ensure _rpc_lock is released
in a finally block). Update request_device_info to pass a sensible timeout or
rely on the revised _send_rpc behavior so background probes cannot hold
_rpc_lock forever and subsequent send_wifi_settings calls can proceed.
---
Duplicate comments:
In `@esp_flasher/improv.py`:
- Around line 187-201: request_wifi_networks currently sends the RPC without
reserving the serialized RPC slot so responses can be dropped by _handle_packet;
fix by acquiring the _rpc_lock, set self._rpc_command =
CMD_REQUEST_WIFI_NETWORKS before calling _write(_build_rpc(...)), then use
try/finally to clear self._rpc_command and release the lock after the wait
(preserving behavior of _wifi_scan_done and _wifi_networks); apply the same
locking/preset-then-clear pattern to the other similar scan/info methods
referenced (the blocks at the other occurrences).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 013bf24d-085c-4a7b-b704-bbd34a12e806
📒 Files selected for processing (3)
esp_flasher/gui.pyesp_flasher/improv.pyesp_flasher/serial_console.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esp_flasher/gui.py`:
- Around line 350-351: The exception handlers in _scan_bg, _request_info_bg, and
_provision_bg use an undefined logger causing NameError; fix by adding a
module-level logger (e.g., import logging and create logger =
logging.getLogger(__name__)) at top of the module or by replacing those
logger.error(...) calls with print(...) if you prefer no logging infrastructure,
then update the three exception blocks to use that module-level logger
identifier to avoid masking the original exceptions.
In `@esp_flasher/improv.py`:
- Around line 210-212: Remove the duplicate unreachable return statement that
repeats "return list(self._wifi_networks)"; keep the single return and ensure
any preceding side-effects (e.g., setting self._rpc_command = None) remain
before that return. Locate the method using self._rpc_command and
self._wifi_networks, delete the extraneous second "return
list(self._wifi_networks)" so there is only one return at the end of the
function.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd827cd6-06bb-4dfb-9664-2ae8070ee87c
📒 Files selected for processing (2)
esp_flasher/gui.pyesp_flasher/improv.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
esp_flasher/improv.py (1)
194-206:⚠️ Potential issue | 🔴 CriticalDo not release the shared RPC slot before the matching response arrives.
These paths still use one global waiter (
_rpc_command,_rpc_event,_rpc_result,_wifi_networks), but they drop_rpc_lockimmediately after writing. A laterrequest_device_info(),request_wifi_networks(), orsend_wifi_settings()can overwrite that shared state while the first call is still waiting, and_handle_packet()then drops the first response as a mismatch on Lines 420-425 or wakes the wrong waiter.esp_flasher/gui.pynow issues these calls from separate threads, so this race is reachable in normal use.With the current single-slot design, keep the lock held until the response/timeout is consumed, or replace the shared waiter fields with per-request state before allowing overlap.
Also applies to: 230-263, 419-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/improv.py` around lines 194 - 206, The code releases the shared RPC slot (_rpc_lock) immediately after sending a request which allows other threads calling request_device_info(), request_wifi_networks(), or send_wifi_settings() to overwrite shared waiter state (_rpc_command, _rpc_event, _rpc_result, _wifi_networks) before the matching response arrives; either hold _rpc_lock across the send-and-wait sequence (acquire before setting _rpc_command/_rpc_event/_rpc_result, send via _write(pkt), then wait on _rpc_event/_wifi_scan_done and only release the lock after the wait completes or times out) or refactor to allocate per-request waiter state (create a request-local object with its own event/result and pass an identifier in _rpc_command so _handle_packet() can route responses to the correct waiter) and update _handle_packet() to match responses to that per-request state instead of the global fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esp_flasher/gui.py`:
- Around line 67-79: The code inserts unescaped device-supplied strings into
RichText, enabling HTML injection; import the html module at top-level and
replace the direct interpolation of val when constructing the QLabel in the loop
(where self._device_info and labels are used to create info_row) with an escaped
version (use html.escape(val)) so the QLabel receives safe text, then keep
forcing RichText via setTextFormat as before.
---
Duplicate comments:
In `@esp_flasher/improv.py`:
- Around line 194-206: The code releases the shared RPC slot (_rpc_lock)
immediately after sending a request which allows other threads calling
request_device_info(), request_wifi_networks(), or send_wifi_settings() to
overwrite shared waiter state (_rpc_command, _rpc_event, _rpc_result,
_wifi_networks) before the matching response arrives; either hold _rpc_lock
across the send-and-wait sequence (acquire before setting
_rpc_command/_rpc_event/_rpc_result, send via _write(pkt), then wait on
_rpc_event/_wifi_scan_done and only release the lock after the wait completes or
times out) or refactor to allocate per-request waiter state (create a
request-local object with its own event/result and pass an identifier in
_rpc_command so _handle_packet() can route responses to the correct waiter) and
update _handle_packet() to match responses to that per-request state instead of
the global fields.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7fec019-aa51-4b98-84cb-9996d4c1036c
📒 Files selected for processing (2)
esp_flasher/gui.pyesp_flasher/improv.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
esp_flasher/gui.py (1)
764-771: Consider flushing the serial buffer before restarting the console reader.When
ImprovDialogcloses, theImprovManagerreceiver thread may have left unconsumed bytes in the serial buffer. The newSerialReaderwill read these as console output, potentially causing brief garbled text. Per the relevant code snippets,SerialReader.start()does not flush the input buffer.♻️ Proposed fix to drain stale Improv bytes
# Restart console reader on the same open port (like JS reconnectConsole) if self._serial_port and self._serial_port.is_open: + # Drain any leftover Improv bytes from the buffer + try: + self._serial_port.reset_input_buffer() + except (OSError, serial.SerialException): + pass self._serial_reader = SerialReader(self._serial_port) self._serial_reader.line_received.connect(self.append_log_line) self._serial_reader.error_occurred.connect(self.handle_serial_error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esp_flasher/gui.py` around lines 764 - 771, Before starting a new SerialReader, drain any stale bytes left by ImprovManager by flushing the serial input buffer on self._serial_port; call the pyserial method (reset_input_buffer()) (and optionally reset_output_buffer()) on self._serial_port inside a try/except before instantiating SerialReader so the new SerialReader (SerialReader.start) doesn't consume leftover Improv bytes and produce garbled console output; reference self._serial_port and SerialReader when making this change and handle exceptions to avoid crashing the UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@esp_flasher/gui.py`:
- Around line 764-771: Before starting a new SerialReader, drain any stale bytes
left by ImprovManager by flushing the serial input buffer on self._serial_port;
call the pyserial method (reset_input_buffer()) (and optionally
reset_output_buffer()) on self._serial_port inside a try/except before
instantiating SerialReader so the new SerialReader (SerialReader.start) doesn't
consume leftover Improv bytes and produce garbled console output; reference
self._serial_port and SerialReader when making this change and handle exceptions
to avoid crashing the UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ced1204-9fd0-4a2e-9475-926b62d67b59
📒 Files selected for processing (2)
esp_flasher/gui.pyesp_flasher/improv.py
Jason2866
changed the title
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes