vt13 test#78
Open
qzhhhi wants to merge 4 commits into
Open
Conversation
Member
Author
|
@coderabbitai pause |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough该 PR 重构遥控器设备抽象架构，将耦合的 Dr16 分解为独立设备层；新增 Vt13 协议设备，引入 RemoteControl 管理层统一输出注册和状态路由；升级 librmcs 依赖，更新 OmniInfantry 整合新架构和 RmcsBoardLite 板卡；增强 RingBuffer 工具类。同时删除多个过时的机器人硬件实现。 ChangesRemote Control Device Abstraction Refactoring
Utility Enhancement and Hardware Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp`:
- Around line 21-39: store_status currently publishes parts independently
(data_part1_, data_part2_, data_part3_), allowing readers in
dbus_receive_callback() / update() to observe a mixed-frame snapshot; change to
publish a consistent frame snapshot atomically by adding a version/seqlock or
double-buffer: e.g. introduce an atomic<uint32_t> status_seq_ (or seqlock) that
writers increment to an odd value before writing the three parts in store_status
and increment to even when done, and modify readers in
update()/dbus_receive_callback() to read status_seq_ before and after reading
data_part1_/data_part2_/data_part3_ and retry if the seq changed (or implement a
double-buffer struct and atomically swap a pointer to the completed buffer);
ensure kStatusSize checks remain and use the existing
data_part1_/data_part2_/data_part3_ storage but gate visibility via the new
sequence/atomic swap.
In `@rmcs_ws/src/rmcs_core/src/hardware/device/vt13.hpp`:
- Around line 178-187: 当前逻辑只在 readable < total_frame_size 时返回 Incomplete，当
header.data_length 导致 total_frame_size 超过本地环形缓冲区最大容量（data_buffer_
的容量）时会永远卡住坏帧头；请在计算 total_frame_size 后增加不可达上限检查（比较 data_buffer_.capacity()
或等效最大容量），如果 total_frame_size 超出容量则按 VerificationFailed 处理并丢弃该坏帧头（从 data_buffer_
弹出相应的头部字节以解除阻塞），确保引用的符号为
RefereeFrameHeader、header.data_length、total_frame_size、data_buffer_、Incomplete 和
VerificationFailed 以便定位修改点。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00970d8c-0268-4961-b122-85e86ad35eb0
📒 Files selected for processing (11)
rmcs_ws/src/rmcs_core/CMakeLists.txtrmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-omni.cpprmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-steering.cpprmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpprmcs_ws/src/rmcs_core/src/hardware/device/remote_control.hpprmcs_ws/src/rmcs_core/src/hardware/device/vt13.hpprmcs_ws/src/rmcs_core/src/hardware/omni_infantry.cpprmcs_ws/src/rmcs_core/src/hardware/sentry.cpprmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpprmcs_ws/src/rmcs_core/src/hardware/steering-infantry.cpprmcs_ws/src/rmcs_utility/include/rmcs_utility/ring_buffer.hpp
💤 Files with no reviewable changes (5)
- rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp
- rmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-steering.cpp
- rmcs_ws/src/rmcs_core/src/hardware/steering-infantry.cpp
- rmcs_ws/src/rmcs_core/src/hardware/sentry.cpp
- rmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-omni.cpp
Comment on lines
+21
to
39
| void store_status(std::span<const std::byte> uart_data) { | ||
| if (uart_data.size() != kStatusSize) | ||
| return; | ||
|
|
||
| // Avoid using reinterpret_cast here because it does not account for pointer alignment. | ||
| // Dr16DataPart structures are aligned, and using reinterpret_cast on potentially unaligned | ||
| // uart_data can cause undefined behavior on architectures that enforce strict alignment | ||
| // requirements (e.g., ARM). | ||
| // Directly accessing unaligned memory through a casted pointer can lead to crashes, | ||
| // inefficiencies, or incorrect data reads. Instead, std::memcpy safely copies the data from | ||
| // unaligned memory to properly aligned structures without violating alignment or strict | ||
| // aliasing rules. | ||
| auto* cursor = uart_data.data(); | ||
|
|
||
| uint64_t part1{}; | ||
| std::memcpy(&part1, uart_data, 6); | ||
| uart_data += 6; | ||
| std::memcpy(&part1, cursor, kPart1Size); | ||
| cursor += kPart1Size; | ||
| data_part1_.store(part1, std::memory_order::relaxed); | ||
|
|
||
| uint64_t part2{}; | ||
| std::memcpy(&part2, uart_data, 8); | ||
| uart_data += 8; | ||
| std::memcpy(&part2, cursor, kPart2Size); | ||
| cursor += kPart2Size; | ||
| data_part2_.store(part2, std::memory_order::relaxed); | ||
|
|
||
| uint32_t part3{}; | ||
| std::memcpy(&part3, uart_data, 4); | ||
| uart_data += 4; | ||
| std::memcpy(&part3, cursor, kPart3Size); | ||
| data_part3_.store(part3, std::memory_order::relaxed); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "检查 Dr16 的生产/消费路径："
rg -n -C2 'dr16_\.store_status|dr16_\.update_status|dbus_receive_callback|update\(\) override' rmcs_ws/src/rmcs_core/src/hardware
echo
echo "检查 Dr16 是否存在帧级同步："
rg -n -C2 'data_part[123]_\.store|data_part[123]_\.load|memory_order|sequence|version|mutex|lock_guard' rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hppRepository: Alliance-Algorithm/RMCS
Length of output: 2758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== OmniInfantry class definition ==="
rg -n "class OmniInfantry" rmcs_ws/src/rmcs_core/src/hardware/ -A 30
echo
echo "=== Check if dbus_receive_callback is called from interrupt/thread ==="
rg -n "dbus_receive_callback|register.*callback|set_callback" rmcs_ws/src/rmcs_core/src/hardware/ -B 2 -A 2
echo
echo "=== Check OmniInfantry initialization and threading ==="
rg -n "OmniInfantry\s*\(" rmcs_ws/src/rmcs_core/src/hardware/ -B 3 -A 5
echo
echo "=== Search for executor/thread pool ==="
rg -n "executor|thread_pool|thread|async|executor_" rmcs_ws/src/rmcs_core/src/hardware/omni_infantry.hpp -iRepository: Alliance-Algorithm/RMCS
Length of output: 6176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for RmcsBoardLite definition ==="
find . -type f \( -name "*.hpp" -o -name "*.h" \) | xargs rg -l "class RmcsBoardLite" 2>/dev/null | head -5
echo
echo "=== Search for where dbus_receive_callback registration happens ==="
rg -n "dbus_receive_callback|on_receive|RegisterCallback" rmcs_ws/src/rmcs_core/src/hardware/ -B 3 -A 3
echo
echo "=== Check if Component/executor provides thread safety guarantees ==="
find . -type f \( -name "*.hpp" -o -name "*.h" \) | xargs rg -l "class Component" | grep executor | head -3
echo
echo "=== Look for any synchronization in Dr16 class ==="
rg -n "class Dr16|private:|protected:" rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp -A 20 | head -60Repository: Alliance-Algorithm/RMCS
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Dr16 class full definition ==="
cat rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp
echo
echo "=== Check surrounding context in omni_infantry.cpp ==="
sed -n '290,300p' rmcs_ws/src/rmcs_core/src/hardware/omni_infantry.cpp
echo
echo "=== Check update_status full implementation ==="
sed -n '42,80p' rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp
echo
echo "=== Search for memory barriers or synchronization ==="
rg -n "acquire|release|seq_cst|barrier|fence" rmcs_ws/src/rmcs_core/src/hardware/device/Repository: Alliance-Algorithm/RMCS
Length of output: 9647
不要把一帧状态拆成 3 个独立原子发布。
虽然单字段上没有竞争，但没有保证帧级一致性。由于 dbus_receive_callback() 和 update() 运行在不同的执行上下文中，读者可能观察到摇杆、鼠标、键盘来自不同的帧（例如 part1 来自帧 N，part2/part3 来自帧 N+1）。应该使用版本号、seqlock 或双缓冲来一次性发布完整快照。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp` around lines 21 - 39,
store_status currently publishes parts independently (data_part1_, data_part2_,
data_part3_), allowing readers in dbus_receive_callback() / update() to observe
a mixed-frame snapshot; change to publish a consistent frame snapshot atomically
by adding a version/seqlock or double-buffer: e.g. introduce an atomic<uint32_t>
status_seq_ (or seqlock) that writers increment to an odd value before writing
the three parts in store_status and increment to even when done, and modify
readers in update()/dbus_receive_callback() to read status_seq_ before and after
reading data_part1_/data_part2_/data_part3_ and retry if the seq changed (or
implement a double-buffer struct and atomically swap a pointer to the completed
buffer); ensure kStatusSize checks remain and use the existing
data_part1_/data_part2_/data_part3_ storage but gate visibility via the new
sequence/atomic swap.
✅ Actions performedReviews paused. |
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.
概述
本PR进行了硬件架构的重构和升级，主要包括：移除了多个旧的机器人硬件组件实现；新增了遥控器设备驱动（Vt13和RemoteControl），支持新型遥控系统；调整了Dr16类的API接口和实现机制；更新了底层库依赖版本；同时扩展了环形缓冲区的功能。
核心变更
新增遥控器驱动支持
rmcs_core/src/hardware/device/vt13.hpp）：实现新型遥控器设备的UART数据接收与解析，支持遥控数据帧和裁判系统数据帧的CRC校验，管理模式切换、鼠标和键盘状态rmcs_core/src/hardware/device/remote_control.hpp）：统一封装遥控器输出接口，根据Vt13的模式切换动态选择从Dr16或Vt13读取鼠标/键盘数据Dr16类重构
store_status()参数格式，由指针+长度改为std::spannoexcept修饰符store_status()（存储原始数据）和update_status()（更新状态），提高线程安全性硬件组件精简
移除了以下过时的硬件组件实现：
DeformableInfantryOmni（deformable-infantry-omni.cpp）DeformableInfantryV2（deformable-infantry-steering.cpp）Sentry（sentry.cpp）SteeringInfantry（steering-infantry.cpp）OmniInfantry更新
librmcs::agent::CBoard升级为librmcs::agent::RmcsBoardLite依赖库升级
更新CMakeLists.txt中librmcs的版本从v3.1.0升级至v3.2.0rc0，同步更新SHA256校验值
工具库扩展
在
RingBuffer<T>中新增peek_front_n()方法，支持批量窥视缓冲区中的元素，用于数据处理优化影响范围