Refactor chassis control for modularity, remove unused components#89
Open
ZGZ713912 wants to merge 24 commits into
Open
Refactor chassis control for modularity, remove unused components#89ZGZ713912 wants to merge 24 commits into
ZGZ713912 wants to merge 24 commits into
Conversation
- Consolidated joint input and output registration in deformable_chassis.cpp and deformable_wheel_controller.cpp using loops and arrays to reduce redundancy. - Removed unused PID control structures and related parameters in deformable_chassis.cpp to simplify the class. - Updated input and output handling for joint states to utilize arrays, improving scalability for future joint additions. - Enhanced readability by using fmt library for formatted string outputs in input/output registration. - Cleaned up unnecessary variables and methods related to IMU calibration and attitude correction in deformable_chassis.cpp.
- Updated the configuration for the deformable infantry steering to correct serial filter values. - Refactored the chassis control code to remove unnecessary scope motor control logic and related variables. - Simplified the hardware implementation by removing the IMU board and adjusting the top and bottom board initializations accordingly. - Enhanced the handling of gimbal pitch and yaw velocity calculations using separate BMI088 instances for better clarity and maintainability. - Updated CAN communication logic to accommodate the removal of the scope motor and streamline data transmission.
Reset the yaw loops only on ctrl-hold state transitions so yaw hold can keep producing torque while active. Remove stale hard-sync and duplicate BMI088 config paths across deformable infantry variants to match the single IMU pipeline.
…d remove ctrl-hold yaw lock - Delete standalone ActiveSuspensionController component - Restore active suspension logic (AttitudePidAxis, IMU calibration, joint I/O) inside DeformableChassis - Remove ctrl-hold yaw lock from both chassis angular-velocity override and gimbal PID lock - Gimbal yaw now follows normal solver path during ctrl-hold, pitch remains locked to level - Simplify configs: remove active_suspension_controller section, restore single-loop PID params
This commit deletes the DeformableChassisController class implementation from the deformable_wheel_controller.cpp file. The removal includes all associated methods, member variables, and dependencies, streamlining the codebase by eliminating unused components.
- Removed the entire deformable infantry steering implementation as it is no longer needed. - Updated the deformable infantry UI to use a loop for registering wheel velocities and joint physical angles, improving code maintainability. - Introduced `kWheelName` static array to avoid repetition and enhance clarity in the UI input registration. - Enhanced the update logic for deformable chassis leg arcs to use a more concise approach for checking readiness of joint angles.
commit 1e99fd9 Author: Zihan Qin <zihanqin2048@gmail.com> Date: Fri Jun 12 11:48:28 2026 +0800 feat: Add event interfaces and board-clocked IMU pipeline (#83) This commit lays the groundwork for upcoming IMU-camera hardware synchronization. - Add event input/output support to rmcs_executor - Add board clock, IMU snapshot, and raw camera frame message types - Integrate board-timestamped BMI088 EKF outputs in OmniInfantry - Add utility primitives used by the sync pipeline Known limitation: - OmniInfantry does not preserve the old BMI088 body-to-sensor mapping. This is intentional because that platform is retired and its original mechanical layout no longer exists. It is kept only as the remaining single-board hardware target for sync pipeline testing, so preserving its legacy IMU axis alignment is no longer necessary. commit c0ab658 Author: Zihan Qin <zihanqin2048@gmail.com> Date: Wed Jun 10 18:00:35 2026 +0800 feat: Fold climbable-infantry into maintained steering-infantry (#82) - Convert the active climbable-infantry line into the maintained steering-infantry variant. Remove the stair-climbing control chain and dedicated climber motors, revert the temporary VT13 / merged-remote stack back to pure DR16, and rename the cleaned result back to steering-infantry because the historical steering-infantry is no longer maintained. - Update SteeringInfantry to the rmcs-board-lite split top/bottom board path and refresh its bringup config for the current robot. Also restore the original /gimbal/calibrate and /steers/calibrate semantics, remove the temporary yaw watchdog path, make pitch control explicitly use LK velocity commands, and recover accidentally removed mainline content while keeping only the required steering-infantry merge delta. Update librmcs from v3.2.0b0 to v3.2.0 for the required board-lite SDK. Co-authored-by: Palejoker <2797572751@qq.com> Co-authored-by: Fin_Resect <chenchengyue201@126.com> commit b5ae6cb Author: Zihan Qin <zihanqin2048@gmail.com> Date: Tue Jun 9 22:20:27 2026 +0800 feat: Integrate steering-hero support (#80) Add support for steering-hero and steering-hero-little, including hardware drivers, chassis control, gimbal control, shooting control, Hero UI integration, corresponding bringup configs, and plugin registration. Also correct LK motor torque constants: - kMG5010Ei10: 0.90909 -> 0.1 - kMG6012Ei8: 1.09 -> 1.09 / 8.0 To keep mainline vehicle control output unchanged, also retune: - omni-infantry yaw velocity PID parameters - sentry bottom yaw velocity PID / viscous FF parameters Also remove the unmaintained mecanum-hero config. Known issues (not blocking this merge): - The Hero UI shooter condition / state_word path is still leftover debug wiring and is not connected to the current active runtime path - HeroFrictionWheelController still preserves the historical friction-wheel index convention; for now this is only documented with a TODO and should later be replaced by an explicit first-stage mapping - steering-hero-little still has a mismatch between PlayerViewer limit parameters and control logic, which requires follow-up calibration - HeroChassisPowerController inherits the existing ChassisPowerController risk of reading uninitialized members / propagating non-finite values; this is a pre-existing shared issue, not newly introduced by this merge - steering-hero-little uses different vehicle_radius values in steering_wheel_status and steering_wheel_controller - HeroFrictionWheelController does not clear its jam fault counter after recovery, and Ctrl+F currently triggers a double profile toggle - PutterController does not reset putter_timeout_count_ during normal stage transitions - ChassisClimberController repeatedly resets back_climber_recover_count during auto-climb - The Hero UI Ctrl+E bottom-yaw tracking toggle is currently level-triggered, so holding the keys causes repeated flipping Co-authored-by: floatpigeon <floatpigeon@proton.me> Co-authored-by: dwx5 <1591215786@qq.com> Co-authored-by: zhzy-star <2807406212@qq.com> commit 8407c1d Author: qzhhhi <zihanqin2048@gmail.com> Date: Sat May 30 23:17:41 2026 +0800 style: Apply clang-format across all C++ source files
commit 2c63c7d Author: creeper5820 <131014151+creeper5820@users.noreply.github.com> Date: Tue Jul 7 00:03:30 2026 +0800 feat: Integrate flight support (#85) - Add flight.yaml parameter config for Flight hardware, gimbal, friction wheels, feeder, referee interaction, auto-aim, and odin_ros_driver. - Add rmcs_core::hardware::Flight hardware plugin supporting flight controller, IMU, remote control, CAN/UART comms, TF, and referee serial. - Add rmcs_core::referee::app::ui::Flight UI plugin for ammo, friction wheel status, and auto-aim mode display. - Replace/add Flight-related hardware, controller, and UI components in plugin registry; remove some legacy chassis/controller registrations. - Add auto-aim toggle, yaw limit support, and direction clamping based on current angle to gimbal control. - Switch shooting control to /auto_aim/should_shoot input and refine auto/manual shooting logic. - Add kMHF7015 motor type support. - Add supercap/battery display toggle to StatusRing. - Minor CMake, description file, and export formatting/cleanup. Co-authored-by: heyeuu <2829004293@qq.com> Co-authored-by: zlq040222 <1542498005@qq.com> commit 1e99fd9 Author: Zihan Qin <zihanqin2048@gmail.com> Date: Fri Jun 12 11:48:28 2026 +0800 feat: Add event interfaces and board-clocked IMU pipeline (#83) This commit lays the groundwork for upcoming IMU-camera hardware synchronization. - Add event input/output support to rmcs_executor - Add board clock, IMU snapshot, and raw camera frame message types - Integrate board-timestamped BMI088 EKF outputs in OmniInfantry - Add utility primitives used by the sync pipeline Known limitation: - OmniInfantry does not preserve the old BMI088 body-to-sensor mapping. This is intentional because that platform is retired and its original mechanical layout no longer exists. It is kept only as the remaining single-board hardware target for sync pipeline testing, so preserving its legacy IMU axis alignment is no longer necessary. commit c0ab658 Author: Zihan Qin <zihanqin2048@gmail.com> Date: Wed Jun 10 18:00:35 2026 +0800 feat: Fold climbable-infantry into maintained steering-infantry (#82) - Convert the active climbable-infantry line into the maintained steering-infantry variant. Remove the stair-climbing control chain and dedicated climber motors, revert the temporary VT13 / merged-remote stack back to pure DR16, and rename the cleaned result back to steering-infantry because the historical steering-infantry is no longer maintained. - Update SteeringInfantry to the rmcs-board-lite split top/bottom board path and refresh its bringup config for the current robot. Also restore the original /gimbal/calibrate and /steers/calibrate semantics, remove the temporary yaw watchdog path, make pitch control explicitly use LK velocity commands, and recover accidentally removed mainline content while keeping only the required steering-infantry merge delta. Update librmcs from v3.2.0b0 to v3.2.0 for the required board-lite SDK. Co-authored-by: Palejoker <2797572751@qq.com> Co-authored-by: Fin_Resect <chenchengyue201@126.com> commit b5ae6cb Author: Zihan Qin <zihanqin2048@gmail.com> Date: Tue Jun 9 22:20:27 2026 +0800 feat: Integrate steering-hero support (#80) Add support for steering-hero and steering-hero-little, including hardware drivers, chassis control, gimbal control, shooting control, Hero UI integration, corresponding bringup configs, and plugin registration. Also correct LK motor torque constants: - kMG5010Ei10: 0.90909 -> 0.1 - kMG6012Ei8: 1.09 -> 1.09 / 8.0 To keep mainline vehicle control output unchanged, also retune: - omni-infantry yaw velocity PID parameters - sentry bottom yaw velocity PID / viscous FF parameters Also remove the unmaintained mecanum-hero config. Known issues (not blocking this merge): - The Hero UI shooter condition / state_word path is still leftover debug wiring and is not connected to the current active runtime path - HeroFrictionWheelController still preserves the historical friction-wheel index convention; for now this is only documented with a TODO and should later be replaced by an explicit first-stage mapping - steering-hero-little still has a mismatch between PlayerViewer limit parameters and control logic, which requires follow-up calibration - HeroChassisPowerController inherits the existing ChassisPowerController risk of reading uninitialized members / propagating non-finite values; this is a pre-existing shared issue, not newly introduced by this merge - steering-hero-little uses different vehicle_radius values in steering_wheel_status and steering_wheel_controller - HeroFrictionWheelController does not clear its jam fault counter after recovery, and Ctrl+F currently triggers a double profile toggle - PutterController does not reset putter_timeout_count_ during normal stage transitions - ChassisClimberController repeatedly resets back_climber_recover_count during auto-climb - The Hero UI Ctrl+E bottom-yaw tracking toggle is currently level-triggered, so holding the keys causes repeated flipping Co-authored-by: floatpigeon <floatpigeon@proton.me> Co-authored-by: dwx5 <1591215786@qq.com> Co-authored-by: zhzy-star <2807406212@qq.com> commit 8407c1d Author: qzhhhi <zihanqin2048@gmail.com> Date: Sat May 30 23:17:41 2026 +0800 style: Apply clang-format across all C++ source files
Co-authored-by: Copilot <copilot@github.com>
commit e95abf7 Author: ZGZ713912 <zgz713912@gmail.com> Date: Thu Jul 9 04:10:12 2026 +0800 refactor(chassis): simplify suspension toggle logic commit bef3729 Author: ZGZ713912 <zgz713912@gmail.com> Date: Thu Jul 9 02:57:18 2026 +0800 feat(controller): add deformable suspension component commit 5101024 Author: ZGZ713912 <zgz713912@gmail.com> Date: Wed Jul 8 23:12:09 2026 +0800 feat(chassis): enhance passive suspension parameters commit 5c483b6 Author: ZGZ713912 <zgz713912@gmail.com> Date: Wed Jul 8 22:38:45 2026 +0800 feat(chassis): add passive suspension parameters
- Output /auto_aim/camera_transform, /auto_aim/barrel_direction, /auto_aim/yaw_velocity from hardware layer - Expose gimbal_yaw_velocity() accessor on TopBoard - Remove standalone auto-aim launch (now runs as Component in RMCS) - Bump rmcs_auto_aim_v2 submodule (single-process architecture, fire controller & tracker refactor, dynamic yaw limits)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough本次变更同时更新了启动/配置、执行器接口、IMU 与时钟链路、可变形底盘控制、Hero 系列控制器与 UI、以及消息和工具库定义。还新增并替换了多个插件注册、硬件节点、控制器实现与数据结构。 Changes启动与配置
执行器与基础设施
可变形底盘与 Hero 传动链
UI、消息与格式化
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Member
Author
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rmcs_ws/src/rmcs_core/src/controller/gimbal/player_viewer.cpp (1)
94-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
viewer_delta_angle_by_mouse_wheel_被连续赋值两次，第一次赋值立即被覆盖。Line 94 使用灵敏度
0.095赋值后，Line 96 立即用0.09覆盖，第一次赋值是死代码。应删除其中一行，或补充缺失的条件分支。🐛 建议修复
*viewer_delta_angle_by_mouse_wheel_ = 0.5 * *mouse_wheel_ * unit_sensitivity(0.095); - *viewer_delta_angle_by_mouse_wheel_ = 0.5 * *mouse_wheel_ * unit_sensitivity(0.09); - if (viewer_reset_) {🤖 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/gimbal/player_viewer.cpp` around lines 94 - 96, The viewer mouse-wheel angle value is assigned twice in the same flow, and the first assignment is immediately overwritten. Update the logic in player_viewer::... that sets viewer_delta_angle_by_mouse_wheel_ so only the intended sensitivity is applied, either by removing the redundant assignment or restoring the missing conditional branch that should make both paths distinct.
🧹 Nitpick comments (12)
rmcs_ws/src/rmcs_core/src/controller/chassis/hero_steering_wheel_controller.cpp (2)
452-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
inf_常量未被使用。
nan_在第 375–376 行有使用，但inf_在整个文件中未被引用，属于死代码。建议移除。static constexpr double nan_ = std::numeric_limits<double>::quiet_NaN(); - static constexpr double inf_ = std::numeric_limits<double>::infinity();🤖 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/chassis/hero_steering_wheel_controller.cpp` around lines 452 - 453, The static constexpr inf_ constant in hero_steering_wheel_controller is unused and should be removed as dead code; keep the existing nan_ constant since it is referenced elsewhere in the controller logic, and delete inf_ from the same constants block so the file only contains actively used symbols.
369-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win移除无意义的零乘项。
0 * cos_varphi_[i]和0 * sin_varphi_[i]是死代码，不影响计算结果但会误导读者以为存在某个被置零的系数项。可以简化为：- auto x = ax - vehicle_radius_ * (az * sin_varphi_[i] + 0 * cos_varphi_[i]); - auto y = ay + vehicle_radius_ * (az * cos_varphi_[i] - 0 * sin_varphi_[i]); + auto x = ax - vehicle_radius_ * az * sin_varphi_[i]; + auto y = ay + vehicle_radius_ * az * cos_varphi_[i];🤖 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/chassis/hero_steering_wheel_controller.cpp` around lines 369 - 370, Remove the dead zero-multiply terms in hero steering wheel kinematics: in the chassis controller’s position calculation, simplify the expressions in the wheel-coordinate loop so the x and y formulas only use the meaningful az, sin_varphi_[i], and cos_varphi_[i] terms. Update the logic around the affected calculations in hero_steering_wheel_controller.cpp to keep the same result without the misleading 0 * cos_varphi_[i] and 0 * sin_varphi_[i] factors.rmcs_ws/src/rmcs_utility/include/rmcs_utility/ring_buffer.hpp (2)
486-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pop_front_until中无符号下溢依赖隐式回绕。Line 492
pos.origin_ + pos.offset_ - out在pos.origin_ < out（视图过期）时会发生无符号下溢，回绕为一个极大值。虽然 Line 493 的count > in - out检查能捕获此情况并安全返回 0，但依赖隐式回绕可读性较差。建议添加显式的过期检查。♻️ 建议改进
const auto in = in_.load(std::memory_order::acquire); const auto out = out_.load(std::memory_order::relaxed); - const auto count = pos.origin_ + pos.offset_ - out; - if (count > in - out) + if (pos.origin_ < out || pos.origin_ + pos.offset_ > in) return 0; + const auto count = pos.origin_ + pos.offset_ - out;🤖 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_utility/include/rmcs_utility/ring_buffer.hpp` around lines 486 - 497, In ring_buffer::pop_front_until, avoid relying on unsigned wraparound when computing the distance from pos to out_. Add an explicit stale-iterator check using pos.origin_, pos.offset_, and out before subtracting, so expired views return 0 directly; keep the existing bounds validation and pop_front_n call unchanged otherwise.
111-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
operator==与operator<=>比较范围不一致。
operator==（Lines 111-114）检查buffer_、origin_、offset_三个字段，而operator<=>（Lines 116-118）仅比较offset_。对于std::strong_ordering，a <=> b == equal应蕴含a == b，但来自不同视图的同offset_迭代器会违反这一契约。虽然跨容器比较本身是 UB，但两个比较运算符的语义不一致容易引发误用。♻️ 建议修复
friend std::strong_ordering operator<=>(BasicIterator lhs, BasicIterator rhs) { + if (lhs.buffer_ != rhs.buffer_) + return std::strong_ordering::less; return lhs.offset_ <=> rhs.offset_; }或至少添加注释说明
operator<=>仅适用于同源迭代器。🤖 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_utility/include/rmcs_utility/ring_buffer.hpp` around lines 111 - 118, `BasicIterator` 的 `operator==` 和 `operator<=>` 比较语义不一致：前者检查 `buffer_`、`origin_`、`offset_`，后者只比较 `offset_`，会导致相等关系与三路比较契约不匹配。请在 `BasicIterator` 的比较运算符实现中统一比较范围，确保 `operator<=>` 与 `operator==` 对同一组字段给出一致结果；如果 `operator<=>` 仅打算用于同源迭代器，请在该处补充明确注释并避免让它看起来可用于跨视图比较。rmcs_ws/src/rmcs_msgs/include/rmcs_msgs/camera_frame_raw.hpp (1)
12-26: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win注意
CameraFrameRaw的体积（~1.5 MB）及拷贝开销。
kFrameSize= 1440 × 1080 = 1,555,200 字节，整个结构体约 1.5 MB。由于结构体是值语义且未禁用拷贝，按值传递或在容器中按值存储会产生大量拷贝开销。建议在文档注释中明确该结构体应通过std::shared_ptr/std::unique_ptr传递，或考虑将帧数据改为堆分配的std::vector<std::byte>以支持移动语义。🤖 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_msgs/include/rmcs_msgs/camera_frame_raw.hpp` around lines 12 - 26, CameraFrameRaw is a large value-type (~1.5 MB) and can be expensive to copy; update the design guidance around CameraFrameRaw to make its intended ownership/ փոխանց usage explicit. In camera_frame_raw.hpp, add documentation near CameraFrameRaw and kFrameSize stating it should not be passed or stored by value and should use std::shared_ptr/std::unique_ptr instead, or refactor the data member to a heap-backed std::vector<std::byte> if you want move-friendly semantics.rmcs_ws/src/rmcs_executor/src/executor.hpp (1)
197-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
output_by_name冲突检查是冗余代码。Lines 176-192 的
declaration_kind_map检查已经会捕获同名不同 kind 的冲突并抛出异常，因此 Lines 197-207 对output_by_name的相同检查永远不会触发。可以移除这段死代码以减少维护负担。🤖 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_executor/src/executor.hpp` around lines 197 - 207, Remove the redundant conflict check in the output registration path: the `output_by_name.find(output.name)` block inside `executor.hpp` duplicates the earlier `declaration_kind_map` validation and will never be reached for the same-name different-kind case. Delete this dead branch and keep the existing `declaration_kind_map` enforcement as the single source of truth for conflicting interface kinds.rmcs_ws/src/rmcs_core/src/hardware/steering-hero-little.cpp (1)
39-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CanReceiveRateCounter是完全未使用的死代码。
CanReceiveRateCounter类定义（Lines 39-107）以及TopBoard/BottomBoard中所有相关的成员变量（Lines 488-491, 828-831）和调用点均已被注释掉。如果该功能暂不需要，建议直接移除这些代码；如果是为将来调试保留，建议通过条件编译宏控制而非注释保留。Also applies to: 488-491, 828-831
🤖 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/steering-hero-little.cpp` around lines 39 - 107, Remove the dead `CanReceiveRateCounter` code since it is no longer used: delete the class definition in `steering-hero-little.cpp`, along with the commented-out `TopBoard` and `BottomBoard` member declarations and any related call sites. If this CAN receive-rate logging is meant to stay for debugging, reintroduce it behind a clear compile-time feature flag instead of leaving the class and references commented out.rmcs_ws/src/rmcs_core/src/controller/adrc/eso.hpp (1)
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value可考虑使用
std::clamp替代自定义实现。项目已使用 C++20 特性（如
[[unlikely]]、requires），std::clamp（C++17 起）可直接使用，语义与当前实现完全一致。♻️ 可选重构
- static double clamp(double x, double lo, double hi) { return std::max(lo, std::min(x, hi)); } + static double clamp(double x, double lo, double hi) { return std::clamp(x, lo, hi); }🤖 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/adrc/eso.hpp` at line 69, The custom clamp helper in `Eso` can be simplified by replacing the local `clamp(double x, double lo, double hi)` implementation with `std::clamp`, since the project already targets C++20 and the standard utility matches the current behavior. Update the `clamp` call site inside the `ESO` header to use the standard algorithm and keep the same parameter ordering and semantics.rmcs_ws/src/rmcs_core/src/controller/chassis/deformable_suspension.cpp (1)
105-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
configure_active_rate_filters_在update()中的调用是冗余的
before_updating()（第 84 行）已使用1.0 / update_dt_()调用过configure_active_rate_filters_，而update()中的dt同样来自update_dt_()。由于configure_active_rate_filters_内部有采样频率变化检查，第二次调用为 no-op，但代码冗余。♻️ 移除冗余调用
const auto posture_target_angles_rad = read_posture_target_angles_rad_(); const auto dt = update_dt_(); - configure_active_rate_filters_(1.0 / dt);🤖 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/chassis/deformable_suspension.cpp` around lines 105 - 106, `update()` 中对 `configure_active_rate_filters_` 的调用是重复的，因为 `before_updating()` 已经用 `update_dt_()` 配置过采样频率。请在 `deformable_suspension::update()` 中移除这次基于 `dt` 的重复调用，保留 `before_updating()` 里的那次配置即可，确保 `update_dt_()`, `configure_active_rate_filters_`, `before_updating()`, 和 `update()` 的职责分离清晰。rmcs_ws/src/rmcs_core/src/controller/gimbal/two_axis_gimbal_solver.hpp (1)
33-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
enable_yaw_limit缺少参数校验
enable_yaw_limit未校验yaw_upper_limit >= yaw_lower_limit。若调用方传入错误顺序的参数，std::clamp(v, lo, hi)在lo > hi时为 C++20 未定义行为。🛡️ 建议添加校验
void enable_yaw_limit( rmcs_executor::Component& component, double yaw_upper_limit, double yaw_lower_limit) { + if (yaw_upper_limit < yaw_lower_limit) + std::swap(yaw_upper_limit, yaw_lower_limit); yaw_cw_max_ = yaw_upper_limit; yaw_cw_min_ = yaw_lower_limit; component.register_input("/gimbal/yaw/angle", gimbal_yaw_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/controller/gimbal/two_axis_gimbal_solver.hpp` around lines 33 - 38, `two_axis_gimbal_solver::enable_yaw_limit` is missing validation for the yaw bounds before storing them. Add a guard in `enable_yaw_limit` to verify `yaw_upper_limit >= yaw_lower_limit` before assigning `yaw_cw_max_` and `yaw_cw_min_`, and reject invalid input with a clear failure path (for example, throw or otherwise signal an error) so later `std::clamp` use in the gimbal solver cannot receive an invalid range. Keep the check close to the existing `enable_yaw_limit` logic in `two_axis_gimbal_solver`.rmcs_ws/src/rmcs_executor/include/rmcs_executor/component.hpp (1)
220-247: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win工作线程在回调抛异常后永久退出，后续事件将被静默丢弃。
worker_main在user_callback_抛出异常时记录日志后直接return，线程结束。此后enqueue()仍会继续入队/丢弃，但再也没有消费者，导致该事件通道被静默停用（仅首次丢弃时打一条 WARN）。对于控制系统中的事件流，单次瞬时异常即造成永久性功能失效，风险较高。请确认这是否为预期设计。若希望具备容错性，可考虑捕获异常后继续循环（丢弃当前事件并
pop_front），而非终止线程。🤖 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_executor/include/rmcs_executor/component.hpp` around lines 220 - 247, The worker_main loop in QueuedEventInputInterface currently terminates the worker thread whenever user_callback_ throws, which permanently disables the queue consumer. Update the exception handling in worker_main so that a callback failure only affects the current event: log the error, ensure the event is removed from queue_ with pop_front, and then continue the loop instead of returning. Keep the same behavior for the normal success path and preserve the stop_requested_ and event_count_ wait logic.rmcs_ws/src/rmcs_core/src/controller/pid/friction_wheel_pid_recorder.cpp (1)
84-106: 🚀 Performance & Scalability | 🔵 Trivial实时循环内的同步文件 I/O 可能引入抖动。
update()在执行器循环（配置中update_rate: 1000.0）上被调用，而这里每个使能周期都直接向log_stream_写入并周期性flush()。磁盘write/flush是阻塞调用，可能在控制线程上造成不可预期的延迟抖动。若该记录器会随实车控制一起运行，建议将序列化后的行推入无锁队列，由独立后台线程负责落盘。若仅用于离线调参台架，可忽略。🤖 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/pid/friction_wheel_pid_recorder.cpp` around lines 84 - 106, The synchronous file writes in update() can block the real-time control loop and introduce jitter. Refactor friction_wheel_pid_recorder::update to avoid direct log_stream_ writes and flush() calls on the executor thread; instead, serialize each sample into a queue/buffer and have a separate background worker thread handle disk I/O and periodic flushing. Keep the existing run_ids_/sample_indices_ bookkeeping in update(), but move the actual persistence logic out of the control path.
🤖 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/controller/chassis/chassis_power_controller.cpp`:
- Around line 95-99: `reset_power_control()` currently clears
`virtual_buffer_energy_`, `boost_mode_`, and `chassis_control_power_limit_`, but
it no longer resets the `supercap_control_enabled_` output, so that signal can
stay enabled in safety/RC-disconnect states. Restore the output reset inside
`reset_power_control()` in `chassis_power_controller.cpp` by setting
`supercap_control_enabled_` to false along with the other power-control outputs,
so the `ChassisPowerController` leaves supercap control disabled whenever this
reset path is taken.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/deformable_mode.hpp`:
- Around line 176-177: `complex_spin_active_` is never set to true, so the
complex spin path in `deformable_mode.hpp` is unreachable and its timer state
never changes. Either implement the missing activation path by setting
`complex_spin_active_` from the relevant mode transition logic in
`deactivate_complex_spin_()`/the surrounding mode handlers, or remove the dead
`if (complex_spin_active_)` branch, the complex spin elapsed-time updates, and
the `complex_spin_active_`/`complex_spin_elapsed_` state if the feature is not
intended to exist.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/hero_chassis_controller.cpp`:
- Around line 185-190: The comment and implementation in
hero_chassis_controller::updateChassisBehavior are inconsistent: the AUTO
rear-following scaling is capped at 0.3, so it cannot be “fully enabled at max”
as the comment says. Either update the clamp in the
measured_translational_speed-based angular_velocity scaling to allow a full 1.0
at translational_velocity_max, or revise the nearby comment to match the
intended 30% maximum behavior.
In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/dual_yaw_controller.cpp`:
- Around line 112-118: The wrap_angle() helper in dual_yaw_controller.cpp has a
broken normalization loop that can run forever for positive inputs; update the
first loop condition so it normalizes negative angles into the [0, 2π) range
instead of incrementing positive values indefinitely. Keep the change localized
to wrap_angle(), and verify the logic still returns a wrapped angle consistent
with how bottom_yaw_control_error() handles angle wrapping.
In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/player_viewer.cpp`:
- Around line 30-32: The player_viewer input registration is creating unused
subscriptions: `gimbal_pitch_angle_` and `gimbal_pitch_raw_angle_` are
registered in `player_viewer.cpp` but never referenced, and
`gimbal_pitch_angle_` duplicates the `"/gimbal/player_viewer/angle"` path
already used by `gimbal_player_viewer_angle_`. Update the `PlayerViewer` setup
to remove the redundant `register_input` calls or wire these members into actual
reads where they are intended to be used, and ensure each input path is
registered only once.
In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/two_axis_gimbal_solver.hpp`:
- Around line 208-229: `clamp_yaw_limit` in `TwoAxisGimbalSolver` is clamping an
unnormalized `target_cw`, so equivalent angles outside `[0, 2π)` can be
incorrectly limited. Normalize `target_cw` into the same `[0, 2π)` domain as
`yaw_cw_min_`/`yaw_cw_max_` before calling `std::clamp`, then keep the existing
rotation update logic for `control_direction` unchanged.
In
`@rmcs_ws/src/rmcs_core/src/controller/shooting/hero_friction_wheel_controller.cpp`:
- Around line 71-95: The profile toggle logic in
hero_friction_wheel_controller::update is being triggered twice for the same
ctrl+f press because both the plain f edge check and the ctrl+f edge check can
fire together. Consolidate the handling into a single branch for the intended
shortcut, and ensure active_profile_ ^= 1 runs only once per key transition
while preserving the friction_soft_start_stop_percentage_ recalculation.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Around line 56-91: `shooting_recorder`’s update/logging path contains unused
variables, empty log writes, and per-call file I/O that should be cleaned up.
Remove or use the dead assignments (`v`, `timestamp`) in the update flow, build
`log_text` only when `friction_wheel_count_ == 4` and skip both `log_stream_`
and `RCLCPP_INFO` when it is empty, and stop reopening `"shoot_recorder"` on
every update by reusing a persistent stream or buffering writes. Also remove or
wire up the unused `friction_velocities_` and `friction_wheels_velocity_`
members and delete the dead helper methods (`analysis1`, `analysis2`, `GetTime`)
if they are not referenced anywhere.
- Around line 33-36: The shooting recorder setup in shooting_recorder.cpp has
unsafe file handling: the log path built with fmt::format("/robot_shoot/{}.log",
ms) may fail if the directory is missing or unwritable, log_stream_.open(file)
is not validated, and the temporary std::ofstream out_file("shoot_recorder") is
created without being used. Update the recorder initialization around
log_stream_.open and the local out_file creation to use a writable, ensured
path, check the stream state immediately after opening, and either write
meaningful content to out_file or remove it if it is unnecessary.
- Around line 237-242: `analysis3()` can index past the end of `velocities` when
there is only one sample, so add the same minimum-size guard used by
`analysis1()` before computing `range2_`. In `shooting_recorder.cpp`, update the
logic around `analysis3()` to require at least 3 elements before reading
`velocities[1]` and `velocities[size - 2]`, and provide a safe fallback for
smaller inputs. Use the existing `velocities` handling and `range_`/`range2_`
assignments as the place to apply the check.
In `@rmcs_ws/src/rmcs_core/src/referee/app/ui/deformable_infantry_ui.cpp`:
- Around line 152-166: `update_deformable_chassis_leg_arcs()` currently
dereferences `*chassis_angle_` without verifying it is ready, which can cause
undefined behavior on early updates. Update the readiness guard in this function
to include `chassis_angle_.ready()` alongside the existing `std::all_of` check
for `joint_physical_angle_`, and return early or hide
`deformable_chassis_leg_arcs_` when any required input is not ready. Keep the
existing `active_suspension_active_.ready()` handling consistent with the other
inputs before calling `deformable_chassis_leg_arcs_.update()`.
In `@rmcs_ws/src/rmcs_core/src/referee/app/ui/hero.cpp`:
- Around line 287-292: Ctrl+E toggling in update_bottom_yaw_tracking_lines() is
level-triggered and flips bottom_yaw_tracking_enabled_ every frame while held.
Change this logic to edge-triggered by tracking an independent previous-frame
Ctrl+E state (for example a new last_bottom_yaw_ctrl_e_ member) and only
toggling when the key transitions from not pressed to pressed; keep
bottom_yaw_anchor_angle_rad_ updates tied to that single transition.
---
Outside diff comments:
In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/player_viewer.cpp`:
- Around line 94-96: The viewer mouse-wheel angle value is assigned twice in the
same flow, and the first assignment is immediately overwritten. Update the logic
in player_viewer::... that sets viewer_delta_angle_by_mouse_wheel_ so only the
intended sensitivity is applied, either by removing the redundant assignment or
restoring the missing conditional branch that should make both paths distinct.
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_core/src/controller/adrc/eso.hpp`:
- Line 69: The custom clamp helper in `Eso` can be simplified by replacing the
local `clamp(double x, double lo, double hi)` implementation with `std::clamp`,
since the project already targets C++20 and the standard utility matches the
current behavior. Update the `clamp` call site inside the `ESO` header to use
the standard algorithm and keep the same parameter ordering and semantics.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/deformable_suspension.cpp`:
- Around line 105-106: `update()` 中对 `configure_active_rate_filters_` 的调用是重复的，因为
`before_updating()` 已经用 `update_dt_()` 配置过采样频率。请在
`deformable_suspension::update()` 中移除这次基于 `dt` 的重复调用，保留 `before_updating()`
里的那次配置即可，确保 `update_dt_()`, `configure_active_rate_filters_`,
`before_updating()`, 和 `update()` 的职责分离清晰。
In
`@rmcs_ws/src/rmcs_core/src/controller/chassis/hero_steering_wheel_controller.cpp`:
- Around line 452-453: The static constexpr inf_ constant in
hero_steering_wheel_controller is unused and should be removed as dead code;
keep the existing nan_ constant since it is referenced elsewhere in the
controller logic, and delete inf_ from the same constants block so the file only
contains actively used symbols.
- Around line 369-370: Remove the dead zero-multiply terms in hero steering
wheel kinematics: in the chassis controller’s position calculation, simplify the
expressions in the wheel-coordinate loop so the x and y formulas only use the
meaningful az, sin_varphi_[i], and cos_varphi_[i] terms. Update the logic around
the affected calculations in hero_steering_wheel_controller.cpp to keep the same
result without the misleading 0 * cos_varphi_[i] and 0 * sin_varphi_[i] factors.
In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/two_axis_gimbal_solver.hpp`:
- Around line 33-38: `two_axis_gimbal_solver::enable_yaw_limit` is missing
validation for the yaw bounds before storing them. Add a guard in
`enable_yaw_limit` to verify `yaw_upper_limit >= yaw_lower_limit` before
assigning `yaw_cw_max_` and `yaw_cw_min_`, and reject invalid input with a clear
failure path (for example, throw or otherwise signal an error) so later
`std::clamp` use in the gimbal solver cannot receive an invalid range. Keep the
check close to the existing `enable_yaw_limit` logic in
`two_axis_gimbal_solver`.
In `@rmcs_ws/src/rmcs_core/src/controller/pid/friction_wheel_pid_recorder.cpp`:
- Around line 84-106: The synchronous file writes in update() can block the
real-time control loop and introduce jitter. Refactor
friction_wheel_pid_recorder::update to avoid direct log_stream_ writes and
flush() calls on the executor thread; instead, serialize each sample into a
queue/buffer and have a separate background worker thread handle disk I/O and
periodic flushing. Keep the existing run_ids_/sample_indices_ bookkeeping in
update(), but move the actual persistence logic out of the control path.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero-little.cpp`:
- Around line 39-107: Remove the dead `CanReceiveRateCounter` code since it is
no longer used: delete the class definition in `steering-hero-little.cpp`, along
with the commented-out `TopBoard` and `BottomBoard` member declarations and any
related call sites. If this CAN receive-rate logging is meant to stay for
debugging, reintroduce it behind a clear compile-time feature flag instead of
leaving the class and references commented out.
In `@rmcs_ws/src/rmcs_executor/include/rmcs_executor/component.hpp`:
- Around line 220-247: The worker_main loop in QueuedEventInputInterface
currently terminates the worker thread whenever user_callback_ throws, which
permanently disables the queue consumer. Update the exception handling in
worker_main so that a callback failure only affects the current event: log the
error, ensure the event is removed from queue_ with pop_front, and then continue
the loop instead of returning. Keep the same behavior for the normal success
path and preserve the stop_requested_ and event_count_ wait logic.
In `@rmcs_ws/src/rmcs_executor/src/executor.hpp`:
- Around line 197-207: Remove the redundant conflict check in the output
registration path: the `output_by_name.find(output.name)` block inside
`executor.hpp` duplicates the earlier `declaration_kind_map` validation and will
never be reached for the same-name different-kind case. Delete this dead branch
and keep the existing `declaration_kind_map` enforcement as the single source of
truth for conflicting interface kinds.
In `@rmcs_ws/src/rmcs_msgs/include/rmcs_msgs/camera_frame_raw.hpp`:
- Around line 12-26: CameraFrameRaw is a large value-type (~1.5 MB) and can be
expensive to copy; update the design guidance around CameraFrameRaw to make its
intended ownership/ փոխանց usage explicit. In camera_frame_raw.hpp, add
documentation near CameraFrameRaw and kFrameSize stating it should not be passed
or stored by value and should use std::shared_ptr/std::unique_ptr instead, or
refactor the data member to a heap-backed std::vector<std::byte> if you want
move-friendly semantics.
In `@rmcs_ws/src/rmcs_utility/include/rmcs_utility/ring_buffer.hpp`:
- Around line 486-497: In ring_buffer::pop_front_until, avoid relying on
unsigned wraparound when computing the distance from pos to out_. Add an
explicit stale-iterator check using pos.origin_, pos.offset_, and out before
subtracting, so expired views return 0 directly; keep the existing bounds
validation and pop_front_n call unchanged otherwise.
- Around line 111-118: `BasicIterator` 的 `operator==` 和 `operator<=>`
比较语义不一致：前者检查 `buffer_`、`origin_`、`offset_`，后者只比较 `offset_`，会导致相等关系与三路比较契约不匹配。请在
`BasicIterator` 的比较运算符实现中统一比较范围，确保 `operator<=>` 与 `operator==` 对同一组字段给出一致结果；如果
`operator<=>` 仅打算用于同源迭代器，请在该处补充明确注释并避免让它看起来可用于跨视图比较。
🪄 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: ad38d6b8-35c6-4d98-89f6-2c89aa96968c
📒 Files selected for processing (108)
.script/template/entrypointrmcs_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/config/flight.yamlrmcs_ws/src/rmcs_bringup/config/mecanum-hero.yamlrmcs_ws/src/rmcs_bringup/config/omni-infantry.yamlrmcs_ws/src/rmcs_bringup/config/sentry.yamlrmcs_ws/src/rmcs_bringup/config/steering-hero-little.yamlrmcs_ws/src/rmcs_bringup/config/steering-hero.yamlrmcs_ws/src/rmcs_bringup/config/steering-infantry.yamlrmcs_ws/src/rmcs_bringup/launch/rmcs.launch.pyrmcs_ws/src/rmcs_core/CMakeLists.txtrmcs_ws/src/rmcs_core/plugins.xmlrmcs_ws/src/rmcs_core/src/broadcaster/tf_broadcaster.cpprmcs_ws/src/rmcs_core/src/broadcaster/value_broadcaster.cpprmcs_ws/src/rmcs_core/src/controller/adrc/eso.hpprmcs_ws/src/rmcs_core/src/controller/adrc/nlesf.hpprmcs_ws/src/rmcs_core/src/controller/adrc/td.hpprmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/chassis_controller.cpprmcs_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_mode.hpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_omni_wheel_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_suspension.cpprmcs_ws/src/rmcs_core/src/controller/chassis/deformable_wheel_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/hero_chassis_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/hero_chassis_power_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/hero_steering_wheel_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/omni_wheel_controller.cpprmcs_ws/src/rmcs_core/src/controller/chassis/qcp_solver.hpprmcs_ws/src/rmcs_core/src/controller/chassis/steering_wheel_status.cpprmcs_ws/src/rmcs_core/src/controller/gimbal/deformable_infantry_gimbal_controller.cpprmcs_ws/src/rmcs_core/src/controller/gimbal/dual_yaw_controller.cpprmcs_ws/src/rmcs_core/src/controller/gimbal/hero_gimbal_controller.cpprmcs_ws/src/rmcs_core/src/controller/gimbal/player_viewer.cpprmcs_ws/src/rmcs_core/src/controller/gimbal/precise_two_axis_gimbal_solver.hpprmcs_ws/src/rmcs_core/src/controller/gimbal/simple_gimbal_controller.cpprmcs_ws/src/rmcs_core/src/controller/gimbal/two_axis_gimbal_solver.hpprmcs_ws/src/rmcs_core/src/controller/pid/friction_wheel_pid_recorder.cpprmcs_ws/src/rmcs_core/src/controller/pid/matrix_pid_calculator.hpprmcs_ws/src/rmcs_core/src/controller/shooting/bullet_feeder_controller_42mm.cpprmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/hero_friction_wheel_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/hero_heat_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpprmcs_ws/src/rmcs_core/src/filter/alpha_beta_angle_filter.hpprmcs_ws/src/rmcs_core/src/filter/imu_ekf.hpprmcs_ws/src/rmcs_core/src/filter/low_pass_filter.hpprmcs_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/benewake.hpprmcs_ws/src/rmcs_core/src/hardware/device/bmi088.hpprmcs_ws/src/rmcs_core/src/hardware/device/bmi088_ekf.hpprmcs_ws/src/rmcs_core/src/hardware/device/board_clock_lifter.hpprmcs_ws/src/rmcs_core/src/hardware/device/gy614.hpprmcs_ws/src/rmcs_core/src/hardware/device/lk_motor.hpprmcs_ws/src/rmcs_core/src/hardware/flight.cpprmcs_ws/src/rmcs_core/src/hardware/omni_infantry.cpprmcs_ws/src/rmcs_core/src/hardware/steering-hero-little.cpprmcs_ws/src/rmcs_core/src/referee/app/ui/deformable_infantry_ui.cpprmcs_ws/src/rmcs_core/src/referee/app/ui/flight.cpprmcs_ws/src/rmcs_core/src/referee/app/ui/hero.cpprmcs_ws/src/rmcs_core/src/referee/app/ui/infantry.cpprmcs_ws/src/rmcs_core/src/referee/app/ui/shape/cfs_scheduler.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/shape/red_black_tree.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/shape/remote_shape.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/shape/shape.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/rangefinder.hpprmcs_ws/src/rmcs_core/src/referee/app/ui/widget/status_ring.hpprmcs_ws/src/rmcs_core/src/referee/command.cpprmcs_ws/src/rmcs_core/src/referee/command/interaction/ui.cpprmcs_ws/src/rmcs_core/src/referee/frame.hpprmcs_ws/src/rmcs_core/src/referee/status.cpprmcs_ws/src/rmcs_core/src/referee/status/field.hpprmcs_ws/src/rmcs_description/include/rmcs_description/sentry_description.hpprmcs_ws/src/rmcs_description/include/rmcs_description/tf_description.hpprmcs_ws/src/rmcs_executor/include/rmcs_executor/component.hpprmcs_ws/src/rmcs_executor/package.xmlrmcs_ws/src/rmcs_executor/src/component.cpprmcs_ws/src/rmcs_executor/src/executor.hpprmcs_ws/src/rmcs_executor/src/main.cpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/board_clock.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/camera_frame_raw.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/chassis_mode.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/full_robot_id.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/game_stage.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/imu_snapshot.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/rmcs_msgs.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/robot_color.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/robot_id.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/serial_interface.hpprmcs_ws/src/rmcs_msgs/include/rmcs_msgs/shoot_condiction.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/atomic_futex.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/crc/dji_crc.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/double_buffer.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/fps_counter.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/memory_pool.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/package_receive.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/pooled_shared_factory.hpprmcs_ws/src/rmcs_utility/include/rmcs_utility/ring_buffer.hpp
💤 Files with no reviewable changes (5)
- rmcs_ws/src/rmcs_core/src/controller/chassis/deformable_wheel_controller.cpp
- rmcs_ws/src/rmcs_bringup/config/mecanum-hero.yaml
- rmcs_ws/src/rmcs_bringup/config/deformable-infantry-steering.yaml
- rmcs_ws/src/rmcs_core/src/hardware/deformable-infantry-steering.cpp
- rmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpp
Comment on lines
+176
to
+177
| if (complex_spin_active_) | ||
| next_mode = rmcs_msgs::ChassisMode::SPIN; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
complex_spin_active_ 从未被设为 true，complex spin 功能为死代码。
complex_spin_active_ 在 Line 353 初始化为 false，deactivate_complex_spin_()（Line 182-185）仅将其设为 false，代码中没有任何位置将其设为 true。因此：
- Line 176-177 的
if (complex_spin_active_)永远为false（死代码） - Line 259-267 的 complex spin 计时逻辑永远不会执行
complex_spin_elapsed_永远为 0
这表明 complex spin 功能尚未实现或被有意禁用但未清理死代码。是否需要我帮助实现激活逻辑，或者应该移除这些死代码？
Also applies to: 259-267, 353-353
🤖 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/chassis/deformable_mode.hpp` around
lines 176 - 177, `complex_spin_active_` is never set to true, so the complex
spin path in `deformable_mode.hpp` is unreachable and its timer state never
changes. Either implement the missing activation path by setting
`complex_spin_active_` from the relevant mode transition logic in
`deactivate_complex_spin_()`/the surrounding mode handlers, or remove the dead
`if (complex_spin_active_)` branch, the complex spin elapsed-time updates, and
the `complex_spin_active_`/`complex_spin_elapsed_` state if the feature is not
intended to exist.
Comment on lines
33
to
+36
| auto file = fmt::format("/robot_shoot/{}.log", ms); | ||
| log_stream_.open(file); | ||
|
|
||
| std::ofstream out_file("shoot_recorder"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
日志文件路径和错误处理存在问题。
/robot_shoot/{}.log是绝对路径，若目录不存在或无写权限则打开失败。log_stream_.open(file)后未检查是否成功打开，后续写入将静默失败。- Line 36 的
std::ofstream out_file("shoot_recorder")创建/截断文件但未写入任何内容即销毁。
🛡️ Proposed fix for file open validation
auto file = fmt::format("/robot_shoot/{}.log", ms);
log_stream_.open(file);
+ if (!log_stream_.is_open()) {
+ RCLCPP_ERROR(get_logger(), "Failed to open log file: %s", file.c_str());
+ }
- std::ofstream out_file("shoot_recorder");
RCLCPP_INFO(get_logger(), "ShootingRecorder initialized, log file: %s", file.c_str());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto file = fmt::format("/robot_shoot/{}.log", ms); | |
| log_stream_.open(file); | |
| std::ofstream out_file("shoot_recorder"); | |
| auto file = fmt::format("/robot_shoot/{}.log", ms); | |
| log_stream_.open(file); | |
| if (!log_stream_.is_open()) { | |
| RCLCPP_ERROR(get_logger(), "Failed to open log file: %s", file.c_str()); | |
| } | |
| RCLCPP_INFO(get_logger(), "ShootingRecorder initialized, log file: %s", file.c_str()); |
🤖 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/shooting_recorder.cpp` around
lines 33 - 36, The shooting recorder setup in shooting_recorder.cpp has unsafe
file handling: the log path built with fmt::format("/robot_shoot/{}.log", ms)
may fail if the directory is missing or unwritable, log_stream_.open(file) is
not validated, and the temporary std::ofstream out_file("shoot_recorder") is
created without being used. Update the recorder initialization around
log_stream_.open and the local out_file creation to use a writable, ensured
path, check the stream state immediately after opening, and either write
meaningful content to out_file or remove it if it is unnecessary.
Comment on lines
+56
to
91
| v = *shoot_timestamp_; | ||
|
|
||
| static constexpr size_t max_velocities_size = 1000; | ||
|
|
||
| velocities.push_back(*initial_speed_); | ||
| if (velocities.size() > max_velocities_size) { | ||
| velocities.erase(velocities.begin()); | ||
| } | ||
|
|
||
| auto log_text = std::string{}; | ||
| analysis3(); | ||
|
|
||
| auto log_text = std::string{}; | ||
| auto timestamp = timestamp_to_string(*shoot_timestamp_); | ||
|
|
||
| if (friction_wheel_count_ == 2) { | ||
| log_text = fmt::format( | ||
| "{},{},{},{},{}", timestamp, *initial_speed_, // | ||
| *friction_wheels_velocity_[0], *friction_wheels_velocity_[1], | ||
| *fractional_temperature_); | ||
| } else if (friction_wheel_count_ == 4) { | ||
| if (friction_wheel_count_ == 4) { | ||
| log_text = fmt::format( | ||
| "{},{},{},{},{},{},{}", timestamp, *initial_speed_, // | ||
| *friction_wheels_velocity_[0], *friction_wheels_velocity_[1], | ||
| *friction_wheels_velocity_[2], *friction_wheels_velocity_[3], | ||
| *fractional_temperature_); | ||
| "{},{},{:.3f},{:.3f},{:.3f},{:.3f},{:.3f},{:.3f},{:.3f}", *initial_speed_, | ||
| (int)velocities.size(), // | ||
| velocity_, excellence_rate_, pass_rate_, range_, range2_, velocity_max, | ||
| velocity_min); | ||
| } | ||
|
|
||
| log_stream_ << log_text << std::endl; | ||
| RCLCPP_INFO(get_logger(), "%s", log_text.c_str()); | ||
| RCLCPP_INFO( | ||
| get_logger(), "%s", | ||
| log_text.c_str()); // Record each shot's initial speed and statistics. | ||
|
|
||
| log_velocity = fmt::format("{:.3f}", *initial_speed_); | ||
| std::ofstream out_file("shoot_recorder", std::ios::app); | ||
| if (out_file.is_open()) { | ||
| out_file << log_velocity << std::endl; | ||
| out_file.close(); | ||
| } | ||
|
|
||
| last_shoot_timestamp_ = *shoot_timestamp_; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
存在大量死代码和低效文件操作。
以下问题需要关注：
- Line 56:
v = *shoot_timestamp_赋值后从未使用。 - Line 68:
timestamp变量计算后未用于log_text。 - Lines 84-88: 每次
update()都重新打开/关闭文件"shoot_recorder"，效率低下。 - Line 70-76:
friction_wheel_count_ != 4时log_text为空但仍写入日志。 - Lines 95, 101:
friction_velocities_和friction_wheels_velocity_声明后从未注册或使用。 - Lines 145-177, 179-214, 245-251:
analysis1()、analysis2()、GetTime()均为死代码。 - Lines 113-127:
velocity_、excellence_rate_等成员变量未初始化。
🤖 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/shooting_recorder.cpp` around
lines 56 - 91, `shooting_recorder`’s update/logging path contains unused
variables, empty log writes, and per-call file I/O that should be cleaned up.
Remove or use the dead assignments (`v`, `timestamp`) in the update flow, build
`log_text` only when `friction_wheel_count_ == 4` and skip both `log_stream_`
and `RCLCPP_INFO` when it is empty, and stop reopening `"shoot_recorder"` on
every update by reusing a persistent stream or buffering writes. Also remove or
wire up the unused `friction_velocities_` and `friction_wheels_velocity_`
members and delete the dead helper methods (`analysis1`, `analysis2`, `GetTime`)
if they are not referenced anywhere.
Comment on lines
+237
to
+242
| sort(velocities.begin(), velocities.end()); | ||
| velocity_max = velocities.back(); | ||
| velocity_min = velocities.front(); | ||
|
|
||
| range_ = velocities.back() - velocities.front(); | ||
| range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
analysis3() 在 velocities.size() == 1 时触发越界访问。
首次射击时，velocities 仅有一个元素。Line 242 执行 velocities[int(velocities.size() - 2)]，其中 velocities.size() - 2 因 size_t 无符号下溢变为 SIZE_MAX，转换为 int 后为负值，导致未定义行为。velocities[1] 同样越界。
对比 analysis1() 中的保护逻辑（Lines 156-160 有 size >= 3 检查），analysis3() 缺少相同保护。
🐛 Proposed fix for the out-of-bounds access
range_ = velocities.back() - velocities.front();
- range2_ = velocities[int(velocities.size() - 2)] - velocities[1];
+ if (velocities.size() >= 3) {
+ range2_ = velocities[int(velocities.size() - 2)] - velocities[1];
+ } else {
+ range2_ = range_;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sort(velocities.begin(), velocities.end()); | |
| velocity_max = velocities.back(); | |
| velocity_min = velocities.front(); | |
| range_ = velocities.back() - velocities.front(); | |
| range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; | |
| sort(velocities.begin(), velocities.end()); | |
| velocity_max = velocities.back(); | |
| velocity_min = velocities.front(); | |
| range_ = velocities.back() - velocities.front(); | |
| if (velocities.size() >= 3) { | |
| range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; | |
| } else { | |
| range2_ = range_; | |
| } |
🤖 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/shooting_recorder.cpp` around
lines 237 - 242, `analysis3()` can index past the end of `velocities` when there
is only one sample, so add the same minimum-size guard used by `analysis1()`
before computing `range2_`. In `shooting_recorder.cpp`, update the logic around
`analysis3()` to require at least 3 elements before reading `velocities[1]` and
`velocities[size - 2]`, and provide a safe fallback for smaller inputs. Use the
existing `velocities` handling and `range_`/`range2_` assignments as the place
to apply the check.
Comment on lines
+287
to
+292
| void update_bottom_yaw_tracking_lines() { | ||
| const bool ctrl_e_pressed = keyboard_->ctrl && keyboard_->e; | ||
| if (ctrl_e_pressed) { | ||
| bottom_yaw_tracking_enabled_ = !bottom_yaw_tracking_enabled_; | ||
| bottom_yaw_anchor_angle_rad_ = *bottom_yaw_angle_; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ctrl+E 采用电平触发切换，会在按住期间每帧反复翻转跟踪状态。
update_bottom_yaw_tracking_lines() 每次 update()（约 1000Hz）都会执行，ctrl_e_pressed 是电平判断。只要 Ctrl+E 持续按住，bottom_yaw_tracking_enabled_ 就会每帧取反、bottom_yaw_anchor_angle_rad_ 每帧被重新锚定，导致开关不可控地抖动、锚点无法稳定。应改为边沿触发（仅在“上一帧未按下→本帧按下”时翻转）。
注意 update_pitch_raw_angle_color() 会在本函数之前更新 last_keyboard_，因此此处需引入独立的上一帧状态用于边沿检测。
🐛 建议的边沿触发修正
- void update_bottom_yaw_tracking_lines() {
- const bool ctrl_e_pressed = keyboard_->ctrl && keyboard_->e;
- if (ctrl_e_pressed) {
- bottom_yaw_tracking_enabled_ = !bottom_yaw_tracking_enabled_;
- bottom_yaw_anchor_angle_rad_ = *bottom_yaw_angle_;
- }
+ void update_bottom_yaw_tracking_lines() {
+ const bool ctrl_e_pressed = keyboard_->ctrl && keyboard_->e;
+ if (ctrl_e_pressed && !last_bottom_yaw_ctrl_e_) {
+ bottom_yaw_tracking_enabled_ = !bottom_yaw_tracking_enabled_;
+ bottom_yaw_anchor_angle_rad_ = *bottom_yaw_angle_;
+ }
+ last_bottom_yaw_ctrl_e_ = ctrl_e_pressed;同时新增成员 bool last_bottom_yaw_ctrl_e_ = false;。
🤖 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/hero.cpp` around lines 287 - 292,
Ctrl+E toggling in update_bottom_yaw_tracking_lines() is level-triggered and
flips bottom_yaw_tracking_enabled_ every frame while held. Change this logic to
edge-triggered by tracking an independent previous-frame Ctrl+E state (for
example a new last_bottom_yaw_ctrl_e_ member) and only toggling when the key
transitions from not pressed to pressed; keep bottom_yaw_anchor_angle_rad_
updates tied to that single transition.
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 18 seconds. |
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
3 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.
本次变更围绕“执行器框架 + 可变形底盘/云台 + 射击链路 + 硬件/配置适配”做了大规模重构与模块化替换，核心包括：
rmcs_executor::Component/Executor的接口模型与依赖绑定机制，引入接口类型（Normal/Event）、事件输入/输出与更严格的注册/冲突校验；更新输出配对的类型映射结构，并调整组件生命周期/销毁与事件收尾逻辑。BoardClock、IMU 快照/相机原始帧结构、atomic_futex、MemoryPool、PooledSharedFactory、环形缓冲可读快照视图/迭代器等），并包含较多格式与常量微调。DeformableChassisModeManager统一管理底盘模式与姿态目标生成；DeformableSuspension作为悬架计算组件（外/内环 PID、激活/参考角、角度限幅、误差/轨迹输出等）；DeformableJointController支持可选的setpoint_velocity输入，速度可用时跳过 TD 生成逻辑；DeformableChassisController（旧的可变形轮控制/组件逻辑整体下线）。gimbal_controller/相关控制参数引入重力前馈、移除俯仰融合相关参数；HeroFrictionWheelController、HeroHeatController、PutterController、以及更新/重写的ShootingRecorder统计与记录流程；Flight、SteeringHeroLittle；ImuEkf、Bmi088Ekf、BoardClockLifter）并改造OmniInfantry的时间基与 EKF 更新流程；DeformableInfantryV2，并调整步兵/全向硬件链路的板卡生命周期与 TF/状态发布）。setpoint_velocity、以及控制器参数删改；rmcs.launch.py移除在robot配置auto.*时自动包含rmcs_auto_aim_v2的逻辑；flight/steering-hero/steering-hero-little等配置文件，并删除若干旧配置条目；.script/template/entrypoint：尝试读取并写入usbcore/usbfs_memory_mb（带容错 Warning 逻辑），其余启动保持不变。rmcs_core/plugins.xml：新增/替换底盘与射击链路插件注册，移除部分旧组件；