Skip to content

Conversation

@willmmiles
Copy link
Member

Backport of #4890 for 0.15. Solves RMT flickering issues on XTensa platforms.

Pull the RMT High-priority Interrupt driver in to a vendored local
library, pending inclusion in upstream NeoPixelBus.

Driver is enabled only for XTensa chips; there's some unresolved
issue with nested interrupts on RISCV.
PlatformIO's Library Dependency Finder will take care of it based on
the #include.
While these were both fortunately included in Arduino.h, as
@coderabbitai suggests, it's best practice to be explicit for anything
one uses directly.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 4, 2025

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.

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

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@netmindz netmindz added this to the 0.15.2 milestone Oct 5, 2025
@willmmiles willmmiles merged commit 4cee975 into wled:0_15_x Oct 5, 2025
20 checks passed
@softhack007
Copy link
Member

softhack007 commented Nov 8, 2025

@willmmiles one question before I merge this into WLED-MM
we are still on NeoPixelBus 2.7.5 - do you think your patch will work with that version, too?

Or maybe not needed for NeoPixelBus < 2.8.3 ?

@softhack007
Copy link
Member

@coderabbitai this PR was created with NeoPixelBus release 2.8.3 in mind. Would it also help to remove flickering artefacts when using NeoPixelBus release 2.7.5?

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 9, 2025

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the list of changed files in this PR
gh pr view 4980 --json files --jq '.files[].path'

Length of output: 198


🏁 Script executed:

#!/bin/bash
# Check platformio.ini for NeoPixelBus version dependency
fd -t f "platformio.ini" --exec cat {}

Length of output: 29237


🏁 Script executed:

#!/bin/bash
# Look at the main changes in the NeoESP32RmtHI library files
fd -t f -p "NeoEsp32RmtHI" --exec cat {}

Length of output: 61414


🏁 Script executed:

#!/bin/bash
# Check if there's any reference to NeoPixelBus version requirements in comments or documentation
rg -i "neopixelbus.*2\.[78]" -A2 -B2

Length of output: 2804


🏁 Script executed:

#!/bin/bash
# Check the original PR #4890 for any version requirements or discussions
gh pr view 4890 --json body,title,comments --jq '.body, .title, .comments[].body' | head -100

Length of output: 4830


🏁 Script executed:

#!/bin/bash
# Search for ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT usage in the codebase
rg "ESP_ERROR_CHECK_WITHOUT_ABORT" -C3

Length of output: 2343


🏁 Script executed:

#!/bin/bash
# Check bus_wrapper.h for NeoPixelBus version checks or compatibility code
cat wled00/bus_wrapper.h | head -200

Length of output: 9133


🌐 Web query:

NeoPixelBus ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT version introduced

💡 Result:

I can look that up — can you tell me where you saw the symbol (compile error, issue link or log) and which ESP/Arduino/ESP-IDF and NeoPixelBus versions you’re using? If you prefer, I’ll search GitHub/NeoPixelBus issues and releases now for when that symbol appeared.


🌐 Web query:

NeoPixelBus 2.7.5 2.8.3 API changes differences

💡 Result:

Short summary of API/behavior changes from NeoPixelBus 2.7.5 → 2.8.3

  • Source of truth: official GitHub releases for Makuna/NeoPixelBus and the project wiki. [1][2]

Key changes (user-visible API/behavior):

  • New pixel/chip support and platform improvements added in the 2.8.x series (e.g. WS2814, HD108 support and various platform/ESP/RP enhancements introduced in 2.8.0+). [1][2]
  • Multiple ESP/ESP32 fixes and timing/back-end improvements (ESP32 S3, ESP32 bit‑bang pin support, ESP8266/ESP32 timing/cadence improvements). [1]
  • Compiler/port fixes in 2.8.2/2.8.3 (for example an ESP32‑C3 bit‑bang compiler error fix) and other bugfix releases — these are mostly fixes rather than breaking API removals. [1]
  • NeoPixelAnimator was updated to support configurable time scales (new constructor/time scale flags NEO_MILLISECONDS / NEO_CENTISECONDS / NEO_DECISECONDS / NEO_SECONDS) which changes how animation timing values are interpreted. This is a behavioral/API enhancement for animations. [3]

Practical impact:

  • Most changes across 2.8.x are platform support and bug fixes; existing APIs remain usable but behavior/timing for animations may differ if you rely on the animator default timescale—check and, if needed, pass the new timescale value when constructing NeoPixelAnimator.
  • If you rely on very specific low‑level timing/bitbang behavior on ESP/ESP32 variants, test on your target hardware (2.8.x contains platform-specific fixes and changed defaults in some backends).

References

  • GitHub releases (Makuna/NeoPixelBus) — release notes for 2.8.0 → 2.8.3. [1]
  • NeoPixelBus wiki / overview (features & v2.8.x notes). [2]
  • Discussion / issue mentioning NeoPixelAnimator timescale update. [3]

If you want, I can:

  • produce a file-by-file diff between tag 2.7.5 and 2.8.3 from the GitHub repo, or
  • highlight exact changed functions/headers that affect your sketch — tell me which APIs you use (e.g., NeoPixelAnimator, Begin/Show/SetPixelColor, specific backends).

References:
[1] Makuna/NeoPixelBus — Releases (GitHub).
[2] NeoPixelBus wiki / overview.
[3] NeoPixelAnimator timescale discussion (GitHub).


🏁 Script executed:

#!/bin/bash
# Search NeoPixelBus GitHub for when ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT was introduced
# Let's check the NeoPixelBus releases and tags around 2.7.5 and 2.8.3
gh api repos/Makuna/NeoPixelBus/releases --jq '.[] | select(.tag_name | match("2\\.[78]\\.[0-9]")) | {tag_name, published_at, name}' | head -20

Length of output: 1379


🏁 Script executed:

#!/bin/bash
# Check if there are any comments in the original PR about NeoPixelBus version compatibility
gh pr view 4890 --json comments --jq '.comments[] | select(.body | contains("2.7") or contains("2.8") or contains("version")) | .body' 

Length of output: 24330


🏁 Script executed:

#!/bin/bash
# Let's check what NeoPixelBus API the driver actually depends on by looking at includes and macros
rg -n "NeoPixelBus|NeoEsp32" lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h | head -30

Length of output: 3033


🏁 Script executed:

#!/bin/bash
# Check the library.json manifest file for NeoPixelBus version requirement
cat lib/NeoESP32RmtHI/library.json

Length of output: 222


🏁 Script executed:

#!/bin/bash
# Search for any version checks or conditional compilation in the driver
rg -n "NEOPIXELBUS_VERSION|NPB_VERSION|ESP_ERROR_CHECK_WITHOUT_ABORT" lib/NeoESP32RmtHI/

Length of output: 768


🏁 Script executed:

#!/bin/bash
# Check if there are any comments about version requirements in the driver source
rg -n "2\.[78]\." lib/NeoESP32RmtHI/ -A2 -B2

Length of output: 262


@softhack007 Based on my analysis of the code, this PR would likely NOT work with NeoPixelBus 2.7.5 without modifications. Here's why:

Key Dependencies

The RMT High-priority Interrupt driver has a hard dependency on NeoPixelBus 2.8.3+ for two main reasons:

  1. API Macros: The driver uses ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT and ESP_ERROR_CHECK_WITHOUT_ABORT macros (used in Lines 76, 89, 119, 122 of NeoEsp32RmtHIMethod.h). According to the retrieved learnings, these are part of the NeoPixelBus library API, but it's unclear if they existed in 2.7.5.

  2. Library Manifest: The lib/NeoESP32RmtHI/library.json explicitly specifies:

    "version": "^2.8.3"
  3. Speed/Timing Types: The driver depends on NeoEsp32RmtSpeed* type definitions and NeoBusChannel from NeoPixelBus for timing parameters.

About the Flickering Fix

The flickering artifact fix itself is achieved through the driver's high-priority interrupt handling implementation (in the assembly ISR shim and C++ interrupt code), which operates at the ESP-IDF level. This mechanism is independent of the NeoPixelBus version - it's the driver's own interrupt management that solves the flickering issue.

Path Forward for 2.7.5 Compatibility

If you need this to work with NeoPixelBus 2.7.5, you would need to:

  1. Check if the macros exist in 2.7.5 by examining that version's source
  2. Add compatibility shims if they're missing - define the macros yourself or use alternative error checking
  3. Test thoroughly - the timing types and channel definitions need to be compatible

Alternatively, consider updating WLED-MM to NeoPixelBus 2.7.9 (which you have experimental compat builds for in platformio.ini) or 2.8.3 to get this fix.

Would you like me to help investigate what specific changes would be needed for 2.7.5 compatibility, or would upgrading NeoPixelBus be the preferred approach?


🧠 Learnings used
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.

Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.

Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.

Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.

Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.

Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:87-91
Timestamp: 2025-09-02T02:15:44.324Z
Learning: ESP_ERROR_CHECK_WITHOUT_ABORT_SILENT_TIMEOUT is part of the NeoPixelBus library API and can be safely depended upon when NeoPixelBus is a declared dependency.

Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants