Improvements ESP32 CDC/UART detection - #3489
Conversation
- Rework detection for S2 as its the only series with USB-OTG only. - Moved WP_TransportType to it's own header file to allow inclusion from C and C++. - Adjust sdkconfig to use default console to free TinyUSB. - Update S2 defconfig to use only CDC for WP transport. - Disabled graphics and touch for S2 build.
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughESP32 transport handling now uses a shared transport definition, adjusts TinyUSB and fallback initialization, applies runtime USB-JTAG reset behavior, and updates ESP32-S2 configuration for UART console, USB CDC, graphics, and touch support. ChangesESP32 transport integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WP_Initialise
participant TinyUSB
participant UART
WP_Initialise->>TinyUSB: Attempt TinyUSB initialization
TinyUSB-->>WP_Initialise: Select TinyUSB or expose fallback path
WP_Initialise->>UART: Initialize UART when configured
WP_Initialise-->>WP_Initialise: Dispatch I/O using g_WP_Transport
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
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 `@targets/ESP32/_common/WireProtocol_HAL_Interface.c`:
- Around line 411-413: Update the initialization failure path around
WP_Transport and WP_Port_Initialised so a failed transport setup leaves
WP_Port_Initialised false, allowing WP_ReceiveBytes and WP_TransmitMessage to
retry initialization; only set it true after a transport succeeds.
In `@targets/ESP32/_include/WireProtocol_Transport.h`:
- Line 22: Rename the shared transport global from WP_Transport to
g_WP_Transport throughout its declaration, definition, and all references.
Update the exported declaration near WireProtocol_Transport.h:22, the definition
near WireProtocol_HAL_Interface.c:47, and usages in
sys_io_ser_native_System_IO_Ports_SerialPort.cpp and targetHAL_Power.c; preserve
the existing type and behavior.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58043d62-cb80-44e8-9a5b-876b9e299ee7
📒 Files selected for processing (6)
targets/ESP32/_IDF/sdkconfig.default.esp32s2targets/ESP32/_common/WireProtocol_HAL_Interface.ctargets/ESP32/_include/WireProtocol_Transport.htargets/ESP32/_nanoCLR/System.IO.Ports/sys_io_ser_native_System_IO_Ports_SerialPort.cpptargets/ESP32/_nanoCLR/targetHAL_Power.ctargets/ESP32/defconfig/ESP32_S2_defconfig
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist