Always allow proper serial console on S3/C3/C6/etc under ALL conditions#330
Always allow proper serial console on S3/C3/C6/etc under ALL conditions#330troyhacks wants to merge 5 commits intoMoonModules:mdevfrom
Conversation
📝 WalkthroughWalkthroughExpands ESP32 target guards and mutual-exclusion checks to include ESP32C6; adds a CDC-on-boot Serial initialization path (Serial.begin(115200) then Serial.setTxTimeoutMs(0)) for ESP32S3/ESP32C3/ESP32C6 (and IDF equivalents); and widens runtime USB-CDC gating to those targets. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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: 1
🤖 Fix all issues with AI agents
In `@wled00/wled.cpp`:
- Around line 470-485: Remove the critical section around
Serial.begin/Serial.setTxTimeoutMs: delete the portMUX_TYPE mux =
portMUX_INITIALIZER_UNLOCKED; portENTER_CRITICAL(&mux); and
portEXIT_CRITICAL(&mux) calls so Serial.begin(115200) and
Serial.setTxTimeoutMs(0) run normally (setup() is single-threaded); if true
exclusivity is required later, replace that pattern with a FreeRTOS recursive
mutex rather than disabling interrupts. Ensure the `#if` branch that currently
contains portENTER_CRITICAL/portEXIT_CRITICAL is simplified to just call
Serial.begin(115200) and Serial.setTxTimeoutMs(0).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/wled.cpp`:
- Around line 470-475: The build validation currently enumerates allowed
CONFIG_IDF_TARGET_* macros but omits CONFIG_IDF_TARGET_ESP32C6, causing C6
builds to hit the "No supported CONFIG_IDF_TARGET was defined" error; update the
sanity-check macros and any mutual-exclusion guards to include
CONFIG_IDF_TARGET_ESP32C6 (add it alongside CONFIG_IDF_TARGET_ESP32,
CONFIG_IDF_TARGET_ESP32S3, CONFIG_IDF_TARGET_ESP32S2, CONFIG_IDF_TARGET_ESP32C3)
so that the checks that reference these macros (the CONFIG_IDF_TARGET_*
validation block and any `#if/`#elif exclusivity guards) accept ESP32C6 builds and
keep existing behavior for other targets.
|
@troyhacks "/etc" = P4 included? Because i'd like to fix this **** once and for all :-) |
The P4 doesn't have this problem. It has real USB ports for USB things. But in the case of that one Olimex P4 board with a single USB-C port, yes, I think it would fix that too. |
|
It works so well I forgot and built an S3 without it, and it immediately stopped when I disconnected the serial console. It can indeed miss some characters and sometimes looks a bit garbled - but it freakin' works. |
| #endif | ||
| #if ARDUINO_USB_CDC_ON_BOOT || ARDUINO_USB_MODE | ||
| #if ARDUINO_USB_CDC_ON_BOOT && (defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6)) | ||
| // WLEDMM avoid "hung devices" when USB_CDC is enabled; see https://github.com/espressif/arduino-esp32/issues/9043 |
There was a problem hiding this comment.
@troyhacks you deleted the comments intentionally? or was this just a side-effect of AI work?
minor updates - added ESP32-P4 to USB-CDC whitelist - restored lost comment
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
Ok, looks good to me.
@troyhacks If you don't stop me in the next 8 hours, i'll merge to mdev 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/wled.cpp`:
- Around line 43-44: The preprocessor target-validation in wled.cpp omits
CONFIG_IDF_TARGET_ESP32P4 causing valid P4 builds to fail; update the `#if`
condition that checks supported CONFIG_IDF_TARGET_* macros to include
CONFIG_IDF_TARGET_ESP32P4 so the `#error` is not triggered for P4 builds, and
verify related CDC guard usages (references to CONFIG_IDF_TARGET_ESP32P4 in
other files like wled_serial.cpp and the CDC guards around lines 476 and 510)
remain consistent after the change.
🧹 Nitpick comments (3)
wled00/wled.cpp (3)
39-41: Duplicate mutual-exclusion check forCONFIG_IDF_TARGET_ESP32.Lines 39–41 and 50–52 are identical checks. The first one at line 39 appears to be a leftover from before the comprehensive block at lines 49–61 was added. Also, for full symmetry,
CONFIG_IDF_TARGET_ESP32S2is missing its own primary check (though all pairs are still implicitly covered).Proposed cleanup — remove duplicate, add S2 primary
- `#if` defined(CONFIG_IDF_TARGET_ESP32) && ( defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6)) - `#error` please fix your build environment. only one CONFIG_IDF_TARGET may be defined - `#endif` // make sure we have a supported CONFIG_IDF_TARGET_Then in the comprehensive block below (after line 52), add the missing S2 check:
+ `#if` defined(CONFIG_IDF_TARGET_ESP32S2) && ( defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6)) + `#error` please fix your build environment. only one CONFIG_IDF_TARGET may be defined + `#endif`Also applies to: 49-52
29-38: Nit:ARDUINO_ARCH_ESP32C3has no primary mutual-exclusion check.S3, S2, and C6 each have a primary
#if defined(ARDUINO_ARCH_...) && (...)guard, but C3 doesn't. All pairs are still covered implicitly (C3 appears in every other target's "others" list), so this is purely a consistency nit.
510-512:setTxTimeoutMs(0)called a second time — intentional safety net?This is the same call already made at line 478. If the intervening wait/delay logic (lines 483–509) cannot reset the TX timeout, the second call is redundant. If it's intentional to guard against re-initialization during the CDC wait sequence, a brief comment would clarify intent.
2 participants
This seems to put an and to the clusterfuck that is Serial on the ESP32-S3/C3/C6/etc.
You can now always have a serial console available with:
-D ARDUINO_USB_MODE=1 -D ARDUINO_USB_CDC_ON_BOOT=1 -D ARDUINO_USB_MSC_ON_BOOT=0 -D ARDUINO_USB_DFU_ON_BOOT=0With
WLED_DEBUGyou get a small delay as it tries a Serial few times, even if usingWLEDMM_NO_SERIAL_WAIT- but it still comes up under "only power connected"Plug back into your laptop, and you get a serial console and firmware uploading just the same as an ESP32-reggo.
This ends the madness, I hope.
Summary by CodeRabbit
New Feature/Fix
Bug Fixes
Summary by CodeRabbit
New Features
Bug Fixes