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

Activity

1. zackees

   added

   type: bugSomething is brokenSomething is broken

   type: regressionPreviously working, now brokenPreviously working, now broken

   on Jun 9, 2026
2. fastled-project-sync

   added this to FastLED Trackeron Jun 9, 2026
3. fastled-project-sync

   moved this to Triage in FastLED Trackeron Jun 9, 2026
4. zackees commented on Jun 9, 2026

   

   zackees

   on Jun 9, 2026

   Last edited by zackees

   MemberAuthor

   More actions

   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().
5. zackees commented on Jun 9, 2026

   

   zackees

   on Jun 9, 2026

   MemberAuthor

   More actions

   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.
6. zackees

   closed this as completedin #2992on Jun 9, 2026
7. fastled-project-sync

   moved this from Triage to Done in FastLED Trackeron Jun 9, 2026
8. zackees commented on Jun 9, 2026

   

   zackees

   on Jun 9, 2026

   MemberAuthor

   More actions

   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.