refactor(firmware/uart): Introduce DMA Ring Buffer Pipeline For UART#32
Merged
refactor(firmware/uart): Introduce DMA Ring Buffer Pipeline For UART#32
Conversation
- Add DMA-based RX ring buffer with IDLE event driven uplink on c_board - Add DMA-based TX ring buffer with idle checkpoints on c_board - Enable USART1/3/6 DMA streams, IRQ handlers, and priorities in CubeMX outputs - Align rmcs_board UART TX idle-checkpoint handling with the new boundary logic
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough引入基于 DMA 的 UART 收发子系统：新增通用 RxBuffer/TxBuffer 实现并重构 Uart 以私有继承缓冲器，绑定 HAL/DMA 中断与回调；扩展 BSP/中断向量以支持更多 USART 的 RX/TX DMA 流与 IRQ（环形 RX、普通 TX）。 Changes
Sequence Diagram(s)sequenceDiagram
participant UART as rgba(100,149,237,0.5) UART外设
participant DMA as rgba(34,139,34,0.5) DMA控制器
participant ISR as rgba(255,165,0,0.5) 中断处理
participant RxBuf as rgba(147,112,219,0.5) RxBuffer
participant Uart as rgba(220,20,60,0.5) Uart实例
participant App as rgba(70,130,180,0.5) 应用层
UART->>DMA: 接收数据写入 DMA 缓冲
DMA->>DMA: 多缓冲循环写入
DMA->>ISR: 触发 TC / M1TC / Error 事件
ISR->>RxBuf: 调用 uart_idle_event_callback()/dma_tc_callback()/rx_error_callback()
RxBuf->>RxBuf: update_in_and_switch_bank_if_requested()
App->>Uart: 调用 try_transmit()/try_dequeue()
Uart->>RxBuf: try_dequeue()
RxBuf->>Uart: handle_uplink(span1, span2, is_idle)
Uart->>App: 将上行数据通过序列化/USB 发出
sequenceDiagram
participant App as rgba(70,130,180,0.5) 应用层
participant Uart as rgba(220,20,60,0.5) Uart实例
participant TxBuf as rgba(147,112,219,0.5) TxBuffer
participant DMA as rgba(34,139,34,0.5) DMA控制器
participant ISR as rgba(255,165,0,0.5) 中断处理
participant UART as rgba(100,149,237,0.5) UART外设
App->>Uart: handle_downlink(data_view)
Uart->>TxBuf: try_enqueue(data_view)
TxBuf->>TxBuf: 记录/合并 idle 检查点
Uart->>TxBuf: try_transmit()
TxBuf->>TxBuf: try_dequeue()，准备连续数据或拷贝到 staging
TxBuf->>DMA: 启动 UART TX DMA（HAL 调用）
DMA->>ISR: 传输完成中断
ISR->>TxBuf: tx_complete_callback()
TxBuf->>Uart: 更新状态并允许后续发送
Estimated code review effort🎯 4 (复杂) | ⏱️ ~45 分钟 Possibly related PRs
诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
firmware/c_board/app/src/uart/tx_buffer.hpp (1)
114-190: 出队逻辑实现良好，但建议添加说明注释。
try_dequeue的两条路径设计合理：
- 严格包路径（第165-178行）：需要连续传输的数据使用 staging buffer，立即更新
out_- 直接路径（第181-187行）：可分段数据直接从环形缓冲区 DMA，使用
in_flight_延迟更新out_建议在第165行附近添加注释，说明
trailing_boundary_segmentable_的语义——它表示在回绕点的数据是否可以被分段传输。当前代码功能正确，但逻辑较复杂，额外注释有助于维护。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/app/src/uart/tx_buffer.hpp` around lines 114 - 190, The try_dequeue implementation is correct but needs an explanatory comment near the strict-path branch: add a concise comment by the check of trailing_boundary_segmentable_ (in try_dequeue) that explains trailing_boundary_segmentable_ means "data spanning the ring wrap point may be transmitted in segmented DMA chunks" and that when false we must flatten the wrapped region into staging_buffer_ and immediately advance out_ (and not rely on in_flight_). Reference try_dequeue, trailing_boundary_segmentable_, staging_buffer_, out_, and in_flight_ in the comment so future readers understand why the wrapped path uses staging and the non-wrapped path defers out_ update via in_flight_.firmware/c_board/app/src/uart/rx_buffer.hpp (1)
133-139: 注释与实现不一致。第134-135行的注释声明 "UART 中断优先级与 DMA 中断优先级相同，因此不需要加锁"，但实际调用的
update_in_and_switch_bank_if_requested方法在第160行使用了InterruptLockGuard。建议：
- 如果中断优先级确实相同且不会相互抢占，可以移除
InterruptLockGuard以减少中断延迟- 或者更新注释，说明使用锁是作为防御性编程的额外保障
根据
dma.c中的配置，USART IRQ 优先级为 1，对应的 DMA RX 流优先级也是 1（如 DMA1_Stream1、DMA2_Stream1、DMA2_Stream5），所以优先级匹配是正确的。当前的InterruptLockGuard提供了额外的安全保障。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/app/src/uart/rx_buffer.hpp` around lines 133 - 139, The comment in uart_idle_event_callback/dma_tc_callback incorrectly claims no locking is required, but update_in_and_switch_bank_if_requested uses InterruptLockGuard; update the comment in uart_idle_event_callback (and/or near update_in_and_switch_bank_if_requested) to state that although UART and DMA IRQ priorities match, InterruptLockGuard is retained as a defensive protection against concurrent access; alternatively, if you choose to remove the guard, remove InterruptLockGuard usage inside update_in_and_switch_bank_if_requested and ensure tests validate no preemption races—refer to uart_idle_event_callback, dma_tc_callback, update_in_and_switch_bank_if_requested and InterruptLockGuard when making the change.
🤖 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/c_board/app/src/uart/uart.cpp`:
- Around line 49-51: HAL_UART_AbortCpltCallback currently calls
get_uart_instance(...).tx_complete_callback() which only clears is_busy_ and
lets try_dequeue() unconditionally advance in_flight_ as if all bytes were sent;
to fix, detect the abort path and read the TX DMA NDTR (hdmatx->Instance->NDTR)
to compute actual transferred bytes and decrement in_flight_ accordingly, or
implement a dedicated abort handler (e.g. uart_tx_abort_handler) invoked from
HAL_UART_AbortCpltCallback that adjusts in_flight_, updates TX buffer state, and
mirrors the existing error-path protections (assert_debug) instead of reusing
the success tx_complete_callback; ensure get_uart_instance,
tx_complete_callback, hdmatx->Instance->NDTR and in_flight_ are the touchpoints
to modify.
In `@firmware/c_board/app/src/uart/uart.hpp`:
- Line 35: handle_downlink currently ignores the return value of
TxBuffer::try_enqueue, silently dropping frames when the TX buffer is full;
change handle_downlink (or its surrounding call path) to check the bool result
from TxBuffer::try_enqueue and make drops observable — e.g., increment a TxDrop
metric/counter or emit a log/trace with context (frame id/size) when try_enqueue
returns false; alternatively (if API change acceptable) propagate the
bool/failure up by changing handle_downlink's signature to return
success/failure so callers can retry or apply backpressure. Ensure you reference
TxBuffer::try_enqueue and handle_downlink when making the change.
---
Nitpick comments:
In `@firmware/c_board/app/src/uart/rx_buffer.hpp`:
- Around line 133-139: The comment in uart_idle_event_callback/dma_tc_callback
incorrectly claims no locking is required, but
update_in_and_switch_bank_if_requested uses InterruptLockGuard; update the
comment in uart_idle_event_callback (and/or near
update_in_and_switch_bank_if_requested) to state that although UART and DMA IRQ
priorities match, InterruptLockGuard is retained as a defensive protection
against concurrent access; alternatively, if you choose to remove the guard,
remove InterruptLockGuard usage inside update_in_and_switch_bank_if_requested
and ensure tests validate no preemption races—refer to uart_idle_event_callback,
dma_tc_callback, update_in_and_switch_bank_if_requested and InterruptLockGuard
when making the change.
In `@firmware/c_board/app/src/uart/tx_buffer.hpp`:
- Around line 114-190: The try_dequeue implementation is correct but needs an
explanatory comment near the strict-path branch: add a concise comment by the
check of trailing_boundary_segmentable_ (in try_dequeue) that explains
trailing_boundary_segmentable_ means "data spanning the ring wrap point may be
transmitted in segmented DMA chunks" and that when false we must flatten the
wrapped region into staging_buffer_ and immediately advance out_ (and not rely
on in_flight_). Reference try_dequeue, trailing_boundary_segmentable_,
staging_buffer_, out_, and in_flight_ in the comment so future readers
understand why the wrapped path uses staging and the non-wrapped path defers
out_ update via in_flight_.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2082925c-ae27-4430-ac07-6e5368aeb2ad
📒 Files selected for processing (12)
firmware/c_board/app/src/uart/rx_buffer.hppfirmware/c_board/app/src/uart/tx_buffer.hppfirmware/c_board/app/src/uart/uart.cppfirmware/c_board/app/src/uart/uart.hppfirmware/c_board/bsp/cubemx/Core/Inc/main.hfirmware/c_board/bsp/cubemx/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/cubemx/Core/Inc/usart.hfirmware/c_board/bsp/cubemx/Core/Src/dma.cfirmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/cubemx/Core/Src/usart.cfirmware/c_board/bsp/cubemx/rmcs_slave.iocfirmware/rmcs_board/src/uart/tx_buffer.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
firmware/c_board/app/src/uart/tx_buffer.hpp (1)
136-154:do-while循环逻辑合理，但考虑添加注释说明终止条件。该循环在
out_恰好位于 checkpoint 边界时等待 idle 超时，设计正确。但循环的终止条件依赖于：(1) 数据可读 (size > 0)，或 (2) idle 超时后 checkpoint 被消费。建议添加简短注释说明这一终止保证，便于后续维护。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/c_board/app/src/uart/tx_buffer.hpp` around lines 136 - 154, The do-while loop that computes size in tx_buffer.hpp can be hard to read because its termination relies on two conditions; add a brief comment above the loop (near the block that uses idle_checkpoints_.peek_front(), is_idle_, out, readable and kStagingBufferSize) that clearly states the loop exits when either data is available (size > 0) or when the idle timeout allows consuming the checkpoint (idle_checkpoints_.pop_front()), and note the early return when !is_idle_ to make the guarantee explicit for future maintainers.
🤖 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/c_board/app/src/uart/uart.cpp`:
- Around line 13-24: The function get_uart_instance has a control-path that can
fall through after calling assert_failed_debug(), leaving no return value;
replace the call to core::utility::assert_failed_debug() with a non-conditional
failure that always stops (e.g. core::utility::assert_failed_always()) or follow
it with a compiler hint like __builtin_unreachable() so the function cannot
return without a valid Uart reference; update the final branch in
get_uart_instance accordingly to ensure defined behavior for unknown UART
handles.
---
Nitpick comments:
In `@firmware/c_board/app/src/uart/tx_buffer.hpp`:
- Around line 136-154: The do-while loop that computes size in tx_buffer.hpp can
be hard to read because its termination relies on two conditions; add a brief
comment above the loop (near the block that uses idle_checkpoints_.peek_front(),
is_idle_, out, readable and kStagingBufferSize) that clearly states the loop
exits when either data is available (size > 0) or when the idle timeout allows
consuming the checkpoint (idle_checkpoints_.pop_front()), and note the early
return when !is_idle_ to make the guarantee explicit for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d12ef95-3a3b-4ec0-9fd9-caa7b077b927
📒 Files selected for processing (3)
firmware/c_board/app/src/uart/tx_buffer.hppfirmware/c_board/app/src/uart/uart.cppfirmware/c_board/app/src/uart/uart.hpp
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/c_board/app/src/uart/rx_buffer.hpp`:
- Around line 58-63: The RX DMA restart path leaves consumed_idle_count_ out of
sync with state.idle_count causing new packets to be misclassified as idle;
modify restart_rx_dma() (or start_rx_dma() when called from it) to reset/sync
consumed_idle_count_ to the current state.idle_count after clearing in/out
counters so is_idle logic (which compares state.idle_count !=
consumed_idle_count_) behaves correctly; apply the same synchronization fix in
the other spots referenced (the other start/restart flows around the regions
using consumed_idle_count_ at lines ~98-106 and ~146-156) so all restart paths
set consumed_idle_count_ = state.idle_count.
In `@firmware/c_board/app/src/uart/tx_buffer.hpp`:
- Around line 115-117: The idle-timing starts too early: change the logic so
is_idle_ and tx_complete_timepoint_ are only set after the UART_FLAG_TC
(transmission complete) is observed/set, not immediately on DMA completion;
locate the code that calls timer::timer->check_expired with
tx_complete_timepoint_ and the place that clears the TC flag, and move the
start-of-idle/timer setup to the branch where UART_FLAG_TC is confirmed (ensure
you don't clear TC before starting the timer and that clearing TC happens after
or as part of acknowledging the TC event); update the guards around is_idle_ and
tx_complete_timepoint_ so the 300µs idle window begins only after UART_FLAG_TC
is set.
In `@firmware/c_board/app/src/uart/uart.hpp`:
- Around line 76-88: handle_uplink currently treats SerializeResult::kBadAlloc
as a success and silently drops UART data; change handle_uplink to return a
boolean (or otherwise propagate success) and only treat
core::protocol::Serializer::SerializeResult::kSuccess as consumption, returning
false for kBadAlloc and kInvalidArgument (or asserting for invalid) so callers
know not to advance the ring buffer; then update RxBuffer::try_dequeue to call
handle_uplink and only advance out_ (the out_.store(...)) when handle_uplink
returned true, ensuring backpressure from usb::get_serializer()/write_uart
prevents data loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 141ebf48-2503-4c18-ba44-e398d816bce7
📒 Files selected for processing (12)
firmware/c_board/app/src/uart/rx_buffer.hppfirmware/c_board/app/src/uart/tx_buffer.hppfirmware/c_board/app/src/uart/uart.cppfirmware/c_board/app/src/uart/uart.hppfirmware/c_board/bsp/cubemx/Core/Inc/main.hfirmware/c_board/bsp/cubemx/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/cubemx/Core/Inc/usart.hfirmware/c_board/bsp/cubemx/Core/Src/dma.cfirmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/cubemx/Core/Src/usart.cfirmware/c_board/bsp/cubemx/rmcs_slave.iocfirmware/rmcs_board/src/uart/tx_buffer.hpp
Comment on lines
+58
to
+63
| const bool is_idle = (readable <= kProtocolMaxPayloadSize) | ||
| ? (state.idle_count != consumed_idle_count_) | ||
| : false; | ||
| if (is_idle) | ||
| consumed_idle_count_ = state.idle_count; | ||
| else if (readable < kMinFragmentSize) |
There was a problem hiding this comment.
重启 RX DMA 时请同步清空 idle 计数。
is_idle 取决于 Line 58-Line 63 的 state.idle_count != consumed_idle_count_，但 restart_rx_dma() 走到 start_rx_dma() 后只清了 in/out。如果错误发生前这两个计数已经不同，恢复后的第一段数据会被误判成 idle 包。
🛠️ 建议修复
- auto in_state = in_state_.load(std::memory_order::relaxed);
- in_state.in = 0;
- in_state_.store(in_state, std::memory_order::relaxed);
- out_.store(0, std::memory_order::relaxed);
+ InState in_state{};
+ in_state_.store(in_state, std::memory_order::relaxed);
+ out_.store(0, std::memory_order::relaxed);
+ consumed_idle_count_ = 0;Also applies to: 98-106, 146-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/c_board/app/src/uart/rx_buffer.hpp` around lines 58 - 63, The RX DMA
restart path leaves consumed_idle_count_ out of sync with state.idle_count
causing new packets to be misclassified as idle; modify restart_rx_dma() (or
start_rx_dma() when called from it) to reset/sync consumed_idle_count_ to the
current state.idle_count after clearing in/out counters so is_idle logic (which
compares state.idle_count != consumed_idle_count_) behaves correctly; apply the
same synchronization fix in the other spots referenced (the other start/restart
flows around the regions using consumed_idle_count_ at lines ~98-106 and
~146-156) so all restart paths set consumed_idle_count_ = state.idle_count.
Member
Author
There was a problem hiding this comment.
所有权分析：
consumed_idle_count_只在try_dequeue()中被读写（line 54, 59, 62）—— 这是消费者侧（主循环）start_rx_dma()被restart_rx_dma()调用，而restart_rx_dma()来自rx_error_callback()（line 141-143）—— 这是DMA 错误中断上下文
如果在中断上下文的 start_rx_dma() 中写 consumed_idle_count_，而主循环的 try_dequeue() 正在读它，就构成了数据竞争（data race），这是未定义行为。
重新评估 bug 的实际影响：
这个 bug 只在 rx_error_callback() → restart_rx_dma() 路径触发，此时：
- DMA 出错，通信流本身已经中断
- 最坏结果是重启后第一个包被误判为 idle，产生一次假的包边界
- 之后
consumed_idle_count_在try_dequeue()中自然同步，后续正常
结论：不值得修。 Bug 存在但影响极小（仅错误恢复路径的一次误判），而修复它要么引入数据竞争，要么需要把 consumed_idle_count_ 改成原子变量或加锁，代价不成比例。
There was a problem hiding this comment.
@qzhhhi，分析非常到位，完全认同。
consumed_idle_count_ 的所有权归消费者（主循环）独占，在中断上下文的 restart_rx_dma() 中写入它确实构成数据竞争——这比原来那次误判更糟糕。错误路径上的一次假 idle 边界影响极小且能自愈，不引入额外同步机制是正确的权衡。撤回这条建议。
✏️ Learnings added
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 32
File: firmware/c_board/app/src/uart/rx_buffer.hpp:58-63
Timestamp: 2026-03-08T08:13:53.514Z
Learning: 在 librmcs c_board 固件（firmware/c_board/app/src/uart/rx_buffer.hpp）中，`consumed_idle_count_` 是消费者侧（主循环 `try_dequeue()`）独占读写的字段，不受任何同步保护。`restart_rx_dma()` 由 `rx_error_callback()`（DMA 错误中断上下文）调用，因此**不能**在该路径中写 `consumed_idle_count_`，否则构成数据竞争（UB）。在 DMA 错误恢复后，第一个数据包可能被误判为 idle，但影响仅限于错误恢复路径的一次假包边界，`consumed_idle_count_` 会在下一次 `try_dequeue()` 中自然同步。这是经过权衡的有意设计：修复代价（引入原子变量或锁）不成比例。
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: 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()` 执行期间保持稳定。
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 32
File: firmware/c_board/app/src/uart/tx_buffer.hpp:115-117
Timestamp: 2026-03-08T07:59:26.041Z
Learning: 在 librmcs c_board 固件（firmware/c_board/app/src/uart/tx_buffer.hpp）中，`tx_complete_timepoint_` 是在 DMA 完成时（`tx_complete_callback()`）记录的，而非等待 `UART_FLAG_TC` 置位后才记录。这是一个经过权衡的有意设计：DMA 完成到 TC 的时间差仅为 1 字节传输时间（1 Mbaud 约 10µs，115200 约 87µs），而 300µs 的 idle 超时已足够保守，剩余空闲时间远超接收端 IDLE 检测所需。引入 TC 中断/轮询所增加的状态机复杂度不值得为这一微小时序偏差买单。
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 32
File: firmware/c_board/app/src/uart/uart.cpp:49-51
Timestamp: 2026-03-08T02:56:51.943Z
Learning: 在 librmcs c_board 固件（firmware/c_board/app/src/uart/uart.cpp）中，`HAL_UART_AbortCpltCallback` 是死代码：整个应用未调用 `HAL_UART_Abort_IT()`；HAL 内部错误恢复路径 `UART_DMAAbortOnError` 走 `HAL_UART_ErrorCallback`；RxBuffer 使用阻塞式 `HAL_DMA_Abort()`，不触发 AbortCplt 回调。该回调已在 PR `#32` 中被删除。
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 10
File: firmware/src/usb/vendor.hpp:87-93
Timestamp: 2026-01-11T11:53:36.838Z
Learning: 在 librmcs 固件项目中，TinyUSB 被配置为无缓冲模式（CFG_TUD_VENDOR_RX_BUFSIZE = 0, CFG_TUD_VENDOR_TX_BUFSIZE = 0），数据直接发送至 USB 端点。在 `firmware/src/usb/vendor.hpp` 的 `try_transmit()` 中，由于 `target_size` 总是 ≤ `max_packet_size` 且 `device_ready()` 预先检查端点可用性，`tud_vendor_n_write()` 在理想状况下应返回完整的 `target_size`，因此使用 `assert_debug(sent == target_size)` 作为调试断言是合理的。
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 32
File: firmware/c_board/app/src/uart/uart.hpp:76-88
Timestamp: 2026-03-08T08:04:08.064Z
Learning: 在 librmcs c_board 固件中，`InterruptSafeBuffer::allocate()`（firmware/c_board/app/src/usb 或类似路径的 interrupt_safe_buffer.hpp:45-47）在缓冲区满时会**直接调用** `led::led->uplink_buffer_full()` 并返回空 span。因此，上行数据路径（如 `Uart::handle_uplink()` → `serializer.write_uart()` → `allocate()`）中的 `kBadAlloc` 结果并非静默丢失——LED 已在 `allocate` 层报告，无需在 `handle_uplink` 层重复通知。这是项目分层错误报告的有意设计。
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 25
File: firmware/c_board/src/spi/spi.hpp:153-164
Timestamp: 2026-02-26T13:38:18.922Z
Learning: 在 librmcs 固件项目的 firmware/c_board/src/spi/spi.hpp 中，`trigger_dma()` 内对 `HAL_DMA_Start_IT` 的调用使用 `assert_debug` 检查返回值是有意为之的。在正确的代码中，DMA 资源已在 `init_dma_transfer()` 中配置妥当，`HAL_DMA_Start_IT` 不应失败。若失败则表明存在逻辑错误（如 DMA 未初始化、参数错误等），应在开发阶段通过断言捕获，而非作为运行时错误恢复场景处理。
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.
Learnt from: qzhhhi
Repo: Alliance-Algorithm/librmcs PR: 11
File: firmware/src/spi/bmi088/gyro.hpp:152-157
Timestamp: 2026-01-14T11:10:16.279Z
Learning: 在 librmcs 固件项目（firmware/src/spi/bmi088/*.hpp）中，对 `__attribute__((packed))` 标记的结构体使用 `reinterpret_cast` + `std::launder` 直接访问 SPI rx_buffer 是安全且有意为之的。编译器会为对齐要求为 1 的 packed 结构体生成显式的非对齐访问代码，在项目的目标平台（HPM SDK RISC-V）上经过验证可以正常工作。
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).
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.
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.
UART DMA 环形缓冲管线重构（简体中文摘要）
概述
本 PR（dev/cboard-uart → main，由 qzhhhi 提交）在 c_board 上引入基于 DMA 的 UART 收/发环形缓冲管线：RX 使用 IDLE 事件驱动的 DMA 环形/多缓冲接收并以上报分片，TX 使用空闲检查点（idle checkpoints）驱动的 DMA 单段传输。CubeMX 生成的 BSP/中断配置启用了 USART1、USART3、USART6 的 RX/TX DMA 流、IRQ 与优先级调整；rmcs_board 的 UART TX 空闲检查点处理也作了适配以匹配新的边界逻辑。PR 讨论线程中有作者多次 @coderabbitai 请求“full review”及自动回复机器人三次等值回覆，但尚无其他审阅讨论或批准记录。
主要改动点
新增：RX 缓冲（firmware/c_board/app/src/uart/rx_buffer.hpp）
新增：TX 缓冲（firmware/c_board/app/src/uart/tx_buffer.hpp）
Uart 类适配与回调分发（firmware/c_board/app/src/uart/uart.hpp/.cpp）
CubeMX / BSP / 中断 与 DMA 配置（生成代码变更）
rmcs_board 适配（firmware/rmcs_board/src/uart/tx_buffer.hpp）
影响与兼容性
代码审查重点（建议关注）
其他注意