Skip to content

feat(firmware/uart): Add DMA-backed UART bridge - #10

Merged
qzhhhi merged 4 commits into
dev/v3from
feature/firmware-uart
Jan 11, 2026
Merged

feat(firmware/uart): Add DMA-backed UART bridge#10
qzhhhi merged 4 commits into
dev/v3from
feature/firmware-uart

Conversation

@qzhhhi

Copy link
Copy Markdown
Member

No description provided.

…bytes
- Encode UART length with byte count
- Rename UART header length fields to reflect semantics
- Serialize/deserialize UART payload length directly (no +1 / -1 mapping)
BREAKING CHANGE: UART length fields are no longer "length - 1" (4-byte UART payloads now use the extended header), and Serializer::write_uart() adds a optional suffix_data parameter.
- Use core::protocol::kProtocolBufferSize for InterruptSafeBuffer storage/limits
- Keep an in-flight batch and transmit it across multiple calls with an offset
- Respect FS/HS max packet size and emit ZLP when needed before releasing the batch
@coderabbitai

coderabbitaiBot commented Jan 11, 2026

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.

高层概览

本次变更添加了完整的UART驱动程序基础设施,包括收发缓冲区、DMA集成、以及协议层的UART处理优化。涉及核心协议层类型重命名、序列化API扩展、和固件层的UART、USB、缓冲区管理模块。

变更内容

内容分组 / 文件变更摘要
协议层UART处理
core/src/protocol/deserializer.cppcore/src/protocol/protocol.hppcore/src/protocol/serializer.hpp
将DataLengthCode重命名为DataLength,DataLengthCodeExtended重命名为DataLengthExtended;重构deserializer中UART长度处理逻辑,支持扩展长度路径和零字节载荷;扩展write_uart API以接受可选的suffix_data参数,更新size计算逻辑。
固件应用层集成
firmware/CMakeLists.txtfirmware/src/app.cpp
添加CONFIG_DMA_MGR配置标志;初始化DMA管理器和UART3,在主循环中集成uart3->try_transmit()调用。
UART驱动程序
firmware/src/uart/uart.hppfirmware/src/uart/uart.cppfirmware/src/uart/rx_buffer.hppfirmware/src/uart/tx_buffer.hpp
新增Uart驱动类集成TxBuffer和RxBuffer;实现RxBuffer用于DMA驱动接收和循环缓冲,RxBuffer处理缓存一致性和分段交付;实现TxBuffer用于DMA驱动发送,支持空闲分隔符和链接描述符;ISR集成处理UART空闲检测。
USB集成与序列化
firmware/src/usb/helper.hppfirmware/src/usb/vendor.hppfirmware/src/usb/vendor.cpp
添加get_serializer()全局访问器;修改uart_deserialized_callback路由UART3数据至uart::uart3->handle_downlink;实现分块USB传输(try_transmit)支持max_packet_size限制和零长度包;添加tud_suspend_cb暂停回调。
缓冲区和工具库
firmware/src/usb/interrupt_safe_buffer.hppfirmware/src/utility/ring_buffer.hpphost/src/utility/ring_buffer.hpp
将batch_count重命名为kBatchCount并集成kProtocolBufferSize常量;新增lock-free SPSC环形缓冲区RingBuffer<T, max_size>支持原地构造、批量操作和原子同步;优化host端ring_buffer中的原子加载操作。

序列图

sequenceDiagram
participant HW as UART硬件
participant DMA as DMA管理器
participant RxBuf as RxBuffer<br/>(接收)
participant Proto as 协议层<br/>反序列化
participant USB as USB端点
Note over HW,USB: UART接收数据路径
HW->>DMA: 数据通过DMA<br/>到循环缓冲
DMA->>RxBuf: 触发idle/TC中断<br/>回调
RxBuf->>RxBuf: 更新read指针<br/>缓存一致性无效化
RxBuf->>Proto: handle_uplink()<br/>分段交付载荷
Proto->>USB: 序列化后写入<br/>USB序列化器
USB->>HW: 通过USB向主机<br/>发送数据
participant App as 应用层
participant TxBuf as TxBuffer<br/>(发送)
participant USB2 as USB端点
participant DMA2 as DMA管理器
participant HW2 as UART硬件
Note over App,HW2: UART发送数据路径
USB2->>Proto: 反序列化<br/>获取数据视图
Proto->>App: handle_downlink()<br/>UartDataView
App->>TxBuf: try_enqueue()<br/>写入环形缓冲
App->>TxBuf: try_dequeue()<br/>触发DMA传输
TxBuf->>DMA2: 配置链接描述符<br/>启动DMA传输
DMA2->>HW2: 数据传输至<br/>UART FIFO
HW2->>HW2: 通过UART<br/>发送数据
Loading

代码审查工作量预估

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

可能相关的PR

诗歌

🐰 协议与缓冲舞蹈,
DMA之流无休,
UART唤醒空闲梦,
USB漂洋过海,
环形之美代代传承。

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ Passed拉取请求标题清晰且专注,准确总结了主要变更:添加DMA支持的UART桥接功能。标题与changeset的核心内容一致,符合规范要求。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

1 similar comment
@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai review

@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai help

@coderabbitai

Copy link
Copy Markdown

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

CodeRabbit commands

These commands are invoked using PR/Issue comments.

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai evaluate custom pre-merge check --instructions <custom-checks-instructions> --name <custom-checks-title> [--mode <error|warning>] to validate the custom pre-merge checks instructions. Defaults to error when --mode is omitted.
    • @coderabbitai ignore pre-merge checks to override pre-merge checks and get an approval on PR.
    • @coderabbitai run pre-merge checks to run pre-merge checks on the pull request.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai generate configuration to create a PR that adds the current resolved configuration as .coderabbit.yaml (or show it if already present).
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit configuration file (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, support, documentation and community

  • Visit our status page to check the current availability of CodeRabbit.
  • Create a ticket on our support page for assistance with any issues or questions.
  • Visit our documentation site for detailed information on how to use CodeRabbit.
  • Join our Discord community to connect with other users and get help from the community.
  • Follow us on X/Twitter for updates and announcements.

@qzhhhi

Copy link
Copy Markdown
MemberAuthor

@coderabbitai full review

@coderabbitai

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

@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: 3

🤖 Fix all issues with AI agents
In @firmware/src/usb/vendor.hpp:
- Around line 87-93: device_ready() currently only checks
tud_vendor_n_write_available(0) existence but not capacity, so
tud_vendor_n_write(0, ...) can perform partial writes and make
core::utility::assert_debug(sent == target_size) fail; fix by either verifying
tud_vendor_n_write_available(0) >= target_size before calling
tud_vendor_n_write(0, data, target_size) or, better, handle partial writes: call
tud_vendor_n_write repeatedly, update transmitted_size_ and advance the data
pointer by the returned sent count until all target_size bytes are written, and
only call tud_vendor_n_write_zlp() when target_size == 0; ensure to use the
existing symbols transmitted_size_, transmitting_batch_, tud_vendor_n_write,
tud_vendor_n_write_available, tud_vendor_n_write_zlp and keep/assert semantics
via core::utility::assert_debug on fatal errors only after retries.
In @firmware/src/utility/ring_buffer.hpp:
- Around line 152-159: The emplace_back lambda uses a conditional noexcept which
fails the strict noexcept requirement on emplace_back_n (and similarly affects
push_back/push_back_n); relax the requires on emplace_back_n (and related
functions) to accept a general invocable (remove the unconditional noexcept
check) or provide an overload that accepts potentially-throwing functors, then
keep emplace_back's lambda without a noexcept specification (or leave it as-is)
so it compiles for types with non-noexcept constructors; update
push_back/push_back_n/emplace_back to use the relaxed trait and document the
exception behavior.
🧹 Nitpick comments (7)
firmware/src/uart/rx_buffer.hpp (1)

110-118: 考虑添加溢出计数器或遥测钩子。

TODO 注释提到需要添加丢弃计数器或遥测钩子。在生产环境中,跟踪缓冲区溢出事件对于诊断系统负载问题很有价值。

需要我帮助实现溢出计数器或创建 issue 来跟踪此任务吗?

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

106-109: 考虑处理空 span 的 memcpy 调用。

view.uart_datasuffix_data 为空 span 时,其 data() 可能返回 nullptr。虽然 size() 为 0 时大多数实现能正确处理,但严格来说 std::memcpy 传入空指针是未定义行为。

🔧 可选的防御性修复
- std::memcpy(cursor, view.uart_data.data(), view.uart_data.size());- cursor += view.uart_data.size();- std::memcpy(cursor, suffix_data.data(), suffix_data.size());- cursor += suffix_data.size();+ if (!view.uart_data.empty()) {+ std::memcpy(cursor, view.uart_data.data(), view.uart_data.size());+ cursor += view.uart_data.size();+ }+ if (!suffix_data.empty()) {+ std::memcpy(cursor, suffix_data.data(), suffix_data.size());+ cursor += suffix_data.size();+ }
firmware/src/uart/uart.hpp (3)

53-53: 参数名 uart_base_ 与成员变量同名,存在遮蔽问题。

静态函数 init_uart 的参数名 uart_base_ 与类成员变量同名(第91行),虽然在静态函数中不会访问成员变量,但这可能导致阅读和维护时的混淆。

建议重命名参数
- static void init_uart(UART_Type* uart_base_, uint32_t irq_num) {- board_init_uart(uart_base_);- uint32_t uart_clock = board_init_uart_clock(uart_base_);+ static void init_uart(UART_Type* uart_base, uint32_t irq_num) {+ board_init_uart(uart_base);+ uint32_t uart_clock = board_init_uart_clock(uart_base);
uart_config_t config{};
- uart_default_config(uart_base_, &config);+ uart_default_config(uart_base, &config);
// ... rest uses uart_base instead of uart_base_

90-91: uart_base_ 成员可声明为 const

uart_base_ 在构造后不再修改,声明为 UART_Type* const uart_base_ 可以更好地表达设计意图。

建议添加 const
 const data::DataId data_id_;
- UART_Type* uart_base_;+ UART_Type* const uart_base_;

81-88: handle_uplink 仅在 debug 模式下验证参数。

assert_debug 在 release 构建中不会执行检查。如果 write_uart 返回 kInvalidArgument 是严重错误,应考虑使用 assert_always 或添加错误处理/日志记录,以便在生产环境中也能检测到问题。

firmware/src/uart/tx_buffer.hpp (1)

161-165: 多个条件合并在单个 assert_always 中,失败时难以定位问题。

当断言失败时,无法直接判断是 dma_mgr_request_resourcedma_mgr_setup_channel 还是 dma_mgr_config_linked_descriptor 失败。

建议拆分断言
- core::utility::assert_always(- dma_mgr_request_resource(&dma_) == status_success- && dma_mgr_setup_channel(&dma_, &config) == status_success- && dma_mgr_config_linked_descriptor(&dma_, &config, &dma_linked_descriptor_mgr_)- == status_success);+ core::utility::assert_always(dma_mgr_request_resource(&dma_) == status_success);+ core::utility::assert_always(dma_mgr_setup_channel(&dma_, &config) == status_success);+ core::utility::assert_always(+ dma_mgr_config_linked_descriptor(&dma_, &config, &dma_linked_descriptor_mgr_)+ == status_success);
firmware/src/utility/ring_buffer.hpp (1)

170-181: push_back_n 的 requires 约束格式有问题。

requires 子句与返回类型 size_t 放在同一行(第174行),虽然语法有效,但可读性较差。建议保持与 emplace_back_n(第115-121行)一致的格式。

建议调整格式
 template <typename F>
requires requires(F& f) {
{ f() } noexcept;
{ T{f()} } noexcept;
- } size_t push_back_n(+ }+ size_t push_back_n(
F generator, size_t count = std::numeric_limits<size_t>::max(), bool fail_fast = false) {
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec3ac54 and fe90c62.

📒 Files selected for processing (15)
  • core/src/protocol/deserializer.cpp
  • core/src/protocol/protocol.hpp
  • core/src/protocol/serializer.hpp
  • firmware/CMakeLists.txt
  • firmware/src/app.cpp
  • firmware/src/uart/rx_buffer.hpp
  • firmware/src/uart/tx_buffer.hpp
  • firmware/src/uart/uart.cpp
  • firmware/src/uart/uart.hpp
  • firmware/src/usb/helper.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
  • firmware/src/usb/vendor.cpp
  • firmware/src/usb/vendor.hpp
  • firmware/src/utility/ring_buffer.hpp
  • host/src/utility/ring_buffer.hpp
🧰 Additional context used
🧠 Learnings (5)
📚 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/uart/uart.hpp
  • firmware/src/uart/rx_buffer.hpp
  • core/src/protocol/serializer.hpp
  • firmware/src/uart/tx_buffer.hpp
  • firmware/src/usb/vendor.hpp
  • host/src/utility/ring_buffer.hpp
  • firmware/src/usb/helper.hpp
  • core/src/protocol/protocol.hpp
  • firmware/src/utility/ring_buffer.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
📚 Learning: 2025-12-29T06:42:42.597Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 6
File: firmware/src/utility/lazy.hpp:22-22
Timestamp: 2025-12-29T06:42:42.597Z
Learning: In bare-metal firmware, objects with static storage duration that are lazily initialized should not rely on non-trivial destructors, because the system runs continuously until reset. If you have global/lazy-initialized objects in firmware, prefer empty or trivial destructors (or rely on startup/hardware reset) to avoid teardown issues. This guideline applies to firmware code across the repository, e.g., modules under firmware/ including header or implementation files that declare such global objects.

Applied to files:

  • firmware/src/uart/uart.hpp
  • firmware/src/uart/rx_buffer.hpp
  • firmware/src/uart/tx_buffer.hpp
  • firmware/src/usb/vendor.hpp
  • firmware/src/usb/helper.hpp
  • firmware/src/utility/ring_buffer.hpp
  • firmware/src/usb/interrupt_safe_buffer.hpp
📚 Learning: 2025-12-26T09:45:56.870Z
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 6
File: firmware/src/usb/interrupt_safe_buffer.hpp:25-47
Timestamp: 2025-12-26T09:45:56.870Z
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/uart/rx_buffer.hpp
  • firmware/src/uart/tx_buffer.hpp
  • firmware/src/usb/vendor.hpp
  • firmware/src/utility/ring_buffer.hpp
  • firmware/src/usb/interrupt_safe_buffer.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-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.cpp
🧬 Code graph analysis (4)
firmware/src/uart/uart.hpp (4)
firmware/src/usb/vendor.hpp (4)
  • data (62-64)
  • data (62-62)
  • data (66-68)
  • data (66-66)
firmware/src/uart/rx_buffer.hpp (4)
  • try_dequeue (50-50)
  • try_dequeue (53-53)
  • is_idle (102-141)
  • is_idle (102-102)
firmware/src/usb/helper.hpp (1)
  • get_serializer (7-7)
firmware/src/usb/vendor.cpp (2)
  • get_serializer (13-13)
  • get_serializer (13-13)
firmware/src/uart/tx_buffer.hpp (1)
firmware/src/utility/ring_buffer.hpp (4)
  • in (53-58)
  • in (66-71)
  • in (94-102)
  • out (79-86)
firmware/src/usb/vendor.hpp (1)
firmware/src/uart/uart.hpp (2)
  • data (41-41)
  • data (41-41)
firmware/src/utility/ring_buffer.hpp (1)
host/src/utility/ring_buffer.hpp (13)
  • in (61-65)
  • in (73-77)
  • in (100-107)
  • out (85-92)
  • emplace_back_n (121-143)
  • emplace_back_n (121-121)
  • value (180-183)
  • value (180-180)
  • value (188-194)
  • value (188-188)
  • pop_front_n (206-232)
  • pop_front_n (206-206)
  • pop_front_n (249-251)
🪛 Clang (14.0.6)
firmware/src/uart/uart.hpp

[error] 3-3: 'cstddef' file not found

(clang-diagnostic-error)

firmware/src/uart/rx_buffer.hpp

[error] 3-3: 'algorithm' file not found

(clang-diagnostic-error)

firmware/src/uart/tx_buffer.hpp

[error] 3-3: 'algorithm' file not found

(clang-diagnostic-error)

firmware/src/usb/vendor.hpp

[error] 3-3: 'algorithm' file not found

(clang-diagnostic-error)

firmware/src/usb/helper.hpp

[error] 3-3: 'core/src/protocol/serializer.hpp' file not found

(clang-diagnostic-error)

firmware/src/utility/ring_buffer.hpp

[error] 3-3: 'cstddef' file not found

(clang-diagnostic-error)

firmware/src/uart/uart.cpp

[error] 1-1: 'firmware/src/uart/uart.hpp' file not found

(clang-diagnostic-error)

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

[error] 8-8: 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 (34)
firmware/CMakeLists.txt (1)

13-13: LGTM!

启用 DMA 管理器的配置项添加合理,与其他 CONFIG_* 变量的模式一致。

host/src/utility/ring_buffer.hpp (1)

61-232: LGTM!代码重构提升了可读性

将原子加载操作提取到局部 const auto 变量的重构是一个良好的改进:

  • 使中间值显式化,提高代码可读性
  • 保留了正确的内存序(acquire/relaxed 的使用未改变)
  • 在某些方法中减少了重复的原子加载操作
  • 语义完全等价,无行为变化
firmware/src/usb/helper.hpp (2)

3-3: 该包含路径正确且文件存在。core/src/protocol/serializer.hpp 文件确实在项目中,include 语句有效且被正确使用(用于声明 core::protocol::Serializer 类型)。静态分析的"文件未找到"报告为误报。


7-7: 该函数的实现已正确处理了常见的固件初始化问题

函数实现位于 firmware/src/usb/vendor.cpp:13,返回由静态 Lazy<Vendor> 对象管理的 Serializer 引用。该实现通过以下机制确保安全性:

  • constinit 限定符保证常量初始化(C++20)
  • Lazy<T> 使用原子操作和 InterruptLockGuard 进行中断安全的延迟初始化
  • SerializerVendor 都具有平凡析构函数,避免了非平凡析构的问题
  • 静态存储期确保引用有效期与应用生命周期一致

该设计适合裸机固件环境。

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

44-48: 验证 UART 头部 API 重命名的完整性

类型别名重命名提升了语义清晰度:

  • DataLength (line 44,UartHeaderLayout)
  • DataLengthExtended (line 48,UartHeaderExtendedLayout)

移除 "Code" 后缀更准确地反映了这些字段表示实际长度而非编码值。验证结果确认:

  • 新名称在序列化器和反序列化器中使用正确(serializer.hpp 行 103、96;deserializer.cpp 行 126、132)
  • 未发现旧的 UART 头部名称残留引用
  • CAN 头部结构(行 33、38)仍使用 DataLengthCode,符合预期,为独立的类型定义

重命名已完整迁移,无遗漏引用。

firmware/src/app.cpp (2)

44-50: uart::uart3->try_transmit() 实现正确,无阻塞风险

try_transmit() 调用 TxBuffer::try_dequeue(),该方法完全非阻塞:

  • DMA 通道若仍在运行则立即返回 false(无等待)
  • 通过原子操作加载/存储缓冲区指针(无锁定)
  • 处理空闲标记的循环受 idle_buffer_ 大小限制,无死循环
  • UART 线路非空闲时快速返回 false(无轮询等待)
  • 仅通过寄存器写入触发 DMA 传输,实际数据由硬件异步处理

uart3 使用 Lazy<Uart> 类型正确初始化,与 ISR 中的 CAN 数据入队形成配对,设计符合裸机中断安全模式。


26-32: 初始化顺序正确,DMA 依赖关系已验证

uart3 对象已在 uart/uart.hpp 中正确声明(第 94-95 行)为 Uart::Lazy 类型。UART3 初始化确实依赖 DMA 管理器已初始化:TxBuffer 和 RxBuffer 的构造器会立即调用 init_dma(),该方法需要调用 dma_mgr_request_resource() 等 API。app.cpp 中 dma_mgr_init()(第 26 行)在 uart::uart3.init()(第 32 行)之前执行,依赖关系正确。所有初始化均在 InterruptLockGuard 下进行,确保线程安全。

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

1-11: LGTM!

ISR 实现简洁正确,通过 SDK 宏声明中断并委托给 uart3 对象处理。静态分析工具报告的宏和头文件问题是构建环境配置问题,不影响代码正确性。

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

117-147: LGTM!

UART 长度处理的重构逻辑清晰:

  • 非扩展长度直接使用 DataLength
  • 扩展长度读取 UartHeaderExtended 并验证边界
  • 零长度负载的处理正确,返回空 span
firmware/src/usb/vendor.cpp (2)

9-13: LGTM!

新增的 get_serializer() 访问器为 UART 上行数据提供序列化接口,与 PR 目标一致。


27-27: LGTM!

tud_suspend_cb 是 TinyUSB 所需的回调,空实现是可接受的。

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

55-59: LGTM!

UART3 数据正确路由到 uart::uart3->handle_downlink(),与相关代码片段中的接口一致。


96-100: ZLP 逻辑正确。

当传输大小恰好是 max_packet_size 的整数倍时,需要发送 ZLP 来标记传输结束,当前逻辑正确处理了这种情况。


113-114: LGTM!

新增的私有成员用于跟踪部分传输状态,设计合理。

firmware/src/uart/rx_buffer.hpp (5)

24-50: LGTM!

RxBuffer 模板类设计合理,使用 CRTP 模式实现上行数据处理回调,常量定义正确(缓冲区大小为 2 的幂次,掩码正确)。


143-156: LGTM!

update_in() 正确处理了 DMA 环形缓冲区的写指针更新,包括环绕情况的处理。直接读取硬件寄存器获取当前 DMA 目标地址是正确的做法。


158-163: LGTM!

缓存行对齐的失效操作正确,确保 DMA 写入的数据对 CPU 可见。


165-178: LGTM!

数据成员布局正确:

  • 数据缓冲区和 DMA 描述符都对齐到缓存行
  • 静态断言验证了 DMA 描述符的布局和大小
  • 原子索引确保 lock-free 访问

102-141: 确认 try_dequeue 存在并发调用安全问题。

UART 空闲中断和 DMA 中断均配置为优先级 1(见 uart.hpp:78rx_buffer.hpp:98),相同的优先级意味着两者可以相互抢占。try_dequeue 中对 out_ 的操作遵循加载-计算-存储的模式:

  1. out_.load(acquire) 加载当前值
  2. 处理数据和回调(handle_uplink
  3. out_.store(release) 写回更新值

虽然使用了 acquire/release 内存序,但这只保证内存可见性,无法防止并发执行。若 DMA 回调在 UART 空闲中断的加载和存储之间执行,会导致 out_ 的更新丢失,造成缓冲区指针不一致。

建议方案:

  1. 确保 UART 和 DMA 中断不能相互抢占(如设置不同优先级或禁用嵌套)
  2. 或在 try_dequeue 中使用 InterruptLockGuard(见 core/src/utility/interrupt_lock_guard.hpp)保护 out_ 的读改写操作
⛔ Skipped due to 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:56.870Z
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()` 执行期间保持稳定。
firmware/src/usb/interrupt_safe_buffer.hpp (3)

9-9: LGTM!

添加 constant.hpp 依赖以使用 kProtocolBufferSize,符合头文件排序规范(根据 learnings,系统库头文件应在项目头文件之前,此处项目头文件放置正确)。


20-21: LGTM!

常量重命名为 kBatchCount 符合 k-前缀命名规范。


57-66: LGTM!

使用 kProtocolBufferSize 替代硬编码的 batch_size,将缓冲区大小定义集中化,便于维护。

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

76-78: LGTM!

新增的 suffix_data 参数支持在 UART 负载后附加额外字节,默认值为空 span,保持向后兼容。


89-90: 扩展长度阈值修正正确。

将阈值从 > 4 改为 >= 4 是正确的修复。非扩展头的 DataLength 字段为 2 位,只能编码 0-3,因此长度为 4 时必须使用扩展头。


207-223: LGTM!

required_uart_size 正确更新以考虑 suffix_data,验证逻辑保持一致。

firmware/src/uart/uart.hpp (2)

94-95: LGTM!

使用 inline constinit 定义全局 uart3 实例是正确的做法。constinit 确保零初始化发生在运行时初始化之前,而 Lazy 模式符合固件中全局对象应使用平凡析构函数的约定(基于 learnings)。


67-67: 无需修改 - 使用的是 HPM SDK 的正确配置方式。

HPM SDK 中 txidle_config.idle_condrxidle_config.idle_cond 都使用同一个枚举类型 uart_rxline_idle_cond_t,这是 SDK 的设计。虽然枚举名称带有 "rxline" 前缀容易造成混淆,但该枚举实际上对 TX 和 RX 空闲检测都适用。代码用法符合 HPM SDK 文档要求。

Likely an incorrect or invalid review comment.

firmware/src/uart/tx_buffer.hpp (3)

42-96: try_enqueue 实现逻辑清晰,边界处理正确。

空闲边界的去重优化(第60-84行)设计合理,能有效减少 idle_buffer_ 的条目数量。对于零长度包(ZLP)和边界共享的处理也是正确的。


200-207: DMA 描述符联合体设计合理。

使用 unionstatic_assert 确保 dma_linked_descriptor_tdma_mgr_linked_descriptor_t 的布局兼容性是良好的实践,可以安全地在两种 API 之间共享内存。


212-214: in_flight_ 非原子变量的使用是安全的。

根据 learnings 中描述的裸机中断安全模型,try_dequeue() 仅在主线程调用,in_flight_ 不需要原子操作。这与项目中 InterruptSafeBuffer 的设计模式一致。

firmware/src/utility/ring_buffer.hpp (4)

21-26: IndexType 自动选择设计良好。

根据 max_size 自动选择最小的索引类型(uint8_t/uint16_t/uint32_t/uint64_t)可以在小型缓冲区上节省内存和原子操作开销。


41-45: 析构函数中调用 clear() 在固件环境下是安全的。

根据 learnings,裸机固件中全局对象不应依赖非平凡析构函数。由于系统运行直到复位,此析构函数实际上不会被调用。对于栈上或动态分配的 RingBuffer 实例,clear() 可以正确销毁剩余元素。


53-58: readable()writable() 内存序使用正确。

  • readable(): 对 in_ 使用 acquire 确保消费者能看到生产者构造的元素
  • writable(): 对 out_ 使用 acquire 确保生产者不会覆盖未消费的元素

这与 SPSC 无锁队列的标准模式一致。

Also applies to: 66-71


261-267: 存储布局和原子索引设计正确。

  • storage_ 使用 alignas(T) 确保正确对齐
  • static_assert 验证原子操作是无锁的
  • in_out_ 初始化为 0

整体实现与 Linux kfifo 的设计理念一致,适合嵌入式环境使用。

Comment threadfirmware/src/uart/tx_buffer.hpp
Comment threadfirmware/src/usb/vendor.hpp
Comment threadfirmware/src/utility/ring_buffer.hpp
- Enable DMA manager and initialize it at boot
- Add UART driver with DMA RX fragments and DMA TX ring-buffered output
- Route USB vendor UART downlink to UART3 and serialize uplink via usb::get_serializer()
- Other UART events are reserved as TODO
@qzhhhi
qzhhhiforce-pushed the feature/firmware-uart branch from fe90c62 to e25a92eCompareJanuary 11, 2026 12:26
Guard memcpy and pointer arithmetic when source spans may be empty (std::span::data() can be null):
- Serializer UART write: skip copies for empty payload/suffix
- Firmware UART TxBuffer enqueue: only copy when size > 0
- Firmware CAN TX: only copy CAN payload when non-empty
- Deserializer feed: treat empty input spans as no-op
Also make deserializer header self-contained by including <coroutine>.
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