feat(firmware/c_board): Migrate c_board firmware from rmcs_slave to librmcs v3 (WIP)#19
feat(firmware/c_board): Migrate c_board firmware from rmcs_slave to librmcs v3 (WIP)#19
Conversation
Add class-scoped identifier-naming rules in .clang-tidy: - ClassMemberCase/ClassMemberSuffix - ClassConstantCase/ClassConstantPrefix This fixes clang-tidy suggesting plain lower_case names for class static members (e.g. `variable`) and enforces the project style (`variable_`).
Walkthrough新增完整的 Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant LED as LED
participant IMU as BMI088
participant SPI as SPI1
participant USB as USB Vendor
participant Buf as InterruptSafeBuffer
participant CAN as CAN1/CAN2
participant UART as UARTs
App->>LED: init()
App->>SPI: init()
App->>IMU: init()
App->>USB: init()
App->>CAN: init()
App->>UART: init()
loop main loop
App->>USB: try_transmit()
USB->>Buf: pop_batch()/serialize()
Buf-->>USB: batch data
App->>CAN: try_transmit()
CAN->>CAN: dequeue & write TX mailbox
App->>UART: try_transmit()
UART->>UART: start HAL transmit IT
IMU->>SPI: read_async()
SPI->>IMU: TxRx Complete (HAL interrupt)
IMU->>USB: handle_uplink(serializer)
USB->>Buf: allocate() / queue batch
App->>LED: update(tick)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
诗篇
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@firmware/c_board/src/timer/delay.cpp`:
- Around line 18-22: HAL_IncTick currently dereferences led::led which may not
be initialized when SysTick fires during HAL_Init; update HAL_IncTick to guard
the call by checking the Lazy LED object's initialization state before calling
led::led->update(tick) (e.g., use led::led.is_initialized() or a
boolean/operator on the Lazy wrapper), or alternatively ensure the interrupt
masking/InterruptMutex semantics prevent SysTick from reaching HAL_IncTick
before App::App() calls led::led.init(); reference HAL_IncTick, led::led,
led::Led::update and the Lazy init path when making the change.
In `@firmware/c_board/src/uart/uart.hpp`:
- Around line 23-25: The template parameter types are inconsistent: Lazy is
declared as utility::Lazy<Uart, UART_HandleTypeDef*, size_t> while the Uart
constructor uses uint16_t for max_receive_size; change the Uart constructor
signature and any related member/storage from uint16_t to size_t so it matches
Lazy's second template parameter (update explicit Uart(UART_HandleTypeDef*
hal_uart_handle, uint16_t max_receive_size) to use size_t and adjust any uses of
max_receive_size accordingly), ensuring Lazy and Uart use the same size type.
In `@firmware/c_board/src/usb/wcid.cpp`:
- Around line 7-8: 修复 wcid.cpp 注释中的拼写重复：在注释段落（以 "WCID allows a device to be used
by a Windows application almost as soon as it is plugged in..." 开头）将出现的短语 "the
the usual scenario" 改为 "the usual scenario"，保留其余注释不变以消除重复词错误。
In `@firmware/c_board/src/utility/assert.cpp`:
- Around line 7-9: 把 volatile 从指向类型前移到指针本身：当前声明 volatile const char* assert_file
和 volatile const char* assert_function 只让被指向的字符为 volatile，而非指针本身，应改为让指针本身为
volatile（同时保持被指向内容为 const），例如 将 assert_file 和 assert_function 的声明改为 const char *
volatile assert_file = nullptr; const char * volatile assert_function =
nullptr;，保留 volatile unsigned int assert_line
不变；在函数或断言设置处使用这些同名符号以确保调试器能读到文件/函数名。
🧹 Nitpick comments (13)
firmware/c_board/src/usb/wcid.cpp (1)
87-87: 函数返回类型为uint8_t，但返回值使用bool字面量。函数声明返回
uint8_t，内部却使用true/false。虽然在 C++ 中bool可隐式转换为uint8_t，但由于此函数通过extern "C"导出供 C 代码调用，建议考虑直接返回bool（C99+ 支持_Bool/bool），或者改用1/0以提高语义一致性。firmware/c_board/.clangd (1)
4-7: 工具链版本15.2.1在四处硬编码。
.clangd不支持变量替换，所以这里没有好的替代方案。但建议在文件顶部添加注释，提示升级工具链时需同步更新这些路径，以降低未来维护遗漏的风险。firmware/c_board/CMakeLists.txt (2)
65-84:HOST_DEBUGGER=OFF时SOURCE_PATH_MAP仍可生效 — 请确认是否为预期行为。Line 76 的条件
NOT SOURCE_PATH_MAP STREQUAL ""独立于HOST_DEBUGGER。这意味着用户可以在HOST_DEBUGGER=OFF的情况下手动传入-DSOURCE_PATH_MAP=/some/path，路径映射仍然会被应用。如果这是有意为之（允许高级用户手动覆盖），建议添加注释说明；如果不是，应将 Line 76 的条件也纳入HOST_DEBUGGER的判断范围内。
91-98:file(GLOB_RECURSE ...)配合CONFIGURE_DEPENDS是可接受的做法，但需注意局限性。
CONFIGURE_DEPENDS可以在部分生成器（如 Ninja）下检测新增/删除文件并触发重新配置，但 CMake 官方文档仍建议在可行时显式列出源文件。对于嵌入式项目，当前方案是常见实践，此处仅作提示。firmware/c_board/src/led/led.hpp (2)
99-99: 缺少constinit，与其他 Lazy 实例声明不一致。项目中其他 Lazy 全局实例（如
logger.hpp中的inline constinit Logger::Lazy logger;、can.hpp中的inline constinit Can::Lazy can1{...};）均使用了constinit。此处应保持一致，以确保编译器强制执行常量初始化。建议修复
-inline utility::Lazy<Led> led; +inline constinit utility::Lazy<Led> led;
14-18:HAL_TIM_PWM_Start返回值未检查。在
can.hpp的config_can()中，所有 HAL 调用的返回值都通过assert_always检查了。建议此处也保持一致，在初始化阶段捕获潜在的硬件配置错误。建议修复
Led() { - HAL_TIM_PWM_Start(&htim5, TIM_CHANNEL_1); - HAL_TIM_PWM_Start(&htim5, TIM_CHANNEL_2); - HAL_TIM_PWM_Start(&htim5, TIM_CHANNEL_3); + constexpr auto ok = HAL_OK; + core::utility::assert_always(HAL_TIM_PWM_Start(&htim5, TIM_CHANNEL_1) == ok); + core::utility::assert_always(HAL_TIM_PWM_Start(&htim5, TIM_CHANNEL_2) == ok); + core::utility::assert_always(HAL_TIM_PWM_Start(&htim5, TIM_CHANNEL_3) == ok); reset(); }firmware/c_board/src/uart/uart.cpp (2)
1-1: 头文件包含路径不一致其他
.cpp文件均使用完整项目路径引入对应头文件（如"firmware/c_board/src/can/can.hpp"），此处使用了相对路径"uart.hpp"。建议统一为完整路径以保持一致性。建议修改
-#include "uart.hpp" +#include "firmware/c_board/src/uart/uart.hpp"Based on learnings: "In the librmcs project, enforce Google C++ Include Style in all C++ source files (.cpp/.cc). The include order should be: 1) the corresponding header..."
17-28: UART 外设与逻辑通道的映射关系不直观，建议添加注释
huart1映射到uart2，huart6映射到uart1——物理外设编号与逻辑通道名称交叉对应，容易引起混淆。如果这是 PCB 布线导致的正确映射，建议添加简短注释说明原因，方便后续维护。firmware/c_board/src/timer/delay.hpp (1)
33-54:delay模板的溢出保护存在间隙Line 40-42 仅当
sizeof(Rep) > sizeof(uint32_t)时进行运行时断言，但对于sizeof(Rep) == sizeof(uint32_t)且Rep为有符号类型（如int32_t）的情况，负值已在上方处理，不会有问题。不过，如果Rep是uint64_t，断言会生效，但assert在 release 构建中（NDEBUG）会被移除，此时大于UINT32_MAX的值会被静默截断。考虑使用项目中已有的
core::utility::assert_always替代assert，确保 release 构建中也能捕获溢出。建议修改
if constexpr (sizeof(Rep) > sizeof(uint32_t)) { - assert(delay.count() <= std::numeric_limits<uint32_t>::max()); + core::utility::assert_always(delay.count() <= std::numeric_limits<uint32_t>::max()); }firmware/c_board/src/uart/uart.hpp (1)
37-46:written_size在memcpy之前更新——依赖裸机 ISR 原子性。第 44-45 行先 store 了新的
written_size，第 46 行才执行memcpy。如果handle_downlink在 ISR 中被调用（USB CDC 接收回调 → 反序列化 → UART 下行），而try_transmit在主循环中运行，由于单核裸机环境 ISR 运行到完成，这是安全的。但建议加一行简短注释说明此顺序依赖，以防未来维护者在多线程环境中误用。firmware/c_board/src/usb/interrupt_safe_buffer.hpp (1)
49-49:kMask的位置与kBatchCount不相邻，建议放在一起以提高可读性。
kMask当前在allocate()方法之后定义（第 49 行），而与其紧密相关的kBatchCount在第 20 行。将两者放在一起更直观。建议移动 kMask
static constexpr size_t kBatchCount = 8; static_assert(std::has_single_bit(kBatchCount), "Batch count must be a power of 2"); + static constexpr size_t kMask = kBatchCount - 1; constexpr InterruptSafeBuffer() = default; std::span<std::byte> allocate(size_t size) noexcept override { // ... } - static constexpr size_t kMask = kBatchCount - 1;firmware/c_board/src/spi/spi.hpp (2)
92-97:tx_rx_size_类型为size_t，但 HAL SPI 函数期望uint16_t。
tx_rx_size_（第 125 行，size_t）被直接传递给HAL_SPI_TransmitReceive和HAL_SPI_TransmitReceive_IT，这两个函数的Size参数类型为uint16_t（第 64 行、第 77 行）。虽然kMaxTransferSize = 32不会溢出，但这是隐式窄化转换，可能产生编译器警告。建议将 tx_rx_size_ 改为 uint16_t
- size_t tx_rx_size_{0}; + uint16_t tx_rx_size_{0};同时调整
transmit_receive和transmit_receive_async的size参数，或在赋值处显式static_cast：- tx_rx_size_ = size; + tx_rx_size_ = static_cast<uint16_t>(size);Also applies to: 120-125
17-36:SpiModule同时继承Immovable并显式 delete 拷贝/移动操作——存在冗余。
Immovable基类已经禁止了拷贝和移动操作，第 24-27 行的显式 delete 声明是冗余的。如果Immovable的私有继承导致错误信息不够清晰是你保留它们的理由，可以考虑改为public继承Immovable，或者直接移除冗余声明。
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (10)
firmware/c_board/src/utility/interrupt_lock.hpp (1)
32-42:Immovable基类已删除拷贝/移动操作，第 36-39 行的显式delete声明冗余。
core::utility::Immovable基类已明确删除拷贝构造、拷贝赋值、移动构造和移动赋值。由于InterruptLockGuard私有继承自Immovable，这些删除在派生类中会自动继承，第 36-39 行的显式声明可以移除。保留这些声明虽无害，但增加维护成本。建议删除冗余的显式delete声明以简化代码。firmware/c_board/src/spi/spi.hpp (3)
47-54:atomic_flag全部使用relaxed序，在异步路径中缺乏 happens-before 保证。
try_lock/unlock实质上充当互斥锁，保护spi_module_、tx_rx_size_以及tx_buffer/rx_buffer等非原子数据。在异步路径中，锁在线程上下文获取、在 ISR 中释放（通过 module callback 调用unlock）。在单核 Cortex-M 上，中断屏障使得
relaxed在实践中是安全的。但在 C++ 抽象内存模型中，relaxed不为非原子数据的读写建立 happens-before 关系。建议将try_lock改为acquire、unlock改为release，这在 ARM 上不会产生额外开销（dmb已被中断隐含），但能让代码在形式上正确且更具可移植性。♻️ 建议的修改
- bool is_locked() const { return locking_.test(std::memory_order::relaxed); } + bool is_locked() const { return locking_.test(std::memory_order::acquire); } - bool try_lock() { return !locking_.test_and_set(std::memory_order::relaxed); } + bool try_lock() { return !locking_.test_and_set(std::memory_order::acquire); } void unlock() { core::utility::assert_debug_lazy([&]() noexcept { return is_locked(); }); - locking_.clear(std::memory_order::relaxed); + locking_.clear(std::memory_order::release); }
17-36:SpiModule已继承Immovable，显式 delete 拷贝/移动是冗余的。
core::utility::Immovable基类应该已经禁止了拷贝和移动操作，Lines 24-27 的显式删除是多余的。如果是出于文档化意图保留也可以接受，仅作提示。
81-84: 异步回调路径中tx_rx_size_在finish_transfer()之后使用，逻辑上安全但略显脆弱。
finish_transfer()清除了spi_module_但没有清除tx_rx_size_，所以 Line 83 读取的值仍然有效。但如果将来finish_transfer()也重置tx_rx_size_，这里就会出 bug。建议在finish_transfer之前取出tx_rx_size_的值，或者让finish_transfer同时返回 size。♻️ 一种更防御性的写法
void transmit_receive_async_callback() { + auto size = tx_rx_size_; if (auto* module = finish_transfer()) - module->transmit_receive_async_callback(rx_buffer, tx_rx_size_); + module->transmit_receive_async_callback(rx_buffer, size); }firmware/c_board/src/led/led.hpp (1)
40-41:user_controlling_只被设为false，缺少设为true的路径。
user_controlling_在reset()中被设为false，update()中被检查，但没有任何公开方法将其设为true。如果这是尚未实现的功能，建议添加// TODO注释说明；否则，update()中的检查是死代码。Also applies to: 96-96
firmware/c_board/src/uart/uart.hpp (1)
37-50:handle_downlink中written_size先 store 后memcpy存在潜在风险。第 44-45 行先将更新后的
written_size写入 atomic，第 46 行才执行memcpy。如果try_transmit（主循环）在handle_downlink（ISR 上下文）的 store 和 memcpy 之间被调度（例如在多核场景、或未来重构执行上下文时），try_transmit在第 76 行读到的written_size将包含尚未拷贝的数据长度，导致发送未初始化的内容。建议将
memcpy移到written_size.store()之前，并在两者之间加一个 signal fence 保证编译器不会重排：♻️ 建议调整顺序
- transmit_buffer.written_size.store( - static_cast<uint8_t>(written_size + size_allowed), std::memory_order::relaxed); std::memcpy(&transmit_buffer.data[written_size], data.uart_data.data(), size_allowed); + std::atomic_signal_fence(std::memory_order::release); + transmit_buffer.written_size.store( + static_cast<uint8_t>(written_size + size_allowed), std::memory_order::relaxed);firmware/c_board/src/can/can.hpp (2)
73-77: Line 76 使用了 TX 寄存器常量来解析 RX 寄存器。
CAN_TI0R_STID_Pos是发送邮箱标识符寄存器的位偏移常量，此处读取的是接收 FIFO 邮箱的RIR寄存器，应使用CAN_RI0R_STID_Pos。虽然在 STM32F4 上二者数值相同（均为 21），不影响功能，但语义不一致会误导读者。建议修正为 RX 寄存器对应的常量
if (data.is_extended_can_id) { data.can_id = ((CAN_RI0R_EXID | CAN_RI0R_STID) & rir) >> CAN_RI0R_EXID_Pos; } else { - data.can_id = (CAN_RI0R_STID & rir) >> CAN_TI0R_STID_Pos; + data.can_id = (CAN_RI0R_STID & rir) >> CAN_RI0R_STID_Pos; }
34-55:handle_downlink中对can_data.size()仅使用assert_debug校验。Line 47 的
assert_debug在 Release 构建中会被移除。如果上游意外传入超过 8 字节的数据，std::memcpy会越界写入mailbox.data（仅 8 字节）。考虑到这是固件代码且 CAN 协议层面保证 ≤ 8，风险较低，但如果 downlink 数据来自外部（USB），建议提升为assert_always或添加std::min防护。建议加固数据长度校验
- core::utility::assert_debug(data.can_data.size() <= 8); - mailbox.data_length_and_timestamp = data.can_data.size(); - if (!data.can_data.empty()) - std::memcpy(mailbox.data, data.can_data.data(), data.can_data.size()); + auto len = std::min(data.can_data.size(), size_t{8}); + mailbox.data_length_and_timestamp = len; + if (len > 0) + std::memcpy(mailbox.data, data.can_data.data(), len);firmware/c_board/src/usb/interrupt_safe_buffer.hpp (2)
27-49:allocate()设计合理，符合裸机 ISR 安全模型。
out_在循环外加载一次（与已知设计意图一致），-1间距保护了已弹出但未释放的批次不被 ISR 覆写。compare_exchange_weak在单 ISR 场景下本质上总是成功，但作为防御性编码是合理的。有一处值得留意的细节：
writeable = kBatchCount - readable - 1中的-1是保证pop_batch()返回后、release_batch()调用前，正在被主线程读取的批次不会被 ISR 重用的关键。建议在此处添加一行注释说明这个-1的安全语义，方便后续维护者理解。
79-94:pop_batch()逻辑正确，signal_fence用法适合裸机场景。
atomic_signal_fence(release)在单核裸机上防止编译器将out_.store重排到batch.empty()检查之前，配合 Cortex-M 中断进出的隐式屏障，确保了 ISR 与主线程之间的可见性。需要注意的是：
pop_batch()在返回批次指针之前就已更新了out_（第 91 行），这意味着调用方在消费完数据并调用release_batch()之前，ISR 已经"看到"了新的out_值。安全性完全依赖于allocate()中writeable = kBatchCount - readable - 1的-1间距保护。当前kBatchCount = 8时余量充足，但如果未来缩小kBatchCount（如改为 2），安全边际会非常紧，建议在kBatchCount的static_assert中加一个最小值约束（例如>= 4）。🛡️ 建议添加最小批次数约束
static constexpr size_t kBatchCount = 8; - static_assert(std::has_single_bit(kBatchCount), "Batch count must be a power of 2"); + static_assert(std::has_single_bit(kBatchCount), "Batch count must be a power of 2"); + static_assert(kBatchCount >= 4, "Batch count must be >= 4 to maintain safety margin");
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
firmware/c_board/src/uart/uart.hpp (3)
53-79:trigger_hal_receive()返回值在try_transmit中被静默忽略。Line 59 调用
trigger_hal_receive()但未检查返回值，而构造函数（Line 29）和handle_uplink（Line 98）均通过assert_always对返回值进行断言。虽然注释说明这是一种轮询补偿机制，但如果接收恢复持续失败，当前逻辑不会有任何提示，可能导致 UART 永久停止接收而无感知。建议至少在失败时通过 LED 信号提示（类似
downlink_buffer_full的做法），或者添加注释说明为何此处故意忽略失败。
32-51: 双缓冲写入逻辑：隐含单写者假设。
handle_downlink的 load-compute-memcpy-store 序列不是原子的。如果两个不同优先级的中断（或主循环与中断）同时调用handle_downlink，会出现经典的 lost-update 竞争——两者读到相同的written_size，写入同一偏移，最终只有一个 store 生效。如果当前设计确实保证
handle_downlink只由单一上下文调用，建议添加简短注释说明这一前提假设，便于后续维护。
127-129: 逻辑名称与 HAL 句柄映射关系不直观。
uart1绑定huart6，uart2绑定huart1——逻辑编号与 USART 外设编号不对应。虽然这可能是硬件设计决定的，但建议添加简短注释说明映射关系（例如引脚/连接器对应），减少后续维护者的困惑。
qzhhhi
changed the title
…ibrmcs v3 (WIP) (#19) Migration source - This migration is based on the c_board firmware from the legacy rmcs_slave repository (baseline commit: a6ca5c3). Completed in this stage - Migrated the build system from xmake to CMake, added presets/toolchain support, and integrated with the existing lint/CI workflow. - Restructured the firmware layout from app/utility to src and aligned it with librmcs naming and project conventions. - Migrated the communication layer from the legacy HAL CDC field protocol to core::protocol (serializer/deserializer), reconnecting CAN/UART/IMU uplink/downlink paths. - Refactored and modernized key modules, including SPI/UART/CAN/USB. - Updated and synchronized CubeMX/HAL project files. Not yet completed - Windows auto-recognition is not supported at this stage (WCID path not wired in yet). - USB EP0 OUT continuation-length handling still needs to be unified and fixed in follow-up migration work. - SPI/UART have not been migrated to DMA-based transfers. - No hardware bring-up or on-device regression testing has been performed yet. Next steps - Continue closing migration gaps and run behavioral regression to align with legacy firmware behavior. - Follow the rmcs_board path to migrate to TinyUSB and phase out HAL CDC. - Restore and validate Windows compatibility after TinyUSB migration is converged.
1 participant
c_board 固件项目初始化（更新）
概述
本 PR 为 c_board 固件引入完整工程骨架、构建/CI 配置与丰富固件子系统实现，包含构建预设、静态分析配置、外设驱动、通信链路与若干实用工具，影响范围较大，需全面审查。
关键变更
构建与 CI
应用框架
外设与通信驱动
传感器驱动（BMI088）
LED、日志与断言
实用工具
API 与导出实体（摘要）
技术要点与设计取舍
审查建议（优先级）
变更规模与影响