refactor USB detection and support for ESP32-S31 - #82
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ESP32‑S31 end-to-end: CLI flag and detection routing, write-flash configuration, ROM-class implementation and fast-paths, stub/firmware image wiring, and a stub JSON file. ChangesESP32-S31 chip support
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant detect_chip
participant ESP32S31ROM
participant LoadFirmwareImage
User->>CLI: run with --esp32s31
CLI->>detect_chip: call detect_chip(port, force_esp32s31=True,...)
detect_chip->>ESP32S31ROM: select ESP32S31ROM as loader
CLI->>LoadFirmwareImage: request bootloader image for esp32s31
LoadFirmwareImage->>ESP32S31ROM: construct ESP32S31FirmwareImage / stub wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@esp_flasher/__main__.py`:
- Line 37: The new CLI flag args.esp32s31 is never passed into chip detection;
update the calls to detect_chip(...) that currently only pass args.esp8266 and
args.esp32 to also pass args.esp32s31 so the flag affects detection, e.g.,
change detect_chip(args.esp8266, args.esp32) -> detect_chip(args.esp8266,
args.esp32, args.esp32s31) (and any other detect_chip invocations), and ensure
the detect_chip implementation signature and internal logic accept and use the
esp32s31 parameter (or add handling code) so the flag is wired end-to-end.
In `@esp_flasher/own_esptool.py`:
- Around line 3584-3588: The hard_reset implementation for ESP32S31ROM calls an
undefined self.watchdog_reset(), causing AttributeError; replace that call with
a valid reset path—either implement a watchdog_reset method on ESP32S31ROM (or
its base class) with the correct USB hard-reset sequence, or change the branch
to call the existing ESPLoader.hard_reset(self) (or another already-defined USB
reset helper) when uses_usb() is true and secure_download_mode is False so the
code uses a defined reset method instead of self.watchdog_reset().
🪄 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: 13d563cb-e2a6-4d0c-833e-4782241b08ee
📒 Files selected for processing (4)
esp_flasher/__main__.pyesp_flasher/common.pyesp_flasher/own_esptool.pyesp_flasher/stubs/esp32s31.json
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
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 `@esp_flasher/own_esptool.py`:
- Around line 3362-3446: ESP32S31ROM is missing S31-specific USB/JTAG register
constants and it overrides _post_connect() without calling the parent, causing
USB detection and watchdog disabling to use C5 values and be skipped; add
S31-specific constants (e.g. UARTDEV_BUF_NO, USB selector values used by
uses_usb()/uses_usb_jtag_serial(), and the S31 watchdog registers/keys that the
ESP32C5ROM helpers expect such as
DR_REG_LP_WDT_BASE/RTC_CNTL_WDTCONFIG0_REG/RTC_CNTL_WDTWPROTECT_REG/RTC_CNTL_WDT_WKEY
values), update hard_reset()/uses_usb_jtag_serial() usage to reference these new
S31 constants instead of C5 addresses, and modify _post_connect() in ESP32S31ROM
to call super()._post_connect() (then perform any S31-specific steps) so the
parent watchdog-disabling logic runs.
🪄 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: b049f60e-83f1-4db9-a535-bf46362ae571
📒 Files selected for processing (4)
esp_flasher/__main__.pyesp_flasher/common.pyesp_flasher/own_esptool.pyesp_flasher/stubs/esp32s31.json
There was a problem hiding this comment.
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 `@esp_flasher/own_esptool.py`:
- Around line 3432-3434: The code added UARTDEV_BUF_NO_USB_JTAG_SERIAL but still
sends all non-OTG resets through ESPLoader.hard_reset(), which bypasses the RTS
settling in ESP32C5ROM.hard_reset(); modify the reset dispatch so that when the
selected USB buffer equals UARTDEV_BUF_NO_USB_JTAG_SERIAL you call
ESP32C5ROM.hard_reset() (or the equivalent S31-specific hard_reset flow) instead
of ESPLoader.hard_reset(), keeping the existing branch for UARTDEV_BUF_NO_USB
(OTG) and the default non-USB path unchanged; reference
UARTDEV_BUF_NO_USB_JTAG_SERIAL, UARTDEV_BUF_NO_USB, ESPLoader.hard_reset, and
ESP32C5ROM.hard_reset to locate and update the conditional.
🪄 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: e8e562d9-eb47-4df9-9425-0672acc4f805
📒 Files selected for processing (1)
esp_flasher/own_esptool.py
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
esp_flasher/own_esptool.py (1)
3579-3582:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall the parent
_post_connect()hook.This override skips
ESP32C5ROM._post_connect(), so the inherited watchdog-disabling flow never runs on S31. On USB-JTAG/Serial connections that can let the RTC/SWD watchdog reset the board mid-flash.🛠️ Suggested fix
def _post_connect(self): + super()._post_connect() if self.uses_usb(): self.ESP_RAM_BLOCK = self.USB_RAM_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 `@esp_flasher/own_esptool.py` around lines 3579 - 3582, The override of _post_connect currently skips the parent watchdog-disabling flow; update the method to call the parent hook (e.g. super()._post_connect() or ESP32C5ROM._post_connect(self)) and then preserve the USB-specific behavior by still setting self.ESP_RAM_BLOCK = self.USB_RAM_BLOCK when self.uses_usb() is true so the inherited watchdog disable runs for S31 and USB-JTAG/Serial connections before adjusting the RAM block.
🤖 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.
Duplicate comments:
In `@esp_flasher/own_esptool.py`:
- Around line 3579-3582: The override of _post_connect currently skips the
parent watchdog-disabling flow; update the method to call the parent hook (e.g.
super()._post_connect() or ESP32C5ROM._post_connect(self)) and then preserve the
USB-specific behavior by still setting self.ESP_RAM_BLOCK = self.USB_RAM_BLOCK
when self.uses_usb() is true so the inherited watchdog disable runs for S31 and
USB-JTAG/Serial connections before adjusting the RAM block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40e2896e-b9f0-4d74-8e62-0737c0dc4a43
📒 Files selected for processing (4)
esp_flasher/__main__.pyesp_flasher/common.pyesp_flasher/own_esptool.pyesp_flasher/stubs/esp32s31.json
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
esp_flasher/own_esptool.py (1)
3432-3434:⚠️ Potential issue | 🔴 CriticalDefine the S31
UARTDEV_BUF_NOregister before reusing the inherited USB helpers.Line 3595, Line 3608, and Line 4267 still call
uses_usb(), butESP32S31ROMnever overridesUARTDEV_BUF_NO. The note on Lines 3580-3582 already says the inherited C5 address bus-faults on S31, so connect/reset/stub-init can still fail immediately on the first USB check. Add the verified S31UARTDEV_BUF_NOaddress, or overrideuses_usb()/uses_usb_jtag_serial()to avoid dereferencing the inherited register until that address is known.Also applies to: 3580-3597, 3607-3611, 4267-4269
🤖 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 `@esp_flasher/own_esptool.py` around lines 3432 - 3434, The ESP32S31ROM subclass is calling inherited USB helpers that reference UARTDEV_BUF_NO_USB / UARTDEV_BUF_NO_USB_JTAG_SERIAL before the S31-specific register is defined; either add the verified S31 addresses for UARTDEV_BUF_NO_USB and UARTDEV_BUF_NO_USB_JTAG_SERIAL in the S31 definitions (so inherited uses_usb()/uses_usb_jtag_serial() can safely read them) or explicitly override uses_usb() and uses_usb_jtag_serial() in ESP32S31ROM to return False (or otherwise avoid dereferencing the inherited register) until the correct S31 UARTDEV_BUF_NO_* addresses are known; update the definitions or overrides referenced by the symbols ESP32S31ROM, UARTDEV_BUF_NO_USB, UARTDEV_BUF_NO_USB_JTAG_SERIAL, uses_usb(), and uses_usb_jtag_serial().
🤖 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.
Duplicate comments:
In `@esp_flasher/own_esptool.py`:
- Around line 3432-3434: The ESP32S31ROM subclass is calling inherited USB
helpers that reference UARTDEV_BUF_NO_USB / UARTDEV_BUF_NO_USB_JTAG_SERIAL
before the S31-specific register is defined; either add the verified S31
addresses for UARTDEV_BUF_NO_USB and UARTDEV_BUF_NO_USB_JTAG_SERIAL in the S31
definitions (so inherited uses_usb()/uses_usb_jtag_serial() can safely read
them) or explicitly override uses_usb() and uses_usb_jtag_serial() in
ESP32S31ROM to return False (or otherwise avoid dereferencing the inherited
register) until the correct S31 UARTDEV_BUF_NO_* addresses are known; update the
definitions or overrides referenced by the symbols ESP32S31ROM,
UARTDEV_BUF_NO_USB, UARTDEV_BUF_NO_USB_JTAG_SERIAL, uses_usb(), and
uses_usb_jtag_serial().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a079ca6-032c-492a-b8d5-c3d8efaf9459
📒 Files selected for processing (4)
esp_flasher/__main__.pyesp_flasher/common.pyesp_flasher/own_esptool.pyesp_flasher/stubs/esp32s31.json
Jason2866
changed the title
There was a problem hiding this comment.
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 `@esp_flasher/own_esptool.py`:
- Around line 4258-4260: When rom_loader.uses_usb_otg() is true the code
currently sets self.ESP_RAM_BLOCK = self.USB_RAM_BLOCK but leaves
FLASH_WRITE_SIZE at its default (0x4000), causing write_flash() to chunk stub
uploads at FLASH_WRITE_SIZE while the transport can only handle USB_RAM_BLOCK;
clamp both values on the OTG path by setting self.ESP_RAM_BLOCK =
self.USB_RAM_BLOCK and also reduce FLASH_WRITE_SIZE to at most
self.USB_RAM_BLOCK (e.g. FLASH_WRITE_SIZE = min(FLASH_WRITE_SIZE,
self.USB_RAM_BLOCK)) so write_flash() and stub transfers use the same USB block
size; apply the same change at the other occurrence mentioned (the second
rom_loader.uses_usb_otg() block).
🪄 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: e404fd6f-b856-43f3-a173-129742ebb0d4
📒 Files selected for processing (1)
esp_flasher/own_esptool.py
Owner
Author
|
@coderabbitai is the refactoring to use VID/PID to detect USB mode solid? Is the ESP32-S31 support correct implemented? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13224 🏁 Script executed: Length of output: 37815 🏁 Script executed: Length of output: 3007
1. VID/PID USB Mode Detection — Is it solid?Yes, the refactoring is solid. The VID/PID logic lives in the shared
All three downstream consumers in
One item to verify: 2. ESP32-S31 Implementation — Correct?Overall yes, the implementation follows the established patterns correctly. A few specific checks: ✅ Correct:
# ESP32S31ROM
def get_flash_voltage(self):
pass # implicitly returns None
# ESP32P4ROM (more explicit)
def get_flash_voltage(self):
raise NotSupportedError(self, "Reading flash voltage")
def get_flash_voltage(self):
raise NotSupportedError(self, "Reading flash voltage")Summary: The implementation is functionally complete and consistent with how other newer ESP32 chips (C5, P4) are handled. The main thing to double-check is whether |
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
Refactor