-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Fixed bug of ESP32 crashing when using DMX out #5316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixed bug of ESP32 crashing when using DMX out: -When exceeding the DMX channel limit, the SparkFunDMX.cpp tried to write to static chanSize because the variables where swapped. -When exceeding the channel upper & lower limit now, the value is ignored, previous implementation "idea" was to set as channel 512 value, which is not correct. -There are still some non-optimal parts in the lib, but it at least works for now.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| 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] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.