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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough添加完整 bringup 配置，重写插件清单，拆分 SteeringHero 为双底板，引入 ChassisClimberController 与 PutterController，增强射击记录与摩擦轮分析，并对若干控制器输入/输出与故障判定逻辑做出调整。 Changes
Sequence Diagram(s)sequenceDiagram
participant Putter as PutterController
participant Feeder as BulletFeeder42mm
participant Sensor as Sensors (photoelectric/angle)
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 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)
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
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.
rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp
Outdated
Show resolved
Hide resolved
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().
rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp
Outdated
Show resolved
Hide resolved
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.
There was a problem hiding this comment.
Actionable comments posted: 4
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/shooting/shooting_recorder.cpp (1)
53-130:⚠️ Potential issue | 🟠 Major
update()函数实际上不执行任何操作。整个
update()方法中的实际工作代码都被注释掉了。在switch语句之后，没有任何实际的日志记录或分析操作被执行。这使得整个组件变成了一个空壳。如果这是临时调试状态，建议：
- 如果组件不再需要，从组件列表中移除（当前已注释）
- 如果仍需使用，恢复核心功能代码
- 清理大量注释代码，保持代码库整洁
🤖 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 53 - 130, The update() method is effectively a no-op because all functional code has been commented out; restore or remove behavior depending on intent: if logging/analysis is required re-enable the core logic around update() (restore the logging/analysis blocks that build log_text, write to log_stream_/outFile, update last_shoot_timestamp_, and perform analysis3()/velocity tracking) and ensure the existing switch on log_mode_ (LogMode::TRIGGER/LogMode::TIMING using shoot_timestamp_, last_shoot_timestamp_, and log_count_) gates execution correctly; otherwise remove the dead code and unregister this component; also prune stale commented blocks to keep the file clean and maintain only necessary branches related to log_mode_, shoot_timestamp_, last_shoot_timestamp_, log_count_, and friction/velocity handling.
♻️ Duplicate comments (2)
rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp (2)
239-239:⚠️ Potential issue | 🟡 Minor
analysis2()中range2_计算缺少边界检查。与
analysis1()不同，analysis2()中访问velocities[1]和velocities[velocities.size() - 2]时没有检查velocities.size() >= 3，当数据量不足时会导致越界访问。注意：当前这些分析函数未被调用（第 106 行已注释），但如果将来启用需要修复。
🐛 建议修复
- range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; + if (velocities.size() >= 3) { + range2_ = velocities[int(velocities.size() - 2)] - velocities[1]; + } else { + range2_ = range_; + }🤖 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 computation of range2_ in analysis2() accesses velocities[1] and velocities[velocities.size() - 2] without verifying the vector has at least 3 elements; add a size guard mirroring analysis1() (e.g., check velocities.size() >= 3) at the start of analysis2() and handle the insufficient-data case by returning early or setting range2_ to a safe default before any indexed access to velocities.
281-281:⚠️ Potential issue | 🟡 Minor
analysis3()中同样缺少边界检查。与
analysis2()相同的问题，访问velocities[1]和velocities[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_; + }🤖 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 281, analysis3() accesses velocities[1] and velocities[velocities.size() - 2] without verifying the vector has enough elements; add a boundary check (similar to analysis2()) before computing range2_ so you only access those indices when velocities.size() >= 2 (or the same minimum size used in analysis2()), and handle the short-vector case by returning early or assigning a safe default to range2_; update the block around analysis3(), the velocities vector use, and the range2_ assignment to include this guard.
🧹 Nitpick comments (4)
rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml (1)
48-48:viewer_motor_zero_point参数可能已废弃。该参数已配置但在当前
steering-hero.cpp的TopBoard中没有看到对应的 viewer motor 配置。如果不再需要，建议移除以避免混淆。🤖 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 48, 配置文件中的 viewer_motor_zero_point 参数看起来已废弃，因为在 steering-hero.cpp 中的 TopBoard 实现找不到对应的 viewer motor 配置；请要么在 TopBoard（steering-hero.cpp）中补充并使用 viewer_motor_zero_point 的相关配置/变量引用（确保名字一致并在初始化/读取配置时使用），要么直接从 rmcs_bringup/config/steering-hero.yaml 中移除 viewer_motor_zero_point 条目以避免混淆，并同时更新任何文档或配置加载逻辑以反映该变更。rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp (2)
323-323: 死代码：putter_receive_callback函数已定义但从未被调用。该函数与
gpio_digital_read_result_callback功能重叠（都设置photoelectric_sensor_status_），但从未被调用。如果不需要，建议删除以避免混淆。🧹 建议删除
- void putter_receive_callback(bool status) { *photoelectric_sensor_status_ = status; } -🤖 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 323, The function putter_receive_callback currently duplicates gpio_digital_read_result_callback by setting photoelectric_sensor_status_ but is never invoked; either remove putter_receive_callback to eliminate dead code or, if intended, wire it into the appropriate registration/ISR where callbacks are set (ensure the registration uses putter_receive_callback and not gpio_digital_read_result_callback), and update any related comments/tests; search for putter_receive_callback and gpio_digital_read_result_callback in steering-hero.cpp to apply the chosen fix.
342-342: 未使用的成员变量photoelectric_sensor_status_atomic。
photoelectric_sensor_status_atomic声明后从未被使用，应删除以保持代码整洁。🧹 建议删除
OutputInterface<std::time_t> camera_capturer_trigger_timestamp_; - std::atomic<bool> photoelectric_sensor_status_atomic{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/hardware/steering-hero.cpp` at line 342, Remove the unused atomic member photoelectric_sensor_status_atomic from steering-hero.cpp by deleting its declaration (std::atomic<bool> photoelectric_sensor_status_atomic{false};) from the class/translation unit where it's declared (e.g., in the SteeringHero implementation), and ensure there are no remaining references; if it was intended for state tracking, replace usages with the existing sensor state variable instead.rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp (1)
152-166: 建议清理未使用的成员变量。以下成员变量声明后从未被实际使用（因为相关代码已注释）：
velocities（第 152 行）velocity_、excellence_rate_、pass_rate_、range_、range2_、velocity_min、velocity_max（第 154-163 行）v（第 165 行）log_velocity（第 148 行）如果功能已废弃，建议删除这些变量和相关的分析函数（
analysis1/2/3、GetTime）以保持代码整洁。🤖 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 152 - 166, 删除未使用的成员变量并清理相关已废弃分析函数：在 shooting_recorder.cpp 中移除成员声明 velocities、log_velocity、velocity_、excellence_rate_、pass_rate_、range_、range2_、velocity_min、velocity_max、v、aim_velocity，以及不再使用的分析函数 analysis1、analysis2、analysis3 和 GetTime；在删除前搜索工程确认这些符号没有被其他文件引用，并在删除后重建/运行单元测试以确保没有遗漏；若某些变量未来需保留，请添加明确的注释或把它们封装为可选/懒加载字段以避免未使用警告。
🤖 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 184-187: The shooting_recorder parameter block is missing the
required aim_velocity setting (referenced in shooting_recorder.cpp at startup),
so add an aim_velocity entry under the shooting_recorder -> ros__parameters
section (alongside friction_wheel_count and log_mode) with an appropriate
numeric value to prevent the node from throwing on startup when enabled; ensure
the key name is exactly aim_velocity to match the parameter lookup in
shooting_recorder.
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/putter_controller.cpp`:
- Around line 303-325: The jam-detection logic in update_putter_jam_detection
uses conditions tied only to PRELOADING and SHOOTING (and NaN), so when called
in other stages (e.g., PRELOADED or UPDATING) it can wrongly increment
putter_faulty_count_; modify update_putter_jam_detection to first filter by
shoot_stage_ (e.g., return/reset when shoot_stage_ is not one of the intended
monitoring states) or explicitly add conditions for PRELOADED/UPDATING to reset
putter_faulty_count_, ensuring the checks around *putter_control_torque_* only
run when shoot_stage_ is PRELOADING or SHOOTING; update references to
putter_control_torque_, shoot_stage_, putter_faulty_count_, putter_initialized,
putter_startpoint, and shooted accordingly.
- Around line 51-52: The controller registers the input symbol bullet_fired_ via
register_input but no corresponding output is provided in the hardware layer, so
bullet_fired_ will never be ready and dereferencing it (e.g., the const bool
bullet_fired_now = *bullet_fired_; and the later if (*bullet_fired_ || ...)) is
unsafe; fix by either (A) adding a matching output registration for
"/gimbal/bullet_fired" in the hardware module (the steering-hero.cpp hardware
layer) so bullet_fired_ becomes ready, or (B) make the input optional and guard
all uses with bullet_fired_.ready() (e.g., replace direct dereferences with
bullet_fired_.ready() ? *bullet_fired_ : false) to avoid undefined behavior.
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp`:
- Around line 169-177: The TopBoard/steering_hero currently registers outputs
like "/gimbal/photoelectric_sensor" but is missing registration for
"/gimbal/bullet_fired", so add a call to
steering_hero.register_output("/gimbal/bullet_fired", bullet_fired_, <default>)
alongside the other register_output calls; also declare the member variable
bullet_fired_ (matching the type used by putter_controller's register_input) in
the class and ensure any firing callback or ISR updates bullet_fired_
appropriately so the value is published to downstream subscribers.
---
Outside diff comments:
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Around line 53-130: The update() method is effectively a no-op because all
functional code has been commented out; restore or remove behavior depending on
intent: if logging/analysis is required re-enable the core logic around update()
(restore the logging/analysis blocks that build log_text, write to
log_stream_/outFile, update last_shoot_timestamp_, and perform
analysis3()/velocity tracking) and ensure the existing switch on log_mode_
(LogMode::TRIGGER/LogMode::TIMING using shoot_timestamp_, last_shoot_timestamp_,
and log_count_) gates execution correctly; otherwise remove the dead code and
unregister this component; also prune stale commented blocks to keep the file
clean and maintain only necessary branches related to log_mode_,
shoot_timestamp_, last_shoot_timestamp_, log_count_, and friction/velocity
handling.
---
Duplicate comments:
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Line 239: The computation of range2_ in analysis2() accesses velocities[1] and
velocities[velocities.size() - 2] without verifying the vector has at least 3
elements; add a size guard mirroring analysis1() (e.g., check velocities.size()
>= 3) at the start of analysis2() and handle the insufficient-data case by
returning early or setting range2_ to a safe default before any indexed access
to velocities.
- Line 281: analysis3() accesses velocities[1] and velocities[velocities.size()
- 2] without verifying the vector has enough elements; add a boundary check
(similar to analysis2()) before computing range2_ so you only access those
indices when velocities.size() >= 2 (or the same minimum size used in
analysis2()), and handle the short-vector case by returning early or assigning a
safe default to range2_; update the block around analysis3(), the velocities
vector use, and the range2_ assignment to include this guard.
---
Nitpick comments:
In `@rmcs_ws/src/rmcs_bringup/config/steering-hero.yaml`:
- Line 48: 配置文件中的 viewer_motor_zero_point 参数看起来已废弃，因为在 steering-hero.cpp 中的
TopBoard 实现找不到对应的 viewer motor 配置；请要么在 TopBoard（steering-hero.cpp）中补充并使用
viewer_motor_zero_point 的相关配置/变量引用（确保名字一致并在初始化/读取配置时使用），要么直接从
rmcs_bringup/config/steering-hero.yaml 中移除 viewer_motor_zero_point
条目以避免混淆，并同时更新任何文档或配置加载逻辑以反映该变更。
In `@rmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpp`:
- Around line 152-166: 删除未使用的成员变量并清理相关已废弃分析函数：在 shooting_recorder.cpp 中移除成员声明
velocities、log_velocity、velocity_、excellence_rate_、pass_rate_、range_、range2_、velocity_min、velocity_max、v、aim_velocity，以及不再使用的分析函数
analysis1、analysis2、analysis3 和
GetTime；在删除前搜索工程确认这些符号没有被其他文件引用，并在删除后重建/运行单元测试以确保没有遗漏；若某些变量未来需保留，请添加明确的注释或把它们封装为可选/懒加载字段以避免未使用警告。
In `@rmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp`:
- Line 323: The function putter_receive_callback currently duplicates
gpio_digital_read_result_callback by setting photoelectric_sensor_status_ but is
never invoked; either remove putter_receive_callback to eliminate dead code or,
if intended, wire it into the appropriate registration/ISR where callbacks are
set (ensure the registration uses putter_receive_callback and not
gpio_digital_read_result_callback), and update any related comments/tests;
search for putter_receive_callback and gpio_digital_read_result_callback in
steering-hero.cpp to apply the chosen fix.
- Line 342: Remove the unused atomic member photoelectric_sensor_status_atomic
from steering-hero.cpp by deleting its declaration (std::atomic<bool>
photoelectric_sensor_status_atomic{false};) from the class/translation unit
where it's declared (e.g., in the SteeringHero implementation), and ensure there
are no remaining references; if it was intended for state tracking, replace
usages with the existing sensor state variable instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d75cc73-1bb5-4cf3-a89e-d86ea71ac7a0
📒 Files selected for processing (8)
librmcs-firmware-c_board-3.0.1-0.dev.4.gbaf538b.dfurmcs_ws/src/rmcs_bringup/config/steering-hero.yamlrmcs_ws/src/rmcs_core/CMakeLists.txtrmcs_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/shooting/putter_controller.cpprmcs_ws/src/rmcs_core/src/controller/shooting/shooting_recorder.cpprmcs_ws/src/rmcs_core/src/hardware/steering-hero.cpp
✅ Files skipped from review due to trivial changes (1)
- rmcs_ws/src/rmcs_core/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- rmcs_ws/src/rmcs_core/src/controller/gimbal/hero_gimbal_controller.cpp
Comment on lines
+51
to
+52
| register_input("/gimbal/photoelectric_sensor", photoelectric_sensor_status_); | ||
| register_input("/gimbal/bullet_fired", bullet_fired_); |
There was a problem hiding this comment.
严重：/gimbal/bullet_fired 输入无数据源。
该控制器注册了 /gimbal/bullet_fired 输入，但在 steering-hero.cpp 的硬件层中没有注册对应的输出。这会导致：
bullet_fired_输入永远不会就绪- 第 105 行
const bool bullet_fired_now = *bullet_fired_;解引用未就绪的输入可能导致未定义行为 - 第 182-184 行的射击检测逻辑
if (*bullet_fired_ || ...)将无法正常工作
需要在 steering-hero.cpp 中添加对应的输出注册，或者将此输入设置为可选并添加就绪检查。
🔧 临时缓解方案：将输入设为可选并添加检查
- register_input("/gimbal/bullet_fired", bullet_fired_);
+ register_input("/gimbal/bullet_fired", bullet_fired_, false);然后在使用前检查就绪状态：
const bool bullet_fired_now = bullet_fired_.ready() ? *bullet_fired_ : 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/putter_controller.cpp` around
lines 51 - 52, The controller registers the input symbol bullet_fired_ via
register_input but no corresponding output is provided in the hardware layer, so
bullet_fired_ will never be ready and dereferencing it (e.g., the const bool
bullet_fired_now = *bullet_fired_; and the later if (*bullet_fired_ || ...)) is
unsafe; fix by either (A) adding a matching output registration for
"/gimbal/bullet_fired" in the hardware module (the steering-hero.cpp hardware
layer) so bullet_fired_ becomes ready, or (B) make the input optional and guard
all uses with bullet_fired_.ready() (e.g., replace direct dereferences with
bullet_fired_.ready() ? *bullet_fired_ : false) to avoid undefined behavior.
Comment on lines
+303
to
+325
| void update_putter_jam_detection() { | ||
| if ((*putter_control_torque_ > -0.03 && shoot_stage_ == ShootStage::PRELOADING) | ||
| || (*putter_control_torque_ < 0.05 && shoot_stage_ == ShootStage::SHOOTING) | ||
| || std::isnan(*putter_control_torque_)) { | ||
| putter_faulty_count_ = 0; | ||
| return; | ||
| } | ||
|
|
||
| // 扭矩异常时累计故障计数 | ||
| if (putter_faulty_count_ < 500) | ||
| ++putter_faulty_count_; | ||
| else { | ||
| putter_faulty_count_ = 0; | ||
| if (shoot_stage_ != ShootStage::SHOOTING) { | ||
| // 非发射状态下检测到堵转：推杆已到位，设置为已初始化 | ||
| putter_initialized = true; | ||
| putter_startpoint = *putter_angle_; | ||
| } else { | ||
| // 发射状态下检测到堵转：认为子弹已发出 | ||
| shooted = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
推杆卡弹检测条件可能存在逻辑问题。
if ((*putter_control_torque_ > -0.03 && shoot_stage_ == ShootStage::PRELOADING)
|| (*putter_control_torque_ < 0.05 && shoot_stage_ == ShootStage::SHOOTING)
|| std::isnan(*putter_control_torque_)) {
putter_faulty_count_ = 0;
return;
}此函数在两个地方被调用：
- 第 187 行：在
SHOOTING状态下 - 第 218 行：在推杆未初始化时（此时
shoot_stage_可能是PRELOADED）
当 shoot_stage_ 为 PRELOADED 或 UPDATING 时，前两个条件都不满足，只有 NaN 检查生效。如果 putter_control_torque_ 不是 NaN，可能会错误地累计故障计数。
建议明确处理所有状态或添加状态过滤。
🔧 建议修复
void update_putter_jam_detection() {
+ // 只在推杆初始化阶段和发射阶段检测卡弹
+ if (shoot_stage_ != ShootStage::PRELOADING && shoot_stage_ != ShootStage::SHOOTING
+ && putter_initialized) {
+ putter_faulty_count_ = 0;
+ return;
+ }
+
if ((*putter_control_torque_ > -0.03 && shoot_stage_ == ShootStage::PRELOADING)🤖 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 303 - 325, The jam-detection logic in update_putter_jam_detection uses
conditions tied only to PRELOADING and SHOOTING (and NaN), so when called in
other stages (e.g., PRELOADED or UPDATING) it can wrongly increment
putter_faulty_count_; modify update_putter_jam_detection to first filter by
shoot_stage_ (e.g., return/reset when shoot_stage_ is not one of the intended
monitoring states) or explicitly add conditions for PRELOADED/UPDATING to reset
putter_faulty_count_, ensuring the checks around *putter_control_torque_* only
run when shoot_stage_ is PRELOADING or SHOOTING; update references to
putter_control_torque_, shoot_stage_, putter_faulty_count_, putter_initialized,
putter_startpoint, and shooted accordingly.
Comment on lines
+169
to
+177
| steering_hero.register_output("/gimbal/yaw/velocity_imu", gimbal_yaw_velocity_imu_); | ||
| steering_hero.register_output("/gimbal/pitch/velocity_imu", gimbal_pitch_velocity_imu_); | ||
|
|
||
| steering_hero.register_output( | ||
| "/gimbal/photoelectric_sensor", photoelectric_sensor_status_, false); | ||
| steering_hero.register_output( | ||
| "/auto_aim/image_capturer/timestamp", camera_capturer_trigger_timestamp_, 0); | ||
| steering_hero.register_output( | ||
| "/auto_aim/image_capturer/trigger", camera_capturer_trigger_, 0); |
There was a problem hiding this comment.
严重：缺少 /gimbal/bullet_fired 输出注册，下游控制器无法获取数据。
根据 putter_controller.cpp:51-52，控制器订阅了 /gimbal/bullet_fired 输入：
register_input("/gimbal/bullet_fired", bullet_fired_);但在 TopBoard 中只注册了 /gimbal/photoelectric_sensor，没有注册 /gimbal/bullet_fired。这会导致 putter_controller 中的 bullet_fired_ 输入永远无法就绪，相关射击检测逻辑将失效。
🔧 建议修复：添加 bullet_fired 输出注册
steering_hero.register_output(
"/gimbal/photoelectric_sensor", photoelectric_sensor_status_, false);
+ steering_hero.register_output("/gimbal/bullet_fired", bullet_fired_, false);
steering_hero.register_output(
"/auto_aim/image_capturer/timestamp", camera_capturer_trigger_timestamp_, 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/hardware/steering-hero.cpp` around lines 169 - 177,
The TopBoard/steering_hero currently registers outputs like
"/gimbal/photoelectric_sensor" but is missing registration for
"/gimbal/bullet_fired", so add a call to
steering_hero.register_output("/gimbal/bullet_fired", bullet_fired_, <default>)
alongside the other register_output calls; also declare the member variable
bullet_fired_ (matching the type used by putter_controller's register_input) in
the class and ensure any firing callback or ISR updates bullet_fired_
appropriately so the value is published to downstream subscribers.
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.
PR 摘要：合并 merge-steering-hero 到 main
概述
将 merge-steering-hero 分支的广泛改动合并到主分支。改动包含大量配置新增、plugins.xml 重写、SteeringHero 硬件架构从单板到双底板的大幅重构、并新增/修改多个控制器与记录器实现与参数，以支持舵轮英雄（Steering Hero）硬件与功能需求（双底板、爬升、发射、记录与自动瞄准等）。
关键变更
风险与审查建议
建议的合并前动作
推荐重点审查文件