hotfix for DMX-serial output (prevents random crashes caused by array bounds violations) #326
Conversation
imports a few DMX serial output improvement from upstream
both ESPDMX and SparkFunDMX have their own global buffers. Conditional compile prevents that these buffers are consuming RAM when not needed.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughBroadened DMX driver selection to additional ESP32 variants, added compile-time diagnostic pragmas, expanded DMX buffer from 512 to 513 to accommodate a start byte, and added bounds checks/clamping for DMX reads, writes, send, and receive paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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. 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses critical array bounds violations in DMX serial output that could cause random crashes. It includes platform alignment updates and conditional compilation improvements to prevent unnecessary code inclusion.
Changes:
- Increased DMX data array size from 512 to 513 elements to account for start byte at index 0
- Added bounds checking in read/write operations to prevent array overflows
- Extended platform support to include ESP32-C3 and ESP32-S2 variants
- Added conditional compilation guards to prevent DMX driver compilation when not needed
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wled00/wled.h | Extended platform check to include ESP32-C3 and ESP32-S2 for ESPDMX driver selection |
| wled00/src/dependencies/dmx/SparkFunDMX.cpp | Fixed array bounds issues by increasing buffer size, adding bounds checks, and limiting write operations |
| wled00/src/dependencies/dmx/ESPDMX.cpp | Increased buffer size and added conditional compilation guard with platform checks |
| wled00/dmx_output.cpp | Updated pragma message text for clarity |
| wled00/dmx_input.cpp | Updated pragma message text for clarity |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
wled00/wled.h (1)
166-172: Align DMX driver selection for ESP32‑C3/S2.The include guard now routes ESP32C3/S2 to ESPDMX, but the global
dmxinstance still selectsSparkFunDMXfor all non‑ESP8266 targets. This will not compile (or will select the wrong driver) on C3/S2. Please mirror the same target guard used for the header selection.🛠️ Proposed fix
-#ifdef ESP8266 +#if defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S2) WLED_GLOBAL DMXESPSerial dmx; `#else` //ESP32 WLED_GLOBAL SparkFunDMX dmx; `#endif`Also applies to: 470-475
wled00/dmx_output.cpp (1)
13-106: Update init path for ESP32‑C3/S2 when using ESPDMX.The driver selection now routes C3/S2 to ESPDMX, but
initDMXOutput()still uses the SparkFun path for all non‑ESP8266 targets. That will callinitWrite()on ESPDMX and fail to compile. Mirror the same target guard used for the include/instance.🛠️ Proposed fix
void initDMXOutput() { - `#ifdef` ESP8266 + `#if` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32S2) dmx.init(512); // initialize with bus length `#else` dmx.initWrite(512); // initialize with bus length `#endif` }
🤖 Fix all issues with AI agents
In `@wled00/src/dependencies/dmx/ESPDMX.cpp`:
- Around line 34-35: The DMX buffer dmxDataStore is sized as dmxMaxChannel+1
(index 0 is start byte) but Serial1.write currently sends channelSize bytes
which omits the last channel; update all Serial1.write usages (and any similar
sends around the dmx handling functions) to send min(channelSize + 1,
dmxMaxChannel + 1) bytes so the start byte plus all channels are transmitted,
and ensure channelSize is clamped/validated against dmxMaxChannel before use
(references: dmxDataStore, dmxMaxChannel, channelSize, and Serial1.write).
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp`:
- Around line 39-40: The current send call uses min(dmxMaxChannel, chanSize)
which limits sent bytes to 512 and omits channel 512; update the write length to
use dmxMaxChannel + 1 as the upper bound (e.g. use min(dmxMaxChannel + 1,
chanSize)) so the 513-byte buffer (dmxData) including the start byte at index 0
is fully transmitted; locate uses in SparkFunDMX.cpp around dmxData and the
DMXSerial.write(...) call (also at the second occurrence noted) and change the
length calculation accordingly.
bounds check before changing global variables
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wled00/src/dependencies/dmx/SparkFunDMX.cpp`:
- Around line 133-138: The write logic in SparkFunDMX::write incorrectly sets
chanSize to the channel index, but update() expects chanSize to be the total
buffer length including the start byte; this can drop the last channel. After
clamping Channel against bounds (and dmxMaxChannel), set chanSize = Channel + 1
(rather than chanSize = Channel) so chanSize reflects length including the start
byte; keep existing clamping behavior and assignment to dmxData[Channel].
- Around line 125-128: SparkFunDMX::read has an off-by-one: it returns
dmxData[Channel - 1] whereas SparkFunDMX::write stores at dmxData[Channel] and
the DMX buffer uses index 0 as the start code and index N for channel N; change
SparkFunDMX::read to return dmxData[Channel] (remove the "- 1" offset) and
delete the outdated comment about subtracting one, keeping the existing
chanSize/dmxMaxChannel bounds checks unchanged so reads remain safe.
If Channel is less than 0, it's clamped to 0 on line 134. Then on line 138, dmxData[0] is written with the value, overwriting the start byte that was just set on line 137. DMX Channel numbers should be 1-based (1-512). The bounds check should clamp Channel to a minimum of 1, not 0, to prevent corrupting the start byte Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@coderabbitai please perform a comprehensive review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 32043
Overall AssessmentThis PR makes important progress toward preventing array bounds violations in DMX output. The SparkFunDMX.cpp changes are solid and comprehensive. However, I've identified a critical bug in ESPDMX.cpp that needs to be addressed. Critical Issue: ESPDMX.cpp🔴 Bug in
|
* clamp channel to dmxMaxChannel (write) * ensure that all data get sent, including start byte
|
@coderabbitai please perform an in-depth review of the data flow, to verify that all DMX data is always sent, no additional data gets sent and array bounds are always respected. Also consider that in case that the data array (channels) is full, it's acceptable to drop or ignore write requests that would require a bigger buffer array. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8475
✅ ESPDMX.cpp - Data Flow AnalysisInitialization & Buffer Setup
Write Operationswrite(Channel, value):
• Clamps: Channel ∈ [1, min(channelSize, dmxMaxChannel)]
• Writes to: dmxDataStore[Channel]
• Full buffer behavior: Channels > dmxMaxChannel clamped to 512Bounds Check: ✅ Safe - dual clamping to both Send Operationsupdate():
• Sends: min(dmxMaxChannel+1, channelSize+1) bytes
• Example: channelSize=32 → sends 33 bytes (indices 0-32)
• Example: channelSize=512 → sends 513 bytes (indices 0-512)Data Transmission: ✅ CORRECT - All DMX data sent, no extra data, no missing data ✅ SparkFunDMX.cpp - Data Flow Analysis (Write Path)Initialization & Buffer Setup
Write Operationswrite(Channel, value):
• Auto-expands: chanSize = min(dmxMaxChannel+1, Channel+1)
• Clamps: Channel ∈ [1, dmxMaxChannel]
• Writes to: dmxData[0]=0 (start), dmxData[Channel]=value
• Full buffer behavior: Channel > 512 clamped to 512Bounds Check: ✅ Safe - clamped to Send Operationsupdate() WRITE path:
• Sends: min(dmxMaxChannel+1, chanSize) bytes
• Example: chanSize=33 → sends 33 bytes (indices 0-32)
• Example: chanSize=513 → sends 513 bytes (indices 0-512)Data Transmission: ✅ CORRECT - All DMX data sent, no extra data Receive Operationsupdate() READ path:
• Guard: if (currentChannel <= dmxMaxChannel)
• Writes to: dmxData[currentChannel++]
• Stops when: currentChannel > max(chanSize, dmxMaxChannel)Bounds Check: ✅ Safe - writes only when 📊 Verification Summary
Edge Case TestingScenario 1: Maximum Capacity (512 channels)
Scenario 2: Write to Channel 513 (Invalid)
Scenario 3: Buffer Expansion (SparkFunDMX only)
Scenario 4: Channel 512 Transmission
🎯 ConclusionBoth drivers are now CORRECT. The PR successfully:
The critical fix in ESPDMX.cpp using 📝 Minor Note (Out of Scope)There's a pre-existing indexing inconsistency in SparkFunDMX.cpp 🧠 Learnings used |
|
@netmindz @troyhacks Could you quickly try the code with DMX hardware? I don't have DMX hardware, and we should be sure that I did not create any "regressions".A quick run with hardware connected should be sufficient, |
|
I'll release this into the wild now, hoping that users will report back in case of new problems. |
NB: there is a PR in upstream that addresses possible logic errors in DMX output (not covered by this MM-specific PR)
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.