fix(motors): migrate dmaInit() to ownership-checked dmaAllocate() - #1397
fix(motors): migrate dmaInit() to ownership-checked dmaAllocate()#1397nerdCopter wants to merge 6 commits into
Conversation
pwm_output_dshot.c (F4) and pwm_output_dshot_hal.c (F7/H7) called dmaInit(), which silently overwrites any existing DMA owner. Replace with dmaAllocate() + dmaEnable(), matching the pattern already used by ADC, transponder, and LED strip. Burst DShot (USE_DSHOT_DMAR, active by default fleet-wide) shares one TIM_UP DMA stream across every motor channel on the same timer. configureTimer is true for every channel of a shared timer processed consecutively, not just the first, so a naive dmaAllocate() swap would make the 2nd+ channel's claim on an already-owned stream fail outright -- a flight-critical regression on any board with multiple motors per timer (confirmed live on HELIOSPRING: all 4 motors on TIM8). Added dshotDmaClaim(), matching the existing uartDmaClaim()-style bypass in serial_uart_stm32f4xx.c: a stream already held by the same owner+resourceIndex is treated as an already-successful claim instead of a conflict. pwm_output_dshot_hal.c had a separate latent issue exposed by this migration: motors on a shared timer processed out of consecutive order set motor->configured = true unconditionally, without verifying the shared timer's DMA claim actually succeeded. Moved the claim check ahead of that shortcut so it's verified for every motor, not assumed from the first one's outcome.
Local CodeRabbit review flagged: LL_EX_DMA_DeInit()/DMA_DeInit() (resets the DMA stream's registers) ran before the dmaAllocate() check in the OWNER_MOTOR path, and pwm_output_dshot.c had the same ordering issue in the OWNER_TIMUP burst path too. A failed claim on a stream already held by an unrelated owner would deinit it out from under that owner instead of leaving it untouched. Moved both claim checks (dshotDmaClaim() for OWNER_TIMUP, dmaAllocate() for OWNER_MOTOR) to before any DMA register access in both files, matching the ordering already established for LED strip and transponder.
|
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 provides up to 4 included reviews per 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)
📝 WalkthroughWalkthroughDShot DMA setup now supports repeated claims of matching resources. It claims and enables DMA resources before timer configuration for burst and regular motor modes, and removes duplicate ownership initialization. ChangesDShot DMA ownership
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This change replaces unchecked DMA initialization with ownership-checked allocation while preserving shared-timer motor claims; the documented fixes and verification leave no actionable merge-blocking risk at the current head. Sequence Diagram(s)sequenceDiagram
participant DShotSetup
participant dshotDmaClaim
participant DMAResource
participant TimerSetup
DShotSetup->>dshotDmaClaim: Claim burst or motor DMA resource
dshotDmaClaim->>DMAResource: Reuse matching claim or allocate resource
DShotSetup->>DMAResource: Enable resource
DShotSetup->>TimerSetup: Configure timer and DMA
Possibly related PRs
🚥 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, AI generated comment Requesting analysis on the shared-timer DMA claim logic specifically, not a full review. pwm_output_dshot.c and pwm_output_dshot_hal.c: burst DShot (USE_DSHOT_DMAR) shares one DMA stream (OWNER_TIMUP, keyed by timerGetTIMNumber()) across every motor channel on the same physical timer. configureTimer is true for every consecutive channel of a shared timer, not just the first (dmaMotorTimerCount only advances when a genuinely new timer is registered in getTimerIndex()). dshotDmaClaim() (new, both files) is meant to make this safe: a stream already owned by the same owner+resourceIndex is treated as an already-successful claim rather than a conflict, matching the existing uartDmaClaim() pattern in serial_uart_stm32f4xx.c. Please analyze:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Locate relevant files ==='
fd -t f -a '^(pwm_output_dshot(_hal)?|serial_uart_stm32f4xx)\.c$' . || true
printf '%s\n' '=== Symbol locations and control-flow context ==='
rg -n -C 8 \
-e '\bdshotDmaClaim\s*\(' \
-e '\bpwmDshotMotorHardwareConfig\s*\(' \
-e '\bgetTimerIndex\s*\(' \
-e '\bmotor->configured\b' \
-e '\bdmaAllocate\s*\(' \
-e '\bdmaEnable\s*\(' \
-e '\b(?:LL_EX_DMA_DeInit|DMA_DeInit|dmaInit)\s*\(' \
-e '\buartDmaClaim\s*\(' \
--glob 'pwm_output_dshot.c' \
--glob 'pwm_output_dshot_hal.c' \
--glob 'serial_uart_stm32f4xx.c' \
.
printf '%s\n' '=== AST outlines ==='
while IFS= read -r f; do
printf '\n--- %s ---\n' "$f"
ast-grep outline "$f" --items all --type function --view expanded || true
done < <(fd -t f -a '^(pwm_output_dshot(_hal)?|serial_uart_stm32f4xx)\.c$' .)Length of output: 28070 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== pwm_output_dshot.c: configuration function ==='
sed -n '141,273p' src/main/drivers/pwm_output_dshot.c | nl -ba -v141
printf '%s\n' '=== pwm_output_dshot_hal.c: configuration function ==='
sed -n '140,284p' src/main/drivers/pwm_output_dshot_hal.c | nl -ba -v140
printf '%s\n' '=== DMA ownership API declarations and definitions ==='
rg -n -C 12 \
-e '^\s*(?:bool|void|resourceOwner_e|uint8_t)\s+dma(?:Allocate|Enable|GetOwner|GetResourceIndex)\s*\(' \
-e '^\s*(?:bool|void|resourceOwner_e|uint8_t)\s+dma(?:Allocate|Enable|GetOwner|GetResourceIndex)\b' \
src
printf '%s\n' '=== All direct DMA operations in the two configuration functions ==='
rg -n -C 3 \
-e '\b(?:DMA_|LL_DMA_|LL_EX_DMA_|dma[A-Z][A-Za-z0-9_]*)' \
src/main/drivers/pwm_output_dshot.c \
src/main/drivers/pwm_output_dshot_hal.cLength of output: 1253 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered_range() {
local file="$1"
local start="$2"
local end="$3"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%d\t%s\n", NR, $0 }' "$file"
}
printf '%s\n' '=== pwm_output_dshot.c: configuration function ==='
numbered_range src/main/drivers/pwm_output_dshot.c 141 273
printf '%s\n' '=== pwm_output_dshot_hal.c: configuration function ==='
numbered_range src/main/drivers/pwm_output_dshot_hal.c 140 284
printf '%s\n' '=== Ownership API implementation candidates ==='
rg -n -C 10 \
-e '\bdmaAllocate\s*\([^;]*\)\s*\{' \
-e '\bdmaEnable\s*\([^;]*\)\s*\{' \
-e '\bdmaGetOwner\s*\([^;]*\)\s*\{' \
-e '\bdmaGetResourceIndex\s*\([^;]*\)\s*\{' \
src/main
printf '%s\n' '=== Claim helper and timer-index definitions ==='
for f in src/main/drivers/pwm_output_dshot.c src/main/drivers/pwm_output_dshot_hal.c; do
printf '\n--- %s ---\n' "$f"
numbered_range "$f" 45 70
doneLength of output: 54473
1. Shared-timer processing order
For each burst-mode call, both files use: dshotDmaClaim(
timerHardware->dmaTimUPIrqHandler,
OWNER_TIMUP,
timerGetTIMNumber(timerHardware->tim))The bypass requires both of these values to match:
This is independent of For a non-consecutive order such as
The same result applies to consecutive channels. Therefore, the bypass is not limited to the common consecutive case. 2.
|
GitHub CodeRabbit analysis on PR#1397 found: GPIO AF config and full timer setup (clock enable, counter/prescaler/period, output compare, channel enable) still ran before the DMA ownership check in both files. A claim failure returned with motor->configured unset and the DMA stream untouched, but had already reconfigured that motor channel's GPIO pin and timer state. Moved both claim checks (dshotDmaClaim() for OWNER_TIMUP, dmaAllocate() for OWNER_MOTOR) to immediately after the existing dmaRef == NULL check, before any GPIO or timer register access -- matching BF 4.5-maintenance's actual structure, where the equivalent claim happens before timerHardware/motor state is even read, let alone GPIO/timer configured.
|
@coderabbitai, AI generated comment Addressed finding #3 (GPIO and timer access before the DMA claim) from the analysis in #1397 (comment). Fixed in commit Findings #1 and #2 (shared-timer claim-order coverage, motor->configured reachability) required no code change -- already correct as analyzed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Commit diff for the two affected files ==='
git show --format=fuller --find-renames -- \
82d475ca6 \
src/main/drivers/pwm_output_dshot.c \
src/main/drivers/pwm_output_dshot_hal.c
printf '%s\n' '=== Function outlines ==='
ast-grep outline src/main/drivers/pwm_output_dshot.c \
--items all --type function --match 'pwmDshotMotorHardwareConfig|dshotDmaClaim' --view expanded
ast-grep outline src/main/drivers/pwm_output_dshot_hal.c \
--items all --type function --match 'pwmDshotMotorHardwareConfig|dshotDmaClaim' --view expanded
printf '%s\n' '=== Ordering-sensitive operations ==='
rg -n -C 5 \
-e '\bdmaRef\s*==\s*NULL\b' \
-e '\bdshotDmaClaim\s*\(' \
-e '\bdmaAllocate\s*\(' \
-e '\bIOConfigGPIOAF\s*\(' \
-e '\b(?:DMA_DeInit|LL_EX_DMA_DeInit|DMA_Init|LL_EX_DMA_Init|DMA_Cmd|dmaEnable)\s*\(' \
src/main/drivers/pwm_output_dshot.c \
src/main/drivers/pwm_output_dshot_hal.cLength of output: 21948
Both implementations now perform the applicable ownership check immediately after the
If This resolves finding 3. Findings 1 and 2 remain correct without changes. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
AI Generated pull-request
Summary
Stage 4 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; stage 3, LED strip, merged as PR#1394). ConvertspwmDshotMotorHardwareConfig()in bothpwm_output_dshot.c(F4) andpwm_output_dshot_hal.c(F7/H7) from uncheckeddmaInit()to ownership-checkeddmaAllocate()+dmaEnable().Why this needed more than a mechanical swap
Burst DShot (
USE_DSHOT_DMAR, active by default fleet-wide, confirmed live on HELIOSPRING: all 4 motors share TIM8) has multiple motor channels legitimately sharing one DMA stream (OWNER_TIMUP, keyed by timer number, not per-motor).configureTimer— the existing flag gating redundant per-timer setup — is true for every channel of a timer processed consecutively, not just the first (confirmed by tracinggetTimerIndex():dmaMotorTimerCountonly advances when a genuinely new timer is registered). A mechanicalif (!dmaAllocate(...)) return;swap would make the 2nd, 3rd, and 4th motor's claim on an already-owned stream fail outright — a flight-critical regression on any board with more than one motor per timer, not a defense-in-depth no-op like stages 1-3.Added
dshotDmaClaim(), matching an existing pattern already in this codebase (uartDmaClaim()inserial_uart_stm32f4xx.c, added for UART reconnect-safety): a stream already held by the same owner+resourceIndex is treated as an already-successful claim instead of a conflict, using the existingdmaGetOwner()/dmaGetResourceIndex()accessors (no new DMA API needed).Review history
Local
coderabbit review --agent(round 1) found a real issue:LL_EX_DMA_DeInit()/DMA_DeInit()(resets the DMA stream's registers) ran before the ownership-check in both theOWNER_MOTORpath (both files) and theOWNER_TIMUPburst path (pwm_output_dshot.conly). A failed claim on a stream already owned by something else would have deinitialized it out from under that owner instead of leaving it untouched. Fixed in7504035e4by moving both claim checks ahead of any DMA register access, matching the ordering already established for LED strip and transponder. Round 2: 0 findings. opencode second-opinion pass could not complete — 4 different models failed with different errors (timeout, auth, server error) in the same session, consistent with an infra-side outage rather than a model-selection issue; not retried further.GitHub CodeRabbit analysis (requested independently of the local review above) found a further real issue: GPIO AF configuration and full timer setup (clock enable, prescaler/period, output compare, channel enable) still ran before the DMA ownership check in both files. A claim failure returned with
motor->configuredunset and the DMA stream untouched, but had already reconfigured that motor channel's GPIO pin and timer state. Fixed in82d475ca6by moving both claim checks to immediately after the existingdmaRef == NULLcheck, before any GPIO or timer register access — matching BF 4.5-maintenance's actual structure, where the equivalent claim happens beforetimerHardware/motorstate is even read. Re-verified: build 7/7, host tests 43/43, localcoderabbit review --agent0 findings.BF-master verdict check
Per the corrected
/bf-gapsprocess (was diffing against BF master, now uses 4.5-maintenance as reference with master checked as fallback for bugfixes/architecture direction):git log 4.5-maintenance..masterforpwm_output_dshot.c/_hal.cshows the shared-timer claim-bypass algorithm (dmaGetOwner()+dmaIsConfigured) is unchanged on master — only retyped by an unrelated opaque-resource refactor (PR#14990). Verdict:not-comparable(type-system divergence only, no gap to adopt or avoid). Full inventory:fix/dmainit-dshot-migration/dmainit-dshot-migration-bf-gaps.md; verdict logged inMIGRATION.md§ BF-branch register.Verification
make test— 43/43 test binaries pass, unchanged from PR#1394's baseline. No test binary directly exercisespwm_output_dshot*.c(same DMA/LL host-test limitation as prior stages).TIM8CH1-4, all 4 motors, processed consecutively) to confirmdshotDmaClaim()'s bypass fires correctly for motors 2-4 and the real allocation only happens once, for motor 1.Related
Summary by CodeRabbit