fixed ADA light Serial conditions for USB CDC (some ESP32 C3 / S2 / S3 )
after a while searching, i found out that USB Serial CDC is not working for ADA light. it seems there are incompatibility in isPinOK when checking for pins are used in case of pins of native USB. this fixes the issue and now the ada light is working fine.
Summary by CodeRabbit
Release Notes
-
Bug Fixes
- Enhanced USB CDC serial communication handling to properly detect and enable serial capabilities when using USB CDC boot mode, improving serial communication reliability across different hardware configurations.
Walkthrough
Added conditional logic in WLED::setup to set serial capability flags (serialCanRX and serialCanTX) based on ARDUINO_USB_CDC_ON_BOOT definition. When USB CDC boot mode is active, both flags are forced true; otherwise, existing pin-allocation logic is preserved.
Changes
| Cohort / File(s) | Summary |
|---|---|
Serial capability flag initialization wled00/wled.cpp |
Added conditional block to handle serialCanRX and serialCanTX assignment based on USB CDC boot mode configuration. |
Estimated code review effort
π― 2 (Simple) | β±οΈ ~10 minutes
- Verify the conditional logic correctly distinguishes between USB CDC boot and standard pin allocation scenarios
- Confirm that forcing both serial flags to true under USB CDC boot mode is the intended behavior
- Check that the fallback logic for non-USB CDC boards preserves existing RX/TX pin allocation semantics
Pre-merge checks and finishing touches
β Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | β Passed | Check skipped - CodeRabbitβs high-level summary is enabled. |
| Title check | β Passed | The title directly and specifically describes the main change: fixing serial conditions for USB CDC on certain ESP32 variants (C3/S2/S3). |
| Docstring Coverage | β Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
β¨ Finishing touches
- [ ] π Generate docstrings
π§ͺ Generate unit tests (beta)
- [ ] Create PR with unit tests
- [ ] Post copyable unit tests in a comment
π Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π₯ Commits
Reviewing files that changed from the base of the PR and between 8a26210a85b66f5036ad7d405cb006ffdccc6156 and 8a09156b07ce0821fd45cca596c6111a6e758ac2.
π Files selected for processing (1)
-
wled00/wled.cpp(1 hunks)
π§ Files skipped from review as they are similar to previous changes (1)
- wled00/wled.cpp
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 help to get the list of available commands and usage tips.
Additionally, the logic for handling serial input is simplified by modifying the early return condition in the serial handler to focus on RX pin availability when Serial is active.
@coderabbitai it seems that rabbits are not good at logic ;-) You missed that the new "simplified" condition in wled_serial.cpp does not preserve previous behaviour in case that ARDUINO_USB_CDC_ON_BOOT is not defined. So this PR has unwanted side-effects for other configurations.
[!TIP] For best results, initiate chat on the files or code changes.
@softhack007 You're absolutely right, and I apologize for missing this logic error! π
Let me break down why the new condition does NOT preserve the previous behavior:
Previous condition:
if (!(serialCanRX && Serial)) return;
Returns early if: serialCanRX is false OR Serial is false
New condition:
if (serialCanRX == false && Serial) { return; }
Returns early if: serialCanRX is false AND Serial is true
Truth table showing the difference:
| serialCanRX | Serial | Old: !(serialCanRX && Serial) |
New: serialCanRX == false && Serial |
Match? |
|---|---|---|---|---|
| false | false | β Returns | β Does NOT return | β BROKEN |
| false | true | β Returns | β Returns | β |
| true | false | β Returns | β Does NOT return | β BROKEN |
| true | true | Continues | Continues | β |
The issue: When Serial is false (which happens on boards without USB CDC when the serial port isn't connected), the new condition will NOT return early and will proceed to handle serial operations, which is incorrect behavior.
Correct condition to preserve original behavior:
if (!serialCanRX || !Serial) return;
or keep the original:
if (!(serialCanRX && Serial)) return;
This is a critical bug that affects all non-USB-CDC configurations.
/\_/\
( o.o ) *oops*
> ^ <
/| |\
(| |)
π§ Learnings used
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
I do not know if forcing serialCanRX or serialCanTX to true is ok. It does not seem so to me.