fix(adc): migrate dmaInit() to ownership-checked dmaAllocate() - #1389
Conversation
adcInit() unconditionally overwrote DMA stream ownership via dmaInit(), unlike bus_spi.c and the UART drivers, which already use dmaAllocate()'s ownership check. Matches BF 4.5-maintenance's adc_stm32*xx.c, which performs the identical dmaAllocate()+dmaEnable() early-return sequence. Part of emuflight#1363 (stage 1 of 5).
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ 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 (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-05-15T14:49:12.754ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughADC DMA initialization for STM32F4, STM32F7, and STM32H7 now uses ChangesADC DMA ownership
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The ADC initialization paths now use ownership-checked DMA allocation while preserving DMA enablement and stream selection; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review Small, focused diff: 3 files (adc_stm32f4xx.c, adc_stm32f7xx.c, adc_stm32h7xx.c), 5 lines
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
git diff --unified=80 HEAD^ HEAD -- \
src/main/drivers/adc_stm32f4xx.c \
src/main/drivers/adc_stm32f7xx.c \
src/main/drivers/adc_stm32h7xx.c
echo '=== Relevant file map ==='
fd -a '^(adc_stm32(f4|f7|h7)xx|bus_spi|dma_stm32.*)\.c$|^adc\.h$|^dma\.h$' src/main 2>/dev/null || true
echo '=== Symbol outlines ==='
ast-grep outline src/main/drivers/adc_stm32f4xx.c --items all --match 'adcInit|dmaAllocate|dmaEnable' 2>/dev/null || true
ast-grep outline src/main/drivers/adc_stm32f7xx.c --items all --match 'adcInit|dmaAllocate|dmaEnable' 2>/dev/null || true
ast-grep outline src/main/drivers/adc_stm32h7xx.c --items all --match 'adcInit|dmaAllocate|dmaEnable' 2>/dev/null || true
ast-grep outline src/main/drivers/bus_spi.c --items all --match 'spiInitBusDMA|dmaAllocate|dmaEnable' 2>/dev/null || true
echo '=== ADC implementations and SPI DMA paths ==='
rg -n -C 35 --type c \
'^\s*(void|bool)\s+adcInit\s*\(|\bdmaAllocate\s*\(|\bdmaEnable\s*\(|\bdmaInit\s*\(' \
src/main/drivers/adc_stm32f4xx.c \
src/main/drivers/adc_stm32f7xx.c \
src/main/drivers/adc_stm32h7xx.c \
src/main/drivers/bus_spi.c
echo '=== DMA API declarations and implementations ==='
rg -n -C 12 --type c --type h \
'^\s*(bool|void)\s+dma(Allocate|Enable|Init)\s*\(|\bdma(Allocate|Enable|Init)\s*\(' \
src/main
echo '=== adcInit declarations and call sites ==='
rg -n -C 8 --glob '*.[ch]' \
'^\s*(extern\s+)?void\s+adcInit\s*\(|\badcInit\s*\(' \
src/mainLength of output: 134694 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== H7 ADC helper initialization ==='
sed -n '80,190p' src/main/drivers/adc_stm32h7xx.c
sed -n '235,355p' src/main/drivers/adc_stm32h7xx.c
echo '=== Flight-controller initialization order ==='
sed -n '380,455p' src/main/fc/fc_init.c
sed -n '610,680p' src/main/fc/fc_init.c
rg -n -C 10 --glob '*.[ch]' \
'\bspiInitBusDMA\s*\(|USE_SPI_DMA_ENABLE_LATE|`#define`\s+USE_ADC_INTERNAL' \
src/main
echo '=== H7 target configuration and ADC DMA resources ==='
rg -n -C 5 --glob '*.[ch]' \
'`#define`\s+USE_ADC_INTERNAL|ADC_INSTANCE|dmaResource|DMA.*ADC|ADC.*DMA' \
src/main/target src/main/drivers/adc_stm32h7xx.c
echo '=== ADC internal API consumers ==='
rg -n -C 5 --glob '*.[ch]' \
'\badcInternal(IsBusy|StartConversion|ReadVrefint|ReadTempsensor)\s*\(' \
src/mainLength of output: 173674
P2 — Preserve H7 internal ADC initialization when regular ADC DMA allocation fails In
If the regular ADC DMA stream is already owned, the code now disables both regular ADC sampling and internal ADC initialization. The F4 and F7 paths do not have this behavior because they initialize internal ADC support before their DMA allocation checks. Keep the failed allocation local to regular ADC DMA setup. Then continue to the existing The remaining requested checks are correct:
🧠 Learnings used
|
adcInit()'s dmaAllocate() failure returned from the whole function, skipping the later adcInitInternalInjected() call (VREFINT/temp sensor). F4/F7 don't have this coupling since they init internal ADC before their DMA claim; H7's own ordering has it after, so a regular-ADC DMA conflict would have also disabled internal ADC monitoring. Scoped the DMA setup into the successful-allocation branch instead of returning early, so a claim failure only drops regular ADC sampling, matching F4/F7. Found by CodeRabbit (github.com/emuflight/pull/1389).
|
@coderabbitai, AI generated comment Fixed in 27305cf, addressing #1389 (comment) (P2 — H7 internal ADC init coupling).
Please verify. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit ==='
git show --no-ext-diff --format=fuller --stat 27305cfcf
echo '=== Commit diff ==='
git show --no-ext-diff --format= --unified=50 27305cfcf -- src/main/drivers/adc_stm32h7xx.c
echo '=== adcInit outline ==='
ast-grep outline src/main/drivers/adc_stm32h7xx.c --items all --match 'adcInit|adcInitInternalInjected'
echo '=== adcInit implementation and internal-init call ==='
rg -n -C 70 --type c \
'^\s*void\s+adcInit\s*\(|\bdmaAllocate\s*\(|\bHAL_DMA_Init\s*\(|\bHAL_ADC_Start_DMA\s*\(|\badcInitInternalInjected\s*\(' \
src/main/drivers/adc_stm32h7xx.cLength of output: 19850
In The existing The P2 finding is addressed. 🧠 Learnings usedIf 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, AI generated comment Requesting a fresh full-diff analyze on the current HEAD (both commits: the
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current HEAD and requested commits ==='
git status --short
git log --oneline -8
git merge-base --is-ancestor 39676e255 HEAD && echo '39676e255 is an ancestor of HEAD'
git merge-base --is-ancestor 27305cfcf HEAD && echo '27305cfcf is an ancestor of HEAD'
BASE="$(git rev-parse 39676e255^)"
echo "BASE=$BASE"
echo
echo '=== Full requested diff ==='
git diff --no-ext-diff --unified=80 "$BASE"..HEAD -- \
src/main/drivers/adc_stm32f4xx.c \
src/main/drivers/adc_stm32f7xx.c \
src/main/drivers/adc_stm32h7xx.c
echo '=== Changed lines, including the H7 follow-up ==='
git show --no-ext-diff --format='commit %H%n%s' --unified=50 39676e255 -- \
src/main/drivers/adc_stm32f4xx.c \
src/main/drivers/adc_stm32f7xx.c \
src/main/drivers/adc_stm32h7xx.c
git show --no-ext-diff --format='commit %H%n%s' --unified=80 27305cfcf -- \
src/main/drivers/adc_stm32h7xx.c
echo '=== ADC implementation outlines ==='
ast-grep outline src/main/drivers/adc_stm32f4xx.c --items all --match 'adcInit|adcInitInternalInjected'
ast-grep outline src/main/drivers/adc_stm32f7xx.c --items all --match 'adcInit|adcInitInternalInjected'
ast-grep outline src/main/drivers/adc_stm32h7xx.c --items all --match 'adcInit|adcInitInternalInjected'
echo '=== Current ADC initialization control flow ==='
rg -n -C 85 --type c \
'^\s*void\s+adcInit\s*\(|\badcInitInternalInjected\s*\(|\bdmaAllocate\s*\(|\bdmaEnable\s*\(|\bdmaInit\s*\(|\bHAL_DMA_Init\s*\(|\bHAL_ADC_Start_DMA\s*\(|\breturn\s*;' \
src/main/drivers/adc_stm32f4xx.c \
src/main/drivers/adc_stm32f7xx.c \
src/main/drivers/adc_stm32h7xx.c
echo '=== SPI reference sequence and DMA API contracts ==='
fd -a '^(bus_spi|dma_stm32.*)\.(c|h)$' src/main | sort
rg -n -C 28 --glob '*.[ch]' \
'\bdmaAllocate\s*\(|\bdmaEnable\s*\(|\bdmaInit\s*\(|^\s*(bool|void)\s+dma(Allocate|Enable|Init)\s*\(' \
src/main/drivers/bus_spi.c src/main/drivers
echo '=== ADC declaration and call sites ==='
rg -n -C 12 --glob '*.[ch]' \
'^\s*(extern\s+)?void\s+adcInit\s*\(|\badcInit\s*\(' \
src/mainLength of output: 243070
The 🧠 Learnings usedYou are interacting with an AI system. |
#1393) * fix(transponder): migrate dmaInit() to ownership-checked dmaAllocate() transponderIrHardwareInit() (stdperiph and HAL variants) claimed its DMA stream via unchecked dmaInit(), which unconditionally overwrites dmaDescriptors[index].owner even if another peripheral already holds the stream. Converted to dmaAllocate() + dmaEnable(), matching BF 4.5-maintenance's transponder_ir_io_stdperiph.c/transponder_ir_io_hal.c exactly. transponderIrHardwareInit() is void in both EF and BF, so this is a one-line-shape swap with no signature or caller changes, same pattern as PR #1389 (ADC, stage 1 of IT#1363's 5-stage migration). Verified no coupling to any later logic in the function: unlike the H7 ADC case fixed in PR #1389, the DMA setup here isn't split from a separate subsystem call — everything after the claim is the same timer/DMA config, so the early return on allocation failure carries no ordering risk. Stage 2 of 5 (ADC done, transponder this PR, then LED strip, motors/DSHOT, SD card). * fix(transponder): assign dmaRef before dmaAllocate() check in stdperiph transponderIrHardwareInit() (STM32F4 stdperiph variant) assigned the file-scope static dmaRef after the new dmaAllocate() early-return, so a claim failure left dmaRef at its NULL initializer. A later transponderIrTransmit()/transponderIrDMAEnable() call would then pass that NULL dmaRef into DMA_SetCurrDataCounter()/DMA_Cmd(), an invalid DMA register write. This failure path could not occur before dmaInit() was replaced with dmaAllocate(), since dmaInit() never failed. BF 4.5-maintenance avoids this by assigning dmaRef before its own dmaAllocate() check; moved the assignment to match. transponderIrInit() still returns true regardless of hardware-init success (matches BF's identical gap in transponder_ir_io_stdperiph.c/transponder_ir_io_hal.c) — out of this PR's scope, since it isn't specific to this migration. * fix(transponder): propagate init success/failure from stdperiph driver transponderIrInit() (STM32F4 stdperiph) always returned true regardless of transponderIrHardwareInit()'s outcome. Before this migration, that was harmless: dmaInit() never failed, so hardware init always completed. Since dmaAllocate() can fail, a claim conflict now leaves the transponder half-configured while transponderIrInit() still reports success -- the io/transponder_ir.c call chain (transponderInit() -> transponderUpdate()/ transponderTransmitOnce()) gates on that return value, so a false success lets transponderIrTransmit() run DMA_SetCurrDataCounter()/DMA_Cmd() on a stream OWNER_TRANSPONDER never actually claimed. Added a transponderInitialised flag matching the pattern already used in transponder_ir_io_hal.c (this codebase's own HAL variant, ported from BF): reset false at the start of transponderIrHardwareInit(), set true only after every step succeeds, returned from transponderIrInit(), and checked in transponderIrDMAEnable()/transponderIrDisable() as defense in depth.
1 participant
AI Generated [pull-request]
Summary
adcInit()(F4/F7/H7) claims its DMA stream via uncheckeddmaInit(), whichunconditionally overwrites
dmaDescriptors[index].ownerregardless of thecurrent owner.
bus_spi.cand the UART drivers already use theownership-checked
dmaAllocate()for this. This PR converts all 3 ADC filesto the same pattern, matching Betaflight 4.5-maintenance's
adc_stm32*xx.cexactly:
if (!dmaAllocate(...)) { return; }followed bydmaEnable(...).Stage 1 of 5 for #1363 (fleet-wide
dmaInit()->dmaAllocate()migration).Remaining stages (transponder, LED strip, motors/DSHOT, SD card) tracked as
separate follow-up PRs against the same issue.
Why this is safe
adcInit()'s DMA claim is boot-time, single call site, always executedbefore
spiInitBusDMA()infc_init.c— no other code re-invokes it atruntime, so
dmaAllocate()cannot structurally fail on any current target.This mirrors the same collision-free argument already established for the
other 12 remaining
dmaInit()sites in Fleet-wide dmaInit() -> dmaAllocate() migration for the 13 remaining call sites (finish BF parity) #1363's investigation.adcInit()isvoidin both EF and BF, so the early-return-on-failurepattern needed zero signature or caller changes anywhere.
dmaEnable()is required alongsidedmaAllocate()—dmaAllocate()onlysets ownership bookkeeping, it never enables the DMA controller's
peripheral clock (found the hard way in PR fix: check DMA stream ownership before UART reopen claims it #1356, same fix applied here
from the start).
Verification
aircraft targets — plus TUNERCF405, SKYSTARSF405AIO, PYRODRONEF7,
FOXEERF405, APEXF7, TMOTORF7, SITL. 10/10 succeeded, no warnings.
dmaCLI output on all 3real aircraft matches each target's own static DMA-stream macro exactly
(HELIOSPRING
DMA2 Stream 4, FOXEERF722V4DMA2 Stream 0on ADC3,STELLARH7DEV
DMA2 Stream 1) — no regression, since the migration onlychanges whether the claim is checked, never which stream gets picked.
FOXEERF722V4 also showed a correct 22.9V battery reading, confirming the
DMA transfer itself is functioning end-to-end, not just the ownership
claim. Clean boot, no fault, on all 3 boards.
Related
dmaAllocate()+dmaEnable()patternthis PR reuses
Summary by CodeRabbit