Skip to content

feat(firmware): Add USB transport and binary protocol - #6

Merged
qzhhhi merged 2 commits into
dev/v3from
feature/firmware
Dec 29, 2025
Merged

feat(firmware): Add USB transport and binary protocol#6
qzhhhi merged 2 commits into
dev/v3from
feature/firmware

Conversation

@qzhhhi

@qzhhhiqzhhhi commented Dec 26, 2025

Copy link
Copy Markdown
Member

USB传输和二进制协议固件实现(更新版)

概述

本 PR 为 librmcs 引入固件端的 USB 传输与二进制协议实现,新增应用主流程、CAN 外设驱动、TinyUSB VENDOR 接口、无锁中断安全缓冲与若干实用并发/断言工具,同时更新构建与开发环境配置。核心目标是通过 USB(Vendor 类)承载二进制协议,桥接主机与四路 CAN 硬件。

主要变更

应用与主循环

  • 新增 App 类(firmware/src/app.hpp/.cpp),在构造时初始化板级、USB、CAN、USB 描述符与供应商接口;提供 [[noreturn]] run() 主循环并在 main() 中启动(调用 tud_task() 和 usb::vendor->try_transmit())。

CAN 总线子系统

  • 新增 Can 类(firmware/src/can/can.hpp):
    • 对 MCAN 外设进行初始化(缓冲区、FIFO、1 Mbps 波特率、IRQ 等)。
    • 提供 handle_downlink()(发送)与 handle_uplink()(接收并序列化到协议)接口。
    • 在 AHB SRAM 中分配 per-channel 消息缓冲并提供四个延迟初始化实例 can0..can3。
  • 新增四个 MCAN 中断服务程序 can0_isr..can3_isr(firmware/src/can/can.cpp),在接收中断时通过 USB 序列化上报。

USB(TinyUSB)与描述符

  • 添加 TinyUSB 配置头(firmware/include/tusb_config.h):MCU/RHPort、内存段/对齐、仅启用 VENDOR 类、端点尺寸(512)等。
  • 新增 UsbDescriptors(firmware/src/usb/usb_descriptors.hpp/.cpp):集中提供设备/配置/字符串描述符,支持高速/全速选择和字符串构建。
  • 新增 TinyUSB 回调封装(firmware/src/usb/usb_descriptors.cpp、firmware/src/usb/vendor.cpp)。

USB 供应商接口与协议处理

  • 新增 Vendor 类(firmware/src/usb/vendor.hpp/.cpp):
    • 作为 VENDOR 接口的反序列化回调接收器,解析二进制协议并路由到 CAN/UART 等处理器(目前 CAN 路由实现);实现 try_transmit() 用于将 InterruptSafeBuffer 中的批量数据通过 tud_vendor_n_write 发送。
    • 使用 InterruptSafeBuffer 作为序列化输出缓冲并提供 Serializer 访问。

中断安全缓冲区

  • 新增 InterruptSafeBuffer(firmware/src/usb/interrupt_safe_buffer.hpp):
    • 实现 ISerializeBuffer,采用无锁多批(8 × 64 字节)设计,原子 in_/out_ 计数,支持 ISR 与主循环安全的批量分配/弹出与清理。

实用并发与断言工具

  • 新增 InterruptLockGuard(firmware/src/utility/interrupt_lock_guard.hpp):RAII 禁用/恢复全局 IRQ(基于 RISC-V mstatus)。
  • 新增 Lazy 模板(firmware/src/utility/lazy.hpp):编译时参数化、线程/中断安全的延迟初始化单例实现。
  • 新增运行时断言设施(firmware/src/utility/assert.cpp):保存断言位置并调用内建陷阱。

构建与开发环境

  • 新增固件级 CMake 构建(firmware/CMakeLists.txt)和预设(firmware/CMakePresets.json),集成 HPM SDK、TinyUSB、RISC-V 工具链与调试器路径映射。
  • 新增 firmware/.clangd、core/setup-clangd 脚本、.gitmodules(添加 firmware/bsp/hpm_sdk 子模块)、.devcontainer 调整与 core/.gitignore 更新。

核心库(host/core)若干断言与缓冲改动

  • 将多处运行时断言从 core::utility::assert(...) 更改为 debug 专用的 core::utility::assert_debug(...)(影响:core/src/coroutine/lifo.hpp、core/src/protocol/deserializer.*、core/src/protocol/serializer.hpp、core/src/utility/assert.inl.hpp、host/src/protocol/handler.cpp、host/src/protocol/stream_buffer.hpp 等)。这些改动将断言行为限制为调试构建,不改变正常控制流(release 下不触发)。
  • host 的 stream_buffer 在 finalize 后显式清除 current_/end_ 指针以避免悬空引用。

小的容器/配置调整

  • .devcontainer/devcontainer.json:添加 HOST_WORKSPACE_FOLDER 环境变量且调整 no_proxy 字段以兼容新变量。
  • .gitmodules:添加 hpm_sdk 子模块引用。
  • core/.gitignore:新增 compile_commands.json 与 .clangd 忽略项。

系统集成要点

  • CAN 中断 → canN_isr → Can::handle_uplink() → usb::vendor.serializer 写入 InterruptSafeBuffer。
  • 主循环:tud_task() 处理 USB 事件;usb::vendor->try_transmit() 弹出已就绪批并调用 tud_vendor_n_write 发送。
  • Vendor 反序列化回调将收到的协议数据路由到相应 CAN 通道(基于 FieldId)。
  • Lazy 与 InterruptLockGuard 确保延迟初始化与临界区在中断上下文中的安全性。

风险与注意事项

  • 新增大量固件/外设相关代码与子模块(HPM SDK),需要在目标硬件或交叉编译环境中验证构建与运行。
  • 断言改为 debug-only 可能在 release 下隐藏某些运行时不变性检查,需在调试阶段充分覆盖序列化/反序列化逻辑。
  • TinyUSB VENDOR 端点宽度与高速/全速行为需在硬件/板级配置下验证(端点 512 字节仅在高速模式)。
  • 中断与无锁缓冲交互高度依赖内存屏障与原子语义,需在目标架构(RISCV32)上验证无竞态。

结论

该 PR 在固件层面实现了 USB Vendor 类承载的二进制协议并将其与四路 CAN 硬件集成,包含从描述符、回调、序列化缓冲到主循环的端到端实现,同时引入了若干并发/断言与构建工具以支持开发与调试。整体设计强调无动态分配、RAII 管理与中断安全,但需在目标平台上进行集成测试以验证时序与原子语义的正确性。

@coderabbitai

coderabbitaiBot commented Dec 26, 2025

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

为固件引入完整的USB Vendor通信路径、CAN外设驱动、无锁中断安全缓冲与懒初始化工具,同时添加构建/clangd配置、CMake 构建预设与 HPM SDK 子模块,并提供应用入口与主循环(tud/USB ↔ 序列化 ↔ CAN 双向链路)。

Changes

Cohort / File(s)变更摘要
开发容器与子模块
\.devcontainer/devcontainer.json`, `.gitmodules`, firmware/bsp/hpm_sdk`
添加 HOST_WORKSPACE_FOLDER 环境变量,调整 no_proxy;新增 HPM SDK 子模块并更新子模块指针。
编辑器/clangd 支持
core/.gitignore, core/setup-clangd, firmware/.clangd
忽略文件模式;新增 setup-clangd 脚本以在 core 下创建 .clangd / compile_commands.json 链接;添加 firmware clangd 配置(riscv 工具链与 include 路径)。
CMake 构建
firmware/CMakeLists.txt, firmware/CMakePresets.json
新增固件 CMakeLists(RISC‑V 工具链、HPM SDK 探测、HOST_DEBUGGER/路径映射、编译选项与系统 include 重路由)与三个 CMake 预设。
TinyUSB 配置
firmware/include/tusb_config.h
新增 TinyUSB 配置头:Board/RHPort、内存段/对齐、只启用 VENDOR 类并设置端点/缓冲尺寸等宏。
USB 描述符与回调
firmware/src/usb/usb_descriptors.hpp, firmware/src/usb/usb_descriptors.cpp
新增 UsbDescriptors 类与 C ABI 回调实现,为 TinyUSB 提供设备/配置/字符串描述符。
USB Vendor 驱动
firmware/src/usb/vendor.hpp, firmware/src/usb/vendor.cpp
新增 Vendor 类(反序列化回调、try_transmit 推送),并实现 TinyUSB vendor 回调(接收/状态钩子)。
中断安全缓冲
firmware/src/usb/interrupt_safe_buffer.hpp
新增无锁批式环形缓冲实现(batch_count=8, batch_size=64),作为 ISerializeBuffer 后端供序列化器使用。
CAN 驱动与 ISR
firmware/src/can/can.hpp, firmware/src/can/can.cpp
新增 Can 类(初始化、1Mbps、TX/RX 处理、downlink/uplink 接口)、四个静态 lazy 实例及四个 MCAN 中断服务例程(can0..can3)。
应用框架
firmware/src/app.hpp, firmware/src/app.cpp
新增 App 类(构造中完成外设初始化、主循环 run),并添加全局 app 实例与 main 入口。
懒初始化与中断守卫
firmware/src/utility/lazy.hpp, firmware/src/utility/interrupt_lock_guard.hpp
新增通用 Lazy 模板(线程/中断安全懒构造)和 InterruptLockGuard RAII(保存/恢复全局 IRQ 状态)。
运行时断言
firmware/src/utility/assert.cpp, core/src/utility/assert.inl.hpp
新增 assert_func(记录位置并触发 trap);将内联断言暴露名从 assert(...) 改为 assert_debug(...)(NDEBUG 下使用 assume / release 行为)。
协议层断言调整
core/src/*, host/src/*
在多处 protocol/serializer/deserializer、coroutine/lifo、host handler、stream_buffer 中将 runtime assert 替换为 debug‑only assert_debug(仅断言语义变为仅调试生效),并在 stream_buffer.finalize_buffer 中清除 current_/end_ 指针。

Sequence Diagram(s)

sequenceDiagram
participant USB as TinyUSB (host ↔ device)
participant Vendor as Firmware::Vendor
participant Deser as Protocol Deserializer
participant CAN as MCAN peripheral
participant ISR as CAN ISR
participant TXBuf as InterruptSafeBuffer
Note over USB,Vendor: 下行数据流 (Host -> Device)
USB->>Vendor: tud_vendor_rx_cb(byte_span, finished)
Vendor->>Deser: feed bytes
Deser->>Vendor: can_deserialized_callback(id, CanDataView)
Vendor->>CAN: handle_downlink(CanDataView)
CAN->>CAN: push TX FIFO (non-blocking)
Note over CAN,ISR: 上行路径 (外设 -> Host)
CAN->>ISR: RXFIFO0_NEW_MSG triggers
ISR->>CAN: read RX FIFO -> build CanDataView
ISR->>Vendor: handle_uplink(field_id, serializer)
Vendor->>TXBuf: serializer writes into InterruptSafeBuffer
Vendor->>USB: try_transmit() -> tud_vendor_n_write(...)
USB-->>Host: USB device transmits data
Loading

Estimated code review effort

🎯 4 (复杂) | ⏱️ ~60 分钟

Possibly related PRs

诗文

🐰 小兔在硅地中探路忙,
USB 振翅,CAN 鼓掌,
缓冲环转免阻挡,懒构造轻轻唱,
中断锁好护安全,主循环迎曙光,
固件新章,跳跃上航 ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 9.38% which is insufficient. The required threshold is 80.00%.You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title clearly and specifically describes the primary addition to the firmware: USB transport and binary protocol implementation.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qzhhhiqzhhhi changed the title Feature/firmwarefeat(firmware): Add USB transport and binary protocol for CANDec 26, 2025
@qzhhhiqzhhhi changed the title feat(firmware): Add USB transport and binary protocol for CANfeat(firmware): Add USB transport and binary protocolDec 26, 2025
@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

@coderabbitai

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (10)
core/setup-clangd (1)

46-50: 建议验证符号链接目标是否存在。

脚本在创建符号链接之前没有验证目标文件是否存在。如果 ../$mode/.clangd../$mode/build/compile_commands.json 尚不存在(例如,尚未运行构建),这可能会导致创建失效的符号链接。

虽然失效的符号链接不会导致错误,但在目标文件创建之前它们不会正常工作。

🔎 可选的改进建议:添加目标存在性检查
 (
cd "$core_dir"
+ if [ ! -e "../$mode/.clangd" ]; then+ echo "Warning: ../$mode/.clangd does not exist yet" >&2+ fi+ if [ ! -e "../$mode/build/compile_commands.json" ]; then+ echo "Warning: ../$mode/build/compile_commands.json does not exist yet" >&2+ fi
ln -s "../$mode/.clangd" .clangd
ln -s "../$mode/build/compile_commands.json" compile_commands.json
)
firmware/src/usb/cdc.cpp (2)

10-17: 接收回调实现正确,可选优化建议

tud_vendor_rx_cb 正确处理了接收数据:

  • 接口检查(第 11 行)确保只处理接口 0
  • 根据 USB 速度计算最大包大小(高速 512 字节,全速 64 字节)符合 USB 规范
  • 使用 size < max_packet_size 判断传输结束是标准做法

可选的小优化:可以将最大包大小计算提取为内联辅助函数,提高代码可读性:

🔎 可选重构建议
+namespace {+inline std::size_t get_max_packet_size() {+ return (tud_speed_get() == TUSB_SPEED_HIGH) ? 512 : 64;+}+} // namespace+
void tud_vendor_rx_cb(uint8_t itf, const uint8_t* buffer, uint16_t size) {
if (itf != 0) [[unlikely]]
return;
- std::size_t max_packet_size = (tud_speed_get() == TUSB_SPEED_HIGH) ? 512 : 64;+ std::size_t max_packet_size = get_max_packet_size();
usb::cdc->handle_downlink(
{reinterpret_cast<const std::byte*>(buffer), size}, size < max_packet_size);
}

19-23: 建议为空回调添加文档说明

三个 TinyUSB 回调函数(tud_resume_cbtud_mount_cbtud_umount_cb)当前为空实现。建议添加注释说明为何这些回调不需要处理逻辑,或者是否计划在未来实现它们。

🔎 建议添加的文档注释
+// Device resume callback - no action required for this device
void tud_resume_cb() {}
+// Device mount callback - initialization handled elsewhere
void tud_mount_cb() {}
+// Device unmount callback - cleanup handled by hardware reset
void tud_umount_cb() {}
firmware/CMakeLists.txt (1)

79-85: 考虑使用显式源文件列表替代 GLOB_RECURSE

GLOB_RECURSE 不会在添加新源文件时自动触发 CMake 重新配置,可能导致构建问题。对于固件项目,显式列出源文件更可靠。

firmware/src/usb/usb_descriptors.hpp (2)

30-58: get_string_descriptor 中存在潜在的边界检查问题。

Line 43 的边界检查在 index >= string_descriptor_array_size 时返回 nullptr,但 index == 0 已在上方处理。建议调整逻辑,将 index == 0 的处理移入统一的分支结构中,或者更清晰地处理 index 范围。

另外,Line 47 的 str_size 计算可以使用 std::min 来提高可读性:

🔎 建议的改进
- str_size = str.size() > 64 - 1 ? 64 - 1 : str.size();+ str_size = std::min<size_t>(str.size(), 63);

117-119: 提醒:地址 TODO 注释。

TODO 注释表明序列号应该使用芯片 ID 动态生成。请确保在后续版本中实现此功能以确保设备唯一性。

需要我帮助生成使用芯片 ID 的序列号实现吗?

firmware/src/usb/interrupt_safe_buffer.hpp (1)

69-70: 批次数据成员的对齐可以优化。

data 成员使用 alignas(size_t) 对齐,但 written_size 在其前面。考虑将 data 放在结构体开头以确保最佳缓存行对齐:

🔎 建议的布局调整
 struct Batch {
+ alignas(size_t) std::byte data[batch_size]{};
std::atomic<size_t> written_size = 0;
- alignas(size_t) std::byte data[batch_size]{};
std::byte* allocate(size_t size) {
// ... + return data + written_size_local;
}
};
firmware/src/can/can.hpp (2)

87-102: FieldHeaderCanStandardIdCanExtendedId 结构体定义但未使用。

这些打包结构体在当前代码中未被引用。如果是为将来实现预留的,请添加注释说明其用途;否则考虑移除以避免死代码。


36-36: CAN 波特率硬编码为 1Mbps。

考虑将波特率作为构造函数参数或编译时配置,以支持不同速率的 CAN 网络。

firmware/src/utility/lazy.hpp (1)

17-17: 析构函数不销毁对象可能导致资源泄漏。

当前析构函数为空,如果 T 持有资源(如文件句柄、内存分配等),这些资源将不会被释放。对于静态生命周期的单例对象这可能是有意为之,但建议添加注释说明设计意图。

🔎 建议添加注释
- constexpr ~Lazy() {}; // No need to deconstruct+ // 析构函数有意为空:Lazy 实例设计为静态生命周期,+ // 程序结束时由操作系统回收资源,避免静态析构顺序问题。+ constexpr ~Lazy() {};
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 34dac24 and 5ec7d56.

📒 Files selected for processing (22)
  • .devcontainer/devcontainer.json
  • .gitmodules
  • core/.gitignore
  • core/setup-clangd
  • firmware/.clangd
  • firmware/CMakeLists.txt
  • firmware/CMakePresets.json
  • firmware/app.yaml
  • firmware/bsp/hpm_sdk
  • firmware/include/tusb_config.h
  • firmware/src/app.cpp
  • firmware/src/app.hpp
  • firmware/src/can/can.cpp
  • firmware/src/can/can.hpp
  • firmware/src/usb/cdc.cpp
  • firmware/src/usb/cdc.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
  • firmware/src/usb/usb_descriptors.cpp
  • firmware/src/usb/usb_descriptors.hpp
  • firmware/src/utility/assert.cpp
  • firmware/src/utility/interrupt_lock_guard.hpp
  • firmware/src/utility/lazy.hpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-20T05:28:56.619Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: core/src/utility/assert.hpp:13-15
Timestamp: 2025-12-20T05:28:56.619Z
Learning: In the librmcs project, include order convention guarantees that `#include "..."` (project files) always come after `#include <...>` (system/library headers), which ensures that modifications to standard macros (like `#undef assert` in core/src/utility/assert.hpp) won't affect other libraries.

Applied to files:

  • firmware/src/utility/assert.cpp
📚 Learning: 2025-12-20T05:31:53.309Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: host/CMakeLists.txt:61-66
Timestamp: 2025-12-20T05:31:53.309Z
Learning: 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. This is a deliberate design choice where catching UB takes priority over interactive debugging convenience.

Applied to files:

  • firmware/src/utility/assert.cpp
📚 Learning: 2025-12-20T05:28:48.614Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: core/src/utility/assert.hpp:13-15
Timestamp: 2025-12-20T05:28:48.614Z
Learning: Require include directives in all header files to place system/library headers (e.g., #include <...>) before project headers (e.g., #include "..."). This ordering reduces cross-library macro interference (such as #undef assert in core/src/utility/assert.hpp) and improves portability. Apply this convention to every .hpp in the repository (e.g., core/src/utility/assert.hpp and other headers).

Applied to files:

  • firmware/src/utility/interrupt_lock_guard.hpp
  • firmware/src/app.hpp
  • firmware/src/usb/usb_descriptors.hpp
  • firmware/src/utility/lazy.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
  • firmware/src/can/can.hpp
  • firmware/src/usb/cdc.hpp
📚 Learning: 2025-12-20T06:17:08.271Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: host/src/transport/usb.cpp:42-62
Timestamp: 2025-12-20T06:17:08.271Z
Learning: 在 librmcs 项目中,下位机使用 USB vendor 类,因此不需要也不应该调用 libusb_attach_kernel_driver 或 libusb_detach_kernel_driver。

Applied to files:

  • firmware/src/usb/cdc.cpp
🧬 Code graph analysis (6)
firmware/src/usb/usb_descriptors.cpp (1)
firmware/src/usb/usb_descriptors.hpp (4)
  • index (20-28)
  • index (20-20)
  • index (30-58)
  • index (30-30)
firmware/src/app.cpp (1)
firmware/src/app.hpp (1)
  • App (12-12)
firmware/src/usb/interrupt_safe_buffer.hpp (1)
host/src/utility/ring_buffer.hpp (3)
  • out (82-89)
  • in_ (61-63)
  • mask (53-53)
firmware/src/can/can.hpp (1)
firmware/src/utility/lazy.hpp (3)
  • assert_always (37-40)
  • assert_always (42-45)
  • assert_always (47-50)
firmware/src/usb/cdc.cpp (1)
firmware/src/usb/cdc.hpp (2)
  • buffer (33-37)
  • buffer (33-33)
firmware/src/usb/cdc.hpp (2)
firmware/src/can/can.hpp (2)
  • data (48-65)
  • data (48-48)
firmware/src/usb/usb_descriptors.hpp (1)
  • reinterpret_cast (16-18)
🪛 Clang (14.0.6)
firmware/include/tusb_config.h

[error] 39-39: CFG_TUSB_MCU must be defined

(clang-diagnostic-error)

🪛 Cppcheck (2.19.0)
firmware/src/can/can.cpp

[error] 7-7: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.

(unknownMacro)

🔇 Additional comments (27)
firmware/src/utility/assert.cpp (2)

5-7: LGTM!

使用 volatile 修饰全局变量是恰当的设计,可确保在 __builtin_trap() 之前编译器不会优化掉这些写入操作,便于在调试器中检查断言位置信息。


9-15: 该审查注释关于 MSVC 兼容性的担忧不适用于此固件模块。

项目的固件部分明确针对 HPM5300EVK RISC-V 微控制器,使用 HPM SDK 提供的 GCC/Clang 工具链编译。MSVC 不支持 RISC-V 架构,因此对该固件模块无关。__builtin_trap() 是 GCC/Clang 在 RISC-V 目标上的正确选择,无需添加条件编译。代码实现正确。

Likely an incorrect or invalid review comment.

core/setup-clangd (2)

1-2: LGTM!错误处理配置良好。

使用 set -eu 是 shell 脚本的良好实践,可以在命令失败或使用未设置的变量时立即退出。


13-25: LGTM!参数验证逻辑清晰且健壮。

输入验证处理得当,对无效参数提供了清晰的使用说明。

core/.gitignore (1)

1-2: LGTM!忽略模式与 setup-clangd 脚本一致。

这些模式正确地忽略了由 setup-clangd 脚本创建的符号链接,防止它们被提交到版本控制中。

firmware/src/utility/interrupt_lock_guard.hpp (2)

1-5: LGTM!头文件包含顺序符合规范。

头文件按照系统/库头文件在前、项目头文件在后的顺序组织,这符合代码库的约定,有助于减少跨库宏干扰并提高可移植性。

基于代码库规范。


11-14: 析构函数中的位掩码操作无法在此仓库内验证。

析构函数调用 restore_global_irq(flags_ & CSR_MSTATUS_MIE_MASK) 时进行了位掩码操作。该操作的正确性取决于 disable_global_irq 的返回值约定:

  • disable_global_irq 返回完整的 MSTATUS 寄存器值,则掩码操作是必需的
  • disable_global_irq 已返回掩码后的值,则掩码操作是冗余的(但无害)

disable_global_irqrestore_global_irq 来自外部的 <board.h> 头文件(HPM 固件),其实现不在此仓库中。建议参考 HPM 板卡文档或固件源码以确认这两个函数的 API 约定。

.devcontainer/devcontainer.json (1)

33-34: LGTM!环境变量配置正确。

添加 HOST_WORKSPACE_FOLDER 环境变量与 PR 中固件构建系统的主机路径解析功能保持一致。JSON 格式正确,变更简洁明了。

.gitmodules (1)

1-3: 子模块配置正确

HPM SDK 子模块的添加符合标准格式,配置正确。

firmware/.clangd (1)

1-11: 验证硬编码的工具链路径是否适用于所有开发环境

配置文件中使用了硬编码的绝对路径 /opt/riscv32-none-elf/,这在不同的开发环境或 CI/CD 系统中可能不可用。

如果项目使用统一的容器化开发环境或所有开发者都遵循相同的工具链安装约定,则此配置是可接受的。否则,建议考虑使用相对路径、环境变量或编译数据库(compile_commands.json)来提高可移植性。

请确认:

  1. 所有开发者是否使用统一的开发环境(如容器)?
  2. 该路径是否在项目文档中有说明?
  3. 是否需要添加设置脚本来处理不同环境的工具链路径?
firmware/CMakePresets.json (1)

1-31: CMake 预设配置合理

三个预设配置(debug、debug-outside、release)正确定义了不同的构建选项。所有预设共享相同的 binaryDir,在切换预设时可能需要清理构建目录以避免配置冲突。

firmware/src/app.cpp (2)

14-28: 考虑添加初始化错误处理

构造函数执行了多个硬件初始化操作(板级、USB、CAN),但没有检查这些初始化调用的返回值或捕获可能的异常。如果任何初始化失败,系统可能处于未定义状态。

建议验证 board_init()board_init_usb()can::canN.init() 等函数的错误处理机制,确保初始化失败时有适当的诊断或故障保护措施。


31-36: 主循环设计符合嵌入式系统典型模式

run() 方法实现了一个紧密的无限轮询循环,持续调用 tud_task()try_transmit()。这是嵌入式固件中常见的超级循环(super loop)模式,适用于不使用 RTOS 的裸机应用。

firmware/src/app.hpp (1)

1-19: 类设计清晰且符合最佳实践

  • 包含顺序正确(系统/库头文件在项目头文件之前)
  • 使用 Immovable 基类防止复制/移动,确保单例语义
  • inline constinitLazy 模板结合使用,提供线程安全的延迟初始化
  • [[noreturn]] 属性准确标注 run() 方法的特性

Based on learnings, 包含指令顺序符合项目约定。

firmware/src/can/can.cpp (4)

21-33: CAN1 ISR 实现与 CAN0 一致

实现模式正确,使用了适当的 [[likely]][[unlikely]] 属性提示编译器优化分支预测。


35-47: CAN2 ISR 实现与 CAN0 一致

实现模式正确。


49-61: CAN3 ISR 实现与 CAN0 一致

实现模式正确。所有四个 ISR 共享相同的处理逻辑,代码结构清晰。


7-19: ISR 实现设计正确,误解了硬件中断屏蔽机制

ISR 仅处理 MCAN_INT_RXFIFO0_NEW_MSG 标志是设计所决定的,而非缺陷。初始化代码在 can.hpp 第 44 行明确仅启用了 MCAN_INT_RXFIFO0_NEW_MSG 中断:

mcan_enable_interrupts(can_base_, MCAN_INT_RXFIFO0_NEW_MSG);

由于只有该一种中断类型被启用,硬件将只针对此中断触发 ISR,其他未启用的中断标志不会产生。ISR 末尾清除所有标志是防御性编程实践,不会对已关闭的中断造成影响。此模式在所有 4 个 MCAN 实例(CAN0-3)中保持一致。

firmware/src/usb/usb_descriptors.cpp (1)

5-29: TinyUSB 描述符回调实现正确

三个回调函数(设备、配置、字符串描述符)正确地使用 extern "C" 链接,并将请求委托给 usb_descriptors 单例。实现简洁明了,符合 TinyUSB 的回调接口要求。

firmware/CMakeLists.txt (1)

91-96: LGTM!

将 SDK 头文件提升为 SYSTEM 包含目录以抑制第三方警告是一个好做法。

firmware/src/usb/cdc.hpp (1)

39-48: LGTM!

CAN 数据回调的 switch 结构清晰,default 分支使用 assert_failed_always() 确保未知字段 ID 不会被静默忽略。

firmware/include/tusb_config.h (2)

38-40: 静态分析误报:CFG_TUSB_MCU 由构建系统定义。

静态分析工具报告的错误是误报。根据 firmware/CMakeLists.txt Line 72,CFG_TUSB_MCU 通过 sdk_compile_definitions(-DCFG_TUSB_MCU=OPT_MCU_HPM) 定义。这个编译时检查是正确的防御性编程。


99-104: 确认直接模式配置符合预期。

CFG_TUD_VENDOR_RX_BUFSIZECFG_TUD_VENDOR_TX_BUFSIZE 设置为 0 启用直接模式。这意味着 TinyUSB 不会进行内部缓冲,应用层需要自行管理数据流。请确认 InterruptSafeBuffer 的实现能够处理这种模式下的数据流控制。

firmware/src/usb/interrupt_safe_buffer.hpp (2)

73-88: pop_batch() 中的内存屏障用法需要确认。

Line 84 使用 std::atomic_signal_fence(std::memory_order_release) 在单线程中断环境下是合适的,但请注意这只提供编译器屏障,不提供硬件内存屏障。如果代码可能在多核环境下运行,应考虑使用 std::atomic_thread_fence

根据 host/src/utility/ring_buffer.hpp 中类似实现使用的 acquire/release 语义,建议保持一致性。


28-50: allocate() 方法的逻辑存在潜在问题。

readable > 0 但当前批次已满(Line 38-39 返回空)时,循环会继续执行到 Line 48 的 compare_exchange_weak,这会尝试分配新批次。但在此之前没有重新检查 writeable,可能导致在缓冲区接近满时分配过多批次。

建议在分配新批次前明确检查 writeable

🔎 建议的逻辑调整
 std::span<std::byte> allocate(size_t size) noexcept override {
core::utility::assert(size <= batch_size);
auto out = out_.load(std::memory_order::relaxed);
while (true) {
auto in = in_.load(std::memory_order::relaxed);
auto readable = in - out;
if (readable) {
if (auto result = batches_[(in - 1) & mask].allocate(size))
return {result, size};
}
auto writeable = batch_count - readable - 1;
if (!writeable) {
// TODO: buffer full indication hook (LED/log); platform pending.
return {};
}
- in_.compare_exchange_weak(in, in + 1, std::memory_order::relaxed);+ if (in_.compare_exchange_weak(in, in + 1, std::memory_order::relaxed)) {+ // 新批次已分配,下次循环会尝试写入+ }+ // CAS 失败时,重新加载 in_ 并重试
}
}

Likely an incorrect or invalid review comment.

firmware/src/utility/lazy.hpp (1)

67-70: LGTM!

使用联合体存储对象或构造参数是一个节省内存的好设计,确保两者不会同时存在。

firmware/src/can/can.hpp (1)

7-8: 头文件顺序应调整:SDK 头文件应放在项目头文件之前。

根据项目约定,应将系统/库头文件(如 board.hhpm_mcan_drv.h)放在项目头文件之前。当前顺序可能导致宏干扰问题。基于 learnings 中的约定。

🔎 建议的头文件顺序
 #include <cstddef>
#include <cstdint>
#include <cstring>
+#include "board.h"+#include "hpm_mcan_drv.h"+-#include "board.h"-#include "hpm_mcan_drv.h"-
#include "core/include/librmcs/data/datas.hpp"

实际上当前顺序是正确的(SDK 头文件在标准库之后、项目头文件之前)。经过重新审视,这符合约定。

Comment threadfirmware/bsp/hpm_sdk
Comment threadfirmware/src/can/can.hpp
Comment threadfirmware/src/usb/vendor.hpp
Comment on lines +71 to +72
.idVendor = 0xa11c,
.idProduct = 0x1235, // TODO: Generate dynamically

@coderabbitaicoderabbitaiBotDec 26, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

USB Vendor ID 0xa11c registered owner

💡 Result:

Do you mean the USB Vendor ID 0xA11C (VID) or a product ID/pair like VID:PID = 0x0EEF:0xA11C? I couldn't find a reliable record for a vendor whose VID is 0xA11C — please confirm which value you have (the full VID:PID string if possible) and I’ll look it up.


🌐 Web query:

USB-IF Vendor ID allocation test experimental VID standards

💡 Result:

Short answer: USB‑IF does not publish an “experimental” or temporary VID pool. Vendor IDs (VIDs) are uniquely assigned by USB‑IF and must be obtained through USB‑IF (membership or paid assignment) for production use; for development people normally either (a) use a silicon/vendor supplier’s VID and request a unique PID from that supplier (allowed for development/prototyping), or (b) obtain their own VID from USB‑IF. Open‑hardware projects sometimes use community workarounds but those are unofficial and discouraged for released products.

References:

  • Silicon Labs: use Silicon Labs VID + request a PID for CP21xx/CP2130 devices (example of vendor PID service). [1]
  • OSHWA guidance: cost/constraints of obtaining a USB VID and recommendation to avoid random/fantasy VIDs. [2]
  • USB guidance / industry tutorials (USB‑101) describing how VIDs are obtained via USB‑IF (membership or paid VID). [3]

🏁 Script executed:

cat -n firmware/src/usb/usb_descriptors.hpp | head -80

Repository: Alliance-Algorithm/librmcs

Length of output: 3184


确保使用合法的 USB Vendor ID。

idVendor = 0xa11c 是一个未注册的 VID,无法在 USB 官方记录中找到。使用未注册的 VID 可能导致与其他设备冲突。根据 USB-IF 政策,USB Vendor ID 必须通过 USB-IF(会员或付费分配)获得。在开发/原型阶段,可以考虑:

  1. 使用硅供应商的 VID 并从该供应商请求唯一的 PID;
  2. 从 USB-IF 获得自己的 VID(用于生产/发布)。

在发布前请替换为合法的 VID。

🤖 Prompt for AI Agents
In firmware/src/usb/usb_descriptors.hpp around lines 71 to 72, the idVendor is
set to 0xa11c which is not a legally assigned USB Vendor ID; replace this
placeholder with a valid VID for your use case: either use your organization's
USB-IF assigned VID, request a vendor-specific PID from your silicon vendor
under their VID for prototyping, or use a properly acquired VID from USB-IF for
production. Update the source to use the chosen legal VID and document the
decision (vendor and PID assignment process) in a comment so the descriptor is
not left using an unregistered placeholder.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们不是一个盈利机构,因此无法付费购买 Vendor ID。
这个 USB 设备仅用于受限的特定场合(比赛),因此无需担心 VID 冲突。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qzhhhi 理解您的情况。对于非盈利项目且仅在受限场合使用的设备,这个方案是可以接受的。

建议在代码中添加注释说明这个 VID 的使用背景和限制,例如:

.idVendor = 0xa11c, // Non-official VID for non-profit competition use only
.idProduct = 0x1235,

这样可以帮助未来的维护者理解这个设计决策。

 /)_/)
(o.o)
/>📝 noted!

✏️ Learnings added
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 6
File: firmware/src/usb/usb_descriptors.hpp:75-76
Timestamp: 2025-12-26T09:46:51.419Z
Learning: 在 librmcs 项目中,USB Vendor ID 0xa11c 是用于非盈利竞赛场合的非官方 VID,项目方无法负担购买官方 VID 的费用,且设备仅在受限特定场合使用。

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: host/src/transport/usb.cpp:42-62
Timestamp: 2025-12-20T06:17:08.271Z
Learning: 在 librmcs 项目中,下位机使用 USB vendor 类,因此不需要也不应该调用 libusb_attach_kernel_driver 或 libusb_detach_kernel_driver。

Comment threadfirmware/src/utility/assert.cpp
Comment threadfirmware/src/utility/lazy.hpp
@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

@coderabbitai

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

@coderabbitai

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (3)
firmware/src/usb/usb_descriptors.hpp (1)

75-77: 确保使用合法的 USB Vendor ID。

此问题在之前的审查中已被标记。idVendor = 0xa11c 是一个未注册的 VID,发布前需要替换为合法的 VID(通过 USB-IF 获得或使用供应商提供的 VID/PID)。

Line 76 的 TODO 注释也表明 idProduct 需要动态生成。

firmware/src/usb/vendor.hpp (1)

66-81: 批次重置逻辑存在问题(已在先前评审中标记)。

try_transmit() 在调用 tud_vendor_n_write() 之前就将 batch->written_size 重置为 0(第 75 行),如果传输失败(sent != written_size),批次数据已丢失且无法重试。建议仅在传输成功后才重置批次状态。

🔎 建议的修复方案
 bool try_transmit() {
if (!device_ready())
return false;
auto batch = transmit_buffer_.pop_batch();
if (!batch)
return false;
auto written_size = batch->written_size.load(std::memory_order::relaxed);
- batch->written_size.store(0, std::memory_order::relaxed);
auto data = reinterpret_cast<uint8_t*>(batch->data);
auto sent = tud_vendor_n_write(0, data, written_size);
+ // 仅在成功发送后重置批次+ if (sent == written_size)+ batch->written_size.store(0, std::memory_order::relaxed);+
return sent == written_size;
}
firmware/src/can/can.hpp (1)

65-65: 未处理传输函数的返回值(已在先前评审中标记)。

mcan_transmit_via_txfifo_nonblocking 的返回值未被检查,可能导致静默丢帧(例如 TX FIFO 已满时)。建议捕获返回值并处理错误情况,至少记录失败信息。

🔎 建议的改进
- mcan_transmit_via_txfifo_nonblocking(can_base_, &frame, nullptr);+ auto status = mcan_transmit_via_txfifo_nonblocking(can_base_, &frame, nullptr);+ if (status != status_success) {+ // TODO: 处理 TX FIFO 满的情况(记录错误或更新失败计数器)+ }
🧹 Nitpick comments (1)
firmware/src/utility/assert.cpp (1)

1-3: 建议调整头文件包含顺序。

根据项目约定,系统/库头文件(#include <...>)应放在项目头文件(#include "...")之前,以减少宏干扰并提高可移植性。

🔎 建议的调整
+#include <source_location>+
#include "core/src/utility/assert.hpp"
-#include <source_location>-
namespace librmcs::core::utility {

基于项目的头文件包含顺序约定。

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ec7d56 and 76442e1.

📒 Files selected for processing (12)
  • firmware/.clangd
  • firmware/src/app.cpp
  • firmware/src/can/can.cpp
  • firmware/src/can/can.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
  • firmware/src/usb/usb_descriptors.cpp
  • firmware/src/usb/usb_descriptors.hpp
  • firmware/src/usb/vendor.cpp
  • firmware/src/usb/vendor.hpp
  • firmware/src/utility/assert.cpp
  • firmware/src/utility/interrupt_lock_guard.hpp
  • firmware/src/utility/lazy.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • firmware/src/utility/lazy.hpp
  • firmware/.clangd
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-20T06:17:08.271Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: host/src/transport/usb.cpp:42-62
Timestamp: 2025-12-20T06:17:08.271Z
Learning: 在 librmcs 项目中,下位机使用 USB vendor 类,因此不需要也不应该调用 libusb_attach_kernel_driver 或 libusb_detach_kernel_driver。

Applied to files:

  • firmware/src/usb/vendor.hpp
  • firmware/src/usb/vendor.cpp
📚 Learning: 2025-12-20T05:28:48.614Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: core/src/utility/assert.hpp:13-15
Timestamp: 2025-12-20T05:28:48.614Z
Learning: Require include directives in all header files to place system/library headers (e.g., #include <...>) before project headers (e.g., #include "..."). This ordering reduces cross-library macro interference (such as #undef assert in core/src/utility/assert.hpp) and improves portability. Apply this convention to every .hpp in the repository (e.g., core/src/utility/assert.hpp and other headers).

Applied to files:

  • firmware/src/usb/vendor.hpp
  • firmware/src/can/can.hpp
  • firmware/src/usb/usb_descriptors.hpp
  • firmware/src/utility/interrupt_lock_guard.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
📚 Learning: 2025-12-20T05:28:56.619Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: core/src/utility/assert.hpp:13-15
Timestamp: 2025-12-20T05:28:56.619Z
Learning: In the librmcs project, include order convention guarantees that `#include "..."` (project files) always come after `#include <...>` (system/library headers), which ensures that modifications to standard macros (like `#undef assert` in core/src/utility/assert.hpp) won't affect other libraries.

Applied to files:

  • firmware/src/utility/assert.cpp
📚 Learning: 2025-12-20T05:31:53.309Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: host/CMakeLists.txt:61-66
Timestamp: 2025-12-20T05:31:53.309Z
Learning: 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. This is a deliberate design choice where catching UB takes priority over interactive debugging convenience.

Applied to files:

  • firmware/src/utility/assert.cpp
🧬 Code graph analysis (6)
firmware/src/usb/vendor.hpp (2)
firmware/src/can/can.hpp (2)
  • data (49-66)
  • data (49-49)
firmware/src/usb/usb_descriptors.hpp (1)
  • reinterpret_cast (20-22)
firmware/src/usb/vendor.cpp (3)
firmware/src/usb/vendor.hpp (2)
  • buffer (33-37)
  • buffer (33-33)
firmware/src/usb/interrupt_safe_buffer.hpp (4)
  • size (25-47)
  • size (25-25)
  • size (52-63)
  • size (52-52)
firmware/src/usb/usb_descriptors.hpp (1)
  • reinterpret_cast (20-22)
firmware/src/can/can.hpp (1)
firmware/src/utility/lazy.hpp (3)
  • assert (44-47)
  • assert (49-52)
  • assert (54-57)
firmware/src/app.cpp (1)
firmware/src/app.hpp (1)
  • App (12-12)
firmware/src/usb/interrupt_safe_buffer.hpp (1)
host/src/utility/ring_buffer.hpp (3)
  • out (82-89)
  • in_ (61-63)
  • mask (53-53)
firmware/src/usb/usb_descriptors.cpp (1)
firmware/src/usb/usb_descriptors.hpp (4)
  • index (24-32)
  • index (24-24)
  • index (34-62)
  • index (34-34)
🪛 Cppcheck (2.19.0)
firmware/src/can/can.cpp

[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.

(unknownMacro)

🔇 Additional comments (23)
firmware/src/utility/assert.cpp (1)

7-17: 实现逻辑正确。

使用 volatile 全局变量存储断言位置信息便于调试器检查,__builtin_trap() 的使用也是嵌入式环境中停止执行的标准做法。

firmware/src/usb/usb_descriptors.hpp (1)

24-32: 描述符获取逻辑实现合理。

配置描述符根据 USB 速度(HS/FS)正确选择,字符串描述符的 UTF-16 转换逻辑也符合 USB 规范。边界检查(Line 47-48)和大小限制(Line 51)都已妥善处理。

Also applies to: 34-62

firmware/src/usb/vendor.cpp (2)

14-21: TinyUSB vendor 回调实现正确。

接口验证、速度相关的包大小确定(HS: 512 字节,FS: 64 字节)以及短包检测逻辑都符合 USB 规范。数据正确转发给 vendor handler。


23-27: 空回调实现合理。

这些空的 TinyUSB 回调作为桩函数是可接受的,可以在需要时添加功能。

firmware/src/app.cpp (2)

15-29: 硬件初始化逻辑正确。

InterruptLockGuard 保护下进行硬件初始化(USB、CAN、描述符)是正确的做法,可防止初始化期间的中断干扰。初始化顺序合理。


32-37: 主循环实现符合嵌入式 USB 设备模式。

无限循环中调用 tud_task() 处理 TinyUSB 事件和 try_transmit() 处理数据传输是标准的嵌入式 USB 设备实现模式。NOLINTNEXTLINE 注释恰当地标记了预期的非静态成员函数。

firmware/src/usb/usb_descriptors.cpp (1)

10-29: TinyUSB 描述符回调实现正确。

回调函数正确委托给 usb_descriptors 单例,使用 extern "C" 确保 C ABI 兼容性。这是 TinyUSB 描述符提供的标准实现模式。

firmware/src/can/can.cpp (1)

13-67: CAN 中断服务程序实现正确。

四个 ISR 实现遵循正确的模式:读取中断标志、提前返回(无标志时)、处理 RXFIFO0 新消息、清除标志。使用 [[likely]][[unlikely]] 属性优化常见路径。代码重复在硬件 ISR 中是可接受的。

静态分析提示关于 SDK_DECLARE_EXT_ISR_M 的警告是误报,这是 HPM SDK 的宏。

firmware/src/utility/interrupt_lock_guard.hpp (1)

11-20: 中断锁保护实现正确。

RAII 模式正确实现了全局中断的禁用和恢复。构造函数禁用中断并保存状态,析构函数恢复中断状态。继承自 Immovable 防止意外的复制/移动是正确的设计。

firmware/src/usb/interrupt_safe_buffer.hpp (2)

69-84: pop_batch 的内存屏障使用恰当。

使用 std::atomic_signal_fence(std::memory_order_release)(Line 80)对于中断上下文是正确的选择。在单核嵌入式系统中,ISR 作为信号处理器运行,signal fence 足以确保内存可见性,相比 thread fence 开销更小。


15-23: 无锁环形缓冲区设计合理。

整体的无锁环形缓冲区设计适合中断安全的 USB 传输场景。批次管理、原子操作使用和边界检查都实现得当。静态断言确保批次数量是 2 的幂次方和原子操作无锁特性是良好的编译时检查。

Also applies to: 51-67, 86-104

firmware/src/usb/vendor.hpp (6)

1-20: 头文件引入顺序正确。

系统/库头文件(<...>)在项目头文件("...")之前,符合项目规范。


23-37: 类结构和下行链路处理逻辑正确。

私有继承 IDeserializeCallbackImmovable 的设计合理,handle_downlink 方法正确地将数据馈送给反序列化器并在完成时调用 finish_transfer()


39-48: CAN 数据回调实现正确。

正确地根据 DataId 将 CAN 数据路由到 can0-can3,并对未知 ID 进行断言处理。


50-62: 确认 UART/IMU 回调的空实现是否符合预期。

uart_deserialized_callbackaccelerometer_deserialized_callbackgyroscope_deserialized_callback 当前是空操作。请确认这是有意为之(例如这些功能暂未实现或在 USB vendor 类中不需要),还是后续需要添加实现。


83-90: 私有成员和辅助方法实现正确。

device_ready() 方法进行了适当的防御性检查,成员变量的初始化顺序与声明顺序一致,逻辑正确。


92-92: 全局实例声明正确。

使用 constinit 确保编译期初始化,配合 Lazy 实现延迟构造,适合固件环境的初始化需求。

firmware/src/can/can.hpp (6)

1-17: 头文件引入顺序正确。

系统/SDK 头文件在项目头文件之前,符合项目规范。


24-47: 构造函数的 MCAN 初始化流程正确。

硬件初始化序列合理,包括消息缓冲区配置、时钟初始化、波特率设置(1 Mbps)、FIFO 配置和中断使能。所有状态检查均使用 assert_always 进行验证。


49-63: CAN 帧转换逻辑正确。

正确地将 CanDataView 转换为 mcan_tx_frame_t,处理了扩展/标准 ID、RTR 标志和数据复制。数据长度断言确保不超过 8 字节。


68-83: 上行链路处理逻辑正确。

正确地从 RX FIFO 读取消息,转换为 CanDataView,并进行序列化。所有字段映射正确,错误检查使用断言进行验证。


88-103: 确认私有结构体的用途。

FieldHeaderCanStandardIdCanExtendedId 这三个打包结构体在本文件中未被使用。请确认它们是否为未来的序列化/帧封装功能预留,或者是否应该移除以避免死代码。


106-122: 静态缓冲区分配和实例初始化正确。

四个 CAN 实例的消息缓冲区正确地放置在 AHB SRAM 中,Lazy 实例使用 constinit 初始化并传入正确的参数。static_assert 确保硬件要求得到满足。

Comment threadfirmware/src/usb/interrupt_safe_buffer.hpp
Comment threadfirmware/src/usb/usb_descriptors.hpp
Comment threadfirmware/src/usb/interrupt_safe_buffer.hpp
@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

@coderabbitai

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
firmware/src/can/can.hpp (2)

80-82: 考虑处理其他序列化错误。

当前仅检查 kInvalidArgument,但 write_can 还可能返回 kInsufficientSpacekSerialPortOccupied。虽然断言失败能暴露编程错误,但更全面的错误处理(例如丢帧计数或重试)可以提高系统鲁棒性。

🔎 可选的改进
- core::utility::assert_always(- serializer.write_can(field_id, data)- != core::protocol::Serializer::SerializeResult::kInvalidArgument);+ auto result = serializer.write_can(field_id, data);+ core::utility::assert_always(result != core::protocol::Serializer::SerializeResult::kInvalidArgument);+ if (result != core::protocol::Serializer::SerializeResult::kSuccess) {+ // TODO: 处理 kInsufficientSpace 或 kSerialPortOccupied(计数器/日志)+ }

88-103: 移除未使用的帧结构体。

FieldHeaderCanStandardIdCanExtendedId 这些私有结构体在类中未被使用。handle_uplink 使用 Serializer::write_can 处理协议帧,handle_downlink 使用 mcan_tx_frame_t。这些结构体可能是早期设计遗留,建议删除以减少代码冗余。

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76442e1 and 870f8c8.

📒 Files selected for processing (11)
  • core/src/coroutine/lifo.hpp
  • core/src/protocol/deserializer.cpp
  • core/src/protocol/deserializer.hpp
  • core/src/protocol/serializer.hpp
  • core/src/utility/assert.inl.hpp
  • firmware/src/can/can.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
  • firmware/src/usb/usb_descriptors.hpp
  • firmware/src/utility/lazy.hpp
  • host/src/protocol/handler.cpp
  • host/src/protocol/stream_buffer.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • firmware/src/usb/usb_descriptors.hpp
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 6
File: firmware/src/usb/interrupt_safe_buffer.hpp:25-47
Timestamp: 2025-12-26T09:45:46.098Z
Learning: 在 librmcs 固件的 `firmware/src/usb/interrupt_safe_buffer.hpp` 中,`InterruptSafeBuffer::allocate()` 在循环外加载 `out_` 值是有意为之的设计。该缓冲区采用裸机中断安全模型:`allocate()` 仅在 ISR 中调用(通过 CAN ISR 中的 serializer),`pop_batch()` 仅在主线程中调用(通过 `App::run()` 中的 `try_transmit()`)。由于项目不使用 RTOS,ISR 执行时主线程被暂停,因此 `out_` 在 `allocate()` 执行期间保持稳定。
📚 Learning: 2025-12-20T06:17:50.471Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: core/src/coroutine/lifo.hpp:0-0
Timestamp: 2025-12-20T06:17:50.471Z
Learning: In coroutine result accessors (e.g., LifoTask::result()), prefer returning T& instead of T to express ownership transfer. Update the API so callers use std::move(task.result()) and adjust callers accordingly. Apply this pattern to similar accessors in core/src/coroutine; ensure const-correctness, avoid dangling references, and update any internal copies or move semantics to reflect the new ownership model.

Applied to files:

  • core/src/coroutine/lifo.hpp
📚 Learning: 2025-12-20T05:28:48.614Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: core/src/utility/assert.hpp:13-15
Timestamp: 2025-12-20T05:28:48.614Z
Learning: Require include directives in all header files to place system/library headers (e.g., #include <...>) before project headers (e.g., #include "..."). This ordering reduces cross-library macro interference (such as #undef assert in core/src/utility/assert.hpp) and improves portability. Apply this convention to every .hpp in the repository (e.g., core/src/utility/assert.hpp and other headers).

Applied to files:

  • core/src/coroutine/lifo.hpp
  • core/src/protocol/deserializer.hpp
  • core/src/protocol/serializer.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
  • host/src/protocol/stream_buffer.hpp
  • firmware/src/utility/lazy.hpp
  • firmware/src/can/can.hpp
  • core/src/utility/assert.inl.hpp
📚 Learning: 2025-12-20T04:28:53.374Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 4
File: core/src/protocol/deserializer.cpp:38-58
Timestamp: 2025-12-20T04:28:53.374Z
Learning: In core/src/protocol/deserializer.cpp, for the default switch case handling an unknown FieldId, keep the current behavior: do not eagerly consume any peeked bytes before invoking deserializing_error(). This path intentionally relies on deserializing_error() resetting input_cursor_ to input_end_ and pending_bytes_ to 0, so that the next peek_bytes() suspends the coroutine until finish_transfer() clears the error state. Ensure this rationale is documented with a comment near the default branch and add a targeted test or regression note to prevent future reintroduction of a byte-consumption in this path. If future changes modify this behavior, verify that the coroutine suspension and error-reset semantics remain consistent and that the existing assertion at the relevant line remains valid.

Applied to files:

  • core/src/protocol/deserializer.cpp
📚 Learning: 2025-12-20T04:28:56.876Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 4
File: core/src/protocol/deserializer.cpp:38-58
Timestamp: 2025-12-20T04:28:56.876Z
Learning: In core/src/protocol/deserializer.cpp, when an unknown FieldId is encountered in the default switch branch, the code does not explicitly consume peeked bytes before calling deserializing_error(). This is intentional: deserializing_error() resets input_cursor_ to input_end_ and pending_bytes_ to 0, causing the next peek_bytes() to suspend the coroutine until finish_transfer() resets the error state. The assertion at line 22 will not fail because the coroutine suspends before re-entering that path.

Applied to files:

  • core/src/protocol/deserializer.hpp
  • core/src/protocol/serializer.hpp
📚 Learning: 2025-12-26T09:45:46.098Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 6
File: firmware/src/usb/interrupt_safe_buffer.hpp:25-47
Timestamp: 2025-12-26T09:45:46.098Z
Learning: 在 librmcs 固件的 `firmware/src/usb/interrupt_safe_buffer.hpp` 中,`InterruptSafeBuffer::allocate()` 在循环外加载 `out_` 值是有意为之的设计。该缓冲区采用裸机中断安全模型:`allocate()` 仅在 ISR 中调用(通过 CAN ISR 中的 serializer),`pop_batch()` 仅在主线程中调用(通过 `App::run()` 中的 `try_transmit()`)。由于项目不使用 RTOS,ISR 执行时主线程被暂停,因此 `out_` 在 `allocate()` 执行期间保持稳定。

Applied to files:

  • firmware/src/usb/interrupt_safe_buffer.hpp
  • firmware/src/utility/lazy.hpp
  • firmware/src/can/can.hpp
📚 Learning: 2025-12-20T05:31:53.309Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 5
File: host/CMakeLists.txt:61-66
Timestamp: 2025-12-20T05:31:53.309Z
Learning: 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. This is a deliberate design choice where catching UB takes priority over interactive debugging convenience.

Applied to files:

  • core/src/utility/assert.inl.hpp
🧬 Code graph analysis (8)
core/src/coroutine/lifo.hpp (1)
core/src/utility/assert.inl.hpp (2)
  • assert_debug (36-44)
  • assert_debug (36-37)
core/src/protocol/deserializer.cpp (2)
core/src/protocol/deserializer.hpp (5)
  • assert_debug (110-150)
  • assert_debug (156-173)
  • assert_debug (191-199)
  • id (20-20)
  • id (22-22)
core/src/utility/assert.inl.hpp (2)
  • assert_debug (36-44)
  • assert_debug (36-37)
core/src/protocol/deserializer.hpp (1)
core/src/utility/assert.inl.hpp (2)
  • assert_debug (36-44)
  • assert_debug (36-37)
core/src/protocol/serializer.hpp (1)
core/src/utility/assert.inl.hpp (2)
  • assert_debug (36-44)
  • assert_debug (36-37)
firmware/src/usb/interrupt_safe_buffer.hpp (2)
core/src/protocol/serializer.hpp (1)
  • size (21-21)
core/src/utility/assert.inl.hpp (2)
  • assert_debug (36-44)
  • assert_debug (36-37)
host/src/protocol/stream_buffer.hpp (1)
core/src/utility/assert.inl.hpp (2)
  • assert_debug (36-44)
  • assert_debug (36-37)
firmware/src/utility/lazy.hpp (2)
core/src/utility/assert.inl.hpp (4)
  • assert_always (30-34)
  • assert_always (30-31)
  • assert_debug (36-44)
  • assert_debug (36-37)
core/src/coroutine/lifo.hpp (5)
  • assert_debug (184-187)
  • assert_debug (189-192)
  • assert_debug (258-258)
  • assert_debug (264-264)
  • static_cast (66-66)
firmware/src/can/can.hpp (1)
core/src/protocol/serializer.hpp (13)
  • field_id (31-74)
  • field_id (31-31)
  • field_id (76-109)
  • field_id (76-76)
  • field_id (162-165)
  • field_id (162-162)
  • field_id (167-170)
  • field_id (167-167)
  • field_id (184-201)
  • field_id (184-184)
  • field_id (203-220)
  • field_id (204-204)
  • field_id (224-235)
🔇 Additional comments (10)
core/src/utility/assert.inl.hpp (1)

36-37: 函数重命名合理,与调试断言语义一致。

将公开函数从 assert 重命名为 assert_debug 清晰地表达了其仅在调试构建中生效的语义。实现逻辑保持不变(NDEBUG 时使用 [[assume]],否则委托给 assert_always),命名更新提升了 API 的可读性。

core/src/coroutine/lifo.hpp (1)

174-174: 协程断言迁移至调试变体,符合项目方向。

将协程完成状态检查(handle_.done())从运行时断言切换为 assert_debug,与整个代码库的调试断言迁移策略一致。这些断言验证内部不变量(协程必须完成才能访问结果),在发布构建中通过 [[assume]] 向编译器提供优化提示。

Also applies to: 185-185, 190-190, 258-258, 264-264

core/src/protocol/deserializer.cpp (1)

17-17: 反序列化器内部不变量检查适合调试断言。

pending_bytes_header_bytes 的运行时检查迁移到 assert_debug。第 23 行的注释明确说明了这些条件在逻辑上不可能失败("stack unwinding is invalid here"),因此适合作为调试时验证的内部不变量,在发布构建中可安全移除。

Also applies to: 23-24

core/src/protocol/serializer.hpp (1)

37-37: 序列化器断言迁移保持了调试时验证。

将分配后的大小检查、游标边界验证和字段头验证从运行时断言切换到 assert_debug。这些检查验证内部状态一致性(如 dst.size() == requiredcursor == dst.data() + dst.size()),在调试构建中捕获逻辑错误,在发布构建中不影响性能。

Also applies to: 72-72, 82-82, 107-107, 117-117, 132-132, 142-142, 157-157, 163-163, 198-198, 232-232

core/src/protocol/deserializer.hpp (1)

44-45: 反序列化器全面的调试断言覆盖。

将指针和缓冲区边界验证(input_cursor_input_end_pending_bytes_requested_bytes_)从运行时断言迁移到 assert_debug。这些检查覆盖了 feed()peek_bytes()consume_peeked() 和相关路径的内部不变量,在调试构建中确保状态一致性,在发布构建中保留现有行为。

Also applies to: 84-84, 103-103, 105-106, 111-112, 119-119, 140-140, 143-143, 157-157, 162-162, 165-168, 192-192, 194-194, 202-203, 206-206

firmware/src/usb/interrupt_safe_buffer.hpp (2)

69-84: pop_batch() 的信号栅栏使用适合裸机中断模型。

第 80 行使用 atomic_signal_fence(memory_order_release) 确保批次数据的写入在更新 out_ 之前对主线程可见。这在裸机环境中是合适的,因为:

  • 信号栅栏防止编译器重排序
  • ISR 与主线程之间通过中断机制同步,无需完整的线程间内存屏障
  • memory_order_release 语义确保之前的写入在释放 out_ 前完成

类似的模式在 clear() 中的第 102 行也得到正确应用。

基于学习记录的裸机中断安全模型,这种同步策略是正确的。


52-66: Batch::allocate() 的无锁分配逻辑正确。

批次内分配使用 CAS 循环(第 59-60 行)确保多个 ISR 可以安全地并发分配到同一批次:

  • 第 56 行:加载当前 written_size
  • 第 57-58 行:检查剩余空间
  • 第 59-60 行:原子地将 written_size 从当前值更新为 current + size
  • 第 62 行:返回 data + written_size_local 指向分配的区域

CAS 失败时循环重试,处理了并发竞争。此方法本身实现正确,但依赖于批次复用前 written_size 被重置为 0(见 Line 25-47 的关键缺陷)。

host/src/protocol/handler.cpp (1)

169-169: Handler 的前置条件检查迁移至调试断言。

start_transmit() 中的 impl_ 非空检查从运行时断言切换到 assert_debug。这是对 Handler 已正确初始化的前置条件验证,适合作为调试时检查,与整个代码库的断言迁移策略一致。

host/src/protocol/stream_buffer.hpp (2)

122-123: StreamBuffer 的前置条件和不变量检查迁移至调试断言。

将缓冲区管理路径中的验证从运行时断言切换到 assert_debug

  • 第 122-123 行:allocate() 的大小和指针前置条件
  • 第 174-176 行:allocate_up_to() 的大小范围验证
  • 第 195 行:init_buffer() 的缓冲区状态不变量
  • 第 210、214 行:finalize_buffer() 的状态和有效载荷检查

这些检查验证内部状态一致性,适合在调试构建中捕获逻辑错误,在发布构建中通过 [[assume]] 提供优化提示。

Also applies to: 174-176, 195-195, 204-204, 210-210, 214-214


217-218: 改进:传输后重置指针提升状态卫生。

finalize_buffer() 中传输缓冲区后将 current_end_ 设置为 nullptr,确保已释放的缓冲区指针不会被意外解引用。这是良好的防御性编程实践,配合调试断言提供了更强的状态验证。

Comment threadfirmware/src/usb/interrupt_safe_buffer.hpp
Comment threadfirmware/src/utility/lazy.hpp
- Updated devcontainer configuration to include HOST_WORKSPACE_FOLDER.
- Added .gitmodules to include the HPM SDK submodule.
- Created .clangd for compiler flags specific to the RISC-V toolchain.
- Introduced CMakeLists.txt for building the firmware with HPM SDK dependencies.
- Added CMakePresets.json for build configurations (debug, release).
- Created app.yaml for application configuration.
- Initialized firmware/bsp/hpm_sdk as a submodule.
- Implemented USB configuration in include/tusb_config.h.
- Developed main application logic in src/app.cpp and src/app.hpp.
- Implemented CAN communication in src/can/can.cpp and src/can/can.hpp.
- Added interrupt-safe buffer management in src/usb/interrupt_safe_buffer.hpp.
- Defined USB descriptors in src/usb/usb_descriptors.cpp and src/usb/usb_descriptors.hpp.
- Created vendor communication handling in src/usb/vendor.cpp and src/usb/vendor.hpp.
- Implemented assertion handling in src/utility/assert.cpp.
- Added interrupt lock guard utility in src/utility/interrupt_lock_guard.hpp.
- Introduced lazy initialization utility in src/utility/lazy.hpp.
@qzhhhi
qzhhhi merged commit c57622f into dev/v3Dec 29, 2025
1 check passed
@github-project-automationgithub-project-automationBot moved this from Todo to Done in RMCS Slave SDKDec 29, 2025
@qzhhhi
qzhhhi deleted the feature/firmware branch December 29, 2025 10:29
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant

@qzhhhi