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.