Open
Conversation
Co-authored-by: zhzy-star <2807406212@qq.com> Co-authored-by: floatpigeon <floatpigeon@proton.me> Co-authored-by: fan <enbofan663@gmail.com>
Co-authored-by: zhzy-star <2807406212@qq.com> Co-authored-by: floatpigeon <floatpigeon@proton.me>
Walkthrough添加完整 bringup 配置，重写插件清单，拆分 SteeringHero 为双底板，引入 ChassisClimberController 与 PutterController，增强射击记录与摩擦轮分析，并对若干控制器输入/输出和故障判定逻辑做出调整（均为源码与配置变更，无运行脚本新增）。 Changes
Sequence Diagram(s)sequenceDiagram
participant Putter as PutterController
participant Feeder as BulletFeeder42mm
participant Sensor as Photoelectric/Angle/Sensors
participant Motor as Motors (putter/feeder/friction)
participant Logger as ShootingRecorder
Putter->>Sensor: 读取预加载/角度/速度/光电信号
Putter->>Feeder: 下发角度/速度设定与进给命令
Feeder->>Motor: 输出角度/速度控制指令
Motor->>Sensor: 返回速度/角度/触发信号
Sensor->>Putter: 报告预加载/已发射/堵转/`bullet_fired`
Putter->>Putter: 状态机切换（PRELOADING → PRELOADED → SHOOTING）
Putter->>Logger: 记录射击事件与速度数据
Logger->>Putter: 分析结果/冷却/保护触发反馈
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp (2)
63-63: 拼写错误：puttter应为putter。变量名
puttter_return_velocity_pid_包含三个 't'，应修正为putter_return_velocity_pid_。此拼写错误出现在多处。Also applies to: 360-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp` at line 63, The variable name puttter_return_velocity_pid_ is misspelled (extra 't')—rename all occurrences of puttter_return_velocity_pid_ to putter_return_velocity_pid_ (including declarations, assignments like puttter_return_velocity_pid_.kp = 0.0015;, uses in methods, header files, and any related initialization or references) so the identifier is consistent across the codebase; update both definition and all references and adjust any related comments or tests to match the corrected symbol putter_return_velocity_pid_.
328-328: 存在多个未使用的成员变量。以下变量已声明但从未使用：
max_bullet_feeder_control_torque_（第 328 行）last_photoelectric_sensor_status_（第 332 行）putter_velocity_pid_（第 363 行）这可能表示实现不完整或是遗留代码。
Also applies to: 332-332, 363-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp` at line 328, The three members max_bullet_feeder_control_torque_, last_photoelectric_sensor_status_, and putter_velocity_pid_ are declared but never used; either remove them or wire them into the PutterController implementation: locate the PutterController class in putter_controller.cpp, and either (A) delete these unused member declarations and any unused includes/tests that reference them, or (B) integrate them by (1) using max_bullet_feeder_control_torque_ to clamp feeder actuation in the feed/drive routine (e.g., feed torque limiter), (2) updating last_photoelectric_sensor_status_ where photoelectric sensor events are processed to debounce/track state changes, and (3) applying putter_velocity_pid_ inside the velocity control path (e.g., Update/PutterControl or velocity loop) to compute motor commands; ensure related constructors/initializers are updated and unit tests/usage are adjusted accordingly.rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp (1)
501-502:BottomBoard_one中存在未使用的输出接口。
gimbal_yaw_velocity_imu_和gimbal_pitch_velocity_imu_在BottomBoard_one中声明但从未注册或使用。这些可能是从旧代码复制过来的遗留成员。♻️ 建议移除未使用的成员
OutputInterface<double> chassis_yaw_velocity_imu_; - OutputInterface<double> gimbal_yaw_velocity_imu_; - OutputInterface<double> gimbal_pitch_velocity_imu_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp` around lines 501 - 502, BottomBoard_one declares unused output members gimbal_yaw_velocity_imu_ and gimbal_pitch_velocity_imu_; remove these dead members from the class to clean up legacy code or, if they were intended to be used, register and emit them where other OutputInterface members are handled (search for other OutputInterface registrations in BottomBoard_one to mirror). Ensure you update constructor/initializer lists and any headers so there are no leftover references to gimbal_yaw_velocity_imu_ or gimbal_pitch_velocity_imu_.rmcs_ws/src/rmcs_core/plugins.xml (2)
86-88: XML 缩进不一致。第 86 行的
<class>标签缺少前导空格，与其他条目不一致。♻️ 建议修复缩进
- <class type="rmcs_core::referee::Status" base_class_type="rmcs_executor::Component"> + <class type="rmcs_core::referee::Status" base_class_type="rmcs_executor::Component">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/plugins.xml` around lines 86 - 88, The <class> element with type="rmcs_core::referee::Status" is mis-indented; update the XML so the <class> tag at that location uses the same leading-space indentation as the surrounding entries (add the missing leading space(s) before the <class> tag) to make the file indentation consistent with other <class> entries in plugins.xml.
2-127: 大多数插件使用通用的 "Test plugin." 描述。建议为每个插件提供更具描述性的说明，以便于理解各组件的功能。例如：
ChassisController→ "底盘运动控制器"HeroGimbalController→ "英雄机器人云台控制器"PutterController→ "推弹机构控制器"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/plugins.xml` around lines 2 - 127, Many classes in plugins.xml use the generic description "Test plugin." which is unhelpful; update each <class>'s <description> to a concise, meaningful Chinese (or English as appropriate) description reflecting its role — e.g., change rmcs_core::controller::chassis::ChassisController to "底盘运动控制器", rmcs_core::controller::gimbal::HeroGimbalController to "英雄机器人云台控制器", rmcs_core::controller::shooting::PutterController to "推弹机构控制器", and similarly replace other generic descriptions such as for rmcs_core::broadcaster::TfBroadcaster, rmcs_core::broadcaster::ValueBroadcaster, rmcs_core::hardware::* and rmcs_core::referee::* with brief domain-specific descriptions so each <class type="..."> entry has a clear, informative <description>.rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml (1)
239-239: 视频路径使用了硬编码的绝对路径。
video_path: "/workspaces/RMCS/rmcs_ws/resources/1.avi"是一个环境特定的绝对路径。建议使用相对路径或通过环境变量/参数配置。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml` at line 239, The video_path entry currently uses an environment-specific absolute path (video_path: "/workspaces/RMCS/rmcs_ws/resources/1.avi"); change it to a portable configuration by using a relative path (e.g., "resources/1.avi") or read from an environment variable/launch parameter (e.g., VIDEO_PATH) and reference that here, so update the video_path key to pull from the env/parameter or a relative location instead of a hardcoded absolute path.rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp (2)
182-275:analysis1()、analysis2()和analysis3()存在大量重复代码。三个分析函数有大量相似的逻辑（求和、排序、计算范围、计算通过率）。建议重构为一个带参数的通用函数。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp` around lines 182 - 275, analysis1(), analysis2(), and analysis3() duplicate summation, sorting, min/max/range calculation and pass/excellence rate logic; refactor by extracting a single parameterized helper (e.g., ComputeVelocityStats or analyzeVelocities) that accepts parameters/flags for behaviors that differ (trimming fraction for analysis2, target-based comparison using aim_velocity for analysis3, or center-on-mean for analysis1), operates on the velocities container and writes velocity_, velocity_max, velocity_min, range_, range2_, excellence_rate_, pass_rate_; call this helper from analysis1/2/3 with the appropriate options so sorting, sum, trimming, rate calculation, and range computations are implemented only once and reuse velocities, aim_velocity, and the existing member variables.
117-122: 每次更新都打开文件效率低下。每次
update()调用时都打开和关闭"shoot_recorder"文件。对于 1000Hz 的更新率，这会产生显著的 I/O 开销。建议在构造函数中打开文件并保持打开状态，或使用缓冲写入。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp` around lines 117 - 122, The code repeatedly opens and closes "shoot_recorder" inside update(), causing heavy I/O at 1000Hz; instead add a std::ofstream member (e.g., out_file_ or shoot_recorder_stream_) to the ShootingRecorder class, open it once in the constructor (check for open failure), write formatted log_velocity to that stream inside update(), and close/flush it in the destructor (or periodically flush to limit disk writes). Also ensure thread-safety if update() can be called concurrently and handle failures gracefully when opening the file.rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp (2)
190-190: 建议使用整数类型表示方向。
back_climber_dir_类型为double，但仅用作方向乘数（-1 或 1）。建议使用int类型以更清晰地表达其用途。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` at line 190, back_climber_dir_ is declared as double but used only as a direction multiplier (-1 or 1); change its type to int (e.g., int back_climber_dir_ = 1) to better express intent and avoid floating math semantics. Update all references/usages of back_climber_dir_ (multiplications, assignments, toggles) to work with an int (add explicit casts only where a floating-point value is required, e.g., static_cast<double>(back_climber_dir_) in expressions that expect double), and ensure any functions or members that previously required a double are adjusted to accept int or receive the cast; search for symbol back_climber_dir_ to locate all sites to update.
94-94: 冗余的条件检查。第 94 行的
if (switch_left != Switch::DOWN)检查是冗余的，因为第 75 行的else if (switch_left != Switch::DOWN)已经确保了这个条件为真。♻️ 建议简化
- if (switch_left != Switch::DOWN) { - back_climber_timer_++; + back_climber_timer_++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` at line 94, Remove the redundant conditional check of switch_left against Switch::DOWN: because the earlier branch already used "else if (switch_left != Switch::DOWN)" the later "if (switch_left != Switch::DOWN)" is unnecessary—locate the second occurrence in chassis_climber_controller.cpp (the block referencing switch_left and Switch::DOWN) and either delete that redundant if (keeping its inner logic merged into the surrounding control flow) or convert the surrounding branching so the condition is evaluated only once, ensuring no duplicated guards remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml`:
- Around line 183-186: The YAML defines parameters under shooting_recorder but
never registers the component, so those params are unused; add a components
entry mapping rmcs_core::controller::shooting::ShootingRecorder to the node name
shooting_recorder in the top-level components list (the same list that contains
other components between lines 4-37) so the ShootingRecorder is instantiated and
picks up friction_wheel_count and log_mode.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Around line 155-158: dual_motor_sync_control is declared const but calls
pid_calculator.update(), which mutates the PID state and causes a compile error;
remove the const qualifier from the member function declaration/definition for
dual_motor_sync_control so it can call pid_calculator.update(), update the
corresponding declaration in the class header, and adjust any call sites that
relied on the method being const.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp`:
- Around line 130-132: The if-statement has its heat-limit check commented out
so the controller ignores heat-based bullet allowance; restore the condition by
re-enabling the boolean expression involving
*control_bullet_allowance_limited_by_heat_ (i.e. change the if to require
*control_bullet_allowance_limited_by_heat_ > 0 && shoot_stage_ ==
ShootStage::PRELOADED), ensure the pointer is null-checked if necessary before
dereferencing, and run unit/manual test of the branch in putter_controller.cpp
to verify behavior.
- Around line 63-73: The PID integral limits for puttter_return_velocity_pid_
are accidentally overwritten due to a copy-paste bug; change the last two
assignments that currently set puttter_return_velocity_pid_.integral_max and
puttter_return_velocity_pid_.integral_min to instead set
putter_velocity_pid_.integral_max and putter_velocity_pid_.integral_min so that
puttter_return_velocity_pid_ keeps its intended integral bounds and
putter_velocity_pid_ receives its correct limits (refer to the symbols
puttter_return_velocity_pid_ and putter_velocity_pid_ in the
constructor/initializer block).
- Around line 182-192: The current control flow in the shooted branch can
overwrite the zeroing of *putter_control_torque_: when shooted is true you
compute angle_err and may set *putter_control_torque_ = 0. and call
set_preloading(), but immediately afterward you always assign
*putter_control_torque_ = puttter_return_velocity_pid_.update(...), undoing the
zero; change the logic in the shooted block (using an else, early
return/continue, or conditional) so that when angle_err > -0.05 you set
*putter_control_torque_ = 0., call set_preloading(), set shooted = false and do
not execute the subsequent puttter_return_velocity_pid_.update(...) assignment;
keep the rest of the branch behavior unchanged.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Line 192: The calculation of range2_ uses velocities[1] and
velocities[velocities.size() - 2] which is unsafe when velocities.size() < 3; in
analysis1(), analysis2(), and analysis3() guard this computation by checking
velocities.size() >= 3 before performing range2_ =
velocities[int(velocities.size() - 2)] - velocities[1]; if the size check fails,
either set range2_ to a safe default (e.g., 0) or early-return/skip the analysis
path so no out-of-bounds access occurs; update all occurrences that compute
range2_ to follow this pattern (reference range2_, velocities,
analysis1/analysis2/analysis3).
- Line 99: The velocities vector is never cleared and pushes *initial_speed_ on
every shot, causing unbounded growth; update the code paths that call
velocities.push_back(*initial_speed_) (in shooting_recorder.cpp) to enforce a
fixed-capacity buffer: introduce a MAX_VELOCITIES constant and after pushing, if
velocities.size() > MAX_VELOCITIES remove the oldest entries (or replace
std::vector with std::deque and pop_front, or use a circular buffer
implementation) so the container never grows indefinitely; apply the same change
to both places where velocities.push_back(*initial_speed_) is used.
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml`:
- Line 239: The video_path entry currently uses an environment-specific absolute
path (video_path: "/workspaces/RMCS/rmcs_ws/resources/1.avi"); change it to a
portable configuration by using a relative path (e.g., "resources/1.avi") or
read from an environment variable/launch parameter (e.g., VIDEO_PATH) and
reference that here, so update the video_path key to pull from the env/parameter
or a relative location instead of a hardcoded absolute path.
In `@rmcs_ws/src/rmcs_core/plugins.xml`:
- Around line 86-88: The <class> element with type="rmcs_core::referee::Status"
is mis-indented; update the XML so the <class> tag at that location uses the
same leading-space indentation as the surrounding entries (add the missing
leading space(s) before the <class> tag) to make the file indentation consistent
with other <class> entries in plugins.xml.
- Around line 2-127: Many classes in plugins.xml use the generic description
"Test plugin." which is unhelpful; update each <class>'s <description> to a
concise, meaningful Chinese (or English as appropriate) description reflecting
its role — e.g., change rmcs_core::controller::chassis::ChassisController to
"底盘运动控制器", rmcs_core::controller::gimbal::HeroGimbalController to "英雄机器人云台控制器",
rmcs_core::controller::shooting::PutterController to "推弹机构控制器", and similarly
replace other generic descriptions such as for
rmcs_core::broadcaster::TfBroadcaster, rmcs_core::broadcaster::ValueBroadcaster,
rmcs_core::hardware::* and rmcs_core::referee::* with brief domain-specific
descriptions so each <class type="..."> entry has a clear, informative
<description>.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Line 190: back_climber_dir_ is declared as double but used only as a direction
multiplier (-1 or 1); change its type to int (e.g., int back_climber_dir_ = 1)
to better express intent and avoid floating math semantics. Update all
references/usages of back_climber_dir_ (multiplications, assignments, toggles)
to work with an int (add explicit casts only where a floating-point value is
required, e.g., static_cast<double>(back_climber_dir_) in expressions that
expect double), and ensure any functions or members that previously required a
double are adjusted to accept int or receive the cast; search for symbol
back_climber_dir_ to locate all sites to update.
- Line 94: Remove the redundant conditional check of switch_left against
Switch::DOWN: because the earlier branch already used "else if (switch_left !=
Switch::DOWN)" the later "if (switch_left != Switch::DOWN)" is
unnecessary—locate the second occurrence in chassis_climber_controller.cpp (the
block referencing switch_left and Switch::DOWN) and either delete that redundant
if (keeping its inner logic merged into the surrounding control flow) or convert
the surrounding branching so the condition is evaluated only once, ensuring no
duplicated guards remain.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp`:
- Line 63: The variable name puttter_return_velocity_pid_ is misspelled (extra
't')—rename all occurrences of puttter_return_velocity_pid_ to
putter_return_velocity_pid_ (including declarations, assignments like
puttter_return_velocity_pid_.kp = 0.0015;, uses in methods, header files, and
any related initialization or references) so the identifier is consistent across
the codebase; update both definition and all references and adjust any related
comments or tests to match the corrected symbol putter_return_velocity_pid_.
- Line 328: The three members max_bullet_feeder_control_torque_,
last_photoelectric_sensor_status_, and putter_velocity_pid_ are declared but
never used; either remove them or wire them into the PutterController
implementation: locate the PutterController class in putter_controller.cpp, and
either (A) delete these unused member declarations and any unused includes/tests
that reference them, or (B) integrate them by (1) using
max_bullet_feeder_control_torque_ to clamp feeder actuation in the feed/drive
routine (e.g., feed torque limiter), (2) updating
last_photoelectric_sensor_status_ where photoelectric sensor events are
processed to debounce/track state changes, and (3) applying putter_velocity_pid_
inside the velocity control path (e.g., Update/PutterControl or velocity loop)
to compute motor commands; ensure related constructors/initializers are updated
and unit tests/usage are adjusted accordingly.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Around line 182-275: analysis1(), analysis2(), and analysis3() duplicate
summation, sorting, min/max/range calculation and pass/excellence rate logic;
refactor by extracting a single parameterized helper (e.g., ComputeVelocityStats
or analyzeVelocities) that accepts parameters/flags for behaviors that differ
(trimming fraction for analysis2, target-based comparison using aim_velocity for
analysis3, or center-on-mean for analysis1), operates on the velocities
container and writes velocity_, velocity_max, velocity_min, range_, range2_,
excellence_rate_, pass_rate_; call this helper from analysis1/2/3 with the
appropriate options so sorting, sum, trimming, rate calculation, and range
computations are implemented only once and reuse velocities, aim_velocity, and
the existing member variables.
- Around line 117-122: The code repeatedly opens and closes "shoot_recorder"
inside update(), causing heavy I/O at 1000Hz; instead add a std::ofstream member
(e.g., out_file_ or shoot_recorder_stream_) to the ShootingRecorder class, open
it once in the constructor (check for open failure), write formatted
log_velocity to that stream inside update(), and close/flush it in the
destructor (or periodically flush to limit disk writes). Also ensure
thread-safety if update() can be called concurrently and handle failures
gracefully when opening the file.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp`:
- Around line 501-502: BottomBoard_one declares unused output members
gimbal_yaw_velocity_imu_ and gimbal_pitch_velocity_imu_; remove these dead
members from the class to clean up legacy code or, if they were intended to be
used, register and emit them where other OutputInterface members are handled
(search for other OutputInterface registrations in BottomBoard_one to mirror).
Ensure you update constructor/initializer lists and any headers so there are no
leftover references to gimbal_yaw_velocity_imu_ or gimbal_pitch_velocity_imu_.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 472351ea-4503-44a6-818c-b4810e13f662
📒 Files selected for processing (9)
rmcs_ws/src/rmcs_bringup/config/steering-hero.yamlrmcs_ws/src/rmcs_core/plugins.xmlrmcs_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/gimbal/dual_yaw_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/bullet_feeder_controller_42mm.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/hardware/steering-hero.cpp
💤 Files with no reviewable changes (1)
- rmcs_ws/src/rmcs_core/src/controller/gimbal/dual_yaw_controller.cpp
Comment on lines
+183
to
+186
| shooting_recorder: | ||
| ros__parameters: | ||
| friction_wheel_count: 2 | ||
| log_mode: 2 # 1: trigger, 2: timing |
There was a problem hiding this comment.
shooting_recorder 参数已配置但组件未注册。
shooting_recorder 节点在第 183-186 行配置了参数，但在第 4-37 行的 components 列表中没有注册 rmcs_core::controller::shooting::ShootingRecorder。这些参数将不会被使用。
如果需要使用 ShootingRecorder，请添加组件注册：
- rmcs_core::controller::shooting::ShootingRecorder -> shooting_recorder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml` around lines 183 - 186,
The YAML defines parameters under shooting_recorder but never registers the
component, so those params are unused; add a components entry mapping
rmcs_core::controller::shooting::ShootingRecorder to the node name
shooting_recorder in the top-level components list (the same list that contains
other components between lines 4-37) so the ShootingRecorder is instantiated and
picks up friction_wheel_count and log_mode.
rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp
Outdated
Show resolved
Hide resolved
rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+130
to
+132
| if ( | ||
| // *control_bullet_allowance_limited_by_heat_ > 0 && | ||
| shoot_stage_ == ShootStage::PRELOADED) |
There was a problem hiding this comment.
热量限制检查被注释掉。
if (
// *control_bullet_allowance_limited_by_heat_ > 0 &&
shoot_stage_ == ShootStage::PRELOADED)热量限制检查（*control_bullet_allowance_limited_by_heat_ > 0）被注释掉，这意味着控制器将忽略热量限制。这在比赛中可能导致违规。请确认这是调试目的还是需要恢复此检查。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp` around
lines 130 - 132, The if-statement has its heat-limit check commented out so the
controller ignores heat-based bullet allowance; restore the condition by
re-enabling the boolean expression involving
*control_bullet_allowance_limited_by_heat_ (i.e. change the if to require
*control_bullet_allowance_limited_by_heat_ > 0 && shoot_stage_ ==
ShootStage::PRELOADED), ensure the pointer is null-checked if necessary before
dereferencing, and run unit/manual test of the branch in putter_controller.cpp
to verify behavior.
Comment on lines
+182
to
+192
| if (shooted) { | ||
| // 子弹已发出：推杆复位 | ||
| const auto angle_err = putter_startpoint - *putter_angle_; | ||
| if (angle_err > -0.05) { | ||
| *putter_control_torque_ = 0.; | ||
| set_preloading(); | ||
| shooted = false; | ||
| } | ||
|
|
||
| *putter_control_torque_ = | ||
| puttter_return_velocity_pid_.update(-80. - *putter_velocity_); |
There was a problem hiding this comment.
控制流逻辑可能导致重复赋值。
在第 182-192 行，当 shooted 为 true 时：
- 第 186 行可能设置
*putter_control_torque_ = 0. - 第 191-192 行随后又会覆盖此值
如果 angle_err > -0.05，第 186 行的赋值会立即被第 191-192 行覆盖。
🔧 建议修复
if (shooted) {
// 子弹已发出：推杆复位
const auto angle_err = putter_startpoint - *putter_angle_;
if (angle_err > -0.05) {
*putter_control_torque_ = 0.;
set_preloading();
shooted = false;
+ } else {
+ *putter_control_torque_ =
+ puttter_return_velocity_pid_.update(-80. - *putter_velocity_);
}
-
- *putter_control_torque_ =
- puttter_return_velocity_pid_.update(-80. - *putter_velocity_);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp` around
lines 182 - 192, The current control flow in the shooted branch can overwrite
the zeroing of *putter_control_torque_: when shooted is true you compute
angle_err and may set *putter_control_torque_ = 0. and call set_preloading(),
but immediately afterward you always assign *putter_control_torque_ =
puttter_return_velocity_pid_.update(...), undoing the zero; change the logic in
the shooted block (using an else, early return/continue, or conditional) so that
when angle_err > -0.05 you set *putter_control_torque_ = 0., call
set_preloading(), set shooted = false and do not execute the subsequent
puttter_return_velocity_pid_.update(...) assignment; keep the rest of the branch
behavior unchanged.
rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp (1)
239-239:⚠️ Potential issue | 🔴 Critical
range2_计算仍有越界访问风险（样本不足 3 条时）。当前实现仍直接访问
velocities[1]和velocities[size-2]。当样本数过少会越界。该问题与历史评论重复，当前版本仍存在。🐛 建议修复
- range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; + if (velocities.size() >= 3) { + range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; + } else { + range2_ = range_; + } @@ - range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; + if (velocities.size() >= 3) { + range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; + } else { + range2_ = range_; + }Also applies to: 281-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp` at line 239, The calculation assigning range2_ directly from velocities[int(velocities.size() - 2)] and velocities[1] is unsafe when velocities has fewer than 3 elements; update the code around the range2_ assignment (and the similar use near the other occurrence) to first check the container size (e.g., size_t n = velocities.size()) and only compute range2_ = velocities[n-2] - velocities[1] when n >= 3, otherwise set range2_ to a safe default (0 or NAN) or skip the computation/early-return so no out-of-bounds access occurs; apply the same guarded check to the other identical spot mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Around line 72-75: The branch where switch_left == Switch::DOWN currently
falls through without updating outputs, causing stale control commands; update
the branch so when switch_left == Switch::DOWN you explicitly clear or zero
actuator commands (e.g., call reset_all_controls() or set the torque/command
outputs to zero) instead of doing nothing; apply the same fix at the other
occurrence referenced (the block around the second occurrence) so both places
handle Switch::DOWN by resetting outputs rather than leaving previous commands
active.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpp`:
- Around line 153-156: The fault-detection logic in detect_friction_faulty() is
inverted: the loop currently returns false when a wheel's actual velocity falls
below half the control target, which prevents any fault from being reported;
change the inner return to return true so that when
abs(*friction_velocities_[i]) < abs(*friction_control_velocities_[i] * 0.5) the
function reports a fault, leaving the final return false as the no-fault case;
adjust any related callers that increment or reset friction_faulty_count_ if
they assume the original inverted meaning.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp`:
- Around line 211-239: reset_all_controls() currently leaves critical firing
state uninitialized: restore shoot_stage_ to a safe startup state (e.g.,
ShootStage::PRELOADING) and clear the shot-completed flag (shooted) so old shot
state cannot carry over after re-enable; update the function to assign
shoot_stage_ = ShootStage::PRELOADING (or the appropriate initial enum) and set
shooted = false alongside the other resets (refer to shoot_stage_ and shooted
symbols).
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Line 88: The if condition reads uninitialized local variable v (declared as v
in shooting_recorder.cpp) before it is assigned; initialize v at declaration
(e.g., set a safe default) or move/perform the assignment to v before the check
so the expression "if (*shoot_timestamp_ == last_shoot_timestamp_ || v ==
*shoot_timestamp_)" never reads an uninitialized value; update the variable v
(and any related logic around shoot_timestamp_ and last_shoot_timestamp_) to
ensure v is assigned before use.
- Line 24: aim_velocity is read directly via
get_parameter("aim_velocity").as_double() which will throw if the parameter is
absent; change the initialization to provide a safe default by either calling
declare_parameter("aim_velocity", <default_double>) and then
get_parameter("aim_velocity").as_double(), or check
node->has_parameter("aim_velocity") (or equivalent has_parameter) and fall back
to a chosen default before assigning to the aim_velocity variable; update the
initialization code in shooting_recorder.cpp where aim_velocity is set to use
one of these safe patterns and ensure the value is converted with as_double().
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp`:
- Around line 219-221: 当前逻辑只在时间戳变化时将 *camera_capturer_trigger_ 置为
true，未变化时不复位导致触发信号变为常高；在检查 last_camera_capturer_trigger_timestamp_ 与
*camera_capturer_trigger_timestamp_ 的分支中增加 else 分支在未变化时将
*camera_capturer_trigger_ 置为 false，之后仍更新
last_camera_capturer_trigger_timestamp_，以恢复“边沿触发”语义；相关符号：last_camera_capturer_trigger_timestamp_,
camera_capturer_trigger_timestamp_, camera_capturer_trigger_。
---
Duplicate comments:
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Line 239: The calculation assigning range2_ directly from
velocities[int(velocities.size() - 2)] and velocities[1] is unsafe when
velocities has fewer than 3 elements; update the code around the range2_
assignment (and the similar use near the other occurrence) to first check the
container size (e.g., size_t n = velocities.size()) and only compute range2_ =
velocities[n-2] - velocities[1] when n >= 3, otherwise set range2_ to a safe
default (0 or NAN) or skip the computation/early-return so no out-of-bounds
access occurs; apply the same guarded check to the other identical spot
mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a53cca6-93fc-45d3-9e62-675d03494731
📒 Files selected for processing (6)
rmcs_ws/src/rmcs_bringup/config/steering-hero.yamlrmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_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/hardware/steering-hero.cpp
rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+153
to
156
| if (abs(*friction_velocities_[i]) < abs(*friction_control_velocities_[i] * 0.5)) | ||
| return false; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
故障检测被完全短路，卡弹保护永远不会触发。
Line 153-156 在命中阈值时返回 false，函数末尾也返回 false，导致 detect_friction_faulty() 恒为 false，friction_faulty_count_ 相关保护逻辑失效。
🐛 建议修复
bool detect_friction_faulty() {
for (size_t i = 0; i < friction_count_; i++) {
- if (abs(*friction_velocities_[i]) < abs(*friction_control_velocities_[i] * 0.5))
- return false;
+ if (std::abs(*friction_velocities_[i]) < std::abs(*friction_control_velocities_[i] * 0.5))
+ return true;
}
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/friction_wheel_controller.cpp`
around lines 153 - 156, The fault-detection logic in detect_friction_faulty() is
inverted: the loop currently returns false when a wheel's actual velocity falls
below half the control target, which prevents any fault from being reported;
change the inner return to return true so that when
abs(*friction_velocities_[i]) < abs(*friction_control_velocities_[i] * 0.5) the
function reports a fault, leaving the final return false as the no-fault case;
adjust any related callers that increment or reset friction_faulty_count_ if
they assume the original inverted meaning.
Comment on lines
+211
to
+239
| void reset_all_controls() { | ||
| last_switch_right_ = rmcs_msgs::Switch::UNKNOWN; | ||
| last_switch_left_ = rmcs_msgs::Switch::UNKNOWN; | ||
| last_mouse_ = rmcs_msgs::Mouse::zero(); | ||
| last_keyboard_ = rmcs_msgs::Keyboard::zero(); | ||
|
|
||
| overdrive_mode_ = false; | ||
|
|
||
| bullet_feeder_control_angle_ = nan_; | ||
| bullet_feeder_angle_pid_.output_max = inf_; | ||
| bullet_feeder_velocity_pid_.reset(); | ||
| bullet_feeder_angle_pid_.reset(); | ||
| *bullet_feeder_control_torque_ = nan_; | ||
|
|
||
| // shoot_stage_ = ShootStage::PRELOADING; | ||
|
|
||
| putter_initialized = false; | ||
| putter_startpoint = nan_; | ||
| putter_return_velocity_pid_.reset(); | ||
| putter_velocity_pid_.reset(); | ||
| putter_return_angle_pid.reset(); | ||
| *putter_control_torque_ = nan_; | ||
|
|
||
| last_preload_flag_ = false; | ||
| last_photoelectric_sensor_status_ = false; | ||
|
|
||
| bullet_feeder_faulty_count_ = 0; | ||
| bullet_feeder_cool_down_ = 0; | ||
| } |
There was a problem hiding this comment.
复位流程没有清空关键状态，状态机会残留旧拍。
reset_all_controls() 未恢复 shoot_stage_（Line 225 被注释）且未重置 shooted，在重新使能后会带入旧状态，影响发射流程稳定性。
🔧 建议修复
void reset_all_controls() {
@@
- // shoot_stage_ = ShootStage::PRELOADING;
+ shoot_stage_ = ShootStage::PRELOADING;
+ shooted = false;
@@
bullet_feeder_faulty_count_ = 0;
bullet_feeder_cool_down_ = 0;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp` around
lines 211 - 239, reset_all_controls() currently leaves critical firing state
uninitialized: restore shoot_stage_ to a safe startup state (e.g.,
ShootStage::PRELOADING) and clear the shot-completed flag (shooted) so old shot
state cannot carry over after re-enable; update the function to assign
shoot_stage_ = ShootStage::PRELOADING (or the appropriate initial enum) and set
shooted = false alongside the other resets (refer to shoot_stage_ and shooted
symbols).
|
|
||
| log_mode_ = static_cast<LogMode>(get_parameter("log_mode").as_int()); | ||
|
|
||
| aim_velocity = get_parameter("aim_velocity").as_double(); |
There was a problem hiding this comment.
aim_velocity 缺少兜底读取，组件启用后可能启动失败。
Line 24 直接 get_parameter("aim_velocity").as_double()，当前配置里未提供该参数时会在初始化阶段失败。
🔧 建议修复
- aim_velocity = get_parameter("aim_velocity").as_double();
+ if (!get_parameter("aim_velocity", aim_velocity)) {
+ aim_velocity = 0.0;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp` at line
24, aim_velocity is read directly via get_parameter("aim_velocity").as_double()
which will throw if the parameter is absent; change the initialization to
provide a safe default by either calling declare_parameter("aim_velocity",
<default_double>) and then get_parameter("aim_velocity").as_double(), or check
node->has_parameter("aim_velocity") (or equivalent has_parameter) and fall back
to a chosen default before assigning to the aim_velocity variable; update the
initialization code in shooting_recorder.cpp where aim_velocity is set to use
one of these safe patterns and ensure the value is converted with as_double().
| case LogMode::TRIGGER: | ||
| // It will be triggered by shooting action | ||
| if (*shoot_timestamp_ == last_shoot_timestamp_) | ||
| if (*shoot_timestamp_ == last_shoot_timestamp_ || v == *shoot_timestamp_) |
There was a problem hiding this comment.
读取未初始化成员 v，首次循环即存在未定义行为。
Line 88 在赋值前读取 v，而 Line 165 声明未初始化。
🐛 建议修复
- double v;
+ double v = 0.0;Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp` at line
88, The if condition reads uninitialized local variable v (declared as v in
shooting_recorder.cpp) before it is assigned; initialize v at declaration (e.g.,
set a safe default) or move/perform the assignment to v before the check so the
expression "if (*shoot_timestamp_ == last_shoot_timestamp_ || v ==
*shoot_timestamp_)" never reads an uninitialized value; update the variable v
(and any related logic around shoot_timestamp_ and last_shoot_timestamp_) to
ensure v is assigned before use.
Comment on lines
+219
to
+221
| if (last_camera_capturer_trigger_timestamp_ != *camera_capturer_trigger_timestamp_) | ||
| *camera_capturer_trigger_ = true; | ||
| last_camera_capturer_trigger_timestamp_ = *camera_capturer_trigger_timestamp_; |
There was a problem hiding this comment.
相机触发信号未复位，会变成常高电平。
Line 219-221 只在时间戳变化时置 true，没有在未变化时回落为 false，触发语义从“边沿”变成“电平”，下游会被持续触发。
🔧 建议修复
- if (last_camera_capturer_trigger_timestamp_ != *camera_capturer_trigger_timestamp_)
- *camera_capturer_trigger_ = true;
+ *camera_capturer_trigger_ =
+ (last_camera_capturer_trigger_timestamp_ != *camera_capturer_trigger_timestamp_);
last_camera_capturer_trigger_timestamp_ = *camera_capturer_trigger_timestamp_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp` around lines 219 - 221,
当前逻辑只在时间戳变化时将 *camera_capturer_trigger_ 置为 true，未变化时不复位导致触发信号变为常高；在检查
last_camera_capturer_trigger_timestamp_ 与 *camera_capturer_trigger_timestamp_
的分支中增加 else 分支在未变化时将 *camera_capturer_trigger_ 置为 false，之后仍更新
last_camera_capturer_trigger_timestamp_，以恢复“边沿触发”语义；相关符号：last_camera_capturer_trigger_timestamp_,
camera_capturer_trigger_timestamp_, camera_capturer_trigger_。
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp (1)
231-291:⚠️ Potential issue | 🟠 Major当
switch_left == DOWN且不满足首个条件时，控制输出未更新。当
switch_left == DOWN && switch_right != DOWN && auto_climb_state_ == IDLE时，代码不会进入任何分支，导致控制扭矩和climbing_forward_velocity_保持上一周期的值，可能造成非预期的持续输出。🐛 建议修复：添加 else 分支
} } + } else { + // switch_left == DOWN && switch_right != DOWN && auto_climb_state_ == IDLE + *climber_front_left_control_torque_ = 0.0; + *climber_front_right_control_torque_ = 0.0; + *climber_back_left_control_torque_ = 0.0; + *climber_back_right_control_torque_ = 0.0; + *climbing_forward_velocity_ = nan_; } last_switch_left_ = switch_left;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` around lines 231 - 291, The branch for switch_left == Switch::DOWN is missing so outputs can persist; add an else block paired with the existing if (switch_left != Switch::DOWN) to explicitly clear outputs when the left switch is DOWN: set *climbing_forward_velocity_ = nan_, set *climber_front_left_control_torque_, *climber_front_right_control_torque_, *climber_back_left_control_torque_, and *climber_back_right_control_torque_ to 0.0, and optionally reset back_climber_timer_ and back_climber_block_count_ so dual_motor_sync_control (front/back) and back-climber state do not continue from prior values (place this else immediately after the block that calls dual_motor_sync_control and handles back_climber_* logic).
🧹 Nitpick comments (6)
rmcs_ws/src/rmcs_core/src/controller/gimbal/hero_gimbal_controller.cpp (1)
68-70: 行为变更：移除了硬件开关强制切换云台模式的功能。此更改移除了通过
switch_right_UP 位置强制进入 ENCODER 模式的逻辑，现在云台模式完全由键盘 Q 键控制。
注释代码清理：建议删除注释掉的旧代码（68-69行），或添加注释说明保留原因（如临时禁用待后续恢复）。
验证意图：请确认此行为变更是预期的，操作员不再能通过遥控器右拨杆直接覆盖云台模式。
♻️ 建议：清理注释代码
- // *gimbal_mode_ = - // *switch_right_ == Switch::UP ? GimbalMode::ENCODER : gimbal_mode_keyboard_; *gimbal_mode_ = gimbal_mode_keyboard_;,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/hero_gimbal_controller.cpp` around lines 68 - 70, The change removed the hardware override that forced ENCODER mode when switch_right_ == Switch::UP by replacing the conditional assignment with always using gimbal_mode_keyboard_; either (a) delete the commented-out lines referencing switch_right_, Switch::UP and GimbalMode::ENCODER to clean up dead code, or (b) replace them with a short explanatory comment near the assignment to gimbal_mode_ (mentioning switch_right_, Switch::UP, GimbalMode::ENCODER and gimbal_mode_keyboard_) stating this behavior is intentionally disabled; also confirm externally that operators can no longer override mode via the right switch if this is intended.rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp (2)
310-310:logger_成员变量在各 Board 类中未被使用。
TopBoard（第 310 行）、BottomBoard_one（第 499 行）和BottomBoard_two（第 661 行）都声明了logger_但从未使用。如果是为将来调试预留可以保留，否则建议移除以保持代码整洁。Also applies to: 499-499, 661-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp` at line 310, The member variable logger_ is declared but unused in the Board classes; remove the unused rclcpp::Logger logger_ declarations from TopBoard, BottomBoard_one, and BottomBoard_two (symbols: logger_, class names TopBoard, BottomBoard_one, BottomBoard_two) to clean up unused members, or if you intend to use it later, instead mark it as [[maybe_unused]] or add explicit usage (e.g., initialize and call logger_.get_name() in a constructor) so the compiler warnings disappear—pick one approach and apply consistently across the three classes.
704-706: 智能指针类型不一致：std::make_unique赋值给std::shared_ptr。成员声明为
std::shared_ptr，但在构造函数中使用std::make_unique创建对象（第 51-58 行）。虽然std::unique_ptr可以隐式转换为std::shared_ptr，但语义上不一致。如果不需要共享所有权，建议统一使用
std::unique_ptr：♻️ 建议修改
- std::shared_ptr<TopBoard> top_board_; - std::shared_ptr<BottomBoard_one> bottom_board_one_; - std::shared_ptr<BottomBoard_two> bottom_board_two_; + std::unique_ptr<TopBoard> top_board_; + std::unique_ptr<BottomBoard_one> bottom_board_one_; + std::unique_ptr<BottomBoard_two> bottom_board_two_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp` around lines 704 - 706, Members top_board_, bottom_board_one_, and bottom_board_two_ are declared as std::shared_ptr but constructed with std::make_unique; change the member types to std::unique_ptr to make ownership semantics consistent: update the declarations of top_board_, bottom_board_one_, and bottom_board_two_ to std::unique_ptr<TopBoard>, std::unique_ptr<BottomBoard_one>, and std::unique_ptr<BottomBoard_two>, keep the existing std::make_unique in the constructor, and adjust any uses that require shared ownership (if any) to accept unique_ptr or use raw/reference accessors instead.rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp (3)
251-251: 冗余条件检查。
if (switch_left != Switch::DOWN)位于else if (switch_left != Switch::DOWN)分支内部（第 231 行），此条件恒为真，可以移除。♻️ 建议修复
- if (switch_left != Switch::DOWN) { - back_climber_timer_++; + back_climber_timer_++; // ... rest of the code with one less indentation level - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` at line 251, Remove the redundant inner condition checking switch_left != Switch::DOWN inside the branch that is already guarded by else if (switch_left != Switch::DOWN); locate the block in chassis_climber_controller.cpp where switch_left is tested against Switch::DOWN and eliminate the nested if (switch_left != Switch::DOWN), promoting its body directly into the outer branch so the logic remains identical but without the unnecessary duplicate check.
22-40: 构造函数中参数获取缺少错误处理。
get_parameter()如果参数未声明或类型不匹配会抛出异常。虽然使用了automatically_declare_parameters_from_overrides(true)，但如果 YAML 配置文件缺少必需参数，运行时会直接崩溃而没有明确的错误提示。♻️ 建议：添加参数默认值或错误处理
- track_velocity_max_ = get_parameter("front_climber_velocity").as_double(); - climber_back_control_velocity_abs_ = get_parameter("back_climber_velocity").as_double(); - sync_coefficient_ = get_parameter("sync_coefficient").as_double(); - climb_timeout_limit_ = get_parameter("climb_timeout_limit").as_int(); + track_velocity_max_ = get_parameter_or("front_climber_velocity", rclcpp::ParameterValue(5.0)).as_double(); + climber_back_control_velocity_abs_ = get_parameter_or("back_climber_velocity", rclcpp::ParameterValue(3.0)).as_double(); + sync_coefficient_ = get_parameter_or("sync_coefficient", rclcpp::ParameterValue(0.5)).as_double(); + climb_timeout_limit_ = get_parameter_or("climb_timeout_limit", rclcpp::ParameterValue(5000)).as_int();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` around lines 22 - 40, The constructor ChassisClimberController currently calls get_parameter(...) directly for many params (front_kp, front_ki, front_kd, back_kp, back_ki, back_kd, front_climber_velocity, back_climber_velocity, sync_coefficient, climb_timeout_limit, back_climber_burst_velocity, back_climber_burst_duration) which can throw if missing or wrong type; update the constructor to either declare parameters with safe defaults using declare_parameter(...) before reading them or wrap get_parameter calls in checks/try-catch to handle exceptions, log a clear error via logger_ (e.g., "missing or invalid parameter <name>") and set sensible defaults or fail gracefully; ensure initialization of front_velocity_pid_calculator_ and back_velocity_pid_calculator_ uses validated values (or post-initialize them after parameter validation) so the PID constructors do not receive invalid data.
123-133: 多处魔法数字建议提取为可配置参数或命名常量。
0.48、0.35（目标俯仰角）、0.1（判断阈值）、50（确认计数）、500/3000/1000（定时器阈值）等硬编码值分散在代码各处，不便于调试和维护。建议将这些阈值提取为 ROS 参数或类常量，例如：
// 作为参数 target_pitch_first_step_ = get_parameter("target_pitch_first_step").as_double(); target_pitch_subsequent_ = get_parameter("target_pitch_subsequent").as_double(); // 或作为命名常量 static constexpr int kBlockConfirmTicks = 50; static constexpr int kDashTimeoutTicks = 3000;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` around lines 123 - 133, Extract the hard-coded "magic" numbers used in the climb logic (e.g. target pitch values 0.48 and 0.35, any thresholds like 0.1, confirm counts like 50, and throttle/timeout values 500/3000/1000) into named class constants or ROS parameters so they are configurable and self-documenting; update the FRONt_UP branch (symbols: auto_climb_state_, AutoClimbState::FRONT_UP, climb_count_, chassis_pitch_imu_, track_control_velocity, override_chassis_vx, RCLCPP_INFO_THROTTLE) to read target_pitch_first_step / target_pitch_subsequent (or kTargetPitchFirstStep, kTargetPitchSubsequent) and replace numeric throttle/timeout/count literals with descriptive names (e.g. kBlockConfirmTicks, kDashTimeoutTicks, kLogThrottleMs) or parameters retrieved via get_parameter(), ensuring defaults are set and used when constructing the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Around line 88-89: The line using RCLCPP_INFO to log *chassis_pitch_imu_
floods logs each cycle; locate the call to RCLCPP_INFO(logger_, "%lf",
*chassis_pitch_imu_) in chassis_climber_controller.cpp (around the loop/update
method) and either remove it entirely or replace it with
RCLCPP_INFO_THROTTLE(logger_, node_clock, period_seconds, "%lf",
*chassis_pitch_imu_) (choose an appropriate throttle period) so the pitch value
is logged at a controlled rate; ensure the static cast/debug line remains
commented or similarly throttled if needed.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp`:
- Around line 509-510: The class BottomBoard_one declares unused member
variables gimbal_yaw_velocity_imu_ and gimbal_pitch_velocity_imu_ that appear to
be leftovers from refactoring (the code actually registers/uses
chassis_yaw_velocity_imu_ and chassis_pitch_velocity_imu_); remove the unused
members gimbal_yaw_velocity_imu_ and gimbal_pitch_velocity_imu_ from
BottomBoard_one to clean up dead code and avoid confusion, ensuring no remaining
references to those symbols exist elsewhere in the class.
---
Duplicate comments:
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Around line 231-291: The branch for switch_left == Switch::DOWN is missing so
outputs can persist; add an else block paired with the existing if (switch_left
!= Switch::DOWN) to explicitly clear outputs when the left switch is DOWN: set
*climbing_forward_velocity_ = nan_, set *climber_front_left_control_torque_,
*climber_front_right_control_torque_, *climber_back_left_control_torque_, and
*climber_back_right_control_torque_ to 0.0, and optionally reset
back_climber_timer_ and back_climber_block_count_ so dual_motor_sync_control
(front/back) and back-climber state do not continue from prior values (place
this else immediately after the block that calls dual_motor_sync_control and
handles back_climber_* logic).
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Line 251: Remove the redundant inner condition checking switch_left !=
Switch::DOWN inside the branch that is already guarded by else if (switch_left
!= Switch::DOWN); locate the block in chassis_climber_controller.cpp where
switch_left is tested against Switch::DOWN and eliminate the nested if
(switch_left != Switch::DOWN), promoting its body directly into the outer branch
so the logic remains identical but without the unnecessary duplicate check.
- Around line 22-40: The constructor ChassisClimberController currently calls
get_parameter(...) directly for many params (front_kp, front_ki, front_kd,
back_kp, back_ki, back_kd, front_climber_velocity, back_climber_velocity,
sync_coefficient, climb_timeout_limit, back_climber_burst_velocity,
back_climber_burst_duration) which can throw if missing or wrong type; update
the constructor to either declare parameters with safe defaults using
declare_parameter(...) before reading them or wrap get_parameter calls in
checks/try-catch to handle exceptions, log a clear error via logger_ (e.g.,
"missing or invalid parameter <name>") and set sensible defaults or fail
gracefully; ensure initialization of front_velocity_pid_calculator_ and
back_velocity_pid_calculator_ uses validated values (or post-initialize them
after parameter validation) so the PID constructors do not receive invalid data.
- Around line 123-133: Extract the hard-coded "magic" numbers used in the climb
logic (e.g. target pitch values 0.48 and 0.35, any thresholds like 0.1, confirm
counts like 50, and throttle/timeout values 500/3000/1000) into named class
constants or ROS parameters so they are configurable and self-documenting;
update the FRONt_UP branch (symbols: auto_climb_state_,
AutoClimbState::FRONT_UP, climb_count_, chassis_pitch_imu_,
track_control_velocity, override_chassis_vx, RCLCPP_INFO_THROTTLE) to read
target_pitch_first_step / target_pitch_subsequent (or kTargetPitchFirstStep,
kTargetPitchSubsequent) and replace numeric throttle/timeout/count literals with
descriptive names (e.g. kBlockConfirmTicks, kDashTimeoutTicks, kLogThrottleMs)
or parameters retrieved via get_parameter(), ensuring defaults are set and used
when constructing the controller.
In `@rmcs_ws/src/rmcs_core/src/controller/gimbal/hero_gimbal_controller.cpp`:
- Around line 68-70: The change removed the hardware override that forced
ENCODER mode when switch_right_ == Switch::UP by replacing the conditional
assignment with always using gimbal_mode_keyboard_; either (a) delete the
commented-out lines referencing switch_right_, Switch::UP and
GimbalMode::ENCODER to clean up dead code, or (b) replace them with a short
explanatory comment near the assignment to gimbal_mode_ (mentioning
switch_right_, Switch::UP, GimbalMode::ENCODER and gimbal_mode_keyboard_)
stating this behavior is intentionally disabled; also confirm externally that
operators can no longer override mode via the right switch if this is intended.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp`:
- Line 310: The member variable logger_ is declared but unused in the Board
classes; remove the unused rclcpp::Logger logger_ declarations from TopBoard,
BottomBoard_one, and BottomBoard_two (symbols: logger_, class names TopBoard,
BottomBoard_one, BottomBoard_two) to clean up unused members, or if you intend
to use it later, instead mark it as [[maybe_unused]] or add explicit usage
(e.g., initialize and call logger_.get_name() in a constructor) so the compiler
warnings disappear—pick one approach and apply consistently across the three
classes.
- Around line 704-706: Members top_board_, bottom_board_one_, and
bottom_board_two_ are declared as std::shared_ptr but constructed with
std::make_unique; change the member types to std::unique_ptr to make ownership
semantics consistent: update the declarations of top_board_, bottom_board_one_,
and bottom_board_two_ to std::unique_ptr<TopBoard>,
std::unique_ptr<BottomBoard_one>, and std::unique_ptr<BottomBoard_two>, keep the
existing std::make_unique in the constructor, and adjust any uses that require
shared ownership (if any) to accept unique_ptr or use raw/reference accessors
instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ff981ff-b640-470f-bd5f-ad9a582c2910
📒 Files selected for processing (5)
rmcs_ws/src/rmcs_bringup/config/steering-hero.yamlrmcs_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/gimbal/hero_gimbal_controller.cpprmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml
rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+509
to
+510
| OutputInterface<double> gimbal_yaw_velocity_imu_; | ||
| OutputInterface<double> gimbal_pitch_velocity_imu_; |
There was a problem hiding this comment.
存在未使用的成员变量。
gimbal_yaw_velocity_imu_ 和 gimbal_pitch_velocity_imu_ 在 BottomBoard_one 中声明但从未使用。该类实际注册和使用的是 chassis_yaw_velocity_imu_ 和 chassis_pitch_imu_（第 389-392 行）。这些变量看起来是重构时遗留的代码。
🧹 建议删除
OutputInterface<double> chassis_yaw_velocity_imu_;
OutputInterface<double> chassis_pitch_imu_;
- OutputInterface<double> gimbal_yaw_velocity_imu_;
- OutputInterface<double> gimbal_pitch_velocity_imu_;
};📝 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
| OutputInterface<double> gimbal_yaw_velocity_imu_; | |
| OutputInterface<double> gimbal_pitch_velocity_imu_; | |
| OutputInterface<double> chassis_yaw_velocity_imu_; | |
| OutputInterface<double> chassis_pitch_imu_; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp` around lines 509 - 510,
The class BottomBoard_one declares unused member variables
gimbal_yaw_velocity_imu_ and gimbal_pitch_velocity_imu_ that appear to be
leftovers from refactoring (the code actually registers/uses
chassis_yaw_velocity_imu_ and chassis_pitch_velocity_imu_); remove the unused
members gimbal_yaw_velocity_imu_ and gimbal_pitch_velocity_imu_ from
BottomBoard_one to clean up dead code and avoid confusion, ensuring no remaining
references to those symbols exist elsewhere in the class.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp (1)
97-110:⚠️ Potential issue | 🟠 Major
switch_left == DOWN时仍可能遗留上一周期输出。当前分支在自动爬坡空闲且
switch_left == Switch::DOWN时既不手动更新也不清零输出，控制量会“粘住”上一次值，存在执行器持续输出风险。🔧 建议修复
- if (auto_climb_state_ != AutoClimbState::IDLE) { + if (auto_climb_state_ != AutoClimbState::IDLE) { apply_auto_climb_control(update_auto_climb_control()); } else if (switch_left != Switch::DOWN) { update_manual_control(); + } else { + reset_all_controls(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp` around lines 97 - 110, When auto climb is IDLE and switch_left == Switch::DOWN the controller neither updates manual control nor clears outputs, leaving actuators stuck; inside the conditional that currently calls update_manual_control() only when switch_left != Switch::DOWN, add a branch to explicitly clear outputs (call reset_all_controls()) when auto_climb_state_ == AutoClimbState::IDLE and switch_left == Switch::DOWN so the control signals are zeroed; locate this logic around the block referencing reset_all_controls(), handle_auto_climb_requests(...), apply_auto_climb_control(update_auto_climb_control()), update_manual_control(), and auto_climb_state_ to implement the change.
🧹 Nitpick comments (1)
rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml (1)
18-20: 禁用组件与参数块并存，建议收敛配置以避免误用当前
gimbal_player_viewer_controller、viewer_angle_pid_controller、auto_aim_controller的组件注册被注释，但参数块仍保留。建议二选一：要么启用组件，要么将对应参数迁移到示例/备用配置文件，避免维护者误判“已生效”。Also applies to: 35-36, 156-168, 240-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml` around lines 18 - 20, Several controller components (gimbal_player_viewer_controller, viewer_angle_pid_controller, auto_aim_controller) are commented out while their parameter blocks remain, which can mislead maintainers; either re-enable the component registrations or move the corresponding parameter sections into a separate example/disabled config file. Locate the commented component entries (references: gimbal_player_viewer_controller, viewer_angle_pid_controller, auto_aim_controller) and for each decide: if the component should be active, uncomment its registration and ensure its parameter block remains in-place; if it should remain disabled, cut the associated parameter blocks and paste them into a new example/disabled file (or wrap them in a clearly marked "disabled_examples" section) and remove them from the active config to avoid accidental use. Ensure consistency across the other mentioned blocks (around the ranges corresponding to lines 35-36, 156-168, 240-276) so no commented component has live parameters in the main config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Around line 318-320: back_climber_block_count_ only increments when
is_back_climber_blocked() is true and is never cleared, so transient stalls
accumulate and cause false “STALLED”; change the logic around
is_back_climber_blocked() in chassis_climber_controller.cpp to reset
back_climber_block_count_ to 0 when not blocked (or decrement/clip it) rather
than only incrementing, and ensure the stall detection uses this counter
compared against the configured threshold (and clamp to a max value to avoid
overflow).
- Around line 393-397: When setpoint is NaN the code currently returns after
setting left_torque_out/right_torque_out to nan_ but leaves the PID internal
state intact; modify the block handling std::isnan(setpoint) to also clear the
controller's PID state (reset integrator to 0, clear/update prev_error to 0 or
NaN, and reset any derivative/filter state or history buffers) in the enclosing
controller (e.g., the PID members used by the chassis climber controller) before
returning so that re-enabling control does not produce a transient jump.
---
Duplicate comments:
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`:
- Around line 97-110: When auto climb is IDLE and switch_left == Switch::DOWN
the controller neither updates manual control nor clears outputs, leaving
actuators stuck; inside the conditional that currently calls
update_manual_control() only when switch_left != Switch::DOWN, add a branch to
explicitly clear outputs (call reset_all_controls()) when auto_climb_state_ ==
AutoClimbState::IDLE and switch_left == Switch::DOWN so the control signals are
zeroed; locate this logic around the block referencing reset_all_controls(),
handle_auto_climb_requests(...),
apply_auto_climb_control(update_auto_climb_control()), update_manual_control(),
and auto_climb_state_ to implement the change.
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml`:
- Around line 18-20: Several controller components
(gimbal_player_viewer_controller, viewer_angle_pid_controller,
auto_aim_controller) are commented out while their parameter blocks remain,
which can mislead maintainers; either re-enable the component registrations or
move the corresponding parameter sections into a separate example/disabled
config file. Locate the commented component entries (references:
gimbal_player_viewer_controller, viewer_angle_pid_controller,
auto_aim_controller) and for each decide: if the component should be active,
uncomment its registration and ensure its parameter block remains in-place; if
it should remain disabled, cut the associated parameter blocks and paste them
into a new example/disabled file (or wrap them in a clearly marked
"disabled_examples" section) and remove them from the active config to avoid
accidental use. Ensure consistency across the other mentioned blocks (around the
ranges corresponding to lines 35-36, 156-168, 240-276) so no commented component
has live parameters in the main config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6497cafe-8c41-4012-9873-847128015521
📒 Files selected for processing (2)
rmcs_ws/src/rmcs_bringup/config/steering-hero.yamlrmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp
Comment on lines
+318
to
+320
| if (is_back_climber_blocked()) { | ||
| back_climber_block_count_++; | ||
| } |
There was a problem hiding this comment.
堵转计数未在恢复后清零，会累积成误判。
back_climber_block_count_ 只增不减，间歇性卡顿也会最终触发“STALLED”，导致过早停机。
🔧 建议修复
- if (is_back_climber_blocked()) {
- back_climber_block_count_++;
- }
+ if (is_back_climber_blocked()) {
+ back_climber_block_count_++;
+ } else {
+ back_climber_block_count_ = 0;
+ }📝 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
| if (is_back_climber_blocked()) { | |
| back_climber_block_count_++; | |
| } | |
| if (is_back_climber_blocked()) { | |
| back_climber_block_count_++; | |
| } else { | |
| back_climber_block_count_ = 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`
around lines 318 - 320, back_climber_block_count_ only increments when
is_back_climber_blocked() is true and is never cleared, so transient stalls
accumulate and cause false “STALLED”; change the logic around
is_back_climber_blocked() in chassis_climber_controller.cpp to reset
back_climber_block_count_ to 0 when not blocked (or decrement/clip it) rather
than only incrementing, and ensure the stall detection uses this counter
compared against the configured threshold (and clamp to a max value to avoid
overflow).
Comment on lines
+393
to
+397
| if (std::isnan(setpoint)) { | ||
| left_torque_out = nan_; | ||
| right_torque_out = nan_; | ||
| return; | ||
| } |
There was a problem hiding this comment.
禁用输出时未重置 PID 状态，重新使能可能出现突变。
setpoint 为 NaN 时直接返回会保留积分/微分历史，后续恢复控制时可能产生瞬时冲击。
🔧 建议修复
if (std::isnan(setpoint)) {
+ pid_calculator.reset();
left_torque_out = nan_;
right_torque_out = nan_;
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rmcs_ws/src/rmcs_core/src/controller/chassis/chassis_climber_controller.cpp`
around lines 393 - 397, When setpoint is NaN the code currently returns after
setting left_torque_out/right_torque_out to nan_ but leaves the PID internal
state intact; modify the block handling std::isnan(setpoint) to also clear the
controller's PID state (reset integrator to 0, clear/update prev_error to 0 or
NaN, and reset any derivative/filter state or history buffers) in the enclosing
controller (e.g., the PID members used by the chassis climber controller) before
returning so that re-enabling control does not produce a transient jump.
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 摘要：合并 merge-steering-hero 到 main
概述
将 steering-hero 分支的变更合并到主分支。改动范围广泛，包含大量配置新增、plugins.xml 重写、SteeringHero 硬件架构大幅重构、并新增/修改多个控制器与记录器的实现与参数。目标是支持“舵轮英雄（Steering Hero）”的双底板硬件架构、增强爬升与发射子系统能力、并提供更丰富的参数化控制与数据记录能力。
关键变更
风险与审查建议
影响范围与推荐动作