fixed ADA light Serial conditions for USB CDC (some ESP32 C3 / S2 / S3 ) #4792
Conversation
WalkthroughAdded 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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (1)
wled00/wled_serial.cpp (1)
72-74: LGTM! Logic simplification aligns with USB CDC fix.The removal of the runtime
Serialcheck is correct sinceserialCanRXis now properly set during setup based on board type. This fixes the USB CDC issue for ESP32 C3/S2/S3.Minor style nitpick: remove the semicolon after the closing brace.
- if (serialCanRX == false) { - return; - }; // arduino docs: `if (Serial)` indicates whether or not the USB CDC serial connection is open. For all non-USB CDC ports, this will always return true + if (serialCanRX == false) { + return; + } // arduino docs: `if (Serial)` indicates whether or not the USB CDC serial connection is open. For all non-USB CDC ports, this will always return true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/wled.cpp(1 hunks)wled00/wled_serial.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
wled00/wled_serial.cpp (1)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
wled00/wled.cpp (4)
Learnt from: blazoncek
PR: #4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize)) is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: blazoncek
PR: #4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Learnt from: KrX3D
PR: #4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: blazoncek
PR: #4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
🔇 Additional comments (1)
wled00/wled.cpp (1)
456-464: Excellent fix for USB CDC boards!This change correctly addresses the root cause by bypassing pin allocation checks for USB CDC boards. The logic is sound:
- USB CDC boards use D+/D- pins for serial, not GPIO 1/3
- Pin allocator incorrectly reports these as unavailable
- Forcing
serialCanRXandserialCanTXtotruebypasses this issue- Maintains backward compatibility for non-USB CDC boards
The clear comment explains the reasoning well.
wled00/wled_serial.cpp
Outdated
| void handleSerial() | ||
| { | ||
| if (!(serialCanRX && Serial)) return; // arduino docs: `if (Serial)` indicates whether or not the USB CDC serial connection is open. For all non-USB CDC ports, this will always return true | ||
| if (serialCanRX == false && Serial) { |
There was a problem hiding this comment.
Hi @rommo911, could it be that your proposed change does not lead to the same behaviour when not having USB-CDC ?
old condition truth table:
| canRX | Serial | result |
|---|---|---|
| false | - | true = return |
| true | false | true = return |
| true | true | false = continue |
new condition truth table:
| canRX | Serial | result |
|---|---|---|
| false | true | true = return |
| false | false | false = continue |
| true | - | false = continue |
As the new condition you propose seems to be broken - how about just removing the "early exit" for the case of USB_CDC ?
#if ARDUINO_USB_CDC_ON_BOOT
if (!Serial) return;
#else
if (!(serialCanRX && Serial)) return;
#endif@blazoncek not sure if serialCanRX is used in other places in the code, so we still need the change in wled.cpp?
There was a problem hiding this comment.
serialCanRX is only used to determine if ADA/Improv can be used. Nothing else AFAIK.
softhack007
changed the title
@coderabbitai it seems that rabbits are not good at logic ;-) |
|
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: New condition: if (serialCanRX == false && Serial) { return; }Returns early if: Truth table showing the difference:
The issue: When 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. (| |) 🧠 Learnings used |
|
I do not know if forcing serialCanRX or serialCanTX to true is ok. It does not seem so to me. |
netmindz
added
bug
connectivity
4 participants
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