Squash/deformable infantry#79
Open
Yukikaze2233 wants to merge 17 commits into
Open
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough该 PR 同步更新交叉构建脚本与文档、多个子模块指针、ROS2 启动/配置与插件注册；新增与重构远控设备层（Dr16/Vt13/RemoteControl）、新增 DeformableInfantryOmniB，并对底盘控制器、射击/摩擦轮控制、UI 与工具库进行广泛重构与增强。 Changes单一审阅切片（整体）
Estimated code review effort:
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/zh-cn/cross_build.md (1)
30-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win变体与目标架构的对应关系标注互换，存在误导。
根据 L12-13 的 sysroot 内置关系与 L28“对向架构”规则：交叉编译
arm64需要 arm64 sysroot（只存在于linux/amd64变体），交叉编译amd64需要 amd64 sysroot（只存在于linux/arm64变体）。因此 L34 与 L40 的变体标注被互换，照此操作会在缺少对应 sysroot 的容器中执行，导致 L91-94 自检失败。📝 建议修正
build-rmcs-cross --target-arch arm64-适用于
linux/arm64的latest-full变体。
+适用于linux/amd64的latest-full变体。build-rmcs-cross --target-arch amd64-适用于
linux/amd64的latest-full变体。
+适用于linux/arm64的latest-full变体。</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/zh-cn/cross_build.mdaround lines 30 - 40, 在 docs 文档中把 build-rmcs-cross
示例的变体说明与目标架构写反了：把 "build-rmcs-cross --target-arch arm64" 对应为 "适用于 linux/arm64 的
latest-full 变体" 和 "build-rmcs-cross --target-arch amd64" 对应为 "适用于 linux/amd64 的
latest-full 变体" 需要互换；改为说明交叉编译 arm64 时应使用 linux/amd64 变体，交叉编译 amd64 时应使用
linux/arm64 变体（修正与 L12-13 sysroot 关系和 L91-94 自检预期一致），更新文中与命令字符串
"build-rmcs-cross --target-arch arm64" 与 "build-rmcs-cross --target-arch amd64"
相邻的变体描述即可。</details> </blockquote></details> <details> <summary>rmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpp (1)</summary><blockquote> `113-134`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **last_* 状态更新应移出 `switch_right != Switch::DOWN` 分支**。当前 `last_switch_right_ / last_switch_left_ / last_keyboard_` 仅在 `if (switch_right != Switch::DOWN)` 内更新；当 `switch_right == Switch::DOWN` 时跳过更新，随后一旦离开 `DOWN`，边沿检测会基于过期的 `last_*` 可能触发（如 `keyboard.v` 上升沿、`MIDDLE -> UP` 切换）。对照 `bullet_feeder_controller_17mm.cpp` 和 `deformable_chassis.cpp`，它们都在 `update()` 末尾/循环外无条件刷新 `last_*`。建议把这三行移动到该 `if` 外。 <details> <summary>🐛 建议将 last_* 更新移出内层分支</summary> ```diff update_friction_velocities(); update_friction_status(); if (*friction_jammed_) RCLCPP_INFO(logger_, "Friction Jammed!"); if (*bullet_fired_) RCLCPP_INFO(logger_, "Bullet Fired!"); - - last_switch_right_ = switch_right; - last_switch_left_ = switch_left; - last_keyboard_ = keyboard; } + + last_switch_right_ = switch_right; + last_switch_left_ = switch_left; + last_keyboard_ = keyboard; }🤖 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/controller/shooting/friction_wheel_controller.cpp` around lines 113 - 134, The three "last" state updates (last_switch_right_, last_switch_left_, last_keyboard_) are only updated inside the if (switch_right != Switch::DOWN) block which causes stale edge-detection when switch_right == DOWN; move the lines that set last_switch_right_ = switch_right; last_switch_left_ = switch_left; last_keyboard_ = keyboard; so they execute unconditionally at the end of the update() path (outside that inner if) in friction_wheel_controller.cpp to mirror the behavior used in bullet_feeder_controller_17mm.cpp and deformable_chassis.cpp.
🧹 Nitpick comments (1)
rmcs_ws/src/rmcs_core/src/referee/app/ui/widget/status_ring.hpp (1)
248-272: ⚡ Quick win
update_supercap与update_supercap_energy的配色分支完全重复。两个函数中基于
value/enable的三段着色逻辑一字不差，可抽取一个私有辅助（如supercap_color(value, enable)）统一维护，避免后续两处阈值不一致。♻️ 建议的去重方式
+ static Shape::Color supercap_color(double value, bool enable) { + if (value > 20.0) + return enable ? Shape::Color::CYAN : Shape::Color::GREEN; + if (value > 13.5) + return enable ? Shape::Color::YELLOW : Shape::Color::ORANGE; + return enable ? Shape::Color::PURPLE : Shape::Color::PINK; + } + void update_supercap(double value, bool enable) { auto angle = 275 + calculate_angle(value, 8.0, supercap_limit_) + 1; supercap_status_.set_angle_end(static_cast<uint16_t>(angle)); - - if (value > 20.0) { - supercap_status_.set_color(enable ? Shape::Color::CYAN : Shape::Color::GREEN); - } else if (value > 13.5) { - supercap_status_.set_color(enable ? Shape::Color::YELLOW : Shape::Color::ORANGE); - } else { - supercap_status_.set_color(enable ? Shape::Color::PURPLE : Shape::Color::PINK); - } + supercap_status_.set_color(supercap_color(value, enable)); } void update_supercap_energy(double value, bool enable, double cutoff_voltage) { auto angle = 275 + calculate_energy_angle(value, cutoff_voltage, supercap_limit_) + 1; supercap_status_.set_angle_end(static_cast<uint16_t>(angle)); - - if (value > 20.0) { - supercap_status_.set_color(enable ? Shape::Color::CYAN : Shape::Color::GREEN); - } else if (value > 13.5) { - supercap_status_.set_color(enable ? Shape::Color::YELLOW : Shape::Color::ORANGE); - } else { - supercap_status_.set_color(enable ? Shape::Color::PURPLE : Shape::Color::PINK); - } + supercap_status_.set_color(supercap_color(value, enable)); }🤖 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/referee/app/ui/widget/status_ring.hpp` around lines 248 - 272, Both update_supercap(...) and update_supercap_energy(...) duplicate the same three-branch color logic; extract a private helper like supercap_color(double value, bool enable) that returns a Shape::Color based on the existing thresholds (>20.0, >13.5, else) and the enable flag, then replace the duplicated if/else blocks in both update_supercap and update_supercap_energy with a single call to supercap_status_.set_color(supercap_color(value, enable)); keep the threshold values and ternary mapping (CYAN/GREEN, YELLOW/ORANGE, PURPLE/PINK) exactly as-is to preserve behavior.
🤖 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_bringup/config/deformable-infantry-steering.yaml`:
- Line 90: 在 deformable-infantry-steering.yaml 中的 upper_limit 值为
-0.65（radians）但注释写为 "-35 deg" 不一致；请将注释与实际数值对齐（例如改为 "-37.2 deg" 或把数值改为 -0.610865
来匹配 -35°），以保持 upper_limit: -0.65 与注释一致并避免误导机械限位与 PID 调参。
In `@rmcs_ws/src/rmcs_bringup/launch/rmcs.launch.py`:
- Around line 54-63: The IncludeLaunchDescription for rmcs_auto_aim_v2 is being
appended unconditionally to entities; move that IncludeLaunchDescription (the
block that constructs
IncludeLaunchDescription(PythonLaunchDescriptionSource([FindPackageShare('rmcs_auto_aim_v2'),
'/launch.py']))) into the existing if is_automatic: branch so it is only
appended when is_automatic is true, removing the unconditional append to avoid
adding the auto-aim dependency in manual mode.
In `@rmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-omni-b.cpp`:
- Around line 501-506: 当前代码在收到 can_id == 0x300 时无论 data.can_data.size() 是否为 8
都会把 supercap_status_received_ 置为 true，导致 update() 在收到非法帧时开始发布旧值/默认值。修复方法：在处理
0x300 帧的分支中，把对 latest_supercap_status_.store(...)、supercap_.store_status(...) 和
supercap_status_received_.store(...) 的调用都包到 data.can_data.size() == 8 的条件块内（使用
device::CanPacket8{data.can_data}、latest_supercap_status_、supercap_.store_status
和 supercap_status_received_ 这几个标识符），只有在确认为合法 8 字节帧时才设置标志；对非 8 字节的帧则忽略，不改变
supercap_status_received_。
In `@rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp`:
- Around line 22-42: 当前实现先写入 last_receive_time_ 再逐个原子写入
data_part1_/data_part2_/data_part3_，会导致 update_status()/valid() 并发读取到跨帧拼接的数据；在
store_status 中先把 uart_data 解析到本地临时变量（例如 local_part1/part2/part3
或临时缓冲区），然后按原子方式一次性写回共享原子变量：先写
data_part1_/data_part2_/data_part3_（memory_order_relaxed），最后再写
last_receive_time_；确保使用的符号为 store_status, last_receive_time_, data_part1_,
data_part2_, data_part3_, update_status(), valid() 以避免读到混合帧快照。
- Around line 33-42: The concurrent snapshot can be torn because
Dr16::store_status() stores data_part1_/data_part2_/data_part3_ with
memory_order::relaxed and then updates last_receive_time_ also with relaxed,
while valid() and update_status() read last_receive_time_ relaxed; fix by
keeping payload stores (data_part1_/2_/3_) as relaxed but make the final
last_receive_time_ store use release semantics (store(...,
std::memory_order::release)) so it acts as a publish; on the reading side in
valid() and update_status() load last_receive_time_ with acquire semantics
(load(..., std::memory_order::acquire)) and only read the payload fields after
seeing a newer timestamp to ensure you observe a consistent snapshot for that
timestamp/sequence number, leaving atomic types unchanged.
In `@rmcs_ws/src/rmcs_core/src/hardware/device/remote_control.hpp`:
- Around line 44-85: The update() branching currently prioritizes dr16_ and
prevents vt13_ mode_switch() (kCine/kSport) from taking effect; change the
selection order to first check vt13_.valid() and vt13_.mode_switch() to apply
VT13-specific outputs, then if VT13 is invalid fall back to dr16_ only when
dr16_.valid(), and finally when neither source is valid explicitly write
zero/UNKNOWN/default values to all outputs (switch_right_output_,
switch_left_output_, joystick_*_output_, mouse_velocity_output_,
mouse_wheel_output_, mouse_output_, keyboard_output_, rotary_knob_output_,
rotary_knob_switch_output_) so stale DR16 state is never propagated. Ensure you
reference vt13_.mode_switch(), vt13_.valid(), dr16_.valid(), and update
assignments accordingly.
In `@rmcs_ws/src/rmcs_core/src/hardware/device/supercap.hpp`:
- Around line 31-32: The newly-registered input
"/chassis/supercap/control_enable" (supercap_control_enabled_) is not used in
generate_command() or generate_disable_command(), so the external enable signal
doesn't affect outgoing CAN commands; update the command.enabled calculation
inside generate_command() and generate_disable_command() to include
supercap_control_enabled_ (e.g., combine it with existing enable logic before
populating command.enabled) so the registered input participates in deciding
command.enabled and flows through to the CAN payload.
In `@rmcs_ws/src/rmcs_core/src/referee/app/ui/deformable_infantry_ui.cpp`:
- Around line 142-147: The chassis direction indicator is hard-coded to
set_angle(0, 30) in update_chassis_direction_indicator(), which prevents it
reflecting the actual chassis heading; update this function to compute the
indicator angle from the current chassis_angle_ (and any needed offset relative
to the gimbal/yaw or chassis_mode_), then call
chassis_direction_indicator_.set_angle(computed_angle, 30) so the UI reflects
real heading; if the fixed-forward behavior was intentional instead, add a clear
comment above update_chassis_direction_indicator() explaining that the indicator
is deliberately pinned to 0° and why.
---
Outside diff comments:
In `@docs/zh-cn/cross_build.md`:
- Around line 30-40: 在 docs 文档中把 build-rmcs-cross 示例的变体说明与目标架构写反了：把
"build-rmcs-cross --target-arch arm64" 对应为 "适用于 linux/arm64 的 latest-full 变体" 和
"build-rmcs-cross --target-arch amd64" 对应为 "适用于 linux/amd64 的 latest-full 变体"
需要互换；改为说明交叉编译 arm64 时应使用 linux/amd64 变体，交叉编译 amd64 时应使用 linux/arm64 变体（修正与
L12-13 sysroot 关系和 L91-94 自检预期一致），更新文中与命令字符串 "build-rmcs-cross --target-arch
arm64" 与 "build-rmcs-cross --target-arch amd64" 相邻的变体描述即可。
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpp`:
- Around line 113-134: The three "last" state updates (last_switch_right_,
last_switch_left_, last_keyboard_) are only updated inside the if (switch_right
!= Switch::DOWN) block which causes stale edge-detection when switch_right ==
DOWN; move the lines that set last_switch_right_ = switch_right;
last_switch_left_ = switch_left; last_keyboard_ = keyboard; so they execute
unconditionally at the end of the update() path (outside that inner if) in
friction_wheel_controller.cpp to mirror the behavior used in
bullet_feeder_controller_17mm.cpp and deformable_chassis.cpp.
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_core/src/referee/app/ui/widget/status_ring.hpp`:
- Around line 248-272: Both update_supercap(...) and update_supercap_energy(...)
duplicate the same three-branch color logic; extract a private helper like
supercap_color(double value, bool enable) that returns a Shape::Color based on
the existing thresholds (>20.0, >13.5, else) and the enable flag, then replace
the duplicated if/else blocks in both update_supercap and update_supercap_energy
with a single call to supercap_status_.set_color(supercap_color(value, enable));
keep the threshold values and ternary mapping (CYAN/GREEN, YELLOW/ORANGE,
PURPLE/PINK) exactly as-is to preserve behavior.
🪄 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: b7185d06-f616-488a-bed6-d9c8b9de4cd2
📒 Files selected for processing (39)
.script/build-rmcs-cross.script/clean-rmcs.script/complete/_build-rmcs-crossdocs/zh-cn/cross_build.mdrmcs_ws/src/hikcamerarmcs_ws/src/rmcs_auto_aim_v2rmcs_ws/src/rmcs_bringup/config/deformable-infantry-omni-b.yamlrmcs_ws/src/rmcs_bringup/config/deformable-infantry-omni.yamlrmcs_ws/src/rmcs_bringup/config/deformable-infantry-steering.yamlrmcs_ws/src/rmcs_bringup/launch/rmcs.launch.pyrmcs_ws/src/rmcs_bringup/package.xmlrmcs_ws/src/rmcs_core/CMakeLists.txtrmcs_ws/src/rmcs_core/plugins.xmlrmcs_ws/src/rmcs_core/src/controller/chassis/chassis_power_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_chassis.cpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_joint_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_joint_layer.hpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_omni_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_steering_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/bullet_feeder_controller_17mm.cpprmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpprmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-omni-b.cpprmcs_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/supercap.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_core/src/referee/app/ui/deformable_infantry_ui.cpprmcs_ws/src/rmcs_core/src/referee/app/ui/widget/animated_toggle.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/widget/crosshair_circle.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/widget/deformable_chassis_top_view.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/widget/status_ring.hpprmcs_ws/src/rmcs_core/src/referee/status.cpprmcs_ws/src/rmcs_utility/include/rmcs_utility/ring_buffer.hpp
💤 Files with no reviewable changes (4)
- rmcs_ws/src/rmcs_core/src/hardware/sentry.cpp
- rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp
- rmcs_ws/src/rmcs_core/src/hardware/steering-infantry.cpp
- rmcs_ws/src/rmcs_core/src/controller/chassis/deformable_joint_layer.hpp
| friction: 1.65 # Nm/(rad/s) | ||
|
|
||
| upper_limit: -0.61 # -35 deg | ||
| upper_limit: -0.65 # -35 deg |
There was a problem hiding this comment.
同步更新角度注释。
-0.65 rad 约等于 -37.2°，不是这里写的 -35 deg。这会误导后续机械限位和 PID 调参。
🤖 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_bringup/config/deformable-infantry-steering.yaml` at line
90, 在 deformable-infantry-steering.yaml 中的 upper_limit 值为 -0.65（radians）但注释写为
"-35 deg" 不一致；请将注释与实际数值对齐（例如改为 "-37.2 deg" 或把数值改为 -0.610865 来匹配 -35°），以保持
upper_limit: -0.65 与注释一致并避免误导机械限位与 PID 调参。
Comment on lines
54
to
+63
| if is_automatic: | ||
| pass | ||
|
|
||
| entities.append( | ||
| IncludeLaunchDescription( | ||
| PythonLaunchDescriptionSource([ | ||
| FindPackageShare('rmcs_auto_aim_v2'), '/launch.py' | ||
| ]) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
把 auto aim 的包含逻辑放回 if is_automatic 分支里。
现在 IncludeLaunchDescription(...) 是无条件追加的，手动机器人配置也会启动 rmcs_auto_aim_v2。这和前面的 is_automatic 判定相矛盾，还会给非自动模式引入额外依赖。
💡 建议修改
- if is_automatic:
- pass
-
- entities.append(
- IncludeLaunchDescription(
- PythonLaunchDescriptionSource([
- FindPackageShare('rmcs_auto_aim_v2'), '/launch.py'
- ])
- )
- )
+ if is_automatic:
+ entities.append(
+ IncludeLaunchDescription(
+ PythonLaunchDescriptionSource(
+ [FindPackageShare("rmcs_auto_aim_v2"), "/launch.py"]
+ )
+ )
+ )🤖 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_bringup/launch/rmcs.launch.py` around lines 54 - 63, The
IncludeLaunchDescription for rmcs_auto_aim_v2 is being appended unconditionally
to entities; move that IncludeLaunchDescription (the block that constructs
IncludeLaunchDescription(PythonLaunchDescriptionSource([FindPackageShare('rmcs_auto_aim_v2'),
'/launch.py']))) into the existing if is_automatic: branch so it is only
appended when is_automatic is true, removing the unconditional append to avoid
adding the auto-aim dependency in manual mode.
Comment on lines
+501
to
+506
| } else if (data.can_id == 0x300) { | ||
| if (data.can_data.size() == 8) | ||
| latest_supercap_status_.store( | ||
| device::CanPacket8{data.can_data}, std::memory_order_relaxed); | ||
| supercap_.store_status(data.can_data); | ||
| supercap_status_received_.store(true, std::memory_order_relaxed); |
There was a problem hiding this comment.
只在收到合法 supercap 帧后再置 supercap_status_received_。
现在即使 data.can_data.size() != 8，supercap_status_received_ 也会被置为 true。之后 update() 会开始发布旧值/默认值，并把它当成“已经收到状态”。
建议修改
} else if (data.can_id == 0x300) {
- if (data.can_data.size() == 8)
+ if (data.can_data.size() == 8) {
latest_supercap_status_.store(
device::CanPacket8{data.can_data}, std::memory_order_relaxed);
+ supercap_status_received_.store(true, std::memory_order_relaxed);
+ }
supercap_.store_status(data.can_data);
- supercap_status_received_.store(true, std::memory_order_relaxed);
}🤖 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/deformable-infantry-omni-b.cpp` around
lines 501 - 506, 当前代码在收到 can_id == 0x300 时无论 data.can_data.size() 是否为 8 都会把
supercap_status_received_ 置为 true，导致 update() 在收到非法帧时开始发布旧值/默认值。修复方法：在处理 0x300
帧的分支中，把对 latest_supercap_status_.store(...)、supercap_.store_status(...) 和
supercap_status_received_.store(...) 的调用都包到 data.can_data.size() == 8 的条件块内（使用
device::CanPacket8{data.can_data}、latest_supercap_status_、supercap_.store_status
和 supercap_status_received_ 这几个标识符），只有在确认为合法 8 字节帧时才设置标志；对非 8 字节的帧则忽略，不改变
supercap_status_received_。
Comment on lines
+22
to
42
| 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. | ||
| last_receive_time_.store(Clock::now(), std::memory_order_relaxed); | ||
|
|
||
| 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.
避免发布跨帧拼接的遥控状态。
这里把一帧拆成 3 个独立原子写入，而且先提交 last_receive_time_。如果 UART 回调和 update_status() 并发，valid() 可能已经变成 true，但 update_status() 读到的仍然可能是新旧分片混合的快照，摇杆/鼠标/键盘会来自不同帧。
Also applies to: 45-88
🤖 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 22 - 42,
当前实现先写入 last_receive_time_ 再逐个原子写入 data_part1_/data_part2_/data_part3_，会导致
update_status()/valid() 并发读取到跨帧拼接的数据；在 store_status 中先把 uart_data 解析到本地临时变量（例如
local_part1/part2/part3 或临时缓冲区），然后按原子方式一次性写回共享原子变量：先写
data_part1_/data_part2_/data_part3_（memory_order_relaxed），最后再写
last_receive_time_；确保使用的符号为 store_status, last_receive_time_, data_part1_,
data_part2_, data_part3_, update_status(), valid() 以避免读到混合帧快照。
Comment on lines
33
to
42
| 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
🌐 Web query:
In standard C++, is std::memory_order::relaxedvalid forstd::atomic::load/store, or should the enumerator be std::memory_order_relaxed?
💡 Result:
In standard C++, you can use either of these forms for the memory_order argument of std::atomic::load / std::atomic::store: 1) std::memory_order::relaxed 2) std::memory_order_relaxed Both are valid because std::memory_order is the enumeration type, with enumerator relaxed, and the library also provides inline constexpr constants like std::memory_order_relaxed that are defined in terms of that enumerator.[1][2] Also, the load/store APIs take an argument of type std::memory_order (by default memory_order::seq_cst), and have a precondition that the order argument is not release/acq_rel for load (and not consume/acquire/acq_rel for store), but relaxed satisfies these constraints.[3][4] So if your question is whether std::memory_order::relaxed is valid vs whether you should use std::memory_order_relaxed: both compile, and the standard library exposes both spellings; the “relaxed” semantics are the same.[1][2][3]
Citations:
- 1: https://cppreference.com/cpp/atomic/memory_order
- 2: https://cppreference.com/w/cpp/atomic/memory_order.html
- 3: https://timsong-cpp.github.io/cppwp/n4868/atomics.types.operations
- 4: https://timsong-cpp.github.io/cppwp/n4861/atomics.types.operations
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp"
if [ ! -f "$file" ]; then
echo "Missing file: $file"
exit 1
fi
# Show the relevant region around the cited lines and nearby status update logic
nl -ba "$file" | sed -n '1,120p'Repository: Alliance-Algorithm/RMCS
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp"
if [ ! -f "$file" ]; then
echo "Missing file: $file"
exit 1
fi
nl -ba "$file" | sed -n '1,220p'Repository: Alliance-Algorithm/RMCS
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp"
nl -ba "$file" | sed -n '1,260p'Repository: Alliance-Algorithm/RMCS
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="rmcs_ws/src/rmcs_core/src/hardware/device/dr16.hpp"
ls -la "$file"
cat -n "$file" | sed -n '1,220p'Repository: Alliance-Algorithm/RMCS
Length of output: 9511
纠正：std::memory_order::relaxed 无需改拼写；保留并重点处理并发快照一致性问题。
std::memory_order::relaxed用在std::atomic::load/store里是合法且语义等价于std::memory_order_relaxed，不属于编译阻塞问题（应移除对“改为_relaxed才能编译”的修改建议）。- 并发一致性：
Dr16::store_status()先last_receive_time_再依次写data_part1_/2_/3_，且全部使用memory_order::relaxed；valid()只看last_receive_time_（也为 relaxed），因此可能出现“时间戳已更新但 payload 仍是旧的/混合的”视图（尤其当store_status()与valid()/update_status()在不同线程并发时）。建议把last_receive_time_作为发布信号：先写三段 payload，再用release语义发布时间戳；读取侧用acquire（并可在update_status()中以时间戳/序列号保证读取到的是同一轮快照）。
🤖 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 33 - 42, The
concurrent snapshot can be torn because Dr16::store_status() stores
data_part1_/data_part2_/data_part3_ with memory_order::relaxed and then updates
last_receive_time_ also with relaxed, while valid() and update_status() read
last_receive_time_ relaxed; fix by keeping payload stores (data_part1_/2_/3_) as
relaxed but make the final last_receive_time_ store use release semantics
(store(..., std::memory_order::release)) so it acts as a publish; on the reading
side in valid() and update_status() load last_receive_time_ with acquire
semantics (load(..., std::memory_order::acquire)) and only read the payload
fields after seeing a newer timestamp to ensure you observe a consistent
snapshot for that timestamp/sequence number, leaving atomic types unchanged.
Comment on lines
+44
to
+85
| void update() { | ||
| if (dr16_.valid() || !vt13_.valid() || vt13_.mode_switch() == Vt13::ModeSwitch::kNormal) { | ||
| *switch_right_output_ = dr16_.switch_right(); | ||
| *switch_left_output_ = dr16_.switch_left(); | ||
|
|
||
| *joystick_right_output_ = dr16_.joystick_right(); | ||
| *joystick_left_output_ = dr16_.joystick_left(); | ||
|
|
||
| *mouse_velocity_output_ = dr16_.mouse_velocity(); | ||
| *mouse_wheel_output_ = dr16_.mouse_wheel(); | ||
|
|
||
| *mouse_output_ = dr16_.mouse(); | ||
| *keyboard_output_ = dr16_.keyboard(); | ||
| } else if (vt13_.mode_switch() == Vt13::ModeSwitch::kCine) { | ||
| *switch_right_output_ = rmcs_msgs::Switch::DOWN; | ||
| *switch_left_output_ = rmcs_msgs::Switch::DOWN; | ||
|
|
||
| *joystick_right_output_ = Eigen::Vector2d::Zero(); | ||
| *joystick_left_output_ = Eigen::Vector2d::Zero(); | ||
|
|
||
| *mouse_velocity_output_ = Eigen::Vector2d::Zero(); | ||
| *mouse_wheel_output_ = 0; | ||
|
|
||
| *mouse_output_ = rmcs_msgs::Mouse::zero(); | ||
| *keyboard_output_ = rmcs_msgs::Keyboard::zero(); | ||
| } else if (vt13_.mode_switch() == Vt13::ModeSwitch::kSport) { | ||
| *switch_right_output_ = rmcs_msgs::Switch::MIDDLE; | ||
| *switch_left_output_ = rmcs_msgs::Switch::MIDDLE; | ||
|
|
||
| *joystick_right_output_ = vt13_.joystick_right(); | ||
| *joystick_left_output_ = vt13_.joystick_left(); | ||
|
|
||
| *mouse_velocity_output_ = vt13_.mouse_velocity(); | ||
| *mouse_wheel_output_ = vt13_.mouse_wheel(); | ||
|
|
||
| *mouse_output_ = vt13_.mouse(); | ||
| *keyboard_output_ = vt13_.keyboard(); | ||
| } | ||
|
|
||
| *rotary_knob_output_ = dr16_.rotary_knob(); | ||
| *rotary_knob_switch_output_ = dr16_.rotary_knob_switch(); | ||
| } |
There was a problem hiding this comment.
当前分支顺序会让 VT13 的模式切换基本失效。
只要 dr16_.valid() 为 true，第一个分支就会先命中，kCine/kSport 永远不会生效；而当两个源都无效时，!vt13_.valid() 又会继续下发旧的 DR16 状态。建议先按 vt13_.valid() + mode_switch() 选源，再在 VT13 无效时回退到有效的 DR16，最后显式输出零值/UNKNOWN。
🤖 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/remote_control.hpp` around lines 44
- 85, The update() branching currently prioritizes dr16_ and prevents vt13_
mode_switch() (kCine/kSport) from taking effect; change the selection order to
first check vt13_.valid() and vt13_.mode_switch() to apply VT13-specific
outputs, then if VT13 is invalid fall back to dr16_ only when dr16_.valid(), and
finally when neither source is valid explicitly write zero/UNKNOWN/default
values to all outputs (switch_right_output_, switch_left_output_,
joystick_*_output_, mouse_velocity_output_, mouse_wheel_output_, mouse_output_,
keyboard_output_, rotary_knob_output_, rotary_knob_switch_output_) so stale DR16
state is never propagated. Ensure you reference vt13_.mode_switch(),
vt13_.valid(), dr16_.valid(), and update assignments accordingly.
Comment on lines
+31
to
+32
| command_component.register_input( | ||
| "/chassis/supercap/control_enable", supercap_control_enabled_); |
There was a problem hiding this comment.
control_enable 目前是死输入。
新注册的 /chassis/supercap/control_enable 没有参与 generate_command()/generate_disable_command()，所以上游新增的使能控制现在不会落到 CAN 命令上。至少要把它并入 command.enabled 的计算，否则这层接口契约是断的。
一个直接的修正方向
- command.enabled = *chassis_output_status_;
+ command.enabled = *chassis_output_status_ && *supercap_control_enabled_;Also applies to: 54-78
🤖 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/supercap.hpp` around lines 31 - 32,
The newly-registered input "/chassis/supercap/control_enable"
(supercap_control_enabled_) is not used in generate_command() or
generate_disable_command(), so the external enable signal doesn't affect
outgoing CAN commands; update the command.enabled calculation inside
generate_command() and generate_disable_command() to include
supercap_control_enabled_ (e.g., combine it with existing enable logic before
populating command.enabled) so the registered input participates in deciding
command.enabled and flows through to the CAN payload.
Comment on lines
142
to
147
| void update_chassis_direction_indicator() { | ||
| auto chassis_mode = *chassis_mode_; | ||
|
|
||
| auto to_referee_angle = [](double angle) { | ||
| return static_cast<int>( | ||
| std::round((2 * std::numbers::pi - angle) / std::numbers::pi * 180)); | ||
| }; | ||
| chassis_direction_indicator_.set_color(chassis_direction_indicator_color(chassis_mode)); | ||
| chassis_direction_indicator_.set_angle(to_referee_angle(*chassis_angle_), 30); | ||
| chassis_direction_indicator_.set_angle(0, 30); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
RoboMaster referee UI chassis direction indicator typical behavior gimbal relative
💡 Result:
In the RoboMaster competition environment, the referee system's client user interface (UI) typically includes a radar or map view that provides situational awareness to the operator. Regarding the orientation of the robot, the "chassis direction" indicator serves to help the operator understand the robot's current heading relative to its gimbal (or the arena). While specific UI implementations can vary by season and competition category (e.g., Collegiate vs. Youth), the chassis direction indicator in the referee UI is designed to function as follows: 1. Relationship to Gimbal: The indicator typically displays the chassis orientation relative to the gimbal's current yaw angle. Because robots often operate in "Gimbal Follow" mode—where the chassis automatically rotates to follow the gimbal's yaw—the UI provides a visual representation of the offset between the two. 2. Visual Representation: On the operator's interface, this is usually depicted as a directional icon (often a triangle or arrow) representing the chassis, overlaid with a representation of the gimbal's aim (often a line or cone). 3. "Gimbal Relative" Behavior: When the UI is in a gimbal-relative mode, the indicator shows the gimbal's aim direction as fixed (e.g., pointing straight up or forward on the UI), while the chassis indicator rotates to show how many degrees the chassis has deviated from the gimbal's centerline. Conversely, in a chassis-centric view, the chassis is often fixed, and the gimbal orientation rotates around it. It is important to distinguish between the referee system UI (which displays data reported by the robot to the server) and the onboard robot control logic. On the robot side, "Gimbal Lead" or "Chassis Follow" modes determine the physical mechanical rotation [1]. The referee UI merely visualizes the telemetry data sent by the robot's main controller regarding its yaw axis state [2][3]. Operators should refer to the specific "Referee System Interface Protocol" document provided for the current competition season, as the data packet structures for custom UI elements are updated annually to reflect changes in competition rules [4][3].
Citations:
- 1: https://dl.djicdn.com/downloads/ROBOMASTER_EP/20200515/RoboMaster%20EP%20Programming%20Guide%20V1.0.pdf
- 2: https://rm-static.djicdn.com/tem/70482/RoboMaster%20Youth%20Championship%20Referee's%20Client%20Interface%20Instructions%20(20220329).pdf
- 3: https://cdn-hz.robomaster.com/documents/3da8772acd7981525525229280483034.pdf
- 4: https://www.robomaster.com/en-US/products/components/detail/3207
底盘方向指示器固定为 set_angle(0, 30)，需确认是否有意
- 目前实现将角度写死，不再反映底盘朝向；而 RoboMaster 裁判系统客户端的“chassis direction”通常用于展示机器人航向/与云台的相对偏移。
- 若确为“始终指向正前方”，建议补充注释说明；否则应改为基于
chassis_angle_计算并设置角度。
🤖 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/referee/app/ui/deformable_infantry_ui.cpp` around
lines 142 - 147, The chassis direction indicator is hard-coded to set_angle(0,
30) in update_chassis_direction_indicator(), which prevents it reflecting the
actual chassis heading; update this function to compute the indicator angle from
the current chassis_angle_ (and any needed offset relative to the gimbal/yaw or
chassis_mode_), then call chassis_direction_indicator_.set_angle(computed_angle,
30) so the UI reflects real heading; if the fixed-forward behavior was
intentional instead, add a clear comment above
update_chassis_direction_indicator() explaining that the indicator is
deliberately pinned to 0° and why.
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
2 participants
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 对 RMCS 可变形步兵机器人仓库进行了大规模改动，涵盖构建脚本增强、交叉编译行为调整、驱动与硬件组件重组、控制器与低层设备接口重构、配置/启动调整以及若干 UI 与工具库改进。
构建与工具链
配置与启动
硬件驱动与设备接口重组
控制器与算法层重构
射击与运动控制调整
UI 与前端改进
公共库与工具变更
依赖与元数据
其他注意事项（影响范围）
代码规模与审查建议