(0.15.x) wait for LEDs output to finish before OS calls that potentially suspend interrupts

[softhack007]

Add a short wait time before calling file.close() and ESP.getFreeHeap(), to prevent LED glitches caused by interrupt stalls.

potential fix for #5427

Summary by CodeRabbit

• New Features

  • Added LED synchronization capabilities to improve timing during file operations and memory checks.

• Bug Fixes

  • Fixed suspend state handling to preserve trigger events during effect servicing.
  • Enhanced LED update coordination during file uploads and JSON operations on affected platforms.

• Refactor

  • Improved internal LED output synchronization across multiple operations.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e336b1bf-e71c-43e9-8bbf-ea14d57118ea

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 8266_wait_b4_close

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[willmmiles]

This can never happen on ESP8266. It's impossible to be handling file IO during bus operations because the system is using cooperative multitasking -- it cannot be pre-empted. The strip operation must complete before any web handling/file IO can proceed.

[softhack007]

@willmmiles i thought that LEDs output is interrupt driven, also on 8266?

Edit: I agree that "if f.close() coincides with strip.service()." is misleading (AI wording....). Can the 8266 LEDs driver continue to send data in the background (ISR) ?

[willmmiles]

I'm just triple-checking this now..

For digital LEDs:

• Bigbang buffer is blocking (obviously)
• DMA methods (pin 3) uses full up-front conversion instead; it's memory intensive
• The UART method (pins 1 or 2) do use an ISR, but they're configured to operate entirely out of SRAM, so it will work with flash disabled

For analog outputs, the system implements soft PWM using an NMI; it's also entirely SRAM and safe to use during flash IO.

I have to take a break for a bit but I've got my test board up and running, I'll see what I can find later today.

[willmmiles]

There is a possible issue if someone's trying to use soft-PWM analog and bit-banging simultaneously, but that's firmly in the category of "has never and probably will never work".

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

wled00/FX_fcn.cpp (1)

1423-1433: ⚠️ Potential issue | 🟠 Major

A late suspend() can still drop the pending redraw.

Line 1423 only handles suspends that are already visible before the reset. If another task suspends during the yield() / palette work window, Line 1432 skips show() after _triggered has already been cleared, and Line 1431 still advances _lastServiceShow for a frame that never went out.

💡 Minimal fix

   if (doShow) {
     yield();
     Segment::handleRandomPalette(); // slowly transition random palette; move it into for loop when each segment has individual random palette
-    _lastServiceShow = nowUp; // update timestamp, for precise FPS control
-    if (!_suspend) show();
+    if (!_suspend) {
+      _lastServiceShow = nowUp; // update timestamp only for frames that were actually sent
+      show();
+    } else {
+      _triggered = true; // re-run the skipped frame immediately after resume()
+    }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@wled00/FX_fcn.cpp` around lines 1423 - 1433, The pending redraw can be lost
if suspend() is called during yield()/Segment::handleRandomPalette(): don't
clear _triggered or advance _lastServiceShow before the final suspend check and
show(); instead re-evaluate _suspend after yield() and
Segment::handleRandomPalette(), only call show() and then set _lastServiceShow
and clear _triggered when show() actually ran (or keep _triggered set if
suspended), ensuring the sequence in the block that handles doShow uses the
post-yield suspend state to decide whether to call show(), update
_lastServiceShow, and reset _triggered.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@wled00/FX_fcn.cpp`:
- Around line 1423-1433: The pending redraw can be lost if suspend() is called
during yield()/Segment::handleRandomPalette(): don't clear _triggered or advance
_lastServiceShow before the final suspend check and show(); instead re-evaluate
_suspend after yield() and Segment::handleRandomPalette(), only call show() and
then set _lastServiceShow and clear _triggered when show() actually ran (or keep
_triggered set if suspended), ensuring the sequence in the block that handles
doShow uses the post-yield suspend state to decide whether to call show(),
update _lastServiceShow, and reset _triggered.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1a821192-cbc4-4f72-a0eb-840b39478abf

📥 Commits

Reviewing files that changed from the base of the PR and between d9bef17 and 01520cb.

📒 Files selected for processing (1)

• wled00/FX_fcn.cpp