less restrictive DDP header acceptance, add safety checks to all protocols#5547
less restrictive DDP header acceptance, add safety checks to all protocols#5547
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThreads packet-length through UDP/WS parsing and handlers, adds guarded header/length checks and bounds clamping for ACN/Art‑Net/E1.31/DDP, relaxes some DDP type/filtering rules, updates DDP constants and function signatures, and adds minor comment/TODO edits. (≤50 words) Changes
Sequence DiagramsequenceDiagram
participant Client
participant AsyncE131 as ESPAsyncE131::parsePacket
participant Handler as handleE131Packet
participant Protocol as E1.31/Art-Net/DDP Logic
Client->>AsyncE131: UDP datagram or WS binary arrives
AsyncE131->>AsyncE131: compute pktLen (payload length)
alt pktLen >= minimal header
AsyncE131->>AsyncE131: guarded ACN/Art‑Net/E1.31 header checks
AsyncE131->>Handler: invoke callback (packetPtr, clientIP, protocol, pktLen)
Handler->>Protocol: validate headers using packetLen, clamp counts
Protocol->>Protocol: safely process DMX/DDP payload (filtering/type handling)
else
AsyncE131-->>Client: drop/ignore packet
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
@netmindz the rabbit seems content with the changes, I did test DDP and artnet from ESP to ESP and it is working but I don't know if there are any edge cases. In theory, the size checks are safe. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/dependencies/e131/ESPAsyncE131.cpp`:
- Line 106: Fix the typo in the inline comment that currently reads "ACS-E1.17"
— it should be "ASC-E1.17"; update the comment near the E1.31 packet identifier
(the line containing E1.31 packet identifier ("ACS-E1.17"), need at least 16
bytes to safely read acn_id) in ESPAsyncE131.cpp so the identifier matches the
spec and the existing "E1.17 ACN Packet Identifier" comment.
🪄 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: 98aee980-c791-400c-b730-199dc0cd6290
📒 Files selected for processing (1)
wled00/src/dependencies/e131/ESPAsyncE131.cpp
|
@DedeHai we might also need to reject "query" packets - it seems that some frontends have a kind of "auto-discovery" feature that might cause LEDs flicker if we interpret the content as pixels. http://www.3waylabs.com/ddp/#Discovery Like this (over-restrictive) example MoonModules@579477c Edit: its not clear to me (from reading specs) if these QUERY/REPLY/STORAGE packets will always have a non-display destination. |
|
|
good point - maybe better to reject explicitly, unless we are 100% sure (from the specs) that these packets will always be 10 bytes only. |
|
@DedeHai I found another 10 bytes "message" - DDP sync. Its basically a "push" with dataLen=0. Do you think we already handle that one properly? If we miss it, that could lead to dropped frames. // sent data to all devices, now broadcast PUSH flag so they all display sync'd
if (NDEVICES > 1) // if 1 device, already sent PUSH above
{
dh.flags1 = DDP_FLAGS1_VER1 | DDP_FLAGS1_PUSH;
dh.id = DDP_ID_DISPLAY;
dh.offset = 0;
dh.len = 0; // <--- !!
UDP_SEND(local-IP-broadcast-address,DDP_PORT,dh,databuf);
} |
agreed.
if a software sends a query packet that contains LED data it also needs to specify the data length. I am not sure ther eis a mechanism that is a non-config packet that contains data AND is valid according to the protocol. It's certainly possible to construct such a bogus message - and if you do, you have to live with the consequences ;) |
@coderabbitai please check if such a "push only" message is handled correctly (I suspect it is not). Also do a cross-check on known projects that use DDP like hyperion, xlights, signalRGB and check if the code from this PR handles all messages including the actual RGB data correctly. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the actual DDP handling code from the PR branch and also look up DDP behavior in the known projects. I'm still waiting for the script results. Let me also look up how xLights specifically handles its DDP PUSH logic:
🔍 Push-Only (PUSH+dataLen=0) Packet HandlingThe DDP spec defines the PUSH flag as "for display synchronization, or marks last packet of Reply." The spec's own multi-device example sends a separate broadcast PUSH packet with The issue: Based on the PR's changes as summarized, when a packet arrives with
So the PUSH-only sync packet is silently discarded — the PUSH flag itself is never checked for the "trigger display now" semantic. This was likely already broken before this PR and this PR doesn't fix it.
🔄 Compatibility with Known DDP Senders
LedFx uses PUSH flag on the last packet of a frame for single-device, and for multiple devices: all devices receive their data, then a separate PUSH is sent. 💡 Suggested FixTo properly handle push-only packets, the DDP handler should check the PUSH flag when if (dataLen == 0) {
if (p->flags1 & DDP_FLAGS1_PUSH) {
// sync-push packet: flush any previously buffered data to display
realtimeApplyToAllParts(); // or the equivalent display-trigger call
}
return; // nothing else to do
}This would match the spec intent and correctly handle multi-device sync from xLights/FPP. For single-device senders that embed PUSH in the last data packet, the existing path (which already triggers display after processing the LED data) remains unchanged.
🏁 Script executed: Length of output: 2853 🏁 Script executed: Length of output: 3622 🏁 Script executed: Length of output: 7450 🏁 Script executed: Length of output: 22222 🏁 Script executed: Length of output: 9188 🌐 Web query: 💡 Result: In DDP, the PUSH flag (bit 0 of header byte 0) marks the final packet in a sequence / frame—i.e., “now apply (latch) what you’ve received.” [1] So a “sync” / “push-only” packet with
Implementation note: common DDP parsers/libraries accept zero-length payloads as valid (i.e., they parse the header and produce an empty data vector), which enables this “push-only” usage. [3] Sources: Citations:
✅ Push-Only Packet (PUSH flag +
|
| Step | Code (e131.cpp) | Outcome |
|---|---|---|
| Line 24 | if (packetLen < DDP_HEADER_LEN) |
10 ≥ 10 → passes |
| Line 34 | destination check | DDP_ID_DISPLAY (1) not in reject list → passes |
| Line 53–54 | dataLen=0, stop = start + 0/3 = 0 |
both start and stop = 0 |
| Line 60 | packetLen < DDP_HEADER_LEN + 0 + 0 |
10 < 10 → false → passes |
| Line 65–68 | numLeds=0, maxDataIndex=0, 0 > 0 |
false → passes |
| Line 73 | realtimeLock(realtimeTimeoutMs, REALTIME_MODE_DDP) |
✅ realtime mode locked |
| Lines 76–78 | for loop i = 0; i < 0 |
zero iterations — correct, data was in earlier packets |
| Line 81 | bool push = p->flags & DDP_FLAGS_PUSH |
push = true |
| Line 82 | `ddpSeenPush | = push` |
| Line 83 | if (!ddpSeenPush || push) |
(false || true) = true |
| Line 84 | e131NewData = true |
✅ display IS triggered |
The ddpSeenPush state machine is actually well-designed:
- Before first PUSH seen: every data packet triggers a render (correct for single-packet-per-frame senders like LedFx/Hyperion/SignalRGB)
- After first PUSH seen: only PUSH packets trigger a render (correct buffered/multi-device sync mode for xLights/FPP)
📊 Cross-check: Known DDP Senders
The DDP PUSH flag marks the final packet in a frame for display synchronization. In multi-device setups, all devices receive their pixel data first, then a broadcast PUSH (with dataLen=0) synchronizes the display update. Common DDP parsers accept zero-length payloads as valid for this purpose.
| Sender | PUSH Pattern | Works with PR? |
|---|---|---|
| LedFx | PUSH on last data packet | ✅ Yes — hits push=true path with actual data |
| xLights | Separate PUSH-only broadcast (dataLen=0) |
✅ Yes — e131NewData=true still triggered |
| FPP (multi-device master) | Separate PUSH-only broadcast (dataLen=0) |
✅ Yes — same as xLights |
| Hyperion | PUSH on last data packet per device | ✅ Yes — standard path |
| SignalRGB | PUSH on data packets | ✅ Yes — standard path |
⚠️ Minor Remaining Gap: Discovery/Query Packets
Discovery packets (DDP_FLAGS_QUERY set, dataLen=0) are not explicitly rejected. In the initial state (ddpSeenPush=false), they would pass all checks, write 0 pixels, and — if no PUSH is set — trigger no render (because !ddpSeenPush || push = true || false = true, actually it WOULD set e131NewData=true for initial state only). After the first PUSH is seen, discovery packets are silently ignored (no render).
This is mostly harmless (0 pixels written), but as softhack007 noted, an explicit early return is cleaner:
// reject query/discovery packets (no display data)
if (p->flags & DDP_FLAGS_QUERY) return;Bottom line: The push-only sync packet is not dropped — frames will not be missed. The ddpSeenPush state machine correctly handles both single-device and multi-device sync patterns. 🎉
🧠 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: 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: 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: 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: 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: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T07:26:14.510Z
Learning: In WLED (wled00/util.cpp), `allocate_buffer()` processes `BFRALLOC_NOBYTEACCESS` in an `if` with an `else` that covers all other flags. On classic ESP32, this means `BFRALLOC_ENFORCE_PSRAM` and `BFRALLOC_PREFER_PSRAM` are silently ignored when `BFRALLOC_NOBYTEACCESS` is also set — IRAM (MALLOC_CAP_32BIT) is always tried first. Combining `BFRALLOC_CLEAR | BFRALLOC_NOBYTEACCESS` is unsafe: the allocator's internal `memset` at util.cpp:901 will perform byte-level access on a potentially IRAM-resident buffer. This affects `Segment::pixels` in `setGeometry()` (FX.h) and `_pixels` in `finalizeInit()` (FX_fcn.cpp). Additionally, `memcpy` on `Segment::pixels` in the copy constructor and copy assignment (FX_fcn.cpp lines ~73 and ~116) is also a byte-level violation when the buffer is in IRAM. The safe pattern is element-wise 32-bit access (for loops over `uint32_t*`) rather than `memset`/`memcpy` for all `BFRALLOC_NOBYTEACCESS` buffers.
Learnt from: DedeHai
Repo: wled/WLED PR: 5464
File: wled00/FX_fcn.cpp:1699-1701
Timestamp: 2026-04-09T09:14:32.589Z
Learning: In WLED, pixel buffers allocated with `BFRALLOC_NOBYTEACCESS` (both `WS2812FX::_pixels` and `Segment::pixels`) are intentionally placed in 32-bit-only accessible IRAM by design. Buffers up to ~1000 LEDs (~4 kB) easily fit within IRAM's ~50 kB pool. `memset` and `memcpy` on these buffers work correctly in practice because the Xtensa newlib assembly implementations use 32-bit stores for word-aligned, word-multiple-size buffers — satisfying the IRAM hardware constraint as an implementation property of the assembly code, which is unlikely to change across IDF versions. The risk of a future violation exists but is accepted as very low. Do not flag `memset`/`memcpy` usage on `BFRALLOC_NOBYTEACCESS` pixel buffers as a critical bug in WLED reviews; note it as a low-risk accepted pattern.
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: 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: 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. 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e131.cpp`:
- Around line 33-40: Replace the current destination/flag checks so only display
DDP packets are allowed: instead of permitting unknown destinations and letting
STORAGE pass when PUSH is set, change the logic to return early unless
p->destination == DDP_ID_DISPLAY, unconditionally return if p->flags &
DDP_FLAGS_STORAGE, then set bool push = p->flags & DDP_FLAGS_PUSH; (i.e., accept
only DDP_ID_DISPLAY and always reject DDP_FLAGS_STORAGE while still reading
DDP_FLAGS_PUSH). Use the existing symbols p, DDP_ID_DISPLAY, DDP_FLAGS_STORAGE
and DDP_FLAGS_PUSH to locate and update the checks.
- Around line 112-113: The current clamp uses artNetMaxData = (packetLen > 19) ?
packetLen - 19 : 0 which wrongly drops the final Art‑Net slot; change the
computation to use packetLen - 18 (art_data at offset 18) so artNetMaxData =
(packetLen > 18) ? packetLen - 18 : 0, and do not mutate dmxChannels here;
instead enforce bounds when copying into the E1.31 buffer (e.g., limit the copy
length or clamp the index using min(dmxChannels, artNetMaxData) where e131_data
is written) so you protect against overruns without shrinking valid Art‑Net
packets.
🪄 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: 914695a5-8edd-407a-8be9-6a1ba0bea18f
📒 Files selected for processing (2)
wled00/e131.cppwled00/src/dependencies/e131/ESPAsyncE131.h
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/src/dependencies/e131/ESPAsyncE131.h
| //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 |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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 atsn = 9 - Packets with
sn = 7andsn = 8from 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
-
Wrong rejections are possible: For frames requiring >~10 packets with sequence numbers enabled, valid new-frame packets whose
snfalls in the 5-slot window behindlastPushSeq(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. -
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.
-
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 = 0for large installations. - Least risk: If in doubt, the entire out-of-sequence rejection for DDP could be disabled (set
sn = 0in 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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
🧩 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: rejectssn ∈ (lastPushSeq−5, lastPushSeq)— e.g. with push at sn=10, rejects {6,7,8,9} - When
lastPushSeq ≤ 5: rejects viasn > (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:
- The feature is disabled by default (
e131SkipOutOfSequence), as@DedeHainoted - Duplicates within a frame are indeed harmless (just repaint)
- The true late-packet problem (high-latency WiFi) tends to occur on small setups, not large ones that need >10 packets/frame
- 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 DDPsequenceNum(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
lastPushSeqand 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:
- The feature is off by default (
e131SkipOutOfSequence) - Late-packet problems (high-latency WiFi) typically affect small setups, not large ones that need >11 packets/frame
- 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.
There was a problem hiding this comment.
Ok, so with RGB (not RGBW which is totally uncommon in large displays, and not supported for HUB75), false rejections start above ~ 82x64 pixels.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@coderabbitai is this code also used for artnet? if so is it correct? if its incorrect, is this a long standing bug? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22514
Why it works for E1.31For E1.31,
So Why it is wrong for Art-NetFor Art-Net,
So
DMX channel 1 ( Is it a long-standing bug?Yes. The loop itself was not introduced by this PR — only the The fix would be: `#ifdef` WLED_ENABLE_DMX
if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) {
if (protocol == P_ARTNET) {
// Art-Net: art_data is 0-indexed (channel 1 at index 0)
for (uint16_t i = 0; i < dmxChannels; i++)
dmx.write(i + 1, e131_data[i]);
} else {
// E1.31: property_values[0] is start code, channels 1..N at index 1..N
for (uint16_t i = 1; i <= dmxChannels; i++)
dmx.write(i, e131_data[i]);
}
dmx.update();
}
`#endif`Would you like to open a follow-up issue to track this, or address it directly in this PR? 🧠 Learnings used |
|
@netmindz please check the off by one bug I found, are you aware of any reports on that? I do not use dmx so don't know what it means really or how the bug would manifest. |
@coderabbitai can this condition be true in Art-Net mode? Please also check the Art-Net specs to clarify if the protocol supports DMX proxy operation. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the code and check the Art-Net spec. 🏁 Script executed: Length of output: 22514 🌐 Web query: 💡 Result: In Art‑Net, a DMX “universe” on the network is identified by a 15‑bit Port‑Address (range 0–32767) made of Net + Sub‑Net + Universe. (art-net.org.uk) Port‑Address (universe) structure
Where it appears in ArtDMX packetsArt‑Net carries DMX512 data in ArtDMX packets, which include:
Common interoperability gotcha (off‑by‑one)The spec’s Universe/Sub‑Net/Net values are 0‑based, but some controllers/UIs display universes as 1‑based; when configuring a proxy, confirm whether “Universe 1” in the UI means Universe=0 or Universe=1 in Art‑Net terms. (art-net.org.uk) Citations:
Can the proxy condition fire in Art-Net mode?Yes, definitively. For Art-Net packets, // Art-Net path:
uni = p->art_universe; // <-- uni set here
...
mde = REALTIME_MODE_ARTNET;
// ... then, unconditionally for both Art-Net and E1.31:
`#ifdef` WLED_ENABLE_DMX
if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) {
for (uint16_t i = 1; i <= dmxChannels; i++)
dmx.write(i, e131_data[i]); // <-- fires for Art-Net too!
dmx.update();
}
`#endif`DDP returns early before reaching this block; E1.31 and Art-Net both fall through to it. Does Art-Net support DMX proxy operation?Art-Net is specifically designed to carry DMX512 data over IP — its ArtDMX packets contain raw DMX512 channel data addressed by a 15-bit Port-Address (Net + Sub-Net + Universe). So using Art-Net as a source for DMX proxy output is not only valid but is a primary use case of the protocol. The problem when proxy fires in Art-Net modeWhen the proxy fires for Art-Net,
For E1.31, This is a pre-existing bug (not introduced by this PR). Would you like me to open a follow-up issue to track it? 🧠 Learnings used |
|
@DedeHai I would lean towards fixing the Art-Net off-by-one in DMX proxy mode, as suggested by the rabbit. To test this, we would need to have a UART DMX fixture that is driven by an Art-Net control desk over network, WLED acting as a proxy between them - so its almost impossible to test without buying semi-professional DMX equipment, plus understanding the logic of the control desk. I still think that using WLED as a WiFi-to-UART router is an abuse, but hey why not ;-) |
@coderabbitai please trace back the commit history and identify the commit (and related PR) that introduced the DMX proxy loop. Also check for old issue tickets related to the change. Was Art-Net support already implemented when the DMX proxy output code was added? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 155 🏁 Script executed: Length of output: 2012 🏁 Script executed: Length of output: 889 🏁 Script executed: Length of output: 2650
📜 Commit HistoryThe DMX proxy loop was introduced in commit The commit message explicitly advertised both E1.31 and Art-Net from day one:
Art-Net support was already present in A later commit 🐛 Existing Issue TicketsIssue Interestingly, one commenter on Summary
Would you like me to open a dedicated follow-up issue for the Art-Net DMX proxy channel off-by-one bug? 🧠 Learnings used |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/e131.cpp (1)
121-123:⚠️ Potential issue | 🔴 CriticalPrevent negative
dmxChannelsfrom malformed E1.31property_value_count.At Line 121,
htons(p->property_value_count) - 1can become-1when the field is0.
That bypasses Line 123 (dmxChannels == 0) and later gets passed tohandleDMXData(uint16_t dmxChannels)at Line 167 as65535, risking OOB reads.💡 Suggested fix
- dmxChannels = htons(p->property_value_count) - 1; + const uint16_t propertyValueCount = htons(p->property_value_count); + if (propertyValueCount == 0) return; // malformed packet + dmxChannels = propertyValueCount - 1; // 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;Also applies to: 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/e131.cpp` around lines 121 - 123, The code computes dmxChannels as htons(p->property_value_count) - 1 which underflows when property_value_count is 0; update the logic to guard against zero before subtracting (e.g., check if p->property_value_count == 0 or if htons(...) == 0 and return) so dmxChannels never becomes 0xFFFF and is never passed to handleDMXData; adjust the block around the computation of dmxChannels (the line using htons(p->property_value_count) and the subsequent if that checks p->property_values[0]) to perform the zero check first and only compute dmxChannels when safe.
♻️ Duplicate comments (1)
wled00/e131.cpp (1)
146-147:⚠️ Potential issue | 🟠 MajorDMX proxy loop still drops the final channel.
At Line 146, the loop condition
i < dmxChannelsomits channeldmxChannels(last valid DMX slot), so proxy output is truncated by one channel.💡 Suggested fix
- for (uint16_t i = 1; i < dmxChannels; i++) + for (uint16_t i = 1; i <= dmxChannels; i++) dmx.write(i, mde == REALTIME_MODE_ARTNET ? e131_data[i-1] : e131_data[i]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/e131.cpp` around lines 146 - 147, The DMX proxy loop in e131.cpp currently iterates "for (uint16_t i = 1; i < dmxChannels; i++)" which omits the final DMX slot; update the loop condition to include the last channel (e.g., change to use <= so it iterates through channel dmxChannels) and keep the existing indexing logic around dmx.write(...) that selects e131_data[i-1] for REALTIME_MODE_ARTNET and e131_data[i] otherwise; ensure you adjust only the loop condition so dmx.write, dmxChannels, e131_data, mde and REALTIME_MODE_ARTNET remain consistent and in-bounds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wled00/e131.cpp`:
- Around line 121-123: The code computes dmxChannels as
htons(p->property_value_count) - 1 which underflows when property_value_count is
0; update the logic to guard against zero before subtracting (e.g., check if
p->property_value_count == 0 or if htons(...) == 0 and return) so dmxChannels
never becomes 0xFFFF and is never passed to handleDMXData; adjust the block
around the computation of dmxChannels (the line using
htons(p->property_value_count) and the subsequent if that checks
p->property_values[0]) to perform the zero check first and only compute
dmxChannels when safe.
---
Duplicate comments:
In `@wled00/e131.cpp`:
- Around line 146-147: The DMX proxy loop in e131.cpp currently iterates "for
(uint16_t i = 1; i < dmxChannels; i++)" which omits the final DMX slot; update
the loop condition to include the last channel (e.g., change to use <= so it
iterates through channel dmxChannels) and keep the existing indexing logic
around dmx.write(...) that selects e131_data[i-1] for REALTIME_MODE_ARTNET and
e131_data[i] otherwise; ensure you adjust only the loop condition so dmx.write,
dmxChannels, e131_data, mde and REALTIME_MODE_ARTNET remain consistent and
in-bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e146e157-2684-4871-a266-1055524579df
📒 Files selected for processing (1)
wled00/e131.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e131.cpp`:
- Around line 111-114: Clamp dmxChannels to the DMX universe maximum before any
DMX proxy write: after computing dmxChannels and artNetMaxData (variables
dmxChannels and artNetMaxData in the Art-Net path) set dmxChannels =
min(dmxChannels, 512) (apply the same cap in the E1.31 path where
dmxChannels/e131_data are set at the second block around the e131 handling).
Ensure this cap is applied before any loop or call that invokes
SparkFunDMX::write()/dmx.write() so channel indices never exceed 512.
🪄 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: aefe9a70-2755-4120-8406-bbe4f9e4b8bf
📒 Files selected for processing (1)
wled00/e131.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e131.cpp`:
- Around line 65-69: The initial packet-length check is double-counting the
optional 4-byte DDP timecode by adding `c` to `dataLen`; update the guard to not
add `c` (e.g., require packetLen >= DDP_HEADER_LEN + dataLen) or remove this
redundant check and rely on the existing `maxDataIndex = c + numLeds *
ddpChannelsPerLed` bounds check to reject undersized packets; modify the code
around `packetLen`, `DDP_HEADER_LEN`, `c`, `dataLen`, and `maxDataIndex` so that
`c` is only considered once (it may be included in `dataLen` when
`DDP_FLAGS_TIME` is set).
🪄 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: 1fdc6714-2c42-4bc6-91bb-fea67783fba2
📒 Files selected for processing (1)
wled00/e131.cpp
@DedeHai I changed my mind (not literally 😉) - fixing the bug now would add a new incompatibility with previous versions, as Art-Net output channels would move one to the left. Let's keep this as-is, and fix it later in which is a complete re-write of the DMX output driver. |
|
@softhack007 how about keeping this PR for main and I'll open a DDP changes only version for the 16 branch? |
|
@DedeHai good idea 👍 |
instead of being restrictive about the DDP protocol WLED now accepts malformed headers again and does packet size checking instead to prevent crashes.
also adding packet size checking for other protocols.
fix for #5532
Summary by CodeRabbit
Bug Fixes
New Features
Chores