feat(firmware): Add RmcsBoard DFU bootloader support#39
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough提取并统一构建上下文与 DFU 产物生成到共享 CMake 模块；将 rmcs_board 重构为 superbuild（app/bootloader 子构建）；在 rmcs_board bootloader 中新增闪存布局、XPI NOR、Writer、Metadata、SHA‑256、DFU 控制与 USB 描述；调整 c_board 构建与 DFU 写入/清理逻辑。 Changes
Sequence Diagram(s)sequenceDiagram
participant Host as USB DFU 客户端
participant Dfu as Dfu 单例
participant Writer as Flash::Writer
participant Metadata as Flash::Metadata
participant Validator as Flash::Validation
Host->>Dfu: download(alt, block_num, data)
Dfu->>Dfu: 检查/建立会话 (expected_block_, downloaded_size_)
alt 新会话且 block_num==0
Dfu->>Writer: begin_session()
Dfu->>Metadata: begin_flashing()
end
Dfu->>Writer: write(address, data_chunk)
Writer->>Writer: 缓存到 sector_buffer_
Host->>Dfu: ... 重复传输块 ...
Host->>Dfu: manifest(alt)
Dfu->>Writer: finish_session()
Dfu->>Validator: validate_candidate_image(downloaded_size)
Validator->>Validator: 校验 size / 向量表 / HASH+SHA256
alt 验证通过
Validator-->>Dfu: true
Dfu->>Metadata: finish_flashing(size, crc32)
Metadata-->>Dfu: is_ready() && size/crc32 匹配
Dfu-->>Host: DFU_STATUS_OK
Dfu->>Metadata: 请求重启（BootMailbox）
else 验证失败
Validator-->>Dfu: false
Dfu->>Writer: abort_session()
Dfu-->>Host: DFU_STATUS_ERR_FIRMWARE
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
🧹 Nitpick comments (1)
firmware/common/cmake/librmcs_firmware.cmake (1)
83-88: 建议添加对VENDOR_ID/PRODUCT_ID/DEVICE_ID的验证。当前函数验证了
TARGET和OUTPUT_DFU，但未验证VENDOR_ID、PRODUCT_ID和DEVICE_ID。如果这些参数缺失，dfu-suffix会在构建时失败，但错误不如在 CMake 配置阶段提前报告清晰。♻️ 建议的验证代码
if(NOT ARG_OUTPUT_DFU) message(FATAL_ERROR "librmcs_add_dfu_image requires OUTPUT_DFU") endif() + if(NOT ARG_VENDOR_ID) + message(FATAL_ERROR "librmcs_add_dfu_image requires VENDOR_ID") + endif() + if(NOT ARG_PRODUCT_ID) + message(FATAL_ERROR "librmcs_add_dfu_image requires PRODUCT_ID") + endif() + if(NOT ARG_DEVICE_ID) + message(FATAL_ERROR "librmcs_add_dfu_image requires DEVICE_ID") + endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/common/cmake/librmcs_firmware.cmake` around lines 83 - 88, The CMake function librmcs_add_dfu_image currently validates ARG_TARGET and ARG_OUTPUT_DFU but not VENDOR_ID, PRODUCT_ID, or DEVICE_ID; add explicit checks like the existing ones by verifying each of VENDOR_ID, PRODUCT_ID, and DEVICE_ID and calling message(FATAL_ERROR "... requires VENDOR_ID"/"... requires PRODUCT_ID"/"... requires DEVICE_ID") if any are missing so CMake configuration fails early; place these checks alongside the existing ARG_TARGET/ARG_OUTPUT_DFU validations inside the librmcs_add_dfu_image logic to ensure dfu-suffix gets valid IDs.
🤖 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/common/cmake/librmcs_firmware.cmake`:
- Around line 83-88: The CMake function librmcs_add_dfu_image currently
validates ARG_TARGET and ARG_OUTPUT_DFU but not VENDOR_ID, PRODUCT_ID, or
DEVICE_ID; add explicit checks like the existing ones by verifying each of
VENDOR_ID, PRODUCT_ID, and DEVICE_ID and calling message(FATAL_ERROR "...
requires VENDOR_ID"/"... requires PRODUCT_ID"/"... requires DEVICE_ID") if any
are missing so CMake configuration fails early; place these checks alongside the
existing ARG_TARGET/ARG_OUTPUT_DFU validations inside the librmcs_add_dfu_image
logic to ensure dfu-suffix gets valid IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45a97cc4-a5d7-41cc-9b7d-53654ebb7ec1
📒 Files selected for processing (6)
firmware/c_board/CMakeLists.txtfirmware/c_board/app/CMakeLists.txtfirmware/c_board/bootloader/src/flash/validation.hppfirmware/c_board/bootloader/src/flash/writer.hppfirmware/c_board/bootloader/src/usb/dfu.hppfirmware/common/cmake/librmcs_firmware.cmake
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
firmware/rmcs_board/app/src/utility/boot_mailbox.hpp (1)
18-35: 把 mailbox 协议提到 app/bootloader 共用头。
reboot_to_bootloader()这里写入的 magic / request / GPR index，在firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp里又维护了一份匹配逻辑。这个握手两边只要有一处改漏就会静默失配，建议把这组协议常量和写入/读取辅助函数抽到共享头，避免协议漂移。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/utility/boot_mailbox.hpp` around lines 18 - 35, The mailbox protocol constants and helpers are duplicated between reboot_to_bootloader() and the bootloader copy, so extract the protocol into a single shared header (declare kMailboxMagic, kMailboxRequestEnterDfu, kMagicGprIndex and the helpers read/write functions such as write_pair) and have both app and bootloader include that header; update reboot_to_bootloader() to call the shared write_pair and remove the duplicate constants/logic from the bootloader implementation so both sides reference the same kMailboxMagic, kMailboxRequestEnterDfu, kMagicGprIndex and write_pair symbols to avoid silent mismatches.firmware/rmcs_board/bootloader/src/crypto/sha256.hpp (1)
25-197: 给这份 SHA-256 实现补上已知向量测试。这是新的自实现哈希代码，后续哪怕只改错一个移位或字节序细节，回归也很难靠人工 review 看出来。建议至少补空消息、
abc和跨 64B 边界输入的 KAT，把sha256_init/sha256_update/sha256_final的行为锁住。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp` around lines 25 - 197, Add deterministic known-answer tests that exercise sha256_init, sha256_update and sha256_final: write unit tests that compute SHA-256 for (1) the empty message, (2) the ASCII string "abc", and (3) a message longer than 64 bytes that forces a transform call across the 64-byte chunk boundary (e.g. 80+ bytes), verifying each result against the standard RFC/online published digests; use Sha256Ctx as the context, call sha256_init, feed bytes via sha256_update (allowing multiple update calls for the cross-boundary case), then sha256_final and compare the 32-byte output to the expected hex values to lock behavior for endianness, padding and bit-length handling.firmware/rmcs_board/bootloader/src/flash/layout.hpp (1)
10-16: 把 flash 分区边界收敛到单一来源。这里把
kMetadataStartAddress/kAppStartAddress写成了绝对地址，同时firmware/rmcs_board/bootloader/CMakeLists.txtLine 67 又单独用_flash_size=0x1F000定义了 bootloader 大小。现在两边刚好一致，但后面只改一边时，链接布局和运行时校验/跳转边界会静默漂移。建议从同一个构建常量生成，或至少补一个生成期校验。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/layout.hpp` around lines 10 - 16, 将分区边界从硬编码常量改为由单一构建常量驱动：在构建系统（CMake）把 bootloader 大小暴露为编译宏（例如 BOOTLOADER_FLASH_SIZE 或类似名），然后在 layout.hpp 中用该宏计算 kAppStartAddress（例如 kAppStartAddress = kFlashBaseAddress + BOOTLOADER_FLASH_SIZE），再根据元数据大小计算 kMetadataStartAddress/kMetadataEndAddress，并相应更新 kAppMaxImageSize；同时添加一个静态断言（static_assert）比对现有硬编码值以防不同步（参考符号：kMetadataStartAddress、kMetadataEndAddress、kAppStartAddress、kAppMaxImageSize、kFlashBaseAddress 以及构建宏名如 BOOTLOADER_FLASH_SIZE/_flash_size）。firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp (1)
51-51: 让sector_size()也自初始化，避免调用顺序陷阱。Line 51 现在会在未初始化时直接返回
0，而同一个类型的其它 public 方法都会先init()。这个 API 不对称，后面很容易被误用。建议修改
- uint32_t sector_size() const { return sector_size_; } + uint32_t sector_size() { + init(); + return sector_size_; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp` at line 51, sector_size() currently returns sector_size_ even when the object hasn't been init()'d, causing a call-order trap; make sector_size() symmetric with other public APIs by ensuring initialization before returning: inside sector_size() call the class init/ensure-initialized path (e.g., invoke init() or an existing ensure_initialized() guard that sets sector_size_) and handle/init errors consistently rather than returning the raw uninitialized sector_size_. Reference: sector_size(), init(), and the member sector_size_.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.build_firmware`:
- Around line 14-17: Dockerfile.build_firmware now produces
librmcs-bootloader-${board}.elf but the release workflow's artifact glob only
matches build/librmcs-firmware*; update the artifact collection glob in
release-package.yml (the pattern at the current artifact step) to also include
librmcs-bootloader* (e.g., add build/librmcs-bootloader* alongside
build/librmcs-firmware*) so the bootloader ELF is picked up for releases.
In `@firmware/rmcs_board/app/CMakeLists.txt`:
- Around line 78-86: The DFU is being written to the build root so packaging
doesn't pick it up; update the librmcs_add_dfu_image call (TARGET
${APP_ELF_NAME}, INPUT_BINARY ${EXECUTABLE_OUTPUT_PATH}/${APP_BIN_NAME}) to set
OUTPUT_DFU to the packaging output subdirectory under the executable output
(i.e. place the DFU inside the packaging "output" folder rather than directly
under ${EXECUTABLE_OUTPUT_PATH}) so the packager can find rmcs_board_app.dfu
during export.
In `@firmware/rmcs_board/app/src/usb/usb_descriptors.hpp`:
- Around line 162-171: Change the device-level class in the USB device
descriptor so the bDeviceClass field is 0x00 instead of
TUSB_CLASS_VENDOR_SPECIFIC; locate the device descriptor where .bDeviceClass is
set (the usb device descriptor struct/initializer that contains .bDeviceClass)
and replace the TUSB_CLASS_VENDOR_SPECIFIC value with 0x00 so the composite
device advertises interface-defined classes (allowing the kItfNumVendor and
kItfNumDfuRuntime interfaces to be recognized correctly).
In `@firmware/rmcs_board/bootloader/src/usb/dfu.hpp`:
- Around line 54-60: Current code calls
flash::Metadata::get_instance().begin_flashing() immediately in the session
start (around flash_writer_.begin_session()), which advances metadata before any
sector is actually committed; change the logic so begin_flashing() is deferred
until the first actual sector/sector-commit path (the code path that performs
the first flash write/commit) or, alternatively, ensure metadata is rolled back
if the session aborts before the first commit; update the session state handling
around session_started_, expected_block_, downloaded_size_, reset_requested_ to
reflect that metadata is only advanced on the first successful sector commit (or
to trigger a metadata rollback on abort when no sector was committed).
---
Nitpick comments:
In `@firmware/rmcs_board/app/src/utility/boot_mailbox.hpp`:
- Around line 18-35: The mailbox protocol constants and helpers are duplicated
between reboot_to_bootloader() and the bootloader copy, so extract the protocol
into a single shared header (declare kMailboxMagic, kMailboxRequestEnterDfu,
kMagicGprIndex and the helpers read/write functions such as write_pair) and have
both app and bootloader include that header; update reboot_to_bootloader() to
call the shared write_pair and remove the duplicate constants/logic from the
bootloader implementation so both sides reference the same kMailboxMagic,
kMailboxRequestEnterDfu, kMagicGprIndex and write_pair symbols to avoid silent
mismatches.
In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp`:
- Around line 25-197: Add deterministic known-answer tests that exercise
sha256_init, sha256_update and sha256_final: write unit tests that compute
SHA-256 for (1) the empty message, (2) the ASCII string "abc", and (3) a message
longer than 64 bytes that forces a transform call across the 64-byte chunk
boundary (e.g. 80+ bytes), verifying each result against the standard RFC/online
published digests; use Sha256Ctx as the context, call sha256_init, feed bytes
via sha256_update (allowing multiple update calls for the cross-boundary case),
then sha256_final and compare the 32-byte output to the expected hex values to
lock behavior for endianness, padding and bit-length handling.
In `@firmware/rmcs_board/bootloader/src/flash/layout.hpp`:
- Around line 10-16: 将分区边界从硬编码常量改为由单一构建常量驱动：在构建系统（CMake）把 bootloader 大小暴露为编译宏（例如
BOOTLOADER_FLASH_SIZE 或类似名），然后在 layout.hpp 中用该宏计算 kAppStartAddress（例如
kAppStartAddress = kFlashBaseAddress + BOOTLOADER_FLASH_SIZE），再根据元数据大小计算
kMetadataStartAddress/kMetadataEndAddress，并相应更新
kAppMaxImageSize；同时添加一个静态断言（static_assert）比对现有硬编码值以防不同步（参考符号：kMetadataStartAddress、kMetadataEndAddress、kAppStartAddress、kAppMaxImageSize、kFlashBaseAddress
以及构建宏名如 BOOTLOADER_FLASH_SIZE/_flash_size）。
In `@firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp`:
- Line 51: sector_size() currently returns sector_size_ even when the object
hasn't been init()'d, causing a call-order trap; make sector_size() symmetric
with other public APIs by ensuring initialization before returning: inside
sector_size() call the class init/ensure-initialized path (e.g., invoke init()
or an existing ensure_initialized() guard that sets sector_size_) and
handle/init errors consistently rather than returning the raw uninitialized
sector_size_. Reference: sector_size(), init(), and the member sector_size_.
🪄 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: 62ad8677-1adc-4965-aef0-87bc8263d870
📒 Files selected for processing (27)
.scripts/lint-targets.ymlDockerfile.build_firmwarefirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/include/tusb_config.hfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.cppfirmware/rmcs_board/app/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/include/tusb_config.hfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/flash/layout.hppfirmware/rmcs_board/bootloader/src/flash/metadata.hppfirmware/rmcs_board/bootloader/src/flash/validation.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hppfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.cppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/assert.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/src/utility/jump.hppfirmware/rmcs_board/bsp/hpm_sdkfirmware/rmcs_board/cmake/merge_compile_commands.cmake
✅ Files skipped from review due to trivial changes (1)
- firmware/rmcs_board/bootloader/src/utility/assert.hpp
🚧 Files skipped from review as they are similar to previous changes (3)
- firmware/rmcs_board/app/src/usb/vendor.cpp
- .scripts/lint-targets.yml
- firmware/rmcs_board/app/src/app.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp (1)
69-72: 统一偏移计算基址，避免常量漂移带来的隐性风险。Line 70 用
kFlashBaseAddress做前置检查，但 Line 71 用BOARD_FLASH_BASE_ADDRESS计算偏移；建议统一到同一常量，避免后续配置变更时出现偏移计算不一致。可选修正示例
static uint32_t to_flash_offset(uintptr_t address) { utility::assert_debug(address >= kFlashBaseAddress); - return static_cast<uint32_t>(address - BOARD_FLASH_BASE_ADDRESS); + return static_cast<uint32_t>(address - kFlashBaseAddress); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp` around lines 69 - 72, The function to_flash_offset performs a bounds check using kFlashBaseAddress but computes the offset using BOARD_FLASH_BASE_ADDRESS, risking inconsistent offsets if those constants diverge; update to_flash_offset to use the same constant for both the assert and the subtraction (choose either kFlashBaseAddress or BOARD_FLASH_BASE_ADDRESS consistently) so the bounds check and offset calculation match (refer to the to_flash_offset function, kFlashBaseAddress, and BOARD_FLASH_BASE_ADDRESS when making the change).firmware/rmcs_board/bootloader/src/flash/writer.hpp (1)
33-55: 建议把“首段写入扇区对齐”前置条件固化为编译期约束。Line 51 的不变量依赖首段写入从扇区起点开始；建议增加
static_assert显式约束分区对齐，避免未来分区参数调整后在 Release 构建中出现静默破坏风险。可选修正示例
class Writer { public: static constexpr uint32_t kTransferBlockSize = CFG_TUD_DFU_XFER_BUFSIZE; + static_assert((kAppStartAddress % kFlashSectorSize) == 0U); + static_assert((kAppEndAddress % sizeof(uint32_t)) == 0U);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/writer.hpp` around lines 33 - 55, The write() loop relies on the first write being sector-aligned (see the invariant utility::assert_debug(sector_offset == buffered_size_) in write) so add a compile-time assertion that the application region boundaries are sector-aligned: introduce or reuse a constexpr sector size (e.g. XpiNor::kSectorSize or a new constexpr kSector_SIZE) and add static_asserts that kAppStartAddress % kSector_SIZE == 0 and (kAppEndAddress - kAppStartAddress) % kSector_SIZE == 0 (or kAppEndAddress % kSector_SIZE == 0) near the constants or at the top of writer.hpp so the invariant used in write() cannot be violated in Release builds.
🤖 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/src/flash/writer.hpp`:
- Around line 33-55: The write() loop relies on the first write being
sector-aligned (see the invariant utility::assert_debug(sector_offset ==
buffered_size_) in write) so add a compile-time assertion that the application
region boundaries are sector-aligned: introduce or reuse a constexpr sector size
(e.g. XpiNor::kSectorSize or a new constexpr kSector_SIZE) and add
static_asserts that kAppStartAddress % kSector_SIZE == 0 and (kAppEndAddress -
kAppStartAddress) % kSector_SIZE == 0 (or kAppEndAddress % kSector_SIZE == 0)
near the constants or at the top of writer.hpp so the invariant used in write()
cannot be violated in Release builds.
In `@firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp`:
- Around line 69-72: The function to_flash_offset performs a bounds check using
kFlashBaseAddress but computes the offset using BOARD_FLASH_BASE_ADDRESS,
risking inconsistent offsets if those constants diverge; update to_flash_offset
to use the same constant for both the assert and the subtraction (choose either
kFlashBaseAddress or BOARD_FLASH_BASE_ADDRESS consistently) so the bounds check
and offset calculation match (refer to the to_flash_offset function,
kFlashBaseAddress, and BOARD_FLASH_BASE_ADDRESS when making the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f05244a9-9302-4031-ae36-d4a0d388dd75
📒 Files selected for processing (4)
firmware/c_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp
qzhhhi
force-pushed
the
dev/rmcsboard-bootloader
branch
from
976eddc to
7075ce7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp (2)
40-42: LANGID 直接按 16 位存会更清晰。这里把两个原始字节
memcpy到uint16_t描述符缓冲区里，可读性上要靠推断目标字节序才能看出是0x0409。把kLanguageId改成std::array<uint16_t, 1>{0x0409U}，并直接写入descriptor_string_buffer_[1]，后面如果扩语言列表也更不容易把“元素数”和“字节数”混在一起。Also applies to: 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp` around lines 40 - 42, Change the language ID handling to use 16-bit units for clarity: replace kLanguageId's type/value with a std::array<uint16_t, N> (for now {0x0409U}) and write those uint16_t elements directly into the descriptor buffer instead of memcpy'ing raw bytes; update the write at the index==0 branch that currently memcpy's into descriptor_string_buffer_[1] to assign the 16-bit value(s) (and do the same at the other occurrence around the 150-range). Refer to kLanguageId and descriptor_string_buffer_ to locate the spots to update.
73-86: 把序列号模板和游标步进绑定到编译期。这段写法依赖
serial_string_的固定模板：每次+ 1都是在跳过已有的-或末尾\0。现在只有最后的 debug assert 能暴露布局漂移，而且一旦模板被改短，越界写会先发生。建议补一个static_assert，把预期 buffer 大小和kUuidWordCount绑定起来。♻️ 建议改法
void update_serial_string() { + constexpr size_t kExpectedSerialBufferSize = 3U + (kUuidWordCount * 10U); + static_assert(kExpectedSerialBufferSize == std::tuple_size_v<decltype(serial_string_)>); + std::array<uint32_t, kUuidWordCount> uuid{};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp` around lines 73 - 86, The update_serial_string function assumes a fixed serial_string_ layout and steps the cursor by +1 after each write_hex_u16; to bind that layout at compile time add a static_assert tying serial_string_.size() to kUuidWordCount so layout drift is caught early, e.g. assert that serial_string_.size() == 3 + kUuidWordCount * 10 (3-byte prefix + two 4-hex groups per UUID word plus two separators = 10 chars per word); place this static_assert near the serial_string_ declaration or at the top of update_serial_string and reference symbols serial_string_, kUuidWordCount and write_hex_u16 so any change to the template or UUID word count fails to compile instead of overflowing.firmware/rmcs_board/bootloader/src/usb/dfu.hpp (1)
95-105: 避免在 manifest 阶段重复跑整镜像校验。这里先
validate_candidate_image()+compute_image_crc32()，后面validate_app_image()又把同一份镜像完整做了一遍 SHA-256 和 CRC32。对固件镜像来说，这会把 manifest 时延直接翻倍，而且DFU_MANIFEST现在只报100 ms，host 侧时序裕量会比较紧。建议复用前面已经得到的downloaded_size_/image_crc32结果，把最后一步收敛成 metadata 写入后的轻量 post-condition 检查；如果还想保留端到端自检，可以降成 debug assert。♻️ 一种收敛方式
const uint32_t image_crc32 = flash::compute_image_crc32(flash::kAppStartAddress, downloaded_size_); - flash::Metadata::get_instance().finish_flashing(downloaded_size_, image_crc32); - - if (!flash::validate_app_image()) + auto& meta = flash::Metadata::get_instance(); + meta.finish_flashing(downloaded_size_, image_crc32); + if (!meta.is_ready() || meta.image_size() != downloaded_size_ + || meta.image_crc32() != image_crc32) { return fail(DFU_STATUS_ERR_FIRMWARE); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/usb/dfu.hpp` around lines 95 - 105, The manifest path is re-validating the full image twice: keep the existing validate_candidate_image(downloaded_size_) and compute_image_crc32(...) results, call flash::Metadata::get_instance().finish_flashing(downloaded_size_, image_crc32), and remove or replace the expensive validate_app_image() call with a lightweight post-condition that reuses downloaded_size_ and image_crc32 (or convert it to a debug-only assert) so DFU_MANIFEST does not rerun full SHA/CRC; update code paths that reference validate_app_image to use the cached downloaded_size_/image_crc32 values instead.firmware/rmcs_board/bootloader/src/flash/writer.hpp (1)
21-21: 把 sector 大小假设显式固化下来。
sector_buffer_固定按kFlashSectorSize分配，但分段和writable的计算走的是XpiNor::instance().sector_size()。现在板级配置一致时没问题；不过一旦两者以后偏离，这里会先按更大的 runtime sector 算出chunk_size，再越界写入 4 KiB buffer。建议在 session 开始时加一个 fail-fast 断言，把这个前提锁死。♻️ 建议修改
- void begin_session() { clear_active_sector(); } + void begin_session() { + utility::assert_always(XpiNor::instance().sector_size() == kSectorBufferCapacity); + clear_active_sector(); + }Also applies to: 47-49, 66-68, 78-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/writer.hpp` at line 21, The code assumes runtime sector size matches the compile-time kFlashSectorSize and can overflow sector_buffer_ when XpiNor::instance().sector_size() is larger; add a fail-fast check at the start of begin_session() (and similarly where chunk_size/writable are computed) that asserts or returns an error if XpiNor::instance().sector_size() != kFlashSectorSize (or is > kFlashSectorSize), referencing begin_session(), sector_buffer_, kFlashSectorSize, and XpiNor::instance().sector_size(); this locks the invariant and prevents calculating a chunk_size that would write past the 4 KiB buffer.firmware/rmcs_board/bootloader/src/flash/metadata.hpp (1)
153-158: 把 metadata“只占 1 个 sector”的前提写成断言。
scan_latest_valid_slot()会扫完整个[kMetadataStartAddress, kMetadataEndAddress)，但erase_and_rescan()只擦除了起始 sector。当前布局刚好成立，不过这里最好加一个static_assert锁住这个假设，避免后续扩展 metadata 区时静默留下脏 slot。♻️ 建议修改
+ static_assert((kMetadataEndAddress - kMetadataStartAddress) == kFlashSectorSize); void erase_and_rescan() { XpiNor::instance().erase_sector(kMetadataStartAddress); scan_latest_valid_slot();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/metadata.hpp` around lines 153 - 158, The code assumes metadata occupies exactly one flash sector but erase_and_rescan() only erases kMetadataStartAddress; add a compile-time check to lock that assumption: insert a static_assert (at header scope near the metadata constants) that (kMetadataEndAddress - kMetadataStartAddress) equals the flash sector size constant/constexpr (e.g., XpiNor::kSectorSize or the project's flash sector size constant) so scan_latest_valid_slot() cannot silently cover more than the erased sector; keep existing runtime assert using latest_valid_slot_ and latest_valid_slot_state_ in erase_and_rescan().
🤖 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/bootloader/src/utility/jump.hpp`:
- Around line 3-5: Replace any use of the GCC builtin __builtin_unreachable() in
this header with the C++23 standard std::unreachable(), and add the <utility>
header if it's not already included; specifically update occurrences of
__builtin_unreachable() (in functions like any jump/jump_to/unreachable helper
within jump.hpp) to call std::unreachable() so the code complies with
CMAKE_CXX_EXTENSIONS OFF and matches core/src/utility/assert.hpp usage.
---
Nitpick comments:
In `@firmware/rmcs_board/bootloader/src/flash/metadata.hpp`:
- Around line 153-158: The code assumes metadata occupies exactly one flash
sector but erase_and_rescan() only erases kMetadataStartAddress; add a
compile-time check to lock that assumption: insert a static_assert (at header
scope near the metadata constants) that (kMetadataEndAddress -
kMetadataStartAddress) equals the flash sector size constant/constexpr (e.g.,
XpiNor::kSectorSize or the project's flash sector size constant) so
scan_latest_valid_slot() cannot silently cover more than the erased sector; keep
existing runtime assert using latest_valid_slot_ and latest_valid_slot_state_ in
erase_and_rescan().
In `@firmware/rmcs_board/bootloader/src/flash/writer.hpp`:
- Line 21: The code assumes runtime sector size matches the compile-time
kFlashSectorSize and can overflow sector_buffer_ when
XpiNor::instance().sector_size() is larger; add a fail-fast check at the start
of begin_session() (and similarly where chunk_size/writable are computed) that
asserts or returns an error if XpiNor::instance().sector_size() !=
kFlashSectorSize (or is > kFlashSectorSize), referencing begin_session(),
sector_buffer_, kFlashSectorSize, and XpiNor::instance().sector_size(); this
locks the invariant and prevents calculating a chunk_size that would write past
the 4 KiB buffer.
In `@firmware/rmcs_board/bootloader/src/usb/dfu.hpp`:
- Around line 95-105: The manifest path is re-validating the full image twice:
keep the existing validate_candidate_image(downloaded_size_) and
compute_image_crc32(...) results, call
flash::Metadata::get_instance().finish_flashing(downloaded_size_, image_crc32),
and remove or replace the expensive validate_app_image() call with a lightweight
post-condition that reuses downloaded_size_ and image_crc32 (or convert it to a
debug-only assert) so DFU_MANIFEST does not rerun full SHA/CRC; update code
paths that reference validate_app_image to use the cached
downloaded_size_/image_crc32 values instead.
In `@firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp`:
- Around line 40-42: Change the language ID handling to use 16-bit units for
clarity: replace kLanguageId's type/value with a std::array<uint16_t, N> (for
now {0x0409U}) and write those uint16_t elements directly into the descriptor
buffer instead of memcpy'ing raw bytes; update the write at the index==0 branch
that currently memcpy's into descriptor_string_buffer_[1] to assign the 16-bit
value(s) (and do the same at the other occurrence around the 150-range). Refer
to kLanguageId and descriptor_string_buffer_ to locate the spots to update.
- Around line 73-86: The update_serial_string function assumes a fixed
serial_string_ layout and steps the cursor by +1 after each write_hex_u16; to
bind that layout at compile time add a static_assert tying serial_string_.size()
to kUuidWordCount so layout drift is caught early, e.g. assert that
serial_string_.size() == 3 + kUuidWordCount * 10 (3-byte prefix + two 4-hex
groups per UUID word plus two separators = 10 chars per word); place this
static_assert near the serial_string_ declaration or at the top of
update_serial_string and reference symbols serial_string_, kUuidWordCount and
write_hex_u16 so any change to the template or UUID word count fails to compile
instead of overflowing.
🪄 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: e47c4390-1947-4a25-bf3d-3ee863cb2bd7
📒 Files selected for processing (29)
.scripts/lint-targets.ymlDockerfile.build_firmwarefirmware/c_board/app/src/timer/timer.hppfirmware/c_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/include/tusb_config.hfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.cppfirmware/rmcs_board/app/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/include/tusb_config.hfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/flash/layout.hppfirmware/rmcs_board/bootloader/src/flash/metadata.hppfirmware/rmcs_board/bootloader/src/flash/validation.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hppfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.cppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/assert.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/src/utility/jump.hppfirmware/rmcs_board/bsp/hpm_sdkfirmware/rmcs_board/cmake/merge_compile_commands.cmake
✅ Files skipped from review due to trivial changes (5)
- firmware/c_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/cmake/merge_compile_commands.cmake
- firmware/c_board/app/src/timer/timer.hpp
- firmware/rmcs_board/bootloader/src/main.cpp
- firmware/rmcs_board/bootloader/include/tusb_config.h
🚧 Files skipped from review as they are similar to previous changes (13)
- firmware/rmcs_board/app/src/usb/vendor.cpp
- firmware/rmcs_board/bsp/hpm_sdk
- firmware/rmcs_board/app/src/app.cpp
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/app/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/app/include/tusb_config.h
- firmware/rmcs_board/bootloader/src/usb/dfu.cpp
- firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp
- firmware/rmcs_board/bootloader/src/utility/assert.hpp
- firmware/rmcs_board/bootloader/src/crypto/sha256.hpp
- firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/bootloader/src/flash/layout.hpp
qzhhhi
force-pushed
the
dev/rmcsboard-bootloader
branch
3 times, most recently
from
059ce81 to
46e5ede
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
firmware/rmcs_board/bootloader/src/crypto/sha256.hpp (2)
25-25: 把kSha256BlockSize改成摘要长度常量名。这里的
32U表示的是 SHA-256 输出摘要长度，不是 64B 的压缩块大小；同一个头里又定义了kChunkBytes = 64U，后续调用方很容易把两者混用。建议重命名为kSha256DigestSize/kSha256HashSize，如果确实要公开块大小，再单独暴露一个 64B 常量。建议修改
-inline constexpr size_t kSha256BlockSize = 32U; +inline constexpr size_t kSha256DigestSize = 32U;Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp` at line 25, The constant kSha256BlockSize is misnamed (it denotes the 32‑byte digest length, not the 64‑byte compression block) and can be confused with kChunkBytes = 64U; rename kSha256BlockSize to a clear digest name (e.g., kSha256DigestSize or kSha256HashSize) and update all uses (functions/classes referencing kSha256BlockSize) accordingly, and if the 64B block size must be exposed, add a separate constant (e.g., kSha256BlockSize = 64U) so callers can unambiguously use digest vs block size; apply the same rename/fix for the related constants referenced around kChunkBytes (lines noted in comment).
77-92: 去掉message_schedule的显式零初始化。这 64 个槽位都会在后续两段循环里被写满，
{}在这里没有语义收益；保留它只会让sha256_transform()的热路径更容易生成多余的初始化代码，镜像全量哈希时会被重复放大。建议修改
- std::array<uint32_t, 64> message_schedule{}; + std::array<uint32_t, 64> message_schedule;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp` around lines 77 - 92, The array message_schedule in sha256_transform is being value-initialized with {} which forces zeroing; remove the explicit zero initialization to avoid needless work on the hot path by changing its declaration to std::array<uint32_t, 64> message_schedule; (the subsequent loops write indices 0..63 so no entries are read uninitialized). Ensure no other code in sha256_transform relies on initial zeros before the two fill loops.firmware/rmcs_board/bootloader/CMakeLists.txt (1)
6-54: 把 HPM 子构建样板继续下沉成共享 helper。这段和
firmware/rmcs_board/app/CMakeLists.txt基本是同一套初始化模板：SDK 定位、project context、warning/ABI/标准设置、source-path-map，以及 HPM include 的 system 化处理都重复了一遍。后续只要再调一次编译选项或 SDK 初始化，很容易两边漂移。建议把公共部分提到firmware/common/cmake/librmcs_firmware.cmake或firmware/rmcs_board/cmake/，子构建里只保留源文件、链接符号和 DFU 参数。Also applies to: 69-77
🤖 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 6 - 54, The bootloader CMakeLists duplicates SDK/board initialization (SDK location, project context, ABI/standards, warning flags, source-path-map and HPM include handling) that also exists in firmware/rmcs_board/app/CMakeLists.txt; extract these shared steps into a common helper in firmware/common/cmake/librmcs_firmware.cmake (or firmware/rmcs_board/cmake/) and replace the duplicated blocks in this file with a single helper call (use the existing include("${CMAKE_CURRENT_SOURCE_DIR}/../../common/cmake/librmcs_firmware.cmake") and add a new function like librmcs_configure_sdk_project that encapsulates librmcs_setup_project_context, sdk_compile_options, sdk_compile_definitions, sdk_inc, sdk_app_inc and the HPM find_package/ENV{HPM_SDK_BASE} logic), then update both bootloader and app CMakeLists to call that helper (also apply the same refactor to the repeated section referenced around lines 69-77).
🤖 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/bootloader/src/usb/usb_descriptors.hpp`:
- Line 153: kAlt0String currently reads "Internal Flash" but the actual DFU
target is XpiNor (see XpiNor::instance() used in writer.hpp); update the static
constexpr std::string_view kAlt0String in usb_descriptors.hpp to a name that
reflects the real medium (e.g., "XPI-NOR Flash" or "XpiNor") so the DFU target
shown to hosts matches XpiNor::instance().
---
Nitpick comments:
In `@firmware/rmcs_board/bootloader/CMakeLists.txt`:
- Around line 6-54: The bootloader CMakeLists duplicates SDK/board
initialization (SDK location, project context, ABI/standards, warning flags,
source-path-map and HPM include handling) that also exists in
firmware/rmcs_board/app/CMakeLists.txt; extract these shared steps into a common
helper in firmware/common/cmake/librmcs_firmware.cmake (or
firmware/rmcs_board/cmake/) and replace the duplicated blocks in this file with
a single helper call (use the existing
include("${CMAKE_CURRENT_SOURCE_DIR}/../../common/cmake/librmcs_firmware.cmake")
and add a new function like librmcs_configure_sdk_project that encapsulates
librmcs_setup_project_context, sdk_compile_options, sdk_compile_definitions,
sdk_inc, sdk_app_inc and the HPM find_package/ENV{HPM_SDK_BASE} logic), then
update both bootloader and app CMakeLists to call that helper (also apply the
same refactor to the repeated section referenced around lines 69-77).
In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp`:
- Line 25: The constant kSha256BlockSize is misnamed (it denotes the 32‑byte
digest length, not the 64‑byte compression block) and can be confused with
kChunkBytes = 64U; rename kSha256BlockSize to a clear digest name (e.g.,
kSha256DigestSize or kSha256HashSize) and update all uses (functions/classes
referencing kSha256BlockSize) accordingly, and if the 64B block size must be
exposed, add a separate constant (e.g., kSha256BlockSize = 64U) so callers can
unambiguously use digest vs block size; apply the same rename/fix for the
related constants referenced around kChunkBytes (lines noted in comment).
- Around line 77-92: The array message_schedule in sha256_transform is being
value-initialized with {} which forces zeroing; remove the explicit zero
initialization to avoid needless work on the hot path by changing its
declaration to std::array<uint32_t, 64> message_schedule; (the subsequent loops
write indices 0..63 so no entries are read uninitialized). Ensure no other code
in sha256_transform relies on initial zeros before the two fill loops.
🪄 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: 8f64921a-72ae-437b-9b36-58fc0dee6063
📒 Files selected for processing (61)
.github/workflows/lint.yml.scripts/lint-targets.ymlAGENTS.mdDockerfile.build_firmwarefirmware/c_board/CMakeLists.txtfirmware/c_board/app/CMakeLists.txtfirmware/c_board/app/src/timer/timer.hppfirmware/c_board/app/src/usb/usb_descriptors.hppfirmware/c_board/bootloader/src/flash/validation.hppfirmware/c_board/bootloader/src/flash/writer.hppfirmware/c_board/bootloader/src/main.cppfirmware/c_board/bootloader/src/usb/dfu.hppfirmware/c_board/bootloader/src/utility/jump.hppfirmware/common/cmake/librmcs_firmware.cmakefirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/include/tusb_config.hfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/app.hppfirmware/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/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/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/rx_buffer.hppfirmware/rmcs_board/app/src/uart/tx_buffer.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/helper.hppfirmware/rmcs_board/app/src/usb/interrupt_safe_buffer.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.cppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.cppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/app/src/utility/assert.cppfirmware/rmcs_board/app/src/utility/boot_mailbox.hppfirmware/rmcs_board/app/src/utility/interrupt_lock.hppfirmware/rmcs_board/app/src/utility/lazy.hppfirmware/rmcs_board/app/src/utility/ring_buffer.hppfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/include/tusb_config.hfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/flash/layout.hppfirmware/rmcs_board/bootloader/src/flash/metadata.hppfirmware/rmcs_board/bootloader/src/flash/validation.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hppfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.cppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/assert.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/src/utility/jump.hppfirmware/rmcs_board/bsp/hpm_sdkfirmware/rmcs_board/cmake/merge_compile_commands.cmake
✅ Files skipped from review due to trivial changes (27)
- firmware/rmcs_board/app/src/uart/tx_buffer.hpp
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/app/src/spi/spi.hpp
- firmware/rmcs_board/app/src/uart/uart.hpp
- firmware/rmcs_board/app/src/usb/usb_descriptors.cpp
- .github/workflows/lint.yml
- firmware/rmcs_board/app/src/spi/bmi088/gyro.hpp
- firmware/rmcs_board/app/src/usb/vendor.hpp
- firmware/rmcs_board/app/src/utility/lazy.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/bsp/hpm_sdk
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- firmware/c_board/app/src/timer/timer.hpp
- firmware/rmcs_board/app/src/can/can.cpp
- firmware/rmcs_board/app/src/gpio/gpio.cpp
- firmware/rmcs_board/app/src/app.hpp
- firmware/rmcs_board/app/src/can/can.hpp
- firmware/c_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/cmake/merge_compile_commands.cmake
- AGENTS.md
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/app/include/tusb_config.h
- firmware/rmcs_board/bootloader/src/utility/assert.hpp
- firmware/rmcs_board/bootloader/src/flash/layout.hpp
- firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp
- firmware/rmcs_board/bootloader/include/tusb_config.h
- firmware/rmcs_board/bootloader/src/flash/metadata.hpp
🚧 Files skipped from review as they are similar to previous changes (13)
- .scripts/lint-targets.yml
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/app/src/app.cpp
- firmware/c_board/bootloader/src/flash/writer.hpp
- firmware/rmcs_board/app/src/usb/vendor.cpp
- firmware/rmcs_board/bootloader/src/utility/jump.hpp
- firmware/c_board/bootloader/src/usb/dfu.hpp
- firmware/rmcs_board/app/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/bootloader/src/flash/validation.hpp
- firmware/rmcs_board/bootloader/src/usb/dfu.cpp
- firmware/common/cmake/librmcs_firmware.cmake
- firmware/c_board/bootloader/src/flash/validation.hpp
There was a problem hiding this comment.
🧹 Nitpick comments (2)
firmware/rmcs_board/bootloader/src/crypto/sha256.hpp (2)
25-25: 常量命名具有误导性：kSha256BlockSize实际上是摘要大小在 SHA-256 术语中：
- Block size = 64 字节（512 位）— 压缩函数的输入块大小
- Digest size = 32 字节（256 位）— 输出哈希大小
此常量值为 32，是摘要/哈希大小，而非块大小。建议重命名为
kSha256DigestSize或kSha256HashSize以避免混淆。♻️ 建议修改
-inline constexpr size_t kSha256BlockSize = 32U; +inline constexpr size_t kSha256DigestSize = 32U;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp` at line 25, The constant kSha256BlockSize is misnamed: its value 32 is the SHA-256 digest/hash size, not the compression block size (64 bytes). Rename the constant to kSha256DigestSize (or kSha256HashSize) and update all references/usages in sha256.hpp and any consumers to use the new identifier; ensure any documentation/comments mentioning block size are corrected and that no logic assumes a 64-byte block based on this constant.
11-16: 注释中字节序描述有误第 16 行声称 "This implementation uses little endian byte order"，但实际上：
sha256_transform（第 82-85 行）以大端序读取数据sha256_final（第 176-183 行）以大端序存储比特长度sha256_final（第 186-196 行）以大端序输出哈希这符合 SHA-256 规范（FIPS 180-4）要求的大端字节序。建议更正注释以避免误导。
📝 建议修改
Algorithm specification can be found here: * http://csrc.nist.gov/publications/fips/fips180-2/fips180-2withchangenotice.pdf - This implementation uses little endian byte order. + This implementation uses big endian byte order as per SHA-256 spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/crypto/sha256.hpp` around lines 11 - 16, The file header incorrectly states "This implementation uses little endian byte order"; update that comment to say the implementation uses big-endian (network) byte order because sha256_transform reads input as big-endian and sha256_final stores the bit length and outputs the digest in big-endian; reference the functions sha256_transform and sha256_final when making the correction so readers understand the comment matches the implementation and the SHA-256 (FIPS 180-4) spec.
🤖 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/src/crypto/sha256.hpp`:
- Line 25: The constant kSha256BlockSize is misnamed: its value 32 is the
SHA-256 digest/hash size, not the compression block size (64 bytes). Rename the
constant to kSha256DigestSize (or kSha256HashSize) and update all
references/usages in sha256.hpp and any consumers to use the new identifier;
ensure any documentation/comments mentioning block size are corrected and that
no logic assumes a 64-byte block based on this constant.
- Around line 11-16: The file header incorrectly states "This implementation
uses little endian byte order"; update that comment to say the implementation
uses big-endian (network) byte order because sha256_transform reads input as
big-endian and sha256_final stores the bit length and outputs the digest in
big-endian; reference the functions sha256_transform and sha256_final when
making the correction so readers understand the comment matches the
implementation and the SHA-256 (FIPS 180-4) spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66d03bf6-0ff7-45ff-ba37-f067f08bf80d
📒 Files selected for processing (4)
firmware/c_board/bootloader/src/crypto/sha256.hppfirmware/c_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/usb/dfu.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- firmware/c_board/bootloader/src/usb/dfu.hpp
- firmware/rmcs_board/bootloader/src/usb/dfu.hpp
qzhhhi
force-pushed
the
dev/rmcsboard-bootloader
branch
from
4fe5751 to
fbf946e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
firmware/c_board/bootloader/src/flash/validation.hpp (1)
12-17:⚠️ Potential issue | 🟡 Minor需要修复 clang-format 格式问题。
Pipeline 报告 Lines 13 和 15 存在 clang-format 违规（内联注释对齐问题）。请运行
clang-format重新格式化这些行。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/bootloader/src/flash/validation.hpp` around lines 12 - 17, The inline comment alignment for kSramEndAddress and kImageHashSuffixSize violates clang-format; run clang-format on validation.hpp (or manually reflow the two lines) so the trailing comments align according to the project's style, ensuring the declarations for kSramEndAddress and kImageHashSuffixSize keep their types/expressions unchanged but adjust spacing so the inline comments (the "Exclusive" for kSramEndAddress and the "// 36" for kImageHashSuffixSize) conform to clang-format output.
🧹 Nitpick comments (6)
firmware/common/cmake/librmcs_firmware.cmake (1)
83-88: 建议验证 DFU 参数是否为必填项。当前仅验证了
TARGET和OUTPUT_DFU，但VENDOR_ID、PRODUCT_ID和DEVICE_ID未做检查。如果调用者遗漏这些参数，dfu-suffix将收到空值，可能导致生成的 DFU 镜像元数据不完整或无效。💡 建议添加参数验证
if(NOT ARG_OUTPUT_DFU) message(FATAL_ERROR "librmcs_add_dfu_image requires OUTPUT_DFU") endif() + if(NOT ARG_VENDOR_ID) + message(FATAL_ERROR "librmcs_add_dfu_image requires VENDOR_ID") + endif() + if(NOT ARG_PRODUCT_ID) + message(FATAL_ERROR "librmcs_add_dfu_image requires PRODUCT_ID") + endif() + if(NOT ARG_DEVICE_ID) + message(FATAL_ERROR "librmcs_add_dfu_image requires DEVICE_ID") + endif()Also applies to: 126-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/common/cmake/librmcs_firmware.cmake` around lines 83 - 88, The CMake function librmcs_add_dfu_image currently only validates ARG_TARGET and ARG_OUTPUT_DFU, leaving VENDOR_ID, PRODUCT_ID and DEVICE_ID unchecked which can pass empty values into dfu-suffix; update the validation block in librmcs_firmware.cmake (the checks around ARG_TARGET/ARG_OUTPUT_DFU and the similar block at lines 126-134) to also require non-empty VENDOR_ID, PRODUCT_ID and DEVICE_ID by adding FATAL_ERROR checks for each of those symbols so callers must provide them before dfu-suffix is constructed.firmware/rmcs_board/CMakeLists.txt (1)
12-14:CMAKE_BUILD_TYPE大小写不一致。此处默认值为
"debug"（小写），而firmware/c_board/CMakeLists.txt使用"Debug"（首字母大写）。CMake 惯例是使用首字母大写的构建类型（Debug、Release、RelWithDebInfo、MinSizeRel）。虽然 HPM SDK 可能接受小写，但项目内保持一致性有助于避免混淆。
💡 建议统一使用首字母大写
if(NOT CMAKE_BUILD_TYPE) - set(CMAKE_BUILD_TYPE "debug") + set(CMAKE_BUILD_TYPE "Debug") endif()同时需要更新
firmware/rmcs_board/bootloader/CMakeLists.txt第 44 行的检查：-if(CMAKE_BUILD_TYPE STREQUAL "debug") +if(CMAKE_BUILD_TYPE STREQUAL "Debug")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/CMakeLists.txt` around lines 12 - 14, The CMake build type default is set to "debug" (lowercase) but other CMakeLists in the project use "Debug" (capitalized); update the default in the CMakeLists that sets CMAKE_BUILD_TYPE to "Debug" (capital D) and also update the build-type validation logic in the bootloader CMakeLists (the check around the build-type at ~line 44) to accept or compare against the capitalized "Debug" value so all CMAKE_BUILD_TYPE usages are consistent with CMake conventions.firmware/rmcs_board/bootloader/src/flash/writer.hpp (1)
40-48: 可选优化：考虑在循环内缓存sector_size。
XpiNor::instance().sector_size()在循环中多次调用。虽然instance()返回静态引用开销很小，但缓存到局部变量可略微提升可读性和性能。♻️ 建议的优化
void write(uintptr_t address, std::span<const std::byte> data) { utility::assert_debug(!data.empty()); utility::assert_debug(address >= kAppStartAddress && address < kAppEndAddress); utility::assert_debug( static_cast<uint64_t>(address) + data.size() <= static_cast<uint64_t>(kAppEndAddress)); + const uint32_t sector_size = XpiNor::instance().sector_size(); size_t input_offset = 0U; while (input_offset < data.size()) { const uintptr_t write_address = address + input_offset; const uintptr_t sector_address = - write_address - (write_address % XpiNor::instance().sector_size()); + write_address - (write_address % sector_size); activate_sector(sector_address); const uintptr_t sector_offset = write_address - active_sector_address_; const size_t writable = - static_cast<size_t>(XpiNor::instance().sector_size() - sector_offset); + static_cast<size_t>(sector_size - sector_offset); const size_t chunk_size = std::min(writable, data.size() - input_offset); // ... - if ((sector_offset + chunk_size) == XpiNor::instance().sector_size()) { + if ((sector_offset + chunk_size) == sector_size) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/writer.hpp` around lines 40 - 48, Cache the repeated call to XpiNor::instance().sector_size() into a local variable (e.g., sector_size) and use that variable inside the while loop where write_address, sector_address, sector_offset, and writable are computed; update uses in calculations and when calling activate_sector if needed so you avoid calling XpiNor::instance().sector_size() multiple times per iteration while keeping logic for activate_sector(sector_address) and active_sector_address_ unchanged.firmware/rmcs_board/bootloader/src/main.cpp (1)
25-25: 明确get_usb_descriptors()调用意图。返回值被显式丢弃
(void)，推测是触发描述符初始化的副作用。建议添加简短注释说明调用目的，便于后续维护。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/main.cpp` at line 25, 在 main.cpp 中对 (void)usb::get_usb_descriptors(); 的意图不明确——请在该调用上方添加一行简短注释说明为什么显式丢弃返回值（例如“仅用于触发 USB 描述符的初始化/注册，返回值可忽略”），并指明该调用依赖的副作用（触发描述符初始化或注册），以便后续维护人员理解调用目的和返回值被忽略的合理性。firmware/rmcs_board/bootloader/src/flash/metadata.hpp (1)
153-158: 把 metadata 的擦除范围和扫描范围绑定起来。Line 154 现在只擦
kMetadataStartAddress所在的一个 sector，但scan_latest_valid_slot()扫的是整个[kMetadataStartAddress, kMetadataEndAddress)。当前布局刚好是一个 sector，所以代码能工作；不过这里最好显式锁死这个前提，避免以后 metadata 区扩容后留下旧 slot。♻️ 可考虑的小改动
void erase_and_rescan() { + static_assert(kMetadataEndAddress - kMetadataStartAddress == kFlashSectorSize); XpiNor::instance().erase_sector(kMetadataStartAddress); scan_latest_valid_slot(); utility::assert_always( latest_valid_slot_ != nullptr && latest_valid_slot_state_ == DataSlotState::kEmpty); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/metadata.hpp` around lines 153 - 158, erase_and_rescan() currently erases only the sector containing kMetadataStartAddress but then scans the whole metadata range [kMetadataStartAddress, kMetadataEndAddress), which will break if metadata grows; change erase_and_rescan() (and its use of XpiNor::instance().erase_sector) to erase every sector that covers the full metadata range from kMetadataStartAddress up to kMetadataEndAddress (e.g., loop over sector addresses or use an erase_range API) before calling scan_latest_valid_slot(), and keep the final assertion on latest_valid_slot_ and latest_valid_slot_state_ (or if you prefer to retain the single-sector approach, add an explicit static_assert or runtime check that kMetadataEndAddress - kMetadataStartAddress <= sector_size to make the one-sector assumption explicit).firmware/rmcs_board/bootloader/src/flash/validation.hpp (1)
15-17: 去掉 hash magic 的字节序耦合。Line 56 把 suffix header 解释成
uint32_t再和0x48415348U比较，这会把 on-flash 格式隐式绑到“生成端和消费端必须使用同一字序写这个 32 位字”。这里直接按 4 个字节比较"HASH"会更稳，也能避免以后在产物生成脚本里踩到反序问题。♻️ 可考虑的小改动
+#include <array> `#include` <cstdint> `#include` <cstring> -inline constexpr uint32_t kImageHashMagic = 0x48415348U; // "HASH" +inline constexpr std::array<uint8_t, 4> kImageHashMagic = {'H', 'A', 'S', 'H'}; inline constexpr uint32_t kImageHashSuffixSize = - sizeof(uint32_t) + static_cast<uint32_t>(crypto::kSha256DigestSize); + static_cast<uint32_t>(kImageHashMagic.size() + crypto::kSha256DigestSize); - uint32_t suffix_magic = 0U; - std::memcpy(&suffix_magic, suffix_ptr, sizeof(suffix_magic)); - if (suffix_magic != kImageHashMagic) + if (std::memcmp(suffix_ptr, kImageHashMagic.data(), kImageHashMagic.size()) != 0) return false; const uint32_t firmware_size = size - kImageHashSuffixSize; - const uint8_t* expected_sha256 = suffix_ptr + sizeof(uint32_t); + const uint8_t* expected_sha256 = suffix_ptr + kImageHashMagic.size();Also applies to: 52-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/validation.hpp` around lines 15 - 17, 当前以 uint32_t 常量 kImageHashMagic 并在读取时把后缀 header 解为 uint32_t 再比较会把 on-flash 格式和主机字节序耦合；改为用按字节比较四字节 ASCII 标记 "HASH"（例如将 kImageHashMagic 改为 constexpr char[4] 或使用 string/byte-array 常量）并在验证逻辑中对比原始 4 字节（如 memcmp 或逐字节比较），同时保留 kImageHashSuffixSize 计算不变，确保读取代码不再做 uint32_t 端序解释以避免反序问题。
🤖 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/bootloader/src/usb/dfu.hpp`:
- Around line 40-45: The download(uint8_t alt, uint16_t block_num, const
uint8_t* data, uint16_t length) path rejects length==0 as DFU_STATUS_ERR_NOTDONE
which breaks legitimate end-of-transfer DNLOAD requests; instead, treat a
zero-length DNLOAD as a valid transfer-complete marker when alt==kDfuAltFlash:
do not return DFU_STATUS_ERR_NOTDONE for length==0, return the success/accept
status used elsewhere (e.g., DFU_STATUS_OK or the same status the state machine
expects) and ensure any downstream logic (manifest transition) is triggered
accordingly so standard DFU hosts can complete the transfer.
---
Outside diff comments:
In `@firmware/c_board/bootloader/src/flash/validation.hpp`:
- Around line 12-17: The inline comment alignment for kSramEndAddress and
kImageHashSuffixSize violates clang-format; run clang-format on validation.hpp
(or manually reflow the two lines) so the trailing comments align according to
the project's style, ensuring the declarations for kSramEndAddress and
kImageHashSuffixSize keep their types/expressions unchanged but adjust spacing
so the inline comments (the "Exclusive" for kSramEndAddress and the "// 36" for
kImageHashSuffixSize) conform to clang-format output.
---
Nitpick comments:
In `@firmware/common/cmake/librmcs_firmware.cmake`:
- Around line 83-88: The CMake function librmcs_add_dfu_image currently only
validates ARG_TARGET and ARG_OUTPUT_DFU, leaving VENDOR_ID, PRODUCT_ID and
DEVICE_ID unchecked which can pass empty values into dfu-suffix; update the
validation block in librmcs_firmware.cmake (the checks around
ARG_TARGET/ARG_OUTPUT_DFU and the similar block at lines 126-134) to also
require non-empty VENDOR_ID, PRODUCT_ID and DEVICE_ID by adding FATAL_ERROR
checks for each of those symbols so callers must provide them before dfu-suffix
is constructed.
In `@firmware/rmcs_board/bootloader/src/flash/metadata.hpp`:
- Around line 153-158: erase_and_rescan() currently erases only the sector
containing kMetadataStartAddress but then scans the whole metadata range
[kMetadataStartAddress, kMetadataEndAddress), which will break if metadata
grows; change erase_and_rescan() (and its use of
XpiNor::instance().erase_sector) to erase every sector that covers the full
metadata range from kMetadataStartAddress up to kMetadataEndAddress (e.g., loop
over sector addresses or use an erase_range API) before calling
scan_latest_valid_slot(), and keep the final assertion on latest_valid_slot_ and
latest_valid_slot_state_ (or if you prefer to retain the single-sector approach,
add an explicit static_assert or runtime check that kMetadataEndAddress -
kMetadataStartAddress <= sector_size to make the one-sector assumption
explicit).
In `@firmware/rmcs_board/bootloader/src/flash/validation.hpp`:
- Around line 15-17: 当前以 uint32_t 常量 kImageHashMagic 并在读取时把后缀 header 解为 uint32_t
再比较会把 on-flash 格式和主机字节序耦合；改为用按字节比较四字节 ASCII 标记 "HASH"（例如将 kImageHashMagic 改为
constexpr char[4] 或使用 string/byte-array 常量）并在验证逻辑中对比原始 4 字节（如 memcmp
或逐字节比较），同时保留 kImageHashSuffixSize 计算不变，确保读取代码不再做 uint32_t 端序解释以避免反序问题。
In `@firmware/rmcs_board/bootloader/src/flash/writer.hpp`:
- Around line 40-48: Cache the repeated call to XpiNor::instance().sector_size()
into a local variable (e.g., sector_size) and use that variable inside the while
loop where write_address, sector_address, sector_offset, and writable are
computed; update uses in calculations and when calling activate_sector if needed
so you avoid calling XpiNor::instance().sector_size() multiple times per
iteration while keeping logic for activate_sector(sector_address) and
active_sector_address_ unchanged.
In `@firmware/rmcs_board/bootloader/src/main.cpp`:
- Line 25: 在 main.cpp 中对 (void)usb::get_usb_descriptors();
的意图不明确——请在该调用上方添加一行简短注释说明为什么显式丢弃返回值（例如“仅用于触发 USB
描述符的初始化/注册，返回值可忽略”），并指明该调用依赖的副作用（触发描述符初始化或注册），以便后续维护人员理解调用目的和返回值被忽略的合理性。
In `@firmware/rmcs_board/CMakeLists.txt`:
- Around line 12-14: The CMake build type default is set to "debug" (lowercase)
but other CMakeLists in the project use "Debug" (capitalized); update the
default in the CMakeLists that sets CMAKE_BUILD_TYPE to "Debug" (capital D) and
also update the build-type validation logic in the bootloader CMakeLists (the
check around the build-type at ~line 44) to accept or compare against the
capitalized "Debug" value so all CMAKE_BUILD_TYPE usages are consistent with
CMake conventions.
🪄 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: f1e8c14c-8afe-4686-baea-8e9ae17bc52f
📒 Files selected for processing (62)
.github/workflows/lint.yml.scripts/lint-targets.ymlAGENTS.mdDockerfile.build_firmwarefirmware/c_board/CMakeLists.txtfirmware/c_board/app/CMakeLists.txtfirmware/c_board/app/src/timer/timer.hppfirmware/c_board/app/src/usb/usb_descriptors.hppfirmware/c_board/bootloader/src/crypto/sha256.hppfirmware/c_board/bootloader/src/flash/validation.hppfirmware/c_board/bootloader/src/flash/writer.hppfirmware/c_board/bootloader/src/main.cppfirmware/c_board/bootloader/src/usb/dfu.hppfirmware/c_board/bootloader/src/utility/jump.hppfirmware/common/cmake/librmcs_firmware.cmakefirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/include/tusb_config.hfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/app.hppfirmware/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/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/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/rx_buffer.hppfirmware/rmcs_board/app/src/uart/tx_buffer.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/helper.hppfirmware/rmcs_board/app/src/usb/interrupt_safe_buffer.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.cppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.cppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/app/src/utility/assert.cppfirmware/rmcs_board/app/src/utility/boot_mailbox.hppfirmware/rmcs_board/app/src/utility/interrupt_lock.hppfirmware/rmcs_board/app/src/utility/lazy.hppfirmware/rmcs_board/app/src/utility/ring_buffer.hppfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/include/tusb_config.hfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/flash/layout.hppfirmware/rmcs_board/bootloader/src/flash/metadata.hppfirmware/rmcs_board/bootloader/src/flash/validation.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hppfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.cppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/assert.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/src/utility/jump.hppfirmware/rmcs_board/bsp/hpm_sdkfirmware/rmcs_board/cmake/merge_compile_commands.cmake
✅ Files skipped from review due to trivial changes (25)
- AGENTS.md
- firmware/rmcs_board/app/src/uart/tx_buffer.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/app/src/app.hpp
- firmware/rmcs_board/app/src/spi/spi.hpp
- .github/workflows/lint.yml
- firmware/rmcs_board/app/src/can/can.hpp
- firmware/c_board/app/src/timer/timer.hpp
- firmware/rmcs_board/app/src/can/can.cpp
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/app/src/uart/uart.hpp
- firmware/rmcs_board/app/src/spi/bmi088/gyro.hpp
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- firmware/rmcs_board/app/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/app/src/utility/lazy.hpp
- firmware/c_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/app/src/usb/vendor.hpp
- firmware/rmcs_board/bsp/hpm_sdk
- firmware/rmcs_board/app/include/tusb_config.h
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/cmake/merge_compile_commands.cmake
- firmware/rmcs_board/bootloader/src/usb/dfu.cpp
- firmware/rmcs_board/bootloader/src/flash/layout.hpp
- firmware/rmcs_board/app/src/usb/usb_descriptors.hpp
🚧 Files skipped from review as they are similar to previous changes (13)
- firmware/rmcs_board/app/src/gpio/gpio.cpp
- firmware/c_board/bootloader/src/utility/jump.hpp
- .scripts/lint-targets.yml
- firmware/rmcs_board/app/src/app.cpp
- firmware/rmcs_board/app/src/usb/vendor.cpp
- firmware/c_board/bootloader/src/main.cpp
- Dockerfile.build_firmware
- firmware/rmcs_board/bootloader/src/utility/jump.hpp
- firmware/rmcs_board/bootloader/src/utility/assert.hpp
- firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/bootloader/src/flash/xpi_nor.hpp
- firmware/rmcs_board/app/CMakeLists.txt
qzhhhi
force-pushed
the
dev/rmcsboard-bootloader
branch
from
fbf946e to
eb38b91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
firmware/rmcs_board/app/src/usb/usb_descriptors.hpp (2)
184-186: 字符串索引使用命名常量更佳
TUD_DFU_RT_DESCRIPTOR中的字符串索引4与get_string_descriptor()中的case 4隐式耦合。建议定义命名常量以提高可维护性：♻️ 建议的重构
+ static constexpr uint8_t kDfuRuntimeStringIndex = 4; + // ... - case 4: str = kDfuRuntimeString; break; + case kDfuRuntimeStringIndex: str = kDfuRuntimeString; break; // ... - TUD_DFU_RT_DESCRIPTOR( - kItfNumDfuRuntime, 4, DFU_ATTR_CAN_DOWNLOAD | DFU_ATTR_WILL_DETACH, 1000, 1024), + TUD_DFU_RT_DESCRIPTOR( + kItfNumDfuRuntime, kDfuRuntimeStringIndex, + DFU_ATTR_CAN_DOWNLOAD | DFU_ATTR_WILL_DETACH, 1000, 1024),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/usb/usb_descriptors.hpp` around lines 184 - 186, TUD_DFU_RT_DESCRIPTOR currently uses the literal string index 4 which is implicitly coupled to the `case 4` in `get_string_descriptor()`; define a named constant (e.g., kStrIdxDfuRuntime) and replace the hardcoded 4 in the TUD_DFU_RT_DESCRIPTOR invocation and the corresponding `case 4` in `get_string_descriptor()` with that constant so the descriptor and string switch use the same named symbol for clarity and maintainability.
162-167: 考虑将接口编号枚举暴露给其他模块使用
InterfaceNumber枚举定义在私有区域，但根据相关代码片段，vendor.cpp:20中使用硬编码的if (itf != 0)来检查 Vendor 接口。若将来接口编号变化，两处需同步修改。建议将此枚举移至 public 区域或提取到共享头文件，使
vendor.cpp可以使用kItfNumVendor而非魔数0：// vendor.cpp 中可改为： if (itf != UsbDescriptors::kItfNumVendor) [[unlikely]] return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/usb/usb_descriptors.hpp` around lines 162 - 167, The InterfaceNumber enum (enum InterfaceNumber, constants like kItfNumVendor) is defined in a private section so vendor.cpp uses a hardcoded 0; move the enum into the public section of UsbDescriptors (or into a small shared header) so other modules can reference the symbolic name, then update vendor.cpp to replace the magic literal with UsbDescriptors::kItfNumVendor (or the shared-header name) to avoid desync when interface numbers change.firmware/rmcs_board/app/CMakeLists.txt (1)
46-48: 不要在 Debug 构建里单独降到-Og。这里会让
rmcs_board_app的 Debug 行为偏离仓库现有策略，也更容易把只在优化下暴露的问题藏起来。更稳妥的是删除这段 override，继续沿用仓库统一的 Debug 优化级别。♻️ 建议修改
-if(CMAKE_BUILD_TYPE STREQUAL "debug") - sdk_compile_options(-Og) -endif()Based on learnings，仓库中的 Debug 构建刻意保持高优化级别（GCC/Clang 为 -O3，MSVC 为 /O2），以便尽早暴露 UB。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/CMakeLists.txt` around lines 46 - 48, Remove the local override that forces -Og in Debug: delete the if(CMAKE_BUILD_TYPE STREQUAL "debug") ... sdk_compile_options(-Og) ... endif() block from CMakeLists.txt so rmcs_board_app inherits the repository's unified Debug optimization level; ensure no other sdk_compile_options call in this file reassigns optimization for the target.firmware/rmcs_board/app/src/utility/boot_mailbox.hpp (1)
14-35: 建议把 mailbox 协议常量下沉到 app/bootloader 共用头。Line 27-29 这组常量和
firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp是强耦合协议，但现在两边各维护一份。以后只要任一侧单独改了 magic / request / GPR index，runtime reboot 到 DFU 就会静默失效，而且编译期看不出来。至少把协议常量抽到 shared header，避免两边实现漂移。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/src/utility/boot_mailbox.hpp` around lines 14 - 35, The mailbox protocol constants (kMailboxMagic, kMailboxRequestEnterDfu, kMagicGprIndex) in BootMailbox are duplicated between the app and the bootloader; extract these constants into a shared header used by both firmware/rmcs_board/app and firmware/rmcs_board/bootloader, update BootMailbox::write_pair / BootMailbox::reboot_to_bootloader to include that shared header and use the shared symbols, and remove the duplicate definitions from the app-side boot_mailbox.hpp so both sides reference the single source of truth for the mailbox protocol.firmware/common/cmake/librmcs_firmware.cmake (1)
83-114: 把librmcs_add_dfu_image()的参数契约收紧一点。现在如果调用方漏传
VENDOR_ID/PRODUCT_ID/DEVICE_ID，这里会一直到构建阶段才在dfu-suffix里以比较难定位的参数错误失败；同时如果同时给了INPUT_BINARY和OUTPUT_BINARY，当前实现也会静默以后者为准。既然这是共享 helper，建议在配置期直接FATAL_ERROR，把失败点前移。♻️ 建议修改
if(NOT ARG_OUTPUT_DFU) message(FATAL_ERROR "librmcs_add_dfu_image requires OUTPUT_DFU") endif() + if(NOT "${ARG_INPUT_BINARY}" STREQUAL "" AND NOT "${ARG_OUTPUT_BINARY}" STREQUAL "") + message( + FATAL_ERROR + "librmcs_add_dfu_image accepts INPUT_BINARY or OUTPUT_BINARY, but not both" + ) + endif() + if("${ARG_VENDOR_ID}" STREQUAL "" OR "${ARG_PRODUCT_ID}" STREQUAL "" + OR "${ARG_DEVICE_ID}" STREQUAL "") + message( + FATAL_ERROR + "librmcs_add_dfu_image requires VENDOR_ID, PRODUCT_ID and DEVICE_ID" + ) + endif() set(input_binary "${ARG_INPUT_BINARY}")Also applies to: 125-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/common/cmake/librmcs_firmware.cmake` around lines 83 - 114, Add stricter argument validation in librmcs_add_dfu_image: FATAL_ERROR at configure-time if any of VENDOR_ID, PRODUCT_ID, or DEVICE_ID are missing, and FATAL_ERROR if both INPUT_BINARY and OUTPUT_BINARY are provided (instead of silently preferring OUTPUT_BINARY). Also ensure the existing checks that require INPUT_ELF and CMAKE_OBJCOPY when OUTPUT_BINARY is used remain, and replicate these stricter validations in the other dfu-suffix handling block later in the same file.firmware/rmcs_board/bootloader/src/flash/metadata.hpp (1)
153-158:erase_and_rescan()最好按 metadata 整个区间擦除。
scan_latest_valid_slot()会扫描到kMetadataEndAddress，但这里重置路径只擦除了首个扇区。当前布局刚好能工作；如果后面 metadata 区扩容，这里会静默留下旧 slot。✏️ 建议修改
void erase_and_rescan() { - XpiNor::instance().erase_sector(kMetadataStartAddress); + for (uintptr_t address = kMetadataStartAddress; address < kMetadataEndAddress; + address += kFlashSectorSize) { + XpiNor::instance().erase_sector(address); + } scan_latest_valid_slot(); utility::assert_always( latest_valid_slot_ != nullptr && latest_valid_slot_state_ == DataSlotState::kEmpty); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/flash/metadata.hpp` around lines 153 - 158, erase_and_rescan currently only erases the first sector at kMetadataStartAddress but scan_latest_valid_slot scans up to kMetadataEndAddress, so if the metadata region grows old slots can remain; modify erase_and_rescan to erase the entire metadata range from kMetadataStartAddress to kMetadataEndAddress (e.g. loop calling XpiNor::instance().erase_sector or use an erase_range helper) before calling scan_latest_valid_slot(), and keep the existing assertion on latest_valid_slot_ and latest_valid_slot_state_.firmware/rmcs_board/bootloader/src/usb/dfu.hpp (1)
110-121: 建议把“成功收尾”和“中止会话”拆成两个 helper。成功路径现在也走
reset_transfer_state()，等于把正常收尾也绑到了flash_writer_.abort_session()的语义上。当前实现大概率没问题，但后续只要Writer对finish_session()/abort_session()的契约收紧，这里就会变成隐式耦合。更稳妥的做法是把“清状态”和“中止 session”分开；manifest()只清状态，abort()/失败路径继续 abort。♻️ 可参考的拆分方式
- reset_transfer_state(); + clear_transfer_state(); reset_requested_ = true; reset_requested_tick_ = mchtmr_get_count(HPM_MCHTMR); return DFU_STATUS_OK; @@ - reset_transfer_state(); + abort_transfer_state(); @@ - uint8_t fail(uint8_t status) { - reset_transfer_state(); + uint8_t fail(uint8_t status) { + abort_transfer_state(); return status; } - void reset_transfer_state() { - flash_writer_.abort_session(); + void clear_transfer_state() { expected_block_ = 0U; downloaded_size_ = 0U; session_started_ = false; } + + void abort_transfer_state() { + flash_writer_.abort_session(); + clear_transfer_state(); + }
block_num == 0U的 restart 路径也同样改成abort_transfer_state()即可。Also applies to: 147-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/bootloader/src/usb/dfu.hpp` around lines 110 - 121, Split state-clearing from writer-abort: keep reset_transfer_state() (or rename to abort_transfer_state()/clear_transfer_state()) as the helper that only clears DFU state and use a separate call to flash_writer_.abort_session() for abort semantics. Update manifest() / the success path to only clear state (call the new clear helper) and update abort(uint8_t alt) and the block_num==0 restart path to call flash_writer_.abort_session() followed by the clear helper (or call an abort_transfer_state() wrapper that does both), ensuring finish_session()/abort_session() contracts are not conflated.
🤖 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/bootloader/src/flash/metadata.hpp`:
- Around line 31-34: The code increments a pointer-to-flash `latest_valid_slot_`
with `++` (when `latest_valid_slot_state_ == DataSlotState::kReady`), which is
undefined because the flash region is not a true `DataSlot[]`; change this to do
address arithmetic like `scan_latest_valid_slot()` does: convert
`latest_valid_slot_` to `uintptr_t`, add `sizeof(DataSlot)` (or the slot
stride), compare the new address against `kMetadataEndAddress`, then cast back
to `DataSlot*` and assign to `latest_valid_slot_`; if the computed address is >=
`kMetadataEndAddress` call `erase_and_rescan()` as before. Use the same pattern
and constants as `scan_latest_valid_slot()` to locate the fix.
---
Nitpick comments:
In `@firmware/common/cmake/librmcs_firmware.cmake`:
- Around line 83-114: Add stricter argument validation in librmcs_add_dfu_image:
FATAL_ERROR at configure-time if any of VENDOR_ID, PRODUCT_ID, or DEVICE_ID are
missing, and FATAL_ERROR if both INPUT_BINARY and OUTPUT_BINARY are provided
(instead of silently preferring OUTPUT_BINARY). Also ensure the existing checks
that require INPUT_ELF and CMAKE_OBJCOPY when OUTPUT_BINARY is used remain, and
replicate these stricter validations in the other dfu-suffix handling block
later in the same file.
In `@firmware/rmcs_board/app/CMakeLists.txt`:
- Around line 46-48: Remove the local override that forces -Og in Debug: delete
the if(CMAKE_BUILD_TYPE STREQUAL "debug") ... sdk_compile_options(-Og) ...
endif() block from CMakeLists.txt so rmcs_board_app inherits the repository's
unified Debug optimization level; ensure no other sdk_compile_options call in
this file reassigns optimization for the target.
In `@firmware/rmcs_board/app/src/usb/usb_descriptors.hpp`:
- Around line 184-186: TUD_DFU_RT_DESCRIPTOR currently uses the literal string
index 4 which is implicitly coupled to the `case 4` in
`get_string_descriptor()`; define a named constant (e.g., kStrIdxDfuRuntime) and
replace the hardcoded 4 in the TUD_DFU_RT_DESCRIPTOR invocation and the
corresponding `case 4` in `get_string_descriptor()` with that constant so the
descriptor and string switch use the same named symbol for clarity and
maintainability.
- Around line 162-167: The InterfaceNumber enum (enum InterfaceNumber, constants
like kItfNumVendor) is defined in a private section so vendor.cpp uses a
hardcoded 0; move the enum into the public section of UsbDescriptors (or into a
small shared header) so other modules can reference the symbolic name, then
update vendor.cpp to replace the magic literal with
UsbDescriptors::kItfNumVendor (or the shared-header name) to avoid desync when
interface numbers change.
In `@firmware/rmcs_board/app/src/utility/boot_mailbox.hpp`:
- Around line 14-35: The mailbox protocol constants (kMailboxMagic,
kMailboxRequestEnterDfu, kMagicGprIndex) in BootMailbox are duplicated between
the app and the bootloader; extract these constants into a shared header used by
both firmware/rmcs_board/app and firmware/rmcs_board/bootloader, update
BootMailbox::write_pair / BootMailbox::reboot_to_bootloader to include that
shared header and use the shared symbols, and remove the duplicate definitions
from the app-side boot_mailbox.hpp so both sides reference the single source of
truth for the mailbox protocol.
In `@firmware/rmcs_board/bootloader/src/flash/metadata.hpp`:
- Around line 153-158: erase_and_rescan currently only erases the first sector
at kMetadataStartAddress but scan_latest_valid_slot scans up to
kMetadataEndAddress, so if the metadata region grows old slots can remain;
modify erase_and_rescan to erase the entire metadata range from
kMetadataStartAddress to kMetadataEndAddress (e.g. loop calling
XpiNor::instance().erase_sector or use an erase_range helper) before calling
scan_latest_valid_slot(), and keep the existing assertion on latest_valid_slot_
and latest_valid_slot_state_.
In `@firmware/rmcs_board/bootloader/src/usb/dfu.hpp`:
- Around line 110-121: Split state-clearing from writer-abort: keep
reset_transfer_state() (or rename to
abort_transfer_state()/clear_transfer_state()) as the helper that only clears
DFU state and use a separate call to flash_writer_.abort_session() for abort
semantics. Update manifest() / the success path to only clear state (call the
new clear helper) and update abort(uint8_t alt) and the block_num==0 restart
path to call flash_writer_.abort_session() followed by the clear helper (or call
an abort_transfer_state() wrapper that does both), ensuring
finish_session()/abort_session() contracts are not conflated.
🪄 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: 4910ee00-97ff-4384-b905-5c7c48bc24b9
📒 Files selected for processing (62)
.github/workflows/lint.yml.scripts/lint-targets.ymlAGENTS.mdDockerfile.build_firmwarefirmware/c_board/CMakeLists.txtfirmware/c_board/app/CMakeLists.txtfirmware/c_board/app/src/timer/timer.hppfirmware/c_board/app/src/usb/usb_descriptors.hppfirmware/c_board/bootloader/src/crypto/sha256.hppfirmware/c_board/bootloader/src/flash/validation.hppfirmware/c_board/bootloader/src/flash/writer.hppfirmware/c_board/bootloader/src/main.cppfirmware/c_board/bootloader/src/usb/dfu.hppfirmware/c_board/bootloader/src/utility/jump.hppfirmware/common/cmake/librmcs_firmware.cmakefirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/include/tusb_config.hfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/app.hppfirmware/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/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/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/rx_buffer.hppfirmware/rmcs_board/app/src/uart/tx_buffer.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/helper.hppfirmware/rmcs_board/app/src/usb/interrupt_safe_buffer.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.cppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.cppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/app/src/utility/assert.cppfirmware/rmcs_board/app/src/utility/boot_mailbox.hppfirmware/rmcs_board/app/src/utility/interrupt_lock.hppfirmware/rmcs_board/app/src/utility/lazy.hppfirmware/rmcs_board/app/src/utility/ring_buffer.hppfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/include/tusb_config.hfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/flash/layout.hppfirmware/rmcs_board/bootloader/src/flash/metadata.hppfirmware/rmcs_board/bootloader/src/flash/validation.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hppfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.cppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/assert.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/src/utility/jump.hppfirmware/rmcs_board/bsp/hpm_sdkfirmware/rmcs_board/cmake/merge_compile_commands.cmake
✅ Files skipped from review due to trivial changes (27)
- firmware/c_board/app/src/timer/timer.hpp
- firmware/rmcs_board/app/src/usb/vendor.hpp
- firmware/rmcs_board/app/src/can/can.cpp
- firmware/c_board/app/src/usb/usb_descriptors.hpp
- AGENTS.md
- firmware/rmcs_board/app/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/app/src/app.hpp
- firmware/rmcs_board/bsp/hpm_sdk
- firmware/rmcs_board/app/src/uart/uart.hpp
- firmware/rmcs_board/app/src/uart/tx_buffer.hpp
- firmware/rmcs_board/app/src/can/can.hpp
- firmware/rmcs_board/app/src/spi/spi.hpp
- firmware/rmcs_board/app/src/utility/lazy.hpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- firmware/rmcs_board/app/src/spi/bmi088/gyro.hpp
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/app/include/tusb_config.h
- firmware/rmcs_board/bootloader/src/utility/jump.hpp
- firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/bootloader/CMakeLists.txt
- firmware/rmcs_board/bootloader/src/main.cpp
- firmware/rmcs_board/bootloader/src/flash/layout.hpp
- firmware/rmcs_board/cmake/merge_compile_commands.cmake
- firmware/rmcs_board/bootloader/src/usb/dfu.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/lint.yml
- firmware/rmcs_board/app/src/gpio/gpio.cpp
- firmware/c_board/bootloader/src/flash/writer.hpp
- firmware/rmcs_board/app/src/usb/vendor.cpp
- .scripts/lint-targets.yml
- firmware/rmcs_board/app/src/app.cpp
- firmware/c_board/bootloader/src/utility/jump.hpp
- firmware/c_board/bootloader/src/main.cpp
- Dockerfile.build_firmware
- firmware/c_board/bootloader/src/usb/dfu.hpp
- firmware/c_board/CMakeLists.txt
- firmware/rmcs_board/bootloader/src/crypto/sha256.hpp
- firmware/rmcs_board/bootloader/src/utility/assert.hpp
…er behavior - Extract shared firmware CMake helpers to firmware/common/ - Align validation and metadata commit timing with rmcs_board bootloader - Support non-word-aligned DFU downloads and fix buffered write abort
- Turn firmware/rmcs_board into a superbuild that delegates to app/ - Move the existing rmcs_board application sources under firmware/rmcs_board/app - Update CI, lint targets, and firmware packaging for the new output layout - Prepare rmcs_board for a future bootloader subbuild
qzhhhi
force-pushed
the
dev/rmcsboard-bootloader
branch
from
eb38b91 to
4c19d18
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
firmware/rmcs_board/app/CMakeLists.txt (1)
46-48: 这里最好把 build type 比较写成大小写无关，或者直接删掉这段。Line 46 现在只匹配小写
debug；如果上层传的是 CMake 更常见的Debug，-Og就完全不会生效。反过来，如果项目想继续沿用仓库里 Debug 保持高优化的约定，这段条件块本身也很容易让人误解。请顺手确认 superbuild 实际传入的值。 Based on learnings: In the librmcs project, Debug builds intentionally enable high optimization levels (-O3 for GCC/Clang, /O2 for MSVC) to expose undefined behavior early while keeping assertions active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/app/CMakeLists.txt` around lines 46 - 48, The CMake check uses a case-sensitive comparison (CMAKE_BUILD_TYPE STREQUAL "debug") so -Og won't apply when the common "Debug" value is used; change the conditional to be case-insensitive or remove it entirely. Update the CMake logic around CMAKE_BUILD_TYPE / sdk_compile_options to either compare a lowercase of CMAKE_BUILD_TYPE (e.g., transform or use STREQUAL against "Debug" and "debug") or delete the block if the project/superbuild intends to enforce high optimization for Debug; also verify what value the superbuild actually passes for CMAKE_BUILD_TYPE so that sdk_compile_options(-Og) is only applied when intended.firmware/common/cmake/librmcs_firmware.cmake (1)
83-98: 把VENDOR_ID/PRODUCT_ID/DEVICE_ID的契约写死会更稳。现在这里无论调用方有没有传值，都会无条件拼出
-v/-p/-d。后续只要有一个调用点漏传，失败就会延后到dfu-suffix的参数解析阶段，而且报错通常会比较绕；作为共享 helper，最好在入口直接FATAL_ERROR，或者只在参数存在时再追加对应 flag。请顺手确认一下这个 helper 是否真的打算支持缺省 IDs。✏️ 一种直接收紧契约的改法
if(NOT ARG_OUTPUT_DFU) message(FATAL_ERROR "librmcs_add_dfu_image requires OUTPUT_DFU") endif() + foreach(required_arg IN ITEMS VENDOR_ID PRODUCT_ID DEVICE_ID) + if("${ARG_${required_arg}}" STREQUAL "") + message(FATAL_ERROR "librmcs_add_dfu_image requires ${required_arg}") + endif() + endforeach()Also applies to: 116-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/common/cmake/librmcs_firmware.cmake` around lines 83 - 98, librmcs_add_dfu_image currently always appends -v/-p/-d flags even if VENDOR_ID/PRODUCT_ID/DEVICE_ID are empty, which defers errors to dfu-suffix parsing; fix by tightening the contract: in the librmcs_add_dfu_image entry (check ARG_TARGET/ARG_OUTPUT_DFU code path) validate VENDOR_ID, PRODUCT_ID and DEVICE_ID and call message(FATAL_ERROR ...) if any required ID is missing, or alternatively only append the corresponding -v/-p/-d flags when the respective variable is non-empty (i.e. test if VENDOR_ID/PRODUCT_ID/DEVICE_ID is defined and not empty before adding each flag); update the same logic block that handles ARG_OUTPUT_BINARY/CMAKE_OBJCOPY so the ID handling is consistent for all callers.
🤖 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/common/cmake/librmcs_firmware.cmake`:
- Around line 83-98: librmcs_add_dfu_image currently always appends -v/-p/-d
flags even if VENDOR_ID/PRODUCT_ID/DEVICE_ID are empty, which defers errors to
dfu-suffix parsing; fix by tightening the contract: in the librmcs_add_dfu_image
entry (check ARG_TARGET/ARG_OUTPUT_DFU code path) validate VENDOR_ID, PRODUCT_ID
and DEVICE_ID and call message(FATAL_ERROR ...) if any required ID is missing,
or alternatively only append the corresponding -v/-p/-d flags when the
respective variable is non-empty (i.e. test if VENDOR_ID/PRODUCT_ID/DEVICE_ID is
defined and not empty before adding each flag); update the same logic block that
handles ARG_OUTPUT_BINARY/CMAKE_OBJCOPY so the ID handling is consistent for all
callers.
In `@firmware/rmcs_board/app/CMakeLists.txt`:
- Around line 46-48: The CMake check uses a case-sensitive comparison
(CMAKE_BUILD_TYPE STREQUAL "debug") so -Og won't apply when the common "Debug"
value is used; change the conditional to be case-insensitive or remove it
entirely. Update the CMake logic around CMAKE_BUILD_TYPE / sdk_compile_options
to either compare a lowercase of CMAKE_BUILD_TYPE (e.g., transform or use
STREQUAL against "Debug" and "debug") or delete the block if the
project/superbuild intends to enforce high optimization for Debug; also verify
what value the superbuild actually passes for CMAKE_BUILD_TYPE so that
sdk_compile_options(-Og) is only applied when intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cabc916e-5def-405f-b654-dd8a87be4a87
📒 Files selected for processing (63)
.github/workflows/lint.yml.scripts/lint-targets.ymlAGENTS.mdDockerfile.build_firmwarefirmware/c_board/CMakeLists.txtfirmware/c_board/app/CMakeLists.txtfirmware/c_board/app/src/timer/timer.hppfirmware/c_board/app/src/usb/usb_descriptors.hppfirmware/c_board/bootloader/src/crypto/sha256.hppfirmware/c_board/bootloader/src/flash/metadata.hppfirmware/c_board/bootloader/src/flash/validation.hppfirmware/c_board/bootloader/src/flash/writer.hppfirmware/c_board/bootloader/src/main.cppfirmware/c_board/bootloader/src/usb/dfu.hppfirmware/c_board/bootloader/src/utility/jump.hppfirmware/common/cmake/librmcs_firmware.cmakefirmware/rmcs_board/CMakeLists.txtfirmware/rmcs_board/app/CMakeLists.txtfirmware/rmcs_board/app/include/tusb_config.hfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/app.hppfirmware/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/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/base.hppfirmware/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/rx_buffer.hppfirmware/rmcs_board/app/src/uart/tx_buffer.hppfirmware/rmcs_board/app/src/uart/uart.cppfirmware/rmcs_board/app/src/uart/uart.hppfirmware/rmcs_board/app/src/usb/helper.hppfirmware/rmcs_board/app/src/usb/interrupt_safe_buffer.hppfirmware/rmcs_board/app/src/usb/usb_descriptors.cppfirmware/rmcs_board/app/src/usb/usb_descriptors.hppfirmware/rmcs_board/app/src/usb/vendor.cppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/app/src/utility/assert.cppfirmware/rmcs_board/app/src/utility/boot_mailbox.hppfirmware/rmcs_board/app/src/utility/interrupt_lock.hppfirmware/rmcs_board/app/src/utility/lazy.hppfirmware/rmcs_board/app/src/utility/ring_buffer.hppfirmware/rmcs_board/bootloader/CMakeLists.txtfirmware/rmcs_board/bootloader/include/tusb_config.hfirmware/rmcs_board/bootloader/src/crypto/sha256.hppfirmware/rmcs_board/bootloader/src/flash/layout.hppfirmware/rmcs_board/bootloader/src/flash/metadata.hppfirmware/rmcs_board/bootloader/src/flash/validation.hppfirmware/rmcs_board/bootloader/src/flash/writer.hppfirmware/rmcs_board/bootloader/src/flash/xpi_nor.hppfirmware/rmcs_board/bootloader/src/main.cppfirmware/rmcs_board/bootloader/src/usb/dfu.cppfirmware/rmcs_board/bootloader/src/usb/dfu.hppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.cppfirmware/rmcs_board/bootloader/src/usb/usb_descriptors.hppfirmware/rmcs_board/bootloader/src/utility/assert.hppfirmware/rmcs_board/bootloader/src/utility/boot_mailbox.hppfirmware/rmcs_board/bootloader/src/utility/jump.hppfirmware/rmcs_board/bsp/hpm_sdkfirmware/rmcs_board/cmake/merge_compile_commands.cmake
✅ Files skipped from review due to trivial changes (27)
- firmware/rmcs_board/app/src/app.hpp
- firmware/rmcs_board/app/src/uart/uart.cpp
- firmware/rmcs_board/app/src/uart/tx_buffer.hpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- firmware/rmcs_board/app/src/spi/bmi088/base.hpp
- firmware/rmcs_board/app/src/gpio/gpio.cpp
- AGENTS.md
- firmware/rmcs_board/app/src/spi/spi.hpp
- firmware/rmcs_board/app/src/can/can.cpp
- firmware/rmcs_board/app/src/uart/uart.hpp
- firmware/rmcs_board/app/src/utility/lazy.hpp
- firmware/rmcs_board/app/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/app/src/can/can.hpp
- firmware/rmcs_board/bsp/hpm_sdk
- firmware/rmcs_board/app/src/usb/vendor.hpp
- firmware/c_board/app/src/timer/timer.hpp
- firmware/c_board/app/src/usb/usb_descriptors.hpp
- firmware/rmcs_board/bootloader/src/usb/usb_descriptors.cpp
- firmware/rmcs_board/bootloader/src/utility/boot_mailbox.hpp
- firmware/rmcs_board/cmake/merge_compile_commands.cmake
- firmware/rmcs_board/bootloader/src/utility/assert.hpp
- firmware/rmcs_board/app/src/spi/spi.cpp
- firmware/rmcs_board/bootloader/src/usb/dfu.cpp
- firmware/rmcs_board/bootloader/src/utility/jump.hpp
- firmware/rmcs_board/bootloader/src/flash/layout.hpp
- firmware/rmcs_board/bootloader/include/tusb_config.h
- Dockerfile.build_firmware
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/lint.yml
- firmware/c_board/bootloader/src/main.cpp
- firmware/rmcs_board/app/src/usb/vendor.cpp
- firmware/c_board/bootloader/src/flash/writer.hpp
- firmware/c_board/bootloader/src/utility/jump.hpp
- .scripts/lint-targets.yml
- firmware/rmcs_board/app/src/spi/bmi088/gyro.hpp
- firmware/rmcs_board/app/include/tusb_config.h
- firmware/c_board/bootloader/src/usb/dfu.hpp
- firmware/c_board/bootloader/src/crypto/sha256.hpp
- firmware/rmcs_board/bootloader/CMakeLists.txt
- firmware/rmcs_board/bootloader/src/crypto/sha256.hpp
- firmware/rmcs_board/bootloader/src/flash/writer.hpp
Member
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Member
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Member
Author
|
@coderabbitai approve |
✅ Actions performedComments resolved. Approval is disabled; enable |
qzhhhi
changed the title
- Introduce the rmcs_board bootloader target with DFU flashing, image validation, and metadata tracking - Add DFU runtime support and mailbox-based reboot handoff from the rmcs_board app - Reserve app flash space for the bootloader and generate rmcs_board_app.dfu artifacts - Merge rmcs_board app and bootloader compile commands and lint both source trees - Update firmware Docker packaging to export the bootloader ELF and versioned app DFU image
- Set firmware USB device descriptors to TUSB_CLASS_UNSPECIFIED for composite devices with interface-level class definitions. - Make the rmcs_board device descriptor and its accessor static to match the compile-time descriptor definition. - Fix clang-format violation in CBoard timer.hpp.
qzhhhi
force-pushed
the
dev/rmcsboard-bootloader
branch
from
4c19d18 to
949c0d1
Compare
qzhhhi
added a commit
that referenced
this pull request
Apr 4, 2026
- Introduce the rmcs_board bootloader target with DFU flashing, image validation, and metadata tracking - Add DFU runtime support and mailbox-based reboot handoff from the rmcs_board app - Reserve app flash space for the bootloader and generate rmcs_board_app.dfu artifacts - Merge rmcs_board app and bootloader compile commands and lint both source trees - Update firmware Docker packaging to export the bootloader ELF and versioned app DFU image
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.
refactor(firmware): Align CBoard DFU flow with future shared bootloader behavior (#39)
refactor(firmware): Split RmcsBoard app into a superbuild layout (#39)
feat(firmware): Add RmcsBoard DFU bootloader support (#39)
fix(firmware): Use interface-defined USB device classes (#39)
本 PR 提取并复用固件层的 CMake 辅助逻辑、重构 rmcs_board 为 superbuild（拆分 app/bootloader 子构建），并在 rmcs_board 中新增完整 bootloader 子系统（含 DFU、闪存布局/编程/元数据、镜像校验、USB 描述符、重启 mailbox、跳转逻辑等）。同时对 CBoard 固件的 DFU 流做了多处行为调整，以与未来 rmcs_board/bootloader 的语义对齐：候选镜像验证时机、元数据提交流程、缓冲写入的中止语义、以及允许非字对齐的 DFU 下载。
主要变更摘要
行为对齐要点与安全性关注
建议测试与关注点
影响评估