fix(esp32-s3): verify and finish #2420 FastLED.show() timing regression

[zackees]

Context

#2421 mixed two separate regressions from #2420: ESP32-S3 FastLED.show() timing drift and binary-size growth. The binary-size work has its own history and follow-ups; this issue intentionally tracks only the timing regression.

Original timing report from #2420:

• Board: ESP32-S3
• LEDs: 35 WS2812B / NeoPixel LEDs on pin 4
• Pacing: FastLED.show() every 5 ms via millisDelay from powerbroker2/SafeString
• FastLED settings: FastLED.setMaxRefreshRate(0), setCpuFrequencyMhz(160)
• Expected sequence time: ~2500 ms
• Observed on master#ea5d8ec: commonly ~2600 ms, with random spikes above 3000 ms
• Later retest on master#787de450d: 2600-2800 ms remained, although the long-running drift toward 3000 ms was no longer present

Since then, #2815, #2817, #2827, and #2828 landed wait-loop improvements. There is not yet a focused hardware confirmation that the #2420 ESP32-S3 timing case is fixed on current master, and current source still has one suspicious gap: IChannelDriver::waitDone() exists, but no ESP32 driver overrides it yet.

Binary-size numbers, PlatformIO exception-frame behavior, and lean-build work are out of scope for this issue.

Reproduction Case

Use the original #2420 sketch and measure one full fade sequence. Prefer serial micros()/millis() instrumentation over stopwatch timing so the result is machine-readable.

#include <Arduino.h>
#include <millisDelay.h>
#include <FastLED.h>

uint8_t i_post_fade = 255;
#define LED_PIN 4
#define FAST_LED_UPDATE_MS 5
#define NUM_LEDS 35 // 35-LED NeoPixel ring
CRGB leds[NUM_LEDS];
uint8_t i_fast_led_delay = FAST_LED_UPDATE_MS;
millisDelay ms_fast_led;
millisDelay ms_delay_post;
uint32_t cycle_start_ms = 0;

void setup() {
  FastLED.addLeds<NEOPIXEL, LED_PIN>(leds, NUM_LEDS).setCorrection(TypicalLEDStrip);
  FastLED.setMaxRefreshRate(0);
  FastLED.show();
  setCpuFrequencyMhz(160);
  Serial.begin(115200);
  ms_delay_post.start(0);
  ms_fast_led.start(i_fast_led_delay);
  cycle_start_ms = millis();
}

void updateLEDs() {
  if (ms_fast_led.justFinished()) {
    FastLED.show();
    ms_fast_led.start(i_fast_led_delay);
  }
}

void loop() {
  if (i_post_fade > 0 && ms_delay_post.justFinished()) {
    const uint8_t i_ring_start = 0;
    const uint8_t i_ring_end = 34;
    const uint8_t i_ring_divisor = 7;
    uint8_t i_ring_counter = ((255 - i_post_fade) / i_ring_divisor) + i_ring_start;

    if (i_ring_counter <= i_ring_end) {
      leds[i_ring_counter] = CRGB::Red;
    }

    if (i_ring_counter - 1 >= i_ring_start && i_ring_counter - 1 <= i_ring_end) {
      leds[i_ring_counter - 1] = CRGB::Black;
    }

    i_post_fade--;

    if (i_post_fade == 0) {
      for (uint8_t i = i_ring_start; i <= i_ring_end; i++) {
        leds[i] = CRGB::Black;
      }

      updateLEDs();
      const uint32_t elapsed = millis() - cycle_start_ms;
      Serial.printf("cycle_ms=%lu\n", (unsigned long)elapsed);
      delay(1000);
      i_post_fade = 255;
      cycle_start_ms = millis();
      ms_delay_post.start(225);
      return;
    } else {
      ms_delay_post.start(5);
    }
  }

  updateLEDs();
}

Original PlatformIO setup from #2420:

[env:esp32s3]
platform = https://github.com/pioarduino/platform-espressif32/releases/download/55.03.38-1/platform-espressif32.zip
board = esp32s3custom
framework = arduino
lib_deps =
  https://github.com/FastLED/FastLED.git#<commit-under-test>
  powerbroker2/SafeString@^4.1.42
build_flags =
  -D "CORE_DEBUG_LEVEL=3"
  -D "LOG_LOCAL_LEVEL=ESP_LOG_INFO"
  -D "FASTLED_WS2812_T1=375"
  -D "FASTLED_WS2812_T2=375"
  -D "FASTLED_WS2812_T3=500"

For repository automation, an equivalent bash autoresearch esp32s3 ... / examples/AutoResearch timing mode would be better than manual PlatformIO testing.

Likely Code Areas

Current master has the #2815 wait-loop scaffolding, but the driver-specific completion wait still appears incomplete:

• src/fl/channels/manager.cpp.hpp:471-520 has the single-driver fast path. It delegates to onlyBusy->waitDone(timeoutMs) when exactly one driver is busy.
• src/fl/channels/driver.h:151-152 defaults waitDone() back to waitForReady(timeoutMs).
• A current tree search only finds the default and the manager call site for waitDone(, so ESP32-S3 RMT5 still falls back to the generic wait path instead of an ISR-exact wait.
• src/fl/channels/driver.cpp.hpp:12-60 and src/fl/channels/manager.cpp.hpp:375-435 are the generic tiered wait loops: instant check, bounded spin, then NetworkDetector-gated yield.
• If fl::NetworkDetector::isAnyNetworkActive() returns true unexpectedly, the wait still calls task::run(250, task::ExecFlags::SYSTEM), which reaches pumpCoroutines(1000) in src/fl/task/executor.cpp.hpp:135-140 and vTaskDelay(ticks) in src/platforms/esp/32/coroutine_esp32.impl.hpp:62-87.
• RMT5 already has an underlying completion primitive: IRMT5Peripheral::waitAllDone() / Rmt5PeripheralESPImpl::waitAllDone() wraps rmt_tx_wait_all_done() (src/platforms/esp/32/drivers/rmt/rmt_5/rmt5_peripheral_esp.cpp.hpp). That is the likely primitive for an RMT5 waitDone() override.
• src/platforms/esp/32/drivers/rmt/rmt_5/channel_driver_rmt.cpp.hpp:214-231 currently calls waitForReady() before beginning a new transmission. That path should be checked alongside the manager's onBeginFrame() / onEndFrame() waits.

Proposal

1. Re-run the [Bug] Compounded timing inaccuracy on master compared to 3.10.3 #2420 reproduction on current master with serial timing output.
2. If current master still drifts beyond tolerance, implement the ESP32-S3/RMT5 waitDone() override around the existing RMT5 waitAllDone() completion primitive.
3. Confirm the no-WiFi/no-BT case avoids the vTaskDelay(>=1 tick) path entirely.
4. Keep the ESP32-S3: I2S driver starves websockets, SPI output incorrect, Audio driver FPS impact #2254 invariant: when WiFi/lwIP/BT is active, FastLED must still yield enough for network tasks.
5. Add a regression test or hardware-harness mode so this does not regress silently.

Acceptance Criteria

• The [Bug] Compounded timing inaccuracy on master compared to 3.10.3 #2420 35-LED ESP32-S3 reproduction reports a 2500 ms cycle within a tight tolerance on current master after the fix. Target: +/-10 ms if the hardware harness is stable; otherwise document the measured jitter floor and use that as the threshold.
• FastLED.show() for a single RMT5 strip with no WiFi/BT active has no visible >=1 FreeRTOS tick floor. For 35 WS2812B LEDs, wall time should be close to the ~1.05 ms bitstream duration plus small overhead.
• RMT5 either overrides IChannelDriver::waitDone() with an ISR/IDF completion wait, or the issue documents why the generic tiered wait is sufficient with current measurements.
• WiFi/lwIP behavior from ESP32-S3: I2S driver starves websockets, SPI output incorrect, Audio driver FPS impact #2254 is not regressed when a network is active.
• Binary-size work remains out of scope.

Open Questions

• Does the original [Bug] Compounded timing inaccuracy on master compared to 3.10.3 #2420 sketch still reproduce on current master after perf(channels): runtime NetworkDetector-gated yield in waitForCondition (refs #2815) #2817/perf(channels): add Tier-2 microsecond spin to waitForCondition (Phase 1 of #2815, Closes #2818) #2827/perf(channels): add waitDone() virtual + manager N=1 fast path (Phase 2 of #2815, Closes #2819) #2828?
• Is the RMT5 waitAllDone() wrapper safe to call from the waitDone() fast path for every RMT5 channel state, including queued/multi-channel cases?
• Is ChannelManager::onBeginFrame()->waitForReady() still necessary for the single RMT5 path, or can readiness be enforced lazily at enqueue/show time without adding frame-start latency?

Related Issues

• [Bug] Compounded timing inaccuracy on master compared to 3.10.3 #2420 - original bug report and reproduction
• [Tracking] ESP32-S3 timing regression + binary bloat from Channel API (from #2420) #2421 - mixed timing + binary-size tracker; closed, but useful for the first timing diagnosis
• perf(channels): replace =1 FreeRTOS-tick busy-sleep in waitForReady with ISR-completion + NetworkDetector-gated yield (refs #2420, #2254, #2493) #2815 - closed umbrella for replacing the >=1 tick wait-loop behavior
• perf(channels): Phase 2 of #2815 - waitDone() virtual + per-driver ISR fast path #2819 - closed Phase 2 issue that added waitDone() scaffolding; current tree still needs driver-specific overrides
• ESP32-S3: I2S driver starves websockets, SPI output incorrect, Audio driver FPS impact #2254 - WiFi/lwIP starvation rationale for deep yields

[zackees]

I looked through the current channel-driver and RMT5 code. My recommendation is to add an ISR completion wake hint to IChannelDriver, but keep poll() as the authoritative state transition and cleanup mechanism.

Relevant current state:

• IChannelDriver::waitDone() already exists, but the default is still waitForReady(timeoutMs), so unmigrated drivers use the generic tiered polling loop.
• ChannelManager::waitForReady() already has a single-driver fast path that calls onlyBusy->waitDone(timeoutMs), but multi-driver waits still poll all drivers.
• RMT5 already has IRMT5Peripheral::registerTxCallback() and Rmt5PeripheralESPImpl::waitAllDone() over rmt_tx_wait_all_done().
• ChannelEngineRMTImpl::transmitDoneCallback() currently only sets ChannelState::transmissionComplete = true and returns false.
• RMT5 poll() is still required after the ISR fires because it calls releaseChannel(), disables/releases completed channels, clears ChannelData::mInUse, and starts any pending channels. So the ISR notification must wake the wait path; it must not replace poll().

Recommended abstract API shape:

enum class ChannelDoneKind : fl::u8 {
    Progress,     // some ISR-visible TX completed; poll() may advance state
    DriverReady,  // optional future stronger signal
};

struct ChannelOnDoneInfo {
    // Must be a resolved concrete bus. `Bus::AUTO` is only a selection sentinel
    // and should not reach the wait/ISR completion path.
    fl::Bus bus;
    ChannelDoneKind kind = ChannelDoneKind::Progress;
};

// I recommend bool rather than void so ESP-IDF callbacks can propagate
// "higher priority task woken" back to the peripheral callback return value.
using ChannelOnDoneFromIsr = fl::function<bool(const ChannelOnDoneInfo&)>;

class IChannelDriver {
public:
    virtual fl::Bus getBus() const FL_NOEXCEPT { return fl::Bus::AUTO; }
    virtual bool supportsOnDoneFromIsr() const FL_NOEXCEPT { return false; }
    virtual void setOnDoneFromIsr(ChannelOnDoneFromIsr cb) FL_NOEXCEPT { (void)cb; }
};

If we want to keep the originally proposed fl::function<void(BUSTYPE)>, that can work, but it loses the ESP-IDF higher_priority_task_woken result. A custom ChannelOnDoneInfo also gives us room to distinguish Progress vs DriverReady later without changing the callback signature.

For the first implementation, I would not make fl::Bus itself a flag enum. fl::Bus is already used as a compact sequential template/runtime identifier, and busName()/the static assert assume those ordinal values. Instead:

1. Add the callback API above.
2. Give ChannelManager one ISR-safe completion notifier.
3. When an ISR-capable driver is registered, call driver->setOnDoneFromIsr(...).
4. The callback just signals a binary semaphore/event and returns whether a higher-priority task was woken.
5. On wake, the manager polls all enabled drivers. This is cheap and preserves the existing cleanup/state-machine behavior.

Important detail: the current fl::binary_semaphore::release() on ESP32 maps to xSemaphoreGive(), not xSemaphoreGiveFromISR(). So this should be a small manager-owned ChannelDoneNotifier or an extension to the semaphore facade with release_from_isr(). The callback itself must stay ISR-safe: no allocation, no logging, no driver cleanup, no channel release.

On ESP32 specifically, the notifier should use the FreeRTOS ISR wake path and preserve the BaseType_t higherPriorityTaskWoken result. In practice that means xSemaphoreGiveFromISR() / task notification from ISR, then returning true through the ESP-IDF peripheral callback when a waiter was woken. ESP-IDF/FreeRTOS can then request a context switch on ISR exit, so the FastLED/loop task blocked in the channel wait can run at the next scheduling opportunity instead of waiting for the next tick or another polling pass. This is the key real-time win for the ESP32 path.

The wait logic should be centralized around the wait goal, because READY and READY_OR_DRAINING are different waits:

enum class ChannelWaitGoal : fl::u8 {
    Ready,
    ReadyOrDraining,
};

bool ChannelManager::waitFor(ChannelWaitGoal goal, fl::u32 timeoutMs) {
    for (;;) {
        const auto state = poll();
        if (goal == ChannelWaitGoal::Ready && state.state == IChannelDriver::DriverState::READY) return true;
        if (goal == ChannelWaitGoal::ReadyOrDraining &&
            (state.state == IChannelDriver::DriverState::READY ||
             state.state == IChannelDriver::DriverState::DRAINING)) return true;

        if (!hasAnyIsrDoneDriver()) {
            return waitForCondition([this, goal]() { /* existing poll predicate */; }, timeoutMs);
        }

        // Preserve #2254 behavior: while drivers are BUSY and WiFi/BT is active,
        // keep preferentially running system/network work instead of only blocking
        // on the completion semaphore.
        if (state.state == IChannelDriver::DriverState::BUSY &&
            fl::NetworkDetector::isAnyNetworkActive()) {
            task::run(250, task::ExecFlags::SYSTEM);
            continue;
        }

        // No network, or we are waiting through DRAINING: suspend/block until an
        // ISR-capable driver reports progress, then poll all drivers again.
        if (!mDoneNotifier.wait(timeout_remaining)) return false;
    }
}

RMT5 integration should be minimal:

• ChannelEngineRMTImpl::getBus() returns fl::Bus::RMT.
• supportsOnDoneFromIsr() returns true.
• setOnDoneFromIsr() stores the callback.
• ChannelState gets a stable owner/callback pointer, or the callback pointer itself, because the current ISR user data is only ChannelState*.
• transmitDoneCallback() keeps doing state->transmissionComplete = true, then invokes the manager callback with {fl::Bus::RMT, ChannelDoneKind::Progress} and returns the callback's bool.
• poll() continues to call releaseChannel(), and releaseChannel() continues to call waitAllDone() before freeing buffers / clearing mInUse.

For a later optimization, add a separate bus mask rather than changing fl::Bus values:

using ChannelBusMask = fl::u32;
constexpr ChannelBusMask channelBusBit(fl::Bus bus) {
    // `AUTO` should have been resolved before entering the wait/ISR path.
    // Seeing it here is a caller bug, not a valid "no bit" condition.
    FL_ASSERT(bus != fl::Bus::AUTO, "Channel wait path requires resolved bus");
    const auto index = static_cast<fl::u8>(bus);
    return ChannelBusMask{1} << index;
}

Then ChannelManager can hold fl::atomic_u32 mDoneBusMask and the ISR callback can CAS-OR the bus bit before signaling the semaphore. The wait loop can CAS-clear observed bits and poll only matching drivers. This path should only ever see resolved concrete buses; Bus::AUTO is a pre-dispatch selection sentinel and should be resolved by channel binding / driver selection before any wait-state or ISR-completion callback is installed. That requires either a CAS loop with the current fl::atomic_u32 API or adding exchange/fetch_or to AtomicReal. Until driver count or poll cost becomes meaningful, the binary-semaphore + poll-all implementation is simpler and less risky.

Suggested tests:

• Mock ISR-capable driver registers the callback and manager wakes from waitForReady() after callback invocation.
• waitForReadyOrDraining() returns on DRAINING; waitForReady() waits through DRAINING until READY.
• Multiple callback invocations coalesce safely into one wake and a poll-all pass.
• RMT5 path verifies the ISR callback only marks progress/signals, while cleanup still happens in poll().

[zackees]

Implementation update: PR #2992 now uses PollNeededCallback / setPollNeededCallback() rather than a done callback or per-driver wait capability.

The current shape is:

• ChannelManager owns one poll-needed wait signal.
• ESP32 uses a manager-owned fl::binary_semaphore for that signal.
• The ESP32 semaphore wrapper is now ISR-aware: release() detects ISR context, calls xSemaphoreGiveFromISR(), and uses portYIELD_FROM_ISR() when needed.
• Drivers only store and invoke the manager callback when ISR-visible state changes. That may mean a chunk/batch/transfer finished, but it does not imply the driver is fully READY; poll() remains the authoritative state-machine and cleanup path.
• If a driver never invokes the callback, the manager still wakes on bounded 1 ms timeout slices and polls, so there is no explicit support-capability gate and no per-driver semaphore.
• Bus::AUTO remains invalid for any future bus-mask wait state; wait/ISR state must use resolved concrete buses only.

This keeps the implementation compatible with future ISR-driven inter-batching while avoiding a larger ISR FSM today.

[zackees]

Final result: #2992 merged to master on 2026-06-09 and resolves this tight-timing tracker.

What landed:

• IChannelDriver::PollNeededCallback / setPollNeededCallback() so ISR-visible driver progress can wake the channel manager promptly.
• A manager-owned platform ChannelPollSignal wait path instead of ESP-specific manager drain loops.
• ESP32 fl::binary_semaphore::release() is now ISR-aware and uses xSemaphoreGiveFromISR() plus portYIELD_FROM_ISR() when appropriate.
• AutoResearch now has a --tight-timing metric for FastLED.show(); FastLED.wait() overhead.

Hardware validation from #2992:

• ESP32-C6 PARLIO: max overhead 1266 us with a 2000 us limit.
• ESP32-C6 RMT: max overhead 296 us with a 2000 us limit.
• ESP32-S3 RMT: max overhead 516 us with a 2000 us limit.
• ESP32-S3 I2S: max overhead 1503 us with a 2000 us limit.

The remaining AutoResearch stack-size cleanup is intentionally tracked separately in #2993.