Conversation
Cherry-picked from MoonModules/WLED-MM@bb6662ee3. - cfg.cpp, set.cpp, wled.cpp, xml.cpp: persist netDebugPrintIP and netDebugPrintPort in config and settings - net_debug.h/cpp: use stored IP/port instead of compile-time host; init moved to initInterfaces - wled.h: DEBUGOUT/DEBUGOUTLN/DEBUGOUTF/DEBUGOUTFlush macros route to NetDebug or Serial based on netDebugEnabled; USER_PRINT* macros added - data/settings_sync.htm: Net Debug IP/port fields in Sync settings - data/index.js: relabel debug link as 'Net Debug' - bus_manager.h, FX.h: remove local DEBUGOUT object alias; update DEBUGBUS_*/DEBUGFX_* macros to use DEBUGOUT()/DEBUGOUTLN()/DEBUGOUTF() - usermods/mpu6050_imu: update local DEBUG_* redefinitions to match - platformio.ini: retain only this branch's usermods env additions; discard all MM-specific environments - usermods/audioreactive/audio_reactive.cpp: no changes (kept HEAD)
…HOST as guard WLED_DEBUG_HOST was used both as a feature-enable guard and as a compile-time IP/hostname string, which was backwards — you had to supply a host at build time just to enable a feature whose whole point is runtime configuration. Replace all preprocessor guards with WLED_ENABLE_NET_DEBUG, following the existing WLED_ENABLE_* convention. A compat shim in wled.h automatically defines WLED_ENABLE_NET_DEBUG when WLED_DEBUG_HOST is set, so existing build configs continue to work. The string value of WLED_DEBUG_HOST (compile-time default IP) is still honoured where used. Files changed: wled.h, wled.cpp, cfg.cpp, set.cpp, xml.cpp, json.cpp, net_debug.cpp
WalkthroughThis PR refactors the network debug system by replacing the ChangesNetwork Debug System Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
added
enhancement
connectivity
|
While adding new debug macros - how about also adding a debug statement that is always printed, like WLED-MM USER_PRINTxx() ? The idea would be that some errors (unexpected situations like "cannot allocate pin", "file not found", "invalid cfg.json", etc) could always be logged on serial, in case that serial is not allocated to leds or used for other protocols. Or via net debug (UDP) if enabled. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
wled00/net_debug.h (1)
10-10: ⚡ Quick winRemove the stale commented member declaration.
This commented field is dead code now and should be deleted to avoid confusion in the class layout.
♻️ Suggested cleanup
- // IPAddress debugPrintHostIP;As per coding guidelines:
**/*.{cpp,h,hpp,js}: Remove dead/unused code — justify or delete it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/net_debug.h` at line 10, Remove the stale commented member declaration "IPAddress debugPrintHostIP;" from net_debug.h; locate the commented line and delete it so the header no longer contains dead/unused commented field, leaving no other changes to the class layout or surrounding comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/cfg.cpp`:
- Around line 660-666: When deserializing the "ndb" interface, validate the
parsed netDebugPrintPort from interfaces["ndb"] (the if_ndb JsonObject) before
accepting it: ensure the value is within 0..65535 and is non-negative; if it's
out of range or not an integer, either clamp to a safe default (e.g. 0 or
previously configured netDebugPrintPort) and emit a warning, or reject it and
leave netDebugPrintPort unchanged. Update the assignment that currently takes
if_ndb["port"] directly to perform this range check and explicit conversion to
an unsigned 16-bit type so downstream UDP calls use a safe port value.
In `@wled00/data/settings_sync.htm`:
- Line 299: The Net Debug port input (input name="NP") allows values up to 99999
which exceeds the valid UDP port range; update the input element for NP to
constrain values to the valid port range by setting min="1" and max="65535" (and
keep type="number") so the browser enforces only 1..65535 can be submitted.
In `@wled00/set.cpp`:
- Around line 526-532: The net debug block currently unconditionally overwrites
netDebugPrintIP[] and netDebugPrintPort which can reset them to 0 when args are
missing or invalid; change the logic to first check for the presence and
validity of each argument from request (e.g. for each "N"+String(i) call
request->hasArg(...) or test that request->arg(...) is non-empty and numeric)
before assigning netDebugPrintIP[i], and likewise verify request->hasArg("NP")
and that its value is a valid port number before assigning netDebugPrintPort so
existing settings are preserved when args are absent/invalid.
In `@wled00/wled.cpp`:
- Around line 830-855: Wrap usage of WLED_DEBUG_HOST in an `#ifdef`
WLED_DEBUG_HOST block and invert the fromString logic so a successful parse
keeps the configured host IP; specifically, inside the netDebugPrintIP
initialization (symbol: netDebugPrintIP and its method fromString) attempt
netDebugPrintIP.fromString(WLED_DEBUG_HOST) only when WLED_DEBUG_HOST is defined
and if that call returns true do not overwrite netDebugPrintIP, otherwise fall
back to Network.localIP(); when assigning the fallback IP from Network.localIP()
(symbol: Network::localIP or Network.localIP) copy all four octets (0..3) into
netDebugPrintIP so the fourth byte is not left uninitialized; leave the
netDebugPrintPort defaulting logic as-is but ensure the
WLED_DEBUG_HOST-dependent code is guarded by `#ifdef/`#endif.
In `@wled00/wled.h`:
- Around line 990-998: The DEBUGOUT macros were changed to function-style calls
which breaks sites expecting a stream object (e.g., DEBUGSR_PRINT macros that
call DEBUGOUT.print()/println()/printf() and server.printStatus(DEBUGOUT));
restore stream-style usage by adding a helper that returns a Print& (e.g.,
implement inline Print& debugOutStream() that returns NetDebug or Serial based
on netDebugEnabled and build flag), expose a macro like DEBUGOUT_STREAM() that
expands to debugOutStream(), and then redefine
DEBUGOUT/DEBUGOUTLN/DEBUGOUTF/DEBUGOUTFlush to call
DEBUGOUT_STREAM().print/println/printf/flush while leaving DEBUGOUT_STREAM()
available for passing into functions such as printStatus and the
DEBUGSR_PRINT/DEBUGSR_PRINTLN/DEBUGSR_PRINTF sites so both function-call and
stream-object patterns work.
---
Nitpick comments:
In `@wled00/net_debug.h`:
- Line 10: Remove the stale commented member declaration "IPAddress
debugPrintHostIP;" from net_debug.h; locate the commented line and delete it so
the header no longer contains dead/unused commented field, leaving no other
changes to the class layout or surrounding comments.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f31603f6-4021-4a4d-8dad-1c9e995dba40
📒 Files selected for processing (13)
usermods/mpu6050_imu/mpu6050_imu.cppwled00/FX.hwled00/bus_manager.hwled00/cfg.cppwled00/data/index.jswled00/data/settings_sync.htmwled00/json.cppwled00/net_debug.cppwled00/net_debug.hwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
| #ifdef WLED_ENABLE_NET_DEBUG | ||
| JsonObject if_ndb = interfaces["ndb"]; | ||
| JsonArray if_ndb_ip = if_ndb["ip"]; | ||
| for (byte i = 0; i < 4; i++) | ||
| CJSON(netDebugPrintIP[i], if_ndb_ip[i]); | ||
| CJSON(netDebugPrintPort, if_ndb["port"]); | ||
| #endif |
There was a problem hiding this comment.
Validate netDebugPrintPort range on deserialize.
netDebugPrintPort is accepted directly from JSON here. A negative or out-of-range value can propagate into UDP calls after integer conversion.
💡 Suggested patch
`#ifdef` WLED_ENABLE_NET_DEBUG
JsonObject if_ndb = interfaces["ndb"];
JsonArray if_ndb_ip = if_ndb["ip"];
for (byte i = 0; i < 4; i++)
CJSON(netDebugPrintIP[i], if_ndb_ip[i]);
CJSON(netDebugPrintPort, if_ndb["port"]);
+ if (netDebugPrintPort < 0 || netDebugPrintPort > 65535) netDebugPrintPort = 0; // fallback handled during interface init
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/cfg.cpp` around lines 660 - 666, When deserializing the "ndb"
interface, validate the parsed netDebugPrintPort from interfaces["ndb"] (the
if_ndb JsonObject) before accepting it: ensure the value is within 0..65535 and
is non-negative; if it's out of range or not an integer, either clamp to a safe
default (e.g. 0 or previously configured netDebugPrintPort) and emit a warning,
or reject it and leave netDebugPrintPort unchanged. Update the assignment that
currently takes if_ndb["port"] directly to perform this range check and explicit
conversion to an unsigned 16-bit type so downstream UDP calls use a safe port
value.
| <input name="N2" type="number" class="s" min="0" max="255" > . | ||
| <input name="N3" type="number" class="s" min="0" max="255" ><br> | ||
| Netcat host Port:<br> | ||
| <input name="NP" type="number" min="0" max="99999" ><br> |
There was a problem hiding this comment.
Constrain Net Debug port to valid UDP range in the form.
Line 299 allows 99999, but valid port values are 1..65535.
🐛 Suggested fix
-<input name="NP" type="number" min="0" max="99999" ><br>
+<input name="NP" type="number" min="1" max="65535" ><br>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <input name="NP" type="number" min="0" max="99999" ><br> | |
| <input name="NP" type="number" min="1" max="65535" ><br> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/data/settings_sync.htm` at line 299, The Net Debug port input (input
name="NP") allows values up to 99999 which exceeds the valid UDP port range;
update the input element for NP to constrain values to the valid port range by
setting min="1" and max="65535" (and keep type="number") so the browser enforces
only 1..65535 can be submitted.
| #ifdef WLED_ENABLE_NET_DEBUG | ||
| for (int i=0;i<4;i++){ | ||
| String a = "N"+String(i); | ||
| netDebugPrintIP[i] = request->arg(a).toInt(); | ||
| } | ||
| netDebugPrintPort = request->arg("NP").toInt(); | ||
| #endif |
There was a problem hiding this comment.
Guard and validate Net Debug fields before applying them.
Line 529 and Line 531 currently overwrite values even when args are absent/invalid, which can silently reset settings to 0 on partial submissions.
🐛 Suggested hardening
`#ifdef` WLED_ENABLE_NET_DEBUG
- for (int i=0;i<4;i++){
- String a = "N"+String(i);
- netDebugPrintIP[i] = request->arg(a).toInt();
- }
- netDebugPrintPort = request->arg("NP").toInt();
+ for (int i = 0; i < 4; i++) {
+ String a = "N" + String(i);
+ if (request->hasArg(a)) {
+ netDebugPrintIP[i] = (uint8_t)constrain(request->arg(a).toInt(), 0, 255);
+ }
+ }
+ if (request->hasArg(F("NP"))) {
+ int np = request->arg(F("NP")).toInt();
+ if (np > 0 && np <= 65535) netDebugPrintPort = np;
+ }
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/set.cpp` around lines 526 - 532, The net debug block currently
unconditionally overwrites netDebugPrintIP[] and netDebugPrintPort which can
reset them to 0 when args are missing or invalid; change the logic to first
check for the presence and validity of each argument from request (e.g. for each
"N"+String(i) call request->hasArg(...) or test that request->arg(...) is
non-empty and numeric) before assigning netDebugPrintIP[i], and likewise verify
request->hasArg("NP") and that its value is a valid port number before assigning
netDebugPrintPort so existing settings are preserved when args are
absent/invalid.
| #ifdef WLED_ENABLE_NET_DEBUG | ||
| if (netDebugPrintIP[0] == 0) { | ||
| if (!netDebugPrintIP && !netDebugPrintIP.fromString(WLED_DEBUG_HOST)) { | ||
| #ifdef ESP8266 | ||
| WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP, 750); | ||
| #else | ||
| #ifdef WLED_USE_ETHERNET | ||
| ETH.hostByName(WLED_DEBUG_HOST, netDebugPrintIP); | ||
| #else | ||
| WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP); | ||
| #endif | ||
| #endif | ||
| } else { | ||
| IPAddress ipAddress = Network.localIP(); | ||
| netDebugPrintIP[0] = ipAddress[0]; | ||
| netDebugPrintIP[1] = ipAddress[1]; | ||
| netDebugPrintIP[2] = ipAddress[2]; | ||
| } | ||
| } | ||
| if (netDebugPrintPort == 0) | ||
| #ifdef WLED_DEBUG_PORT | ||
| netDebugPrintPort = WLED_DEBUG_PORT; | ||
| #else | ||
| netDebugPrintPort = 7868; //Default value | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WLED_DEBUG_HOST define presence =="
rg -n --type=cpp --type=h '^\s*#\s*define\s+WLED_DEBUG_HOST\b|^\s*#\s*ifndef\s+WLED_DEBUG_HOST\b'
echo
echo "== netDebug init block =="
rg -n -C4 'netDebugPrintIP|WLED_DEBUG_HOST|netDebugPrintPort' wled00/wled.cpp
echo
echo "== netDebugPrintIP octet assignments in wled.cpp =="
rg -n 'netDebugPrintIP\[[0-3]\]\s*=' wled00/wled.cppRepository: wled/WLED
Length of output: 91
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Lines 830-855 in wled00/wled.cpp ==="
sed -n '830,855p' wled00/wled.cpp
echo
echo "=== Context around netDebugPrintIP declaration ==="
rg -n 'netDebugPrintIP' wled00/wled.cpp | head -20
echo
echo "=== IPAddress class definition ==="
fd -t f '\.h$' -x grep -l 'class IPAddress' {} \;
echo
echo "=== Check if WLED_DEBUG_HOST is defined anywhere in project ==="
rg -r '\.{1,2}' --type=cpp --type=h 'define.*WLED_DEBUG_HOST|WLED_DEBUG_HOST\s*=' . 2>/dev/null || trueRepository: wled/WLED
Length of output: 1432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for WLED_DEBUG or DEBUG_HOST in config files ==="
find . -type f \( -name "*.h" -o -name "*.cpp" -o -name "*.json" -o -name "*.md" \) -exec grep -l "DEBUG_HOST\|DEBUG_PORT" {} \; 2>/dev/null || true
echo
echo "=== Check wled.h for netDebugPrintIP declaration ==="
grep -n 'netDebugPrintIP\|IPAddress' wled00/wled.h | head -20
echo
echo "=== Look for IPAddress usage with bracket notation ==="
rg -n 'IPAddress.*\[[0-3]\]' wled00/ | head -10
echo
echo "=== Check default values or config setup ==="
rg -n 'WLED_DEBUG_HOST|netDebugPrintIP' wled00/set.cpp | head -20Repository: wled/WLED
Length of output: 825
Net-debug init has compile-time dependency, inverted logic, and incomplete IP assignment bugs.
Line 832 references WLED_DEBUG_HOST without #ifdef WLED_DEBUG_HOST guard despite it being undefined in the codebase. The condition if (!netDebugPrintIP && !netDebugPrintIP.fromString(WLED_DEBUG_HOST)) inverts the fallback logic: when fromString() succeeds, the else branch overwrites it with the local IP instead of keeping the parsed value. Additionally, the local IP fallback (lines 844–846) assigns only octets 0–2, leaving octet 3 unset when IPAddress requires all 4 octets.
Suggested fix
`#ifdef` WLED_ENABLE_NET_DEBUG
- if (netDebugPrintIP[0] == 0) {
- if (!netDebugPrintIP && !netDebugPrintIP.fromString(WLED_DEBUG_HOST)) {
+ bool netDebugIpUnset = (netDebugPrintIP[0] == 0 && netDebugPrintIP[1] == 0 && netDebugPrintIP[2] == 0 && netDebugPrintIP[3] == 0);
+ if (netDebugIpUnset) {
+ `#ifdef` WLED_DEBUG_HOST
+ if (!netDebugPrintIP.fromString(WLED_DEBUG_HOST)) {
`#ifdef` ESP8266
WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP, 750);
`#else`
`#ifdef` WLED_USE_ETHERNET
ETH.hostByName(WLED_DEBUG_HOST, netDebugPrintIP);
`#else`
WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP);
`#endif`
`#endif`
- } else {
- IPAddress ipAddress = Network.localIP();
- netDebugPrintIP[0] = ipAddress[0];
- netDebugPrintIP[1] = ipAddress[1];
- netDebugPrintIP[2] = ipAddress[2];
}
+ `#else`
+ IPAddress ipAddress = Network.localIP();
+ netDebugPrintIP[0] = ipAddress[0];
+ netDebugPrintIP[1] = ipAddress[1];
+ netDebugPrintIP[2] = ipAddress[2];
+ netDebugPrintIP[3] = ipAddress[3];
+ `#endif`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifdef WLED_ENABLE_NET_DEBUG | |
| if (netDebugPrintIP[0] == 0) { | |
| if (!netDebugPrintIP && !netDebugPrintIP.fromString(WLED_DEBUG_HOST)) { | |
| #ifdef ESP8266 | |
| WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP, 750); | |
| #else | |
| #ifdef WLED_USE_ETHERNET | |
| ETH.hostByName(WLED_DEBUG_HOST, netDebugPrintIP); | |
| #else | |
| WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP); | |
| #endif | |
| #endif | |
| } else { | |
| IPAddress ipAddress = Network.localIP(); | |
| netDebugPrintIP[0] = ipAddress[0]; | |
| netDebugPrintIP[1] = ipAddress[1]; | |
| netDebugPrintIP[2] = ipAddress[2]; | |
| } | |
| } | |
| if (netDebugPrintPort == 0) | |
| #ifdef WLED_DEBUG_PORT | |
| netDebugPrintPort = WLED_DEBUG_PORT; | |
| #else | |
| netDebugPrintPort = 7868; //Default value | |
| #endif | |
| #endif | |
| `#ifdef` WLED_ENABLE_NET_DEBUG | |
| bool netDebugIpUnset = (netDebugPrintIP[0] == 0 && netDebugPrintIP[1] == 0 && netDebugPrintIP[2] == 0 && netDebugPrintIP[3] == 0); | |
| if (netDebugIpUnset) { | |
| `#ifdef` WLED_DEBUG_HOST | |
| if (!netDebugPrintIP.fromString(WLED_DEBUG_HOST)) { | |
| `#ifdef` ESP8266 | |
| WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP, 750); | |
| `#else` | |
| `#ifdef` WLED_USE_ETHERNET | |
| ETH.hostByName(WLED_DEBUG_HOST, netDebugPrintIP); | |
| `#else` | |
| WiFi.hostByName(WLED_DEBUG_HOST, netDebugPrintIP); | |
| `#endif` | |
| `#endif` | |
| } | |
| `#else` | |
| IPAddress ipAddress = Network.localIP(); | |
| netDebugPrintIP[0] = ipAddress[0]; | |
| netDebugPrintIP[1] = ipAddress[1]; | |
| netDebugPrintIP[2] = ipAddress[2]; | |
| netDebugPrintIP[3] = ipAddress[3]; | |
| `#endif` | |
| } | |
| if (netDebugPrintPort == 0) | |
| `#ifdef` WLED_DEBUG_PORT | |
| netDebugPrintPort = WLED_DEBUG_PORT; | |
| `#else` | |
| netDebugPrintPort = 7868; //Default value | |
| `#endif` | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled.cpp` around lines 830 - 855, Wrap usage of WLED_DEBUG_HOST in an
`#ifdef` WLED_DEBUG_HOST block and invert the fromString logic so a successful
parse keeps the configured host IP; specifically, inside the netDebugPrintIP
initialization (symbol: netDebugPrintIP and its method fromString) attempt
netDebugPrintIP.fromString(WLED_DEBUG_HOST) only when WLED_DEBUG_HOST is defined
and if that call returns true do not overwrite netDebugPrintIP, otherwise fall
back to Network.localIP(); when assigning the fallback IP from Network.localIP()
(symbol: Network::localIP or Network.localIP) copy all four octets (0..3) into
netDebugPrintIP so the fourth byte is not left uninitialized; leave the
netDebugPrintPort defaulting logic as-is but ensure the
WLED_DEBUG_HOST-dependent code is guarded by `#ifdef/`#endif.
| #define DEBUGOUT(x) netDebugEnabled?NetDebug.print(x):Serial.print(x) | ||
| #define DEBUGOUTLN(x) netDebugEnabled?NetDebug.println(x):Serial.println(x) | ||
| #define DEBUGOUTF(x...) netDebugEnabled?NetDebug.printf(x):Serial.printf(x) | ||
| #define DEBUGOUTFlush() netDebugEnabled?NetDebug.flush():Serial.flush() | ||
| #else | ||
| #define DEBUGOUT Serial | ||
| #define DEBUGOUT(x) Serial.print(x) | ||
| #define DEBUGOUTLN(x) Serial.println(x) | ||
| #define DEBUGOUTF(x...) Serial.printf(x) | ||
| #define DEBUGOUTFlush() Serial.flush() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Object-style DEBUGOUT member calls (incompatible with function-like DEBUGOUT) =="
rg -nP --type=cpp --type=h '\bDEBUGOUT\s*\.\s*(print|println|printf|flush)\s*\('
echo
echo "== Bare DEBUGOUT identifier usages in affected areas =="
rg -nP '\bDEBUGOUT\b(?!\s*\()' wled00/wled.cpp usermods/audioreactive/audio_reactive.cppRepository: wled/WLED
Length of output: 1346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the exact content of wled.h around lines 990-998
echo "=== wled.h lines 980-1010 ==="
sed -n '980,1010p' wled00/wled.h | cat -n
echo
echo "=== Check if there are any wrapper functions for stream compatibility ==="
rg -nA5 'debugOutStream|DEBUGOUT_STREAM|Print.*DEBUGOUT' wled00/wled.h
echo
echo "=== Verify the exact definitions of DEBUGOUT-related macros ==="
rg -nP '^\s*#define\s+(DEBUGOUT|DEBUGSR_|PLOT_)' wled00/wled.h usermods/audioreactive/audio_reactive.cpp | head -20Repository: wled/WLED
Length of output: 1519
DEBUGOUT API change breaks existing stream-style call sites.
Making DEBUGOUT a function-style macro breaks code that uses it as a stream object. usermods/audioreactive/audio_reactive.cpp:55-67 defines DEBUGSR_PRINT, DEBUGSR_PRINTLN, and DEBUGSR_PRINTF using DEBUGOUT.print(), DEBUGOUT.println(), and DEBUGOUT.printf() syntax. Additionally, wled00/wled.cpp:323 calls server.printStatus(DEBUGOUT), passing DEBUGOUT as a stream argument. These become compile-time errors under the new macro definition.
Consider using a wrapper function that returns a reference to the appropriate stream, allowing both function-call and stream-object usage patterns:
`#if` defined(WLED_ENABLE_NET_DEBUG)
inline Print& debugOutStream() { return netDebugEnabled ? static_cast<Print&>(NetDebug) : static_cast<Print&>(Serial); }
`#else`
inline Print& debugOutStream() { return static_cast<Print&>(Serial); }
`#endif`
`#define` DEBUGOUT_STREAM() debugOutStream()
`#define` DEBUGOUT(x) DEBUGOUT_STREAM().print(x)
`#define` DEBUGOUTLN(x) DEBUGOUT_STREAM().println(x)
`#define` DEBUGOUTF(x...) DEBUGOUT_STREAM().printf(x)
`#define` DEBUGOUTFlush() DEBUGOUT_STREAM().flush()Then migrate stream-argument call sites (e.g., printStatus(...)) to use DEBUGOUT_STREAM().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled.h` around lines 990 - 998, The DEBUGOUT macros were changed to
function-style calls which breaks sites expecting a stream object (e.g.,
DEBUGSR_PRINT macros that call DEBUGOUT.print()/println()/printf() and
server.printStatus(DEBUGOUT)); restore stream-style usage by adding a helper
that returns a Print& (e.g., implement inline Print& debugOutStream() that
returns NetDebug or Serial based on netDebugEnabled and build flag), expose a
macro like DEBUGOUT_STREAM() that expands to debugOutStream(), and then redefine
DEBUGOUT/DEBUGOUTLN/DEBUGOUTF/DEBUGOUTFlush to call
DEBUGOUT_STREAM().print/println/printf/flush while leaving DEBUGOUT_STREAM()
available for passing into functions such as printStatus and the
DEBUGSR_PRINT/DEBUGSR_PRINTLN/DEBUGSR_PRINTF sites so both function-call and
stream-object patterns work.
There was a problem hiding this comment.
DEBUGSR_ macros in audioreactive UM should be adjusted. It's not necessary to have a legacy support helper only for this usermod.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== DEBUGSR_ macros in audioreactive ==="
rg -nP '#define\s+DEBUGSR_' usermods/audioreactive/audio_reactive.cpp
echo
echo "=== wled.cpp printStatus call ==="
rg -n 'printStatus' wled00/wled.cpp
Length of output: 363
@softhack007, understood — the DEBUGSR_ macros in the audioreactive usermod just need to be updated to use the new function-style macros directly. Here's the fix:
// usermods/audioreactive/audio_reactive.cpp, lines 55-57
`#define` DEBUGSR_PRINT(x) DEBUGOUT(x)
`#define` DEBUGSR_PRINTLN(x) DEBUGOUTLN(x)
`#define` DEBUGSR_PRINTF(x...) DEBUGOUTF(x)One separate issue remains in wled00/wled.cpp line 323:
server.printStatus(DEBUGOUT); // DEBUGOUT is no longer a stream objectprintStatus() takes a Print& argument, so DEBUGOUT (now a function-like macro) can't be passed here directly. This call site will need to be handled — either by replacing it with a different logging approach, or by introducing a small helper just for this one call site. How would you like to handle that one?
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5559
File: wled00/wled.h:990-998
Timestamp: 2026-05-03T11:37:43.177Z
Learning: In the WLED audioreactive usermod (usermods/audioreactive/audio_reactive.cpp), DEBUGSR_PRINT/DEBUGSR_PRINTLN/DEBUGSR_PRINTF macros should use the function-style DEBUGOUT(x)/DEBUGOUTLN(x)/DEBUGOUTF(x...) macros, not the old stream-style DEBUGOUT.print()/println()/printf() syntax.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
| } | ||
|
|
||
| debugUdp.beginPacket(debugPrintHostIP, netDebugPrintPort); | ||
| debugUdp.beginPacket(netDebugPrintIP, netDebugPrintPort); |
There was a problem hiding this comment.
Maybe it's also a good time to scrutinise the net debug UDP core code - I found it quite unreliable in the past, even when only dumping packets via netcat. Maybe we should directly move to a standard logging protocol? Afaik the main difference is in establishing the connection, after that its basicially a printf with some meta data added.
Netdebug was an overnight hack by a contributor, it was never tested under "real load".
There was a problem hiding this comment.
@coderabbitai please analyze the net debug core code for possible stability issues, buffer overruns and potential for heap fragmentation. Suggest improvements / re-design options that would improve stability.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch net_debug.h full content
fd -e h -g 'net_debug*' --exec cat {}
echo "===== wled.h DEBUGOUT section ====="
# Extract DEBUGOUT/NetDebug/debugUdp relevant sections from wled.h
rg -n "DEBUGOUT|NetDebug|debugUdp|netDebugPrint|netDebugEnabled|USER_PRINT|USER_FLUSH" wled00/wled.h | head -80
echo "===== debugUdp declaration ====="
rg -n "debugUdp" wled00/ --type cpp --type h -C2
echo "===== WiFiUDP buffer sizes ====="
rg -rn "WiFiUDP|UDP_TX_PACKET_MAX|UDP_RX_PACKET_MAX" wled00/ --type cpp --type h | head -30
echo "===== NetworkDebugPrinter class definition ====="
ast-grep --pattern 'class NetworkDebugPrinter { $$$ }'
echo "===== wled.h net debug block ====="
grep -n -A 5 -B 2 "WLED_ENABLE_NET_DEBUG\|DEBUGOUT\b\|DEBUGOUTLN\|DEBUGOUTF" wled00/wled.h | head -100
Length of output: 6723
@softhack007, here is a detailed stability analysis of the net debug core, with redesign suggestions:
Net Debug Stability Analysis
🔴 Issue 1 — Per-character UDP packets (Critical heap fragmentation)
This is the most severe problem. write(uint8_t c) fires a complete UDP transaction for every single byte:
debugUdp.beginPacket(netDebugPrintIP, netDebugPrintPort); // heap alloc
debugUdp.write(c); // 1 byte payload
debugUdp.endPacket(); // heap free + syscallNetworkDebugPrinter inherits from Print, and Print::print(const char*) iterates character-by-character, calling write(uint8_t) for each. So DEBUG_PRINT("hello world\n") generates 12 heap alloc/free cycles and 12 UDP packets. A typical boot sequence with dozens of DEBUG_PRINTLN calls produces thousands of these micro-transactions. On ESP32, WiFiUDP::beginPacket() calls into lwIP and allocates a pbuf chain from the heap each time; this is the primary driver of heap fragmentation.
Fix — add a line-oriented ring buffer:
// net_debug.h — add to private section:
static constexpr size_t TX_BUF_SIZE = 512;
uint8_t _txBuf[TX_BUF_SIZE];
size_t _txLen = 0;
// net_debug.cpp — replace both write() overrides:
size_t NetworkDebugPrinter::write(uint8_t c) {
if (!WLED_CONNECTED || !netDebugEnabled) return 0;
if (_txLen < TX_BUF_SIZE) _txBuf[_txLen++] = c;
if (c == '\n' || _txLen == TX_BUF_SIZE) flush();
return 1;
}
size_t NetworkDebugPrinter::write(const uint8_t *buf, size_t size) {
if (!WLED_CONNECTED || buf == nullptr || size == 0 || !netDebugEnabled) return 0;
size_t written = 0;
while (written < size) {
size_t chunk = min(size - written, TX_BUF_SIZE - _txLen);
memcpy(_txBuf + _txLen, buf + written, chunk);
_txLen += chunk;
written += chunk;
if (_txLen == TX_BUF_SIZE || _txBuf[_txLen - 1] == '\n') flush();
}
return written;
}
void NetworkDebugPrinter::flush() {
if (_txLen == 0 || !WLED_CONNECTED || !netDebugEnabled) return;
if (debugUdp.beginPacket(netDebugPrintIP, netDebugPrintPort)) {
debugUdp.write(_txBuf, _txLen);
debugUdp.endPacket();
}
_txLen = 0;
}This reduces packet count from N-per-string to 1-per-line while keeping _txBuf as a stack-like member (no additional heap allocation).
🔴 Issue 2 — No IP / Port validation before beginPacket()
netDebugPrintPort is int, initialized to 0. netDebugPrintIP is initialized to 0.0.0.0. Neither is validated before calling beginPacket(). On some lwIP builds, sending to 0.0.0.0:0 can assert or silently corrupt socket state.
Fix:
// Add guard at top of both write() / flush():
if (netDebugPrintPort == 0 || (uint32_t)netDebugPrintIP == 0) return 0;Also, netDebugPrintPort should be uint16_t, not int, to make the type semantics explicit and avoid accidental negative values from deserialization:
// wled.h
WLED_GLOBAL uint16_t netDebugPrintPort _INIT(0);🟠 Issue 3 — No mutex / thread-safety for the global NetDebug singleton
WLED runs multiple FreeRTOS tasks on ESP32 (network task, async webserver, main loop, etc.). DEBUG_PRINT* macros can be called from several of these. debugUdp is a stateful object — concurrent beginPacket/write/endPacket calls from two tasks will corrupt its internal send-buffer pointer. The _txLen counter in the proposed ring buffer is equally vulnerable.
Fix — add a lightweight portMUX or SemaphoreHandle:
// net_debug.h — private section:
portMUX_TYPE _mux = portMUX_INITIALIZER_UNLOCKED;
// net_debug.cpp — in flush():
taskENTER_CRITICAL(&_mux);
// ... send ...
taskEXIT_CRITICAL(&_mux);Alternatively, for a non-IRQ context, a binary SemaphoreHandle_t is less invasive (won't disable interrupts). On ESP8266, the single-core architecture makes this less critical but still worth guarding against re-entrancy via yield() inside lwIP.
🟠 Issue 4 — DEBUGOUT macro ternary without statement wrapping
`#define` DEBUGOUT(x) netDebugEnabled?NetDebug.print(x):Serial.print(x)This bare ternary can cause surprising parse behavior when used as a statement in certain contexts (e.g., immediately after an if without braces, or as part of a comma expression). The return value is silently discarded. Prefer a do { } while(0) pattern or an if/else form:
`#define` DEBUGOUT(x) do { if (netDebugEnabled) NetDebug.print(x); else Serial.print(x); } while(0)
`#define` DEBUGOUTLN(x) do { if (netDebugEnabled) NetDebug.println(x); else Serial.println(x); } while(0)
`#define` DEBUGOUTF(x...) do { if (netDebugEnabled) NetDebug.printf(x); else Serial.printf(x); } while(0)🟡 Issue 5 — USER_FLUSH() is a no-op when net debug is active
`#ifdef` WLED_ENABLE_NET_DEBUG
`#define` USER_FLUSH() {} // ← silent no-opWith the proposed ring buffer, a flush() call becomes meaningful (drains buffered content). The no-op means that any partial line left in the buffer (e.g., a message without a trailing \n) will never be sent. USER_FLUSH() should call NetDebug.flush() once flush() is properly implemented.
🟡 Issue 6 — No size cap check in write(const uint8_t*, size_t)
Currently:
size = debugUdp.write(buf, size);The UDP MTU on the local network is ~1472 bytes (Ethernet) and the ESP32 lwIP send-buffer for UDP is bounded by CONFIG_LWIP_UDP_RECVMBOX_SIZE. A caller passing a very large size (unlikely for debug strings but possible in principle) will silently truncate without returning a correct written count. The ring buffer approach above naturally handles this by chunking.
Summary Table
| Issue | Severity | Impact |
|---|---|---|
| Per-character UDP packets | 🔴 Critical | Heavy heap fragmentation, network spam |
| No IP/port validation | 🔴 Critical | Potential lwIP assert / socket corruption |
| No thread-safety (mutex) | 🟠 Moderate | Data corruption under concurrent tasks |
| Bare ternary DEBUGOUT macros | 🟠 Moderate | Parse hazard in edge call sites |
| USER_FLUSH() no-op | 🟡 Low | Partial lines silently dropped |
| No UDP size cap | 🟡 Low | Silent truncation for large buffers |
int instead of uint16_t for port |
🟡 Low | Semantic / type safety |
The single highest-impact change is Issue 1 (ring buffer with newline-triggered flush) — it addresses heap fragmentation, packet count, and indirectly the size-cap issue in one pass.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When calling raw lwIP APIs (e.g., around `ntpUdp.begin()` or any `lwIP`/`tcpip`-layer call) in this codebase on ESP32 Arduino-ESP32 platforms where core locking/checking is enabled, wrap the lwIP call with `LOCK_TCPIP_CORE()` / `UNLOCK_TCPIP_CORE()` from `lwip/tcpip.h`. This prevents thread-safety/core-violation crashes without requiring `sdkconfig` changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:45.980Z
Learning: When using lwIP “raw” APIs in WLED on ESP32 (Arduino-ESP32 / IDF 5.5+), don’t rely on LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() unless CONFIG_LWIP_TCPIP_CORE_LOCKING=y is guaranteed. CONFIG_LWIP_CHECK_THREAD_SAFETY=y can trigger the assertion “Required to lock TCPIP core functionality!” when raw lwIP calls occur off the TCPIP thread. If the locking mode isn’t enabled (or can’t be changed via sdkconfig), schedule lwIP work (e.g., ntpUdp.begin() and similar raw lwIP calls) via tcpip_callback() so it runs on the TCPIP thread; this should work regardless of the locking-mode setting. Review any similar raw lwIP usage for correct thread/context handling.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:698-700
Timestamp: 2026-03-28T01:37:15.541Z
Learning: In this WLED codebase, when using `DEBUG_PRINTLN(F("..."))`, an explicit trailing `\n` inside the `F("...")` string (e.g., `DEBUG_PRINTLN(F("Warning!\n"))`) may be intentional to create a blank line in debug output as a visual separator. During code review, do not automatically flag these as “double newline” or recommend removing the `\n`—first verify with the author/context that the extra newline is deliberate.
Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:02.808Z
Learning: When working with WLED strings stored in PROGMEM on ESP8266/ESP32, assume you can’t use `strchr_P` for character scanning. If you need to scan a PROGMEM string for a character (e.g., searching for '@' within effect data), review changes to ensure they perform an explicit RAM copy first (e.g., via `strncpy_P` into a buffer) and then scan in RAM. Avoid proposing incremental/streaming PROGMEM character-by-character scans without a RAM buffer on these targets, since it’s impractical with the available APIs.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:09.816Z
Learning: In WLED’s WLED code, if a pixel/buffer allocation uses `BFRALLOC_NOBYTEACCESS` (and especially on classic ESP32 where byte-level access to IRAM-resident buffers is unsafe), avoid using byte-wise operations like `memset`/`memcpy` on that buffer. Specifically, do not combine `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` and do not perform `memcpy`/`memset` over `Segment::pixels` (e.g., in `setGeometry()`/`finalizeInit()` or copy ctor/assignment). Instead, use element-wise 32-bit access (loop over `uint32_t*` and assign/copy per element) to ensure 32-bit access only.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:29.218Z
Learning: In WLED, when reviewing ESP32 code that uses pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (e.g., `WS2812FX::_pixels` and `Segment::pixels`), treat `memset`/`memcpy` on those buffers as an intentionally low-risk, accepted pattern. These buffers are designed to be IRAM-safe for 32-bit-only access, and the Xtensa newlib assembly implementations perform 32-bit word stores for word-aligned, word-multiple-sized buffers (so the IRAM constraint is satisfied in practice). Do not flag `memset`/`memcpy` on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug; if any concern arises, downgrade severity to low and focus on potential future regressions rather than an immediate correctness violation.
Learnt from: softhack007
Repo: wled/WLED PR: 5508
File: wled00/FX.cpp:4137-4142
Timestamp: 2026-04-16T09:49:58.587Z
Learning: In WLED effect/overlay implementations (C++ under wled00), ensure effects operate through the SEGMENT.* read/write APIs for their own pixels/overlay state. Reserve global frame reads/writes like strip.getPixelColor* for copy_* style post-processing only. Do not manually composite/blend across lower layers inside the effect; segment-to-segment blending is handled later by WS2812FX::blendSegment(), so effects should produce their per-segment contributions without trying to composite with underlying layers themselves.
| #endif | ||
|
|
||
| // macros to print "user messages" to Serial | ||
| // cannot do this on -S2, due to buggy USBCDC serial driver |
There was a problem hiding this comment.
@netmindz ah, you already have a provision for USER_PRINT 👍 did not see this when writing my first comment.
The all you'd need is to also import caUseSerial from WLED-MM - this allows to always support USER_PRINT even without netdebug.
| #endif | ||
| #else | ||
| // if serial is available, we use Serial.print directly | ||
| #define USER_PRINT(x) { if (canUseSerial()) DEBUGOUT(x); } |
There was a problem hiding this comment.
@coderabbitai where is the implementation of ``canUseSerial()`?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "canUseSerial" --type=cpp --type=c -C3
Length of output: 501
🏁 Script executed:
#!/bin/bash
rg -n "canUseSerial" -C3
Length of output: 501
🏁 Script executed:
#!/bin/bash
# Search in .ino files (not picked up by default rg type filters)
rg -n "canUseSerial" --glob="*.ino" -C3
echo "---"
# Also check fcn_declare.h for declaration
grep -n "canUseSerial" wled00/fcn_declare.h
echo "---"
# Check all files including hidden
rg -n "canUseSerial" --no-ignore -C3
Length of output: 509
🏁 Script executed:
#!/bin/bash
# Check if canUseSerial exists on main branch
git fetch origin main 2>/dev/null
git grep -n "canUseSerial" origin/main
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any serial availability/guard helper functions
rg -n "bool.*[Ss]erial" wled00/ -C2
echo "---"
# Check if serialCanRX / serialCanTX are used as guards
rg -n "serialCan" wled00/ -C2
Length of output: 12284
@softhack007 — canUseSerial() does not exist anywhere in this repository. It is referenced in the newly added USER_PRINT/USER_PRINTLN/USER_PRINTF macros (Lines 1030–1032 of wled.h) that are active in the default build (when neither WLED_DEBUG nor WLED_ENABLE_NET_DEBUG is defined), but there is no declaration or definition of it in any .h, .cpp, or .ino file on this branch — or on main.
This would cause a compilation error for a standard build. The existing WLED idiom for guarding serial output is the global boolean serialCanTX (declared in wled.h line 529 and set in wled.cpp lines 524–525 based on pin-allocation checks), which is already used consistently throughout wled_serial.cpp and ir.cpp.
The macros should likely read:
`#define` USER_PRINT(x) { if (serialCanTX) Serial.print(x); }
`#define` USER_PRINTLN(x) { if (serialCanTX) Serial.println(x); }
`#define` USER_PRINTF(x...) { if (serialCanTX) Serial.printf(x); }(or route through DEBUGOUT/DEBUGOUTLN/DEBUGOUTF if that's preferred, but still guarded by serialCanTX).
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
There was a problem hiding this comment.
I this the function was forgotten when porting over from WLED-MM
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Try to fetch the WLED-MM canUseSerial function from raw GitHub
curl -s "https://raw.githubusercontent.com/MoonModules/WLED-MM/459a66491d7a3993ef456eb5cbfaad4c9d806f3b/wled00/wled_serial.cpp" | sed -n '80,110p'
Length of output: 1794
@softhack007 — confirmed, that's exactly the missing piece. The function retrieved from WLED-MM is richer than a simple serialCanTX check — it guards against several serial-conflict scenarios:
bool canUseSerial(void) {
// On S3/C3/C6/P4, USB CDC blocks if disconnected
`#if` ARDUINO_USB_CDC_ON_BOOT && (defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32P4)) && !defined(WLED_ENABLE_NET_DEBUG)
if (!Serial) return false;
`#endif`
// TX pin allocated to LEDs or other non-debug function
if (pinManager.isPinAllocated(hardwareTX) && (pinManager.getPinOwner(hardwareTX) != PinOwner::DebugOut))
return false;
// Serial in use for Adalight / TPM2NET / generic realtime streaming
if ((realtimeMode == REALTIME_MODE_GENERIC) || (realtimeMode == REALTIME_MODE_ADALIGHT) || (realtimeMode == REALTIME_MODE_TPM2NET))
return false;
// Don't interfere with IMPROV
if (improvActive > 0) return false;
// Continuous Serial Streaming active
if (continuousSendLED == true) return false;
return true;
}Two things to note for the port:
- The
!defined(WLED_DEBUG_HOST)guard in the USB-CDC check should be updated to!defined(WLED_ENABLE_NET_DEBUG)to match the new flag used in this PR. - The function needs to be declared in
fcn_declare.h(or equivalent) so it is visible whenwled.hmacros expand it — that declaration is also absent from this PR.
Without both the definition and the declaration, any build that doesn't define WLED_DEBUG or WLED_ENABLE_NET_DEBUG will fail to compile at the USER_PRINT/USER_PRINTLN/USER_PRINTF macros.
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5559
File: wled00/wled.h:1030-1030
Timestamp: 2026-05-03T11:55:40.143Z
Learning: In WLED (wled00/wled_serial.cpp), the function `canUseSerial()` (ported from WLED-MM) checks whether Serial output is safe to use: it blocks on disconnected USB-CDC (S3/C3/C6/P4 with ARDUINO_USB_CDC_ON_BOOT), TX pin conflicts, realtime modes GENERIC/ADALIGHT/TPM2NET, improvActive > 0, and continuousSendLED. This function must be declared in fcn_declare.h and defined in wled_serial.cpp. The upstream WLED main branch does NOT have this function — it only has the simpler `serialCanTX` boolean. The WLED_ENABLE_NET_DEBUG guard (not WLED_DEBUG_HOST) should be used inside it.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
There was a problem hiding this comment.
I agree that the minimal replacement could be checking if (serialCanTX
There was a problem hiding this comment.
@softhack007 — agreed, serialCanTX is the right minimal replacement here. The three affected macros in the #else branch become:
// if serial is available, we use Serial.print directly
`#define` USER_PRINT(x) { if (serialCanTX) Serial.print(x); }
`#define` USER_PRINTLN(x) { if (serialCanTX) Serial.println(x); }
`#define` USER_PRINTF(x...) { if (serialCanTX) Serial.printf(x); }
`#define` USER_FLUSH() { Serial.flush(); }This avoids the missing-function compile error and stays consistent with how serialCanTX is already used throughout wled_serial.cpp and ir.cpp. No declaration/definition changes are needed in fcn_declare.h or wled_serial.cpp.
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 5559
File: wled00/wled.h:1030-1030
Timestamp: 2026-05-03T11:57:15.014Z
Learning: In WLED (wled00/wled.h), the USER_PRINT/USER_PRINTLN/USER_PRINTF macros (active when neither WLED_DEBUG nor WLED_ENABLE_NET_DEBUG is defined) should guard serial output with `if (serialCanTX)` — i.e., `#define USER_PRINT(x) { if (serialCanTX) Serial.print(x); }`. The `canUseSerial()` function from WLED-MM is NOT present in upstream WLED and must NOT be referenced here; use the existing `serialCanTX` boolean instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
| #endif | ||
|
|
||
| // macros to print "user messages" to Serial | ||
| // cannot do this on -S2, due to buggy USBCDC serial driver |
There was a problem hiding this comment.
@coderabbitai please verify if the restriction for -S2 (buggy USBCDC) is still needed. For comparison, there is a workaround in WLED-MM but it has the side-effect that sometimes serial output could get lost
There was a problem hiding this comment.
🧩 Analysis chain
Let me look at the referenced WLED-MM code and check the current state of the S2 USBCDC bug.
🏁 Script executed:
#!/bin/bash
# Find all S2/USBCDC related guards and comments in WLED codebase
rg -n "S2|USBCDC|CDC_ON_BOOT|USB_CDC|canUseSerial" --type=cpp --type=h -C2
Length of output: 86561
🏁 Script executed:
#!/bin/bash
# Find canUseSerial and S2 serial-related guards
rg -n "canUseSerial|CONFIG_IDF_TARGET_ESP32S2|ARDUINO_USB_CDC_ON_BOOT|USBCDC" --type=cpp --type=h -C3
echo "---"
# Also check fcn_declare for canUseSerial declaration
rg -n "canUseSerial" -C3
Length of output: 37006
🏁 Script executed:
#!/bin/bash
# Find canUseSerial and S2 serial-related guards
rg -n "canUseSerial|CONFIG_IDF_TARGET_ESP32S2|ARDUINO_USB_CDC_ON_BOOT|USBCDC" --type=cpp --type=h -C3
echo "=== wled.cpp S2 serial handling ==="
rg -n "S2\|serial\|Serial\|canUseSerial" wled00/wled.cpp -i -C2 | head -80
echo "=== fcn_declare for canUseSerial ==="
rg -n "canUseSerial" -C3
Length of output: 37075
🌐 Web query:
ESP32-S2 USBCDC Arduino Serial driver bug status 2025
💡 Result:
As of 2025, the main “Arduino Serial over native USB CDC on ESP32‑S2” bugs people commonly refer to were already fixed upstream in the Espressif Arduino core:
- Dropped RX data /
setRxBufferSize()not taking effect / RX queue forced to 256 bytes (USB CDC on boot): fixed by PR#6413, merged Mar 28, 2022 (milestone 2.0.3). So any Arduino-ESP32 core ≥ 2.0.3 should include this fix. (github.com) ESP_LOG*/log_enot printing when using USB‑CDC (TinyUSB OTG) on ESP32‑S2/S3: fixed by PR#10123, merged Aug 13, 2024; it’s listed in the Arduino-ESP32 3.1.0-RC1 release notes and later. (github.com)
If what you’re seeing in 2025 is “no serial output / can’t open COM port”, that’s often configuration/host-side, not the old core bug—e.g. needing Tools → “USB CDC On Boot” enabled and the right USB mode/flash settings. (docs.espressif.com)
Citations:
- 1: Fixes USB CDC setRxBufferSize(), begin(), _onRX() espressif/arduino-esp32#6413
- 2: fix(USB): OTG S2 and S3 debug print espressif/arduino-esp32#10123
- 3: https://docs.espressif.com/projects/arduino-esp32/en/latest/tutorials/cdc_dfu_flash.html?utm_source=openai
Let me look at the canUseSerial() implementation and fetch the WLED-MM code for comparison.
🏁 Script executed:
#!/bin/bash
echo "=== canUseSerial definition ==="
ast-grep --pattern 'bool canUseSerial() { $$$ }'
rg -n "canUseSerial" --type=cpp --type=h -A5
echo "=== fcn_declare.h for canUseSerial ==="
rg -n "canUseSerial" wled00/fcn_declare.h -C3
echo "=== WLED-MM referenced lines via curl ==="
curl -s "https://raw.githubusercontent.com/MoonModules/WLED-MM/459a66491d7a3993ef456eb5cbfaad4c9d806f3b/wled00/wled.cpp" | sed -n '490,560p'
Length of output: 4309
🏁 Script executed:
#!/bin/bash
echo "=== canUseSerial definition ==="
rg -n "canUseSerial" -A10
echo "=== arduino-esp32 issue 9043 context - S2 setTxTimeoutMs ==="
curl -s "https://raw.githubusercontent.com/MoonModules/WLED-MM/459a66491d7a3993ef456eb5cbfaad4c9d806f3b/wled00/wled.cpp" | sed -n '470,500p'
Length of output: 2178
🏁 Script executed:
#!/bin/bash
echo "=== Search canUseSerial in all files ==="
rg -rn "canUseSerial" .
echo "=== wled.cpp lines around Serial for canUseSerial ==="
grep -n "canUseSerial\|bool.*serial\|Serial.*bool" wled00/wled.cpp wled00/util.cpp wled00/network.cpp 2>/dev/null | head -30
echo "=== Check fcn_declare.h for canUseSerial ==="
cat wled00/fcn_declare.h | grep -n "canUseSerial\|serial"
Length of output: 1237
🏁 Script executed:
#!/bin/bash
echo "=== wled_serial.cpp canUseSerial ==="
cat wled00/wled_serial.cpp | grep -n "canUseSerial" -A10 -B5
echo "=== Check all .cpp and .h files for canUseSerial definition ==="
grep -rn "bool canUseSerial\|canUseSerial()" wled00/ --include="*.cpp" --include="*.h" --include="*.ino"
Length of output: 359
@softhack007 — I checked the codebase and researched the current status of the S2 USBCDC driver. Here are the key findings:
1. The S2 USBCDC restriction is still relevant
The S2 CDC-over-USB crash is still being actively worked around in the current codebase — in wled00/FX_2Dfcn.cpp, there is an explicit delay(1) guard specifically for S2:
`#if` defined(CONFIG_IDF_TARGET_ESP32S2)
delay(1); // on S2 the CDC output can crash without a delay
`#endif`The WLED-MM workaround applies Serial.setTxTimeoutMs(0) (for arduino-esp32 issue #9043) only to S3/C3/C6/P4 — the ESP32-S2 is intentionally excluded:
// WLED-MM: only for S3/C3/C6/P4, NOT S2:
`#if` ARDUINO_USB_CDC_ON_BOOT && (defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || ...)
Serial.setTxTimeoutMs(0); // potential side-effect: incomplete debug outputSome specific arduino-esp32 bugs have been fixed — the RX buffer bug was resolved in PR #6413 (merged March 2022, milestone 2.0.3), and the ESP_LOG* not printing via TinyUSB was fixed in PR #10123 (merged August 2024, in 3.1.0-RC1). However, blocking TX/flush behavior for the S2 remains unaddressed by setTxTimeoutMs(0), which is why WLED-MM explicitly excludes the S2 from that fix.
2. Critical issue: canUseSerial() is undefined ⚠️
More importantly, searching the entire PR branch reveals that canUseSerial() is referenced in three macros but never defined anywhere:
// wled.h — only usages, no definition found:
`#define` USER_PRINT(x) { if (canUseSerial()) DEBUGOUT(x); }
`#define` USER_PRINTLN(x) { if (canUseSerial()) DEBUGOUTLN(x); }
`#define` USER_PRINTF(x...) { if (canUseSerial()) DEBUGOUTF(x); }This #else branch is entered when neither WLED_DEBUG nor WLED_ENABLE_NET_DEBUG is defined — i.e., the standard release build. Any such build will fail to compile/link with an undefined reference.
The intent appears to be a runtime guard similar to WLED-MM's if (Serial) checks. A minimal implementation could be:
// in wled00/fcn_declare.h or inline:
inline bool canUseSerial() {
`#if` defined(ARDUINO_USB_CDC_ON_BOOT) || defined(ARDUINO_USB_MODE)
return (bool)Serial; // only safe if USB CDC has actually initialized
`#else`
return true; // standard UART serial is always ready after Serial.begin()
`#endif`
}For S2 specifically, given the CDC crash risk under heavy output, a more conservative approach (like WLED-MM's Serial.setTxTimeoutMs(0) equivalent or the delay(1) pattern) may also be needed. The WLED-MM note that setTxTimeoutMs(0) has "the side-effect that sometimes serial output could get lost" is the acknowledged trade-off.
Summary: The S2 restriction comment is still warranted. The more pressing issue is that canUseSerial() must be defined before this PR is mergeable.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@wled00/bus_manager.h`:
- Line 43: The DEBUGBUS_PRINTF_P macro currently maps to DEBUGOUTF which calls
plain printf and therefore breaks PROGMEM PSTR() format strings on ESP8266; fix
by adding a PROGMEM-aware path: either implement a new DEBUGOUTF_P that calls
printf_P() (or NetDebug.printf_P()) on ESP8266 and fallback to printf on other
platforms, or modify DEBUGOUTF to detect _P usage and route to printf_P; then
change DEBUGBUS_PRINTF_P to map to DEBUGOUTF_P (or the new PROGMEM-aware
DEBUGOUTF) so calls like DEBUGBUS_PRINTF_P(PSTR(...), ...) print correctly.
In `@wled00/json.cpp`:
- Around line 371-373: The runtime handling currently masks the net-debug toggle
behind WLED_DEBUG; change the conditional so netDebugEnabled is updated when
WLED_ENABLE_NET_DEBUG is defined (regardless of WLED_DEBUG). Specifically,
replace the guarded line that sets netDebugEnabled from root[F("debug")] so it
is compiled when WLED_ENABLE_NET_DEBUG is defined (use `#if`
defined(WLED_ENABLE_NET_DEBUG) ... `#endif`) and keep the assignment
netDebugEnabled = root[F("debug")] | netDebugEnabled; so the /json/state "debug"
field actually toggles net debug at runtime; keep existing symbols
netDebugEnabled and root[F("debug")] to locate and modify the code.
- Around line 867-872: The net-debug bit handling is incorrectly inside the
WLED_DEBUG preprocessor guard so builds with only WLED_ENABLE_NET_DEBUG won't
advertise support or status; move or duplicate the WLED_ENABLE_NET_DEBUG-related
logic out of the `#ifdef` WLED_DEBUG block so that the code that sets the os
status bits for net-debug runs regardless of WLED_DEBUG. Specifically, ensure
the code that inspects netDebugEnabled and sets/clears the 0x0100/0x0080 bits
(the os variable and netDebugEnabled checks) is executed for builds with
WLED_ENABLE_NET_DEBUG so /json/info reports net-debug capability and runtime
status while still keeping any verbose debug-only behavior inside WLED_DEBUG.
In `@wled00/wled.h`:
- Around line 1019-1028: The USER_PRINT*, USER_PRINTLN, and USER_PRINTF macros
currently forward to DEBUG_PRINT* even when only WLED_ENABLE_NET_DEBUG is
defined, which makes them no-ops because DEBUG_PRINT is compiled out unless
WLED_DEBUG is set; change the macro logic so that when WLED_DEBUG is defined
USER_PRINT*/USER_PRINTF map to DEBUG_PRINT*/DEBUG_PRINTF, but when only
WLED_ENABLE_NET_DEBUG is defined they map to the Net-Debug output functions (do
not forward to DEBUG_PRINT); also ensure USER_FLUSH uses the Net-Debug flush in
that branch (keep references to USER_PRINT, USER_PRINTLN, USER_PRINTF,
DEBUG_PRINT, WLED_ENABLE_NET_DEBUG, WLED_DEBUG, USER_FLUSH, DEBUGOUTFlush to
locate and update the macros).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fde3cfb3-3c9a-4448-a39f-5e5180ea8911
📒 Files selected for processing (5)
wled00/bus_manager.hwled00/cfg.cppwled00/data/index.jswled00/json.cppwled00/wled.h
✅ Files skipped from review due to trivial changes (1)
- wled00/data/index.js
| #define DEBUGBUS_PRINT(x) DEBUGOUT(x) | ||
| #define DEBUGBUS_PRINTLN(x) DEBUGOUTLN(x) | ||
| #define DEBUGBUS_PRINTF(x...) DEBUGOUTF(x) | ||
| #define DEBUGBUS_PRINTF_P(x...) DEBUGOUTF(x) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find definition of DEBUGOUTF and DEBUGOUT macros
rg -n --type=cpp --type=h -C3 'define\s+DEBUGOUTF\b'
rg -n --type=cpp --type=h -C3 'define\s+DEBUGOUT\b'Repository: wled/WLED
Length of output: 1720
🏁 Script executed:
# Check line 482 in bus_manager.h for the actual usage
sed -n '475,490p' wled00/bus_manager.h
# Search for printf_P usage to see if it's used elsewhere
rg -n 'printf_P' wled00/ --type=cpp --type=h | head -20
# Search for historical _P debugging patterns
rg -n 'DEBUGOUT.*_P\b' wled00/ --type=cpp --type=h | head -20Repository: wled/WLED
Length of output: 2811
DEBUGBUS_PRINTF_P mapping violates PROGMEM string handling.
Line 43 maps DEBUGBUS_PRINTF_P to DEBUGOUTF, which from wled.h (lines 993, 998) calls Serial.printf(x) or NetDebug.printf(x) — not printf_P().
The actual callsite at line 482 uses:
DEBUGBUS_PRINTF_P(PSTR("Bus: Config (%d-%d, type:%d, CO:%d, rev:%d, skip:%d, AW:%d kHz:%d, mA:%d/%d, driver:%s)\n"), ...)On ESP8266, PSTR() strings reside in PROGMEM and must be read via printf_P(). Calling plain printf() on a PROGMEM pointer produces garbled output or faults.
The project guideline states: "Use PSTR() with DEBUG_PRINTF_P() for format strings." This change violates that by silently dropping the _P distinction.
Either:
- Define a separate
DEBUGOUTF_Pmacro that callsprintf_P(), or - Make
DEBUGOUTFitself PROGMEM-aware (route_Pcalls throughprintf_P()on ESP8266).
Then wire DEBUGBUS_PRINTF_P to the correct macro.
🤖 Prompt for 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.
In `@wled00/bus_manager.h` at line 43, The DEBUGBUS_PRINTF_P macro currently maps
to DEBUGOUTF which calls plain printf and therefore breaks PROGMEM PSTR() format
strings on ESP8266; fix by adding a PROGMEM-aware path: either implement a new
DEBUGOUTF_P that calls printf_P() (or NetDebug.printf_P()) on ESP8266 and
fallback to printf on other platforms, or modify DEBUGOUTF to detect _P usage
and route to printf_P; then change DEBUGBUS_PRINTF_P to map to DEBUGOUTF_P (or
the new PROGMEM-aware DEBUGOUTF) so calls like DEBUGBUS_PRINTF_P(PSTR(...), ...)
print correctly.
| #if defined(WLED_DEBUG) && defined(WLED_ENABLE_NET_DEBUG) | ||
| netDebugEnabled = root[F("debug")] | netDebugEnabled; | ||
| #endif |
There was a problem hiding this comment.
/json/state can't toggle net debug unless WLED_DEBUG is also on.
The runtime flag is still guarded by WLED_DEBUG && WLED_ENABLE_NET_DEBUG, so a WLED_ENABLE_NET_DEBUG build silently ignores {"debug":...} even though the PR exposes net debug as a runtime feature.
Suggested fix
- `#if` defined(WLED_DEBUG) && defined(WLED_ENABLE_NET_DEBUG)
+ `#ifdef` WLED_ENABLE_NET_DEBUG
netDebugEnabled = root[F("debug")] | netDebugEnabled;
`#endif`🤖 Prompt for 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.
In `@wled00/json.cpp` around lines 371 - 373, The runtime handling currently masks
the net-debug toggle behind WLED_DEBUG; change the conditional so
netDebugEnabled is updated when WLED_ENABLE_NET_DEBUG is defined (regardless of
WLED_DEBUG). Specifically, replace the guarded line that sets netDebugEnabled
from root[F("debug")] so it is compiled when WLED_ENABLE_NET_DEBUG is defined
(use `#if` defined(WLED_ENABLE_NET_DEBUG) ... `#endif`) and keep the assignment
netDebugEnabled = root[F("debug")] | netDebugEnabled; so the /json/state "debug"
field actually toggles net debug at runtime; keep existing symbols
netDebugEnabled and root[F("debug")] to locate and modify the code.
| #ifdef WLED_DEBUG | ||
| os = 0x80; | ||
| #ifdef WLED_DEBUG_HOST | ||
| #ifdef WLED_ENABLE_NET_DEBUG | ||
| os |= 0x0100; | ||
| if (!netDebugEnabled) os &= ~0x0080; | ||
| #endif |
There was a problem hiding this comment.
Net-debug status bits are still hidden behind the outer WLED_DEBUG guard.
Because this block only runs inside #ifdef WLED_DEBUG, /json/info won't advertise net-debug support/status for WLED_ENABLE_NET_DEBUG-only builds. That makes the runtime feature look unavailable even when it is compiled in.
Suggested fix
uint16_t os = 0;
`#ifdef` WLED_DEBUG
- os = 0x80;
- `#ifdef` WLED_ENABLE_NET_DEBUG
- os |= 0x0100;
- if (!netDebugEnabled) os &= ~0x0080;
- `#endif`
+ os |= 0x0080;
`#endif`
+ `#ifdef` WLED_ENABLE_NET_DEBUG
+ os |= 0x0100;
+ if (netDebugEnabled) os |= 0x0080;
+ `#endif`🤖 Prompt for 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.
In `@wled00/json.cpp` around lines 867 - 872, The net-debug bit handling is
incorrectly inside the WLED_DEBUG preprocessor guard so builds with only
WLED_ENABLE_NET_DEBUG won't advertise support or status; move or duplicate the
WLED_ENABLE_NET_DEBUG-related logic out of the `#ifdef` WLED_DEBUG block so that
the code that sets the os status bits for net-debug runs regardless of
WLED_DEBUG. Specifically, ensure the code that inspects netDebugEnabled and
sets/clears the 0x0100/0x0080 bits (the os variable and netDebugEnabled checks)
is executed for builds with WLED_ENABLE_NET_DEBUG so /json/info reports
net-debug capability and runtime status while still keeping any verbose
debug-only behavior inside WLED_DEBUG.
| #if defined(WLED_DEBUG) || defined(WLED_ENABLE_NET_DEBUG) | ||
| // use DEBUG_PRINT | ||
| #define USER_PRINT(x) DEBUG_PRINT(x) | ||
| #define USER_PRINTLN(x) DEBUG_PRINTLN(x) | ||
| #define USER_PRINTF(x...) DEBUG_PRINTF(x) | ||
| #ifdef WLED_ENABLE_NET_DEBUG | ||
| #define USER_FLUSH() {} | ||
| #else | ||
| #define USER_FLUSH() {DEBUGOUTFlush();} | ||
| #endif |
There was a problem hiding this comment.
USER_PRINT* becomes a no-op in net-debug-only builds.
This branch is selected when WLED_ENABLE_NET_DEBUG is enabled, but it forwards to DEBUG_PRINT*, which expands to nothing unless WLED_DEBUG is also defined. That drops the runtime/user-facing messages this PR is trying to expose over Net Debug. As per coding guidelines, Use DEBUG_PRINTF() / DEBUG_PRINTLN() for debug output in C++ (compiled out unless -D WLED_DEBUG).
Suggested fix
-#if defined(WLED_DEBUG) || defined(WLED_ENABLE_NET_DEBUG)
- // use DEBUG_PRINT
+#if defined(WLED_DEBUG)
+ // use DEBUG_PRINT
`#define` USER_PRINT(x) DEBUG_PRINT(x)
`#define` USER_PRINTLN(x) DEBUG_PRINTLN(x)
`#define` USER_PRINTF(x...) DEBUG_PRINTF(x)
+ `#ifdef` WLED_ENABLE_NET_DEBUG
+ `#define` USER_FLUSH() {}
+ `#else`
+ `#define` USER_FLUSH() {DEBUGOUTFlush();}
+ `#endif`
+#elif defined(WLED_ENABLE_NET_DEBUG)
+ // use net debug routing directly even when WLED_DEBUG is off
+ `#define` USER_PRINT(x) DEBUGOUT(x)
+ `#define` USER_PRINTLN(x) DEBUGOUTLN(x)
+ `#define` USER_PRINTF(x...) DEBUGOUTF(x)
`#ifdef` WLED_ENABLE_NET_DEBUG
`#define` USER_FLUSH() {}
`#else`
`#define` USER_FLUSH() {DEBUGOUTFlush();}
`#endif`🤖 Prompt for 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.
In `@wled00/wled.h` around lines 1019 - 1028, The USER_PRINT*, USER_PRINTLN, and
USER_PRINTF macros currently forward to DEBUG_PRINT* even when only
WLED_ENABLE_NET_DEBUG is defined, which makes them no-ops because DEBUG_PRINT is
compiled out unless WLED_DEBUG is set; change the macro logic so that when
WLED_DEBUG is defined USER_PRINT*/USER_PRINTF map to DEBUG_PRINT*/DEBUG_PRINTF,
but when only WLED_ENABLE_NET_DEBUG is defined they map to the Net-Debug output
functions (do not forward to DEBUG_PRINT); also ensure USER_FLUSH uses the
Net-Debug flush in that branch (keep references to USER_PRINT, USER_PRINTLN,
USER_PRINTF, DEBUG_PRINT, WLED_ENABLE_NET_DEBUG, WLED_DEBUG, USER_FLUSH,
DEBUGOUTFlush to locate and update the macros).
3 participants
This pull request introduces significant improvements to the WLED network debug (Net Debug) system, shifting from hostname-based configuration to direct IP address and port configuration, and making Net Debug settings user-configurable through the web UI and JSON config. It also unifies and simplifies debug macro usage throughout the codebase.
Key changes:
Net Debug System Overhaul
WLED_DEBUG_HOST(hostname-based) configuration with a newWLED_ENABLE_NET_DEBUGsystem that uses a direct IP address (netDebugPrintIP) and port (netDebugPrintPort) for UDP debug output, improving reliability and flexibility. LegacyWLED_DEBUG_HOSTsupport is maintained for backward compatibility. [1] [2] [3] [4]Debug Macro and Output Refactoring
DEBUG_PRINT,DEBUGFX_PRINT,DEBUGBUS_PRINT, etc.) to use unified function-style macros (DEBUGOUT(x),DEBUGOUTLN(x),DEBUGOUTF(x...)), simplifying the code and ensuring consistent debug output routing. [1] [2] [3] [4]WLED_ENABLE_NET_DEBUGinstead ofWLED_DEBUG_HOST, clarifying feature toggling and pin allocation. [1] [2] [3]Configuration and Web UI Enhancements
settings_sync.htm), enabling users to configure network debug output directly from the browser.JSON API and Info Output
These changes modernize and robustify the Net Debug system, make debug output more maintainable, and improve user configurability and transparency.
Summary by CodeRabbit
Release Notes
Refactor
New Features
UI Changes