refactor(imuf9001): migrate gyro driver from USE_DMA_SPI_DEVICE to spiSequence/imufIntCallback#1283
Draft
nerdCopter wants to merge 9 commits into
Draft
Conversation
Replace the legacy USE_DMA_SPI_DEVICE code path with the general spiSequence/busSegment_t infrastructure introduced in the Stage M SPI refactor (PRs emuflight#1121, emuflight#1128–emuflight#1131). Key changes: - accgyro_mpu.c: new #ifdef USE_GYRO_IMUF9001 block with imufTxBuf[58]/imufRxBuf[58] (FAST_RAM_ZERO_INIT), crcErrorCount, imufIntCallback (DMA completion: CRC-validate, copy gyroADCf/ADCRaw), imufPrepareDmaRead (build TX command before each EXTI-triggered read), mpuImufSetupDma (wire gyro->segments at init time). - mpuIntExtiHandler: IMUF path now calls imufPrepareDmaRead + spiSequence instead of the legacy gyroDmaSpiStartRead; other SPI gyros keep the existing GYRO_EXTI_INT_DMA spiSequence call unchanged. - accgyro_imuf9001.c: add imufSpiGyroRead (trivial readFn satisfying the gyroUpdateSensor call now unconditionally present), wire readFn and call mpuImufSetupDma from imufSpiGyroInit. - bus_spi.c: remove USE_DMA_SPI_DEVICE fallback in spiSequence() that routed IMUF's SPI bus through dmaSpiTransmitReceive; IMUF now uses the standard non-blocking segment queue path. - gyro.c: remove #ifndef USE_DMA_SPI_DEVICE guard around readFn call in gyroUpdateSensor; remove gyroDmaSpiFinishRead/gyroDmaSpiStartRead wrappers. - nvic.h: NVIC_PRIO_MPU_INT_EXTI unconditionally NVIC_BUILD_PRIORITY(0x0f,0x0f) (matches BF 4.5-m; was elevated when USE_DMA_SPI_DEVICE was defined). - dma_stm32f4xx/f7xx: restore DMA2_ST2/ST3 IRQ handlers (previously excluded to avoid conflicting with dma_spi's raw IRQ handler). - fc_tasks: GYROPID task reverts to TASK_PRIORITY_REALTIME; timing is now driven by EXTI→spiSequence→imufIntCallback. - crcErrorCount moved from dma_spi.c/dma_spi_hal.c to accgyro_mpu.c under USE_GYRO_IMUF9001; exported via accgyro_mpu.h and accgyro_imuf9001.h for cli.c visibility. Closes emuflight#1143 (partial — commit 2 removes the legacy driver files). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…defines Delete the legacy dma_spi driver layer now that all IMUF9001 targets have been migrated to the general spiSequence infrastructure: Deleted files: - drivers/dma_spi.c — F4 StdPeriph custom DMA driver (HELIOSPRING/MODE2FLUX) - drivers/dma_spi.h — shared header (dmaTxBuffer, dmaRxBuffer, dmaSpiReadStatus) - drivers/dma_spi_hal.c — F7 HAL custom DMA driver (STRIXF10/F722) - drivers/dma_spi_hal.h — HAL variant header Target changes (HELIOSPRING, MODE2FLUX, STRIXF10): - Remove USE_DMA_SPI_DEVICE and all DMA_SPI_* pin/stream/channel/flag defines from target.h (45–47 lines per target). - Remove dma_spi.c / dma_spi_hal.c from TARGET_SRC in target.mk. No functional change — commit 1 already switched all call sites to the spiSequence path. This commit makes the codebase consistent by removing the now-unreachable dead driver files and their build hooks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two hardware-verified fixes on HELIOSPRING: 1. SPI mode mismatch: the old USE_DMA_SPI_DEVICE spiSequence fallback bypassed spiSequenceStart entirely and called dmaSpiTransmitReceive on SPI1 hardware that dmaSpiInit had set to Mode 0 (CPOL_Low/CPHA_1Edge, Prescaler_4 = 21 MHz). The new polling path uses spiSequenceStart which applies the per-device leadingEdge setting — which defaulted to false (Mode 3) since no explicit spiSetClkPhasePolarity call existed for IMUF. Fix: set leadingEdge=true and divisor=4 in imuf9001SpiDetect to match original dmaSpiInit settings. 2. CRC gate removed: imufIntCallback was gating all data updates on CRC match. The original mpuGyroDmaSpiReadFinish had no CRC gate — it always copied data. Restore that behavior: always copy, tally errors diagnostically only. Both bugs together caused: 50+ second init (all 10 SETUP attempts timing out), setArmingDisabled(NO_GYRO), zero gyro/accel movement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… header imufSpiGyroRead previously returned true unconditionally, allowing GYROPID to run every targetLooptime regardless of whether imufIntCallback had deposited a fresh IMUF sample. This was a regression from the old TASK_PRIORITY_TRIGGER + isDmaSpiDataReady path, which only allowed GYROPID to execute when the DMA transfer completion ISR set dmaSpiDeviceDataReady. With the unconditional-true readFn: - At gyro_sync_denom=1 (32 kHz targetLooptime) and imuf_rate=IMUF_16000, every other GYROPID cycle processed stale data. Filters saw duplicate input samples; the PID integrator accumulated at 2x the sensor rate. - The 32 kHz loop capability of HELIOSPRING/STRIXF10 was effectively halved in data quality, even though the task fired at the correct period. Fix: return gyro->dataReady from imufSpiGyroRead. imufIntCallback (called from the EXTI ISR's synchronous SPI completion) sets dataReady = true after each fresh sample; gyroUpdateSensor clears it after consuming the sample. GYROPID now returns early when no new sample is available, exactly matching the old isDmaSpiDataReady gate. The loop naturally runs at the actual IMUF output rate (16 kHz default, 32 kHz when imuf_rate = IMUF_32000 with gyro_sync_denom = 1). Also fix a pre-existing ODR violation: accgyro_imuf9001.h declared isImufCalibrating without extern, creating a tentative definition in every translation unit that included the header. Changed to extern; the single definition remains in accgyro_imuf9001.c. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
At 16 kHz IMUF rate the EXTI ISR was firing every 62.5 µs and executing a
full 32-byte synchronous SPI polling transfer (~15-20 µs at 21 MHz). That
burned ~32% of the F405 CPU in interrupt context, starving TASK_SERIAL and
all other lower-priority tasks. Symptoms: very slow EmuConfigurator MSP
responses, stepped/blocky sensor display (configurator only received updates
every several hundred ms instead of every 10 ms).
Fix: reduce the EXTI ISR to a single flag write (< 1 µs):
imufTransferPending = true;
imufSpiGyroRead (called by gyroUpdateSensor each GYROPID cycle) now owns
the transfer. When imufTransferPending is set it clears the flag, calls
imufPrepareDmaRead, and runs spiSequence synchronously in task context.
spiSequence invokes imufIntCallback on completion, which sets dataReady;
gyroUpdateSensor proceeds with the fresh sample and clears dataReady.
If imufTransferPending is clear, imufSpiGyroRead returns dataReady=false
and gyroUpdateSensor skips the cycle -- naturally gating the GYROPID loop
to IMUF's actual data rate (same behaviour as the old isDmaSpiDataReady
checkFunc with TASK_PRIORITY_TRIGGER).
Also move imufTxBuf/imufRxBuf out of FAST_RAM_ZERO_INIT (CCM SRAM). They
are now in regular AHB SRAM. The CCM buffers had no functional impact on
the polling path but would block a future DMA migration (DMA cannot read
from CCM on F4).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ve duplicate crcErrorCount extern crcErrorCount was declared in both accgyro_imuf9001.h and accgyro_mpu.h after commit c5b3b08 added it to the MPU header alongside the new imufTransferPending extern. Duplicate extern declarations are legal C but create two apparent sources of truth and were flagged by CodeRabbit. Fix: move both imuf-specific globals (crcErrorCount, imufTransferPending) to accgyro_imuf9001.h where they semantically belong. Remove them from accgyro_mpu.h, leaving that header focused on the generic MPU/SPI bridge interface (mpuImufSetupDma, imufPrepareDmaRead). cli.c includes accgyro_imuf9001.h and uses crcErrorCount — no include change needed there. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Member
Author
|
unsoldered 16k/16k unsoldered 8k/8k |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/drivers/accgyro/accgyro_mpu.c (1)
191-192: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the IMUF buffer/transfer-size contract.
The callback assumes
xferLen >= 4, word alignment, and enough received bytes forsizeof(imufData_t), while the buffers are fixed at 58 bytes and the previous static sizing check was removed. Please re-add compile-time buffer assertions and keepimuf_modenormalized before assigningsegments[0].len.Proposed guard for the fixed buffers
static uint8_t imufTxBuf[58]; static uint8_t imufRxBuf[58]; +STATIC_ASSERT(sizeof(imufTxBuf) >= sizeof(imufCommand_t), imuf_tx_buffer_too_small); +STATIC_ASSERT(sizeof(imufRxBuf) >= sizeof(imufData_t), imuf_rx_buffer_too_small);Also applies to: 203-210, 244-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/drivers/accgyro/accgyro_mpu.c` around lines 191 - 192, Restore the IMUF transfer-size contract in the IMUF SPI path: the fixed buffers imufTxBuf and imufRxBuf must be validated at compile time against the assumptions used by the callback (xferLen >= 4, word alignment, and enough space for sizeof(imufData_t)). Re-add static assertions near the buffer declarations and ensure the receive path still guards the transfer length before using the data. Also normalize imuf_mode before assigning segments[0].len so the segment length logic stays consistent with the mode selection.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/drivers/accgyro/accgyro_mpu.c`:
- Around line 225-243: Clear the stale IMUF transfer state in imufPrepareDmaRead
so old commands are not resent on later reads. After handling
IMUF_COMMAND_SETPOINT, make sure imufTxBuf is reset to a no-command payload (and
CRC cleared if needed) before the next DMA transfer, similar to the
IMUF_DONE_CALIBRATING branch. Use the existing symbols imufPrepareDmaRead,
imufTxBuf, imufCommand_t, and isSetpointNew to locate the fix.
- Around line 146-153: The EXTI handler in accgyro_mpu.c is gating the IMUF
deferral only on USE_GYRO_IMUF9001, which prevents the generic GYRO_USES_SPI DMA
path from running for non-IMUF SPI gyros in the same build. Update the decision
logic around the EXTI callback so the imufTransferPending deferral is applied
only when the detected sensor is IMUF9001, while all other SPI gyros still reach
the existing spiSequence() path via gyro->gyroModeSPI. Use the symbols
USE_GYRO_IMUF9001, GYRO_USES_SPI, gyro->gyroModeSPI, and spiSequence() to keep
the fix localized.
---
Nitpick comments:
In `@src/main/drivers/accgyro/accgyro_mpu.c`:
- Around line 191-192: Restore the IMUF transfer-size contract in the IMUF SPI
path: the fixed buffers imufTxBuf and imufRxBuf must be validated at compile
time against the assumptions used by the callback (xferLen >= 4, word alignment,
and enough space for sizeof(imufData_t)). Re-add static assertions near the
buffer declarations and ensure the receive path still guards the transfer length
before using the data. Also normalize imuf_mode before assigning segments[0].len
so the segment length logic stays consistent with the mode selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15fcbb25-fe2b-4cdc-94f0-331f1512509c
📒 Files selected for processing (24)
src/main/drivers/accgyro/accgyro_imuf9001.csrc/main/drivers/accgyro/accgyro_imuf9001.hsrc/main/drivers/accgyro/accgyro_mpu.csrc/main/drivers/accgyro/accgyro_mpu.hsrc/main/drivers/bus_spi.csrc/main/drivers/dma_spi.csrc/main/drivers/dma_spi.hsrc/main/drivers/dma_spi_hal.csrc/main/drivers/dma_spi_hal.hsrc/main/drivers/dma_stm32f4xx.csrc/main/drivers/dma_stm32f7xx.csrc/main/drivers/nvic.hsrc/main/fc/fc_core.csrc/main/fc/fc_init.csrc/main/fc/fc_tasks.csrc/main/interface/cli.csrc/main/sensors/gyro.csrc/main/sensors/gyro.hsrc/main/target/HELIOSPRING/target.hsrc/main/target/HELIOSPRING/target.mksrc/main/target/MODE2FLUX/target.hsrc/main/target/MODE2FLUX/target.mksrc/main/target/STRIXF10/target.hsrc/main/target/STRIXF10/target.mk
💤 Files with no reviewable changes (19)
- src/main/drivers/dma_stm32f7xx.c
- src/main/drivers/nvic.h
- src/main/drivers/dma_spi_hal.h
- src/main/drivers/dma_spi.h
- src/main/sensors/gyro.h
- src/main/target/MODE2FLUX/target.mk
- src/main/drivers/dma_stm32f4xx.c
- src/main/interface/cli.c
- src/main/drivers/dma_spi_hal.c
- src/main/fc/fc_tasks.c
- src/main/target/MODE2FLUX/target.h
- src/main/drivers/dma_spi.c
- src/main/target/STRIXF10/target.h
- src/main/fc/fc_init.c
- src/main/target/HELIOSPRING/target.mk
- src/main/drivers/bus_spi.c
- src/main/target/HELIOSPRING/target.h
- src/main/fc/fc_core.c
- src/main/sensors/gyro.c
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Codacy Static Code Analysis
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-15T14:49:12.754Z
Learnt from: nerdCopter
Repo: emuflight/EmuFlight PR: 1189
File: src/main/drivers/bus_spi.c:631-631
Timestamp: 2026-05-15T14:49:12.754Z
Learning: In the EmuFlight repo (emuflight/EmuFlight), the STM32F303 (F303) targets currently do not compile on the master branch due to a pre-existing hardware RAM budget overflow (unrelated to any specific PR). When reviewing driver changes in shared driver files (e.g., src/main/drivers/bus_spi.c), avoid adding or fixing F303-specific driver behavior as part of the current PR; instead, defer those F303-targeted fixes to a dedicated F3-targeted PR and only revisit once the RAM overflow/build issue is resolved.
Applied to files:
src/main/drivers/accgyro/accgyro_imuf9001.csrc/main/drivers/accgyro/accgyro_mpu.c
🪛 GitHub Check: Codacy Static Code Analysis
src/main/drivers/accgyro/accgyro_mpu.c
[failure] 210-210: src/main/drivers/accgyro/accgyro_mpu.c#L210
Does not check for buffer overflows when copying to destination (CWE-120). Make sure destination can always hold the source data.
🔇 Additional comments (5)
src/main/target/STRIXF10/target.mk (1)
4-5: LGTM!src/main/drivers/accgyro/accgyro_imuf9001.h (1)
190-192: LGTM!src/main/drivers/accgyro/accgyro_mpu.h (1)
250-252: LGTM!src/main/drivers/accgyro/accgyro_mpu.c (1)
46-49: LGTM!Also applies to: 88-88, 249-260, 524-524
src/main/drivers/accgyro/accgyro_imuf9001.c (1)
202-215: LGTM!Also applies to: 420-422, 511-511, 538-538
Comment on lines
+146
to
153
| #if defined(USE_GYRO_IMUF9001) | ||
| // Keep EXTI ISR < 1 µs: SPI transfer is done by imufSpiGyroRead in GYROPID task context. | ||
| imufTransferPending = true; | ||
| #elif defined(GYRO_USES_SPI) | ||
| if (gyro->gyroModeSPI == GYRO_EXTI_INT_DMA) { | ||
| spiSequence(&gyro->dev, gyro->segments); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate the IMUF deferral by the detected sensor, not just the build flag.
With USE_GYRO_IMUF9001 enabled, this #if skips the generic GYRO_USES_SPI DMA read path for every MPU EXTI callback in that firmware image. Non-IMUF SPI gyros would only set imufTransferPending and never start their SPI sequence.
Proposed fix
`#if` defined(USE_GYRO_IMUF9001)
- // Keep EXTI ISR < 1 µs: SPI transfer is done by imufSpiGyroRead in GYROPID task context.
- imufTransferPending = true;
-#elif defined(GYRO_USES_SPI)
+ if (gyro->mpuDetectionResult.sensor == IMUF_9001_SPI) {
+ // Keep EXTI ISR < 1 µs: SPI transfer is done by imufSpiGyroRead in GYROPID task context.
+ imufTransferPending = true;
+ }
+#if defined(GYRO_USES_SPI)
+ else if (gyro->gyroModeSPI == GYRO_EXTI_INT_DMA) {
+ spiSequence(&gyro->dev, gyro->segments);
+ }
+#endif
+#elif defined(GYRO_USES_SPI)
if (gyro->gyroModeSPI == GYRO_EXTI_INT_DMA) {
spiSequence(&gyro->dev, gyro->segments);
}
`#endif`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #if defined(USE_GYRO_IMUF9001) | |
| // Keep EXTI ISR < 1 µs: SPI transfer is done by imufSpiGyroRead in GYROPID task context. | |
| imufTransferPending = true; | |
| #elif defined(GYRO_USES_SPI) | |
| if (gyro->gyroModeSPI == GYRO_EXTI_INT_DMA) { | |
| spiSequence(&gyro->dev, gyro->segments); | |
| } | |
| #endif | |
| `#if` defined(USE_GYRO_IMUF9001) | |
| if (gyro->mpuDetectionResult.sensor == IMUF_9001_SPI) { | |
| // Keep EXTI ISR < 1 µs: SPI transfer is done by imufSpiGyroRead in GYROPID task context. | |
| imufTransferPending = true; | |
| } | |
| `#if` defined(GYRO_USES_SPI) | |
| else if (gyro->gyroModeSPI == GYRO_EXTI_INT_DMA) { | |
| spiSequence(&gyro->dev, gyro->segments); | |
| } | |
| `#endif` | |
| `#elif` defined(GYRO_USES_SPI) | |
| if (gyro->gyroModeSPI == GYRO_EXTI_INT_DMA) { | |
| spiSequence(&gyro->dev, gyro->segments); | |
| } | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/drivers/accgyro/accgyro_mpu.c` around lines 146 - 153, The EXTI
handler in accgyro_mpu.c is gating the IMUF deferral only on USE_GYRO_IMUF9001,
which prevents the generic GYRO_USES_SPI DMA path from running for non-IMUF SPI
gyros in the same build. Update the decision logic around the EXTI callback so
the imufTransferPending deferral is applied only when the detected sensor is
IMUF9001, while all other SPI gyros still reach the existing spiSequence() path
via gyro->gyroModeSPI. Use the symbols USE_GYRO_IMUF9001, GYRO_USES_SPI,
gyro->gyroModeSPI, and spiSequence() to keep the fix localized.
Comment on lines
+225
to
243
| FAST_CODE void imufPrepareDmaRead(gyroDev_t *gyro) { | ||
| imufCommand_t *txCmd = (imufCommand_t *)imufTxBuf; | ||
| if (isImufCalibrating == IMUF_IS_CALIBRATING) { | ||
| memset(imufTxBuf, 0, sizeof(imufCommand_t)); | ||
| txCmd->command = IMUF_COMMAND_CALIBRATE; | ||
| txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11); | ||
| isImufCalibrating = IMUF_DONE_CALIBRATING; | ||
| } else if (isImufCalibrating == IMUF_DONE_CALIBRATING) { | ||
| txCmd->command = 0; | ||
| txCmd->crc = 0; | ||
| imufEndCalibration(); | ||
| } else if (isSetpointNew) { | ||
| txCmd->command = IMUF_COMMAND_SETPOINT; | ||
| txCmd->param1 = getSetpointRateInt(0); | ||
| txCmd->param2 = getSetpointRateInt(1); | ||
| txCmd->param3 = getSetpointRateInt(2); | ||
| txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11); | ||
| isSetpointNew = 0; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear stale IMUF commands before preparing each transfer.
After a setpoint update, isSetpointNew is cleared but imufTxBuf still contains IMUF_COMMAND_SETPOINT, so later “no command” reads keep resending the last setpoint command.
Proposed fix
FAST_CODE void imufPrepareDmaRead(gyroDev_t *gyro) {
imufCommand_t *txCmd = (imufCommand_t *)imufTxBuf;
+ memset(imufTxBuf, 0, sizeof(imufCommand_t));
if (isImufCalibrating == IMUF_IS_CALIBRATING) {
- memset(imufTxBuf, 0, sizeof(imufCommand_t));
txCmd->command = IMUF_COMMAND_CALIBRATE;
txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11);
isImufCalibrating = IMUF_DONE_CALIBRATING;
} else if (isImufCalibrating == IMUF_DONE_CALIBRATING) {
- txCmd->command = 0;
- txCmd->crc = 0;
imufEndCalibration();
} else if (isSetpointNew) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FAST_CODE void imufPrepareDmaRead(gyroDev_t *gyro) { | |
| imufCommand_t *txCmd = (imufCommand_t *)imufTxBuf; | |
| if (isImufCalibrating == IMUF_IS_CALIBRATING) { | |
| memset(imufTxBuf, 0, sizeof(imufCommand_t)); | |
| txCmd->command = IMUF_COMMAND_CALIBRATE; | |
| txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11); | |
| isImufCalibrating = IMUF_DONE_CALIBRATING; | |
| } else if (isImufCalibrating == IMUF_DONE_CALIBRATING) { | |
| txCmd->command = 0; | |
| txCmd->crc = 0; | |
| imufEndCalibration(); | |
| } else if (isSetpointNew) { | |
| txCmd->command = IMUF_COMMAND_SETPOINT; | |
| txCmd->param1 = getSetpointRateInt(0); | |
| txCmd->param2 = getSetpointRateInt(1); | |
| txCmd->param3 = getSetpointRateInt(2); | |
| txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11); | |
| isSetpointNew = 0; | |
| } | |
| FAST_CODE void imufPrepareDmaRead(gyroDev_t *gyro) { | |
| imufCommand_t *txCmd = (imufCommand_t *)imufTxBuf; | |
| memset(imufTxBuf, 0, sizeof(imufCommand_t)); | |
| if (isImufCalibrating == IMUF_IS_CALIBRATING) { | |
| txCmd->command = IMUF_COMMAND_CALIBRATE; | |
| txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11); | |
| isImufCalibrating = IMUF_DONE_CALIBRATING; | |
| } else if (isImufCalibrating == IMUF_DONE_CALIBRATING) { | |
| imufEndCalibration(); | |
| } else if (isSetpointNew) { | |
| txCmd->command = IMUF_COMMAND_SETPOINT; | |
| txCmd->param1 = getSetpointRateInt(0); | |
| txCmd->param2 = getSetpointRateInt(1); | |
| txCmd->param3 = getSetpointRateInt(2); | |
| txCmd->crc = getCrcImuf9001((uint32_t *)imufTxBuf, 11); | |
| isSetpointNew = 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/drivers/accgyro/accgyro_mpu.c` around lines 225 - 243, Clear the
stale IMUF transfer state in imufPrepareDmaRead so old commands are not resent
on later reads. After handling IMUF_COMMAND_SETPOINT, make sure imufTxBuf is
reset to a no-command payload (and CRC cleared if needed) before the next DMA
transfer, similar to the IMUF_DONE_CALIBRATING branch. Use the existing symbols
imufPrepareDmaRead, imufTxBuf, imufCommand_t, and isSetpointNew to locate the
fix.
…serts CodeRabbit PR emuflight#1283 review findings: - imufPrepareDmaRead unconditionally clears imufTxBuf before each transfer, so a stale IMUF_COMMAND_SETPOINT is no longer resent after isSetpointNew is cleared (matches behavior gap present since the original USE_DMA_SPI_DEVICE code, now fixed in the migrated path) - Restore STATIC_ASSERT guards on imufTxBuf/imufRxBuf sizes against imufCommand_t/imufData_t, equivalent to the asserts removed with dma_spi.c
Member
Author
|
@coderabbitai, AI generated comment Addressed findings from review #4603838966:
Build-verified on HELIOSPRING after both fixes (clean compile, RAM/FLASH unchanged). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd accgyro_mpu.c
echo "----"
sed -n '1,260p' src/main/drivers/accgyro/accgyro_mpu.c | grep -n "imufTxBuf\|imufRxBuf\|STATIC_ASSERT\|imufPrepareDmaRead\|memset"
echo "----diff context----"
git log --oneline -10Length of output: 2345
On the EXTI gating point: your reasoning is sound — since none of the three IMUF9001 targets pair Thanks for the build verification on HELIOSPRING as well. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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.
AI Generated [pull-request]
Summary
USE_DMA_SPI_DEVICE/dma_spi.*infrastructure (~710 lines deleted) and replaces it with the standard EmuFlightspiSequence+imufIntCallbackbus modelimufSpiGyroRead(called in GYROPID task context), reducing EXTI ISR latency from ~15–20µs to <1µsimufTxBuf/imufRxBuffrom CCM SRAM to AHB SRAM (prerequisite for future true DMA migration; DMA cannot read CCM on F4)imufQuat,isImufCalibrating,crcErrorCount,imufTransferPending) exclusively inaccgyro_imuf9001.h; fixes ODR violation onisImufCalibratingimuf9001SpiDetect(was silently inheriting bus state → zero gyro output)Files Changed
drivers/dma_spi.c,dma_spi.h,dma_spi_hal.c,dma_spi_hal.hdrivers/accgyro/accgyro_imuf9001.cimufSpiGyroReadperforms deferred SPI transfer; SPI Mode 0 explicit initdrivers/accgyro/accgyro_mpu.cimufTransferPendingonly (<1µs); buffers to AHB SRAMdrivers/accgyro/accgyro_imuf9001.hexternODR fixdrivers/accgyro/accgyro_mpu.htarget/HELIOSPRING,STRIXF10,MODE2FLUXUSE_DMA_SPI_DEVICEdefinesfc/fc_tasks.c,sensors/gyro.cisDmaSpiDataReadycheckFunc;dataReadygate is now insideimufSpiGyroReadKnown Limitation
32kHz gyro loop produces stepped waveforms and MSP starvation on IMUF9001 targets. This is a pre-existing condition also present on master — not a regression of this PR. Tracked in #1281. Fix requires true non-blocking DMA for the SPI transfer; the AHB SRAM buffer migration in this PR is the prerequisite for that work.
Test Plan
gyro_sync_denom=4)Related Issues
Summary by CodeRabbit
New Features
Bug Fixes