Fixed bug of ESP32 crashing when using DMX out#5316
Open
Fixed bug of ESP32 crashing when using DMX out#5316
Conversation
Fixed bug of ESP32 crashing when using DMX out: -When exceeding the DMX channel limit, the SparkFunDMX.cpp tried to write to static chanSize because the variables where swapped. -When exceeding the channel upper & lower limit now, the value is ignored, previous implementation "idea" was to set as channel 512 value, which is not correct. -There are still some non-optimal parts in the lib, but it at least works for now.
Contributor
WalkthroughReworks DMX channel handling in SparkFunDMX.cpp: replaces defaultMax fallback with dmxMaxChannel in initRead/initWrite, changes read() to return 0 for out-of-range channels, and simplifies write() to validate channel and directly assign into dmxData. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp`:
- Around line 129-132: The write method in SparkFunDMX::write has an off-by-one
bounds check allowing Channel == chanSize which writes past the end of dmxData;
update the condition to reject Channel values >= chanSize (i.e., ensure valid
Channel is 1..chanSize-1) so accesses to dmxData[Channel] cannot overflow;
modify the check that currently uses (Channel > chanSize) to use (Channel >=
chanSize) (referencing SparkFunDMX::write, Channel, chanSize, chanQuant and
dmxData).
Comment on lines
129
to
132
| void SparkFunDMX::write(int Channel, uint8_t value) { | ||
| if (Channel < 0) Channel = 0; | ||
| if (Channel > chanSize) chanSize = Channel; | ||
| dmxData[0] = 0; | ||
| dmxData[Channel] = value; //add one to account for start byte | ||
| if ((Channel < 1) || (Channel > chanSize)) return; | ||
| dmxData[Channel] = value; //start byte at dmxData[0] | ||
| } |
Contributor
There was a problem hiding this comment.
Off-by-one error in bounds check allows potential buffer overflow.
In write mode, chanSize = chanQuant + 1. When chanQuant = 512 (max), chanSize = 513. The check Channel > chanSize evaluates Channel > 513, which allows Channel = 513 to pass, resulting in dmxData[513] — an out-of-bounds write (array indices are 0–512).
Proposed fix
void SparkFunDMX::write(int Channel, uint8_t value) {
- if ((Channel < 1) || (Channel > chanSize)) return;
+ if ((Channel < 1) || (Channel >= chanSize)) return;
dmxData[Channel] = value; //start byte at dmxData[0]
}Based on learnings, in WLED the priority is to prevent out-of-bounds memory access.
🤖 Prompt for AI Agents
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp` around lines 129 - 132, The
write method in SparkFunDMX::write has an off-by-one bounds check allowing
Channel == chanSize which writes past the end of dmxData; update the condition
to reject Channel values >= chanSize (i.e., ensure valid Channel is
1..chanSize-1) so accesses to dmxData[Channel] cannot overflow; modify the check
that currently uses (Channel > chanSize) to use (Channel >= chanSize)
(referencing SparkFunDMX::write, Channel, chanSize, chanQuant and dmxData).
Member
There was a problem hiding this comment.
@coderabbitai is this off-by-one error already present in the previous code, or was it introduced by changes in this PR?
Contributor
There was a problem hiding this comment.
@softhack007, the off-by-one error is newly introduced by this PR.
Previous code behavior:
The old code had a different bug:
if (Channel > chanSize) chanSize = Channel; // BUG: swaps/corrupts chanSize variable
dmxData[Channel] = value;When Channel > chanSize, it would incorrectly modify chanSize itself (the variable swap bug mentioned in the PR description), then proceed to write to dmxData[Channel]. This could corrupt the chanSize variable and potentially write out of bounds.
New code behavior:
The PR fixes the variable swap bug by adding a bounds check and early return. However, the check has an off-by-one error:
if ((Channel < 1) || (Channel > chanSize)) return; // Allows Channel == chanSize
dmxData[Channel] = value;Since chanSize = chanQuant + 1 (line 117) and max chanQuant = 512, we get chanSize = 513. The condition Channel > 513 allows Channel = 513 to pass, accessing dmxData[513], which is out of bounds (valid indices: 0–512).
The fix I suggested (Channel >= chanSize) would properly reject Channel = 513 and ensure only valid channels 1–512 can write to array indices 1–512, while index 0 remains the start byte.
🧠 Learnings used
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: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
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: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
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: 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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
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.
Member
There was a problem hiding this comment.
@coderabbitai line 123 uses the same condition - is that line still correct or does it also need to use (Channel >= chanSize) ?
Contributor
There was a problem hiding this comment.
@softhack007, line 123 (read()) is correct as-is with Channel > chanSize. The reason is that chanSize is assigned differently in each init function:
initRead():chanSize = chanQuant(no+1)initWrite():chanSize = chanQuant + 1(adds 1 for the start byte)
So for read() with max chanQuant = 512:
chanSize = 512- The check
Channel > chanSizerejects anything above 512 ✓ - Max valid access is
dmxData[Channel - 1]=dmxData[511], well within the 0–512 index range ✓
For write() with max chanQuant = 512:
chanSize = 513- The check
Channel > chanSize(i.e.,> 513) allowsChannel = 513 - That would access
dmxData[513]which is out of bounds ✗
So the off-by-one is isolated to write() only, and the fix Channel >= chanSize is needed there but not in read(). The two functions use chanSize with different semantics, which is admittedly a bit confusing but works correctly for read().
🧠 Learnings used
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: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
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: softhack007
Repo: wled/WLED PR: 5442
File: wled00/bus_manager.cpp:462-468
Timestamp: 2026-03-23T23:33:45.480Z
Learning: In WLED's TYPE_ANALOG_2CH_HBRIDGE (wled00/bus_manager.cpp), the dead zone at the CCT midpoint (level=127 and level=128 both producing _data[0]=0, _data[1]=0) is intentional by design. The map() blending is directional: WW channel (_data[0]) is brightest at level=0 and fades to 0 at level=127; CW channel (_data[1]) starts at 0 at level=128 and is brightest at level=255. The zero-output at the midpoint is the H-bridge polarity transition boundary, ensuring no overlap between the two polarity directions. Do not flag this dead zone as a bug in future reviews.
Learnt from: Mdbelen
Repo: wled/WLED PR: 5522
File: wled00/dmx_output.cpp:5-9
Timestamp: 2026-04-21T23:32:50.241Z
Learning: In WLED's `dmx_output.cpp` (ESP8266 path), `uart_tx_fifo_available()` is declared `inline __attribute__((always_inline))` in the ESP8266 Arduino core's `uart.cpp` and is NOT exported through any header. The workaround used is `#include "uart.cpp"` directly, which works because the function is always_inline (no ODR violation for that symbol). A cleaner alternative is to reproduce the one-liner body inline using the `USS()` and `USTXC` macros from `esp8266_peri.h`. Do not flag `#include "uart.cpp"` as an unconditional build failure for this file — it is a known limitation of the ESP8266 core's API surface.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
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: 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: 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: 5434
File: wled00/colors.cpp:79-83
Timestamp: 2026-03-21T11:43:36.767Z
Learning: In WLED's `color_fade()` (wled00/colors.cpp), the `video=true` path computes `maxc = (maxc>>2) + 1` and only preserves a channel if its value strictly exceeds that threshold (`r > maxc`, etc.). This means very dark non-zero channels (e.g., r==1 or r==2) can be dropped to zero in video mode. This is intentional: it prevents flickering in corner regions of gradients and is an accepted design compromise. Do not flag this as a bug in future reviews.
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: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.
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: 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: 0
File: :0-0
Timestamp: 2026-04-13T20:46:14.014Z
Learning: In WLED (wled00/wled.cpp), on ESP32 with arduino-esp32 v2.0.x (IDF V4 branch), calling `pinMode(hardwareRX, INPUT_PULLDOWN)` has an unexpected side effect: it routes the RX pin through the GPIO matrix instead of keeping it on UART0's IOMUX path, effectively detaching the pin from the UART unit and causing serial receive to stop working (issue `#5501`). The correct way to enable a pull-down on the hardware RX pin without disrupting the UART mapping is to call `gpio_pulldown_en((gpio_num_t)hardwareRX)` directly from the ESP-IDF GPIO driver (`driver/gpio.h`). This is the fix applied in PR `#5503`. Do NOT use `pinMode()` to set pull resistors on UART RX/TX pins on ESP32.
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/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: 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/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: 5480
File: docs/cpp.instructions.md:267-491
Timestamp: 2026-04-08T15:03:02.041Z
Learning: In this codebase, the upstream FastLED library has been replaced by the custom `fastled_slim` fork. Do not call the legacy FastLED trig alias names `sin8()`, `cos8()`, `sin16()`, or `cos16()` (they have no backward-compat shims and will fail compilation). Use only the fork’s supported names: `sin8_t()`, `cos8_t()`, `sin16_t()`, `cos16_t()` (declared in `fastled_slim.h`, implemented in `wled_math.cpp`) for the fixed-point variants, and use `sin_approx()` / `cos_approx()` for float approximations.
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.
This was referenced Jan 25, 2026
netmindz
added
bug
connectivity
netmindz
requested changes
Mar 18, 2026
Member
netmindz
left a comment
netmindz
left a comment
There was a problem hiding this comment.
Please fix the off by one error found by the rabbit
Member
|
@Nuet0815 please fix the off-by-one problem (#5316 (comment)), then re-test on your hardware. Your feedback is essential to include your PR in the upcoming 16.0.0 release. |
Member
|
@coderabbitai please re-review this PR. Provide a summary of good practices you found, and a prioritized list of suggested improvements. |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 6795
✅ Good practices found
🔴 Prioritized list of suggested improvementsPriority 1 — Off-by-one in
|
| Priority | Location | Issue | Severity |
|---|---|---|---|
| 1 | write() |
Channel > chanSize should be Channel >= chanSize |
🔴 UB / crash |
| 2 | read() |
dmxData[Channel-1] should be dmxData[Channel] |
🟠 Wrong data |
| 3 | update() |
Document the chanSize invariant |
🟡 Clarity |
| 4 | initWrite() |
Explicitly set dmxData[0] = 0 |
🟢 Robustness |
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T11:41:46.737Z
Learning: In the WLED repository (wled/WLED), trailing-whitespace removals or other pure-whitespace "optimizations" must NOT be intermixed with real code/logic changes in the same PR. Reviewers prefer whitespace cleanup to be submitted in a dedicated, separate PR. Reason: intermixing makes diffs harder to read (obscures actual logic changes) and causes trouble when cherry-picking commits. Always flag PRs that mix whitespace-only edits with functional changes and ask the contributor to separate them.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
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: 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: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
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: 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: 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-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
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: 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: 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: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-13T20:46:14.014Z
Learning: In WLED (wled00/wled.cpp), on ESP32 with arduino-esp32 v2.0.x (IDF V4 branch), calling `pinMode(hardwareRX, INPUT_PULLDOWN)` has an unexpected side effect: it routes the RX pin through the GPIO matrix instead of keeping it on UART0's IOMUX path, effectively detaching the pin from the UART unit and causing serial receive to stop working (issue `#5501`). The correct way to enable a pull-down on the hardware RX pin without disrupting the UART mapping is to call `gpio_pulldown_en((gpio_num_t)hardwareRX)` directly from the ESP-IDF GPIO driver (`driver/gpio.h`). This is the fix applied in PR `#5503`. Do NOT use `pinMode()` to set pull resistors on UART RX/TX pins on ESP32.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048`, the NTP/UDP crash on ESP32-C5 (pioarduino IDF 5.5.x): CONFIG_LWIP_TCPIP_CORE_LOCKING defaults to n (disabled) in arduino-esp32/pioarduino, while CONFIG_LWIP_CHECK_THREAD_SAFETY=y is what actually causes the "Required to lock TCPIP core functionality!" assertion. LOCK_TCPIP_CORE() / UNLOCK_TCPIP_CORE() macros are only functional when CONFIG_LWIP_TCPIP_CORE_LOCKING=y; using them without enabling that config has no effect. The correct fix without sdkconfig changes is to use tcpip_callback() to schedule ntpUdp.begin() (and other raw lwIP API calls) on the TCPIP thread, which works regardless of the locking mode setting. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048` (wled00/cfg.cpp), the NTP crash on ESP32-C5 (pioarduino + IDF 5.5.x) is caused by both CONFIG_LWIP_TCPIP_CORE_LOCKING=y AND CONFIG_LWIP_CHECK_THREAD_SAFETY=y being enabled by default in the pioarduino/arduino-esp32 IDF 5.x sdkconfig. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6. Since pioarduino already has TCPIP_CORE_LOCKING enabled, wrapping ntpUdp.begin() (and any other raw lwIP API call) with LOCK_TCPIP_CORE()/UNLOCK_TCPIP_CORE() from lwip/tcpip.h is directly applicable without sdkconfig changes in platformio.ini.
✅ Actions performed
Full review triggered.
Contributor
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/dmx/SparkFunDMX.cpp`:
- Around line 123-124: The read-path is off by one: the start code lives in
dmxData[0] so a 1-based API should return dmxData[Channel] not dmxData[Channel -
1]; update the read(Channel) logic to use dmxData[Channel] and adjust the bounds
check to ensure Channel is within 1..(chanSize-1) (or otherwise ensure chanSize
represents the highest valid 1-based channel index), referencing the read
function, Channel parameter, chanSize and the dmxData buffer when making the
change.
🪄 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: 3e423af9-5b6c-4bab-a28d-a1d68638a9a0
📒 Files selected for processing (1)
wled00/src/dependencies/dmx/SparkFunDMX.cpp
Comment on lines
+123
to
124
| if ((Channel < 1) || (Channel > chanSize)) return 0; | ||
| return(dmxData[Channel - 1]); //subtract one to account for start byte |
Contributor
There was a problem hiding this comment.
Fix the read-path indexing.
read(1) currently returns dmxData[0], which is the start code, so every DMX channel is shifted down by one. If the API is 1-based, this should read from dmxData[Channel] instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp` around lines 123 - 124, The
read-path is off by one: the start code lives in dmxData[0] so a 1-based API
should return dmxData[Channel] not dmxData[Channel - 1]; update the
read(Channel) logic to use dmxData[Channel] and adjust the bounds check to
ensure Channel is within 1..(chanSize-1) (or otherwise ensure chanSize
represents the highest valid 1-based channel index), referencing the read
function, Channel parameter, chanSize and the dmxData buffer when making the
change.
Member
@coderabbitai please verify if this finding is correct. Review all call sites of the functions involved and check if channel handling is indeed off by one here. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12468
Verification of the
|
| DMX concept | Array index |
|---|---|
| Start code (0x00) | dmxData[0] |
| DMX channel 1 | dmxData[1] |
| DMX channel N | dmxData[N] |
write(Channel, value) stores at dmxData[Channel] — consistent with this.
The receive path in update() starts currentChannel = 0 and does dmxData[currentChannel++] = DMXSerial.read(), so the first received byte (start code) lands at dmxData[0], and channel 1 at dmxData[1] — also consistent.
But read(Channel) returns dmxData[Channel - 1], so read(1) returns dmxData[0] — the start byte, not channel 1 data. The comment //subtract one to account for start byte has the logic backwards.
However — critical context: read() is currently dead code in WLED.
SparkFunDMX.h defines DMX_SEND_ONLY unconditionally at the top of the file:
`#define` DMX_SEND_ONLY // this disables DMX sending features, and saves us two GPIO pins(The comment is also wrong — it should say "disables DMX receiving features".) Both read() and initRead() are guarded by #if !defined(DMX_SEND_ONLY) and are therefore never compiled in the current WLED build. There are zero call sites of SparkFunDMX::read() anywhere in the codebase.
Additional asymmetry if read() is ever re-enabled
initRead() sets chanSize = chanQuant (no +1), while initWrite() sets chanSize = chanQuant + 1. So for initRead(512), chanSize = 512, and the upper bound check Channel > chanSize lets Channel = 512 through but then dmxData[512 - 1] = dmxData[511] returns channel 511's data instead of channel 512's. The fix (dmxData[Channel]) would also require aligning the chanSize semantics between read and write paths.
Summary
| Finding | Correct? | Impact in current build |
|---|---|---|
Priority 1: write() uses > chanSize instead of >= chanSize |
✅ Confirmed | 🔴 Active bug (OOB write possible) |
Priority 2: read() returns dmxData[Channel-1] instead of dmxData[Channel] |
✅ Confirmed | 🟡 Latent only — read() is compiled out by DMX_SEND_ONLY |
The Priority 1 fix (>= chanSize in write()) remains the more urgent active issue. Priority 2 should still be fixed to keep the library correct for any future use of DMX receive mode.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5442
File: wled00/bus_manager.cpp:462-468
Timestamp: 2026-03-23T23:33:45.480Z
Learning: In WLED's TYPE_ANALOG_2CH_HBRIDGE (wled00/bus_manager.cpp), the dead zone at the CCT midpoint (level=127 and level=128 both producing _data[0]=0, _data[1]=0) is intentional by design. The map() blending is directional: WW channel (_data[0]) is brightest at level=0 and fades to 0 at level=127; CW channel (_data[1]) starts at 0 at level=128 and is brightest at level=255. The zero-output at the midpoint is the H-bridge polarity transition boundary, ensuring no overlap between the two polarity directions. Do not flag this dead zone as a bug in future reviews.
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: 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: 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: 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: 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: DedeHai
Repo: wled/WLED PR: 5434
File: wled00/colors.cpp:79-83
Timestamp: 2026-03-21T11:43:36.767Z
Learning: In WLED's `color_fade()` (wled00/colors.cpp), the `video=true` path computes `maxc = (maxc>>2) + 1` and only preserves a channel if its value strictly exceeds that threshold (`r > maxc`, etc.). This means very dark non-zero channels (e.g., r==1 or r==2) can be dropped to zero in video mode. This is intentional: it prevents flickering in corner regions of gradients and is an accepted design compromise. Do not flag this as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-15T15:54:12.455Z
Learning: In WLED (wled00/FX_fcn.cpp), when `ledmap.json` (index 0) contains `width` or `height` fields, setting `isMatrix = true` and updating `Segment::maxWidth`/`maxHeight` is intentional and correct behaviour — a ledmap with those fields is by definition a 2D setup. Do not flag this as an unexpected or unintended trigger for 2D mode in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FXparticleSystem.cpp:598-606
Timestamp: 2026-03-09T19:16:30.985Z
Learning: In WLED's FXparticleSystem.cpp, the particle system (both 1D and 2D) is intentionally RGB-only and does not process or output the white channel. The `fast_color_scaleAdd()` function discards the W byte by design. Even though `CRGBW` is used internally for color representation (e.g., `baseRGB`, palette lookups via `ColorFromPalette`), the W channel is never rendered. Do not flag the missing W channel handling in particle system rendering paths as a bug.
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: 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: 0
File: :0-0
Timestamp: 2026-04-13T20:46:14.014Z
Learning: In WLED (wled00/wled.cpp), on ESP32 with arduino-esp32 v2.0.x (IDF V4 branch), calling `pinMode(hardwareRX, INPUT_PULLDOWN)` has an unexpected side effect: it routes the RX pin through the GPIO matrix instead of keeping it on UART0's IOMUX path, effectively detaching the pin from the UART unit and causing serial receive to stop working (issue `#5501`). The correct way to enable a pull-down on the hardware RX pin without disrupting the UART mapping is to call `gpio_pulldown_en((gpio_num_t)hardwareRX)` directly from the ESP-IDF GPIO driver (`driver/gpio.h`). This is the fix applied in PR `#5503`. Do NOT use `pinMode()` to set pull resistors on UART RX/TX pins on ESP32.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/cfg.cpp:673-673
Timestamp: 2026-03-27T21:17:51.985Z
Learning: In WLED PR `#5048`, the NTP/UDP crash on ESP32-C5 (pioarduino IDF 5.5.x): CONFIG_LWIP_TCPIP_CORE_LOCKING defaults to n (disabled) in arduino-esp32/pioarduino, while CONFIG_LWIP_CHECK_THREAD_SAFETY=y is what actually causes the "Required to lock TCPIP core functionality!" assertion. LOCK_TCPIP_CORE() / UNLOCK_TCPIP_CORE() macros are only functional when CONFIG_LWIP_TCPIP_CORE_LOCKING=y; using them without enabling that config has no effect. The correct fix without sdkconfig changes is to use tcpip_callback() to schedule ntpUdp.begin() (and other raw lwIP API calls) on the TCPIP thread, which works regardless of the locking mode setting. The tasmota core (IDF 5.3.x, used for C6) likely has CONFIG_LWIP_CHECK_THREAD_SAFETY=n, which is why the same violation does not crash on C6.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed bug of ESP32 crashing when using DMX out:
-When exceeding the DMX channel limit, the SparkFunDMX.cpp tried to write to static chanSize (line 132) because the variables where swapped.
-When exceeding the channel upper & lower limit now, the value is ignored, previous implementation "idea" was to set as channel 512 value, which is not correct.
-There are still some non-optimal parts in the lib, but it at least works for now.
Summary by CodeRabbit
Bug Fixes
Refactor