Minimal C3 bitbanging fix

[willmmiles]

This is the simplest, lamest solution to C3 glitches: replace it with the bitbanging driver. I am presenting this here as a draft for reference if we decide we need an immediate solution, no matter the performance cost. This is not a good solution by any means - merely a working one on my test system.

Sadly it turned out that NPB's bitbanging driver also wasn't completely stable on RISCV systems, I am guessing due to cache issues returning from ISR handling. Lowering the latch time estimates and prefetching the pixel outputs on to the stack seemed to help on my test system. (Probably I should've added a "ISR return fudge factor" instead of permuting the timings directly; that could be done if there's interest in pursing this.

Summary by CodeRabbit

• Improvements

  • Added dedicated support for ESP32-C3 boards with improved addressable LED driver compatibility, ensuring proper control and reliability on this microcontroller variant.

• Chores

  • Updated NeoPixel library dependency to the latest version.

[coderabbitai]

Important

Review skipped

Draft detected.

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9fed5eed-74aa-4583-a523-b51ecc4c4a2f

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)

---

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.}

[DedeHai]

as per my tests: to fully fix glitching on C3, do not allow any ISRs during BB from start to finish.

edit:
for reference:
https://github.com/DedeHai/WLED/blob/5860b73b09f273fa4b7bd5475b675a2339611f61/wled00/src/WLEDpixelBus/WLEDpixelBus_BitBang.cpp#L331

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

wled00/bus_wrapper.h (1)

254-259: Consider adding a compile-time constructor contract check.

The wrapper forwards only the first 4 arguments to the parent NeoEsp32BitBang*Method class, silently discarding the NeoBusChannel parameter. Per WLED coding guidelines, use static_assert to formalize this contract and catch API incompatibilities at compile time rather than at runtime.

Suggested refactor

 template<typename T> class NPBMethodStripChannelWrapper : public T {
 public:
+    static_assert(std::is_constructible<T, uint8_t, uint16_t, size_t, size_t>::value,
+                  "NeoEsp32BitBang method must support ctor(pin, pixelCount, elementSize, settingsSize)");
-    NPBMethodStripChannelWrapper(uint8_t pin, uint16_t pixelCount, size_t elementSize, size_t settingsSize, NeoBusChannel channel) : T(pin, pixelCount, elementSize, settingsSize) {
-    };
+    NPBMethodStripChannelWrapper(uint8_t pin, uint16_t pixelCount, size_t elementSize, size_t settingsSize, NeoBusChannel /*channel*/)
+      : T(pin, pixelCount, elementSize, settingsSize) {}
 };

🤖 Prompt for 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.

In `@wled00/bus_wrapper.h` around lines 254 - 259, The
NPBMethodStripChannelWrapper constructor accepts a NeoBusChannel parameter but
silently discards it by not forwarding it to the parent class constructor. Add a
static_assert inside the NPBMethodStripChannelWrapper template class to verify
that the parent class T does not require a NeoBusChannel parameter in its
constructor, formalizing the API contract at compile time and preventing future
incompatibilities when the parent class signature changes.

Source: Coding guidelines

🤖 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 `@platformio.ini`:
- Line 133: This change to the NeoPixelBus dependency in platformio.ini modifies
the git commit reference and requires explicit approval from a WLED maintainer
or organization member before it can be merged, as platformio.ini changes are
under a hard approval gate. Ensure that a maintainer or WLED organization member
has explicitly approved this dependency switch by adding their approval comment
to the pull request before proceeding with the merge.

---

Nitpick comments:
In `@wled00/bus_wrapper.h`:
- Around line 254-259: The NPBMethodStripChannelWrapper constructor accepts a
NeoBusChannel parameter but silently discards it by not forwarding it to the
parent class constructor. Add a static_assert inside the
NPBMethodStripChannelWrapper template class to verify that the parent class T
does not require a NeoBusChannel parameter in its constructor, formalizing the
API contract at compile time and preventing future incompatibilities when the
parent class signature changes.

🪄 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: 5dde25c6-fd46-499c-934e-70c3d4dd90cf

📥 Commits

Reviewing files that changed from the base of the PR and between ae4b2b3 and ad5417d.

📒 Files selected for processing (2)

• platformio.ini
• wled00/bus_wrapper.h

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Require explicit maintainer/WLED member approval for this platformio.ini change before merge.

This file is under a hard approval gate, and this dependency switch should not merge without explicit maintainer or WLED organization-member sign-off.

Based on learnings from the coding rules: “Changes to platformio.ini require maintainer approval” and “MUST be approved explicitly by a maintainer or WLED organisation Member.”

🤖 Prompt for 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.

In `@platformio.ini` at line 133, This change to the NeoPixelBus dependency in
platformio.ini modifies the git commit reference and requires explicit approval
from a WLED maintainer or organization member before it can be merged, as
platformio.ini changes are under a hard approval gate. Ensure that a maintainer
or WLED organization member has explicitly approved this dependency switch by
adding their approval comment to the pull request before proceeding with the
merge.

Source: Coding guidelines

[DedeHai]

@willmmiles one thing I recently thought of but did not check yet:
on the S2 we allow I2S output even though it clashes with AR. We could do the same for C3, maybe there is a minimal check that can be done through an IDF call to check if the I2S is initialized - although beginStrip() is called before UM-setup, so this check would need to be done in AR usermod... aaand we are back to a hardware manager ;)

[willmmiles]

as per my tests: to fully fix glitching on C3, do not allow any ISRs during BB from start to finish.

I don't think that's acceptable in the general case. It'll risk watchdog failures and other panics from the wifi system when under load. As far as I'm aware, ESP-IDF can only reliably handle interrupt gaps of a few ms.

The NPB patch here doesn't include latch time corrections for all LED types - I only tuned the one on my test bench as a proof-of-concept. If you tested this patch, what flavour were you using?

[DedeHai]

I don't think that's acceptable in the general case

what would be the worst case scenario to test if its viable? I did test with 1000 LEDs so ~30ms blocking and saw no issue with that, heavy UI reload and AR audio incoming stream. We could put a 512 LED limit on BB outputs.

[willmmiles]

@willmmiles one thing I recently thought of but did not check yet: on the S2 we allow I2S output even though it clashes with AR. We could do the same for C3, maybe there is a minimal check that can be done through an IDF call to check if the I2S is initialized - although beginStrip() is called before UM-setup, so this check would need to be done in AR usermod... aaand we are back to a hardware manager ;)

So we are!

I'm posting this for posterity as a starting place if we decide we need a quick drop-in workaround for 16.x. For 17.0 I think we should merge your new driver framework with the expanded hardware options including parallel bitbanging, SPI, and the like, and offer direct selection of driver and options via the JSON config.

[willmmiles]

I don't think that's acceptable in the general case

what would be the worst case scenario to test if its viable? I did test with 1000 LEDs so ~30ms blocking and saw no issue with that, heavy UI reload and AR audio incoming stream. We could put a 512 LED limit on BB outputs.

That's interesting - I was under the impression the IWDT default was 20ms. Are you explicitly disabling it?

I do agree we'll want to put a cap on bitbanging lengths regardless.

[willmmiles]

what would be the worst case scenario to test if its viable?

autocannon targeting /presets.json hits both bandwidth and flash IO. Another thing to check is enabling wifi sleep, and then triggering a lot of broadcast traffic. I'm not sure what happens if wifi sleep is enabled and you miss the collection window.

[DedeHai]

I'm posting this for posterity as a starting place if we decide we need a quick drop-in workaround for 16.x.

we can definitely add it as an option, in my WLEDbus branch I alread added it to the UI selection menu (well, claude did, I only glanced over that part and it looked legit and it works). In 16.0 ESP32, S2, S3 already offer that selection menu (RMT / I2S) so adding BB to it and restricting to 512 or 1024 LEDs if selected should be doable, although it may be a bit of a painful chore.

[willmmiles]

I'm posting this for posterity as a starting place if we decide we need a quick drop-in workaround for 16.x.

we can definitely add it as an option, in my WLEDbus branch I alread added it to the UI selection menu (well, claude did, I only glanced over that part and it looked legit and it works). In 16.0 ESP32, S2, S3 already offer that selection menu (RMT / I2S) so adding BB to it and restricting to 512 or 1024 LEDs if selected should be doable, although it may be a bit of a painful chore.

Don't spend too much time on the integration -- it's still my intention to replace the bus configuration backend with a proper JSON configuration structure which allows unique properties for each hardware interface. (I still really, really hate the existing solution of mapping bus hardware types to integers. It leaves us stuck with either a global mapping for all hardware for all platforms -- so anything new requires a core change -- or worse, a unique string mapping for each platform in the UI. Hardware type selection needs to be strings.)

[DedeHai]

I tested the absolute worst case: 2048 LEDs, 400kHz bitbang output. this blocks for 125ms from enter to exit critical. It somehow still works just fine. UI is quite responsive albeit not blazing fast. DDP streaming from VideoLab through websocket does not work anymore though, I do get one update every 10 seconds tops so something gets congested - but no crashes. FPS drop to 7 but that is expected. Does not matter if I have wifi sleep enabled or disabled, its about the same.

edit:
sync sendout also works from the BB unit. Sync receive on the BB uint is sluggish, it works if I set the sender to repeat 3 times but there is a significant delay of up to 1s.

[softhack007]

as per my tests: to fully fix glitching on C3, do not allow any ISRs during BB from start to finish.

I don't think that's acceptable in the general case. It'll risk watchdog failures and other panics from the wifi system when under load. As far as I'm aware, ESP-IDF can only reliably handle interrupt gaps of a few ms.

@DedeHai it means you were able to "hold" a critical section up for more than 100ms? Wow, that's quite long.

And it's much more than what I would have guessed from reading the FreeRTOS docs https://m.freertos.org/Documentation/02-Kernel/04-API-references/04-RTOS-kernel-control/01-taskENTER_CRITICAL_taskEXIT_CRITICAL

Critical sections must be kept very short, otherwise they will adversely affect interrupt response times. Every call to taskENTER_CRITICAL() must be closely paired with a call to taskEXIT_CRITICAL().
FreeRTOS API functions must not be called from within a critical section. taskENTER_CRITICAL() and taskEXIT_CRITICAL() must not be called from an interrupt service routine (ISR).

[DedeHai]

it means you were able to "hold" a critical section up for more than 100ms? Wow, that's quite long.

I have no idea how the ESP framework handles watchdogs exactly, never looked into that. But from my debug sessions where an ISR gets stuck it looks like it triggers after a very long time, like 1 or 2 seconds. What may be an issue with keeping interrupts from firing may be IR receivers, don't have one so could not test.

[willmmiles]

I did a bit of digging and found out that the default interrupt watchdogs are absurdly long (300ms)!

Yes, interrupt latencies on the >1 ms scale will pretty much kill IR reception, I think - the library we're currently using depends on software timing. Tradeoffs, tradeoffs, everywhere...

[softhack007]

(a bit off-topic) I'm curious if the rabbit can dig up some constraints 🤔

@coderabbitai please check if the official documentation for esp-idf v4.4.x and arduino-esp32 v2.0.x has some statements / hints on how long a critical section (taskENTER_CRITICAL()) can be held open before the system becomes unreliable.

Also check the esp-idf and arduino-esp32 source code for additional information - assume a standard arduino-esp32 configuration of esp-idf, especially a task scheduler period of 1ms.

Would flash filesystem access, wifi, or PSRAM access be degraded when the looptask holds a critical section open for 100ms?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🤖 web_search executed:

ESP-IDF portMUX_TYPE spinlock taskENTER_CRITICAL maximum duration documentation esp32

Length of output: 14783

---

🤖 web_search executed:

CONFIG_ESP_INT_WDT_TIMEOUT_MS default value esp-idf interrupt watchdog

Length of output: 5190

---

🤖 web_search executed:

esp-idf FreeRTOS portENTER_CRITICAL disables interrupts on both cores esp32 documentation

Length of output: 13568

---

🤖 web_search executed:

esp32 wifi requires interrupts not disabled for more than a few milliseconds timing critical

Length of output: 10125

---

🤖 web_search executed:

esp-idf Kconfig ESP_INT_WDT_TIMEOUT_MS default "300" OR "custom" milliseconds

Length of output: 6058

---

@softhack007 Good question — I dug through the ESP-IDF docs/source and here's what I could actually substantiate (v4.4.x / v5.x docs are consistent on this point, and the arduino-esp32 core just wraps this same ESP-IDF FreeRTOS port):

1. Does the documentation state a maximum duration for taskENTER_CRITICAL()?

No hard numeric limit is documented. The FreeRTOS (IDF) programming guide only says qualitatively:

"Critical sections should be kept as short as possible... The longer the critical section lasts, the longer a pending interrupt can be delayed."

and separately, from the Watchdogs guide:

"Neither critical sections nor interrupt handlers should ever block waiting for another event to occur."

There's no explicit "N milliseconds is safe" statement — the de facto hard ceiling is the Interrupt Watchdog (IWDT), which will panic/reset the SoC if the FreeRTOS tick ISR is starved for too long.

2. What is the actual IWDT ceiling for a standard arduino-esp32 config on C3?

From the ESP-IDF Kconfig source (components/esp_system/Kconfig):

config ESP_INT_WDT_TIMEOUT_MS
    default 300 if !(SPIRAM && IDF_TARGET_ESP32)
    default 800 if (SPIRAM && IDF_TARGET_ESP32)
    range 10 10000

Note the PSRAM exception (800ms) only applies to the classic ESP32 target — the C3 always gets the default 300ms IWDT timeout regardless of whether PSRAM is fitted (this asymmetry is even flagged as a known ESP-IDF issue for other PSRAM-capable targets like S3). The assert in int_wdt.c also requires this timeout be ≥ 2× the tick period (so with a 1ms tick, minimum configurable is technically 10ms per the Kconfig range, but must stay meaningfully above the tick period).

So: DedeHai's 125ms test critical section is under the 300ms default IWDT limit on C3, but leaves only ~58% margin before a panic — not a comfortable safety margin, and if anything else briefly overlaps (e.g., a flash op right before/after), it could tip over.

3. Mechanics — does this block both cores?

Per the port.c source, taskENTER_CRITICAL(&spinlock):

• Disables interrupts (up to configMAX_SYSCALL_INTERRUPT_PRIORITY) only on the calling core.
• Then spins on the shared spinlock.

C3 is single-core, so this is moot for this PR, but worth noting for the general-case discussion in this thread (S2/S3/dual-core parts): the other core's interrupts are not directly disabled by taskENTER_CRITICAL, only its ability to acquire that same spinlock is blocked.

4. Would flash, WiFi, or PSRAM access be degraded by a 100ms critical section?

• Flash: SPI flash writes/erases already invoke a stronger, independent mechanism — per the Speed Optimization guide: "Task execution is always completely suspended when writing to the built-in SPI flash chip. Only IRAM-Safe Interrupt Handlers continue executing." This is orthogonal to taskENTER_CRITICAL but compounds the same risk class — if a flash write is queued and your BB critical section also blocks the tick, you're stacking two interrupt-starvation windows.
• WiFi: the WiFi driver task runs at high priority (23), pinned to core 0 by default per the same guide. Any interrupts it relies on at priority ≤ configMAX_SYSCALL_INTERRUPT_PRIORITY (levels 1–2, the ESP-IDF default allocation for most drivers) will be blocked for the full critical-section duration; only interrupts explicitly allocated at level 3+ (uncommon, requires assembly handlers) survive. This lines up with willmmiles' concern about beacon/RX timing.
• PSRAM: docs don't state PSRAM access itself gets "slower" due to a critical section; the PSRAM+IWDT exception exists because IDF's own critical sections/ISRs that walk large PSRAM regions can legitimately take longer, so the watchdog is given more slack — that's tuning the watchdog to the platform, not evidence that PSRAM access speed changes during someone else's critical section.

Bottom line: There's no documented "safe" numeric ceiling beyond "as short as possible" — the practical ceiling is the IWDT default of 300ms on C3 (unaffected by PSRAM, unlike classic ESP32). A 100–125ms critical section is under that limit with no PSRAM adjustment in play, but it eats a large chunk of the watchdog margin and, per docs, will delay lower-priority interrupts (including most WiFi-related ones on core 0) for the full duration — consistent with willmmiles' concern that this isn't safe "in the general case," even if DedeHai's specific bench tests didn't trip a panic.

(°ω°) — happy to dig further into the "restrictions and considerations" list in the critical-sections doc if useful, e.g. the bullet points on what APIs must never be called inside a critical section.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🤖 web_search executed:

ESP-IDF v4.4 taskENTER_CRITICAL portMUX_TYPE spinlock maximum duration documentation FreeRTOS ESP32

Length of output: 11190

---

🤖 web_search executed:

ESP-IDF CONFIG_ESP_INT_WDT_TIMEOUT_MS default value interrupt watchdog timeout esp32

Length of output: 5014

---

🤖 web_search executed:

ESP-IDF task watchdog timer TWDT default timeout 5 seconds esp32 idle task

Length of output: 10371

---

🤖 web_search executed:

esp32 arduino disabling interrupts for long time effect on wifi stack timing beacon interval

Length of output: 13351

---

@softhack007 Here's what I could dig up:

No hard "safe duration" number exists in the official docs — Espressif's stance is simply "critical sections should be kept as short as possible" and that the risk grows monotonically with duration. But there are two concrete thresholds baked into the framework that act as de-facto ceilings:

1. Interrupt Watchdog (IWDT) — CONFIG_ESP_INT_WDT / CONFIG_ESP_INT_WDT_TIMEOUT_MS. This is the real gatekeeper for critical sections, since it triggers a panic if no interrupt-level activity happens on a CPU for longer than the configured timeout. On stock arduino-esp32 (which wraps esp-idf v4.4.x), this defaults to 300ms — which lines up exactly with willmmiles's finding in the thread. There's a hard assert in int_wdt.c requiring the timeout be at least 2× the FreeRTOS tick period (1ms tick by default → 2ms minimum), but 300ms is the practical shipped default. Espressif's docs explicitly note the default is bumped even higher automatically when PSRAM support is enabled, "as a critical section or interrupt routine that accesses a large amount of PSRAM takes longer to complete in some circumstances" — confirming that PSRAM access inside a critical section is a recognized risk multiplier, not a hard-and-fast crash trigger, but Espressif clearly treats it as slower/less predictable.

   This explains DedeHai's result: a ~125ms critical section survives comfortably under the 300ms IWDT ceiling on a PSRAM-less C3.

2. Task Watchdog (TWDT) — CONFIG_ESP_TASK_WDT_TIMEOUT_S, default 5 seconds, watches the per-core idle tasks. Irrelevant at ms-scale bitbanging, but it's the second line of defense if something goes really wrong.

Regarding your specific sub-questions:

• Flash filesystem access — no explicit numeric doc statement found. The theoretical risk is around cache/IPC coordination between cores for external SPI flash access (especially on single-core-vs-dual-core-locking chips); if flash-cache handling needs the other core's cooperation while it's stuck in a critical section, that's a candidate for the IWDT/cache-lock livelock workaround code (CONFIG_ESP32_ECO3_CACHE_LOCK_FIX) that exists specifically for this class of problem on original ESP32. C3 is single-core RISC-V though, so cross-core IPC/cache-lock isn't a factor there — the bigger risk on C3 is simply the IWDT itself and any single-core interrupt-driven peripheral (flash SPI included) not being serviced.
• WiFi — no official quantified statement either, but real-world evidence (multiple espressif/arduino-esp32 issues) shows that blocking interrupts/tasks for tens of ms can desync modem-sleep timing relative to the AP's beacon interval (typically 100ms) and cause missed beacons/disconnects or spurious "ghost" ISR firings; WiFi.setSleep(false) (i.e., WIFI_PS_NONE) is the community workaround, consistent with what's been discussed upthread about DDP/websocket congestion under load.
• PSRAM — officially acknowledged as slowing down critical sections (see IWDT note above); C3 typically doesn't have PSRAM in the common WLED configs, so this is likely moot for this specific PR.

Net takeaway: the ~300ms IWDT default is the actual hard ceiling that matters here, not some documented "recommended max." DedeHai's 125ms held critical section is well within that margin, but pushing further (larger LED counts / slower bitbang speeds) risks approaching it, especially if PSRAM is ever added to the mix.

Citations:

• https://docs.espressif.com/projects/esp-idf/en/v4.4.4/esp32/api-guides/freertos-smp.html
• https://docs.espressif.com/projects/esp-idf/en/v4.4/esp32/api-reference/system/wdts.html
• https://github.com/espressif/esp-idf/blob/master/components/esp_system/int_wdt.c
• GPIO36 "fake" falling interrupts espressif/arduino-esp32#4903
• The WiFi task suspends other tasks on both cores of ESP32 espressif/arduino-esp32#4176

┏(・o・)┛♪ digging through esp-idf source is fun

---

✏️ Learnings added

Learnt from: softhack007
URL: https://github.com/wled/WLED/pull/5690

Timestamp: 2026-07-06T20:06:53.256Z
Learning: For the WLED repository (wled00), regarding ESP32-C3 bitbang driver stability discussions (e.g. PR `#5690`): the ESP-IDF Interrupt Watchdog (IWDT, CONFIG_ESP_INT_WDT_TIMEOUT_MS) defaults to ~300ms on arduino-esp32 and is the practical hard ceiling before a critical section (taskENTER_CRITICAL) triggers a panic/reset, not some smaller "recommended" duration. This default is raised further automatically when PSRAM is enabled since PSRAM access inside a critical section takes longer. There's no official Espressif-documented safe duration for holding a critical section other than staying under the IWDT timeout; WiFi modem-sleep/beacon timing (~100ms interval) can also be disrupted by long blocked interrupts, based on community-reported arduino-esp32 issues, though this is not officially quantified.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.