fix(led-strip): migrate dmaInit() to ownership-checked dmaAllocate() - #1394
Conversation
light_ws2811strip_stdperiph.c and light_ws2811strip_hal.c called dmaInit(), which silently overwrites any existing DMA owner. Replace with dmaAllocate() + dmaEnable(), matching the pattern already used by ADC and transponder (PR#1389, PR#1393). Both driver variants already gate ws2811LedStripDMAEnable() on a file-scope ws2811Initialised flag, set true only at the end of a fully successful ws2811LedStripHardwareInit(). The new early return on dmaAllocate() failure happens before that assignment, so no signature change or caller update is needed.
Local CodeRabbit review flagged: TIM_Cmd(timer, ENABLE) in the
stdperiph variant ran before the dmaAllocate() check, so a claim
failure left the timer running with the output pin driving the LED
data line. The HAL variant had the same ordering risk relative to
its GPIO/timer-init calls, though its PWM start was already gated
after allocation.
Moved the dmaAllocate() call to combine with the existing dmaRef-NULL
early-return, before any GPIO/RCC/timer/DMA-clock setup, matching BF
4.5-maintenance's stm32/light_ws2811strip_{hal,stdperiph}.c structure
exactly. A claim failure now leaves no hardware touched at all.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-05-15T14:49:12.754ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughWS2811 hardware initialization now allocates the DMA channel and stops when allocation fails. Output setup enables the allocated channel with ChangesWS2811 DMA handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized change replaces unchecked DMA initialization with ownership-checked allocation for LED-strip support in two implementations. Reported builds and unit tests pass, and no actionable merge-blocking risk remains beyond normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai, AI generated comment Requesting analysis on the DMA-claim ordering specifically (this PR is merged; asking as an independent second check, not a re-review). light_ws2811strip_stdperiph.c and light_ws2811strip_hal.c: dmaAllocate() was moved to combine with the pre-existing dmaRef == NULL early-return at the top of ws2811LedStripHardwareInit(), before any GPIO/RCC/timer/DMA-clock setup in either file. Please analyze:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
std_file="$(fd -t f '^light_ws2811strip_stdperiph\.c$' . | head -n1)"
hal_file="$(fd -t f '^light_ws2811strip_hal\.c$' . | head -n1)"
printf '%s\n' "=== Local file paths ==="
printf '%s\n' "$std_file" "$hal_file"
for file in "$std_file" "$hal_file"; do
printf '\n=== Outline: %s ===\n' "$file"
ast-grep outline "$file" --items all --match 'ws2811LedStripHardwareInit|ws2811LedStripDMAEnable|ws2811Initialised' || true
printf '\n=== Relevant definitions and state writes: %s ===\n' "$file"
rg -n -C 8 \
'ws2811LedStripHardwareInit\s*\(|ws2811LedStripDMAEnable\s*\(|\bws2811Initialised\b|dmaAllocate\s*\(|dmaEnable\s*\(' \
"$file"
done
mkdir -p /tmp/bf45
for name in light_ws2811strip_stdperiph.c light_ws2811strip_hal.c; do
gh api \
-H 'Accept: application/vnd.github.raw+json' \
"repos/betaflight/betaflight/contents/src/main/drivers/${name}?ref=4.5-maintenance" \
> "/tmp/bf45/${name}"
done
printf '\n=== Upstream function order and state writes ===\n'
for file in /tmp/bf45/*.c; do
printf '\n--- %s ---\n' "$file"
rg -n -C 8 \
'ws2811LedStripHardwareInit\s*\(|ws2811LedStripDMAEnable\s*\(|\bws2811Initialised\b|dmaAllocate\s*\(|dmaEnable\s*\(' \
"$file"
done
printf '\n=== Diff: local vs Betaflight 4.5-maintenance ===\n'
diff -u /tmp/bf45/light_ws2811strip_stdperiph.c "$std_file" || true
diff -u /tmp/bf45/light_ws2811strip_hal.c "$hal_file" || trueLength of output: 8481 🏁 Script executed: #!/bin/bash
set -euo pipefail
std_file="$(fd -t f '^light_ws2811strip_stdperiph\.c$' . | head -n1)"
hal_file="$(fd -t f '^light_ws2811strip_hal\.c$' . | head -n1)"
printf '%s\n' '=== Complete local initialization functions ==='
for file in "$std_file" "$hal_file"; do
printf '\n--- %s ---\n' "$file"
sed -n '/void ws2811LedStripHardwareInit(/,/^}/p' "$file"
sed -n '/void ws2811LedStripDMAEnable(/,/^}/p' "$file"
printf '\nState occurrences:\n'
rg -n -C 4 '\bws2811Initialised\b' "$file"
done
printf '%s\n' '\n=== Initialization callers and timer lookup definition ==='
rg -n -C 5 \
'\bws2811LedStripHardwareInit\s*\(|\btimerGetByTag\s*\(' \
src/main -g '*.[ch]' || true
printf '%s\n' '\n=== Candidate repositories and 4.5-maintenance refs ==='
for repo in emuflight/EmuFlight betaflight/betaflight; do
printf '\n--- %s branches matching maintenance ---\n' "$repo"
gh api --paginate "repos/${repo}/branches?per_page=100" \
--jq '.[] | select(.name | test("4[._-]5|maintenance"; "i")) | .name' || true
done
printf '%s\n' '\n=== Locate both upstream files for candidate refs ==='
for repo_ref in \
'emuflight/EmuFlight 4.5-maintenance' \
'betaflight/betaflight 4.5-maintenance' \
'betaflight/betaflight maintenance/4.5' \
'betaflight/betaflight 4.5.0'; do
repo="${repo_ref% *}"
ref="${repo_ref##* }"
for path in \
src/main/drivers/light_ws2811strip_stdperiph.c \
src/main/drivers/light_ws2811strip_hal.c \
src/main/drivers/stm32/light_ws2811strip_stdperiph.c \
src/main/drivers/stm32/light_ws2811strip_hal.c; do
status="$(gh api -i "repos/${repo}/contents/${path}?ref=${ref}" 2>/dev/null | head -n1 || true)"
if printf '%s' "$status" | grep -q '200'; then
printf 'FOUND repo=%s ref=%s path=%s\n' "$repo" "$ref" "$path"
fi
done
doneLength of output: 28640 🏁 Script executed: #!/bin/bash
set -euo pipefail
std_file="$(fd -t f '^light_ws2811strip_stdperiph\.c$' . | head -n1)"
hal_file="$(fd -t f '^light_ws2811strip_hal\.c$' . | head -n1)"
mkdir -p /tmp/bf45
for name in light_ws2811strip_stdperiph.c light_ws2811strip_hal.c; do
gh api \
-H 'Accept: application/vnd.github.raw+json' \
"repos/betaflight/betaflight/contents/src/main/drivers/stm32/${name}?ref=4.5-maintenance" \
> "/tmp/bf45/${name}"
done
printf '%s\n' '=== Upstream exact initialization functions ==='
for file in /tmp/bf45/*.c; do
printf '\n--- %s ---\n' "$file"
sed -n '/void ws2811LedStripHardwareInit(/,/^}/p' "$file"
sed -n '/void ws2811LedStripDMAEnable(/,/^}/p' "$file"
done
printf '%s\n' '\n=== Normalized local vs upstream function diffs ==='
python3 - "$std_file" "$hal_file" <<'PY'
import re
import sys
from pathlib import Path
def function(text, name):
start = re.search(rf'\bvoid\s+{re.escape(name)}\s*\([^)]*\)\s*\{{', text)
if not start:
raise SystemExit(f"Function {name} not found")
i = start.end()
level = 1
while level and i < len(text):
if text[i] == '{':
level += 1
elif text[i] == '}':
level -= 1
i += 1
return text[start.start():i]
def normalize(text):
text = re.sub(r'/\*.*?\*/', '', text, flags=re.S)
text = re.sub(r'//.*', '', text)
return '\n'.join(line.rstrip() for line in text.splitlines() if line.strip())
for local_path in map(Path, sys.argv[1:]):
upstream_path = Path('/tmp/bf45') / local_path.name
print(f'\n--- {local_path.name} ---')
for name in ('ws2811LedStripHardwareInit', 'ws2811LedStripDMAEnable'):
local = normalize(function(local_path.read_text(), name))
upstream = normalize(function(upstream_path.read_text(), name))
print(f'{name}: {"IDENTICAL" if local == upstream else "DIFFERENT"}')
if local != upstream:
import difflib
print('\n'.join(difflib.unified_diff(
upstream.splitlines(), local.splitlines(),
fromfile='BF-4.5', tofile='EmuFlight', lineterm=''
)))
PY
printf '%s\n' '\n=== dmaAllocate implementation and ownership behavior ==='
rg -n -C 12 '\bdmaAllocate\s*\(' src/main -g '*.[ch]'
printf '%s\n' '\n=== All LED-strip initialization call sites ==='
rg -n -C 6 '\bws2811LedStripInit\s*\(|\bws2811LedStripHardwareInit\s*\(' . \
-g '*.[ch]' -g '!**/build/**' -g '!**/dist/**'Length of output: 3679 🏁 Script executed: #!/bin/bash
set -euo pipefail
std_file="$(fd -t f '^light_ws2811strip_stdperiph\.c$' . | head -n1)"
hal_file="$(fd -t f '^light_ws2811strip_hal\.c$' . | head -n1)"
printf '%s\n' '=== Upstream symbols and DMA allocation contexts ==='
for file in /tmp/bf45/light_ws2811strip_stdperiph.c /tmp/bf45/light_ws2811strip_hal.c; do
printf '\n--- %s: outline ---\n' "$file"
ast-grep outline "$file" --items all || true
printf '\n--- %s: dmaAllocate and setup context ---\n' "$file"
rg -n -C 35 '\bdmaAllocate\s*\(|\bdmaInit\s*\(|\bws2811Initialised\b|\bIOInit\s*\(|\bRCC_ClockCmd\s*\(|__DMA1_CLK_ENABLE\s*\(|\bHAL_TIM_PWM_Init\s*\(' "$file" || true
done
printf '%s\n' '\n=== Local DMA allocation contexts ==='
for file in "$std_file" "$hal_file"; do
printf '\n--- %s ---\n' "$file"
rg -n -C 35 '\bdmaAllocate\s*\(|\bws2811Initialised\b|\bIOInit\s*\(|\bRCC_ClockCmd\s*\(|__DMA1_CLK_ENABLE\s*\(|\bHAL_TIM_PWM_Init\s*\(' "$file"
done
printf '%s\n' '\n=== dmaAllocate implementation and descriptor state ==='
rg -n -C 20 'bool\s+dmaAllocate\s*\(|dmaAllocate\s*\(' src/main/drivers -g '*.[ch]' || true
printf '%s\n' '\n=== All local LED-strip init entry-point call sites ==='
rg -n -C 8 '\bws2811LedStripInit\s*\(|\bws2811LedStripHardwareInit\s*\(' . \
-g '*.[ch]' -g '!**/build/**' -g '!**/dist/**' || true
printf '%s\n' '\n=== Full timerGetByTag implementation ==='
timer_common="$(fd -t f '^timer_common\.c$' src/main | head -n1)"
sed -n '/const timerHardware_t \*timerGetByTag(/,/^}/p' "$timer_common"Length of output: 85557
You are interacting with an AI system. |
1 participant
AI Generated pull-request
Summary
Stage 3 of 5 in IT#1363's fleet-wide
dmaInit()→dmaAllocate()migration (stage 1, ADC, merged as PR#1389; stage 2, transponder, merged as PR#1393). Convertsws2811LedStripHardwareInit()in bothlight_ws2811strip_stdperiph.candlight_ws2811strip_hal.cfrom uncheckeddmaInit()to ownership-checkeddmaAllocate()+dmaEnable().Why this is safe
ws2811LedStripHardwareInit()isvoidin EF (BF's equivalent returnsbool, checked by its one caller) — but EF already gates the DMA-enable path on a file-scopews2811Initialisedflag, settrueonly at the end of a fully successful init and checked byws2811LedStripDMAEnable()before it touches any DMA register. An early return on allocation failure happens before that assignment, so the existing flag closes the same reachability gap BF closes via itsboolreturn — no signature change or caller update needed.dmaRef == NULLearly-return before this change; thedmaAllocate()check was combined into that same early-return, matching BF 4.5-maintenance'sstm32/light_ws2811strip_{hal,stdperiph}.cstructure exactly (allocate check first, before any GPIO/RCC/timer/DMA-clock setup).dmaInit()never fails (unconditional overwrite), so this claim cannot currently collide with anything on any existing target — boot-time, single call site per variant, defense-in-depth/BF-parity change, not a fix for an active bug.Review history
Local
coderabbit review --agent(round 1) found one real issue: in the stdperiph variant,TIM_Cmd(timer, ENABLE)ran before thedmaAllocate()check, so a claim failure would leave the timer running with the output pin driving the LED data line. Fixed indb2c44d0cby moving thedmaAllocate()call to combine with the existingdmaRef == NULLearly-return, before any hardware setup, in both variants (matching BF's ordering exactly). Localcoderabbit review --agent(round 2) and an opencode second-opinion pass both came back clean — opencode additionally confirmed no DMA register access occurs between the allocation check anddmaEnable()in either variant, and that the change's only owner-leak-on-later-failure characteristic is a pre-existing pattern (nodmaRelease()API exists in EF or BF), not introduced here.Verification
USE_LED_STRIPvia thecommon_fc_pre.hfleet default), plus CRAZYBEEF4FS (F4 stdperiph), MATEKF722HD (F7 HAL), SPRACINGH7EF (H7 HAL, the only H7 target withUSE_LED_STRIPenabled), plus SITL.make test— 33/33 test binaries pass (incl.test_ledstrip_unittest,test_ws2811_unittest), unchanged from the PR#1393 baseline.dma/resourceCLI on real hardware without a strip attached, but only after enablingfeature LED_STRIP+ save + reboot on the target — compilingUSE_LED_STRIPin alone does not trigger the DMA-claimingws2811LedStripHardwareInit()call (fc_init.conly calls it vialedStripEnable(), gated on the feature flag). Not yet performed — build-only this session.Related
Summary by CodeRabbit