refactor(firmware/c_board): Split app/bsp layout and align firmware packaging#27
Merged
refactor(firmware/c_board): Split app/bsp layout and align firmware packaging#27
Conversation
…ackaging - Move app code to `app/src/` and `app/include/` to make room for future bootloaders. - Isolate CubeMX files in `bsp/cubemx/`, rename linker script to `STM32F407XX_APP.ld`, and enable `NoMain=true`. - Move `main()` to the app layer to control startup flow, relocate startup interrupt-disable logic from `startup_stm32f407xx.s` to `main()`, and keep TinyUSB IRQ dispatch in CubeMX ISR. - Refactor CMake with top-level shared options and `app/CMakeLists.txt`; update toolchain path, presets, and lint targets. - Adapt `.scripts/patch_cubemx` for the new CubeMX root and patch strategy. - Update `Dockerfile.build_firmware` to package only release ELFs (excluding debug artifacts) from specific board outputs (`rmcs_board`, `c_board_app`). No runtime feature changes are introduced to the c_board application logic; startup and packaging flows are strictly reorganized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough将 firmware/c_board 源重构至 app 子目录，新增 TinyUSB 应用目标并引入 c_board_app，可执行入口与 CubeMX 生成入口被替换；重构 CMake/工具链与 patch_cubemx 行为；调整构建脚本（Dockerfile.build_firmware）与若干启动/中断初始化语义。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 Possibly related PRs
诗歌
🚥 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 (4)
firmware/c_board/cmake/gcc-arm-none-eabi.cmake (1)
5-6: 移除手动设置的CMAKE_C_COMPILER_ID和CMAKE_CXX_COMPILER_ID。这两个变量是 CMake 的输出变量，应由 CMake 在项目配置时（通过
project()或enable_language()）自动检测，而非手动赋值。由于你已在第 10、12 行正确设置了实际的编译器可执行文件路径（arm-none-eabi-gcc、arm-none-eabi-g++），CMake 将自动识别编译器 ID 为 GNU，无需显式手动设置。手动赋值是非规范做法，违反了现代 CMake 工具链最佳实践。♻️ 建议修改
-set(CMAKE_C_COMPILER_ID GNU) -set(CMAKE_CXX_COMPILER_ID GNU)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/cmake/gcc-arm-none-eabi.cmake` around lines 5 - 6, Remove the manual assignments to CMAKE_C_COMPILER_ID and CMAKE_CXX_COMPILER_ID: delete the set(CMAKE_C_COMPILER_ID GNU) and set(CMAKE_CXX_COMPILER_ID GNU) lines so CMake can auto-detect the compiler ID; rely on the existing settings of CMAKE_C_COMPILER and CMAKE_CXX_COMPILER (the arm-none-eabi-gcc/arm-none-eabi-g++ assignments) and ensure project() or enable_language() is used to trigger detection rather than forcing the *_COMPILER_ID variables.firmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.c (1)
28-28: 考虑通过包含官方头文件而非手写前置声明来保证 TinyUSB 接口的一致性。第 28 行的手写前置声明
tusb_int_handler虽然在此处有效，但长期维护中可能面临与上游签名漂移的风险。建议改为直接包含 TinyUSB 头文件（如#include <tusb.h>），或在统一的适配头中集中管理此声明，以便编译器能自动验证 API 签名。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.c` at line 28, Replace the handwritten forward declaration of tusb_int_handler with the official TinyUSB header or a centralized adapter header so the compiler verifies the API signature: remove the line declaring void tusb_int_handler(uint8_t rhport, bool in_isr); and instead include the TinyUSB public header (e.g., `#include` <tusb.h>) or a project adapter header that exposes tusb_int_handler, ensuring the symbol and signature come from the upstream header rather than a local manual declaration..scripts/patch_cubemx (1)
42-48: 注释与实际代码不符：OBJECT 库不是静态库。
add_library(stm32cubemx OBJECT)创建的是对象库而非静态库。建议将注释改为更准确的描述。📝 建议修复注释
-STM32CUBEMX_TAIL = """# Create stm32cubemx static library +STM32CUBEMX_TAIL = """# Create stm32cubemx object library add_library(stm32cubemx OBJECT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.scripts/patch_cubemx around lines 42 - 48, 注释与实际代码不符：在 STM32CUBEMX_TAIL 字符串中当前说明“Create stm32cubemx static library”误导为静态库，而 add_library(stm32cubemx OBJECT) 实际创建的是对象库；请更新该注释文本以准确描述为“Create stm32cubemx object library”或类似表述，确保唯一标识符 STM32CUBEMX_TAIL 和 add_library(stm32cubemx OBJECT) 保持不变。firmware/c_board/app/CMakeLists.txt (1)
32-39: 关于 GLOB_RECURSE 的使用说明。
GLOB_RECURSE配合CONFIGURE_DEPENDS在实践中是可接受的，但需注意：
- CMake 官方文档建议显式列出源文件以确保可靠的增量构建
CONFIGURE_DEPENDS在某些生成器上可能有性能开销对于当前固件项目规模，这种方式是合理的权衡。如果将来构建时间成为问题，可以考虑显式列出源文件。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/app/CMakeLists.txt` around lines 32 - 39, The CMakeLists uses GLOB_RECURSE with CONFIGURE_DEPENDS to populate C_BOARD_APP_SOURCES which can hurt incremental build reliability and performance; either replace the GLOB_RECURSE usage by explicitly listing source files (move individual .c/.cpp/.S paths into C_BOARD_APP_SOURCES) so builds are stable, or if you keep globbing, add a clear comment near the GLOB_RECURSE block documenting the tradeoff and remove CONFIGURE_DEPENDS to avoid generator-specific overhead until you need auto-refresh; refer to the CMake variable C_BOARD_APP_SOURCES and the existing GLOB_RECURSE invocation to locate and apply the change.
🤖 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/CMakeLists.txt`:
- Around line 69-75: The CMake generator expression in target_compile_options
for ${target} uses a single expression with multiple space-separated flags
($<$<COMPILE_LANGUAGE:CXX>:-Wall -Wextra -Wpedantic>), which will be split
before evaluation and break the expression; replace that single multi-flag
generator expression with one generator expression per flag (e.g.
$<$<COMPILE_LANGUAGE:CXX>:-Wall>, $<$<COMPILE_LANGUAGE:CXX>:-Wextra>,
$<$<COMPILE_LANGUAGE:CXX>:-Wpedantic>) so each compiler option is preserved,
keeping the other entries (like $<$<COMPILE_LANGUAGE:CXX>:-fno-unwind-tables>)
unchanged.
---
Nitpick comments:
In @.scripts/patch_cubemx:
- Around line 42-48: 注释与实际代码不符：在 STM32CUBEMX_TAIL 字符串中当前说明“Create stm32cubemx
static library”误导为静态库，而 add_library(stm32cubemx OBJECT)
实际创建的是对象库；请更新该注释文本以准确描述为“Create stm32cubemx object library”或类似表述，确保唯一标识符
STM32CUBEMX_TAIL 和 add_library(stm32cubemx OBJECT) 保持不变。
In `@firmware/c_board/app/CMakeLists.txt`:
- Around line 32-39: The CMakeLists uses GLOB_RECURSE with CONFIGURE_DEPENDS to
populate C_BOARD_APP_SOURCES which can hurt incremental build reliability and
performance; either replace the GLOB_RECURSE usage by explicitly listing source
files (move individual .c/.cpp/.S paths into C_BOARD_APP_SOURCES) so builds are
stable, or if you keep globbing, add a clear comment near the GLOB_RECURSE block
documenting the tradeoff and remove CONFIGURE_DEPENDS to avoid
generator-specific overhead until you need auto-refresh; refer to the CMake
variable C_BOARD_APP_SOURCES and the existing GLOB_RECURSE invocation to locate
and apply the change.
In `@firmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.c`:
- Line 28: Replace the handwritten forward declaration of tusb_int_handler with
the official TinyUSB header or a centralized adapter header so the compiler
verifies the API signature: remove the line declaring void
tusb_int_handler(uint8_t rhport, bool in_isr); and instead include the TinyUSB
public header (e.g., `#include` <tusb.h>) or a project adapter header that exposes
tusb_int_handler, ensuring the symbol and signature come from the upstream
header rather than a local manual declaration.
In `@firmware/c_board/cmake/gcc-arm-none-eabi.cmake`:
- Around line 5-6: Remove the manual assignments to CMAKE_C_COMPILER_ID and
CMAKE_CXX_COMPILER_ID: delete the set(CMAKE_C_COMPILER_ID GNU) and
set(CMAKE_CXX_COMPILER_ID GNU) lines so CMake can auto-detect the compiler ID;
rely on the existing settings of CMAKE_C_COMPILER and CMAKE_CXX_COMPILER (the
arm-none-eabi-gcc/arm-none-eabi-g++ assignments) and ensure project() or
enable_language() is used to trigger detection rather than forcing the
*_COMPILER_ID variables.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (64)
.scripts/lint-targets.yml.scripts/patch_cubemxDockerfile.build_firmwarefirmware/c_board/CMakeLists.txtfirmware/c_board/CMakePresets.jsonfirmware/c_board/app/CMakeLists.txtfirmware/c_board/app/include/tusb_config.hfirmware/c_board/app/src/app.cppfirmware/c_board/app/src/app.hppfirmware/c_board/app/src/can/can.cppfirmware/c_board/app/src/can/can.hppfirmware/c_board/app/src/gpio/gpio.cppfirmware/c_board/app/src/led/led.hppfirmware/c_board/app/src/logger/logger.hppfirmware/c_board/app/src/spi/bmi088/accel.hppfirmware/c_board/app/src/spi/bmi088/base.hppfirmware/c_board/app/src/spi/bmi088/gyro.hppfirmware/c_board/app/src/spi/spi.cppfirmware/c_board/app/src/spi/spi.hppfirmware/c_board/app/src/timer/delay.cppfirmware/c_board/app/src/timer/delay.hppfirmware/c_board/app/src/uart/uart.cppfirmware/c_board/app/src/uart/uart.hppfirmware/c_board/app/src/usb/helper.hppfirmware/c_board/app/src/usb/interrupt_safe_buffer.hppfirmware/c_board/app/src/usb/usb_descriptors.cppfirmware/c_board/app/src/usb/usb_descriptors.hppfirmware/c_board/app/src/usb/vendor.cppfirmware/c_board/app/src/usb/vendor.hppfirmware/c_board/app/src/utility/assert.cppfirmware/c_board/app/src/utility/interrupt_lock.hppfirmware/c_board/app/src/utility/lazy.hppfirmware/c_board/app/src/utility/ring_buffer.hppfirmware/c_board/bsp/cubemx/.gitignorefirmware/c_board/bsp/cubemx/Core/Inc/can.hfirmware/c_board/bsp/cubemx/Core/Inc/dma.hfirmware/c_board/bsp/cubemx/Core/Inc/gpio.hfirmware/c_board/bsp/cubemx/Core/Inc/main.hfirmware/c_board/bsp/cubemx/Core/Inc/spi.hfirmware/c_board/bsp/cubemx/Core/Inc/stm32f4xx_hal_conf.hfirmware/c_board/bsp/cubemx/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/cubemx/Core/Inc/tim.hfirmware/c_board/bsp/cubemx/Core/Inc/usart.hfirmware/c_board/bsp/cubemx/Core/Inc/usb_otg.hfirmware/c_board/bsp/cubemx/Core/Src/can.cfirmware/c_board/bsp/cubemx/Core/Src/dma.cfirmware/c_board/bsp/cubemx/Core/Src/gpio.cfirmware/c_board/bsp/cubemx/Core/Src/main.cfirmware/c_board/bsp/cubemx/Core/Src/spi.cfirmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_hal_msp.cfirmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/cubemx/Core/Src/syscalls.cfirmware/c_board/bsp/cubemx/Core/Src/sysmem.cfirmware/c_board/bsp/cubemx/Core/Src/system_stm32f4xx.cfirmware/c_board/bsp/cubemx/Core/Src/tim.cfirmware/c_board/bsp/cubemx/Core/Src/usart.cfirmware/c_board/bsp/cubemx/Core/Src/usb_otg.cfirmware/c_board/bsp/cubemx/STM32F407XX_APP.ldfirmware/c_board/bsp/cubemx/cmake/stm32cubemx/CMakeLists.txtfirmware/c_board/bsp/cubemx/rmcs_slave.iocfirmware/c_board/bsp/cubemx/startup_stm32f407xx.sfirmware/c_board/cmake/gcc-arm-none-eabi.cmakefirmware/c_board/cubemx/cmake/gcc-arm-none-eabi.cmakefirmware/c_board/src/app.cpp
💤 Files with no reviewable changes (4)
- firmware/c_board/cubemx/cmake/gcc-arm-none-eabi.cmake
- firmware/c_board/bsp/cubemx/Core/Src/main.c
- firmware/c_board/src/app.cpp
- firmware/c_board/bsp/cubemx/startup_stm32f407xx.s
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.scripts/patch_cubemx (1)
38-41: 建议添加注释说明 "Midllewares" 拼写是故意保留的。正则表达式中的
Midllewares（少了一个d）是 CubeMX 生成文件中的原始拼写错误。建议添加注释说明这是刻意匹配，避免未来维护者误以为是脚本中的笔误而"修正"它。💡 建议添加注释
+# Note: "Midllewares" typo is intentional - matches CubeMX's actual output DRIVERS_MIDDLEWARES_TO_EOF_PATTERN = re.compile( r"^# Drivers Midllewares[\s\S]*\Z", re.MULTILINE, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.scripts/patch_cubemx around lines 38 - 41, The regex constant DRIVERS_MIDDLEWARES_TO_EOF_PATTERN intentionally contains the misspelling "Midllewares" to match the exact header produced by CubeMX; add a concise inline comment above this declaration explaining that the spelling is preserved intentionally to match CubeMX-generated files (do not correct it), mention CubeMX as the source of the misspelling, and note that future maintainers should not "fix" the string because it is deliberately matching external output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.scripts/patch_cubemx:
- Around line 87-91: The script fails on re-run because the replacement block
assumes the anchor exists; modify the logic around
DRIVERS_MIDDLEWARES_TO_EOF_PATTERN.subn and the subsequent check so the code is
idempotent: after computing patched and replacements, if replacements == 0 then
check for presence of STM32CUBEMX_TAIL (or STM32CUBEMX_TAIL_ANCHOR) inside
patched and treat that as "already patched" (no-op) instead of raising
ValueError; otherwise, if neither the anchor nor the tail are present, raise the
error as before. Use the existing symbols (DRIVERS_MIDDLEWARES_TO_EOF_PATTERN,
STM32CUBEMX_TAIL, STM32CUBEMX_TAIL_ANCHOR, patched, replacements) to implement
this detection.
---
Nitpick comments:
In @.scripts/patch_cubemx:
- Around line 38-41: The regex constant DRIVERS_MIDDLEWARES_TO_EOF_PATTERN
intentionally contains the misspelling "Midllewares" to match the exact header
produced by CubeMX; add a concise inline comment above this declaration
explaining that the spelling is preserved intentionally to match
CubeMX-generated files (do not correct it), mention CubeMX as the source of the
misspelling, and note that future maintainers should not "fix" the string
because it is deliberately matching external output.
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.
app/src/andapp/include/to make room for future bootloaders.bsp/cubemx/, rename linker script toSTM32F407XX_APP.ld, and enableNoMain=true.main()to the app layer to control startup flow, relocate startup interrupt-disable logic fromstartup_stm32f407xx.stomain(), and keep TinyUSB IRQ dispatch in CubeMX ISR.app/CMakeLists.txt; update toolchain path, presets, and lint targets..scripts/patch_cubemxfor the new CubeMX root and patch strategy.Dockerfile.build_firmwareto package only release ELFs (excluding debug artifacts) from specific board outputs (rmcs_board,c_board_app).No runtime feature changes are introduced to the c_board application logic; startup and packaging flows are strictly reorganized.
c_board 固件重构：应用层与 BSP 分离（更新摘要）
概述
本 PR 将 c_board 固件重构为明确的 app / bsp 分层，主要目标为支持未来 bootloader、清晰化启动组织及改进固件打包流程。声称无运行时功能性变化；改动集中在源码/头文件重组、启动入口迁移、中断初始化语义、CMake 架构重构以及构建/打包脚本更新。
主要改动点
目录与文件重组
启动流程与中断管理
应用实现迁移
CMake 架构重构
构建与打包脚本调整
其他小改动
风险与注意事项
结论
这是一次以构建、打包与启动组织为中心的重构：将应用从 CubeMX 输出中剥离为独立 app 层、将 CubeMX 保留为 BSP 提供者并改造为对象库集成，调整工具链与 CI 打包以只产出应用 release ELF。需要在硬件/CI 上验证启动与中断相关的细微语义变化。