Shrink WS2811/WS2812 LED strip DMA buffer with a chunked circular refill - #11798
Open
sensei-hacker wants to merge 9 commits into
Open
Shrink WS2811/WS2812 LED strip DMA buffer with a chunked circular refill#11798sensei-hacker wants to merge 9 commits into
sensei-hacker wants to merge 9 commits into
Conversation
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.
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoStream WS2811 DMA data through a compact circular buffer
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
RAM / Flash usage vs. base branch — commit
|
|
Test firmware build ready — commit Download firmware for PR #11798 244 targets built. Find your board's
|
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.
impl_timerPWMConfigChannelDMA() assigned LL_DMA_MDATAALIGN_HALFWORD to PeriphOrM2MSrcDataSize in the 2-byte element case. LL_DMA_Init masks that field against DMA_SxCR_PSIZE (bits 11-12), but MDATAALIGN_HALFWORD is MSIZE_0 (bit 13), so it masked to zero and silently configured PSIZE as BYTE rather than HALFWORD. No existing caller hit this: DSHOT uses 4-byte elements and takes the case below, which already used the correct PDATAALIGN constant. Any 2-byte timer DMA would have gotten byte-wide writes to CCR.
ledStripDMABuffer had been narrowed to uint16_t on the assumption that CCR is a 16-bit register. That made the element size 2, which selected the halfword branch of impl_timerPWMConfigChannelDMA() -- the one carrying the PSIZE typo fixed in the preceding commit -- so DMA wrote CCR a byte at a time. Every bit then decoded as a 1 and the strip lit solid white on H7. Use timerDMASafeType_t, whose name states the requirement. Bounds-check nextGroupToAssign directly in the refill callback instead of inferring the end of the strip from the half that just finished; that check lags by one refill and could assign a group index past totalGroups-1. Skip the CCR write in ws2811SetIdleHigh() while a transfer is in flight, since DMA owns the register then. ws2811StopTransfer() applies the requested idle level once the transfer completes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
timerDMASafeType_t-typed (word-width) rather than narrowed to the destination register's width, since that width isn't uniform across the timer/DMA backends this driver runs on. Verified the new hook is a no-op for the only other circular-DMA consumer (motor DShot idle-packet repeat during EEPROM writes), which never registers a callback.hsvToRgb24()so repeated colors (very common — most strips use only a handful of distinct colors) skip the conversion math.impl_timerPWMStopDMAnow 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.ws2811SetIdleHigh()(the PINIO idle-level hook) now guards against a null/uninitialized timer channel and against racing an in-flight DMA transfer's direct writes to the compare register; the requested idle level is still recorded and applied once the transfer completes or init finishes.RAM impact
ledStripDMABuffer: 12,460 B (uint32_t[3115], one-shot buffer sized for a full 128-LED strip) -> 768 B (uint32_t[192], chunked circular buffer), a ~16x reduction.Testing