Shrink WS2811/WS2812 LED strip DMA buffer with a chunked circular refill by sensei-hacker · Pull Request #11798 · iNavFlight/inav

sensei-hacker · 2026-08-18T05:45:43Z

Summary

Reduces static RAM used by the WS2811/WS2812 LED strip driver, part of the ongoing RAM reduction effort for 128KB-RAM F4 targets.

Changes

Right-size the DMA buffer element type to the timer's actual 16-bit CCR register width (uint32_t -> uint16_t), fixing a latent 2x waste. (First commit, already landed separately in an earlier pass.)
Bound LED strip processing and the DMA transfer to the actually-configured LED count instead of always processing the full 128-slot maximum.
Add an opt-in, null-guarded circular-DMA refill callback hook to all three timer/DMA backends (F4 StdPeriph, H7/F7 HAL, AT32), invoked from the DMA half/full-transfer-complete interrupt. Verified to be a no-op for the only other circular-DMA consumer (motor DShot idle-packet repeat during EEPROM writes), which never registers a callback.
Replace the whole-strip one-shot DMA buffer (6,230 bytes for a 128-LED strip) with a small 2-half circular buffer (384 bytes) refilled via that hook as each 4-LED group finishes transmitting. WS2812's reset/latch condition is a minimum, not a maximum, low duration, so the old fixed DMA preamble and idle-tail buffer elements are gone too — replaced by a cheap timestamp check and a direct write to the timer's preload-buffered compare register, respectively.
Add a small (4-entry) memoization cache to hsvToRgb24() so repeated colors (very common — most strips use only a handful of distinct colors) skip the conversion math.
All three timer backends' impl_timerPWMStopDMA now poll for the DMA stream's enable bit to actually clear before returning, matching the same poll already used elsewhere for the same hardware constraint — needed because this stop path can now run from a circular-DMA refill callback while a stream is still mid-flight, not just after a one-shot transfer completes.

RAM impact

ledStripDMABuffer: 12,460 B (original uint32_t[3115]) -> 6,230 B (uint16_t[3115], already landed) -> 384 B (uint16_t[192], this PR). BLUEBERRYF405 total RAM: 114,708 B -> 108,876 B.

Testing

Built cleanly (zero warnings) on representative targets for all three timer backends: BLUEBERRYF405 and HGLRCF405V2 (F4 StdPeriph), IFLIGHT_H743_AIO_V2 (H7 HAL), DAKEFPVF435 (AT32).
Reviewed with the inav-code-review agent; addressed all CRITICAL/IMPORTANT findings (a build-breaking gap in the HAL/AT32 backends, missing volatile on ISR-shared state, and the DMA-stop poll gap above).
Not yet tested on real hardware. This touches a shared DMA IRQ handler used by motor PWM/DShot as well as the LED strip, and changes the LED strip's DMA timing model from one-shot to continuously-refilled circular. Requesting review/testing from someone with hardware before merge:
- LED strip visual behavior across a range of configured LED counts, including counts not a multiple of 4 (e.g. 1, 5, 10) and a range spanning more than one buffer refill.
- Motor PWM/DShot output unaffected (shared IRQ handler) — ideally on an H7/AT32 board too, since the refill-callback hook was newly added to those backends in response to code review, not the originally hand-tested F4 path.
- PINIO idle-high interaction on the LED strip pin, if used.

TIM3/TIM4's CCR is a 16-bit register; the buffer only needs to hold compare values 0-3, so 32-bit elements wasted RAM without matching any hardware requirement. Halves ledStripDMABuffer from 12,460 B to 6,230 B on affected F4 targets.

Lets a circular-DMA consumer refill each half of its buffer as DMA finishes sending it, instead of pre-loading the whole transfer up front. Opt-in and null-guarded: DMA_IT_HT is only enabled, and the callback only invoked, when a consumer registers one via impl_timerPWMSetDMARefillCallback, so existing circular-DMA users (motor DShot idle-packet repeat) see no behavior change.

hsvToRgb24 reruns its full divide/multiply conversion for every LED on every strip update (up to 100Hz), even though most updates set only a handful of distinct colors across the whole strip. A 4-entry direct-mapped cache, keyed on exact HSV input equality, skips the recompute on a repeat.

ledStripDMABuffer held every WS2811_LED_STRIP_LENGTH (128) LED's worth of protocol bits for a single one-shot DMA burst, regardless of how many LEDs were actually configured (6,230 bytes, uint16_t elements). It's now a 2-half, 4-LED-per-half circular buffer (384 bytes) refilled from the DMA half/full-transfer interrupt as each group finishes transmitting, via the refill callback hook added in the timer driver. ws2811UpdateStrip() now takes the actual configured LED count and bounds the transfer to it instead of always processing the full 128 slots. WS2812's reset/latch condition is a minimum, not a maximum, low duration, so the old fixed DMA preamble and idle-tail buffer elements are gone too: starting a transfer now does a cheap timestamp check against how long the line has already been idling low (falling back to a blocking wait only if it hasn't been, e.g. right after PINIO idle-high), and stopping does a direct write to the timer's preload-buffered compare register instead of one more DMA element.

The refill-callback hook was only wired up in the F4 StdPeriph timer backend, but light_ws2811strip.c calls it unconditionally on every platform with USE_LED_STRIP — leaving H7/F7 (HAL) and AT32 targets with an undefined reference to impl_timerPWMSetDMARefillCallback at link time. Mirrors the same opt-in, null-guarded HT/TC dispatch added to the StdPeriph backend. Also has all three backends' impl_timerPWMStopDMA poll for the DMA stream's enable bit to actually clear before returning, matching the poll already used by impl_timerPWMSetDMACircular for the same hardware constraint (disabling a stream isn't instantaneous) - stop is now called from a circular-DMA refill callback, not just after a one-shot transfer's TC event, so it can run while a stream is still mid-flight.

The chunked-buffer refill state (activeLedCount, totalGroups, nextGroupToAssign, groupInHalf, lineIdleLow, lastLowAtUs) is written from both task context and the DMA refill ISR. Accesses don't actually overlap in practice, but marking them volatile documents that and matches the existing convention (TCH_t's dmaState is volatile for the same reason) instead of relying on the compiler not reordering around the assumption.

qodo-code-review · 2026-08-18T05:45:53Z

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

qodo-free-for-open-source-projects · 2026-08-18T05:48:23Z

PR Summary by Qodo

Stream WS2811 DMA data through a compact circular buffer

✨ Enhancement 🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Streams WS2811 data through a 384-byte circular DMA buffer instead of a full-strip buffer.
• Limits encoding to configured LEDs and caches repeated HSV-to-RGB conversions.
• Adds opt-in refill IRQ support and safe DMA stopping across all timer backends.

Diagram

graph TD
  L["LED Strip Driver"] -->|"starts circular DMA"| A["Timer DMA API"] -->|"configures"| B["Timer Backends"] -->|"raises HT/TC"| I["DMA IRQ"] -->|"invokes"| R["Refill Callback"] -->|"refills"| M["Circular Buffer"] -->|"writes compare values"| O["Timer CCR"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Use hardware double-buffer DMA

➕ Could let hardware switch between memory buffers without half-transfer refill indexing.
➕ May reduce application-managed circular-buffer state on supported MCUs.
➖ Not uniformly available or implemented across the F4, H7/F7, and AT32 backends.
➖ Adds platform-specific complexity while retaining the same ISR timing and refill requirements.

2. Retain one-shot DMA with sized allocations

➕ Preserves the existing one-shot timing model.
➕ Avoids shared circular-DMA IRQ changes.
➖ Requires RAM proportional to configured LED count and complicates deterministic embedded memory management.
➖ Cannot deliver the fixed 384-byte static buffer target.

Recommendation: Keep the portable two-half circular DMA design. It achieves the required fixed RAM reduction while centralizing the opt-in refill behavior in the existing timer abstraction; hardware double-buffer DMA is not a worthwhile cross-platform trade-off.

Files changed (9) +316 / -63

Enhancement (9) +316 / -63

colorconversion.cCache recent HSV-to-RGB conversions +36/-2

Cache recent HSV-to-RGB conversions

• Adds a four-entry cache keyed by HSV values and returns cached RGB results for repeated colors. This avoids repeated conversion arithmetic during common LED-strip patterns.

src/main/common/colorconversion.c

light_ws2811strip.cTransmit LED data with chunked circular DMA +135/-29

Transmit LED data with chunked circular DMA

• Replaces the full-strip DMA payload with a two-half buffer containing four LEDs per half and refills released halves from DMA IRQ context. Limits output to the configured LED count, enforces the WS2812 reset gap with timestamps, and directly controls idle-high/low compare output.

src/main/drivers/light_ws2811strip.c

light_ws2811strip.hRemove legacy DMA sizing constants +1/-6

Remove legacy DMA sizing constants
• Removes full-strip delay and buffer-size constants that no longer apply to chunked transfers. Changes the update API to accept the active LED count.
src/main/drivers/light_ws2811strip.h

timer.hDefine optional timer DMA refill callbacks +10/-0

Define optional timer DMA refill callbacks

• Adds an optional circular-DMA refill callback type and stores the callback on each timer channel. The callback communicates whether a half-transfer or full-transfer event released a buffer half.

src/main/drivers/timer.h

timer_impl.hExpose DMA refill callback registration +1/-0

Expose DMA refill callback registration
• Declares the common implementation hook used by circular-DMA consumers to register a refill callback.
src/main/drivers/timer_impl.h

timer_impl_hal.cHandle circular DMA refill IRQs on HAL targets +44/-8

Handle circular DMA refill IRQs on HAL targets

• Dispatches half-transfer and transfer-complete events to registered circular-DMA callbacks without stopping the stream. Enables those interrupts only for callback users and waits for DMA disable completion before returning.

src/main/drivers/timer_impl_hal.c

timer_impl_stdperiph.cHandle circular DMA refill IRQs on StdPeriph targets +43/-8

Handle circular DMA refill IRQs on StdPeriph targets
• Adds callback-driven half/full circular-DMA IRQ handling and conditional interrupt configuration. Polls the stream enable bit after stop requests to ensure a circular transfer has actually stopped.
src/main/drivers/timer_impl_stdperiph.c

timer_impl_stdperiph_at32.cHandle circular DMA refill IRQs on AT32 targets +44/-8

Handle circular DMA refill IRQs on AT32 targets
• Adds callback-driven circular-DMA interrupt handling and conditional half/full interrupt configuration for AT32. Waits for channel disable completion before marking DMA idle.
src/main/drivers/timer_impl_stdperiph_at32.c

ledstrip.cPass configured LED count to DMA updates +2/-2

Pass configured LED count to DMA updates

• Supplies the current configured LED count for normal updates and strip disable updates, preventing unnecessary transmission of unused LED slots.

src/main/io/ledstrip.c

qodo-free-for-open-source-projects · 2026-08-18T05:52:24Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Null ws2811TCH deref~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

ws2811SetIdleHigh() writes through timerCCR(ws2811TCH) without checking ws2811TCH/ws2811Initialised,
so a failed/absent LED-strip init can cause a hard fault when PINIO calls it. ws2811LedStripInit()
has multiple early-return paths that leave ws2811TCH NULL.

Code

src/main/drivers/light_ws2811strip.c[R295-297]

+    lineIdleLow = !high;
+    *timerCCR(ws2811TCH) = high ? 255 : 0;
+    if (!high) {

Evidence
PINIO can call ws2811SetIdleHigh() regardless of whether ws2811LedStripInit() successfully set up
ws2811TCH. The new implementation dereferences ws2811TCH without checks, and ws2811LedStripInit()
can return before initializing ws2811TCH/marking the strip initialized.
src/main/drivers/light_ws2811strip.c[293-300]
src/main/drivers/pinio.c[195-201]
src/main/drivers/light_ws2811strip.c[144-171]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ws2811SetIdleHigh()` unconditionally dereferences `ws2811TCH` via `timerCCR(ws2811TCH)`. If LED-strip initialization never ran or returned early (no suitable timer, DMA config failure, etc.), `ws2811TCH` can be NULL and this becomes a hard-fault path.
### Issue Context
`pinioSetDuty()` routes `index == 0` directly to `ws2811SetIdleHigh()`, so this can be triggered even when LED strip init did not succeed.
### Fix
Add a defensive guard at the top of `ws2811SetIdleHigh()`:
- `if (!ws2811Initialised || !ws2811TCH) return;`
Optionally also avoid touching `lineIdleLow/lastLowAtUs` when not initialized.
### Fix Focus Areas
- src/main/drivers/light_ws2811strip.c[293-300]
- src/main/drivers/pinio.c[195-201]
- src/main/drivers/light_ws2811strip.c[144-171]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Idle-high overridden at end~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

ws2811StopTransfer() always forces *timerCCR(...)=0 and lineIdleLow=true, so an earlier
ws2811SetIdleHigh(true) request (PINIO) can be lost when the DMA refill ISR stops the transfer. This
leaves the LED strip/PINIO line in the wrong electrical state until the next PINIO update.

Code

src/main/drivers/light_ws2811strip.c[R229-232]

+    timerPWMStopDMA(ws2811TCH);
+    *timerCCR(ws2811TCH) = 0;
+    lineIdleLow = true;
+    lastLowAtUs = micros();

Evidence
The DMA completion path (refill callback) calls ws2811StopTransfer(), which currently always drives
CCR to 0 and marks the line idle-low. PINIO’s interface to the LED strip pin is ws2811SetIdleHigh();
its requested state is not preserved across the stop path.
src/main/drivers/light_ws2811strip.c[225-249]
src/main/drivers/light_ws2811strip.c[293-300]
src/main/drivers/pinio.c[195-201]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
At the end of a circular DMA LED transfer, `ws2811DMARefillCallback()` calls `ws2811StopTransfer()`, which unconditionally drives the line low (`CCR=0`) and marks it idle-low. This can override a PINIO-driven idle-high request made via `ws2811SetIdleHigh(true)`.
### Issue Context
- `pinioSetDuty(index==0)` uses `ws2811SetIdleHigh(duty>0)`.
- When the last LED group completes, the DMA refill callback stops the DMA and forces low.
### Fix
Track the desired idle level separately (e.g. `static volatile bool idleHighRequested;` updated by `ws2811SetIdleHigh()`), and in `ws2811StopTransfer()` set CCR to either 0 or a constant-high value based on that desired level instead of always forcing low.
If latch/reset timing requires a low gap before returning high, enforce that before restoring high.
### Fix Focus Areas
- src/main/drivers/light_ws2811strip.c[225-233]
- src/main/drivers/light_ws2811strip.c[235-249]
- src/main/drivers/light_ws2811strip.c[293-300]
- src/main/drivers/pinio.c[195-201]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

github-actions · 2026-08-18T06:16:50Z

RAM / Flash usage vs. base branch — commit 429312c

No size baseline is available yet for this PR's base branch (first run after this feature shipped, or a new branch). This comment will show deltas once a baseline exists.

Target	Flash Δ	RAM Δ
MATEKF405	661223 B (no baseline)	131612 B (no baseline)
MATEKF722	464055 B (no baseline)	114188 B (no baseline)
MATEKF765	687119 B (no baseline)	139092 B (no baseline)
MATEKH743	717199 B (no baseline)	141292 B (no baseline)

github-actions · 2026-08-18T06:17:09Z

Test firmware build ready — commit 429312c

Download firmware for PR #11798

244 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.

The old idle-tail lived in the DMA buffer itself, so it was memory- safe by construction and got resent every transfer. Replacing it with a direct CCR write dropped both properties: - ws2811SetIdleHigh() dereferenced ws2811TCH unconditionally, so a PINIO call after a failed/absent LED strip init (ws2811TCH still NULL) would hard fault. Now guarded the same way ws2811UpdateStrip already is. - ws2811StopTransfer() unconditionally forced the line low, silently discarding a prior ws2811SetIdleHigh(true) once the in-flight transfer finished. Now tracks the requested idle level separately and restores it instead of always going low.

sensei-hacker added 6 commits August 11, 2026 22:10

sensei-hacker added this to the 10.0 milestone Aug 18, 2026

sensei-hacker added Cleanup/refactor Testing Required labels Aug 18, 2026

qodo-free-for-open-source-projects Bot reviewed Aug 18, 2026

View reviewed changes

Comment thread src/main/drivers/light_ws2811strip.c

Comment thread src/main/drivers/light_ws2811strip.c Outdated

Navigation Menu

Uh oh!

Shrink WS2811/WS2812 LED strip DMA buffer with a chunked circular refill - #11798

Shrink WS2811/WS2812 LED strip DMA buffer with a chunked circular refill#11798
sensei-hacker wants to merge 7 commits into
iNavFlight:maintenance-10.xfrom
sensei-hacker:shrink-ledstrip-dma-buffer

sensei-hacker commented Aug 18, 2026

Uh oh!

qodo-code-review Bot commented Aug 18, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 18, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

sensei-hacker commented Aug 18, 2026

Summary

Changes

RAM impact

Testing

Uh oh!

qodo-code-review Bot commented Aug 18, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-free-for-open-source-projects Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading