Fix for ESP32C3 Glitching on Long Strips#5683
Conversation
…a function of the number of LEDs to avoid interrupting RMT transmissions, which corrupts end of frame during animations every 5 seconds
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIn ChangesESP32C3 heap-check deferral
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/wled.cpp`:
- Line 183: The timeout calculation in the waitMs assignment at line 183
performs integer division (/ 1000) before applying the 1.3 margin multiplier,
causing the margin to be applied to an already-truncated value. This results in
shorter-than-intended waits for longer strips. Rearrange the arithmetic
operations so that the 1.3 margin is applied before the division by 1000, or
ensure floating-point arithmetic is preserved throughout the calculation until
the final cast to unsigned, so the margin factor is applied to the full value
rather than the truncated intermediate result.
🪄 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: a4d3a621-6da2-496e-8798-07f63c8fd8ff
📒 Files selected for processing (1)
wled00/wled.cpp
softhack007
added
the
AI
| while (strip.isUpdating() && (millis() - t0 < 15)) delay(1); // be nice, but not too nice. Waits up to 15ms | ||
| // Most strips run at 800kHz, each LED needs 24 bits = 30µs per LED | ||
| // Add 30% margin and convert to ms, minimum 15ms | ||
| uint32_t waitMs = max(15u, (unsigned)(strip.getLengthTotal() * 30 / 1000 * 1.3)); |
There was a problem hiding this comment.
in general has a point BUT:
- don't use floats
- only calculate if we actually need to wait
- getLengthTotal() is not the length of the longest strip but total configred LEDs, including gaps
in general, waiting on a single core MCU is bad. the proper way to do it is to defer this to a loop when LEDs are not updating.
edit:
even the original wait is a hack and a workaround for unfixable deeper bugs.
|
@bigfella5000 I think you're only scratching on the surface - the real problem on -C3 is the shared "RMT refill" interrupt. This interrupt can be stalled by many FreeRTOS or hardware driver activities, like file access or calculating unused memory. You can have a look at this PR for a more complete picture The wled dev team is aware of the problem, and the only real solution for -C3 is to rewrite the leds driver, or implement a high-priority RMT driver (similar to #4890) for RISC-V chips. Both options are ongoing experiments, but the code is not stable yet - @DedeHai and @willmmiles might be able to tell more. Your idea to make the wait time depend on number of leds is interesting, and could be combined with #5435 for a more robust solution. However I see a fundamental flaw: Edit: with 4096 LEDs, your patch would calculate a wait time of ~160ms, which is totally unacceptable as a delay that happens each 5 seconds. |
|
Thanks for pointing to #5435. The 0_15_x build solved the issue on all effects I tested including the Stream effect. One issue I'm having with it, which is likely well known, is that when the esp32c3 boots after uploading the 0_15_x build, I'm still seeing some glitching for the first few seconds. I'm assuming this is because there's plenty of other tasks interfering on boot. One other potential solution for avoiding the interference between the heap check and led updates would be the following: #ifdef CONFIG_IDF_TARGET_ESP32C3
// calling getContiguousFreeHeap() during led update causes glitches on C3
// this can (probably) be removed once RMT driver for C3 is fixed
// If strip updating, defer heap check until future
if (strip.isUpdating()) {
heapTime = millis() - 4999;
} else
#endif{
uint32_t heap = getContiguousFreeHeap(); // ESP32 family needs ~10k of contiguous free heap for UI to work properly
#endif
if (heap < MIN_HEAP_SIZE - 1024) heapDanger+=5; // allow 1k of "wiggle room" for things that do not respect min heap limits
else heapDanger = 0;
switch (heapDanger) {
case 15: // 15 consecutive seconds
DEBUG_PRINTLN(F("Heap low, purging segments"));
strip.purgeSegments();
strip.setTransition(0); // disable transitions
for (unsigned i = 0; i < strip.getSegmentsNum(); i++) {
strip.getSegments()[i].setMode(FX_MODE_STATIC); // set static mode to free effect memory
}
errorFlag = ERR_NORAM; // alert UI TODO: make this a distinct error: segment reset
break;
case 30: // 30 consecutive seconds
DEBUG_PRINTLN(F("Heap low, reset segments"));
strip.resetSegments(); // remove all but one segments from memory
errorFlag = ERR_NORAM; // alert UI TODO: make this a distinct error: segment reset
break;
case 45: // 45 consecutive seconds
DEBUG_PRINTF_P(PSTR("Heap panic! Reset strip, reset connection\n"));
strip.~WS2812FX(); // deallocate strip and all its memory
new(&strip) WS2812FX(); // re-create strip object, respecting current memory limits
if (!Update.isRunning()) forceReconnect = true; // in case wifi is broken, make sure UI comes back, set disableForceReconnect = true to avert
errorFlag = ERR_NORAM; // alert UI TODO: make this a distinct error: strip reset
break;
default:
break;
}
heapTime = (uint16_t)millis();
}However, the 0_15_x handles the interference more generally and in more locations by using waitForLEDs(), so I'm assuming that's a better solution. |
bigfella5000
force-pushed
the
fix/esp32c3-heap-glitch
branch
from
daeb462 to
3835df0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
bigfella5000
force-pushed
the
fix/esp32c3-heap-glitch
branch
from
3835df0 to
c282d21
Compare
| // calling getContiguousFreeHeap() during led update causes glitches on C3 | ||
| // this can (probably) be removed once RMT driver for C3 is fixed | ||
| unsigned t0 = millis(); | ||
| while (strip.isUpdating() && (millis() - t0 < 15)) delay(1); // be nice, but not too nice. Waits up to 15ms |
There was a problem hiding this comment.
Second thought - as 15ms is just the upper limit for the waiting time - it would be sufficient here to change 15 into 50, and drop all the other changes.
There was a problem hiding this comment.
that is what I thought too, however, it may be better to give the main loop several tries to get a slot where the strip is idle. Otherwise, this can result in unnecessary FPS hickups every 5s. I don't think there is a universal solution though, if target FPS is higher than actual FPS the main loop may not/never be running at the right moment.
There was a problem hiding this comment.
I agree. A limited number of retries - maybe an extra 2seconds when a new measurement is due - could reduce hickups. If the main loops runs strip.show() in each iteration (high FPS), this will not help much. But users could still reduce target FPS to prevent glitches.
| while (strip.isUpdating() && (millis() - t0 < 15)) delay(1); // be nice, but not too nice. Waits up to 15ms | ||
| // If strip updating, defer heap check until future | ||
| if (strip.isUpdating()) { | ||
| heapTime = millis() - 4999; |
There was a problem hiding this comment.
Not a good idea - this basicially postpones the heap check to infinite future, especially when target FPS is high.
@bigfella5000 For any future PR you may do - please don't See https://github.com/wled/WLED?tab=contributing-ov-file#updating-your-code |
3 participants
Hardware
Fix
Made wait time before calling getContiguousFreeHeap() on ESP32C3 to be a function of the number of LEDs to avoid interrupting RMT transmissions, which corrupts end of frame during animations every 5 seconds. At 800kHz with 24 bits per LED, a full frame takes N × 30µs. For strips longer than ~500 LEDs the fixed 15ms wait is insufficient, causing getContiguousFreeHeap() to interrupt an in-progress RMT transmission and corrupt the last portion of the frame (visible as a white flash every 5 seconds). This PR replaces the hardcoded value with a calculation based on actual strip length with a safety margin, floored at the original 15ms so short strips are unaffected.
Ongoing Issue
When running the effect given in "Wavesins API Command," my solution stopped the glitching. And in all other effects I tested, the issue is fixed. However, when running the effect given in "Stream API Command," the issue is still persisting. Additionally, the glitching occurs not just in the last quarter of the frame as it did with the other effects, but instead on the whole frame. And instead of glitching every 5 seconds, it's glitching every 6 seconds. Even when I raised the margin in the waitMS calculation, the glitch still occurred the exact same way on the Stream effect.
Wavesins API Command
{"on":true,"bri":255,"transition":0,"bs":0,"mainseg":0,"seg":[{"id":0,"start":0,"stop":832,"grp":1,"spc":0,"of":0,"on":true,"frz":false,"bri":255,"cct":127,"set":0,"lc":1,"n":"","col":[[115,0,255],[255,4,0],[0,0,0]],"fx":184,"sx":15,"ix":245,"pal":18,"c1":124,"c2":34,"c3":1,"sel":true,"rev":false,"mi":false,"o1":false,"o2":false,"o3":false,"si":0,"m12":0,"bm":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0}]}Stream API Command
{"on":true,"bri":255,"transition":0,"bs":0,"mainseg":0,"seg":[{"id":0,"start":0,"stop":832,"grp":1,"spc":0,"of":0,"on":true,"frz":false,"bri":255,"cct":127,"set":0,"lc":1,"n":"","col":[[115,0,255],[255,4,0],[0,0,0]],"fx":39,"sx":255,"ix":39,"pal":48,"c1":128,"c2":128,"c3":16,"sel":true,"rev":false,"mi":false,"o1":false,"o2":false,"o3":false,"si":0,"m12":0,"bm":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0},{"stop":0}]}Summary by CodeRabbit