Fixed bug of ESP32 crashing when using DMX out #5316
Conversation
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
Fixed bug of ESP32 crashing when using DMX out: -When exceeding the DMX channel limit, the SparkFunDMX.cpp tried to write to static chanSize because the variables where swapped. -When exceeding the channel upper & lower limit now, the value is ignored, previous implementation "idea" was to set as channel 512 value, which is not correct. -There are still some non-optimal parts in the lib, but it at least works for now.
Contributor
WalkthroughModifies DMX channel handling in SparkFunDMX.cpp by replacing defaultMax fallback with explicit dmxMaxChannel cap in initialization functions, strengthening read() bounds checking to return 0 for out-of-range channels, and simplifying write() function logic with direct dmxData array access. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp`:
- Around line 129-132: The write method in SparkFunDMX::write has an off-by-one
bounds check allowing Channel == chanSize which writes past the end of dmxData;
update the condition to reject Channel values >= chanSize (i.e., ensure valid
Channel is 1..chanSize-1) so accesses to dmxData[Channel] cannot overflow;
modify the check that currently uses (Channel > chanSize) to use (Channel >=
chanSize) (referencing SparkFunDMX::write, Channel, chanSize, chanQuant and
dmxData).
Comment on lines
129
to
132
| void SparkFunDMX::write(int Channel, uint8_t value) { | ||
| if (Channel < 0) Channel = 0; | ||
| if (Channel > chanSize) chanSize = Channel; | ||
| dmxData[0] = 0; | ||
| dmxData[Channel] = value; //add one to account for start byte | ||
| if ((Channel < 1) || (Channel > chanSize)) return; | ||
| dmxData[Channel] = value; //start byte at dmxData[0] | ||
| } |
Contributor
There was a problem hiding this comment.
Off-by-one error in bounds check allows potential buffer overflow.
In write mode, chanSize = chanQuant + 1. When chanQuant = 512 (max), chanSize = 513. The check Channel > chanSize evaluates Channel > 513, which allows Channel = 513 to pass, resulting in dmxData[513] — an out-of-bounds write (array indices are 0–512).
Proposed fix
void SparkFunDMX::write(int Channel, uint8_t value) {
- if ((Channel < 1) || (Channel > chanSize)) return;
+ if ((Channel < 1) || (Channel >= chanSize)) return;
dmxData[Channel] = value; //start byte at dmxData[0]
}Based on learnings, in WLED the priority is to prevent out-of-bounds memory access.
🤖 Prompt for AI Agents
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp` around lines 129 - 132, The
write method in SparkFunDMX::write has an off-by-one bounds check allowing
Channel == chanSize which writes past the end of dmxData; update the condition
to reject Channel values >= chanSize (i.e., ensure valid Channel is
1..chanSize-1) so accesses to dmxData[Channel] cannot overflow; modify the check
that currently uses (Channel > chanSize) to use (Channel >= chanSize)
(referencing SparkFunDMX::write, Channel, chanSize, chanQuant and dmxData).
Member
There was a problem hiding this comment.
@coderabbitai is this off-by-one error already present in the previous code, or was it introduced by changes in this PR?
Contributor
There was a problem hiding this comment.
@softhack007, the off-by-one error is newly introduced by this PR.
Previous code behavior:
The old code had a different bug:
if (Channel > chanSize) chanSize = Channel; // BUG: swaps/corrupts chanSize variable
dmxData[Channel] = value;When Channel > chanSize, it would incorrectly modify chanSize itself (the variable swap bug mentioned in the PR description), then proceed to write to dmxData[Channel]. This could corrupt the chanSize variable and potentially write out of bounds.
New code behavior:
The PR fixes the variable swap bug by adding a bounds check and early return. However, the check has an off-by-one error:
if ((Channel < 1) || (Channel > chanSize)) return; // Allows Channel == chanSize
dmxData[Channel] = value;Since chanSize = chanQuant + 1 (line 117) and max chanQuant = 512, we get chanSize = 513. The condition Channel > 513 allows Channel = 513 to pass, accessing dmxData[513], which is out of bounds (valid indices: 0–512).
The fix I suggested (Channel >= chanSize) would properly reject Channel = 513 and ensure only valid channels 1–512 can write to array indices 1–512, while index 0 remains the start byte.
🧠 Learnings used
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR `#4876`, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
This was referenced Jan 25, 2026
netmindz
added
bug
connectivity
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed bug of ESP32 crashing when using DMX out:
-When exceeding the DMX channel limit, the SparkFunDMX.cpp tried to write to static chanSize (line 132) because the variables where swapped.
-When exceeding the channel upper & lower limit now, the value is ignored, previous implementation "idea" was to set as channel 512 value, which is not correct.
-There are still some non-optimal parts in the lib, but it at least works for now.
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.