Skip to content
2 changes: 1 addition & 1 deletion wled00/data/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ function sendDDP(ws, start, len, colors) {
let pkt = new Uint8Array(11 + dLen); // DDP header is 10 bytes, plus 1 byte for WLED websocket protocol indicator
pkt[0] = 0x02; // DDP protocol indicator for WLED websocket. Note: below DDP protocol bytes are offset by 1
pkt[1] = 0x40; // flags: 0x40 = no push, 0x41 = push (i.e. render), note: this is DDP protocol byte 0
pkt[2] = 0x00; // reserved
pkt[2] = 0x00; // upper nibble is reserved, lower nibble is sequence number, if set to 0 no sequence checking is done (if enabled)
pkt[3] = 0x0B; // RGB, 8bit per channel
pkt[4] = 0x01; // destination id (not used but 0x01 is default output)
pkt[5] = (off >> 24) & 255; // DDP protocol 4-7 is offset
Expand Down
60 changes: 44 additions & 16 deletions wled00/e131.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#define MAX_CHANNELS_PER_UNIVERSE 512

// forward declarations
static void handleDDPPacket(e131_packet_t* p);
static void handleDDPPacket(e131_packet_t* p, size_t packetLen);
static void handleArtnetPollReply(IPAddress ipAddress);
static void prepareArtnetPollReply(ArtPollReply *reply);
static void sendArtnetPollReply(ArtPollReply *reply, IPAddress ipAddress, uint16_t portAddress);
Expand All @@ -17,20 +17,31 @@ static void sendArtnetPollReply(ArtPollReply *reply, IPAddress ipAddress, uint16

//DDP protocol support, called by handleE131Packet
//handles RGB data only
static void handleDDPPacket(e131_packet_t* p) {
static void handleDDPPacket(e131_packet_t* p, size_t packetLen) {
static bool ddpSeenPush = false; // have we seen a push yet?
int lastPushSeq = e131LastSequenceNumber[0];

if (packetLen < DDP_HEADER_LEN) return; // too short to safely read any DDP header fields

// reject unsupported color data types (only RGB and RGBW are supported)
uint8_t maskedType = p->dataType & 0x3F; // mask out custom and reserved flags, only type bits are relevant
if (maskedType != DDP_TYPE_RGB24 && maskedType != DDP_TYPE_RGBW32) return;
//uint8_t maskedType = p->dataType & 0x3F; // mask out custom and reserved flags, only type bits are relevant
//if (maskedType != DDP_TYPE_RGB24 && maskedType != DDP_TYPE_RGBW32) return;

// note: for maximum compatibility we do not reject unknonw or malformed data types but simply default to RGB24 and check there is enough data available in the packet to do so
// also we assume 8bit per channel and currently do not support other bit depths

// reject control, status and config packets (not implemented)
if (p->destination == DDP_ID_CONTROL || p->destination == DDP_ID_STATUS || p->destination == DDP_ID_CONFIG) return;

// reject query and response packets (not implemented)
if (p->flags & (DDP_FLAGS_QUERY | DDP_FLAGS_REPLY)) return;

// reject status and config packets (not implemented)
if (p->destination == DDP_ID_STATUS || p->destination == DDP_ID_CONFIG) return;
bool push = p->flags & DDP_FLAGS_PUSH; // push flag means "render now"
if (!push && (p->flags & DDP_FLAGS_STORAGE)) return; // reject "from storage" flag but still let the push flag pass if set along with it
Comment thread
coderabbitai[bot] marked this conversation as resolved.

//reject late packets belonging to previous frame (assuming 4 packets max. before push)
//reject late packets belonging to previous frame (assuming 4 packets max. before push, if more are used and packets are very late, they are still accepted)
if (e131SkipOutOfSequence && lastPushSeq) {
int sn = p->sequenceNum & 0xF;
int sn = p->sequenceNum & 0xF; // sequence number is 4 bits, 1-15, 0 means unused
Copy link
Copy Markdown
Member

@softhack007 softhack007 Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai please check if WLED handling of sequence numbers is compatible with DDP protocol specs. Accepting an "outdated" packet may be acceptable, but a wrong rejection should be avoided, even when more than 15 packets are needed to transmit a full "frame". Also check against the real implementation of known DDP senders like LedFx, hyperion, xlights, OpenRGB, FPP.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me research the DDP protocol spec and how known senders handle sequence numbers.

@softhack007 Here is a detailed analysis of WLED's DDP sequence number handling against the spec and known senders.


DDP Sequence Number Spec

The DDP spec (byte 1) defines nnnn as a sequence number from 1–15, or zero if not used, and states the sequence number should be incremented with each new packet sent. A sender can send duplicate packets with the same sequence number and DDP header for redundancy. The sequence number is ignored if zero.

Key point: the sequence number is per-packet (not per-frame), and wraps in the ring 1→2→…→15→1.


WLED's Rejection Logic

WLED's current code (after this PR):

int sn = p->sequenceNum & 0xF; // 0 = disabled
if (sn) {
  if (lastPushSeq > 5) {
    if (sn > (lastPushSeq - 5) && sn < lastPushSeq) return;  // reject window: 5 slots before last push
  } else {
    if (sn > (10 + lastPushSeq) || sn < lastPushSeq) return;
  }
}

lastPushSeq is the sequence number of the last push packet received. The window rejects any packet whose sn falls in the 5 slots immediately preceding lastPushSeq, on the assumption those are late-arriving packets from the previous frame.


Where This Breaks Down

The 5-slot window hardcodes an assumption of ≤5 packets per frame.

Consider a large installation with, e.g., 5760 LEDs (12 packets of 480 RGB pixels @ 1440 bytes each, a common xLights packet size):

  • Previous frame push: lastPushSeq = 11
  • Reject window: sn ∈ {7, 8, 9, 10}
  • New frame packets arrive in sequence: 12, 13, 14, 15, 1, 2, 3, 4, 5, 6, 7, 8 → push at sn = 9
  • Packets with sn = 7 and sn = 8 from the new frame fall inside the reject window and are wrongly dropped, causing visual corruption/flicker on those LED ranges mid-frame.

This means for any installation needing roughly >10 packets per frame (with sequence numbers enabled), current-frame packets will be wrongly rejected. OpenRGB's DDP implementation uses sequence numbers from 1–15 per spec, and xLights defaults to 1440 bytes per DDP packet, which is 480 RGB pixels — meaning only ~7–8 packets are needed before wrapping issues arise for mid-size installs.


Known Sender Behaviors

Sender Seq Numbers Risk
xLights Increments per packet, 1–15 🔴 Affected for >~10 packets/frame
OpenRGB 4-bit nibble per spec 🔴 Same
FPP Standard DDP, per-packet 🔴 Same
LedFx / Hyperion Typically use seq=0 (disabled) or low packet counts 🟡 Likely safe

Senders that set sn = 0 (disabled) bypass this check entirely and are unaffected.


Summary of Issues

  1. Wrong rejections are possible: For frames requiring >~10 packets with sequence numbers enabled, valid new-frame packets whose sn falls in the 5-slot window behind lastPushSeq (after wrap) are silently dropped. The PR comment "if more are used and packets are very late, they are still accepted" is not fully accurate for this wrap-around case.

  2. The 15-value ring is too small: With only 15 sequence numbers and a 5-slot window, there's only room for about 10 "safe" slots per frame. Any frame needing more packets will cycle through the reject window.

  3. Safe path exists: If the sender sets sn = 0 (sequence checking disabled), WLED's code correctly skips all rejection.


Recommendations

  • Best fix: Increase the "assumed max lag" from 5 to a smaller window (e.g., 2–3) to reduce false rejections, or better yet, track the sequence number of the last accepted packet (not just the push packet) and reject only obvious backwards steps.
  • Safe guard: Consider documenting that this feature is unreliable for frames requiring >10 packets and advising senders (or WLED's own DDP broadcast) to use sn = 0 for large installations.
  • Least risk: If in doubt, the entire out-of-sequence rejection for DDP could be disabled (set sn = 0 in WLED's own output), since DDP spec explicitly allows ignoring it. The spec does not mandate receivers implement rejection.

🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.

Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-20T17:38:19.745Z
Learning: In WLED DDP packet handling (wled00/e131.cpp), the destination field should be validated to reject non-display packets. Packets with destination=DDP_ID_STATUS (251) or destination=DDP_ID_CONFIG (250) should be rejected and not processed as LED display data. Only packets with destination=DDP_ID_DISPLAY (1) should be accepted for LED output. WLED doesn't distinguish between multiple display IDs (e.g., display 1 vs display 2), but it must distinguish between DISPLAY packets versus STATUS/CONFIG packets.

Learnt from: softhack007
Repo: wled/WLED PR: 5547
File: wled00/e131.cpp:33-40
Timestamp: 2026-04-28T09:41:31.415Z
Learning: In WLED DDP packet handling (wled00/e131.cpp), the destination filtering uses an explicit deny-list (reject DDP_ID_CONTROL, DDP_ID_STATUS, DDP_ID_CONFIG) rather than an allow-list (only DDP_ID_DISPLAY). This is intentional for maximum compatibility — devices may use non-standard or future destination IDs that should still drive LED output. Do not suggest switching to an allow-list (`p->destination != DDP_ID_DISPLAY`) in future reviews of this code.

Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-13T09:36:13.994Z
Learning: WLED serial streaming frame maxima:
- Adalight: protocol max 65,536 LEDs (count = hi<<8 | lo, plus 1), but WLED’s uint16_t parser caps it at 65,535.
- TPM2 serial (RGB): protocol max payload 65,535 bytes → 21,845 LEDs (bytes/3); effective output always limited by configured LED count and MAX_LEDS.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR `#4876`, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.

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.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T16:10:42.477Z
Learning: In WLED (wled/WLED), the segments vector (`strip.segments`) is highly volatile: preset loads, effect changes, and JSON state updates in json.cpp can add/remove/reorder segments and change their active status at any time. Segments have no stable persistent identifier — only their ordinal index in the vector, which also changes. Code that tries to maintain a discovery-time binding from an external protocol (e.g., Espalexa/Alexa) to a specific segment using ordinal-among-active-segments as a key will silently map to wrong segments after any preset load. A correct implementation must store the init-time segment-index-to-device mapping explicitly and guard against the segment at that index changing identity after a vector rebuild.

Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).

Learnt from: softhack007
Repo: wled/WLED PR: 5436
File: wled00/fcn_declare.h:394-395
Timestamp: 2026-03-22T00:09:16.015Z
Learning: In WLED (wled00/fcn_declare.h), the `prepareHostname(char* hostname, size_t maxLen)` signature change from the old `prepareHostname(char* hostname)` is intentional and breaking. There is no compatibility shim; any usermod or downstream caller must explicitly pass the buffer size. Do not suggest a default-size shim for this function.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/bus_manager.cpp:549-556
Timestamp: 2026-02-11T16:20:02.872Z
Learning: ESP-IDF V5 requires a complete rewrite of the LEDC (PWM/analog LED) subsystem in WLED. The framework now manages LEDC channels internally (unlike V4 where WLED managed them directly), and dithering functionality may not work as it did before. Direct LEDC struct access (e.g., `LEDC.channel_group[gr].channel[ch].duty.duty`) used in the current implementation will need to be replaced with proper HAL API calls. This affects `wled00/bus_manager.cpp` BusPwm class and related pin manager LEDC allocation code.

Learnt from: softhack007
Repo: wled/WLED PR: 5482
File: wled00/wled.h:579-579
Timestamp: 2026-04-08T21:25:44.777Z
Learning: In WLED PR `#5482` (wled00/wled.h), the `denyWsecUpload` flag is confirmed to default to `false`. The complementary defence against malformed wsec.json content (bootloop/brick risk from malformed JSON) is handled by PR `#5484`, which hardens JSON file reading/parsing at load time. Do not suggest moving `denyWsecUpload` default to `true` or replacing JSON validation with upload blocking — both mechanisms serve different purposes and should coexist.

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-28T00:51:30.926Z
Learning: In WLED PR `#5048`, the NTP-related crash on ESP32-C5 (pioarduino IDF 5.5.x with CONFIG_LWIP_CHECK_THREAD_SAFETY=y) is definitively NOT caused by ntpUdp.begin(). The crash dump shows the real call chain: WLED::loop() → handleTime() → handleNetworkTime() (ntp.cpp:212) → AsyncDNS::query() (asyncDNS.h:45) → dns_gethostbyname_addrtype() → dns_alloc_pcb() → dns_alloc_random_port() → udp_new_ip_type() ← ASSERT. The fix must be in AsyncDNS::query() (asyncDNS.h:45): the call to dns_gethostbyname_addrtype() must be dispatched to the TCPIP thread via tcpip_callback_with_block() on IDF V5+. The ntpUdp.begin() call sites (wled.cpp:926 and set.cpp:562) are NOT the crash location for this particular bug.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@softhack007 I looked at this code in detail, it is not perfect but rejecting "late packets" seems reasonable. if large setups are needed, just disable this option in the sync settings (it is disabled by default), it is not possible to have both, late packet rejection AND a large setup as the sequence number for DDP is limited to 1-15 and to check for late packets, a max window of 7 would be possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DedeHai @coderabbitai I was actually wondering if we could make improvements for #5412 while working on the code.

The DDP spec reads like they wanted to support packet re-transmit within one "push frame", so the sequence counter could be used to identify duplicates (in a history window of 15 packets max). I think dups (same frame) are not a problem in WLED, because they simply cause a re-painting of some pixels. You're right that rejecting late packets - from the previous "push frame" - might be useful in some setups - especially with high latency / unstable WiFi and with lower pixel counts. My fear is that users will simply enable "Skip out-of-sequence packets" (which makes sense for other protocols like Art-Net), and later complain that a bigger setup always misses pixel updates when sequence checks are enabled.

I'm not sure atm where is the "breaking limit" (matrix size) when sequence checking will start to mis-behave. But as this is a legacy "buggy feature" I could live with a use-as-is decision 🤔, so we still support the previous "proper use" cases.

What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@softhack007, here's my analysis of the sequence number situation and whether improvements are practical for this PR.


Exact Breaking-Point Calculation

The rejection window always excludes exactly 4 sequence numbers as "late from previous frame":

  • When lastPushSeq > 5: rejects sn ∈ (lastPushSeq−5, lastPushSeq) — e.g. with push at sn=10, rejects {6,7,8,9}
  • When lastPushSeq ≤ 5: rejects via sn > (10+lastPushSeq) || sn < lastPushSeq — e.g. with push at sn=3, rejects {14,15,1,2}

So: 15 total − 1 (zero/disabled) − 4 (rejected window) = 10 safe data-packet slots + 1 for the push packet itself = 11 packets max per frame without false-rejection.

At xLights' default 1440 bytes/packet:

  • RGB (3 ch): 480 LEDs/packet × 11 = ~5,280 LEDs max
  • RGBW (4 ch): 360 LEDs/packet × 11 = ~3,960 LEDs max

For typical WLED ESP32 setups that max out around 1,000–2,500 LEDs, this is usually fine. The problematic zone is large matrix setups (2,000–8,000+ LEDs with RGBW), which is exactly the target use case for #5412.


Can We Improve It?

Several options with different trade-offs:

Option Window size Safe slots RGB LED limit Con
Current code 4 10+push ~5,280 Fails for large RGBW or >5K RGB installs
Reduce window to 2 2 12+push ~6,240 Less protection against genuinely late packets
Track last-accepted sn (not push sn) 4 10+push ~5,280 Better accuracy but same fundamental limit
Disable DDP seq checking entirely 15 unlimited No protection, consistent with sn=0 path

The fundamental constraint is the DDP spec's 4-bit sequence number (1-15 ring). No algorithm can safely distinguish "late from previous frame" from "early in new frame" with only 15 values and an unbounded number of packets per frame.


Recommendation

Given that:

  1. The feature is disabled by default (e131SkipOutOfSequence), as @DedeHai noted
  2. Duplicates within a frame are indeed harmless (just repaint)
  3. The true late-packet problem (high-latency WiFi) tends to occur on small setups, not large ones that need >10 packets/frame
  4. The breaking limit (~5,280 RGB / ~3,960 RGBW) exceeds most WLED deployments

I'd suggest accepting use-as-is for this PR but adding a comment near the rejection logic that documents the limit, so future contributors understand why the feature can't safely be extended to large setups:

// Note: with a 15-value (1–15) DDP sequence ring and a 4-slot rejection window,
// frames requiring more than ~10 packets may experience false rejections.
// "Skip out-of-sequence packets" (e131SkipOutOfSequence) is disabled by default;
// for large installations it should remain disabled.
if (e131SkipOutOfSequence && lastPushSeq) {

This way the behavior is documented, and users/maintainers know the safe operating envelope without any code risk from changing the window algorithm. The #5412 improvement (if it relates to large-matrix sequence behavior) could then be tracked as a separate, more surgical change.


🏁 Script executed:

#!/bin/bash
gh issue view 5412 --repo wled/WLED 2>/dev/null | head -60

Length of output: 0


🌐 Web query:

WLED github issue 5412 DDP sequence

💡 Result:

GitHub issue wled/WLED #5412 (opened Mar 9, 2026) reports a potential bug in WLED’s DDP packet sequence / out‑of‑order handling when the setting “skip out-of sequence packets” is enabled. [1]

  • In wled00/e131.cpp, WLED uses only the low 4 bits of the DDP sequenceNum (sequenceNum & 0xF). [1]
  • The code comment says it’s “assuming 4 packets max. before push”, and the logic effectively uses a small window (it compares against lastPushSeq and a 5-sequence range) to reject “late packets belonging to previous frame”. [1]
  • The issue concern: if a single “frame” is split across more than ~4–5 DDP packets (e.g., large installations like HUB75 64×64 streaming), then valid packets that arrive slightly late/out-of-order could be discarded, potentially causing corrupted / incomplete LED output. The issue asks to verify whether that “4 packets max before push” assumption is actually valid per DDP expectations. [1]

DDP itself is the UDP protocol WLED listens for on port 4048. [2]

Sources: [1] [2]


@softhack007, confirmed — issue #5412 reports exactly this problem: large installations like HUB75 64×64 streaming split a single frame across more than ~4–5 DDP packets, and the code's "4 packets max before push" assumption causes valid packets to be discarded. My earlier analysis lines up with the issue report.

To summarize the math concisely:

  • The rejection window always excludes exactly 4 sequence numbers (the code checks sn > (lastPushSeq−5) && sn < lastPushSeq, which spans 4 values, not 5).
  • That leaves 10 safe data-packet slots + 1 push packet = 11 total per frame before false rejections begin.
  • At 480 RGB LEDs per 1440-byte DDP packet: ~5,280 RGB / ~3,960 RGBW LEDs is the breaking point.
  • A 64×64 HUB75 matrix = 4,096 pixels — right at the RGBW edge.

For this PR, I'd suggest the pragmatic "use-as-is + document the limit" approach, since:

  1. The feature is off by default (e131SkipOutOfSequence)
  2. Late-packet problems (high-latency WiFi) typically affect small setups, not large ones that need >11 packets/frame
  3. No algorithm can do better given DDP's 4-bit (1–15) sequence ring without cross-frame state

A targeted comment referencing #5412 would help future contributors understand the constraint:

//reject late packets belonging to previous frame (assuming 4 packets max. before push, if more are used and packets are very late, they are still accepted)
// Note: the 4-bit DDP sequence ring (1-15) limits safe use of this feature to frames requiring
// ≤10 DDP packets (~5280 RGB / ~3960 RGBW LEDs at 1440 bytes/packet). See issue `#5412`.
// For larger installations, keep "Skip out-of-sequence packets" (e131SkipOutOfSequence) disabled.
if (e131SkipOutOfSequence && lastPushSeq) {

This keeps the PR focused on the safety fixes it targets, and creates a clear breadcrumb for whoever addresses #5412 properly in a future dedicated PR.


🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.

Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-20T17:38:19.745Z
Learning: In WLED DDP packet handling (wled00/e131.cpp), the destination field should be validated to reject non-display packets. Packets with destination=DDP_ID_STATUS (251) or destination=DDP_ID_CONFIG (250) should be rejected and not processed as LED display data. Only packets with destination=DDP_ID_DISPLAY (1) should be accepted for LED output. WLED doesn't distinguish between multiple display IDs (e.g., display 1 vs display 2), but it must distinguish between DISPLAY packets versus STATUS/CONFIG packets.

Learnt from: softhack007
Repo: wled/WLED PR: 5547
File: wled00/e131.cpp:33-40
Timestamp: 2026-04-28T09:41:31.415Z
Learning: In WLED DDP packet handling (wled00/e131.cpp), the destination filtering uses an explicit deny-list (reject DDP_ID_CONTROL, DDP_ID_STATUS, DDP_ID_CONFIG) rather than an allow-list (only DDP_ID_DISPLAY). This is intentional for maximum compatibility — devices may use non-standard or future destination IDs that should still drive LED output. Do not suggest switching to an allow-list (`p->destination != DDP_ID_DISPLAY`) in future reviews of this code.

Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/set.cpp:551-555
Timestamp: 2026-03-27T21:00:25.902Z
Learning: In WLED PR `#5048` (wled00/set.cpp lines 551-555), the CONFIG_IDF_TARGET_ESP32C5 block that unconditionally forces ntpEnabled = false is an intentional **temporary hotfix** by softhack007 for a known ESP32-C5 crash: `assert failed: udp_new_ip_type udp.c:1278 (Required to lock TCPIP core functionality!)`. Do not flag this as a permanent design issue; the TODO comment in the code already notes it should be resolved properly once the underlying IDF/TCPIP bug on C5 is fixed. A future permanent solution should use a target capability flag rather than overwriting the user's setting.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.

Learnt from: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: softhack007
Repo: wled/WLED PR: 5482
File: wled00/wled.h:579-579
Timestamp: 2026-04-08T21:25:44.777Z
Learning: In WLED PR `#5482` (wled00/wled.h), the `denyWsecUpload` flag is confirmed to default to `false`. The complementary defence against malformed wsec.json content (bootloop/brick risk from malformed JSON) is handled by PR `#5484`, which hardens JSON file reading/parsing at load time. Do not suggest moving `denyWsecUpload` default to `true` or replacing JSON validation with upload blocking — both mechanisms serve different purposes and should coexist.

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: 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.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-13T09:36:13.994Z
Learning: WLED serial streaming frame maxima:
- Adalight: protocol max 65,536 LEDs (count = hi<<8 | lo, plus 1), but WLED’s uint16_t parser caps it at 65,535.
- TPM2 serial (RGB): protocol max payload 65,535 bytes → 21,845 LEDs (bytes/3); effective output always limited by configured LED count and MAX_LEDS.

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: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Learnt from: softhack007
Repo: wled/WLED PR: 5436
File: wled00/fcn_declare.h:394-395
Timestamp: 2026-03-22T00:09:16.015Z
Learning: In WLED (wled00/fcn_declare.h), the `prepareHostname(char* hostname, size_t maxLen)` signature change from the old `prepareHostname(char* hostname)` is intentional and breaking. There is no compatibility shim; any usermod or downstream caller must explicitly pass the buffer size. Do not suggest a default-size shim for this function.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/bus_manager.cpp:549-556
Timestamp: 2026-02-11T16:20:02.872Z
Learning: ESP-IDF V5 requires a complete rewrite of the LEDC (PWM/analog LED) subsystem in WLED. The framework now manages LEDC channels internally (unlike V4 where WLED managed them directly), and dithering functionality may not work as it did before. Direct LEDC struct access (e.g., `LEDC.channel_group[gr].channel[ch].duty.duty`) used in the current implementation will need to be replaced with proper HAL API calls. This affects `wled00/bus_manager.cpp` BusPwm class and related pin manager LEDC allocation code.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR `#4876`, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.

Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.

Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-28T00:51:30.926Z
Learning: In WLED PR `#5048`, the NTP-related crash on ESP32-C5 (pioarduino IDF 5.5.x with CONFIG_LWIP_CHECK_THREAD_SAFETY=y) is definitively NOT caused by ntpUdp.begin(). The crash dump shows the real call chain: WLED::loop() → handleTime() → handleNetworkTime() (ntp.cpp:212) → AsyncDNS::query() (asyncDNS.h:45) → dns_gethostbyname_addrtype() → dns_alloc_pcb() → dns_alloc_random_port() → udp_new_ip_type() ← ASSERT. The fix must be in AsyncDNS::query() (asyncDNS.h:45): the call to dns_gethostbyname_addrtype() must be dispatched to the TCPIP thread via tcpip_callback_with_block() on IDF V5+. The ntpUdp.begin() call sites (wled.cpp:926 and set.cpp:562) are NOT the crash location for this particular bug.

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: 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.

Copy link
Copy Markdown
Member

@softhack007 softhack007 Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so with RGB (not RGBW which is totally uncommon in large displays, and not supported for HUB75), false rejections start above ~ 82x64 pixels.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not perfect and the fix is to uncheck that option, its more a question of documentation. Until looking into this code for this PR I did not even know that option existed :)
We can always refine at a later point if this becomes an issue with growing number of HUB75 users. The key point of this PR is to make DDP as compatible as we dare to at the cost of accepting wrong packets that lead to a screwed up display (which will be rare).

What I am most concerned about in this PR is rejecting valid packets in non DDP protocols, which may be the case if the rabbit is correct in its statement above on artnet packets.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if we can not verify that to be working in a real setup, I'd rather comment out those packet size checks for now as this PR will block 16.0 release until done.

Copy link
Copy Markdown
Member

@softhack007 softhack007 Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. As testing is very hard to do, especially we don't know much about all the possible senders - maybe this PR should strictly focus on "prevent array bounds violations" and on DDP. Which means, any clamping that is not driven by array/buffer bounds should be commented out.

if (sn) {
if (lastPushSeq > 5) {
if (sn > (lastPushSeq -5) && sn < lastPushSeq) return;
Expand All @@ -40,7 +51,8 @@ static void handleDDPPacket(e131_packet_t* p) {
}
}

unsigned ddpChannelsPerLed = ((p->dataType & 0b00111000)>>3 == 0b011) ? 4 : 3; // data type 0x1B (formerly 0x1A) is RGBW (type 3, 8 bit/channel)
unsigned ddpChannelsPerLed = 3; // default to RGB
if ((p->dataType & 0b00111000)>>3 == 0b011) ddpChannelsPerLed = 4; // RGBW data type (see DDP protocol definition)

uint32_t start = htonl(p->channelOffset) / ddpChannelsPerLed;
start += DMXAddress / ddpChannelsPerLed;
Expand All @@ -50,8 +62,14 @@ static void handleDDPPacket(e131_packet_t* p) {
unsigned c = 0;
if (p->flags & DDP_FLAGS_TIME) c = 4; //packet has timecode flag, we do not support it, but data starts 4 bytes later

// ensure the received packet is at least as long as the header claims
if (packetLen < DDP_HEADER_LEN + c + dataLen) {
DEBUG_PRINTLN(F("DDP packet incomplete"));
return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

unsigned numLeds = stop - start; // stop >= start is guaranteed
unsigned maxDataIndex = c + numLeds * ddpChannelsPerLed; // validate bounds before accessing data array
unsigned maxDataIndex = numLeds * ddpChannelsPerLed; // validate bounds before accessing data array
if (maxDataIndex > dataLen) {
DEBUG_PRINTLN(F("DDP packet data bounds exceeded, rejecting."));
return;
Expand All @@ -66,7 +84,6 @@ static void handleDDPPacket(e131_packet_t* p) {
}
}

bool push = p->flags & DDP_FLAGS_PUSH;
ddpSeenPush |= push;
if (!ddpSeenPush || push) { // if we've never seen a push, or this is one, render display
e131NewData = true;
Expand All @@ -76,32 +93,41 @@ static void handleDDPPacket(e131_packet_t* p) {
}

//E1.31 and Art-Net protocol support
void handleE131Packet(e131_packet_t* p, IPAddress clientIP, byte protocol){
void handleE131Packet(e131_packet_t* p, IPAddress clientIP, byte protocol, size_t packetLen){

int uni = 0, dmxChannels = 0;
uint8_t* e131_data = nullptr;
int seq = 0, mde = REALTIME_MODE_E131;

if (protocol == P_ARTNET)
{
if (packetLen < 10) return; // need at least art_opcode (offset 8, 2 bytes)
if (p->art_opcode == ARTNET_OPCODE_OPPOLL) {
handleArtnetPollReply(clientIP);
return;
}
if (packetLen < 18) return; // need art_length (offset 16, 2 bytes) for DMX data
uni = p->art_universe;
dmxChannels = htons(p->art_length);
const int artNetMaxData = (packetLen >= 18) ? (int)(packetLen - 18) : 0; // art_data at offset 18; clamp so e131_data[dmxChannels] stays in bounds
if (dmxChannels > artNetMaxData) dmxChannels = artNetMaxData;
if (dmxChannels > MAX_CHANNELS_PER_UNIVERSE) dmxChannels = MAX_CHANNELS_PER_UNIVERSE;
e131_data = p->art_data;
Comment thread
DedeHai marked this conversation as resolved.
seq = p->art_sequence_number;
mde = REALTIME_MODE_ARTNET;
} else if (protocol == P_E131) {
if (packetLen < 126) return; // need up to property_values[0] (offset 125) and property_value_count (offset 123)
// Ignore PREVIEW data (E1.31: 6.2.6)
if ((p->options & 0x80) != 0) return;
dmxChannels = htons(p->property_value_count) - 1;
dmxChannels = htons(p->property_value_count) - 1; // on malformed packets, this can become negative, checked below
// DMX level data is zero start code. Ignore everything else. (E1.11: 8.5)
if (dmxChannels == 0 || p->property_values[0] != 0) return;
if (dmxChannels <= 0 || p->property_values[0] != 0) return;
uni = htons(p->universe);
e131_data = p->property_values;
seq = p->sequence_number;
const int e131MaxData = (packetLen > 126) ? (int)(packetLen - 126) : 0; // property_values at offset 125; clamp so e131_data[dmxChannels] stays in bounds
if (dmxChannels > e131MaxData) dmxChannels = e131MaxData;
if (dmxChannels > MAX_CHANNELS_PER_UNIVERSE) dmxChannels = MAX_CHANNELS_PER_UNIVERSE;
if (e131Priority != 0) {
if (p->priority < e131Priority ) return;
// track highest priority & skip all lower priorities
Expand All @@ -110,15 +136,17 @@ void handleE131Packet(e131_packet_t* p, IPAddress clientIP, byte protocol){
}
} else { //DDP
realtimeIP = clientIP;
handleDDPPacket(p);
handleDDPPacket(p, packetLen);
return;
}

#ifdef WLED_ENABLE_DMX
// does not act on out-of-order packets yet
if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) {
// Art-Net: art_data is 0-indexed (channel 1 at index 0)
// E1.31: property_values[0] is start code, (channel 1 at index 1)
for (uint16_t i = 1; i <= dmxChannels; i++)
dmx.write(i, e131_data[i]);
dmx.write(i, mde == REALTIME_MODE_ARTNET ? e131_data[i-1] : e131_data[i]);
dmx.update();
}
#endif
Expand Down
2 changes: 1 addition & 1 deletion wled00/fcn_declare.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ void initDMXInput();
void handleDMXInput();

//e131.cpp
void handleE131Packet(e131_packet_t* p, IPAddress clientIP, byte protocol);
void handleE131Packet(e131_packet_t* p, IPAddress clientIP, byte protocol, size_t packetLen);
void handleDMXData(uint16_t uni, uint16_t dmxChannels, uint8_t* e131_data, uint8_t mde, uint8_t previousUniverses);
// void handleArtnetPollReply(IPAddress ipAddress); // local function, only used in e131.cpp
// void prepareArtnetPollReply(ArtPollReply* reply); // local function, only used in e131.cpp
Expand Down
57 changes: 32 additions & 25 deletions wled00/src/dependencies/e131/ESPAsyncE131.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
#include "../network/Network.h"
#include <string.h>

// E1.17 ACN Packet Identifier
// E1.17 ACN Packet Identifier "ASC-E1.17"
const byte ESPAsyncE131::ACN_ID[12] = { 0x41, 0x53, 0x43, 0x2d, 0x45, 0x31, 0x2e, 0x31, 0x37, 0x00, 0x00, 0x00 };

// Art-Net Packet Identifier
// Art-Net Packet Identifier "Art-Net"
const byte ESPAsyncE131::ART_ID[8] = { 0x41, 0x72, 0x74, 0x2d, 0x4e, 0x65, 0x74, 0x00 };

// Constructor
Expand Down Expand Up @@ -99,36 +99,43 @@ bool ESPAsyncE131::initMulticast(uint16_t port, uint16_t universe, uint8_t n) {

void ESPAsyncE131::parsePacket(AsyncUDPPacket _packet) {
bool error = false;
uint8_t protocol = P_E131;
uint8_t protocol = P_ARTNET;
const size_t pktLen = _packet.length();

e131_packet_t *sbuff = reinterpret_cast<e131_packet_t *>(_packet.data());

//E1.31 packet identifier ("ACS-E1.17")
if (memcmp(sbuff->acn_id, ESPAsyncE131::ACN_ID, sizeof(sbuff->acn_id)))
protocol = P_ARTNET;

if (protocol == P_ARTNET) {
if (memcmp(sbuff->art_id, ESPAsyncE131::ART_ID, sizeof(sbuff->art_id)))
error = true; //not "Art-Net"
if (sbuff->art_opcode != ARTNET_OPCODE_OPDMX && sbuff->art_opcode != ARTNET_OPCODE_OPPOLL)
error = true; //not a DMX or poll packet
} else { //E1.31 error handling
if (htonl(sbuff->root_vector) != ESPAsyncE131::VECTOR_ROOT)
error = true;
if (htonl(sbuff->frame_vector) != ESPAsyncE131::VECTOR_FRAME)
error = true;
if (sbuff->dmp_vector != ESPAsyncE131::VECTOR_DMP)
error = true;
if (sbuff->property_values[0] != 0)
error = true;
}


// E1.31 packet identifier (ACN_ID = "ASC-E1.17"), need at least 16 bytes to safely read acn_id (offset 4, length 12).
if (pktLen >= 16) {
if (!memcmp(sbuff->acn_id, ESPAsyncE131::ACN_ID, sizeof(sbuff->acn_id)))
protocol = P_E131;
}

if (protocol == P_ARTNET) {
if (memcmp(sbuff->art_id, ESPAsyncE131::ART_ID, sizeof(sbuff->art_id)))
error = true; //not ART_ID = "Art-Net"
if (sbuff->art_opcode != ARTNET_OPCODE_OPDMX && sbuff->art_opcode != ARTNET_OPCODE_OPPOLL)
error = true; //not a DMX or poll packet
} else { //E1.31 error handling
if (pktLen < 126) { // need up to property_values[0] at offset 125
error = true;
} else {
if (htonl(sbuff->root_vector) != ESPAsyncE131::VECTOR_ROOT)
error = true;
if (htonl(sbuff->frame_vector) != ESPAsyncE131::VECTOR_FRAME)
error = true;
if (sbuff->dmp_vector != ESPAsyncE131::VECTOR_DMP)
error = true;
if (sbuff->property_values[0] != 0)
error = true;
}
}

if (error && _packet.localPort() == DDP_DEFAULT_PORT) { //DDP packet
error = false;
protocol = P_DDP;
}

if (!error) {
_callback(sbuff, _packet.remoteIP(), protocol);
_callback(sbuff, _packet.remoteIP(), protocol, pktLen);
}
}
21 changes: 13 additions & 8 deletions wled00/src/dependencies/e131/ESPAsyncE131.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,24 @@ typedef struct ip_addr ip4_addr_t;
#define DDP_FLAGS_VER 0xc0 // version mask
#define DDP_FLAGS_VER1 0x40 // version=1
#define DDP_FLAGS_PUSH 0x01
#define DDP_FLAGS_QUERY 0x02
#define DDP_FLAGS_REPLY 0x04
#define DDP_FLAGS_STORAGE 0x08
#define DDP_FLAGS_QUERY 0x02 // unsupported - used by XLights for auto-discovery
#define DDP_FLAGS_REPLY 0x04 // unsupported - response packet from another display
#define DDP_FLAGS_STORAGE 0x08 // unsupported - show data from a storage unit instead of from packet data field. Data field defines storage unit (by name, number, URL or whatever mechanism wanted).
#define DDP_FLAGS_TIME 0x10

#define DDP_CHANNELS_PER_PACKET 1440 // 480 leds

#define DDP_TYPE_RGB24 0x0B // 00 001 011 (RGB , 8 bits per channel, 3 channels)
#define DDP_TYPE_RGBW32 0x1B // 00 011 011 (RGBW, 8 bits per channel, 4 channels)
#define DDP_TYPE_LEGACY 0x01 // 00 000 001 legacy RGB 8bit definition
Comment thread
DedeHai marked this conversation as resolved.
#define DDP_TYPE_UNDEF 0x00 // type and bit depth undefined

#define DDP_ID_DISPLAY 1
#define DDP_ID_CONFIG 250
#define DDP_ID_STATUS 251
// DDP Source or Destination ID (header byte 3)
#define DDP_ID_DISPLAY 1 // default output device
#define DDP_ID_CONTROL 246 // JSON control (not implemented)
#define DDP_ID_CONFIG 250 // JSON config (not implemented)
#define DDP_ID_STATUS 251 // JSON status (not implemented)
#define DDP_ID_ALL 255 // all devices

#define ARTNET_OPCODE_OPDMX 0x5000
#define ARTNET_OPCODE_OPPOLL 0x2000
Expand Down Expand Up @@ -212,7 +217,7 @@ typedef union {
} ArtPollReply;

// new packet callback
typedef void (*e131_packet_callback_function) (e131_packet_t* p, IPAddress clientIP, byte protocol);
typedef void (*e131_packet_callback_function) (e131_packet_t* p, IPAddress clientIP, byte protocol, size_t packetLen);

class ESPAsyncE131 {
private:
Expand Down Expand Up @@ -267,4 +272,4 @@ class E131Priority {
}
};

#endif // ESPASYNCE131_H_
#endif // ESPASYNCE131_H_
1 change: 1 addition & 0 deletions wled00/udp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,7 @@ uint8_t realtimeBroadcast(uint8_t type, IPAddress client, uint16_t length, const

// write the header
/*0*/ddpUdp.write(flags);
// TODO: sequence number should be 1-15 as 0 means "unused", it has no bad consequences other than out of sequence packet may be accepted
/*1*/ddpUdp.write(sequenceNumber++ & 0x0F); // sequence may be unnecessary unless we are sending twice (as requested in Sync settings)
/*2*/ddpUdp.write(isRGBW ? DDP_TYPE_RGBW32 : DDP_TYPE_RGB24);
/*3*/ddpUdp.write(DDP_ID_DISPLAY);
Expand Down
12 changes: 3 additions & 9 deletions wled00/ws.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,13 @@ void wsEvent(AsyncWebSocket * server, AsyncWebSocketClient * client, AwsEventTyp
if (!data || len < offset+1) return; // catch invalid / single-byte payload
switch (data[0]) {
case BINARY_PROTOCOL_E131:
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_E131);
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_E131, len - offset);
break;
case BINARY_PROTOCOL_ARTNET:
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_ARTNET);
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_ARTNET, len - offset);
break;
case BINARY_PROTOCOL_DDP:
if (len < 10 + offset) return; // DDP header is 10 bytes (+1 protocol byte)
size_t ddpDataLen = (data[8+offset] << 8) | data[9+offset]; // data length in bytes from DDP header
uint8_t flags = data[0+offset];
if ((flags & DDP_FLAGS_TIME) ) ddpDataLen += 4; // timecode flag adds 4 bytes to data length
if (len < (10 + offset + ddpDataLen)) return; // not enough data, prevent out of bounds read
// could be a valid DDP packet, forward to handler
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_DDP);
handleE131Packet((e131_packet_t*)&data[offset], client->remoteIP(), P_DDP, len - offset);
}
}
} else {
Expand Down
Loading