fix(firmware/c_board): Switch SPI1 async transfer to DMA to avoid OVR errors#25
Merged
fix(firmware/c_board): Switch SPI1 async transfer to DMA to avoid OVR errors#25
Conversation
Walkthrough在 STM32F4 平台引入 DMA（DMA2_Stream0/Stream3）并将 SPI1 异步传输切换为 DMA 路径，新增 DMA 初始化、IRQ 处理、SPI DMA 绑定及回调签名调整，同时修改若干 GPIO EXTI 边沿、断言指示和构建配置。 Changes
Sequence Diagram(s)sequenceDiagram
participant Init as 初始化
participant DMA as DMA 控制器
participant SPI as SPI1 外设
participant IRQ as NVIC/IRQ
Init->>DMA: MX_DMA_Init()
DMA->>DMA: 使能 DMA2 时钟，配置 Stream0/Stream3 IRQ 优先级
DMA->>IRQ: 使能 DMA 中断向量
Init->>SPI: MX_SPI1_Init()
SPI->>DMA: 配置 Stream0(RX) 与 Stream3(TX) 并链接到 SPI
SPI->>DMA: 触发 DMA RX/TX 传输
DMA->>IRQ: 传输完成或错误触发中断
IRQ->>SPI: DMA2_Stream0/3_IRQHandler -> HAL_DMA_IRQHandler -> HAL 回调
SPI->>Module: 驱动回调 -> module->transmit_receive_async_callback(size)
sequenceDiagram
participant HAL as HAL 回调
participant SpiDrv as Spi 驱动
participant Module as SpiModule (加速计/陀螺)
participant USB as USB 上行
HAL->>SpiDrv: DMA 完成/错误回调 (hal_dma_handle)
SpiDrv->>Module: module->transmit_receive_async_callback(size)
Module->>Module: if size>0: 从 spi_.rx_buffer 解析数据
Module->>USB: 通过 serializer 发送上行
Module->>SpiDrv: spi_.unlock()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c (1)
339-350: 存在死代码：HAL_PCD_IRQHandler永远不会被执行。
return语句在第 343 行导致HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS)永远不会被调用。如果这是预期行为（TinyUSB 完全接管 USB 中断处理），建议删除或注释掉死代码以提高可读性。♻️ 建议清理死代码
void OTG_FS_IRQHandler(void) { /* USER CODE BEGIN OTG_FS_IRQn 0 */ tusb_int_handler(0, true); return; - - /* USER CODE END OTG_FS_IRQn 0 */ - HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS); - /* USER CODE BEGIN OTG_FS_IRQn 1 */ - - /* USER CODE END OTG_FS_IRQn 1 */ + /* USER CODE END OTG_FS_IRQn 0 */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c` around lines 339 - 350, OTG_FS_IRQHandler contains dead code because the early return after calling tusb_int_handler(0, true) prevents HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS) from ever executing; if TinyUSB should fully own the ISR, remove or comment out the unreachable HAL_PCD_IRQHandler call and its surrounding USER CODE markers in OTG_FS_IRQHandler, otherwise remove the early return and ensure tusb_int_handler is only called conditionally so HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS) can run; update the function OTG_FS_IRQHandler and references to tusb_int_handler and hpcd_USB_OTG_FS accordingly to keep intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/c_board/src/spi/spi.hpp`:
- Around line 81-83: The async path currently conflates "0" as both a valid
transfer length and a failure signal; enforce tx_rx_size_ > 0 at the async
transfer entry (where the transfer is queued/scheduled) so zero-length requests
are rejected/returned early and never reach the async callback, and update
transmit_receive_async_callback to only call
module->transmit_receive_async_callback(tx_rx_size_) when success && tx_rx_size_
> 0 (otherwise treat as failure). If you prefer a clearer API, change the module
callback signature to accept an explicit success flag (e.g.,
transmit_receive_async_callback(bool success, size_t size)) and use
finish_transfer()/tx_rx_size_ to populate both fields.
---
Nitpick comments:
In `@firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c`:
- Around line 339-350: OTG_FS_IRQHandler contains dead code because the early
return after calling tusb_int_handler(0, true) prevents
HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS) from ever executing; if TinyUSB should
fully own the ISR, remove or comment out the unreachable HAL_PCD_IRQHandler call
and its surrounding USER CODE markers in OTG_FS_IRQHandler, otherwise remove the
early return and ensure tusb_int_handler is only called conditionally so
HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS) can run; update the function
OTG_FS_IRQHandler and references to tusb_int_handler and hpcd_USB_OTG_FS
accordingly to keep intent clear.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
firmware/c_board/bsp/HAL/.mxprojectfirmware/c_board/bsp/HAL/Core/Inc/dma.hfirmware/c_board/bsp/HAL/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/HAL/Core/Src/dma.cfirmware/c_board/bsp/HAL/Core/Src/gpio.cfirmware/c_board/bsp/HAL/Core/Src/main.cfirmware/c_board/bsp/HAL/Core/Src/spi.cfirmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/HAL/cmake/stm32cubemx/CMakeLists.txtfirmware/c_board/bsp/HAL/rmcs_slave.iocfirmware/c_board/src/spi/bmi088/accel.hppfirmware/c_board/src/spi/bmi088/gyro.hppfirmware/c_board/src/spi/spi.cppfirmware/c_board/src/spi/spi.hppfirmware/c_board/src/utility/assert.cppfirmware/rmcs_board/src/spi/bmi088/accel.hppfirmware/rmcs_board/src/spi/bmi088/gyro.hppfirmware/rmcs_board/src/spi/spi.hpp
Improve on-board failure visibility in assert path. - Initialize GPIOH in assert handler when needed - Drive RGB LED to red before __builtin_trap() - Disable interrupt in assert_func
qzhhhi
force-pushed
the
fix/cboard-spi
branch
from
248d1ca to
cc0c804
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
firmware/c_board/bsp/HAL/Core/Inc/main.h (1)
35-36: 建议移除冗余的设备头stm32f407xx.h包含（第35行）。
stm32f4xx.h作为 CMSIS 统一入口，已在构建配置中通过STM32F407xx编译宏自动激活该设备头的条件包含。直接包含stm32f407xx.h属于冗余，会增加芯片型号耦合。建议仅保留stm32f4xx.h，这样日后迁移到同系列其他型号时，仅需修改编译宏定义，无需调整代码。修正 diff
-#include "stm32f407xx.h" // IWYU pragma: export `#include` "stm32f4xx.h" // IWYU pragma: export🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bsp/HAL/Core/Inc/main.h` around lines 35 - 36, Remove the redundant device-specific include: delete the `#include` "stm32f407xx.h" line and keep only `#include` "stm32f4xx.h" in main.h so the CMSIS umbrella header (activated by the STM32F407xx compile macro) selects the device header; preserve existing IWYU pragmas on the remaining include if needed.firmware/c_board/src/spi/spi.hpp (1)
94-111: 建议在失败分支重置active_dma_count_，避免残留 DMA 状态。当前失败路径会清理 SPI/DMA，但没有把
active_dma_count_复位到kDmaNotPerformed。这会让状态机留下陈旧计数，后续诊断和边界行为更难推断。🔧 建议修改（示意）
if (!success) [[unlikely]] { + active_dma_count_.store(kDmaNotPerformed, std::memory_order::relaxed); HAL_DMA_Abort(hal_spi_handle_->hdmarx); HAL_DMA_Abort(hal_spi_handle_->hdmatx); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/src/spi/spi.hpp` around lines 94 - 111, In transmit_receive_async_callback(bool success) ensure that on the failure branch you reset the DMA state by setting active_dma_count_ = kDmaNotPerformed before returning/continuing; specifically, inside the if (!success) [[unlikely]] block (or immediately after DMA aborts) update active_dma_count_ to kDmaNotPerformed so the state machine and diagnostics no longer see a stale DMA count—adjust any related logic in finish_transfer() expectations if needed.firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c (1)
339-347: 建议清理OTG_FS_IRQHandler中的不可达代码，避免误导维护者。Line 343 已直接
return，因此 Line 346 的HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS);永远不会执行。若这是有意切到 TinyUSB 直连路径，建议删除不可达分支或补一条注释说明“故意绕过 HAL_PCD_IRQHandler”。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c` around lines 339 - 347, The OTG_FS_IRQHandler function contains unreachable code because tusb_int_handler(0, true); is followed by an immediate return, so HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS); is never executed; either remove the unreachable HAL_PCD_IRQHandler call or clearly document why the HAL path is intentionally bypassed (e.g., add comment "intentionally using TinyUSB direct handler, bypass HAL_PCD_IRQHandler"), and ensure the tusb_int_handler call and return remain correct; update only the OTG_FS_IRQHandler function to remove ambiguity by deleting the unreachable branch or adding the explanatory comment referencing OTG_FS_IRQHandler, tusb_int_handler, HAL_PCD_IRQHandler, and hpcd_USB_OTG_FS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/c_board/src/spi/spi.hpp`:
- Around line 152-163: 目前对 HAL_DMA_Start_IT 的调用仅用 core::utility::assert_debug
校验，发布版会被去除导致失败时状态不一致并使 update() 无限等待。请在对 hal_spi_handle_->hdmarx 和
hal_spi_handle_->hdmatx 的两次 HAL_DMA_Start_IT 调用后增加显式错误检查：若返回值非 HAL_OK，应立即调用
transmit_receive_async_callback(false) 清理并恢复 active_dma_count_ / SPI
状态（确保与正常路径一样递减/重置），并避免继续执行启动后的流程；在检查位置引用 HAL_DMA_Start_IT、hal_spi_handle_,
transmit_receive_async_callback 和 active_dma_count_ 以定位修改点。
---
Nitpick comments:
In `@firmware/c_board/bsp/HAL/Core/Inc/main.h`:
- Around line 35-36: Remove the redundant device-specific include: delete the
`#include` "stm32f407xx.h" line and keep only `#include` "stm32f4xx.h" in main.h so
the CMSIS umbrella header (activated by the STM32F407xx compile macro) selects
the device header; preserve existing IWYU pragmas on the remaining include if
needed.
In `@firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c`:
- Around line 339-347: The OTG_FS_IRQHandler function contains unreachable code
because tusb_int_handler(0, true); is followed by an immediate return, so
HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS); is never executed; either remove the
unreachable HAL_PCD_IRQHandler call or clearly document why the HAL path is
intentionally bypassed (e.g., add comment "intentionally using TinyUSB direct
handler, bypass HAL_PCD_IRQHandler"), and ensure the tusb_int_handler call and
return remain correct; update only the OTG_FS_IRQHandler function to remove
ambiguity by deleting the unreachable branch or adding the explanatory comment
referencing OTG_FS_IRQHandler, tusb_int_handler, HAL_PCD_IRQHandler, and
hpcd_USB_OTG_FS.
In `@firmware/c_board/src/spi/spi.hpp`:
- Around line 94-111: In transmit_receive_async_callback(bool success) ensure
that on the failure branch you reset the DMA state by setting active_dma_count_
= kDmaNotPerformed before returning/continuing; specifically, inside the if
(!success) [[unlikely]] block (or immediately after DMA aborts) update
active_dma_count_ to kDmaNotPerformed so the state machine and diagnostics no
longer see a stale DMA count—adjust any related logic in finish_transfer()
expectations if needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
firmware/c_board/bsp/HAL/.mxprojectfirmware/c_board/bsp/HAL/Core/Inc/dma.hfirmware/c_board/bsp/HAL/Core/Inc/main.hfirmware/c_board/bsp/HAL/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/HAL/Core/Src/dma.cfirmware/c_board/bsp/HAL/Core/Src/gpio.cfirmware/c_board/bsp/HAL/Core/Src/main.cfirmware/c_board/bsp/HAL/Core/Src/spi.cfirmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/HAL/cmake/stm32cubemx/CMakeLists.txtfirmware/c_board/bsp/HAL/rmcs_slave.iocfirmware/c_board/src/app.cppfirmware/c_board/src/spi/bmi088/accel.hppfirmware/c_board/src/spi/bmi088/gyro.hppfirmware/c_board/src/spi/spi.cppfirmware/c_board/src/spi/spi.hppfirmware/c_board/src/utility/assert.cppfirmware/rmcs_board/src/spi/bmi088/accel.hppfirmware/rmcs_board/src/spi/bmi088/gyro.hppfirmware/rmcs_board/src/spi/spi.hpp
🚧 Files skipped from review as they are similar to previous changes (6)
- firmware/c_board/bsp/HAL/Core/Src/main.c
- firmware/c_board/bsp/HAL/cmake/stm32cubemx/CMakeLists.txt
- firmware/c_board/bsp/HAL/Core/Src/spi.c
- firmware/c_board/src/spi/bmi088/accel.hpp
- firmware/c_board/bsp/HAL/Core/Src/gpio.c
- firmware/c_board/bsp/HAL/Core/Inc/dma.h
… errors (#25) The previous interrupt-based SPI async path could hit HAL_SPI_ERROR_OVR at high SPI clock rates because RXNE was not serviced fast enough before the next byte. - Replace HAL_SPI_TransmitReceive_IT with HAL_SPI_TransmitReceive_DMA - Add SPI1 DMA resources (DMA2 Stream0/3), DMA init, and DMA IRQ handlers - Wire CubeMX-generated DMA files into build and startup init sequence - Add SPI error callback fallback to finish transfer and release CS/lock - Fix BMI088 DRDY EXTI to falling edge (active-low configuration) - Synchronize SPI async callback interface changes in firmware/rmcs_board
…reams finish (#25) - Replace HAL_SPI_TransmitReceive_DMA usage with a SPI-owned minimal DMA trigger path - Register SPI1 RX/TX DMA callbacks in Spi and start both streams directly - Track active DMA stream count and finalize transfer only after RX+TX both complete - Move async finalize trigger into main loop via spi::spi1->update() - Keep HAL SPI callbacks as fail-fast/error fallback This addresses an inherent issue in HAL_SPI_TransmitReceive_DMA completion behavior (RX-driven completion while TX DMA completion can lag), which can surface as HAL_ERROR under CAN RX interrupt pressure.
qzhhhi
force-pushed
the
fix/cboard-spi
branch
from
a2008cd to
11adb62
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c (1)
339-347:OTG_FS_IRQHandler里存在不可达分支，建议收敛为单一路径。Line 343 的
return让 Line 346 的HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS)永远不会执行。建议明确只保留一种处理中断的实现，避免后续维护误判。♻️ 建议精简（若目标是仅由 TinyUSB 处理中断）
void OTG_FS_IRQHandler(void) { /* USER CODE BEGIN OTG_FS_IRQn 0 */ tusb_int_handler(0, true); - return; - - /* USER CODE END OTG_FS_IRQn 0 */ - HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS); + /* USER CODE END OTG_FS_IRQn 0 */ /* USER CODE BEGIN OTG_FS_IRQn 1 */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c` around lines 339 - 347, OTG_FS_IRQHandler contains an unreachable branch because the call to tusb_int_handler(0, true) is followed by an early return, so HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS) never runs; fix by choosing a single interrupt handling path: either remove the return and let both handlers run (if both are required) or remove the HAL_PCD_IRQHandler call (and its related code) so only tusb_int_handler is used. Update OTG_FS_IRQHandler accordingly and ensure no leftover comment blocks or dead code remain referencing HAL_PCD_IRQHandler or hpcd_USB_OTG_FS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/c_board/src/spi/spi.hpp`:
- Around line 93-112: In transmit_receive_async_callback, the debug assert on
success runs before the failure cleanup and can abort cleanup in Debug builds;
move the core::utility::assert_debug_lazy([&]() noexcept { return success; }) so
it executes after the failure-handling and resource-release steps (the
HAL_DMA_Abort calls, active_dma_count_.store(...), CLEAR_BIT on CR2, resetting
hal_spi_handle_->TxXferCount/RxXferCount/State, and the
finish_transfer()/module->transmit_receive_async_callback call) so that even
when success==false the DMA/SPI abort, state reset and finish_transfer
invocation always run before the debug-only fail-fast assertion. Ensure
references remain to transmit_receive_async_callback, HAL_DMA_Abort,
active_dma_count_, CLEAR_BIT, hal_spi_handle_->State, and finish_transfer().
---
Nitpick comments:
In `@firmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.c`:
- Around line 339-347: OTG_FS_IRQHandler contains an unreachable branch because
the call to tusb_int_handler(0, true) is followed by an early return, so
HAL_PCD_IRQHandler(&hpcd_USB_OTG_FS) never runs; fix by choosing a single
interrupt handling path: either remove the return and let both handlers run (if
both are required) or remove the HAL_PCD_IRQHandler call (and its related code)
so only tusb_int_handler is used. Update OTG_FS_IRQHandler accordingly and
ensure no leftover comment blocks or dead code remain referencing
HAL_PCD_IRQHandler or hpcd_USB_OTG_FS.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
firmware/c_board/bsp/HAL/.mxprojectfirmware/c_board/bsp/HAL/Core/Inc/dma.hfirmware/c_board/bsp/HAL/Core/Inc/main.hfirmware/c_board/bsp/HAL/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/HAL/Core/Src/dma.cfirmware/c_board/bsp/HAL/Core/Src/gpio.cfirmware/c_board/bsp/HAL/Core/Src/main.cfirmware/c_board/bsp/HAL/Core/Src/spi.cfirmware/c_board/bsp/HAL/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/HAL/cmake/stm32cubemx/CMakeLists.txtfirmware/c_board/bsp/HAL/rmcs_slave.iocfirmware/c_board/src/app.cppfirmware/c_board/src/spi/bmi088/accel.hppfirmware/c_board/src/spi/bmi088/gyro.hppfirmware/c_board/src/spi/spi.cppfirmware/c_board/src/spi/spi.hppfirmware/c_board/src/utility/assert.cppfirmware/rmcs_board/src/spi/bmi088/accel.hppfirmware/rmcs_board/src/spi/bmi088/gyro.hppfirmware/rmcs_board/src/spi/spi.hpp
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.
The previous interrupt-based SPI async path could hit HAL_SPI_ERROR_OVR at high
SPI clock rates because RXNE was not serviced fast enough before the next byte.
概述
本 PR 将 STM32F4 上 SPI1 的异步传输从中断方式（HAL_SPI_TransmitReceive_IT）切换为 DMA（HAL_SPI_TransmitReceive_DMA），以避免在高 SPI 时钟下因 RXNE 未及时读取导致的 HAL_SPI_ERROR_OVR。为此新增并集成了 SPI1 所需的 DMA 资源与中断处理、将 DMA 初始化并入启动流程，增加 SPI 错误回调兜底，修正 BMI088 的 DRDY EXTI 配置，并在断言路径增加 LED 可视化以提高故障可见性。上层 SPI 回调接口在驱动层改动后已同步更新。
主要改动点
DMA 基础设施
SPI1 的 DMA 支持（HAL MSP 层）
SPI 异步传输与回调接口变更（驱动层与上层同步）
接口与上层驱动适配（BMI088）
EXTI 与 GPIO 配置修正
断言与可视化
其它启动/构建调整
重要文件（要点）
HAL / 启动相关
驱动/上层适配
兼容性与行为影响
风险与审查建议
结论
该 PR 将 SPI1 异步传输切换为 DMA 并在 HAL/MSP、向量表、CubeMX 配置、驱动接口与上层实现间做了全面适配。总体设计合理，能有效降低高速 SPI 导致的 OVR 错误；审查重点应放在 DMA 资源分配冲突、回调并发与错误处理的正确性，以及 EXTI 与断言处理的上下文安全性。