WLED icon indicating copy to clipboard operation
WLED copied to clipboard

fixed ADA light Serial conditions for USB CDC (some ESP32 C3 / S2 / S3 )

Open rommo911 opened this issue 7 months ago β€’ 4 comments

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.

rommo911 avatar Jul 28 '25 22:07 rommo911

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.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Jul 28 '25 22:07 coderabbitai[bot]

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.

softhack007 avatar Nov 07 '25 21:11 softhack007

[!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.

coderabbitai[bot] avatar Nov 07 '25 21:11 coderabbitai[bot]

I do not know if forcing serialCanRX or serialCanTX to true is ok. It does not seem so to me.

blazoncek avatar Nov 07 '25 21:11 blazoncek