Uh oh!
There was an error while loading. Please reload this page.
feat(firmware/rmcs_board)!: Split rmcs_board into Pro and Lite variants - #44
Merged
Conversation
Walkthrough引入 pro/lite 两个板级变体并重构外设初始化与中断：将分散的 ISR 与寄存器访问迁移到板级接口与数组化驱动（can_array/uart_array/spi_bmi088），新增 constexpr GpioPin 抽象，移除对 UART DBUS 下行的直接转发。 Changes
sequenceDiagram
participant Host
participant USBVendor as "USB Vendor"
participant Vendor as "Vendor::handler"
participant Board as "board (board_app)"
participant UART as "uart::uart_array[idx]"
rect rgba(200,200,255,0.5)
Host->>USBVendor: 发送下行数据包
end
rect rgba(200,255,200,0.5)
USBVendor->>Vendor: 解串并调用 uart_deserialized_callback(data_id, data)
Vendor->>Board: 查询板级状态（如 kUserHsFsSwitchPin）
end
rect rgba(255,200,200,0.5)
Vendor->>UART: 按 data_id 映射索引 -> uart_array[index]->handle_downlink(data)
UART-->>Vendor: 处理完成
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
firmware/rmcs_board/app/src/spi/bmi088/accel.cpp (1)
1-5: 建议在这里直接包含board_app.hpp。这个 IRQ 入口是板级公开符号，但当前定义没有包含它的声明头；如果
board_app.hpp里的签名后续调整，这个翻译单元不会得到编译期约束。建议像firmware/rmcs_board/app/src/spi/spi.cpp一样把board_app.hpp纳入这里。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/spi/bmi088/accel.cpp` around lines 1 - 5, The IRQ handler bmi088_accel_dataready_irq_handler is a board-level public symbol but this translation unit doesn't include the board_app.hpp declaration; add an `#include` for board_app.hpp at the top of accel.cpp (as done in spi.cpp) so the handler signature and any future changes to board-level declarations are checked at compile time and the symbol is correctly declared.firmware/rmcs_board/boards/pro/app/board_app.cpp (1)
20-41: 未知外设基址不要静默返回0。这些 helper 的默认分支会返回
0，而can.hpp/uart.hpp会继续把它当时钟频率喂给底层初始化。板级映射一旦写错，故障会延后成难定位的外设起不来，而不是在 bring-up 阶段立即暴露。建议在默认分支assert_always(false)，return 0只保留给编译器。可参考的最小修正
+#include "core/src/utility/assert.hpp"+ uint32_t init_can_clock(MCAN_Type* ptr) { if (ptr == HPM_MCAN0) { clock_add_to_group(clock_can0, 0); clock_set_source_divider(clock_can0, clk_src_pll1_clk0, 10); return clock_get_frequency(clock_can0); @@ - return 0;+ core::utility::assert_always(false);+ return 0; }对
init_uart_clock()和init_spi_clock()同样套用这一模式即可。Also applies to: 44-70, 72-86, 90-128, 130-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/boards/pro/app/board_app.cpp` around lines 20 - 41, init_can_clock currently silently returns 0 for unknown MCAN_Type pointers which lets higher-level init (e.g., can.hpp/uart.hpp) proceed with an invalid clock frequency; change the default branch to call assert_always(false) (or equivalent runtime-fatal/assert) to fail fast and keep a trailing return 0 only for the compiler, and apply the same pattern to init_uart_clock and init_spi_clock so any incorrect board base mapping immediately triggers an assertion rather than silently returning 0.firmware/rmcs_board/app/src/uart/uart.hpp (1)
39-49: DBUS 的 RX-only 约束还没有被类型层面编码。
uart_dbus仍然实例化完整Uart/TxBuffer，kDbusBoardConfig也依然要求dma_src_tx。当前如果上层不再调用发送路径，运行时确实可用；但这会把“只接收”的约束留在约定里，后续很容易被误接回发送逻辑。更稳的做法是给 DBUS 单独建一个 RX-only 变体，或把 TX 资源做成可选配置。Also applies to: 104-109, 134-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/uart/uart.hpp` around lines 39 - 49, The DBUS device should be encoded as RX-only at the type level instead of instantiating full Uart/TX resources; update the Uart construction and board config handling so TX is optional or provide a dedicated RxOnlyUart variant. Change the Uart constructor (and Lazy<Uart,...> usage) to accept an optional TxBuffer/tx DMA source (e.g., std::optional or a separate overload) and modify kDbusBoardConfig usage to allow dma_src_tx to be empty for DBUS; alternatively add a new RxOnlyUart class that constructs only RxBuffer and uart_base_ and calls init_uart without allocating/initializing TxBuffer, and replace uart_dbus instantiation to use that Rx-only type.
🤖 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/rmcs_board/app/src/usb/vendor.hpp`:
- Around line 108-113: Vendor handling assumes BOARD_UART2 is enabled whenever
BOARD_UART3 is enabled, but that dependency is implicit and causes out-of-bounds
accesses to uart::uart_array and kBoardConfigs; fix by adding a compile-time
validation such as a preprocessor check (e.g., in uart.hpp or vendor.hpp) that
emits an `#error` when BOARD_UART3 is defined but BOARD_UART2 is not, or
alternatively replace the fixed-index accesses in vendor.hpp (the case for
data::DataId::kUart2 / kUart3) with a safe dynamic mapping/lookup that checks
uart::uart_array size before indexing to eliminate the fragile dependency.
In `@firmware/rmcs_board/boards/lite/board.c`:
- Around line 109-116: The code sets sysctl_resource_xtal mode but waits on the
wrong busy flag (sysctl_resource_usb0); update the busy-wait to check the XTAL
resource instead. Specifically, in the sequence around
sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, 0x03) and the
subsequent restore call, replace the call to
sysctl_resource_target_is_busy(HPM_SYSCTL, sysctl_resource_usb0) with
sysctl_resource_target_is_busy(HPM_SYSCTL, sysctl_resource_xtal) so the code
correctly waits for the XTAL mode switch to complete before restoring the
original mode.
In `@firmware/rmcs_board/boards/pro/board.c`:
- Around line 109-117: 当前代码在切换 sysctl_resource_xtal 模式后错误地等待
sysctl_resource_usb0 的 busy 标志，可能导致 XTAL 模式切换重叠并引起启动不稳定；请将等待调用从
sysctl_resource_target_is_busy(HPM_SYSCTL, sysctl_resource_usb0) 改为等待对应的
sysctl_resource_xtal（即调用 sysctl_resource_target_is_busy(HPM_SYSCTL,
sysctl_resource_xtal)），确保在调用 sysctl_resource_target_set_mode(HPM_SYSCTL,
sysctl_resource_xtal, ...) 后正确等待该资源完成再恢复原始模式 tmp。
In `@firmware/rmcs_board/boards/pro/board.h`:
- Line 35: The declaration for board_init_usb in board.h uses the old C-style
prototype; update the prototype to the C11 form by changing the declaration of
board_init_usb to use an explicit void parameter (i.e., declare board_init_usb
as void board_init_usb(void);) so it matches the implementation of
board_init_usb and the Lite board header; ensure no other declarations or extern
"C" blocks are affected.
In `@host/include/librmcs/agent/rmcs_board_lite.hpp`:
- Around line 68-90: The methods can_receive_callback(data::DataId, const
data::CanDataView&) and uart_receive_callback(data::DataId, const
data::UartDataView&) are marked only as final but should also include the
override specifier to match other overridden methods; update their declarations
to use "override final" (i.e., change the signatures of can_receive_callback and
uart_receive_callback to include override before final) so they match the
pattern used by can0_receive_callback/can1_receive_callback/... and satisfy
modernize-use-override.
---
Nitpick comments:
In `@firmware/rmcs_board/app/src/spi/bmi088/accel.cpp`:
- Around line 1-5: The IRQ handler bmi088_accel_dataready_irq_handler is a
board-level public symbol but this translation unit doesn't include the
board_app.hpp declaration; add an `#include` for board_app.hpp at the top of
accel.cpp (as done in spi.cpp) so the handler signature and any future changes
to board-level declarations are checked at compile time and the symbol is
correctly declared.
In `@firmware/rmcs_board/app/src/uart/uart.hpp`:
- Around line 39-49: The DBUS device should be encoded as RX-only at the type
level instead of instantiating full Uart/TX resources; update the Uart
construction and board config handling so TX is optional or provide a dedicated
RxOnlyUart variant. Change the Uart constructor (and Lazy<Uart,...> usage) to
accept an optional TxBuffer/tx DMA source (e.g., std::optional or a separate
overload) and modify kDbusBoardConfig usage to allow dma_src_tx to be empty for
DBUS; alternatively add a new RxOnlyUart class that constructs only RxBuffer and
uart_base_ and calls init_uart without allocating/initializing TxBuffer, and
replace uart_dbus instantiation to use that Rx-only type.
In `@firmware/rmcs_board/boards/pro/app/board_app.cpp`:
- Around line 20-41: init_can_clock currently silently returns 0 for unknown
MCAN_Type pointers which lets higher-level init (e.g., can.hpp/uart.hpp) proceed
with an invalid clock frequency; change the default branch to call
assert_always(false) (or equivalent runtime-fatal/assert) to fail fast and keep
a trailing return 0 only for the compiler, and apply the same pattern to
init_uart_clock and init_spi_clock so any incorrect board base mapping
immediately triggers an assertion rather than silently returning 0.
🪄 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: a56496b5-b8b5-4fce-9206-e43218b10adc
📒 Files selected for processing (40)
firmware/c_board/app/src/usb/vendor.hppfirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/can/can.cppfirmware/rmcs_board/app/src/can/can.hppfirmware/rmcs_board/app/src/gpio/gpio.cppfirmware/rmcs_board/app/src/gpio/gpio.hppfirmware/rmcs_board/app/src/gpio/gpio_pin.hppfirmware/rmcs_board/app/src/spi/bmi088/accel.cppfirmware/rmcs_board/app/src/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/rmcs_board/app/src/spi/bmi088/gyro.cppfirmware/rmcs_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/app/src/spi/spi.cppfirmware/rmcs_board/app/src/spi/spi.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/boards/lite/CMakeLists.txtfirmware/rmcs_board/boards/lite/app/board_app.cppfirmware/rmcs_board/boards/lite/app/board_app.hppfirmware/rmcs_board/boards/lite/board.cfirmware/rmcs_board/boards/lite/board.hfirmware/rmcs_board/boards/lite/lite.yamlfirmware/rmcs_board/boards/pro/CMakeLists.txtfirmware/rmcs_board/boards/pro/app/board_app.cppfirmware/rmcs_board/boards/pro/app/board_app.hppfirmware/rmcs_board/boards/pro/board.cfirmware/rmcs_board/boards/pro/board.hfirmware/rmcs_board/boards/pro/pro.yamlfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpphost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board_lite.hpphost/include/librmcs/agent/rmcs_board_pro.hpp
💤 Files with no reviewable changes (4)
- firmware/rmcs_board/app/src/gpio/gpio.hpp
- firmware/c_board/app/src/usb/vendor.hpp
- host/include/librmcs/agent/c_board.hpp
- firmware/rmcs_board/app/src/gpio/gpio.cpp
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
qzhhhi added a commit
that referenced
this pull request
Apr 14, 2026
…ts (#44) - Add dedicated Pro and Lite board definitions for rmcs_board and move board-specific CAN, UART, SPI, GPIO, and USB setup into per-board implementations. - Update the rmcs_board superbuild to select custom boards through BOARD_SEARCH_PATH, assign per-board USB product IDs, and support bootloader auto/manual startup behavior. BREAKING CHANGE: Rename librmcs::agent::RmcsBoard to RmcsBoardPro and add librmcs::agent::RmcsBoardLite.
creeper5820
commented
Apr 14, 2026
Contributor
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
firmware/rmcs_board/app/src/can/can.hpp (1)
25-28: 硬件基址别用uint32_t保存。
HardwareConfig::base随后会被重新解释成MCAN_Type*，这里把地址压成uint32_t实际上把实现绑死在当前 32 位地址宽度上了；一旦后面有 host-side tooling、不同位宽 target，或者相关宏的类型发生变化，就会出现静默截断。用uintptr_t会更稳，也和同 PR 新增的GpioPin做法一致。♻️ 可参考的修改
struct HardwareConfig { - uint32_t base;+ uintptr_t base; uint32_t irq_num; }; @@ - : data_id_(data_id)- , can_base_(reinterpret_cast<MCAN_Type*>(board_config.base)) {+ : data_id_(data_id)+ , can_base_(reinterpret_cast<MCAN_Type*>(board_config.base)) { @@ constexpr HardwareConfig kBoardConfigs[] = { - {.base = BOARD_CAN0(HPM_MCAN, _BASE), .irq_num = BOARD_CAN0(IRQn_MCAN, )},- {.base = BOARD_CAN1(HPM_MCAN, _BASE), .irq_num = BOARD_CAN1(IRQn_MCAN, )},- {.base = BOARD_CAN2(HPM_MCAN, _BASE), .irq_num = BOARD_CAN2(IRQn_MCAN, )},- {.base = BOARD_CAN3(HPM_MCAN, _BASE), .irq_num = BOARD_CAN3(IRQn_MCAN, )},+ {.base = static_cast<uintptr_t>(BOARD_CAN0(HPM_MCAN, _BASE)),+ .irq_num = BOARD_CAN0(IRQn_MCAN, )},+ {.base = static_cast<uintptr_t>(BOARD_CAN1(HPM_MCAN, _BASE)),+ .irq_num = BOARD_CAN1(IRQn_MCAN, )},+ {.base = static_cast<uintptr_t>(BOARD_CAN2(HPM_MCAN, _BASE)),+ .irq_num = BOARD_CAN2(IRQn_MCAN, )},+ {.base = static_cast<uintptr_t>(BOARD_CAN3(HPM_MCAN, _BASE)),+ .irq_num = BOARD_CAN3(IRQn_MCAN, )}, };Also applies to: 35-38, 121-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/can/can.hpp` around lines 25 - 28, The HardwareConfig::base field (and any other address-storage fields in this file) is using uint32_t which can truncate on non-32-bit address spaces; change the type to uintptr_t for HardwareConfig::base and similarly replace uint32_t with uintptr_t for the other address fields referenced around the other occurrences (the fields at the sections noted by the reviewer, e.g., the struct at lines ~35-38 and the definitions around ~121-125) so addresses are stored using an integer type sized to a pointer (matching the existing GpioPin approach).
🤖 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/rmcs_board/boards/pro/app/board_app.cpp`:
- Around line 107-127: The init_uart function currently treats HPM_UART4 like
other UARTs but never configures its IOC/PAD so callers get a non-zero clock
from init_uart_clock(ptr) and a false “success”; update init_uart to explicitly
handle HPM_UART4 by adding an else-if branch for ptr == HPM_UART4 that either
(a) sets the proper HPM_IOC->PAD[IOC_PAD_...] .FUNC_CTL entries for UART4
TX/RX/DE pins (i.e. add the missing pinmux similar to the HPM_UART1/HPM_UART3
branches), or (b) if UART4 pinmux is not supported, return an explicit
failure/abort (e.g. return 0 or assert) instead of falling through to
init_uart_clock(ptr); ensure you do not call init_uart_clock(ptr) for HPM_UART4
when rejecting it.
---
Nitpick comments:
In `@firmware/rmcs_board/app/src/can/can.hpp`:
- Around line 25-28: The HardwareConfig::base field (and any other
address-storage fields in this file) is using uint32_t which can truncate on
non-32-bit address spaces; change the type to uintptr_t for HardwareConfig::base
and similarly replace uint32_t with uintptr_t for the other address fields
referenced around the other occurrences (the fields at the sections noted by the
reviewer, e.g., the struct at lines ~35-38 and the definitions around ~121-125)
so addresses are stored using an integer type sized to a pointer (matching the
existing GpioPin approach).
🪄 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: 8c5176d1-2e56-4ae9-a25a-f1d8c8d73fea
📒 Files selected for processing (40)
firmware/c_board/app/src/usb/vendor.hppfirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/can/can.cppfirmware/rmcs_board/app/src/can/can.hppfirmware/rmcs_board/app/src/gpio/gpio.cppfirmware/rmcs_board/app/src/gpio/gpio.hppfirmware/rmcs_board/app/src/gpio/gpio_pin.hppfirmware/rmcs_board/app/src/spi/bmi088/accel.cppfirmware/rmcs_board/app/src/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/rmcs_board/app/src/spi/bmi088/gyro.cppfirmware/rmcs_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/app/src/spi/spi.cppfirmware/rmcs_board/app/src/spi/spi.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/boards/lite/CMakeLists.txtfirmware/rmcs_board/boards/lite/app/board_app.cppfirmware/rmcs_board/boards/lite/app/board_app.hppfirmware/rmcs_board/boards/lite/board.cfirmware/rmcs_board/boards/lite/board.hfirmware/rmcs_board/boards/lite/lite.yamlfirmware/rmcs_board/boards/pro/CMakeLists.txtfirmware/rmcs_board/boards/pro/app/board_app.cppfirmware/rmcs_board/boards/pro/app/board_app.hppfirmware/rmcs_board/boards/pro/board.cfirmware/rmcs_board/boards/pro/board.hfirmware/rmcs_board/boards/pro/pro.yamlfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpphost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board_lite.hpphost/include/librmcs/agent/rmcs_board_pro.hpp
💤 Files with no reviewable changes (4)
- firmware/c_board/app/src/usb/vendor.hpp
- firmware/rmcs_board/app/src/gpio/gpio.hpp
- host/include/librmcs/agent/c_board.hpp
- firmware/rmcs_board/app/src/gpio/gpio.cpp
✅ Files skipped from review due to trivial changes (10)
- firmware/rmcs_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.cpp
- firmware/rmcs_board/boards/lite/CMakeLists.txt
- firmware/rmcs_board/boards/pro/pro.yaml
- firmware/rmcs_board/boards/lite/board.h
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/app/src/can/can.cpp
- host/include/librmcs/agent/rmcs_board_pro.hpp
- host/include/librmcs/agent/rmcs_board_lite.hpp
- firmware/rmcs_board/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (14)
- firmware/rmcs_board/app/src/spi/bmi088/gyro.cpp
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/bootloader/src/main.cpp
- firmware/rmcs_board/boards/pro/CMakeLists.txt
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/boards/pro/board.h
- firmware/rmcs_board/boards/lite/lite.yaml
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/app/src/spi/spi.hpp
- firmware/rmcs_board/bootloader/CMakeLists.txt
- firmware/rmcs_board/boards/pro/board.c
- firmware/rmcs_board/boards/lite/app/board_app.hpp
Uh oh!
There was an error while loading. Please reload this page.
qzhhhi added a commit
that referenced
this pull request
Apr 14, 2026
…ts (#44) - Add dedicated Pro and Lite board definitions for rmcs_board and move board-specific CAN, UART, SPI, GPIO, and USB setup into per-board implementations. - Update the rmcs_board superbuild to select custom boards through BOARD_SEARCH_PATH, assign per-board USB product IDs, and support bootloader auto/manual startup behavior. - Build and package separate Pro and Lite rmcs_board firmware artifacts in CI. BREAKING CHANGE: Rename librmcs::agent::RmcsBoard to RmcsBoardPro and add librmcs::agent::RmcsBoardLite.
qzhhhi added a commit
that referenced
this pull request
Apr 14, 2026
…ts (#44) - Add dedicated Pro and Lite board definitions for rmcs_board and move board-specific CAN, UART, SPI, GPIO, and USB setup into per-board implementations. - Update the rmcs_board superbuild to select custom boards through BOARD_SEARCH_PATH, assign per-board USB product IDs, and support bootloader auto/manual startup behavior. - Build and package separate Pro and Lite rmcs_board firmware artifacts in CI. BREAKING CHANGE: Rename librmcs::agent::RmcsBoard to RmcsBoardPro and add librmcs::agent::RmcsBoardLite.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
firmware/rmcs_board/boards/pro/app/board_app.cpp (1)
107-127:⚠️ Potential issue | 🟡 Minor不要让
HPM_UART4走成“初始化成功”。Line 127 现在会对未做任何 IOC/PAD 配置的
HPM_UART4继续调用init_uart_clock(ptr)并返回非零时钟值。这样一旦上层误传HPM_UART4，驱动会表现为初始化成功，但 TX/RX 实际不可用，排查会很隐蔽。建议在不支持的分支直接返回0（或断言失败），只对已完成 pinmux 的 UART 继续开时钟。🛠️ 建议修改
} else if (ptr == HPM_UART5) { HPM_IOC->PAD[IOC_PAD_PA23].FUNC_CTL = IOC_PA23_FUNC_CTL_UART5_TXD; HPM_IOC->PAD[IOC_PAD_PA22].FUNC_CTL = IOC_PA22_FUNC_CTL_UART5_RXD; + } else {+ return 0; } return init_uart_clock(ptr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/boards/pro/app/board_app.cpp` around lines 107 - 127, The init_uart function currently falls through and calls init_uart_clock(ptr) for unsupported UARTs (e.g., HPM_UART4), causing a misleading "initialized" state; modify init_uart (the function handling HPM_UART0/HPM_UART1/HPM_UART2/HPM_UART3/HPM_UART5) to explicitly handle unsupported values (including HPM_UART4) by returning 0 (or triggering an assert/fail) instead of calling init_uart_clock, so only branches that set up IOC/PAD perform the clock initialization and return a non-zero clock handle.
🧹 Nitpick comments (2)
Dockerfile.build_firmware (1)
10-19: 可选：去掉单元素board循环并消除rmcs_board文件名前缀硬编码当前
for board in rmcs_board只有一个元素，且拷贝文件名仍写死rmcs_board_*。可小幅重构以降低后续扩展/改名成本。♻️ 可选重构示例
- for board in rmcs_board; do \- for variant in pro lite; do \- build_dir="/tmp/build-${board}-${variant}"; \- artifact_name="${board}-${variant}"; \- cmake --preset release -S "firmware/${board}" -B "${build_dir}" -DBOARD="${variant}"; \+ board="rmcs_board"; \+ for variant in pro lite; do \+ build_dir="/tmp/build-${board}-${variant}"; \+ artifact_name="${board}-${variant}"; \+ cmake --preset release -S "firmware/${board}" -B "${build_dir}" -DBOARD="${variant}"; \ cmake --build "${build_dir}"; \ - cp "${build_dir}/bootloader/output/rmcs_board_bootloader.elf" \+ cp "${build_dir}/bootloader/output/${board}_bootloader.elf" \ "/output/librmcs-bootloader-${artifact_name}.elf"; \ - cp "${build_dir}/app/output/rmcs_board_app.dfu" \+ cp "${build_dir}/app/output/${board}_app.dfu" \ "/output/librmcs-firmware-${artifact_name}-${LIBRMCS_PROJECT_VERSION}.dfu"; \ - done; \- done; \+ done; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.build_firmware` around lines 10 - 19, Loop currently iterates over a single literal "rmcs_board" and still hardcodes filenames with the rmcs_board prefix; change the loop to use a meaningful variable or remove the redundant loop and make file names and cmake sources use the board variable dynamically so renaming/adding boards is easy. Replace occurrences of the literal rmcs_board in the cmake -S and cp source paths and in the copied filenames (the rmcs_board_bootloader.elf and rmcs_board_app.dfu targets) to reference the variable used for the board (e.g., "${board}") or derive artifact_name from that variable so "${board}-${variant}" drives both build_dir and the cp destination names; keep artifact_name and build_dir logic intact but remove the single-element loop or generalize it to multiple board names.firmware/rmcs_board/app/src/can/can.hpp (1)
128-135: 建议使用std::size计算数组大小。
kCanCount的计算可以使用更现代的std::size()替代手动的sizeof除法，提高可读性和类型安全性。♻️ 建议的修改
-constexpr size_t kCanCount = sizeof(can_array) / sizeof(can_array[0]);+constexpr size_t kCanCount = std::size(can_array);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/can/can.hpp` around lines 128 - 135, Replace the manual sizeof division used to compute kCanCount with std::size to improve clarity and safety: change the computation to use std::size(can_array) and ensure the header that provides std::size (e.g., <iterator> or the appropriate C++ standard header used in the project) is included if not already; update the symbol kCanCount and references to it accordingly while leaving the can_array initialization unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@firmware/rmcs_board/boards/pro/app/board_app.cpp`:
- Around line 107-127: The init_uart function currently falls through and calls
init_uart_clock(ptr) for unsupported UARTs (e.g., HPM_UART4), causing a
misleading "initialized" state; modify init_uart (the function handling
HPM_UART0/HPM_UART1/HPM_UART2/HPM_UART3/HPM_UART5) to explicitly handle
unsupported values (including HPM_UART4) by returning 0 (or triggering an
assert/fail) instead of calling init_uart_clock, so only branches that set up
IOC/PAD perform the clock initialization and return a non-zero clock handle.
---
Nitpick comments:
In `@Dockerfile.build_firmware`:
- Around line 10-19: Loop currently iterates over a single literal "rmcs_board"
and still hardcodes filenames with the rmcs_board prefix; change the loop to use
a meaningful variable or remove the redundant loop and make file names and cmake
sources use the board variable dynamically so renaming/adding boards is easy.
Replace occurrences of the literal rmcs_board in the cmake -S and cp source
paths and in the copied filenames (the rmcs_board_bootloader.elf and
rmcs_board_app.dfu targets) to reference the variable used for the board (e.g.,
"${board}") or derive artifact_name from that variable so "${board}-${variant}"
drives both build_dir and the cp destination names; keep artifact_name and
build_dir logic intact but remove the single-element loop or generalize it to
multiple board names.
In `@firmware/rmcs_board/app/src/can/can.hpp`:
- Around line 128-135: Replace the manual sizeof division used to compute
kCanCount with std::size to improve clarity and safety: change the computation
to use std::size(can_array) and ensure the header that provides std::size (e.g.,
<iterator> or the appropriate C++ standard header used in the project) is
included if not already; update the symbol kCanCount and references to it
accordingly while leaving the can_array initialization unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 048c1410-5bc2-43c1-913f-88bbee6ed8fd
📒 Files selected for processing (41)
Dockerfile.build_firmwarefirmware/c_board/app/src/usb/vendor.hppfirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/can/can.cppfirmware/rmcs_board/app/src/can/can.hppfirmware/rmcs_board/app/src/gpio/gpio.cppfirmware/rmcs_board/app/src/gpio/gpio.hppfirmware/rmcs_board/app/src/gpio/gpio_pin.hppfirmware/rmcs_board/app/src/spi/bmi088/accel.cppfirmware/rmcs_board/app/src/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/rmcs_board/app/src/spi/bmi088/gyro.cppfirmware/rmcs_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/app/src/spi/spi.cppfirmware/rmcs_board/app/src/spi/spi.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/boards/lite/CMakeLists.txtfirmware/rmcs_board/boards/lite/app/board_app.cppfirmware/rmcs_board/boards/lite/app/board_app.hppfirmware/rmcs_board/boards/lite/board.cfirmware/rmcs_board/boards/lite/board.hfirmware/rmcs_board/boards/lite/lite.yamlfirmware/rmcs_board/boards/pro/CMakeLists.txtfirmware/rmcs_board/boards/pro/app/board_app.cppfirmware/rmcs_board/boards/pro/app/board_app.hppfirmware/rmcs_board/boards/pro/board.cfirmware/rmcs_board/boards/pro/board.hfirmware/rmcs_board/boards/pro/pro.yamlfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpphost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board_lite.hpphost/include/librmcs/agent/rmcs_board_pro.hpp
💤 Files with no reviewable changes (4)
- firmware/c_board/app/src/usb/vendor.hpp
- firmware/rmcs_board/app/src/gpio/gpio.hpp
- host/include/librmcs/agent/c_board.hpp
- firmware/rmcs_board/app/src/gpio/gpio.cpp
✅ Files skipped from review due to trivial changes (8)
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.cpp
- firmware/rmcs_board/boards/pro/pro.yaml
- firmware/rmcs_board/boards/lite/lite.yaml
- firmware/rmcs_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/boards/lite/CMakeLists.txt
- firmware/rmcs_board/boards/pro/board.h
- firmware/rmcs_board/boards/lite/board.h
🚧 Files skipped from review as they are similar to previous changes (14)
- firmware/rmcs_board/app/src/spi/bmi088/gyro.cpp
- firmware/rmcs_board/bootloader/src/usb/dfu.hpp
- firmware/rmcs_board/app/CMakeLists.txt
- firmware/rmcs_board/boards/pro/CMakeLists.txt
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/bootloader/src/main.cpp
- firmware/rmcs_board/app/src/can/can.cpp
- firmware/rmcs_board/boards/lite/board.c
- host/include/librmcs/agent/rmcs_board_pro.hpp
- firmware/rmcs_board/boards/pro/app/board_app.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/CMakeLists.txt
- host/include/librmcs/agent/rmcs_board_lite.hpp
…ts (#44) - Add dedicated Pro and Lite board definitions for rmcs_board and move board-specific CAN, UART, SPI, GPIO, and USB setup into per-board implementations. - Update the rmcs_board superbuild to select custom boards through BOARD_SEARCH_PATH, assign per-board USB product IDs, and support bootloader auto/manual startup behavior. - Build and package separate Pro and Lite rmcs_board firmware artifacts in CI. BREAKING CHANGE: Rename librmcs::agent::RmcsBoard to RmcsBoardPro and add librmcs::agent::RmcsBoardLite.
- Remove the DBUS downlink path from c_board and rmcs_board firmware. - Drop dbus_transmit() from the corresponding host agents so the software interface matches the hardware direction. BREAKING CHANGE: Remove dbus_transmit() from CBoard, RmcsBoardPro and RmcsBoardLite.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
firmware/rmcs_board/bootloader/CMakeLists.txt (1)
53-56: 可选：对BOARD_BOOTLOADER_MODE先做小写归一化，减少误用。当前只接受小写字符串；可在校验前统一转小写，提高命令行传参容错性。
♻️ 可选改动示例
+string(TOLOWER "${BOARD_BOOTLOADER_MODE}" BOARD_BOOTLOADER_MODE_NORMALIZED)-if(NOT "${BOARD_BOOTLOADER_MODE}" STREQUAL "auto"- AND NOT "${BOARD_BOOTLOADER_MODE}" STREQUAL "manual")+if(NOT "${BOARD_BOOTLOADER_MODE_NORMALIZED}" STREQUAL "auto"+ AND NOT "${BOARD_BOOTLOADER_MODE_NORMALIZED}" STREQUAL "manual") message(FATAL_ERROR "BOARD_BOOTLOADER_MODE must be auto or manual") endif() -if("${BOARD_BOOTLOADER_MODE}" STREQUAL "auto")+if("${BOARD_BOOTLOADER_MODE_NORMALIZED}" STREQUAL "auto") set(bootloader_mode_auto 1) else() set(bootloader_mode_auto 0) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/CMakeLists.txt` around lines 53 - 56, 在校验 BOARD_BOOTLOADER_MODE 前先将其归一化为小写以提高容错性：使用 CMake 的 string(TOLOWER ...) 将 BOARD_BOOTLOADER_MODE 转为新的变量（例如 board_bootloader_mode_lc），然后用该小写变量与 "auto" 和 "manual" 做比较并在不匹配时触发 message(FATAL_ERROR ...)。在说明中保留原变量名 BOARD_BOOTLOADER_MODE 以便追踪调用处。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@firmware/rmcs_board/bootloader/CMakeLists.txt`:
- Around line 53-56: 在校验 BOARD_BOOTLOADER_MODE 前先将其归一化为小写以提高容错性：使用 CMake 的
string(TOLOWER ...) 将 BOARD_BOOTLOADER_MODE 转为新的变量（例如
board_bootloader_mode_lc），然后用该小写变量与 "auto" 和 "manual" 做比较并在不匹配时触发
message(FATAL_ERROR ...)。在说明中保留原变量名 BOARD_BOOTLOADER_MODE 以便追踪调用处。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85b562c7-c432-4aa6-926b-52f0c0d01f14
📒 Files selected for processing (41)
Dockerfile.build_firmwarefirmware/c_board/app/src/usb/vendor.hppfirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/can/can.cppfirmware/rmcs_board/app/src/can/can.hppfirmware/rmcs_board/app/src/gpio/gpio.cppfirmware/rmcs_board/app/src/gpio/gpio.hppfirmware/rmcs_board/app/src/gpio/gpio_pin.hppfirmware/rmcs_board/app/src/spi/bmi088/accel.cppfirmware/rmcs_board/app/src/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/rmcs_board/app/src/spi/bmi088/gyro.cppfirmware/rmcs_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/app/src/spi/spi.cppfirmware/rmcs_board/app/src/spi/spi.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/boards/lite/CMakeLists.txtfirmware/rmcs_board/boards/lite/app/board_app.cppfirmware/rmcs_board/boards/lite/app/board_app.hppfirmware/rmcs_board/boards/lite/board.cfirmware/rmcs_board/boards/lite/board.hfirmware/rmcs_board/boards/lite/lite.yamlfirmware/rmcs_board/boards/pro/CMakeLists.txtfirmware/rmcs_board/boards/pro/app/board_app.cppfirmware/rmcs_board/boards/pro/app/board_app.hppfirmware/rmcs_board/boards/pro/board.cfirmware/rmcs_board/boards/pro/board.hfirmware/rmcs_board/boards/pro/pro.yamlfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpphost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board_lite.hpphost/include/librmcs/agent/rmcs_board_pro.hpp
💤 Files with no reviewable changes (4)
- firmware/rmcs_board/app/src/gpio/gpio.hpp
- firmware/c_board/app/src/usb/vendor.hpp
- host/include/librmcs/agent/c_board.hpp
- firmware/rmcs_board/app/src/gpio/gpio.cpp
✅ Files skipped from review due to trivial changes (6)
- firmware/rmcs_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/boards/pro/CMakeLists.txt
- firmware/rmcs_board/boards/pro/board.h
- firmware/rmcs_board/boards/lite/lite.yaml
- firmware/rmcs_board/boards/lite/board.h
- firmware/rmcs_board/boards/pro/pro.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/bootloader/src/usb/dfu.hpp
- Dockerfile.build_firmware
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/bootloader/src/main.cpp
- firmware/rmcs_board/app/src/can/can.cpp
- firmware/rmcs_board/app/src/app.cpp
- firmware/rmcs_board/boards/lite/CMakeLists.txt
- firmware/rmcs_board/boards/pro/app/board_app.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.cpp
- firmware/rmcs_board/app/src/spi/bmi088/gyro.cpp
- firmware/rmcs_board/CMakeLists.txt
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- host/include/librmcs/agent/rmcs_board_pro.hpp
- firmware/rmcs_board/boards/lite/app/board_app.hpp
- host/include/librmcs/agent/rmcs_board_lite.hpp
- firmware/rmcs_board/app/src/can/can.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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
feat(firmware/rmcs_board)!: Split rmcs_board into Pro and Lite variants
BREAKING CHANGE: Rename librmcs::agent::RmcsBoard to RmcsBoardPro and add librmcs::agent::RmcsBoardLite.
fix(firmware)!: Make DBUS receive-only
BREAKING CHANGE: Remove dbus_transmit() from CBoard, RmcsBoardPro and RmcsBoardLite.
功能与架构概述
将 rmcs_board 固件拆分为两个独立变体：Pro 与 Lite。每个变体提供独立的板级实现（引脚映射、外设初始化、中断注册、USB PID 等），通过 CMake 的 BOARD/BOARD_SEARCH_PATH 选择并在超构建中注入到固件/bootloader 构建流程中；同时将 DBUS（UART→主机）通路改为单向接收，软件接口与硬件方向保持一致。
主要变更要点
分拆与构建
板级差异与 USB PID
外设与中断重构
GPIO 抽象与 BMI088
DBUS 单向化（重大兼容变更）
Bootloader 行为调整
其他改动（整理与质量）
影响与兼容性（Breaking changes）
API/符号变化（主机端）
固件端
新增/删除文件与工作量估计
使用建议与注意事项