Non-blocking, unified, dependency-less DMXOutput#5522
Open
Non-blocking, unified, dependency-less DMXOutput#5522
Conversation
This increases clarity that WLED_ENABLE_DMX_INPUT is not a subset of WLED_ENABLE_DMX but independent.
…allocation. This is useful for boards with DMX.
This limits the number of used pixels to total to 512 channels used for transmitting via DMX according to the DMX start and spacing settings.
All DMX functions are handled on its own for ESP32 (remove SparkFunDMX dependency) features: * non-blocking DMX update function * maximum refresh rate setting * some more useful functions For ESP8266 stay with DMXESPSerial for now and wrap that in DMXOutput.
Contributor
WalkthroughThis PR refactors WLED's DMX output support by introducing a new unified Changes
Sequence DiagramsequenceDiagram
participant WLED as WLED Core
participant DMXOut as DMXOutput
participant UART as UART/Serial
WLED->>DMXOut: init(dmxOutputPin, updateRate)
activate DMXOut
DMXOut->>UART: create HardwareSerial, configure TX pin
DMXOut->>DMXOut: initialize channel buffer
deactivate DMXOut
loop Main Loop
WLED->>DMXOut: handleDMXOutput()
activate DMXOut
alt not busy() and rate-limited
DMXOut->>DMXOut: write(channel, value)
DMXOut->>DMXOut: update()
activate DMXOut
DMXOut->>UART: send DMX break (baud/format switch)
DMXOut->>UART: transmit full channel buffer
DMXOut->>UART: reset start code
deactivate DMXOut
end
deactivate DMXOut
end
WLED->>DMXOut: end()
activate DMXOut
DMXOut->>UART: stop serial, deallocate resources
deactivate DMXOut
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
coderabbitai
Bot
added
enhancement
hardware
![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: 10
🧹 Nitpick comments (3)
wled00/dmx_output.h (2)
30-98: Missing<Arduino.h>include and a fewconstcorrectness nits.This header uses
HardwareSerial*,uint8_t,uint16_twithout including anything. It works only because every current consumer transitively pulls inArduino.hviawled.hfirst (clang's standalone parse correctly reports the errors shown in the static-analysis hints). Add an explicit#include <Arduino.h>so the header is self-contained.Also, the query methods should be
constso they can be called on aconst DMXOutput&:🧹 Proposed tweaks
`#ifndef` DMX_OUTPUT_H `#define` DMX_OUTPUT_H + +#include <Arduino.h> ... - uint8_t read(uint16_t channel); + uint8_t read(uint16_t channel) const; ... - unsigned long getLastDmxOut(); + unsigned long getLastDmxOut() const; - bool busy(); + bool busy(); // non-const: delays on ESP8266, touches hw state - void setUpdateRate(uint8_t updateRate); - uint8_t getUpdateRate(); + void setUpdateRate(uint8_t updateRate); + uint8_t getUpdateRate() const; - int timeToNextUpdate(); + int timeToNextUpdate() const;As per coding guidelines: “Mark getter/query methods as
const”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.h` around lines 30 - 98, Add a self-contained Arduino include and make the query/getter methods const: add `#include` <Arduino.h> at the top of the DMXOutput header, and mark the query methods getLastDmxOut(), busy(), getUpdateRate(), timeToNextUpdate() (and any other non-mutating accessors like read()/readBytes() if intended to be non-mutating) as const in their declarations so they can be called on const DMXOutput instances; ensure the signatures in the class (DMXOutput::~DMXOutput, init, end, write, writeBytes, read, readBytes, update, handleDMXOutput, getLastDmxOut, busy, setUpdateRate, getUpdateRate, timeToNextUpdate) remain consistent with any corresponding definitions.
21-27: ParenthesizeDMX_CHANNELSand preferconstexprfor these header constants.
#define DMX_CHANNELS DMX_CHANNEL_TOP + 1is unparenthesized — any future use likeDMX_CHANNELS * Norsizeof(buf)/DMX_CHANNELSwould silently compute the wrong value. Current call sites happen to be safe, but this is a classic foot-gun.Additionally, per the coding guidelines, header-scope compile-time constants should be
constexprrather than#define, which also gives them proper types and namespacing.♻️ Suggested refactor
-#define DMX_CHANNEL_TOP 512 -#define DMX_CHANNELS DMX_CHANNEL_TOP + 1 - -#define DMXSPEED 250000 -#define DMXFORMAT SERIAL_8N2 -#define BREAKSPEED 83000 -#define BREAKFORMAT SERIAL_8N1 // unused, instead, DMXFORMAT is used +constexpr uint16_t DMX_CHANNEL_TOP = 512; +constexpr uint16_t DMX_CHANNELS = DMX_CHANNEL_TOP + 1; +constexpr uint32_t DMXSPEED = 250000; +constexpr uint32_t BREAKSPEED = 83000; +constexpr auto DMXFORMAT = SERIAL_8N2; +// BREAKFORMAT intentionally omitted — unused, DMXFORMAT is used for breaks too.As per coding guidelines: “Prefer
constexprover#definefor compile-time constants in C++”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.h` around lines 21 - 27, Replace the unparenthesized macro and other header `#defines` with typed constexpr variables and ensure the DMX_CHANNELS expression is evaluated correctly; specifically, change the macros DMX_CHANNEL_TOP and DMX_CHANNELS to constexpr (e.g., constexpr int DMX_CHANNEL_TOP = 512; constexpr int DMX_CHANNELS = DMX_CHANNEL_TOP + 1) so DMX_CHANNELS behaves correctly in arithmetic contexts, and similarly convert DMXSPEED, BREAKSPEED (use an appropriate integer type), and named constants like DMXFORMAT/BREAKFORMAT to constexpr or scoped constexpr integers/enum values as appropriate for the platform; keep the original names (DMX_CHANNEL_TOP, DMX_CHANNELS, DMXSPEED, DMXFORMAT, BREAKSPEED, BREAKFORMAT) so call sites remain valid.wled00/dmx_output.cpp (1)
11-50:init()initialization-state check and pin guard miss a couple of edge cases.Two small issues in
init():
- Line 14 uses
if(_uartNo > 0)to mean “already initialized”, but_uartNois only default-initialized to-1whenDMXOutputis a statically constructed global. GivendmxOutputis a global here that's fine, but a secondinit()call after a failed first call (where_uartNowas never assigned) is silently re-entered — consider asserting_dmxSerial == nullptror using>= 0with an explicit “initialized” flag, so the contract matches the doc comment.- Line 37
if(outputPin < 1) return false;is a no-op for the common “disabled” sentinel of-1becauseoutputPinisuint8_t, which makes it255(see thewled.hcomment ondmxOutputPin). Changing the parameter toint8_t(orint) makes this guard actually protect against the documented sentinel.Finally, per the coding guidelines the preprocessor
#errorat line 24–26 could be astatic_assert(SOC_UART_NUM > 1, "...")— but sinceSOC_UART_NUMis only defined via SOC caps,#erroris acceptable; flagging only for awareness.As per coding guidelines: “Use
static_assertover#if ...#error`` for compile-time assertions in C++”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 11 - 50, DMXOutput::init currently uses if(_uartNo > 0) to detect prior initialization and takes outputPin as uint8_t which breaks the documented -1 sentinel; change the init precondition to check a proper initialized flag or _uartNo >= 0 and/or assert _dmxSerial == nullptr (referencing _uartNo and _dmxSerial) so a failed prior init doesn't leave init re-entrant, and change the outputPin parameter from uint8_t to a signed type (int8_t or int) and update the guard from if(outputPin < 1) to correctly handle the -1 disabled sentinel before calling PinManager::allocatePin and pinMode/ digitalWrite; leave the SOC_UART_NUM `#error` as-is or replace with static_assert only if SOC_UART_NUM is a compile-time constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/cfg.cpp`:
- Around line 610-612: dmxOutputPin is deserialized as a signed int and then
passed to DMXOutput::init(uint8_t, ...) via CJSON(dmxOutputPin,
if_live_dmx[F("dmxOutputPin")]); validate the integer value before narrowing:
check that dmxOutputPin is within the valid range (>=1 and <= WLED_NUM_PINS) and
handle out-of-range values (e.g., ignore, set to 0/disabled, or log/error)
before casting to uint8_t; update the code path that calls DMXOutput::init (and
the if_live_dmx handling) to perform this range check and only pass a uint8_t
when the value is confirmed valid to avoid integer wrapping.
In `@wled00/data/settings_dmx.htm`:
- Line 57: The inline style on the help paragraph uses an invalid CSS property
`size: 0.8em`; update the <p> element (the help text paragraph referencing "For
pin settings, go to Sync settings.") to use `font-size: 0.8em` instead of `size:
0.8em` (keep the other style rules intact: padding-top, margin-top, font-style:
italic) so the smaller text is applied correctly.
In `@wled00/data/settings_sync.htm`:
- Around line 53-56: The hideNoDMXOutput function currently sets
gId("dmxOutput").style.display = "inline", which can collapse the div layout;
change it to set gId("dmxOutput").style.display = "block" (keep
gId("dmxOutputOff").style.display="none" as is) so the DMX output div
(gId("dmxOutput")) renders as a block containing its <h4> and form row; update
the display assignment in the hideNoDMXOutput function accordingly.
In `@wled00/dmx_output.cpp`:
- Around line 175-182: Validate and clamp DMX configuration before calculating
maxLen: ensure DMXGap != 0 (return/skip or set to 1 if invalid), ensure DMXStart
<= DMX_CHANNELS (or clamp DMXStart to DMX_CHANNELS), and assert DMXChannels >=
1; then compute maxLen using safe subtraction/cast (e.g., treat DMX_CHANNELS -
DMXStart as unsigned guarded value) so division by DMXGap and subtraction cannot
underflow or divide-by-zero; apply these checks around the block that computes
maxLen (references: DMXGap, DMXStart, DMX_CHANNELS, DMXChannels,
strip.getLengthTotal(), maxLen) and clamp len to the sanitized maxLen before
iterating.
- Around line 5-9: The code currently includes "uart.cpp" in dmx_output.cpp for
ESP8266 which will fail compile and can cause ODR/linker issues; instead remove
that include and add an external declaration for the required ESP8266 core
function (declare extern "C" int uart_tx_fifo_available(); or the correct
signature) so the compiler knows the symbol and the linker will resolve it from
the ESP8266 Arduino core; update the conditional branch around the include (the
`#ifdef` ESP8266 section) to declare the function prototype referencing
uart_tx_fifo_available() rather than including uart.cpp and verify the declared
signature matches the core header.
In `@wled00/e131.cpp`:
- Around line 117-121: The proxy currently always forwards from &e131_data[1],
which misaligns Art-Net (art_data starts at index 0) and can read past the
payload; update the proxy logic around e131ProxyUniverse / uni and
dmxOutput.writeBytes to pick the correct data pointer and length based on
protocol: use art_data (offset 0) when the packet is Art‑Net and e131_data+1
(offset 1) for E1.31, and clamp the byte count to the actual payload length
(min(dmxChannels, payloadLength - offset)) before calling dmxOutput.writeBytes
and dmxOutput.update. Ensure you reference the packet type/flags available in
the surrounding code to choose between art_data and e131_data and adjust the
offset accordingly.
In `@wled00/set.cpp`:
- Around line 477-479: When handling request->arg(F("IDMO")).toInt() for
dmxOutputPin (inside the WLED_ENABLE_DMX_OUTPUT block), avoid blindly saving 0
from toInt(): read the raw arg string first, if it's empty leave dmxOutputPin
unchanged, otherwise parse to an int and only assign it when parsed >= 1 and
passes runtime GPIO validation (use your existing runtime-valid pin check or the
same logic used by dmx_output.cpp which rejects outputPin < 1); for any
malformed/invalid value set dmxOutputPin = -1 (disabled) or keep prior -1, but
never store 0 as a valid pin.
In `@wled00/wled.cpp`:
- Around line 571-572: Move the guarded DMX pin reservation so the configured
DMX GPIO is allocated before any LED buses or usermods can claim it: call
dmxOutput.init(dmxOutputPin, 43) immediately after config load (right after
loading settings) and before beginStrip() and usermod setup, and remove the
later dmxOutput.init(...) call currently located after beginStrip(); reference
dmxOutput.init, dmxOutputPin and beginStrip to find and update the code.
In `@wled00/wled.h`:
- Line 43: The DMX feature flag was renamed from WLED_ENABLE_DMX to
WLED_ENABLE_DMX_OUTPUT and legacy uses will be silently ignored; add a migration
guard in wled.h before the DMX include/block that detects if WLED_ENABLE_DMX is
defined and emits a preprocessor warning or error (e.g., `#warning` or `#error`)
instructing users to rename it to WLED_ENABLE_DMX_OUTPUT, and optionally define
WLED_ENABLE_DMX_OUTPUT automatically for backward compatibility when the legacy
flag is present so existing platformio_override.ini/my_config.h/CI configs don’t
drop DMX silently.
- Around line 451-456: The DMX output code silently converts the sentinel
DMX_TXPIN_DEFAULT (-1) to 255 when calling DMXOutput::init, causing spurious
"pin 255" allocation errors; fix by (1) changing DMXOutput::init signature to
accept a signed type (int8_t or int) instead of uint8_t so the sentinel
round-trips and the existing outputPin < 1 guard behaves as intended (update
declaration in dmx_output.h and its implementation), and (2) in the caller
(where dmxOutput.init(dmxOutputPin, 43) is invoked) skip calling dmxOutput.init
entirely when dmxOutputPin equals DMX_TXPIN_DEFAULT (or < 0) to avoid attempting
init with an unset pin; finally, reconcile the default updateRate value between
the DMXOutput::init default (44) and the call site/PR (43) so the documented
default matches the actual used value.
---
Nitpick comments:
In `@wled00/dmx_output.cpp`:
- Around line 11-50: DMXOutput::init currently uses if(_uartNo > 0) to detect
prior initialization and takes outputPin as uint8_t which breaks the documented
-1 sentinel; change the init precondition to check a proper initialized flag or
_uartNo >= 0 and/or assert _dmxSerial == nullptr (referencing _uartNo and
_dmxSerial) so a failed prior init doesn't leave init re-entrant, and change the
outputPin parameter from uint8_t to a signed type (int8_t or int) and update the
guard from if(outputPin < 1) to correctly handle the -1 disabled sentinel before
calling PinManager::allocatePin and pinMode/ digitalWrite; leave the
SOC_UART_NUM `#error` as-is or replace with static_assert only if SOC_UART_NUM is
a compile-time constant.
In `@wled00/dmx_output.h`:
- Around line 30-98: Add a self-contained Arduino include and make the
query/getter methods const: add `#include` <Arduino.h> at the top of the DMXOutput
header, and mark the query methods getLastDmxOut(), busy(), getUpdateRate(),
timeToNextUpdate() (and any other non-mutating accessors like read()/readBytes()
if intended to be non-mutating) as const in their declarations so they can be
called on const DMXOutput instances; ensure the signatures in the class
(DMXOutput::~DMXOutput, init, end, write, writeBytes, read, readBytes, update,
handleDMXOutput, getLastDmxOut, busy, setUpdateRate, getUpdateRate,
timeToNextUpdate) remain consistent with any corresponding definitions.
- Around line 21-27: Replace the unparenthesized macro and other header `#defines`
with typed constexpr variables and ensure the DMX_CHANNELS expression is
evaluated correctly; specifically, change the macros DMX_CHANNEL_TOP and
DMX_CHANNELS to constexpr (e.g., constexpr int DMX_CHANNEL_TOP = 512; constexpr
int DMX_CHANNELS = DMX_CHANNEL_TOP + 1) so DMX_CHANNELS behaves correctly in
arithmetic contexts, and similarly convert DMXSPEED, BREAKSPEED (use an
appropriate integer type), and named constants like DMXFORMAT/BREAKFORMAT to
constexpr or scoped constexpr integers/enum values as appropriate for the
platform; keep the original names (DMX_CHANNEL_TOP, DMX_CHANNELS, DMXSPEED,
DMXFORMAT, BREAKSPEED, BREAKFORMAT) so call sites remain valid.
🪄 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: 931d3154-cb77-4ca0-b2d7-48b7b035e84f
📒 Files selected for processing (22)
platformio_override.sample.initools/cdata.jswled00/cfg.cppwled00/const.hwled00/data/settings_dmx.htmwled00/data/settings_sync.htmwled00/dmx_output.cppwled00/dmx_output.hwled00/e131.cppwled00/fcn_declare.hwled00/pin_manager.cppwled00/pin_manager.hwled00/set.cppwled00/src/dependencies/dmx/ESPDMX.cppwled00/src/dependencies/dmx/ESPDMX.hwled00/src/dependencies/dmx/LICENSE.mdwled00/src/dependencies/dmx/SparkFunDMX.cppwled00/src/dependencies/dmx/SparkFunDMX.hwled00/wled.cppwled00/wled.hwled00/wled_server.cppwled00/xml.cpp
💤 Files with no reviewable changes (6)
- wled00/src/dependencies/dmx/LICENSE.md
- wled00/fcn_declare.h
- wled00/src/dependencies/dmx/ESPDMX.h
- wled00/src/dependencies/dmx/ESPDMX.cpp
- wled00/src/dependencies/dmx/SparkFunDMX.h
- wled00/src/dependencies/dmx/SparkFunDMX.cpp
Comment on lines
+117
to
+121
| #ifdef WLED_ENABLE_DMX_OUTPUT | ||
| // does not act on out-of-order packets yet | ||
| if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) { | ||
| for (uint16_t i = 1; i <= dmxChannels; i++) | ||
| dmx.write(i, e131_data[i]); | ||
| dmx.update(); | ||
| dmxOutput.writeBytes(1, &e131_data[1], dmxChannels); | ||
| dmxOutput.update(); |
Contributor
There was a problem hiding this comment.
Use the protocol-specific DMX data offset before proxying.
Art-Net payloads start at art_data[0], while E1.31 property values reserve index 0 for the start code. The unconditional &e131_data[1] shifts Art-Net output by one channel and reads past the payload for full 512-channel packets.
🐛 Proposed fix
`#ifdef` WLED_ENABLE_DMX_OUTPUT
// does not act on out-of-order packets yet
if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) {
- dmxOutput.writeBytes(1, &e131_data[1], dmxChannels);
+ const uint16_t dmxDataOffset = (mde == REALTIME_MODE_ARTNET) ? 0 : 1;
+ dmxOutput.writeBytes(1, &e131_data[dmxDataOffset], dmxChannels);
dmxOutput.update();
}
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/e131.cpp` around lines 117 - 121, The proxy currently always forwards
from &e131_data[1], which misaligns Art-Net (art_data starts at index 0) and can
read past the payload; update the proxy logic around e131ProxyUniverse / uni and
dmxOutput.writeBytes to pick the correct data pointer and length based on
protocol: use art_data (offset 0) when the packet is Art‑Net and e131_data+1
(offset 1) for E1.31, and clamp the byte count to the actual payload length
(min(dmxChannels, payloadLength - offset)) before calling dmxOutput.writeBytes
and dmxOutput.update. Ensure you reference the packet type/flags available in
the surrounding code to choose between art_data and e131_data and adjust the
offset accordingly.
Comment on lines
+571
to
+572
| #ifdef WLED_ENABLE_DMX_OUTPUT | ||
| dmxOutput.init(dmxOutputPin, 43); |
Contributor
There was a problem hiding this comment.
Reserve the configured DMX pin before buses and usermods claim it.
dmxOutput.init() is the first actual PinManager allocation for dmxOutputPin, but it runs after beginStrip() and usermod setup. A default LED bus or usermod can claim the configured DMX GPIO first, causing DMX output init to fail.
🔧 Proposed fix
Move the guarded init to immediately after config load and before beginStrip():
DEBUG_PRINTLN(F("Reading config"));
bool needsCfgSave = deserializeConfigFromFS();
DEBUG_PRINTF_P(PSTR("heap %u\n"), getFreeHeapSize());
+
+#ifdef WLED_ENABLE_DMX_OUTPUT
+ if (dmxOutputPin > 0 && dmxOutputPin < (int)WLED_NUM_PINS) dmxOutput.init(static_cast<uint8_t>(dmxOutputPin), 43);
+#endif
`#if` defined(STATUSLED) && STATUSLED>=0Then remove the later init:
-#ifdef WLED_ENABLE_DMX_OUTPUT
- dmxOutput.init(dmxOutputPin, 43);
-#endif
`#ifdef` WLED_ENABLE_DMX_INPUT
dmxInput.init(dmxInputReceivePin, dmxInputTransmitPin, dmxInputEnablePin, dmxInputPort);
`#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled.cpp` around lines 571 - 572, Move the guarded DMX pin reservation
so the configured DMX GPIO is allocated before any LED buses or usermods can
claim it: call dmxOutput.init(dmxOutputPin, 43) immediately after config load
(right after loading settings) and before beginStrip() and usermod setup, and
remove the later dmxOutput.init(...) call currently located after beginStrip();
reference dmxOutput.init, dmxOutputPin and beginStrip to find and update the
code.
DedeHai
reviewed
Apr 22, 2026
| uartNo = SOC_UART_NUM - 1; // use last UART as default | ||
| } | ||
| if(uartNo == 0) { | ||
| DEBUG_PRINTF_P(PSTR("DMXOutput: Error: Cannot run on chips with <=1 hardware UART, or with UART0.")); |
Collaborator
There was a problem hiding this comment.
why is this restriction necessary?
Author
There was a problem hiding this comment.
Good question. I'm inclined to say I only updated some module here and these restrictions applied before too, even more restrictive. SparkFunDMX would throw an error if HardwareSerial(2) didn't exist.
This restriction was eased by netmindz in his commit to allow 2-UART SoCs.
The problem IMO is that hardware resources don't have any means to be "reserved/dedicated" to a specific functionality, like PinManager does for pins. I don't have much clue what modules and built-in functionality exists, so what conflicts can or will arise. All I can say is Serial0 would probably be a bad choice, as at least in the beginning we have some debug output there, even if WLED_DEBUG is disabled. If this is enabled, then it gets ofc so much worse.
And even if hw is only used in an ordered, sequential manner, you'd need to pay attention to fully reset all registers, as depending on the HAL, you'd assume reset condition when initializing, thus don't reset bits that were set by other users before.
Collaborator
There was a problem hiding this comment.
Fair point. We really need a resource manager, there is a growing list of features that have hardware resource conflicts. This is something that came up in discussions several times, it is only a question of when and how.
Member
|
I'll try and have a proper look through, but a quick bit of feedback Please don't rename the define for the output feature, it's just an unnecessary breaking change. Now we are on V4 for all builds I'll probably be removing the DMX_INPUT as it's supposed on all esp32s and has minimal size impact |
netmindz
added
the
connectivity
Member
|
Thanks for the PR @Mdbelen , can you clarify which MCUs you have tested with and have you used a DMX tester or logic analyzer to validate the output of your new driver? |
…TPUT" This reverts commit bc80c4a. It does keep the PinManager name change from DMX to DMX_OUTPUT!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
wled00/e131.cpp (1)
119-121:⚠️ Potential issue | 🔴 CriticalUse the protocol-specific payload offset when proxying DMX.
Art-Net payload starts at
art_data[0], while E1.31 reservesproperty_values[0]for the start code. The unconditional&e131_data[1]shifts every Art-Net channel by one and reads past the payload on full 512-channel packets.🐛 Suggested fix
if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) { - dmxOutput.writeBytes(1, &e131_data[1], dmxChannels); + const uint16_t dmxDataOffset = (mde == REALTIME_MODE_ARTNET) ? 0 : 1; + dmxOutput.writeBytes(1, &e131_data[dmxDataOffset], dmxChannels); dmxOutput.update(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/e131.cpp` around lines 119 - 121, The proxying currently always uses the E1.31 payload offset (&e131_data[1]) which misaligns Art‑Net packets (art_data starts at art_data[0]) and can read past the buffer on 512‑channel frames; update the proxy logic in the block checking e131ProxyUniverse/uni to choose the correct pointer and offset based on the source protocol (use art_data for Art‑Net starting at index 0, and e131_data starting at index 1 for E1.31) when calling dmxOutput.writeBytes and ensure dmxChannels bounds are respected; locate the branch around e131ProxyUniverse, uni, dmxOutput.writeBytes and adjust to pass the protocol‑appropriate buffer pointer and length.wled00/set.cpp (1)
477-479:⚠️ Potential issue | 🟠 MajorApply and validate the DMX TX pin change instead of only storing
toInt().
toInt()turns empty/malformed input into0, so this can silently lose the-1disabled sentinel. More importantly, the live driver is only initialized once inWLED::setup()(wled00/wled.cpp), so saving a new pin here leaves DMX output bound to the old GPIO until reboot. Please validate the submitted value and either reinitializedmxOutputhere or force a reboot/apply step when the pin changes.🔧 Suggested fix
`#ifdef` WLED_ENABLE_DMX - dmxOutputPin = request->arg(F("IDMO")).toInt(); + if (request->hasArg(F("IDMO"))) { + const int newPin = request->arg(F("IDMO")).toInt(); + if (newPin >= -1 && newPin < WLED_NUM_PINS && newPin != dmxOutputPin) { + dmxOutputPin = newPin; + doReboot = true; // or rebind dmxOutput here + } + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/set.cpp` around lines 477 - 479, The code assigns dmxOutputPin = request->arg(F("IDMO")).toInt() which silently converts empty/malformed input to 0 and loses the -1 sentinel, and it only updates the stored value without reinitializing the DMX driver (initialized in WLED::setup()), so changes don't take effect until reboot; fix by parsing and validating the raw string from request->arg(F("IDMO")) (preserve -1 as a valid "disabled" value and reject non-numeric input), update the stored dmxOutputPin only if the parsed value is valid, and when the pin actually changes either reinitialize the DMX output driver immediately (call the driver init/reset function used in WLED::setup() — reference the same DMX init routine) or mark the config as needing an apply/reboot and return a response instructing the user to reboot/apply.wled00/wled.cpp (1)
571-572:⚠️ Potential issue | 🟠 MajorReserve the configured DMX GPIO before buses and usermods claim it.
dmxOutput.init()is the firstPinManagerallocation for the TX pin, but it runs afterbeginStrip()and usermod setup. If either path grabs the configured GPIO first, DMX output init fails on boot even though the setting is valid.🔧 Suggested fix
DEBUG_PRINTLN(F("Reading config")); bool needsCfgSave = deserializeConfigFromFS(); DEBUG_PRINTF_P(PSTR("heap %u\n"), getFreeHeapSize()); + +#ifdef WLED_ENABLE_DMX + dmxOutput.init(dmxOutputPin, 43); +#endif `#if` defined(STATUSLED) && STATUSLED>=0 if (!PinManager::isPinAllocated(STATUSLED)) {-#ifdef WLED_ENABLE_DMX - dmxOutput.init(dmxOutputPin, 43); -#endif `#ifdef` WLED_ENABLE_DMX_INPUT dmxInput.init(dmxInputReceivePin, dmxInputTransmitPin, dmxInputEnablePin, dmxInputPort); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/wled.cpp` around lines 571 - 572, The DMX TX pin is being allocated too late (dmxOutput.init is called after beginStrip() and usermod setup), so other buses/usermods can claim it first; move the reservation earlier by reserving the configured DMX GPIO with the PinManager before any bus or usermod claims it (either call dmxOutput.init(dmxOutputPin, 43) before beginStrip()/usermod setup or explicitly call the PinManager reserve API for the TX pin—e.g., pinManager.reservePin(dmxOutputPin, /*owner*/ DMX_TX) —so the pin is locked for DMX use prior to beginStrip() and usermod initialization).
🧹 Nitpick comments (4)
wled00/dmx_output.cpp (2)
157-167: Consider integer math fortimeToNextUpdate().The float division + conversion + ceil-rounding can be expressed as pure integer math (
(1000 + _updateRate - 1) / _updateRate) which avoids pulling the softfloat path on ESP8266 and is simpler to reason about. This function is called on every loop iteration.♻️ Proposed refactor
- // treat _updateRate as maximum, so round up the refresh delay - float fdmxFrameTime = 1000.0 / _updateRate; - int dmxFrameTime = (uint16_t)fdmxFrameTime; - if(fdmxFrameTime - dmxFrameTime > 0.0) dmxFrameTime += 1; // if fractional part > 0, add one - - return dmxFrameTime - (millis() - _lastDmxOutMillis); + // treat _updateRate as maximum, so round up the refresh delay + const int dmxFrameTime = (1000 + _updateRate - 1) / _updateRate; + return dmxFrameTime - (int)(millis() - _lastDmxOutMillis);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 157 - 167, The timeToNextUpdate() function uses float math to compute frame delay which pulls in softfloat on ESP8266 and is inefficient; replace the float/ceil logic with integer arithmetic by computing dmxFrameTime = (1000 + _updateRate - 1) / _updateRate (handle _updateRate==0 earlier as already done), then return dmxFrameTime - (millis() - _lastDmxOutMillis) while keeping the existing early checks for _uartNo < 0 and _updateRate == 0; update only DMXOutput::timeToNextUpdate() and keep references to _updateRate, _lastDmxOutMillis, _uartNo and millis() intact.
5-10:uart.hinclude no longer needed.After inlining the FIFO check with
USS()/USTXC(line 131), onlyesp8266_peri.his actually used from these two headers;uart.hcan be dropped from the ESP8266 branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 5 - 10, Remove the now-unneeded uart.h include from the ESP8266 branch in dmx_output.cpp: keep only `#include` "esp8266_peri.h" under the `#ifdef` ESP8266 conditional since the FIFO check was inlined using USS()/USTXC; delete the `#include` "uart.h" line and ensure the conditional block still correctly selects esp8266_peri.h for ESP8266 and hal/uart_ll.h for other platforms.wled00/dmx_output.h (2)
53-92: Mark getter/query methodsconst.
read,readBytes,getLastDmxOut,getUpdateRate,busy, andtimeToNextUpdatedon't mutate instance state and should beconst, matching the project guideline and making them callable from const contexts. (readBytescurrently takes a non-const out-buffer — that's fine, only the method itself needs const qualification.)♻️ Proposed fix
- uint8_t read(uint16_t channel); + uint8_t read(uint16_t channel) const; ... - bool readBytes(uint16_t channelStart, uint8_t values[], uint16_t len); + bool readBytes(uint16_t channelStart, uint8_t values[], uint16_t len) const; ... - unsigned long getLastDmxOut(); + unsigned long getLastDmxOut() const; ... - bool busy(); + bool busy() const; ... - uint8_t getUpdateRate(); + uint8_t getUpdateRate() const; ... - int timeToNextUpdate(); + int timeToNextUpdate() const;(Remember to mirror the
conston the corresponding definitions indmx_output.cpp.)As per coding guidelines: "Mark getter/query methods as
constand usestaticfor methods not accessing instance state".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.h` around lines 53 - 92, Several accessor/query methods in the DMX output class are not marked const; change the declarations of read(uint16_t channel), readBytes(uint16_t channelStart, uint8_t values[], uint16_t len), getLastDmxOut(), getUpdateRate(), busy(), and timeToNextUpdate() to be const methods (e.g., uint8_t read(uint16_t channel) const), and update the matching definitions in dmx_output.cpp to include the same const qualifier so they are callable from const contexts and adhere to the project guideline.
24-30: Preferconstexprover#definefor these compile-time constants.
DMX_CHANNEL_TOP,DMX_CHANNELS,DMXSPEED,BREAKSPEEDare plain integer constants and don't need preprocessor scoping.DMXFORMAT/BREAKFORMATresolve toSERIAL_8N2/SERIAL_8N1which areuint32_tenum-ish values — also representable asconstexpr. This gives them type safety and keeps them out of the global macro namespace (whereDMX_CHANNELSrisks collision with theDMXChannelsglobal variable used inhandleDMXOutput()).♻️ Proposed refactor
-#define DMX_CHANNEL_TOP 512 -#define DMX_CHANNELS (DMX_CHANNEL_TOP + 1) - -#define DMXSPEED 250000 -#define DMXFORMAT SERIAL_8N2 -#define BREAKSPEED 83000 -#define BREAKFORMAT SERIAL_8N1 // unused, instead, DMXFORMAT is used +constexpr uint16_t DMX_CHANNEL_TOP = 512; +constexpr uint16_t DMX_CHANNELS = DMX_CHANNEL_TOP + 1; +constexpr uint32_t DMXSPEED = 250000; +constexpr uint32_t BREAKSPEED = 83000; +constexpr auto DMXFORMAT = SERIAL_8N2; +constexpr auto BREAKFORMAT = SERIAL_8N1; // unused, instead, DMXFORMAT is usedAs per coding guidelines: "Prefer
constexprover#definefor compile-time constants in C++".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.h` around lines 24 - 30, Replace the preprocessor macros with typed constexpr variables: change DMX_CHANNEL_TOP, DMX_CHANNELS, DMXSPEED, BREAKSPEED to constexpr integers (e.g., int or std::size_t where appropriate) and change DMXFORMAT and BREAKFORMAT to constexpr uint32_t (or the exact enum type that SERIAL_8N2/SERIAL_8N1 resolve to); update any includes if needed (e.g., <cstdint>) and ensure references such as DMXChannels in handleDMXOutput() still compile (avoiding macro collisions) by removing the `#define` lines and declaring the new constexpr symbols with the same names.
🤖 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/dmx_output.cpp`:
- Line 131: The declaration of uart_tx_fifo_available (using USS(_uartNo) >>
USTXC) is missing its terminating semicolon which causes compilation failure;
update the statement in dmx_output.cpp where uart_tx_fifo_available is defined
(referencing the symbols uart_tx_fifo_available, USS, USTXC, _uartNo) by adding
the missing semicolon to the end of the line so the local variable declaration
is properly terminated.
- Around line 63-72: The end() method in DMXOutput leaks the TX pin because
init() allocates it via PinManager::allocatePin(_outputPin, true,
PinOwner::DMX_OUTPUT) but end() never releases it; modify DMXOutput::end() to
call PinManager::deallocatePin(_outputPin, PinOwner::DMX_OUTPUT) (or the
appropriate deallocation API) before clearing _outputPin/_uartNo and deleting
_dmxSerial, and also ensure any failure paths in DMXOutput::init() deallocate
the pin if allocation succeeded but initialization failed so the pin is not
permanently owned by DMX_OUTPUT.
In `@wled00/dmx_output.h`:
- Around line 94-101: The member _updateRate of class DMXOutput is left
uninitialized which can cause undefined behavior if accessed before init(); set
a safe in-class initializer for _updateRate (for example 0) in the DMXOutput
declaration so any global/default-constructed DMXOutput has a well-defined
value; update the member declaration for _updateRate and ensure callers such as
timeToNextUpdate() and init() rely on that initialized value.
---
Duplicate comments:
In `@wled00/e131.cpp`:
- Around line 119-121: The proxying currently always uses the E1.31 payload
offset (&e131_data[1]) which misaligns Art‑Net packets (art_data starts at
art_data[0]) and can read past the buffer on 512‑channel frames; update the
proxy logic in the block checking e131ProxyUniverse/uni to choose the correct
pointer and offset based on the source protocol (use art_data for Art‑Net
starting at index 0, and e131_data starting at index 1 for E1.31) when calling
dmxOutput.writeBytes and ensure dmxChannels bounds are respected; locate the
branch around e131ProxyUniverse, uni, dmxOutput.writeBytes and adjust to pass
the protocol‑appropriate buffer pointer and length.
In `@wled00/set.cpp`:
- Around line 477-479: The code assigns dmxOutputPin =
request->arg(F("IDMO")).toInt() which silently converts empty/malformed input to
0 and loses the -1 sentinel, and it only updates the stored value without
reinitializing the DMX driver (initialized in WLED::setup()), so changes don't
take effect until reboot; fix by parsing and validating the raw string from
request->arg(F("IDMO")) (preserve -1 as a valid "disabled" value and reject
non-numeric input), update the stored dmxOutputPin only if the parsed value is
valid, and when the pin actually changes either reinitialize the DMX output
driver immediately (call the driver init/reset function used in WLED::setup() —
reference the same DMX init routine) or mark the config as needing an
apply/reboot and return a response instructing the user to reboot/apply.
In `@wled00/wled.cpp`:
- Around line 571-572: The DMX TX pin is being allocated too late
(dmxOutput.init is called after beginStrip() and usermod setup), so other
buses/usermods can claim it first; move the reservation earlier by reserving the
configured DMX GPIO with the PinManager before any bus or usermod claims it
(either call dmxOutput.init(dmxOutputPin, 43) before beginStrip()/usermod setup
or explicitly call the PinManager reserve API for the TX pin—e.g.,
pinManager.reservePin(dmxOutputPin, /*owner*/ DMX_TX) —so the pin is locked for
DMX use prior to beginStrip() and usermod initialization).
---
Nitpick comments:
In `@wled00/dmx_output.cpp`:
- Around line 157-167: The timeToNextUpdate() function uses float math to
compute frame delay which pulls in softfloat on ESP8266 and is inefficient;
replace the float/ceil logic with integer arithmetic by computing dmxFrameTime =
(1000 + _updateRate - 1) / _updateRate (handle _updateRate==0 earlier as already
done), then return dmxFrameTime - (millis() - _lastDmxOutMillis) while keeping
the existing early checks for _uartNo < 0 and _updateRate == 0; update only
DMXOutput::timeToNextUpdate() and keep references to _updateRate,
_lastDmxOutMillis, _uartNo and millis() intact.
- Around line 5-10: Remove the now-unneeded uart.h include from the ESP8266
branch in dmx_output.cpp: keep only `#include` "esp8266_peri.h" under the `#ifdef`
ESP8266 conditional since the FIFO check was inlined using USS()/USTXC; delete
the `#include` "uart.h" line and ensure the conditional block still correctly
selects esp8266_peri.h for ESP8266 and hal/uart_ll.h for other platforms.
In `@wled00/dmx_output.h`:
- Around line 53-92: Several accessor/query methods in the DMX output class are
not marked const; change the declarations of read(uint16_t channel),
readBytes(uint16_t channelStart, uint8_t values[], uint16_t len),
getLastDmxOut(), getUpdateRate(), busy(), and timeToNextUpdate() to be const
methods (e.g., uint8_t read(uint16_t channel) const), and update the matching
definitions in dmx_output.cpp to include the same const qualifier so they are
callable from const contexts and adhere to the project guideline.
- Around line 24-30: Replace the preprocessor macros with typed constexpr
variables: change DMX_CHANNEL_TOP, DMX_CHANNELS, DMXSPEED, BREAKSPEED to
constexpr integers (e.g., int or std::size_t where appropriate) and change
DMXFORMAT and BREAKFORMAT to constexpr uint32_t (or the exact enum type that
SERIAL_8N2/SERIAL_8N1 resolve to); update any includes if needed (e.g.,
<cstdint>) and ensure references such as DMXChannels in handleDMXOutput() still
compile (avoiding macro collisions) by removing the `#define` lines and declaring
the new constexpr symbols with the same names.
🪄 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: 86e36cfb-18e2-4b1e-b3d5-f9e4693dc53f
📒 Files selected for processing (11)
wled00/cfg.cppwled00/data/settings_dmx.htmwled00/data/settings_sync.htmwled00/dmx_input.cppwled00/dmx_output.cppwled00/dmx_output.hwled00/e131.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
✅ Files skipped from review due to trivial changes (2)
- wled00/data/settings_dmx.htm
- wled00/cfg.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/data/settings_sync.htm
Comment on lines
+94
to
+101
| private: | ||
| HardwareSerial* _dmxSerial; | ||
| uint8_t _outputPin; // DMX TX pin | ||
| uint8_t _dmxData[DMX_CHANNELS] = {0}; | ||
| int8_t _uartNo = -1; | ||
| uint8_t _updateRate; | ||
| unsigned long _lastDmxOutMillis = 0; | ||
| }; |
Contributor
There was a problem hiding this comment.
Initialize _updateRate in the member declaration.
_updateRate has no in-class initializer, so a default-constructed global DMXOutput dmxOutput leaves it indeterminate until init() is called. timeToNextUpdate() guards via _uartNo < 0 today, but any future call path that reads _updateRate before init() would hit UB. Trivial to make watertight:
♻️ Proposed fix
int8_t _uartNo = -1;
- uint8_t _updateRate;
+ uint8_t _updateRate = 43;
unsigned long _lastDmxOutMillis = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/dmx_output.h` around lines 94 - 101, The member _updateRate of class
DMXOutput is left uninitialized which can cause undefined behavior if accessed
before init(); set a safe in-class initializer for _updateRate (for example 0)
in the DMXOutput declaration so any global/default-constructed DMXOutput has a
well-defined value; update the member declaration for _updateRate and ensure
callers such as timeToNextUpdate() and init() rely on that initialized value.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wled00/dmx_output.cpp (2)
159-169: Optional: integer-only ceiling math intimeToNextUpdate().
fdmxFrameTime/ cast / fractional check can be replaced with a single integer ceiling expression, avoiding the FP divide/subtract on every call (this is invoked from the hot service path viaupdate()).♻️ Proposed refactor
- // treat _updateRate as maximum, so round up the refresh delay - float fdmxFrameTime = 1000.0 / _updateRate; - int dmxFrameTime = (uint16_t)fdmxFrameTime; - if(fdmxFrameTime - dmxFrameTime > 0.0) dmxFrameTime += 1; // if fractional part > 0, add one - - return dmxFrameTime - (millis() - _lastDmxOutMillis); + // treat _updateRate as maximum, so round up the refresh delay + const int dmxFrameTime = (1000 + _updateRate - 1) / _updateRate; // integer ceil(1000 / _updateRate) + return dmxFrameTime - (int)(millis() - _lastDmxOutMillis);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 159 - 169, Replace the floating-point ceiling logic in DMXOutput::timeToNextUpdate with integer-only math: compute the frame period as an integer ceiling using (_updateRate) and integer arithmetic (e.g., (1000 + _updateRate - 1) / _updateRate) instead of using fdmxFrameTime, casting and fractional checks; keep the same early returns for _uartNo and _updateRate and return the same expression dmxFrameTime - (millis() - _lastDmxOutMillis) using the new integer dmxFrameTime variable so the hot path avoids FP divide/subtract.
148-157: Mark pure getters asconst.
getLastDmxOut()andgetUpdateRate()don't modify instance state and should beconst; same applies totimeToNextUpdate()(Line 159) — it only reads_uartNo,_updateRate,_lastDmxOutMillis. The header declaration will need to match.-unsigned long DMXOutput::getLastDmxOut() { +unsigned long DMXOutput::getLastDmxOut() const { return _lastDmxOutMillis; } ... -uint8_t DMXOutput::getUpdateRate() { +uint8_t DMXOutput::getUpdateRate() const { return _updateRate; }As per coding guidelines: "Mark getter/query methods as
constand usestaticfor methods not accessing instance state".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 148 - 157, The getters getLastDmxOut(), getUpdateRate() and the method timeToNextUpdate() are pure queries and should be marked const; update their definitions in the .cpp (DMXOutput::getLastDmxOut(), DMXOutput::getUpdateRate(), DMXOutput::timeToNextUpdate()) to end with const and make the matching declarations in the header const as well, and if you find any method that does not access instance state at all consider marking it static; ensure signatures in both header and implementation match to fix const-correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/dmx_output.cpp`:
- Around line 159-169: Replace the floating-point ceiling logic in
DMXOutput::timeToNextUpdate with integer-only math: compute the frame period as
an integer ceiling using (_updateRate) and integer arithmetic (e.g., (1000 +
_updateRate - 1) / _updateRate) instead of using fdmxFrameTime, casting and
fractional checks; keep the same early returns for _uartNo and _updateRate and
return the same expression dmxFrameTime - (millis() - _lastDmxOutMillis) using
the new integer dmxFrameTime variable so the hot path avoids FP divide/subtract.
- Around line 148-157: The getters getLastDmxOut(), getUpdateRate() and the
method timeToNextUpdate() are pure queries and should be marked const; update
their definitions in the .cpp (DMXOutput::getLastDmxOut(),
DMXOutput::getUpdateRate(), DMXOutput::timeToNextUpdate()) to end with const and
make the matching declarations in the header const as well, and if you find any
method that does not access instance state at all consider marking it static;
ensure signatures in both header and implementation match to fix
const-correctness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcc131bf-6314-4747-87d7-5da36ca2d87b
📒 Files selected for processing (1)
wled00/dmx_output.cpp
Author
|
I've changed back to WLED_ENABLE_DMX without _OUTPUT as requested. Note that I kept the DMX_OUTPUT naming with PinManager. Hope that's okay. |
Collaborator
|
As this moves away from any library (which I am not at all opposed to), it requires thorough testing on all supported platforms. |
Author
|
True in general. As I said in the initial message, the principle is the same between SparkFunDMX, DMXESPOutput (used for ESP8266) and still very similar to this module. The difference in mine is that i set Maybe if I find time, I'll add more UI improvements to allow the TX pin to be changed without rebooting. And add an input field to set the DMXOutput serial port (like with DMXInput) and update rate. |
Collaborator
In my experience each and every ESP can act differently as the hardware implementations are not the same and they do have quirks, so testing is absolutely required. |
Author
|
Successfully tested WLED as Art-Net to DMX with ESP32WROVER and Cameo and Eurolight fixtures. |
1 task
|
Successfully tested this on esp32c3dev (Pin 5) using a SN75176B IC and a Eurolite fixture |
Member
|
@coderabbitai please compare the new implementation in this PR with the previous code in |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27401
✅ Behaviors preserved
|
| Aspect | Old ESPDMX (ESP8266) | Old SparkFunDMX (ESP32) | New DMXOutput |
|---|---|---|---|
| Break format | SERIAL_8N1 ✅ | SERIAL_8N1 ✅ | SERIAL_8N2 |
| Break baud | 83333 | 83333 | 83000 |
| TX pin HIGH before break | Yes | Yes | No |
| Blocking per frame | ~23ms | ~23ms | ~150µs (non-blocking) |
| Channel 512 sent | No (bug) | Yes | Yes ✅ |
| Start code enforcement | Zero-init only | dmxData[0]=0 on every write() |
Zero-init only |
| Universe overflow guard | No | No | Yes ✅ |
| Dynamic channel count | Yes | Yes | No (fixed 513) |
The break format issue (point 1) is the most critical one worth verifying on an oscilloscope — does the break pulse duration and framing match what DMX receivers expect?
🧠 Learnings used
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: 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: 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: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
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: 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: 4838
File: wled00/const.h:90-102
Timestamp: 2026-04-20T19:04:17.626Z
Learning: In WLED (wled00/const.h), ESP-IDF v5.3's `soc/soc_caps.h` defines `SOC_RMT_TX_CANDIDATES_PER_GROUP` which matches exactly the WLED_MAX_RMT_CHANNELS hardcoded values per target: esp32=8, esp32s2=4, esp32s3=4, esp32c3=2, esp32c6=2, esp32p4=4. This macro could replace the per-target `#elif` chain for WLED_MAX_RMT_CHANNELS using `SOC_RMT_TX_CANDIDATES_PER_GROUP * SOC_RMT_GROUPS`. However, SOC_I2S_NUM (hardware peripheral count: 2/1/2/1/1/3) cannot replace WLED_MAX_I2S_CHANNELS, which represents NPB parallel DMA output capability (8/8/8/0/0/?) and is a library-support concept, not a hardware count. The soc_caps.h header is not officially stable public API but has been consistent in practice since IDF 4.4.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
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: 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.
Member
|
@coderabbitai the previous implementation in Is the refresh rate limit problem addressed properly in this PR? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10899
Dual-gate in
|
Member
|
@Mdbelen please check if any of the differences listed by the rabbit in #5522 (comment) would need a fix in your new code. |
Member
|
@coderabbitai please check if this PR implies other behavioral changes (user-visible and not yet covered by the PR description) or ripple effects that should be considered by reviewers. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 34209
🐛 1.
|
| # | Category | Severity | Covered in PR desc? |
|---|---|---|---|
| 1 | dmxOutputPin → GPIO0 on fresh install |
Bug | ❌ No |
| 2 | ESP8266 DEFAULT_LED_PIN 1→2 migration |
Migration | ❌ No |
| 3 | No reboot warning for pin change in UI | UX gap | ❌ No |
| 4 | Update rate hardcoded, no UI to change | Incomplete feature | Partial |
| 5 | WLED_ENABLE_DMX_OUTPUT flag removed |
Breaking for custom builds | ❌ No |
| 6 | E1.31 proxy rate-capped at 43 Hz | Improvement | ❌ No |
| 7 | ESP8266 pin ≠ 2 silently fails | UX gap | ❌ No |
| 8 | HardwareSerial heap-allocated |
Style change | ❌ No |
🧠 Learnings used
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: 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: 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: 4838
File: wled00/const.h:90-102
Timestamp: 2026-04-20T19:04:17.626Z
Learning: In WLED (wled00/const.h), ESP-IDF v5.3's `soc/soc_caps.h` defines `SOC_RMT_TX_CANDIDATES_PER_GROUP` which matches exactly the WLED_MAX_RMT_CHANNELS hardcoded values per target: esp32=8, esp32s2=4, esp32s3=4, esp32c3=2, esp32c6=2, esp32p4=4. This macro could replace the per-target `#elif` chain for WLED_MAX_RMT_CHANNELS using `SOC_RMT_TX_CANDIDATES_PER_GROUP * SOC_RMT_GROUPS`. However, SOC_I2S_NUM (hardware peripheral count: 2/1/2/1/1/3) cannot replace WLED_MAX_I2S_CHANNELS, which represents NPB parallel DMA output capability (8/8/8/0/0/?) and is a library-support concept, not a hardware count. The soc_caps.h header is not officially stable public API but has been consistent in practice since IDF 4.4.
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: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
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: 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: 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: 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: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
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: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-12T07:15:56.657Z
Learning: Document changes in pull requests with clear description of what changed and why, including expected impact on user-visible behavior, related issue links, and screenshots for new features
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: 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: 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.
softhack007
reviewed
Apr 30, 2026
| for (uint16_t i = 1; i <= dmxChannels; i++) | ||
| dmx.write(i, e131_data[i]); | ||
| dmx.update(); | ||
| dmxOutput.writeBytes(1, &e131_data[1], dmxChannels); |
Member
There was a problem hiding this comment.
Should this be e131_data[0] in Art-Net mode? Afaik the first channel offset is 0 for Art-Net, but 1 for other E131 protocols?
Member
There was a problem hiding this comment.
@coderabbitai please verify if my concern is correct here.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how e131_data is assigned per protocol and confirm the Art-Net vs E1.31 offset difference
rg -n "e131_data\s*=" wled00/e131.cpp
rg -n "art_data\|property_values\|dataOffset\|REALTIME_MODE_ARTNET" wled00/e131.cpp | head -40
Length of output: 107
@softhack007, your concern is correct. Here is the analysis:
The e131_data pointer is assigned differently per protocol:
- Art-Net (Line 93):
e131_data = p->art_data;→ DMX channel 1 lives at index 0 - E1.31 (Line 103):
e131_data = p->property_values;→ index 0 is the zero start-code byte, DMX channel 1 lives at index 1
The proxy universe block:
dmxOutput.writeBytes(1, &e131_data[1], dmxChannels);…passes &e131_data[1] unconditionally. That is correct for E1.31 but off by one for Art-Net: it silently drops DMX channel 1 and potentially reads one byte past the declared dmxChannels length.
However, this is a pre-existing bug, not a regression introduced by this PR. The old code had identical behaviour:
for (uint16_t i = 1; i <= dmxChannels; i++)
dmx.write(i, e131_data[i]); // same: starts at [1], skips channel 1 for Art-NetThe handleDMXData() function does compensate for the Art-Net offset via if (mde == REALTIME_MODE_ARTNET && dataOffset > 0) dataOffset--;, but that adjustment is not applied in the proxy universe path.
A correct fix for this PR would be to mirror that adjustment in the proxy block:
`#ifdef` WLED_ENABLE_DMX
// does not act on out-of-order packets yet
if (e131ProxyUniverse > 0 && uni == e131ProxyUniverse) {
// Art-Net data starts at index 0; E1.31 data starts at index 1 (index 0 is zero start-code)
uint8_t* proxyData = (mde == REALTIME_MODE_ARTNET) ? &e131_data[0] : &e131_data[1];
dmxOutput.writeBytes(1, proxyData, dmxChannels);
dmxOutput.update();
}
`#endif`In summary: your concern is valid, the bug predates this PR, but this PR is the right place to fix it since the code is being reworked anyway.
🧠 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: 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: 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: 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: 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: 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: 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/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: 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: 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: 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: 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: 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: 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: 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: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
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.
Member
There was a problem hiding this comment.
However, this is a pre-existing bug, not a regression introduced by this PR.
As the PR is a major modernization/overhaul of previous code, I would like to see this old problem addressed, too.
Contributor
There was a problem hiding this comment.
@softhack007, agreed — since this is a comprehensive overhaul, it's the right time to fix it. Here is the proposed change:
- Fix the Art-Net vs E1.31 start-index mismatch in the proxy universe path in
wled00/e131.cpp
🧠 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: 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: 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: 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: 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: 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: 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/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: 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: 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: 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-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: 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: 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: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.
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: 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: 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: 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: 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: 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: softhack007
Repo: wled/WLED PR: 4838
File: platformio.ini:149-150
Timestamp: 2026-03-29T16:47:56.452Z
Learning: In WLED PR `#4838` (ESP-IDF V5 branch, platformio.ini): The original FastLED library has been replaced with a custom `fastled-slim` fork. As a result, the `[v5_pioarduino_workaround]` section (containing `lib_archive = yes`) and the commented-out `post:pio-scripts/fastled_cxx_workaround.py` extra script are intentional dead code pending removal after back-to-back testing. Do NOT flag `lib_archive = yes` in `[v5_pioarduino_workaround]` as unreferenced or suggest moving it to active env blocks — the entire section is slated for removal.
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:3-5
Timestamp: 2026-02-18T00:11:02.403Z
Learning: In PR `#4585`, the seven_segment_display_reloaded_v2 usermod intentionally uses `#error` to require MQTT (`#ifdef WLED_DISABLE_MQTT `#error` "This user mod requires MQTT to be enabled." `#endif``) because the integration flow has not been tested or validated without MQTT, and failing fast at compile time is preferred over allowing potentially broken builds until a full MQTT-optional refactor is completed.
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.
Failed to handle agent chat message. Please try again.
Author
There was a problem hiding this comment.
Is there any bug report about this? I kept everything as-is because I figured this would lead to a misalignment by one channel for Art-Net (or E1.31) and somebody would have reported for sure. So I thought e131.cpp handles this somewhere internally by biasing the channels for me already.
Will look into it.
Contributor
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!
Author
|
This btw also fixes the bug covered in #5316 . @softhack007 Thanks for chasing the rabbit though this PR! Here's some comments to the findings:
According to wikipedia, DMX defines only minimum times for break and MAB. As noted in comment on dmx_output.cpp:114
As the new implementation keeps the UART "alive" all the time and never releases the pin to GPIO, this is unnecessary in all cases.
I tried to do a real pessimistic implementation, so I did a 0,5% increase in break time here to account for possible crystal drifts 🙃 (this is calculated from the wikipedia's 120us, as before).
I believe this bug is untrue, as dmxOutputPin is initialized to -1 in wled.h and the CJSON macro will do the suggested handling already. Can you confirm this @softhack007 @coderabbitai ?
I didn't remember changing anything about LED pins haha. Turns out this comes from @netmindz cherry-picked commit. AI comment makes sense to me, so I'll revert those lines, k? ESP8266 can only TX DMX (Serial1) on pin 2.
True, planned to add to a future release. Will probably add to this PR very soon IMO that covers all important AI findings. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The `#define` CJSON(a,b) a = b | a
// i.e.:
dmxOutputPin = if_live_dmx[F("dmxOutputPin")] | dmxOutputPin;The ArduinoJSON Here's the final status summary of all findings:
✏️ Learnings added
🧠 Learnings used |
coderabbitai
Bot
added
the
optimization
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/dmx_output.cpp (1)
181-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp
DMXChannelsbefore indexingDMXFixtureMap.
DMXFixtureMaphas 15 entries, but both loops run up toDMXChannels. A malformed config value above 15 reads past the map and turns channel generation into undefined behavior.🛠️ Proposed fix
bool calc_brightness = true; + const uint8_t channelCount = (DMXChannels > sizeof(DMXFixtureMap)) ? sizeof(DMXFixtureMap) : DMXChannels; // check if no shutter channel is set - for (unsigned i = 0; i < DMXChannels; i++) { + for (uint8_t i = 0; i < channelCount; i++) { if (DMXFixtureMap[i] == 5) calc_brightness = false; } @@ - for (int j = 0; j < DMXChannels; j++) { + for (uint8_t j = 0; j < channelCount; j++) { int DMXAddr = DMXFixtureStart + j; switch (DMXFixtureMap[j]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/dmx_output.cpp` around lines 181 - 224, Clamp DMXChannels to the actual size of DMXFixtureMap before any indexing to avoid out-of-bounds access: compute a uint8_t safeChannels = min(DMXChannels, (int)(sizeof(DMXFixtureMap)/sizeof(DMXFixtureMap[0]))); then replace uses of DMXChannels in both for loops and any indexing (the loop that checks DMXFixtureMap and the inner switch that uses DMXFixtureMap[j]) with safeChannels (or use safeChannels as the loop bound) so all accesses to DMXFixtureMap[j] are within bounds.
🧹 Nitpick comments (1)
wled00/data/settings_sync.htm (1)
51-56: ⚡ Quick winWire
IDMOinto the existing pin-selector flow.The new DMX output pin stays a raw number input, so it misses the availability/conflict filtering that
IDMR,IDMT, andIDMEalready get throughmakePinSelect()anddmxPinUpd(). AddingIDMOto the same list keeps the UI consistent and prevents selecting obviously bad/owned pins.♻️ Proposed change
-const dmxNs = ["IDMR","IDMT","IDME"]; +const dmxNs = ["IDMR","IDMT","IDME","IDMO"];As per coding guidelines: "When updating Web UI files in
wled00/data/, make use of common functions inwled00/data/common.jswhenever possible."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sync.htm` around lines 51 - 56, The DMX output pin IDMO is not included in the pin-selector flow so it remains a raw number input and misses availability/conflict filtering; update the dmxNs array to include "IDMO" alongside "IDMR","IDMT","IDME" so that makePinSelect() is called for it and its onchange is wired to dmxPinUpd(), and ensure dmxOwnPins is computed using gN(...) for the new IDMO entry so the existing dmxPinUpd() logic applies without further changes.
🤖 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/data/settings_sync.htm`:
- Around line 205-208: The DMX output setting block (div id "dmxOutput" with
input name "IDMO") is missing a reboot-required hint; update the UI markup for
the DMX TX/IDMO control to include the same explicit note shown in the DMX input
block that changes take effect only after restart, and mention that the value is
applied in WLED::setup() so users must reboot for the new DMX output pin to take
effect.
In `@wled00/dmx_output.cpp`:
- Around line 99-103: DMXOutput::readBytes uses channelStart + len which can
overflow; replace that check with a non-overflowing test such as verifying
channelStart is within range and len does not exceed the remaining channels
(e.g., check channelStart >= DMX_CHANNELS || len > DMX_CHANNELS - channelStart)
before calling memcpy on _dmxData so out-of-bounds reads are prevented.
---
Outside diff comments:
In `@wled00/dmx_output.cpp`:
- Around line 181-224: Clamp DMXChannels to the actual size of DMXFixtureMap
before any indexing to avoid out-of-bounds access: compute a uint8_t
safeChannels = min(DMXChannels,
(int)(sizeof(DMXFixtureMap)/sizeof(DMXFixtureMap[0]))); then replace uses of
DMXChannels in both for loops and any indexing (the loop that checks
DMXFixtureMap and the inner switch that uses DMXFixtureMap[j]) with safeChannels
(or use safeChannels as the loop bound) so all accesses to DMXFixtureMap[j] are
within bounds.
---
Nitpick comments:
In `@wled00/data/settings_sync.htm`:
- Around line 51-56: The DMX output pin IDMO is not included in the pin-selector
flow so it remains a raw number input and misses availability/conflict
filtering; update the dmxNs array to include "IDMO" alongside
"IDMR","IDMT","IDME" so that makePinSelect() is called for it and its onchange
is wired to dmxPinUpd(), and ensure dmxOwnPins is computed using gN(...) for the
new IDMO entry so the existing dmxPinUpd() logic applies without further
changes.
🪄 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: f56fb0d6-95e8-4308-94b3-77f9557e32f2
📒 Files selected for processing (8)
wled00/data/settings_sync.htmwled00/dmx_input.cppwled00/dmx_output.cppwled00/dmx_output.hwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
✅ Files skipped from review due to trivial changes (1)
- wled00/dmx_input.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/set.cpp
- wled00/xml.cpp
Comment on lines
+205
to
+208
| <div id="dmxOutput" style="display: none"> | ||
| <h3>Wired DMX Output Pin</h3> | ||
| DMX TX: <input name="IDMO" type="number" min="-1" max="99"><br/> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Add the missing reboot-required hint for IDMO.
This setting is only applied during WLED::setup(), so saving a new DMX output pin will not take effect until restart. The new section should say that explicitly, like the DMX input block does.
💡 Proposed change
<div id="dmxOutput" style="display: none">
<h3>Wired DMX Output Pin</h3>
DMX TX: <input name="IDMO" type="number" min="-1" max="99"><br/>
+ <i class="warn">Reboot required to apply changes.</i>
</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/data/settings_sync.htm` around lines 205 - 208, The DMX output setting
block (div id "dmxOutput" with input name "IDMO") is missing a reboot-required
hint; update the UI markup for the DMX TX/IDMO control to include the same
explicit note shown in the DMX input block that changes take effect only after
restart, and mention that the value is applied in WLED::setup() so users must
reboot for the new DMX output pin to take effect.
Comment on lines
+99
to
+103
| bool DMXOutput::readBytes(uint16_t channelStart, uint8_t values[], uint16_t len) { | ||
| if(channelStart + len > DMX_CHANNELS) return false; // out of bounds | ||
|
|
||
| memcpy(values, &_dmxData[channelStart], len); | ||
| return true; |
Contributor
There was a problem hiding this comment.
Use a non-overflowing bounds check before memcpy.
channelStart + len > DMX_CHANNELS can wrap for large inputs, so calls like readBytes(65535, buf, 2) bypass the guard and read past _dmxData.
🛡️ Proposed fix
bool DMXOutput::readBytes(uint16_t channelStart, uint8_t values[], uint16_t len) {
- if(channelStart + len > DMX_CHANNELS) return false; // out of bounds
+ if(channelStart > DMX_CHANNELS) return false;
+ if(len > DMX_CHANNELS - channelStart) return false; // out of bounds
memcpy(values, &_dmxData[channelStart], len);
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/dmx_output.cpp` around lines 99 - 103, DMXOutput::readBytes uses
channelStart + len which can overflow; replace that check with a non-overflowing
test such as verifying channelStart is within range and len does not exceed the
remaining channels (e.g., check channelStart >= DMX_CHANNELS || len >
DMX_CHANNELS - channelStart) before calling memcpy on _dmxData so out-of-bounds
reads are prevented.
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
5 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.
tl;dr:
Related to #5216 also see previous comments there. This is an alternative to make dmx.update() non-blocking. With less dependencies, more unified between ESP8266&32 and I claim more stable.
Fixes #5133
The core essence of this PR is dmx_output.cpp:57, the rest is cosmetics.
Long:
Independent from https://github.com/Stijn-Jacobs I also encountered the problem that once DMX output gets enabled in v0.15.3, WLED becomes super laggy regarding LED output via ArtNet, and almost unreachable via network. Short digging showed, that the DMX output is implemented in a blocking way, leading to 23ms of CPU time per loop run just for DMX.
This is unacceptable.
Stijn kindly prepared a fix in 5216 and I made mine in parallel locally. While I evaluated Stijns solution, I saw on oscilloscope, that the DMX frame initial MAB pulse, which is supposed to be at least 12us, sometimes is only 8us. I consider this suboptimal and would feel a little unsure using this. The used esp_dmx dependency is comparatively super complicated for what DMX needs, so I decided to post my solution as an alternative here.
The DMX part is quite trivial and the same for DMXESPOutput and SparkFunDMX. Now that DMX Input has been moved to another module altogether, I saw no more reason to keep those two separate dependencies and merged both in this new DMXOutput module. ESP8266 is still mostly blocking (due to limitations in HAL/Arduino), while ESP32 is almost completely non-blocking and takes less than 1ms per run.
The module is prepared to have an update rate element added to the web UI in future. Setting update rate to 0 ensures the max possible rate. Maximum with all 512/513 DMX channels is 43Hz, this is currently the default.
[outdated, see comments: I decided to also change the #defines to better differentiate between the new DMXInput and DMXOutput modules. The new way to enable DMXOutput is to set WLED_ENABLE_DMX_OUTPUT instead of WLED_ENABLE_DMX, which I found confusing.]
Also, there's the possibility to set the initial TX pin using DMX_TXPIN_DEFAULT=1337. kno.wled.ge would need an update here.
In this branch, I also included some commits from netmindz #5287 adding TX pin setting on web UI, and the update rate limiting from Stijn (sadly couldn't cherry-pick as was mixed with too many other changes).
I did not use the number of DMX-fixtures UI element, because I consider it irrelevant for ESP32 and found it more confusing. This would lower the processor and RAM load marginally. It could be possibly helpful for ESP8266, but I think this is not the focus here.
This is tested on my ESP32 hardware with an oscilloscope. Didn't get the chance to connect my DMX fixtures, yet, but I'm pretty sure that will work. Untested on ESP8266, no hardware available.
Summary by CodeRabbit
New Features
Improvements