DMX improvements and configuration option. #5216
Conversation
…of fixtures configuration option to the DMX settings.
WalkthroughAdds a DMX fixture-count parameter (DMXNumFixtures) across config, UI, and runtime; limits DMX output to that fixture range and adds per-frame rate guards; removes SparkFun DMX implementation and adds an ESP32 esp_dmx-based DMX output backend. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wled00/data/settings_dmx.htm (1)
66-67: Consider using MAX_LEDS constant instead of hardcoded 1500.Line 66 uses
max="1500"for the DMX start LED input, but this should ideally reference theMAX_LEDSconstant to stay consistent with the configured LED limit.Based on learnings, magic numbers like this can be addressed in context.
Note: The
max="512"for number of fixtures is appropriate as it matches the DMX universe limit.wled00/dmx_output.cpp (1)
38-45: Remove redundant strip.getBrightness() call.Line 41 calls
strip.getBrightness()again, butbrightnesswas already assigned from this function at line 30. Use the cached variable instead:🔎 Proposed fix
// Change detection: only update if strip data has changed since last update static unsigned long lastStripShow = 0; unsigned long currentShow = strip.getLastShow(); - if (currentShow == lastStripShow && brightness == strip.getBrightness() && brightness != 0) { + if (currentShow == lastStripShow && brightness != 0) { return; }Note: The
brightness != 0check in the condition is also somewhat redundant since line 36 already returns whenbrightness == 0 && lastSentBrightness == 0, but keeping it doesn't hurt and makes the intent clearer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wled00/cfg.cpp(2 hunks)wled00/data/settings_dmx.htm(1 hunks)wled00/dmx_output.cpp(3 hunks)wled00/set.cpp(1 hunks)wled00/src/dependencies/dmx/SparkFunDMX.cpp(1 hunks)wled00/xml.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_dmx.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_dmx.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_dmx.htm
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/set.cppwled00/xml.cppwled00/dmx_output.cppwled00/cfg.cppwled00/src/dependencies/dmx/SparkFunDMX.cpp
🧠 Learnings (15)
📓 Common learnings
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: 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.
📚 Learning: 2025-09-01T10:26:17.959Z
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.
Applied to files:
wled00/data/settings_dmx.htmwled00/set.cppwled00/xml.cppwled00/dmx_output.cppwled00/cfg.cppwled00/src/dependencies/dmx/SparkFunDMX.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
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.
Applied to files:
wled00/data/settings_dmx.htmwled00/set.cppwled00/xml.cppwled00/dmx_output.cppwled00/cfg.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/settings_dmx.htm
📚 Learning: 2025-10-10T18:34:06.550Z
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.
Applied to files:
wled00/set.cppwled00/xml.cppwled00/dmx_output.cppwled00/cfg.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
wled00/set.cppwled00/xml.cppwled00/dmx_output.cppwled00/cfg.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/set.cppwled00/dmx_output.cppwled00/cfg.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/set.cppwled00/dmx_output.cppwled00/cfg.cpp
📚 Learning: 2025-12-01T07:01:16.949Z
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.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/set.cppwled00/dmx_output.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
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.
Applied to files:
wled00/set.cppwled00/dmx_output.cppwled00/cfg.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/set.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/dmx_output.cpp
🧬 Code graph analysis (1)
wled00/xml.cpp (1)
wled00/util.cpp (4)
printSetFormValue(114-116)printSetFormValue(114-114)printSetFormValue(121-123)printSetFormValue(121-121)
🔇 Additional comments (11)
wled00/xml.cpp (1)
639-639: LGTM!The NF field is correctly emitted alongside other DMX settings fields.
wled00/set.cpp (1)
643-646: LGTM!The NF parameter validation and assignment are correctly implemented with appropriate bounds checking (1-512, matching the DMX universe limit).
wled00/cfg.cpp (2)
737-737: LGTM!The deserialization of the
num-fixturesfield is correctly implemented using the CJSON macro pattern, consistent with other DMX configuration fields.
1239-1239: LGTM!The serialization of the
num-fixturesfield is correctly implemented and follows the same pattern as other DMX configuration fields.wled00/dmx_output.cpp (7)
15-15: LGTM!The DMX rate limit of 44 Hz correctly matches the DMX512 theoretical maximum update rate.
22-23: LGTM!Good defensive check to ensure segments exist before accessing strip data.
25-28: LGTM!Rate limiting implementation correctly limits DMX updates to the configured maximum rate.
30-36: LGTM!The brightness tracking correctly allows one final DMX update when brightness transitions to 0, then skips subsequent updates when the strip remains off.
56-62: LGTM!The optimization to process only the configured number of fixtures (DMXNumFixtures) instead of all LEDs is correctly implemented and will provide the performance improvement described in the PR objectives.
63-63: LGTM!The loop bounds are correctly updated to respect the configured fixture limit.
100-100: LGTM!Correctly updates the brightness tracking variable after sending DMX data.
|
looks good to me, except the rabbit's remark: the break signal should be low. |
netmindz
left a comment
netmindz
left a comment
There was a problem hiding this comment.
DMX is meant to be a constant stream of data, not only on state change. Fixtures will consider it a failure state of they do not receive updates
|
If we are doing anything with DMX output we should really be ditching the sparkfun implementation and using the esp_dmx as is used for input as it appears to be more complaint |
Good point, bit of an oversight on my part. I've resolved it.
I've been testing a bit with the esp_dmx, and seem to get much better performance out of it. It seems to not influence the frame rate at all, which was the case with the sparkfun implementation. I will push my suggested changes soon However the library documentation explains that input and output concurrently is not possible. I don't see any code that would prevent anyone from doing so. Wouldn't it be best to prevent compilation with the DMX output + input flag defined. As it most certainly would cause issues, even before the switch over to esp_dmx for output? |
…erformance of the DMX output.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wled00/src/dependencies/dmx/EspDmxOutput.cpp (3)
17-18: Document the rationale for defaultMax = 32.The magic number 32 for
defaultMaxwould benefit from a comment explaining why this specific default was chosen.🔎 Suggested comment addition
#define dmxMaxChannel 512 -#define defaultMax 32 +#define defaultMax 32 // Default channel count when initWrite receives invalid input
30-32: Verify RS485 transceiver requirements for rtsPin.The
rtsPinis set toDMX_PIN_NO_CHANGE, but most RS485 transceivers require direction control (DE/RE pins). Users with typical RS485 hardware will need to set this to a valid GPIO.Consider adding a more prominent note in comments or documentation about this requirement.
🔎 Suggested comment enhancement
static const int txPin = 2; // transmit DMX data over this pin (default is GPIO2) static const int rxPin = DMX_PIN_NO_CHANGE; // RX unused for DMX output -static const int rtsPin = DMX_PIN_NO_CHANGE; // RS485 DE/RE pin (UART RTS). Set to a GPIO to control transceiver direction. +static const int rtsPin = DMX_PIN_NO_CHANGE; // RS485 DE/RE pin (UART RTS). IMPORTANT: Set this to a valid GPIO for RS485 transceivers that require direction control (most do).
80-88: Clarify misleading comment about break signal.The comment at line 83 says "Always send the break signal first", but
dmx_write_slot(dmxPort, 0, DMX_SC)writes the start code to slot 0, not the break signal. The actual DMX break signal (timing mark) is generated internally bydmx_send()at line 87.🔎 Suggested comment correction
- // Always send the break signal first - dmx_write_slot(dmxPort, 0, DMX_SC); + // Ensure NULL start code is set before transmission + dmx_write_slot(dmxPort, 0, DMX_SC); - // Send the frame currently staged in the driver buffer. + // Send the frame (dmx_send generates break signal and transmits data) dmx_send(dmxPort, txSize);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/dmx_output.cpp(2 hunks)wled00/src/dependencies/dmx/EspDmxOutput.cpp(1 hunks)wled00/src/dependencies/dmx/EspDmxOutput.h(1 hunks)wled00/src/dependencies/dmx/SparkFunDMX.cpp(0 hunks)wled00/src/dependencies/dmx/SparkFunDMX.h(0 hunks)
💤 Files with no reviewable changes (2)
- wled00/src/dependencies/dmx/SparkFunDMX.h
- wled00/src/dependencies/dmx/SparkFunDMX.cpp
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/src/dependencies/dmx/EspDmxOutput.h
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/src/dependencies/dmx/EspDmxOutput.cppwled00/dmx_output.cpp
🧠 Learnings (13)
📓 Common learnings
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: 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: 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.
📚 Learning: 2025-09-01T10:26:17.959Z
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.
Applied to files:
wled00/src/dependencies/dmx/EspDmxOutput.hwled00/src/dependencies/dmx/EspDmxOutput.cppwled00/dmx_output.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
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.
Applied to files:
wled00/src/dependencies/dmx/EspDmxOutput.cppwled00/dmx_output.cpp
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/src/dependencies/dmx/EspDmxOutput.cppwled00/dmx_output.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
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.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
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.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-10-20T09:38:51.997Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-11-22T12:12:53.486Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5105
File: wled00/FX.h:968-968
Timestamp: 2025-11-22T12:12:53.486Z
Learning: In WLED's WS2812FX class (wled00/FX.h), the _pixels array stores LED colors indexed by unmapped (logical) LED positions, not physical positions. The getPixelColor(n) function intentionally checks if the logical LED at index n maps to a valid physical position via getMappedPixelIndex(n), then accesses _pixels[n] with the unmapped index. If the mapping is invalid (e.g., -1 for gaps in LED maps, or out of bounds), it returns 0 (black). This allows peek preview to correctly show gaps in LED mappings as black pixels. The mapping from logical to physical positions is only applied when writing to the hardware bus, not when reading from the pixel buffer.
<!-- [add_learning]
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/dmx_output.cpp
📚 Learning: 2025-09-16T18:08:42.848Z
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.
Applied to files:
wled00/dmx_output.cpp
🧬 Code graph analysis (1)
wled00/src/dependencies/dmx/EspDmxOutput.h (1)
wled00/src/dependencies/dmx/EspDmxOutput.cpp (6)
initWrite(42-62)initWrite(42-42)write(64-78)write(64-64)update(80-88)update(80-80)
🔇 Additional comments (8)
wled00/src/dependencies/dmx/EspDmxOutput.h (1)
1-16: LGTM! Clean minimal interface.The header provides a clean, minimal interface for ESP32 DMX output. The three methods (initWrite, write, update) align with WLED's DMX API requirements and match the implementation in EspDmxOutput.cpp.
wled00/dmx_output.cpp (6)
6-10: LGTM! Documentation updated for new library.The comments correctly reference the new esp_dmx library and point to the appropriate source file for pin configuration on ESP32/S3.
15-16: LGTM! Rate limiting correctly implements 44 Hz DMX refresh.The rate limiting logic correctly implements a maximum 44 Hz update rate using ceiling division to ensure the limit is never exceeded. The timing is appropriate for DMX512 specification.
Also applies to: 25-28, 35-35
22-23: LGTM! Appropriate safety check.Checking for segment count prevents potential issues when accessing strip data with no configured segments.
46-47: LGTM! Appropriate guard against zero-length strip.This check prevents unnecessary processing when the strip has no LEDs configured.
48-53: Define a constant for DMX channel/fixture limit instead of hardcoding 512.The optimization correctly limits processing to configured fixtures with proper bounds checking. DMXNumFixtures is validated in
wled00/set.cpp(range 1-512). However, the hardcoded value512appears throughout the codebase (inset.cpp,dmx_output.cpp, anddmx_input.cpp). Define a constant likeDMX_MAX_FIXTURESor similar in a header file and use it consistently to improve maintainability and reduce risk of inconsistencies.⛔ Skipped due to learnings
Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:11.994Z Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:30.955Z Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.Learnt from: mval-sg Repo: wled/WLED PR: 4876 File: wled00/xml.cpp:0-0 Timestamp: 2025-08-28T08:09:20.630Z Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.Learnt from: mval-sg Repo: wled/WLED PR: 4876 File: wled00/wled_eeprom.cpp:0-0 Timestamp: 2025-09-01T10:26:17.959Z Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.Learnt from: DedeHai Repo: wled/WLED PR: 4997 File: wled00/e131.cpp:33-44 Timestamp: 2025-10-10T18:34:06.550Z Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:11.994Z Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.Learnt from: softhack007 Repo: wled/WLED PR: 0 File: :0-0 Timestamp: 2025-11-14T13:37:30.955Z Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.Learnt from: DedeHai Repo: wled/WLED PR: 4926 File: wled00/FX.cpp:4727-4730 Timestamp: 2025-11-16T19:40:46.260Z Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.Learnt from: BobLoeffler68 Repo: wled/WLED PR: 5109 File: wled00/FX.cpp:3174-3343 Timestamp: 2025-11-27T06:33:11.436Z Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.Learnt from: DedeHai Repo: wled/WLED PR: 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: 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.
32-33: The brightness==0 check is a reasonable optimization with proper documentation.When the LED strip is off (brightness=0), skipping DMX writes avoids unnecessary bus traffic. The rate limiting (44Hz max) and shutter channel mechanism (if configured) already handle brightness control appropriately. This behavior is intentional and aligns with the recent performance improvements made in the DMX output refactor.
Likely an incorrect or invalid review comment.
wled00/src/dependencies/dmx/EspDmxOutput.cpp (1)
64-78: LGTM! Write method correctly handles bounds and size tracking.The write method properly validates inputs, tracks transmit size growth, and maintains both local and driver buffers. The clamping logic at lines 73-74 ensures size stays within all applicable bounds.
Note: Channel 0 writes are allowed but will be overwritten by
update()to enforce the NULL start code, as documented in the comment.
| void EspDmxOutput::initWrite(int chanQuant) { | ||
| if (chanQuant > dmxMaxChannel || chanQuant <= 0) chanQuant = defaultMax; | ||
| maxSize = static_cast<size_t>(chanQuant) + 1; // +1 for start code | ||
| txSize = 1; // start with just start code | ||
|
|
||
| // Configure the driver if needed. | ||
| if (!dmx_driver_is_installed(dmxPort)) { | ||
| dmx_config_t config = DMX_CONFIG_DEFAULT; | ||
| dmxInstalled = dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT); | ||
| } else { | ||
| dmxInstalled = true; | ||
| } | ||
|
|
||
| if (dmxInstalled) { | ||
| dmx_set_pin(dmxPort, txPin, rxPin, rtsPin); | ||
|
|
||
| // Ensure NULL start code (slot 0). | ||
| dmxData[0] = DMX_SC; | ||
| dmx_write_slot(dmxPort, 0, DMX_SC); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix boolean logic for dmx_driver_install return value handling.
Line 50 incorrectly assigns dmx_driver_install return value directly to dmxInstalled. Since dmx_driver_install returns esp_err_t (where ESP_OK = 0 indicates success), successful installation would set dmxInstalled = false, preventing all subsequent DMX operations.
- dmxInstalled = dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT);
+ dmxInstalled = (dmx_driver_install(dmxPort, &config, DMX_INTR_FLAGS_DEFAULT) == ESP_OK);🤖 Prompt for AI Agents
In wled00/src/dependencies/dmx/EspDmxOutput.cpp around lines 42 to 62, the code
assigns the esp_err_t return from dmx_driver_install directly to the bool
dmxInstalled which treats ESP_OK (0) as false; instead capture the return into
an esp_err_t variable and set dmxInstalled to (err == ESP_OK). Optionally log or
handle non-ESP_OK errors before proceeding so dmxInstalled correctly reflects
success and subsequent DMX setup runs only when installation succeeded.
3 participants
As already reported in my #5133 , I found out that the current DMX implementation was negatively impacting performance. It previously tried to push all of the led strips registered onto the DMX bus.
With this PR I've added a new option to configure how many DMX fixtures worth of output should be sent.
I've also improved the performance of the DMX update code by about 50% from my testing. Previously it created 2 serials in each update cycle. I've changed the DMX break signal to now be constructed manually, instead of setting up a serial connection for it. It went from 19 FPS avg on esp32, to about 28.I've also added rate limiting to the DMX code, to make sure it is updated at 44hz (DMX theoretical maximum speed) at most and only if a change is detected.
I've tested these changes on my own hardware, which has DMX output added to it.
In a perfect world, the break signal should be sent while keeping the serial connection alive. This way the serial connection wouldn't have to be torn down and started again, causing an even greater performance boost. I was not able to get this working reliably yet.Update:
I've changed the implementation from the old sparkfun one, to use esp_dmx instead. It seems to no longer have any negative impact at all on the frame rate, where previously I've seen about 20 frames per second, now >100 is possible. Also removed the legacy code from sparkfun implementation where it was reading DMX, this has since been moved.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.