Closed
Conversation
wip: develop fsm
|
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该PR引入了完整的ROS 2 RMCS导航组件框架，包括CMake构建配置、C++组件实现、Python决策节点、导航栈配置、启动文件、工具库和测试基础设施，建立完整的导航子系统。 Changes
Sequence DiagramsequenceDiagram
participant User
participant LaunchSystem as Launch System
participant NavComponent as rmcs_navigation Component
participant Nav2Stack as Nav2 Stack
participant Planner as Planner Server
participant Controller as Controller Server
participant ROS2Topics as ROS 2 Topics/Actions
User->>LaunchSystem: Execute custom.launch.py
LaunchSystem->>NavComponent: Start Navigation Component
LaunchSystem->>Nav2Stack: Start Nav2 (bt_navigator, controller, planner)
NavComponent->>ROS2Topics: Subscribe /cmd_vel_smoothed
NavComponent->>ROS2Topics: Subscribe /{node_name}/command
NavComponent->>ROS2Topics: Expose /referee_status Service
User->>ROS2Topics: Publish /move_base_simple/goal (PoseStamped)
ROS2Topics->>NavComponent: Goal received
NavComponent->>Nav2Stack: Send NavigateToPose action goal
Nav2Stack->>Planner: Request path plan
Planner->>Nav2Stack: Return planned path
Nav2Stack->>Controller: Execute trajectory
Controller->>ROS2Topics: Publish /cmd_vel_smoothed
ROS2Topics->>NavComponent: Update cmd_vel_smoothed
NavComponent->>NavComponent: Monitor timeout (100ms)
NavComponent->>NavComponent: Update chassis/gimbal velocity
Controller->>Nav2Stack: Motion complete
Nav2Stack->>ROS2Topics: Emit action result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (14)
test/map.cc-1-4 (1)
1-4:⚠️ Potential issue | 🟠 Major缺少头文件
core/map/generator.hh将导致编译失败。该头文件在仓库中不存在，编译时会报错。建议：
- 添加缺失的头文件实现
src/core/map/generator.hh- 或移除此 include 直到实现完成
测试用例当前为空，建议添加
// TODO: implement test注释以明确后续工作。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/map.cc` around lines 1 - 4, The test includes a non-existent header "core/map/generator.hh" which breaks build; either add the missing header implementation (create src/core/map/generator.hh with the declarations/definitions needed) or remove the include from test/map.cc until the generator is implemented; also mark the empty test TEST(map, generate) with a comment like "// TODO: implement test" so intent is clear for future work.src/brain/plan.hh-19-23 (1)
19-23:⚠️ Potential issue | 🟠 Major潜在的数据竞争：
update_information缺少线程同步。根据
src/brain/plan.cc的上下文，information_()返回对共享Information成员的非 const 引用，而do_plan_()读取相同的字段。若update_information()从多线程调用（例如多线程执行器），会产生数据竞争。如果此组件保证在单线程执行器中运行，建议添加注释说明此约束。否则，考虑添加互斥锁保护：
🔒 示例：添加互斥锁保护
在
Impl中添加std::mutex，并在update_information中加锁：// In Impl struct std::mutex mtx; // In update_information template <std::invocable<Information&> F> auto update_information(F&& function) noexcept -> void { std::lock_guard lock(pimpl->mtx); // 需要通过 pimpl 访问 std::forward<F>(function)(information_()); do_plan_(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/brain/plan.hh` around lines 19 - 23, update_information currently mutates information_() then calls do_plan_() without synchronization which can cause races; either document that this component is single-thread-only or add mutex protection: add a std::mutex mtx to Impl (accessible via pimpl) and acquire a lock_guard in update_information before invoking the passed function and calling do_plan_; keep the lock scope spanning both the mutation (information_()) and the subsequent do_plan_() to prevent concurrent access.src/util/pimpl.hh-11-14 (1)
11-14:⚠️ Potential issue | 🟠 Major
noexcept构造函数与可能抛出异常的 Impl 不匹配。宏将
CLASS()和~CLASS()声明为noexcept，但根据src/brain/plan.cc:24-26，Impl::Impl()的noexcept构造函数内部执行std::unordered_map插入操作，这可能因哈希计算或内存分配而抛出异常。若std::make_unique<Impl>()抛出异常，将触发std::terminate。建议移除宏中构造函数的
noexcept限定，或确保所有Impl构造函数真正不抛出异常。🔧 建议修复
`#define` RMCS_PIMPL_DEFINITION(CLASS) \ public: \ - explicit CLASS() noexcept; \ + explicit CLASS(); \ ~CLASS() noexcept; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/pimpl.hh` around lines 11 - 14, The macro RMCS_PIMPL_DEFINITION currently marks the generated constructor and destructor as noexcept but Impl::Impl() (see Impl::Impl and its use via std::make_unique<Impl>()) can throw (e.g., unordered_map insert/alloc), so remove the noexcept from the CLASS() declaration in RMCS_PIMPL_DEFINITION (or alternatively ensure every Impl's constructor is noexcept and handles allocations safely); update the macro to declare explicit CLASS() without noexcept so std::make_unique<Impl>() exceptions propagate instead of causing std::terminate.src/util/fsm.hh-90-94 (1)
90-94:⚠️ Potential issue | 🟠 Major
spin_once()在状态未注册时会崩溃。如果
current_state对应的状态未通过use()注册，states_map.at(...)返回的unique_ptr为空，get()返回nullptr，随后调用current_event->on_begin()将导致空指针解引用。建议添加运行时检查或断言：
🛡️ 建议修复：添加空指针检查
auto spin_once() -> bool { if (current_event == nullptr) { - current_event = states_map.at(std::to_underlying(current_state)).get(); + auto& state_ptr = states_map.at(std::to_underlying(current_state)); + assert(state_ptr && "State handler not registered"); + current_event = state_ptr.get(); current_event->on_begin(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/fsm.hh` around lines 90 - 94, spin_once() currently dereferences a nullptr when current_state wasn't registered via use(); modify spin_once() to fetch the pointer from states_map (using states_map.at(std::to_underlying(current_state))) and check that the returned unique_ptr.get() is non-null before assigning to current_event and calling current_event->on_begin(); if the pointer is null, either assert/throw a clear runtime_error indicating the unregistered state or return false/handle gracefully so no null dereference occurs (refer to symbols: spin_once, current_event, states_map, current_state, use, on_begin).src/brain/rule.hh-11-14 (1)
11-14:⚠️ Potential issue | 🟠 MajorYAML 解析缺少错误处理，可能导致节点崩溃。
config["health"].as<std::uint16_t>()在键缺失或类型不匹配时会抛出YAML::BadConversion异常。由于调用链（brain.cc→PlanBox::set_rule→Rule构造函数）中没有 try-catch，任何 YAML 配置错误都会导致节点意外终止。建议添加错误处理或使用带默认值的
as<T>(default)方法：🛡️ 建议修复：添加带默认值的安全解析
explicit Limit(const YAML::Node& config) { - health = config["health"].as<std::uint16_t>(); - bullet = config["bullet"].as<std::uint16_t>(); + if (config["health"]) { + health = config["health"].as<std::uint16_t>(); + } + if (config["bullet"]) { + bullet = config["bullet"].as<std::uint16_t>(); + } }或使用带默认值的版本：
health = config["health"].as<std::uint16_t>(0); bullet = config["bullet"].as<std::uint16_t>(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/brain/rule.hh` around lines 11 - 14, The Limit constructor currently calls config["health"].as<std::uint16_t>() and config["bullet"].as<std::uint16_t>() without handling YAML::BadConversion or missing keys; update the Limit(const YAML::Node& config) constructor to safely parse these values by either using the as<T>(default) overload (e.g., as<std::uint16_t>(0)) for health and bullet or wrapping the parses in a try-catch that handles YAML::BadConversion and provides sane defaults or rethrows a clearer exception; ensure you modify the Limit constructor (and adjust any callers like Rule construction if you choose to propagate errors) so missing or wrong-typed YAML nodes no longer crash the process.src/app/test.ccm-13-18 (1)
13-18:⚠️ Potential issue | 🟠 Major
run()目前为空，Test实际不执行任何状态机逻辑当前实现既没有注册状态处理器，也没有驱动
fsm进入事件循环；这个导出的类在运行时等同于空操作。建议在构造阶段完成use<...>注册，并在run()中循环spin_once()到END。💡 可参考的最小修复方向
export class Test final { private: enum class Status { TEST, END }; rmcs::Fsm<Status> fsm{Status::TEST}; + auto setup() -> void { + fsm.use<Status::TEST>( + [] {}, + [] { return Status::END; }); + fsm.use<Status::END>( + [] {}, + [] { return Status::END; }); + } public: + Test() { setup(); } auto run() -> void { - // + while (!fsm.spin_once()) { + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/test.ccm` around lines 13 - 18, The Test export currently does nothing because no state handlers are registered and run() is empty; in the class constructor register handlers using fsm.use<...>(...) for each state (e.g., TEST and others) and any transition handlers you need, then implement run() to repeatedly call fsm.spin_once() (or the Fsm method that advances one step) in a loop until the state becomes Status::END (or !fsm.running()); ensure you reference rmcs::Fsm<Status> fsm, the use<T>() registrations, and spin_once()/Status::END so the state machine is driven to completion.test/fsm.cc-5-45 (1)
5-45:⚠️ Potential issue | 🟠 Major使用
ament_add_gtest注册但采用自定义main()而非 TEST 用例的结构test/fsm.cc 采用独立可执行程序的结构（自定义
main()函数），但在 CMakeLists.txt 中使用ament_add_gtest(fsm_test fsm.cc)注册。ament_add_gtest期望文件包含TEST()或TEST_F()宏，这导致框架无法正确识别和执行测试，容易出现断言数为 0 的情况。建议改写为标准 Google Test 用例并移除自定义main()函数。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fsm.cc` around lines 5 - 45, The current test defines a standalone main() which conflicts with using ament_add_gtest; remove the custom main() and convert the code into a Google Test case (e.g., TEST(FsmTest, RunsToEnd) or a TEST_F fixture) that constructs rmcs::Fsm{Status::A}, registers the states via fsm.use<Status::A/B/C>(), checks fsm.fully_registered()/registered_count() with EXPECT/ASSERT, then runs the event loop using the existing b_event_count/b_event_limit and fsm.spin_once() inside the test body and assert the expected outcome (e.g., final state reached or spin loop exited). Ensure no standalone main() remains so ament_add_gtest can discover and run the test.src/util/rclcpp/node.ccm-3-5 (1)
3-5:⚠️ Potential issue | 🟠 Major添加缺失的标准库头文件以确保编译可移植性
代码使用了
std::string_view（第17行）、std::ranges::all_of和std::ranges::empty（第18、24行）、std::string（第45行）、std::format和std::format_string（第50、55、60行）、std::forward（第51、56、61行），但未直接包含对应的标准库头文件。这些特性的声明可能通过rclcpp头文件的传递包含隐式引入，但依赖这种间接包含会影响代码可移植性。需要补齐直接依赖。📦 建议补充头文件
module; `#include` <rclcpp/executors.hpp> `#include` <rclcpp/node.hpp> +#include <format> +#include <ranges> +#include <string> +#include <string_view> +#include <utility>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/rclcpp/node.ccm` around lines 3 - 5, The file uses std::string_view, std::ranges::all_of, std::ranges::empty, std::string, std::format, std::format_string and std::forward but does not directly include their standard headers; add the appropriate headers (e.g., <string_view>, <string>, <utility>, <format>, and <ranges> or <algorithm> as needed) at the top of node.ccm so these symbols are declared explicitly rather than relying on transitive includes from rclcpp; ensure the includes appear before any code that references std::format/std::format_string, std::forward, std::string_view, std::string, and std::ranges utilities.src/brain/plan.cc-16-27 (1)
16-27:⚠️ Potential issue | 🟠 Major
plan_map和do_plan()返回值未被使用
plan_map中注册的 lambdas 从未被调用，do_plan()返回的计划名称被do_plan_()丢弃（返回类型为void）。这导致规划逻辑无实际效果。🐛 建议执行计划动作
-auto PlanBox::do_plan_() noexcept -> void { pimpl->do_plan(); } +auto PlanBox::do_plan_() noexcept -> void { + auto plan_name = pimpl->do_plan(); + if (auto it = pimpl->plan_map.find(plan_name); it != pimpl->plan_map.end()) { + it->second(); + } +}Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/brain/plan.cc` around lines 16 - 27, The registered lambdas in Impl::plan_map (keys to_the_home and cruise_mode) are never invoked and do_plan()'s result is discarded by do_plan_() which is void; update the flow so selecting a plan executes its corresponding action: modify do_plan_() (or do_plan()) to return the chosen plan name (or directly execute) and then lookup plan_map[plan] and invoke the stored std::function; ensure Impl::plan_map entries (to_the_home, cruise_mode) contain real handlers and that callers of do_plan_ use its return value or let do_plan_ call plan_map.at(...)() to perform the action.src/main.cc-10-21 (1)
10-21:⚠️ Potential issue | 🟠 Major修复拼写错误并处理 ROS 节点自旋
第 17 行存在拼写错误："Uncathed" 应为 "Uncaught"。
此外，
app_node被创建但从未调用spin()，这意味着如果应用中添加了 ROS 订阅或服务，它们将无法处理回调。目前app::Test::run()为空实现，但后续功能需要确保节点能处理 ROS 消息。拼写错误修复
auto always_run = [&]<class App> { try { auto app = App{}; app.run(); } catch (const std::exception& e) { - app_node.error("Uncathed exception while app running, shutdown now"); + app_node.error("Uncaught exception while app running, shutdown now"); app_node.error(" {}", e.what()); app_node.shutdown(); } };需要添加 executor 来处理 ROS 消息，可以在
app.run()内部实现自旋逻辑或在main()中为节点创建 executor。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cc` around lines 10 - 21, Fix the typo in the error message and ensure the ROS node spins so callbacks run: change the string passed to app_node.error from "Uncathed exception while app running, shutdown now" to "Uncaught exception while app running, shutdown now", and add executor/spin logic so the created app_node actually processes ROS callbacks — either integrate spinning into App::run (e.g., accept or create a rclcpp::Executor and call spin/poll inside run) or create a rclcpp::executors::Executor in main alongside app_node and call spin_some()/spin() while the app is running; update the catch block to shutdown the executor/node as needed (references: app_node, always_run, App, app.run, App::run/app::Test::run).scripts/follow_waypoints_runner.py-27-28 (1)
27-28:⚠️ Potential issue | 🟠 Major这里的 yaw 反算在有 roll/pitch 时会失真。
_quaternion_to_yaw()只使用了z/w，把x/y分量直接丢掉了。默认里程计话题/aft_mapped_to_init这类 3D 姿态一旦带有滚转或俯仰，yaw_error就会算错，进而提前或延后触发 waypoint 到达判定。建议改成完整四元数公式。🧮 建议修改
-def _quaternion_to_yaw(z: float, w: float) -> float: - return math.atan2(2.0 * w * z, 1.0 - 2.0 * z * z) +def _quaternion_to_yaw(x: float, y: float, z: float, w: float) -> float: + siny_cosp = 2.0 * (w * z + x * y) + cosy_cosp = 1.0 - 2.0 * (y * y + z * z) + return math.atan2(siny_cosp, cosy_cosp)- current_yaw = _quaternion_to_yaw( - self.current_pose.orientation.z, - self.current_pose.orientation.w, - ) + current_yaw = _quaternion_to_yaw( + self.current_pose.orientation.x, + self.current_pose.orientation.y, + self.current_pose.orientation.z, + self.current_pose.orientation.w, + )Also applies to: 209-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/follow_waypoints_runner.py` around lines 27 - 28, _quaternion_to_yaw currently computes yaw using only z and w, which breaks when roll/pitch are nonzero; replace it with the full quaternion-to-yaw formula that uses all components (x, y, z, w): compute yaw as atan2(2*(w*z + x*y), 1 - 2*(y*y + z*z)). Update the function signature and callers so they pass x and y as well (and adjust any code computing yaw_error), and apply the same fix to the other occurrence referenced (the block around the other quaternion-to-yaw usage at lines 209-212).scripts/follow_waypoints_runner.py-170-175 (1)
170-175:⚠️ Potential issue | 🟠 Major失败分支当前会让 runner 永久空转。
goal 被拒绝或结果非成功时，这里只清掉
goal_in_flight并记录日志，没有 retry、skip，也没有finished终止。而_ensure_started()只会在current_index < 0时再次启动流程，所以第一个失败 waypoint 之后节点就不会再前进了。这里至少要明确一种失败策略。Also applies to: 194-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/follow_waypoints_runner.py` around lines 170 - 175, The failure branch leaves the runner stuck because it only clears goal_in_flight and logs, so after a rejected/failed waypoint the loop never advances; update the handler for rejected goals (the block checking goal_handle.accepted) and the analogous result-non-success block to implement a clear failure policy: either mark the run finished (set finished = True) or advance to the next waypoint by incrementing current_index and invoking the routine that starts/dispatches the next goal (use _ensure_started() or the existing dispatch/advance helper used elsewhere), and optionally reset/initialize a per-waypoint retry counter if you want retries; ensure goal_in_flight is False, update current_index or finished consistently, and call the same restart/advance logic in both failure locations so the runner does not permanently stall.scripts/follow_waypoints_runner.py-104-107 (1)
104-107:⚠️ Potential issue | 🟠 Major不要把缺失坐标的 waypoint 静默补成
0.0。
dict形式的 waypoint 一旦漏了x或y，这里会直接把目标改到(0, 0)附近继续运行。配置里只要有个键名拼写错误，就会把机器人发到错误位置。x/y应该是必填项，只有yaw适合保留默认值。🧩 建议修改
elif isinstance(point, dict): - x = float(point.get("x", 0.0)) - y = float(point.get("y", 0.0)) + if "x" not in point or "y" not in point: + raise RuntimeError(f"Waypoint is missing x/y: {point}") + x = float(point["x"]) + y = float(point["y"]) yaw = float(point.get("yaw", 0.0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/follow_waypoints_runner.py` around lines 104 - 107, The current branch that handles dict waypoints silently substitutes missing x/y to 0.0; change it to validate x and y as required fields instead of defaulting them: when isinstance(point, dict) check for presence of "x" and "y" (and that they are convertible to float), raise a clear error (or log and abort) naming the offending waypoint (use the loop index or the point content) if either is missing or invalid, while still allowing "yaw" to default to 0.0; update the code paths around the variables x, y, yaw so x and y are only set from point.get(...) after presence is confirmed.launch/custom.launch.py-188-200 (1)
188-200:⚠️ Potential issue | 🟠 Major在模式分支内部延迟解析可选依赖的 launch 路径。
generate_launch_description()在第188-189行立即解析livox_ros_driver2和point_lio的share目录。但根据MODE_MAP，static模式根本不需要这些包（use_livox_driver=False, use_point_lio=False），而bag模式也不需要livox_ros_driver2（use_livox_driver=False）。实际使用这些路径的地方是在_build_mode_actions()函数内的条件分支中（第102行和第110行）。当前的设计会导致：用户运行
mode=static或mode=bag时，如果机器上没有安装对应的可选包，launch 会在第188-189行直接失败，完全无法启动，而不是按需延迟解析这些依赖。建议将这些路径解析移到_build_mode_actions()函数内，改为在实际需要使用时再解析。此外，第220行中硬编码的bag默认路径
/workspaces/data/bag/battlefield_1是开发者本地路径，降低了可移植性，建议使用相对路径或从配置文件加载。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/custom.launch.py` around lines 188 - 200, generate_launch_description() eagerly calls get_package_share_directory for livox_ros_driver2 and point_lio (livox_share/point_lio_share and livox_launch/point_lio_launch) even when MODE_MAP indicates use_livox_driver/use_point_lio are false; move the package/share resolution and os.path.join creation for livox_launch and point_lio_launch into the conditional branches inside _build_mode_actions() so those packages are only resolved when use_livox_driver or use_point_lio is true, and clean up the now-unused top-level livox_share/point_lio_share variables; also replace the hardcoded bag default path (currently a literal like "/workspaces/data/bag/battlefield_1") with a relative-path default or load it from a configuration/launch argument so the default is portable.
🟡 Minor comments (5)
src/core/environment/voxel-map.ccm-5-9 (1)
5-9:⚠️ Potential issue | 🟡 MinorC++20 模块编译配置已禁用
CMakeLists.txt 中的整个 C++20 模块编译配置（第 31-61 行）当前被注释掉，包括模块文件的文件集定义。这导致
src/core/environment/voxel-map.ccm及其他.ccm模块文件无法被编译。当前项目仅通过${PROJECT_NAME}_brain库目标使用传统的.cc源文件进行构建。若要启用此模块及其他 C++20 模块的编译，需取消 CMakeLists.txt 第 31-61 行的注释。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/environment/voxel-map.ccm` around lines 5 - 9, CMake currently has the C++20 module build configuration commented out, so module files like voxel-map.ccm (which defines rmcs::VoxelMap) are never compiled; to fix this, re-enable the C++20 module build by uncommenting the module-related block in CMakeLists (the block that defines the file set for .ccm files and the module handling), ensure the project sets C++20 modules support (e.g., enable the experimental module support or target_compile_features/target_compile_options for cxx_std_20 as used by your build) and add voxel-map.ccm (and other .ccm files) to the target_sources or file set so rmcs::VoxelMap in voxel-map.ccm is built into the ${PROJECT_NAME}_brain target.config/custom.yaml-6-8 (1)
6-8:⚠️ Potential issue | 🟡 Minor硬编码的绝对路径影响可移植性。
bag.path使用了硬编码的绝对路径/workspaces/data/bag/520双创飙车事件/，这会导致：
- 在其他开发环境或用户系统上无法直接使用
- 路径中的中文字符可能在某些系统上引发编码问题
建议使用相对路径、环境变量或在
.gitignore中忽略此文件并提供custom.yaml.example模板。💡 建议的配置模板方案
创建
config/custom.yaml.example:debug_goal_bridge: ros__parameters: use_sim_time: false goal_frame: base_link bag: path: /path/to/your/bag/directory/ use_clock: true并在
.gitignore中添加:config/custom.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/custom.yaml` around lines 6 - 8, The hard-coded absolute path in config/custom.yaml (bag.path) reduces portability and risks encoding issues; replace it by documenting a placeholder or environment-driven value: remove the literal "/workspaces/..." and set bag.path to a relative or templated placeholder (e.g., ./bags/ or ${BAG_PATH}) in config/custom.yaml.example, add config/custom.yaml to .gitignore, and update any code that reads bag.path to fall back to an environment variable (BAG_PATH) or the relative default if not set; ensure the unique key "bag.path" is updated in the example and the repo ignore so developers populate their own local config.rmcs-navigationConfig.cmake.in-1-13 (1)
1-13:⚠️ Potential issue | 🟡 Minorrmcs-navigationConfig.cmake.in 模板文件未被使用。
该模板文件存在但未被处理。确认的问题：
@CONF_INCLUDE_DIRS@占位符从未定义，整个代码库中没有configure_file()调用来处理此模板- 模板引用的
export_rmcs-navigation.cmake文件生成过程不明确- 在此 ament_cmake 项目中，配置文件生成应通过
ament_package()自动处理，但该 .in 模板并未被正确集成建议移除这些未使用的 .in 文件（rmcs-navigationConfig.cmake.in 和 rmcs-navigationConfigVersion.cmake.in），或在 CMakeLists.txt 中补充适当的配置（如设置 CONF_INCLUDE_DIRS 变量并调用 configure_file()）。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rmcs-navigationConfig.cmake.in` around lines 1 - 13, The rmcs-navigationConfig.cmake.in template is never processed because `@CONF_INCLUDE_DIRS`@ is undefined and no configure_file() is invoked; either remove the unused templates (rmcs-navigationConfig.cmake.in and rmcs-navigationConfigVersion.cmake.in) or integrate them into CMake: define CONF_INCLUDE_DIRS, call configure_file() to generate rmcs-navigationConfig.cmake from rmcs-navigationConfig.cmake.in, and ensure the export_rmcs-navigation.cmake target generation is wired into the install/export steps (or rely on ament_package() to produce the proper config files); update CMakeLists.txt accordingly to call configure_file(), set the CONF_INCLUDE_DIRS value, and produce/export export_rmcs-navigation.cmake so the IMPORTED targets referenced in the template are valid.scripts/debug_goal_bridge.py-130-142 (1)
130-142:⚠️ Potential issue | 🟡 Minor
finally块中的return会抑制异常根据静态分析提示（Ruff B012），
finally块中的return（第 141 行）会抑制run_goal(next_goal)可能抛出的任何异常。🐛 建议重构以避免在 finally 中 return
def _run_goal_thread(self, msg: PoseStamped) -> None: + current_msg = msg + while True: try: - self.run_goal(msg) - finally: - with self._lock: - self._current_compute_handle = None - self._current_follow_handle = None - next_goal = self._pending_goal - self._pending_goal = None - if next_goal is None: - self._busy = False - return - self.run_goal(next_goal) + self.run_goal(current_msg) + finally: + with self._lock: + self._current_compute_handle = None + self._current_follow_handle = None + next_goal = self._pending_goal + self._pending_goal = None + if next_goal is None: + self._busy = False + break + current_msg = next_goal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/debug_goal_bridge.py` around lines 130 - 142, The finally block in _run_goal_thread currently contains a return which can suppress exceptions; refactor so the finally only does state mutation under self._lock (clearing _current_compute_handle, _current_follow_handle and capturing next_goal into a local var while setting _pending_goal to None) and does not return; after the try/finally, check the captured next_goal: if None set self._busy = False inside the lock or via a safe path, otherwise call self.run_goal(next_goal) outside the finally/lock; use the existing symbols _run_goal_thread, run_goal, _lock, _current_compute_handle, _current_follow_handle, _pending_goal, and _busy to locate and implement the change.launch/custom.launch.py-218-221 (1)
218-221:⚠️ Potential issue | 🟡 Minor避免把开发机本地路径当成 bag 默认值。
/workspaces/data/bag/battlefield_1是环境相关的绝对路径。mode=bag时如果用户没显式覆盖参数，默认值大概率就是不存在的路径，体验上等同于开箱即坏。更稳妥的做法是默认留空，并在 bag 模式下显式校验。🧩 建议修改
- bag_path_default = str( - bag_config.get("path", "/workspaces/data/bag/battlefield_1") - ) + bag_path_default = str(bag_config.get("path", ""))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/custom.launch.py` around lines 218 - 221, The code currently sets bag_path_default to a hardcoded dev-specific absolute path via bag_config.get("path", "/workspaces/data/bag/battlefield_1"); change this to use an empty default (e.g., "" or None) instead of the absolute path, and add explicit validation when running in bag mode (check the runtime mode, e.g., mode == "bag") to raise a clear error or prompt if bag_path is empty or the path does not exist; update references to bag_config and bag_path_default and ensure the validation occurs before any code that assumes the bag path exists.
🧹 Nitpick comments (15)
src/core/environment/voxel-map.ccm (1)
1-3: 不必要的全局模块片段声明
module;用于声明全局模块片段（Global Module Fragment），其目的是在模块声明之前放置预处理指令（如#include）。当前代码中module;与export module之间没有任何内容，可以直接移除。♻️ 建议的修改
-module; - export module core.env.voxel_map;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/environment/voxel-map.ccm` around lines 1 - 3, 当前文件在顶端保留了不必要的全局模块片段标记 "module;" 而随后直接声明了导出模块 "export module core.env.voxel_map;"; 请删除孤立的 "module;" 行，保留并使用 "export module core.env.voxel_map;" 作为模块声明以清理代码并避免冗余全局模块片段声明。AGENTS.md (1)
38-41: 代码块缺少语言标识符。该代码块展示的是 C++ 示例代码，应指定语言类型以启用语法高亮。
📝 建议修复
-``` +```cpp # 使用大括号构造和类型后置 auto var = T { };</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@AGENTS.mdaround lines 38 - 41, The fenced code block showing the C++
example ("auto var = T { };") lacks a language identifier; update the opening
fence fromtocpp so the block becomescpp followed by the line "auto var = T { };" and the closingto enable C++ syntax highlighting in the
AGENTS.md document.</details> </blockquote></details> <details> <summary>src/brain/plan.hh (1)</summary><blockquote> `1-3`: **缺少 `<tuple>` 和 `<cstdint>` 头文件。** `goal_position()` 返回 `std::tuple<double, double>`，`Information` 使用 `std::uint16_t`，但未显式包含 `<tuple>` 和 `<cstdint>` 头文件。虽然这些可能通过 `<yaml-cpp/yaml.h>` 间接包含，但依赖传递包含是脆弱的。 <details> <summary>📦 建议添加必要的头文件</summary> ```diff `#pragma` once +#include <cstdint> +#include <tuple> `#include` "util/pimpl.hh" `#include` <yaml-cpp/yaml.h> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/brain/plan.hh` around lines 1 - 3, Add explicit includes for <tuple> and <cstdint> at the top of the header: goal_position() returns std::tuple<double,double> and the Information type uses std::uint16_t, so include those headers to avoid relying on transitive includes; update the include block that currently contains util/pimpl.hh and <yaml-cpp/yaml.h> to also `#include` <tuple> and `#include` <cstdint>. ``` </details> </blockquote></details> <details> <summary>src/brain/rule.hh (1)</summary><blockquote> `1-3`: **缺少 `<tuple>` 和 `<vector>` 头文件。** `Battlefield` 使用 `std::tuple` 和 `std::vector`，但未显式包含相应头文件。 <details> <summary>📦 建议添加必要的头文件</summary> ```diff `#pragma` once `#include` <cstdint> +#include <tuple> +#include <vector> `#include` <yaml-cpp/yaml.h> ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/brain/rule.hh` around lines 1 - 3, Header file is missing includes for types used by Battlefield: add explicit includes for <tuple> and <vector> so std::tuple and std::vector are available; update the top of rule.hh (where YAML and cstdint are included) to also include <tuple> and <vector> to resolve missing type declarations used by Battlefield. ``` </details> </blockquote></details> <details> <summary>config/nav2.yaml (1)</summary><blockquote> `3-3`: **`use_sim_time` 硬编码为 `true`，可能影响实机部署。** 整个配置文件中 `use_sim_time: true` 被硬编码在多个节点配置中。在实机部署时需要设置为 `false`。建议通过 launch 文件参数化此配置，或创建单独的实机配置文件。 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@config/nav2.yaml` at line 3, The config currently hardcodes use_sim_time: true which breaks real-robot runs; change it to be driven by a launch arg or separate real-world params instead of a hardcoded value. Update your launch file to declare and expose a boolean launch argument (e.g., use_sim_time) and pass that into the Nav2 parameter override when composing nodes, or create an alternate parameters file for real hardware and switch files based on a launch flag; ensure all occurrences of use_sim_time in the Nav2 params are replaced with the launch-provided value or the alternate file so the real-robot deployment can set it to false. ``` </details> </blockquote></details> <details> <summary>src/util/pimpl.hh (1)</summary><blockquote> `4-9`: **抑制未使用 include 警告的方式不够优雅。** 此辅助函数仅为消除未使用 `<memory>` 的警告而存在。由于宏 `RMCS_PIMPL_DEFINITION` 展开时会使用 `std::unique_ptr`，包含该头文件的用户代码自然会使用 `<memory>`，此 workaround 应无必要。若确实需要，考虑使用 `[[maybe_unused]]` 属性或在文档中说明头文件的必要依赖。 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/util/pimpl.hh` around lines 4 - 9, The helper function pimpl::internal::use_memory_header that assigns to std::ignore to silence an unused <memory> include should be removed; instead either rely on the fact RMCS_PIMPL_DEFINITION uses std::unique_ptr (so <memory> is a real dependency) or, if you must keep a no-op symbol, mark it with [[maybe_unused]] to avoid side-effecty code. Update/remove use_memory_header and add a short comment in the header stating that RMCS_PIMPL_DEFINITION requires <memory> (or annotate the no-op with [[maybe_unused]] if you prefer to keep it). ``` </details> </blockquote></details> <details> <summary>src/util/fsm.hh (1)</summary><blockquote> `28-28`: **枚举值必须从 0 开始且连续，否则数组大小计算错误。** `std::to_underlying(StateKey::END)` 假设枚举值从 0 开始且连续递增。如果用户定义的枚举有间隙或显式值（例如 `A = 0, B = 10, END = 11`），`states_map` 大小将不正确，可能导致越界访问。 建议在文档或注释中明确此约束，或添加编译时检查。 <details> <summary>📝 建议：添加使用约束注释</summary> ```diff template <typename state_type> class Fsm { static_assert(std::is_enum_v<state_type>, "state_type must be an enum"); static_assert(requires { state_type::END; }, "state_type::END is required"); + // Note: state_type enum values must be sequential starting from 0, + // with END as the last value. Example: + // enum class State { A, B, C, END }; // OK: A=0, B=1, C=2, END=3 ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/util/fsm.hh` at line 28, The code assumes StateKey values are 0-based and contiguous for states_map sizing; add a compile-time check and a short comment to enforce this. Implement a constexpr validator (e.g., validate_contiguous_enum<StateKey>()) and a static_assert that std::to_underlying(StateKey::END) equals the number of contiguous values starting at 0 (and/or that the first valid enum value is 0), then keep the existing std::array<StateUnique, std::to_underlying(StateKey::END)> states_map; and add a brief comment above mentioning the 0-based contiguous enum constraint; reference the StateKey enum and the states_map declaration when adding the check. ``` </details> </blockquote></details> <details> <summary>launch/follow_waypoints.launch.py (1)</summary><blockquote> `38-62`: **应改用 `launch_ros.actions.Node` 替代 `ExecuteProcess` 启动此 ROS 节点** `follow_waypoints_runner.py` 是一个正规的 ROS 节点（继承自 `rclpy.node.Node`），使用参数声明机制。当前方式虽可运行，但需手动拼接命令行参数。改为 `Node` 可直接通过 `parameters` 属性传递，参数管理、日志处理和生命周期行为更标准，同时与项目其他启动文件（如 `nav2.launch.py`）保持一致。 示例参考： ```python from launch_ros.actions import Node Node( package="rmcs-navigation", executable="follow_waypoints_runner.py", parameters=[ {"config_file": LaunchConfiguration("follow_waypoints_file")}, {"odom_topic": LaunchConfiguration("odom_topic")}, {"distance_tolerance": LaunchConfiguration("distance_tolerance")}, {"yaw_tolerance": LaunchConfiguration("yaw_tolerance")}, ], output="screen", ) ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@launch/follow_waypoints.launch.py` around lines 38 - 62, 将当前用 ExecuteProcess 启动 follow_waypoints_runner.py 的代码替换为使用 launch_ros.actions.Node：不要手动拼接 cmd 列表，改用 Node(package="rmcs-navigation", executable="follow_waypoints_runner.py") 并通过 parameters 传入参数字典，使用 LaunchConfiguration("follow_waypoints_file") 映射到 config_file，LaunchConfiguration("odom_topic") 映射到 odom_topic，LaunchConfiguration("distance_tolerance") 映射到 distance_tolerance，LaunchConfiguration("yaw_tolerance") 映射到 yaw_tolerance，保留 output="screen"；移除原先对 ExecuteProcess 的 cmd 构建以便使用 ROS 参数声明机制和标准生命周期/日志行为。 ``` </details> </blockquote></details> <details> <summary>scripts/goal_topic_bridge.py (2)</summary><blockquote> `41-49`: **`wait_for_server()` 无超时可能导致无限阻塞** `wait_for_server()` 没有设置超时参数，如果 action server 不可用，会导致线程无限阻塞。此外，取消目标时未等待取消操作完成就发送新目标，可能产生竞态条件。 <details> <summary>♻️ 建议添加超时并等待取消完成</summary> ```diff def _send_goal(self, msg: PoseStamped) -> None: - self._client.wait_for_server() + if not self._client.wait_for_server(timeout_sec=5.0): + self.get_logger().warning("navigate_to_pose action server not available") + return with self._lock: current_goal = self._current_goal if current_goal is not None: cancel_future = current_goal.cancel_goal_async() - cancel_future.add_done_callback(lambda _: None) + rclpy.spin_until_future_complete(self, cancel_future, timeout_sec=2.0) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/goal_topic_bridge.py` around lines 41 - 49, In _send_goal, avoid indefinite blocking and race conditions by calling wait_for_server with a sensible timeout (e.g., pass a timeout/duration) and handling the timeout case (log/error/return) instead of blocking forever, and when cancelling a prior goal use the returned cancel_future from cancel_goal_async to wait for completion (e.g., await or call result() with timeout or attach a done-wait) before proceeding to send the new goal; update references in this method (_send_goal, wait_for_server, cancel_goal_async, cancel_future) accordingly to handle timeout/failure paths and ensure the cancel completes before sending the next goal. ``` </details> --- `60-68`: **未处理 `future.result()` 可能抛出的异常** 如果 future 由于异常而完成，`future.result()` 会抛出该异常。建议添加 try-except 处理。 <details> <summary>🛡️ 建议添加异常处理</summary> ```diff def _handle_goal_response(self, future) -> None: - goal_handle = future.result() + try: + goal_handle = future.result() + except Exception as e: + self.get_logger().error(f"Failed to send goal: {e}") + return if goal_handle is None or not goal_handle.accepted: ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/goal_topic_bridge.py` around lines 60 - 68, The handler _handle_goal_response should catch exceptions raised by future.result(): wrap the call to future.result() in a try/except, log the exception (using self.get_logger().error or warning with the exception details) and return early if an exception occurs, then continue with the existing acceptance check and the critical-section update of self._current_goal and last_goal inside the self._lock. Ensure you reference future.result(), goal_handle, self.get_logger(), self._lock, self._current_goal and self._last_goal when making the change. ``` </details> </blockquote></details> <details> <summary>CMakeLists.txt (1)</summary><blockquote> `20-23`: **Eigen3 和 PCL 已声明但未链接** `find_package(Eigen3 REQUIRED)` 和 `find_package(PCL REQUIRED)` 已声明，但 `${PROJECT_NAME}_brain` 目标未链接这些库。根据之前的 learnings，如果这是为未来使用预留的，建议添加注释说明。 <details> <summary>💡 建议添加说明注释</summary> ```diff +# TODO: Eigen3 is reserved for future matrix operations in planning find_package(Eigen3 REQUIRED) find_package(yaml-cpp REQUIRED) find_package(rclcpp REQUIRED) +# TODO: PCL is reserved for future point cloud processing find_package(PCL REQUIRED) ``` </details> Based on learnings: "In CMakeLists.txt files, when you intentionally include a dependency for future use, add a brief comment explaining the rationale." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 20 - 23, The CMakeLists declares find_package(Eigen3 REQUIRED) and find_package(PCL REQUIRED) but never links them to the target `${PROJECT_NAME}_brain`; either link the required libraries (use target_link_libraries(${PROJECT_NAME}_brain PUBLIC Eigen3::Eigen PCL::PCL or the correct imported targets for your packages) or, if they are intentionally reserved for future use, add a brief comment next to the find_package lines explaining that they are declared now for future features and therefore not linked to `${PROJECT_NAME}_brain` yet. ``` </details> </blockquote></details> <details> <summary>src/brain.cc (1)</summary><blockquote> `71-100`: **FSM 状态转换逻辑为空壳实现** 每个状态的 `spin` 函数体只有注释 `// ...`，且立即无条件返回下一个状态。这意味着 FSM 会在一次 `spin_once()` 调用中快速遍历所有状态直到 `END`。 如果这是 WIP 代码，建议添加 TODO 注释标明待实现的逻辑；如果是预期行为，请确认这符合设计意图。 需要我帮助实现基于 `game_stage` 输入的状态转换逻辑吗？ <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/brain.cc` around lines 71 - 100, The FSM in generate_brain_fsm currently has empty spin lambdas that immediately return the next state, causing the FSM to fast-forward to END; replace these placeholders with real transition logic (or TODO markers) that inspect the node's runtime input (e.g. a member like game_stage or a sensor/event flag) and only return the next BrainStage when the appropriate condition is met; update the lambdas for BrainStage::WaitForBegin, ::Prepare, ::Running and ::GameOver to capture any needed members (keep [this] where state handlers need access) and evaluate the real conditions before returning the next enum value, or add explicit TODO comments in each lambda documenting the intended condition and a short TODO tag so reviewers know it's WIP, ensuring brain_fsm.fully_registered() remains checked afterward. ``` </details> </blockquote></details> <details> <summary>scripts/debug_goal_bridge.py (1)</summary><blockquote> `72-80`: **重复创建 service client 效率较低** `get_lifecycle_state()` 和 `change_lifecycle_state()` 每次调用都创建新的 service client。建议在 `__init__` 中缓存这些 client 或复用实例。 Also applies to: 82-94 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/debug_goal_bridge.py` around lines 72 - 80, get_lifecycle_state and change_lifecycle_state currently call create_client each time which is inefficient; modify the class to create and cache needed service clients (e.g., GetState and ChangeState clients) during __init__ and reuse them instead of recreating per call—either store as attributes like self._get_state_client and self._change_state_client or keep a dict keyed by f"/{node_name}/get_state" / f"/{node_name}/change_state" and lookup/reuse clients in get_lifecycle_state and change_lifecycle_state (refer to create_client, get_lifecycle_state, change_lifecycle_state to locate spots). ``` </details> </blockquote></details> <details> <summary>launch/nav2.launch.py (1)</summary><blockquote> `106-106`: **建议使用列表解包替代连接操作** 静态分析建议使用 `[*remappings, ("cmd_vel", "cmd_vel_nav")]` 替代列表连接，这是更符合 Python 习惯的写法。 <details> <summary>♻️ 建议的修改</summary> ```diff Node( package="nav2_controller", executable="controller_server", name="controller_server", output="screen", parameters=[controller_params, {"use_sim_time": use_sim_time}], - remappings=remappings + [("cmd_vel", "cmd_vel_nav")], + remappings=[*remappings, ("cmd_vel", "cmd_vel_nav")], ), ``` ```diff Node( package="nav2_behaviors", executable="behavior_server", name="behavior_server", output="screen", parameters=[planner_params, {"use_sim_time": use_sim_time}], - remappings=remappings + [("cmd_vel", "cmd_vel_nav")], + remappings=[*remappings, ("cmd_vel", "cmd_vel_nav")], ), ``` ```diff Node( package="nav2_velocity_smoother", executable="velocity_smoother", name="velocity_smoother", output="screen", parameters=[planner_params, {"use_sim_time": use_sim_time}], - remappings=remappings + [("cmd_vel", "cmd_vel_nav")], + remappings=[*remappings, ("cmd_vel", "cmd_vel_nav")], ), ``` </details> Also applies to: 122-122, 146-146 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@launch/nav2.launch.py` at line 106, Replace list concatenation expressions that append a single tuple to remappings (e.g., the statement remappings = remappings + [("cmd_vel", "cmd_vel_nav")]) with list-unpacking syntax: remappings = [*remappings, ("cmd_vel", "cmd_vel_nav")]. Update the same pattern wherever remappings is extended (the other occurrences following the same form) so each uses the unpacking form to follow Python idioms and avoid creating an intermediate list via +. ``` </details> </blockquote></details> <details> <summary>scripts/static_grid_publisher.py (1)</summary><blockquote> `65-84`: **考虑缓存栅格数据以减少内存分配** `publish_grid()` 每次调用都创建新的 `[0] * (width * height)` 列表，对于默认的 505x505 栅格，每 0.5 秒分配约 255K 整数。对于 "empty" 模式，可以预先创建并复用数据。 <details> <summary>♻️ 建议优化</summary> ```diff def __init__(self) -> None: # ... existing code ... self.timer = self.create_timer(0.5, self.publish_grid) + self._empty_data = [0] * (self.width * self.height) def publish_grid(self) -> None: msg = OccupancyGrid() # ... header setup ... - msg.data = [0] * (self.width * self.height) + if self.map_mode == "demo": + msg.data = [0] * (self.width * self.height) + self.mark_rect(msg.data, -0.4, 0.4, 0.8, 3.2) + # ... other mark_rect calls ... + else: + msg.data = self._empty_data.copy() - if self.map_mode == "demo": - self.mark_rect(msg.data, -0.4, 0.4, 0.8, 3.2) - # ... ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/static_grid_publisher.py` around lines 65 - 84, publish_grid currently allocates a new list [0] * (self.width * self.height) on every call; create and reuse a cached base grid (e.g., self._base_grid) sized once in the class __init__ (or lazily on first publish_grid call) to avoid repeated allocations, then in publish_grid use that cached list directly for self.map_mode == "empty" and for other modes (when calling mark_rect) make a cheap copy (e.g., working = list(self._base_grid)) before mutating so mark_rect updates do not change the cached base; update references in publish_grid, mark_rect, and class init to use self._base_grid. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `a594c362-6d6b-4137-91cd-feab0ab36c78` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between dde4d5bd38f8be3e0e0782fc00a43228bfb0d235 and e87c52b6c99f95513a742432299a34a585b7fc55. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `test/.cache/clangd/index/fsm.cc.B7DEAC8886980F9C.idx` is excluded by `!**/.cache/**` </details> <details> <summary>📒 Files selected for processing (38)</summary> * `.clangd` * `AGENTS.md` * `CMakeLists.txt` * `README.md` * `cmake/export.cmake` * `cmake/version.cmake` * `config/custom.yaml` * `config/follow_waypoints.yaml` * `config/nav2.yaml` * `config/rule.yaml` * `launch/custom.launch.py` * `launch/follow_waypoints.launch.py` * `launch/nav2.launch.py` * `package.xml` * `plugins.xml` * `rmcs-navigationConfig.cmake.in` * `rmcs-navigationConfigVersion.cmake.in` * `scripts/debug_goal_bridge.py` * `scripts/follow_waypoints_runner.py` * `scripts/goal_topic_bridge.py` * `scripts/static_grid_publisher.py` * `src/app/common.cc` * `src/app/common.hh` * `src/app/rmuc.ccm` * `src/app/rmul.ccm` * `src/app/test.ccm` * `src/brain.cc` * `src/brain/plan.cc` * `src/brain/plan.hh` * `src/brain/rule.hh` * `src/core/environment/voxel-map.ccm` * `src/main.cc` * `src/util/fsm.hh` * `src/util/pimpl.hh` * `src/util/rclcpp/node.ccm` * `test/CMakeLists.txt` * `test/fsm.cc` * `test/map.cc` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+1
to
+4
| install( | ||
| TARGETS ${PROJECT_NAME}_brain | ||
| DESTINATION lib/${PROJECT_NAME} | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 对比插件声明路径与安装路径是否一致
rg -n -C2 '<library path=' plugins.xml
rg -n -C2 'install\(|TARGETS\s+\$\{PROJECT_NAME\}_brain|DESTINATION\s+lib/\$\{PROJECT_NAME\}|LIBRARY DESTINATION|ARCHIVE DESTINATION|RUNTIME DESTINATION' cmake/export.cmakeRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 586
🏁 Script executed:
# Find plugin-related code and loading mechanism
fd -t f '\.cpp$|\.h$|\.py$' | head -20
rg -l 'plugin|Plugin|load.*library|dlopen' --type cpp --type h | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 296
🏁 Script executed:
# Check for any CMake configuration related to library search paths or RPATH
rg -n 'RPATH|CMAKE_PREFIX_PATH|LD_LIBRARY_PATH|lib_DIRS|find_package' cmake/ --type cmake | head -30Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 134
🏁 Script executed:
# Look for other plugin installations to compare
rg -n 'install.*TARGETS' cmake/ -A 3 | head -40Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 60
🏁 Script executed:
# Check the main CMakeLists.txt for any configuration
cat CMakeLists.txt 2>/dev/null | head -100Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 2294
🏁 Script executed:
# Search for rmcs_executor related code and how it loads plugins
rg -n 'rmcs_executor|rmcs::.*Component' src/ --type cppRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 265
🏁 Script executed:
# Check for any test or CI configuration that might reveal issues
fd -t f '\.yml$|\.yaml$|CMakeLists\.txt' | xargs grep -l 'test\|CI\|plugin' 2>/dev/null | head -10Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 129
🏁 Script executed:
# Look for any documentation or README about plugin setup
fd -t f 'README|INSTALL|SETUP|docs' | head -10Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 87
🏁 Script executed:
# Check if there's any ament-related CMake configuration for library installation
cat cmake/export.cmake 2>/dev/nullRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 594
🏁 Script executed:
# Check README for plugin setup information
cat README.mdRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 501
🏁 Script executed:
# Check test configuration
cat test/CMakeLists.txtRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 753
🏁 Script executed:
# Search for any plugin resolution or library path configuration in rmcs_executor
# First, let's see if rmcs_executor is available and check its installation pattern
rg -n 'install.*lib' --type cmakeRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 60
🏁 Script executed:
# Look for any usage examples or documentation about plugin paths
fd -t f '\.md$|\.txt$' | xargs grep -l 'plugin\|library.*path' 2>/dev/nullRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 92
🏁 Script executed:
# Check for package.xml to understand ROS 2 package structure
fd -t f 'package.xml'Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 89
🏁 Script executed:
# Look for any installation or build verification scripts
fd -t f 'install\.sh|setup\.sh|build\.sh|test\.sh'Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 60
🏁 Script executed:
# Check git history or issues that might reference plugin loading problems
git log --oneline --all 2>/dev/null | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 174
🏁 Script executed:
# Search for how other components/plugins are installed in rmcs ecosystem
rg -n 'DESTINATION.*lib' --type cmakeRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 134
修正插件库安装目录以解决运行时加载失败问题
plugins.xml 声明的库名为 rmcs-navigation_brain，但当前安装到 lib/${PROJECT_NAME} 子目录会导致 pluginlib 无法找到该库。ROS 2 的 ament_cmake 和 pluginlib 系统期望库直接安装在 lib/ 目录中。这将在运行时导致 rmcs_executor 无法加载该插件组件。
推荐修复
install(
TARGETS ${PROJECT_NAME}_brain
- DESTINATION lib/${PROJECT_NAME}
+ LIBRARY DESTINATION lib
+ ARCHIVE DESTINATION lib
+ RUNTIME DESTINATION bin
)📝 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
| install( | |
| TARGETS ${PROJECT_NAME}_brain | |
| DESTINATION lib/${PROJECT_NAME} | |
| ) | |
| install( | |
| TARGETS ${PROJECT_NAME}_brain | |
| LIBRARY DESTINATION lib | |
| ARCHIVE DESTINATION lib | |
| RUNTIME DESTINATION bin | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake/export.cmake` around lines 1 - 4, The plugin library is being installed
under lib/${PROJECT_NAME}, causing pluginlib to miss rmcs-navigation_brain;
change the install destination so the target ${PROJECT_NAME}_brain is installed
directly into lib by updating the install() call that references TARGETS
${PROJECT_NAME}_brain and DESTINATION lib/${PROJECT_NAME} to use DESTINATION lib
so the built library name (rmcs-navigation_brain) is placed where ROS2/pluginlib
expects it.
Comment on lines
+177
to
+178
| result_future = self.goal_handle.get_result_async() | ||
| result_future.add_done_callback(self._on_goal_result) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "follow_waypoints_runner.py"Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 114
🏁 Script executed:
wc -l ./scripts/follow_waypoints_runner.pyRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 118
🏁 Script executed:
cat -n ./scripts/follow_waypoints_runner.pyRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 9651
🏁 Script executed:
rg "add_done_callback|cancel_goal_async|get_result_async" --type py -B 2 -A 2Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 3833
先取消当前 goal 再立刻发下一个，会导致状态污染和日志错位。
cancel_goal_async() 是异步的，这里未等待取消完成就直接调用 _send_next_goal()（第 228 行），导致旧 goal 的 result callback 仍会在随后进入 _on_goal_result()。此时 self.current_index 已指向下一跳，导致：
- 日志中输出的 waypoint 序号错误（第 189、196 行）
- 若旧 goal 状态为 4（成功），会再次调用
_send_next_goal()（第 191 行），多发一个 goal goal_handle/goal_in_flight状态被覆盖
同样的问题也存在于 177-178 行的 result callback 注册处。建议在 cancel 完成后再发下一跳，或为 callback 绑定 goal ID 并忽略过期结果。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/follow_waypoints_runner.py` around lines 177 - 178, The result
callback is registered on result_future and cancel_goal_async() is invoked
without awaiting its completion, which allows stale goal results to reach
_on_goal_result() after self.current_index has advanced; to fix, either await
the cancellation future before calling _send_next_goal() (i.e., await the
cancel_goal_async() completion and only then call _send_next_goal()) or bind the
active goal identifier to the result callback (capture goal_handle.get_goal_id()
when sending a goal and in _on_goal_result() ignore results whose goal id
differs from the captured id), and ensure updates to goal_in_flight and
goal_handle only occur for the matching goal id to avoid state/log pollution.
src/brain.cc
Outdated
Comment on lines
+135
to
+141
| auto update() -> void override { | ||
| auto update = [this](PlanBox::Information& info) { | ||
| std::ignore = this; | ||
| info.health = 0.0; | ||
| info.health = 0.0; | ||
| }; | ||
| plan_box.update_information(update); |
There was a problem hiding this comment.
info.health 被重复赋值，info.bullet 未设置
在 update() lambda 中，info.health 被赋值两次（第 138 和 139 行），而 info.bullet 从未被设置。这会导致 do_plan() 中的 bullet 判断逻辑使用未初始化的值。
🐛 建议修复
auto update = [this](PlanBox::Information& info) {
std::ignore = this;
info.health = 0.0;
- info.health = 0.0;
+ info.bullet = 0.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
| auto update() -> void override { | |
| auto update = [this](PlanBox::Information& info) { | |
| std::ignore = this; | |
| info.health = 0.0; | |
| info.health = 0.0; | |
| }; | |
| plan_box.update_information(update); | |
| auto update() -> void override { | |
| auto update = [this](PlanBox::Information& info) { | |
| std::ignore = this; | |
| info.health = 0.0; | |
| info.bullet = 0.0; | |
| }; | |
| plan_box.update_information(update); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/brain.cc` around lines 135 - 141, The lambda passed to
plan_box.update_information in update() mistakenly assigns info.health twice and
never sets info.bullet, leaving PlanBox::Information::bullet uninitialized for
do_plan(); modify the lambda (inside update()) to remove the duplicate health
assignment and instead set info.bullet to an appropriate initial value (and keep
the single intended info.health assignment), ensuring PlanBox::Information
members are correctly initialized before plan_box.update_information returns.
src/component/brain/plan.cc
Outdated
Comment on lines
+29
to
+41
| auto do_plan() const noexcept { | ||
| auto health = information.health; | ||
| if (health < rule->limit.health) { | ||
| return to_the_home; | ||
| } | ||
|
|
||
| auto bullet = information.bullet; | ||
| if (bullet < rule->limit.bullet) { | ||
| return to_the_home; | ||
| } | ||
|
|
||
| return cruise_mode; | ||
| } |
There was a problem hiding this comment.
rule 可能为空指针导致崩溃
do_plan() 直接解引用 rule->limit，但 rule 是 std::unique_ptr<brain::Rule>，在 set_rule() 被调用之前为空。如果在规则设置前调用 do_plan()，会导致空指针解引用。
🐛 建议添加空指针检查
auto do_plan() const noexcept {
+ if (!rule) {
+ return cruise_mode; // 或返回安全默认值
+ }
auto health = information.health;
if (health < rule->limit.health) {
return to_the_home;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto do_plan() const noexcept { | |
| auto health = information.health; | |
| if (health < rule->limit.health) { | |
| return to_the_home; | |
| } | |
| auto bullet = information.bullet; | |
| if (bullet < rule->limit.bullet) { | |
| return to_the_home; | |
| } | |
| return cruise_mode; | |
| } | |
| auto do_plan() const noexcept { | |
| if (!rule) { | |
| return cruise_mode; // 或返回安全默认值 | |
| } | |
| auto health = information.health; | |
| if (health < rule->limit.health) { | |
| return to_the_home; | |
| } | |
| auto bullet = information.bullet; | |
| if (bullet < rule->limit.bullet) { | |
| return to_the_home; | |
| } | |
| return cruise_mode; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/brain/plan.cc` around lines 29 - 41, do_plan() dereferences rule->limit
without checking rule, causing a potential null dereference; update do_plan() to
guard against rule being null by checking rule (the
std::unique_ptr<brain::Rule>) before accessing rule->limit and return a safe
default (e.g., to_the_home or cruise_mode) when rule is not set; reference the
do_plan() method, the rule unique_ptr, set_rule(), rule->limit,
information.health/bullet, and the return values to_the_home/cruise_mode when
adding the null check and early-return logic.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/brain.cc (1)
137-140:⚠️ Potential issue | 🟠 Major这里把
health连续写了两次，bullet没有刷新。规划逻辑会读取
bullet；第二次仍写health会让bullet保持默认值或上一次的值，后续决策会基于错误数据。🐛 建议修改
auto update = [this](PlanBox::Information& info) { std::ignore = this; info.health = 0.0; - info.health = 0.0; + info.bullet = 0; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/brain.cc` around lines 137 - 140, The lambda update (auto update = [this](PlanBox::Information& info)) erroneously writes info.health twice and never refreshes info.bullet; change the second assignment to update/reset info.bullet (or explicitly clear it to the correct default) so PlanBox::Information.bullet is refreshed before planning reads it, ensuring you still set info.health once and then assign info.bullet appropriately inside update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/brain.cc`:
- Around line 129-133: 当调用
ament_index_cpp::get_package_share_directory(kAppLabel.data()),
YAML::LoadFile(config_file) 或 plan_box.set_rule(config_yaml)
失败时，当前异常缺少包名和配置路径定位信息；请在这段代码周围添加捕获（try/catch）逻辑，捕获所有异常后在抛出或记录前将
kAppLabel、config_file（或 config_path +
"config/rule.yaml"）和原始异常信息一并附加到错误消息中，确保重新抛出带有上下文的异常或使用组件日志记录（包含
get_package_share_directory、YAML::LoadFile、plan_box.set_rule 的调用点信息）以便上线后快速定位问题。
- Around line 115-122: The subscription callback for
Node::create_subscription<Twist> uses an unsupported signature (const
std::unique_ptr<Twist>&) which will fail to compile; change the lambda parameter
to a supported type such as Twist::ConstSharedPtr (or Twist::UniquePtr if you
want ownership) and update the lambda body to dereference the shared/unique
pointer when assigning to command_velocity->x()/y()/z(), and keep setting
command_received_timestamp and has_warning_timeout as before; locate the
subscription_twist creation and replace the callback signature accordingly.
In `@src/brain/plan.hh`:
- Around line 20-24: 模板约束和调用方式不匹配：在 update_information 模板中，改为约束函数类型为
std::invocable<F, Information&> 并使用 std::invoke(std::forward<F>(function),
information_()) 来调用回调（以支持成员函数指针等可调用对象），同时移除或替换无条件 noexcept 为条件
noexcept(noexcept(std::invoke(std::declval<F>(),
std::declval<Information&>())))，并保持在调用后调用 do_plan_().
---
Duplicate comments:
In `@src/brain.cc`:
- Around line 137-140: The lambda update (auto update =
[this](PlanBox::Information& info)) erroneously writes info.health twice and
never refreshes info.bullet; change the second assignment to update/reset
info.bullet (or explicitly clear it to the correct default) so
PlanBox::Information.bullet is refreshed before planning reads it, ensuring you
still set info.health once and then assign info.bullet appropriately inside
update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e24fc76-7ace-400d-9680-c04fd77cd3b2
📒 Files selected for processing (3)
config/plan.yamlsrc/brain.ccsrc/brain/plan.hh
✅ Files skipped from review due to trivial changes (1)
- config/plan.yaml
src/brain.cc
Outdated
Comment on lines
+129
to
+133
| const auto config_path = ament_index_cpp::get_package_share_directory(kAppLabel.data()); | ||
| const auto config_file = std::filesystem::path{config_path} / "config" / "rule.yaml"; | ||
| const auto config_yaml = YAML::LoadFile(config_file); | ||
|
|
||
| plan_box.set_rule(config_yaml); |
There was a problem hiding this comment.
规则配置初始化失败时缺少足够的定位信息。
get_package_share_directory()、LoadFile() 和 set_rule() 任一处抛异常，都会让组件构造直接失败；但当前异常里没有包名或配置路径，上线后很难快速定位是部署问题还是 YAML 内容问题。
🛡️ 建议修改
- const auto config_path = ament_index_cpp::get_package_share_directory(kAppLabel.data());
- const auto config_file = std::filesystem::path{config_path} / "config" / "rule.yaml";
- const auto config_yaml = YAML::LoadFile(config_file);
-
- plan_box.set_rule(config_yaml);
+ try {
+ const auto config_path = ament_index_cpp::get_package_share_directory(kAppLabel.data());
+ const auto config_file = std::filesystem::path{config_path} / "config" / "rule.yaml";
+ const auto config_yaml = YAML::LoadFile(config_file.string());
+ plan_box.set_rule(config_yaml);
+ } catch (const std::exception& e) {
+ throw std::runtime_error{
+ std::format("Failed to initialize planning rules for {}: {}", kAppLabel, e.what())
+ };
+ }📝 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
| const auto config_path = ament_index_cpp::get_package_share_directory(kAppLabel.data()); | |
| const auto config_file = std::filesystem::path{config_path} / "config" / "rule.yaml"; | |
| const auto config_yaml = YAML::LoadFile(config_file); | |
| plan_box.set_rule(config_yaml); | |
| try { | |
| const auto config_path = ament_index_cpp::get_package_share_directory(kAppLabel.data()); | |
| const auto config_file = std::filesystem::path{config_path} / "config" / "rule.yaml"; | |
| const auto config_yaml = YAML::LoadFile(config_file.string()); | |
| plan_box.set_rule(config_yaml); | |
| } catch (const std::exception& e) { | |
| throw std::runtime_error{ | |
| std::format("Failed to initialize planning rules for {}: {}", kAppLabel, e.what()) | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/brain.cc` around lines 129 - 133, 当调用
ament_index_cpp::get_package_share_directory(kAppLabel.data()),
YAML::LoadFile(config_file) 或 plan_box.set_rule(config_yaml)
失败时，当前异常缺少包名和配置路径定位信息；请在这段代码周围添加捕获（try/catch）逻辑，捕获所有异常后在抛出或记录前将
kAppLabel、config_file（或 config_path +
"config/rule.yaml"）和原始异常信息一并附加到错误消息中，确保重新抛出带有上下文的异常或使用组件日志记录（包含
get_package_share_directory、YAML::LoadFile、plan_box.set_rule 的调用点信息）以便上线后快速定位问题。
Comment on lines
+20
to
+24
| template <std::invocable<Information&> F> | ||
| auto update_information(F&& function) noexcept -> void { | ||
| std::forward<F>(function)(information_()); | ||
| do_plan_(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
C++20 std::invocable的判定是否基于std::invoke？如果模板约束是 std::invocable<Information&>，实现里直接写 function(arg)而不是std::invoke(function, arg)，会不会遗漏成员函数指针等合法可调用对象？
💡 Result:
是的，C++20 std::invocable 的判定基于 std::invoke。 std::invocable<F, Args...> 的定义是检查 std::invoke(std::forward(f), std::forward<Args...>(args)...) 是否有效，其中 f 是 F&& 类型，args 是 Args&& 类型。 如果模板约束是 std::invocable<Information&, Arg>（假设 function 是 Information& 类型，arg 是 Arg 类型），则保证 std::invoke(function, arg) 有效，但不保证 function(arg) 有效。 对于成员函数指针（如 &Class::method，类型 Ret(Class::)(Arg)），std::invocable<&Class::method, Class&, Arg> 为 true，因为 std::invoke(&Class::method, obj, arg) 等价于 obj.(&Class::method)(arg) 或 obj.method(arg) 有效。但直接 function(arg) 编译失败，因为成员函数指针没有 operator，也不是普通函数指针。 因此，在实现中使用 function(arg) 而非 std::invoke(function, arg) 会遗漏成员函数指针等合法可调用对象，会导致编译错误。 要正确调用所有满足约束的可调用对象，必须使用 std::invoke(function, arg)。
Citations:
- 1: https://en.cppreference.com/w/cpp/concepts/invocable.html
- 2: https://docs4.w3cub.com/cpp/concepts/invocable/
- 3: https://timsong-cpp.github.io/cppwp/n4861/concepts.callable
- 4: https://en.cppreference.com/w/cpp/utility/functional/invoke.html
- 5: https://devblogs.microsoft.com/oldnewthing/20220401-00/?p=106426
- 6: https://stackoverflow.com/questions/32918679/in-c-11-how-to-invoke-an-arbitrary-callable-object
std::invocable 约束与实际调用方式不一致。
模板约束 std::invocable<Information&> 基于 std::invoke 语义，保证 std::invoke(function, information_()) 有效，但不保证直接调用 function(information_()) 有效。成员函数指针等合法的可调用对象会因为直接调用语法而编译失败。另外，无条件 noexcept 会导致回调抛异常时直接 std::terminate。
修改建议
`#include` "util/pimpl.hh"
+#include <functional>
`#include` <yaml-cpp/yaml.h>
@@
template <std::invocable<Information&> F>
- auto update_information(F&& function) noexcept -> void {
- std::forward<F>(function)(information_());
+ auto update_information(F&& function)
+ noexcept(noexcept(std::invoke(std::forward<F>(function), information_())))
+ -> void {
+ std::invoke(std::forward<F>(function), information_());
do_plan_();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/brain/plan.hh` around lines 20 - 24, 模板约束和调用方式不匹配：在 update_information
模板中，改为约束函数类型为 std::invocable<F, Information&> 并使用
std::invoke(std::forward<F>(function), information_())
来调用回调（以支持成员函数指针等可调用对象），同时移除或替换无条件 noexcept 为条件
noexcept(noexcept(std::invoke(std::declval<F>(),
std::declval<Information&>())))，并保持在调用后调用 do_plan_().
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (3)
src/component/brain/plan.hh (1)
20-23:⚠️ Potential issue | 🟠 Major模板约束和实际调用方式仍然不一致。
std::invocable按std::invoke语义判定，但这里直接function(...)，成员函数指针等合法 callable 仍会编译失败；无条件noexcept也会把回调异常升级成std::terminate。C++20 `std::invocable` 的判定是否基于 `std::invoke`？如果模板约束是 `std::invocable<F, Information&>`，实现里直接写 `function(information_())` 会不会漏掉成员函数指针等合法 callable？无条件 `noexcept` 是否会把回调异常变成 `std::terminate`？🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/brain/plan.hh` around lines 20 - 23, 模板约束和调用需要统一：在 update_information 模板上改用 std::invocable<F, Information&> 作为约束，且不要直接用 function(...) 调用；改为通过 std::invoke(std::forward<F>(function), information_()) 来支持成员函数指针等所有可调用对象，并将 noexcept 改为条件 noexcept(std::is_nothrow_invocable_v<F, Information&>)（或移除 noexcept 如果不确定），保持调用后仍执行 do_plan_().src/component/component.cc (1)
65-72:⚠️ Potential issue | 🔴 Critical订阅回调签名需要改成 rclcpp 支持的类型。
const std::unique_ptr<Twist>&不在create_subscription()支持的回调签名里，这里会直接编译失败。改成Twist::ConstSharedPtr或Twist::UniquePtr。可直接修改成这样
subscription_twist = Node::create_subscription<Twist>( - "/cmd_vel_smoothed", 10, [&, this](const std::unique_ptr<Twist>& msg) { + "/cmd_vel_smoothed", 10, [this](Twist::ConstSharedPtr msg) { command_velocity->x() = msg->linear.x; command_velocity->y() = msg->linear.y; command_velocity->z() = msg->angular.z; command_received_timestamp = std::chrono::steady_clock::now(); has_warning_timeout = false; });ROS 2 rclcpp::Node::create_subscription 支持哪些回调签名？`const std::unique_ptr<geometry_msgs::msg::Twist>&` 是否合法，还是应改用 `geometry_msgs::msg::Twist::ConstSharedPtr` / `UniquePtr`？🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/component.cc` around lines 65 - 72, The subscription callback for subscription_twist uses an unsupported signature (const std::unique_ptr<Twist>&) for Node::create_subscription; change the lambda parameter to a supported type such as Twist::ConstSharedPtr (or Twist::UniquePtr) and update the body to access fields via the pointer (msg->linear.x, msg->linear.y, msg->angular.z), leaving the rest of the logic (setting command_velocity->x/y/z, command_received_timestamp, has_warning_timeout) intact; locate this inside the subscription_twist creation call that invokes Node::create_subscription and replace the parameter type only.src/decision/follow_waypoints_runner.py (1)
177-178:⚠️ Potential issue | 🔴 Critical取消当前 goal 后不要立刻推进到下一跳。
cancel_goal_async()是异步的；旧 goal 的 result callback 之后仍会进入_on_goal_result()。那时current_index已经前移，日志会错位，成功结果还会再次_send_next_goal()，导致多发 waypoint。这里要么等 cancel 完成后再推进，要么给 callback 绑定 goal id/sequence 并忽略过期结果。Also applies to: 180-191, 220-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/decision/follow_waypoints_runner.py` around lines 177 - 178, When cancelling a goal you must avoid advancing current_index or sending the next waypoint until the old goal's cancel has completed or its result is verified as not stale; modify the cancel flow around cancel_goal_async() and the result callback handling so that either (a) you await the cancel future (from cancel_goal_async) before calling _send_next_goal(), or (b) bind the goal's unique id/sequence into the future callback you register on result_future (from get_result_async()/result_future.add_done_callback) and in _on_goal_result ignore results whose goal id/sequence does not match the active goal_id/current_index. Update the code paths that call cancel_goal_async(), _on_goal_result, get_result_async(), result_future.add_done_callback, and _send_next_goal to implement one of these approaches consistently.
🧹 Nitpick comments (6)
src/component/util/rclcpp/node.ccm (2)
9-14:qos命名空间未导出与
prefix命名空间相同，qos命名空间中的预设也未导出，模块外部无法访问。建议修复
namespace rmcs::util { -namespace qos { +export namespace qos { inline const auto debug = rclcpp::QoS{10}.reliable().keep_last(10); inline const auto real_time = rclcpp::QoS{5}.best_effort().keep_last(5); } // namespace qos🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/util/rclcpp/node.ccm` around lines 9 - 14, The qos presets are not exported from the C++ module, so make them available outside the module by exporting them: change the declaration to export the qos namespace or export the individual symbols so other translation units can access debug and real_time (refer to the namespace qos and the inline const auto debug and real_time definitions using rclcpp::QoS{...}). Ensure the export applies to the namespace or to each variable (e.g., export namespace qos { ... } or export inline const auto debug/... ) so the presets are visible outside the module.
15-29: 命名验证工具未导出
check_naming和naming_standard定义在模块内但未使用export关键字，模块外部无法访问这些工具。如果这些工具仅供模块内部使用，建议移至匿名命名空间；如果需要外部使用，需要添加export。如需导出，建议修复
-namespace prefix { +export namespace prefix { -constexpr auto check_naming(std::string_view name) noexcept -> bool { +constexpr auto check_naming(std::string_view name) noexcept -> bool { ... } -constexpr auto naming_standard = "Names must match pattern: ^[a-z0-9_/]+$"; +constexpr auto naming_standard = "Names must match pattern: ^[a-z0-9_/]+$"; } // namespace prefix🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/util/rclcpp/node.ccm` around lines 15 - 29, The helper functions check_naming and naming_standard are defined inside namespace prefix but not exported from the module; either export them for external use or move them into an internal (unnamed) namespace if they should be internal. To export, add the module export keyword (e.g., export constexpr auto check_naming(...) noexcept -> bool and export constexpr auto naming_standard = "...";) inside namespace prefix so callers can access them; otherwise wrap their definitions in an anonymous namespace (or remove namespace prefix and place them in unnamed namespace) to mark them internal. Ensure both check_naming and naming_standard are handled the same way.CMakeLists.txt (2)
56-59: 建议将find_package(pluginlib)与其他依赖声明放在一起为保持一致性和可读性，建议将
find_package(pluginlib REQUIRED)移动到其他find_package语句附近（第 20-29 行区域）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 56 - 59, Move the find_package(pluginlib REQUIRED) call to be grouped with the other find_package declarations near the top of the CMakeLists (so all dependency find_package calls are together); keep pluginlib_export_plugin_description_file(rmcs_executor plugins.xml) where it is, just relocate the find_package line so dependency declarations are consistent and readable.
20-23: PCL 依赖声明后未使用
find_package(PCL REQUIRED)声明了 PCL 依赖，但未在target_include_directories或target_link_libraries中引用。如果是为将来使用而保留，建议添加注释说明（类似 Eigen3 的情况）。💡 建议添加注释或移除未使用的依赖
+# TODO: PCL will be used for point cloud processing in future iterations find_package(PCL REQUIRED)Based on learnings: "In CMakeLists.txt files, when you intentionally include a dependency for future use... add a brief comment explaining the rationale."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 20 - 23, The CMakeLists currently calls find_package(PCL REQUIRED) but never uses it in target_include_directories or target_link_libraries; either remove the unused find_package(PCL REQUIRED) or explicitly consume it: add PCL include/link usage to the appropriate target (e.g., reference PCL include/libs via target_include_directories/target_link_libraries or use the PCL::PCL target) or, if intentional for future work, add a brief comment next to find_package(PCL REQUIRED) explaining why it’s declared now (similar to the Eigen3 comment pattern).launch/custom.launch.py (2)
165-175: 硬编码的world->base_link变换坐标变换坐标
"1.2", "6.3", "0"为硬编码值，这可能是特定场地或机器人的初始位置。建议将这些值提取到配置文件或作为 launch argument 暴露，以提高灵活性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/custom.launch.py` around lines 165 - 175, 当前在 launch 中以静态硬编码方式在 Node(package="tf2_ros", executable="static_transform_publisher", name="world_to_base_link_tf") 的 arguments 列表里使用了 "1.2", "6.3", "0" 作为 world->base_link 初始平移，需将这些值暴露为可配置项；修改方案：为 launch 文件添加 launch arguments（例如 world_x, world_y, world_z 或从 config 参数读取），在创建 Node 时用传入的 LaunchConfiguration/DeclareLaunchArgument 值替换硬编码字符串，并确保为每个参数设置合理默认值和描述以保持向后兼容性。
248-250: 建议添加配置文件异常处理
_load_custom_config在文件不存在或 YAML 格式错误时会抛出未捕获的异常。建议添加适当的错误处理以提供更友好的错误信息。💡 建议添加异常处理
def _load_custom_config(config_file): - with open(config_file, "r", encoding="utf-8") as stream: - return yaml.safe_load(stream) or {} + try: + with open(config_file, "r", encoding="utf-8") as stream: + return yaml.safe_load(stream) or {} + except FileNotFoundError: + print(f"Warning: Config file not found: {config_file}, using defaults") + return {} + except yaml.YAMLError as e: + raise RuntimeError(f"Failed to parse config file {config_file}: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/custom.launch.py` around lines 248 - 250, The helper _load_custom_config currently opens and parses YAML without handling errors; wrap the file open and yaml.safe_load calls in a try/except that catches FileNotFoundError/PermissionError and yaml.YAMLError, and handle each case by either returning an empty dict for missing/unreadable config or raising a clearer exception/logging a user-friendly message that includes the config_file path and the underlying error; make sure the function name _load_custom_config is updated to use the new error handling and preserve returning {} when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@launch/custom.launch.py`:
- Around line 152-155: The LaunchConfiguration object bag_path is being used
directly when building bag_cmd, which embeds the object reference instead of the
actual string; convert LaunchConfiguration values to strings before building the
command by calling bag_path.perform(context) (and similarly call
bag_use_clock.perform(context) before doing .lower()), or alternatively
construct the ExecuteProcess cmd using launch.substitutions (i.e., pass
LaunchConfiguration/substitution objects instead of raw Python strings) so that
ExecuteProcess receives proper substitutions; update the code around bag_cmd,
bag_use_clock, and the ExecuteProcess invocation (where result_actions is
appended) to use one of these two approaches.
In `@src/component/brain/plan.cc`:
- Around line 43-46: The goal_position() implementation is a placeholder
returning NaN via goal_x/goal_y; replace the stub with real logic that reads the
plan's authoritative goal state (e.g., the member variables or methods declared
in plan.hh such as the goal fields or any set_goal/update_goal functions),
compute and assign goal_x and goal_y from that state (or from the current
path/goal index if the class stores path_ or goal_index), validate they are
finite (fallback to a safe default or std::optional/empty result if not), and
return the computed tuple; ensure you update/initialize the underlying goal
storage wherever the class exposes mutators so goal_position() no longer returns
NaN.
- Line 62: PlanBox::do_plan_() currently calls pimpl->do_plan() and discards its
return value; Impl::do_plan() actually returns a plan name (e.g. "to_the_home"
or "cruise_mode") that should be used to run the corresponding callable stored
in plan_map. Change do_plan_() to capture the returned plan key from
Impl::do_plan(), look it up in PlanBox::plan_map (or pimpl->plan_map if stored
in Impl), and invoke the callable if found; add a safe fallback (log or no-op)
when the key is missing or callable is empty and preserve noexcept behavior of
do_plan_().
In `@src/component/brain/plan.hh`:
- Around line 20-23: update_information calls do_plan_ unconditionally which
lets Impl::do_plan access rule->limit.* while PlanBox's rule is null by default;
to fix, add a guard in update_information (or in do_plan_) to check that rule is
set before calling do_plan_ (e.g., return early if rule == nullptr), or make
rule a required constructor parameter so it cannot be null; reference symbols:
update_information, do_plan_(), Impl::do_plan, PlanBox, rule, set_rule().
In `@src/component/util/fsm.hh`:
- Around line 40-75: The code currently allows registering/starting the sentinel
END state and later indexing states_map out of range; forbid END both at
compile-time and runtime: add a compile-time check (e.g., static_assert or
std::enable_if) in the template overloads use<state>() and start_on<state>() to
ensure the template parameter is not state_type::END, and add a runtime guard in
the non-template use(state_type state, ...) and start_on(state_type) functions
that throws or asserts if state == state_type::END before touching states_map
(and include a clear error message referencing END and states_map). Ensure you
update all overloads mentioned (use(state_type,...), use<state>(),
use<state>(on_event), and start_on variants) so END cannot be registered or
started.
- Around line 90-101: spin_once() can dereference a null handler when
states_map.at(std::to_underlying(current_state)).get() returns nullptr; before
calling current_event->on_begin() or ->on_event() check that the handler exists
(and that the map lookup succeeded) and handle the missing-handler case by
logging/throwing a clear error or transitioning to a safe state instead of
dereferencing. Locate spin_once, current_event, states_map, current_state and
add an existence check after lookup (or use find + nullptr check) and
return/handle appropriately; ensure the function still returns whether next ==
StateKey::END only after a valid next is obtained or after handling the error.
In `@src/component/util/rclcpp/node.ccm`:
- Around line 1-6: 该模块文件 src/component/util/rclcpp/node.ccm 未在 CMakeLists.txt
中通过 FILE_SET CXX_MODULES 注册，导致模块不会被编译或导出；打开项目的 CMakeLists.txt，找到定义该库的
add_library(...)（当前只包含 src/component/component.cc），并将 node.ccm 加入到该库的 FILE_SET
CXX_MODULES 列表中（或创建新的 file_set 并在 target_link_libraries/target_sources
中引用），确保模块名 util.rclcpp_node 的编译单元被包含并导出以便链接。
In `@src/component/util/status.hh`:
- Around line 19-39: Both free functions to_string(rmcs_msgs::GameStage) and
to_string(RobotStatus) are defined in a header and must be marked inline to
avoid ODR violations when included in multiple translation units; update the
declarations/definitions of to_string(rmcs_msgs::GameStage) and
to_string(RobotStatus) in this header to be inline (or alternatively move their
definitions into a .cc file), keeping the same logic and noexcept specifier, and
consider returning std::string_view or const char* later as an optional
optimization.
In `@src/decision/decision.py`:
- Around line 43-45: The entrypoint() function currently spins with while True:
continue, causing a 100% CPU busy loop; replace that busy-loop with a blocking
behavior or an explicit unimplemented signal: either raise
NotImplementedError("entrypoint not implemented") to fail fast until real logic
is added, or block without CPU use (e.g., use time.sleep(1) in the loop or
threading.Event().wait() to wait indefinitely) and add the necessary import
(time or threading) and a short comment indicating this is a deliberate
placeholder; update the body of entrypoint() accordingly.
- Around line 17-19: The CruiseMethod class has an invalid type annotation and a
mutable default for waypoints; change the annotation of waypoints to a sequence
of coordinate pairs (e.g. list[tuple[float, float]] or Sequence[Tuple[float,
float]]) and replace the shared empty list default with a dataclasses-safe
default_factory (use field(default_factory=list) from dataclasses); update
imports to include dataclasses.field (and typing.Tuple/Sequence if you choose
those) and keep method_name as-is.
In `@src/decision/follow_waypoints_runner.py`:
- Around line 27-28: _quaternion_to_yaw currently computes yaw using only z and
w components which is only valid when x and y are zero; replace the
implementation to use the full quaternion-to-yaw formula that includes x and y
(yaw = atan2(2*(w*z + x*y), 1 - 2*(y*y + z*z))) and update any other occurrences
of the simplified formula (the other location referenced around lines 209-212)
to use the same full formula; locate and modify the function named
_quaternion_to_yaw and the other identical snippet to accept x, y, z, w (or read
x and y where available) and compute yaw with the full expression so yaw_error
is correct when x/y are nonzero.
In `@src/decision/goal_topic_bridge.py`:
- Around line 31-55: Replace the per-message thread spawn in _on_goal/_send_goal
with a single sequential worker: stop creating a new Thread for every incoming
PoseStamped and instead push msg into a FIFO queue (e.g., collections.deque or
queue.Queue) and have one dedicated worker thread/loop that processes entries
one-by-one; inside that worker perform the entire cancel → wait_for_server →
send_goal_async → handle acceptance lifecycle while holding/using self._lock to
update self._current_goal and self._last_goal so they cannot be interleaved by
concurrent sends, and ensure wait_for_server is called once or from the worker
(not blocking per-message threads) and that _handle_goal_response is invoked
from the serialized worker context or safely updates shared state under the same
lock.
---
Duplicate comments:
In `@src/component/brain/plan.hh`:
- Around line 20-23: 模板约束和调用需要统一：在 update_information 模板上改用 std::invocable<F,
Information&> 作为约束，且不要直接用 function(...) 调用；改为通过
std::invoke(std::forward<F>(function), information_()) 来支持成员函数指针等所有可调用对象，并将
noexcept 改为条件 noexcept(std::is_nothrow_invocable_v<F, Information&>)（或移除
noexcept 如果不确定），保持调用后仍执行 do_plan_().
In `@src/component/component.cc`:
- Around line 65-72: The subscription callback for subscription_twist uses an
unsupported signature (const std::unique_ptr<Twist>&) for
Node::create_subscription; change the lambda parameter to a supported type such
as Twist::ConstSharedPtr (or Twist::UniquePtr) and update the body to access
fields via the pointer (msg->linear.x, msg->linear.y, msg->angular.z), leaving
the rest of the logic (setting command_velocity->x/y/z,
command_received_timestamp, has_warning_timeout) intact; locate this inside the
subscription_twist creation call that invokes Node::create_subscription and
replace the parameter type only.
In `@src/decision/follow_waypoints_runner.py`:
- Around line 177-178: When cancelling a goal you must avoid advancing
current_index or sending the next waypoint until the old goal's cancel has
completed or its result is verified as not stale; modify the cancel flow around
cancel_goal_async() and the result callback handling so that either (a) you
await the cancel future (from cancel_goal_async) before calling
_send_next_goal(), or (b) bind the goal's unique id/sequence into the future
callback you register on result_future (from
get_result_async()/result_future.add_done_callback) and in _on_goal_result
ignore results whose goal id/sequence does not match the active
goal_id/current_index. Update the code paths that call cancel_goal_async(),
_on_goal_result, get_result_async(), result_future.add_done_callback, and
_send_next_goal to implement one of these approaches consistently.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 56-59: Move the find_package(pluginlib REQUIRED) call to be
grouped with the other find_package declarations near the top of the CMakeLists
(so all dependency find_package calls are together); keep
pluginlib_export_plugin_description_file(rmcs_executor plugins.xml) where it is,
just relocate the find_package line so dependency declarations are consistent
and readable.
- Around line 20-23: The CMakeLists currently calls find_package(PCL REQUIRED)
but never uses it in target_include_directories or target_link_libraries; either
remove the unused find_package(PCL REQUIRED) or explicitly consume it: add PCL
include/link usage to the appropriate target (e.g., reference PCL include/libs
via target_include_directories/target_link_libraries or use the PCL::PCL target)
or, if intentional for future work, add a brief comment next to find_package(PCL
REQUIRED) explaining why it’s declared now (similar to the Eigen3 comment
pattern).
In `@launch/custom.launch.py`:
- Around line 165-175: 当前在 launch 中以静态硬编码方式在 Node(package="tf2_ros",
executable="static_transform_publisher", name="world_to_base_link_tf") 的
arguments 列表里使用了 "1.2", "6.3", "0" 作为 world->base_link 初始平移，需将这些值暴露为可配置项；修改方案：为
launch 文件添加 launch arguments（例如 world_x, world_y, world_z 或从 config 参数读取），在创建
Node 时用传入的 LaunchConfiguration/DeclareLaunchArgument
值替换硬编码字符串，并确保为每个参数设置合理默认值和描述以保持向后兼容性。
- Around line 248-250: The helper _load_custom_config currently opens and parses
YAML without handling errors; wrap the file open and yaml.safe_load calls in a
try/except that catches FileNotFoundError/PermissionError and yaml.YAMLError,
and handle each case by either returning an empty dict for missing/unreadable
config or raising a clearer exception/logging a user-friendly message that
includes the config_file path and the underlying error; make sure the function
name _load_custom_config is updated to use the new error handling and preserve
returning {} when appropriate.
In `@src/component/util/rclcpp/node.ccm`:
- Around line 9-14: The qos presets are not exported from the C++ module, so
make them available outside the module by exporting them: change the declaration
to export the qos namespace or export the individual symbols so other
translation units can access debug and real_time (refer to the namespace qos and
the inline const auto debug and real_time definitions using rclcpp::QoS{...}).
Ensure the export applies to the namespace or to each variable (e.g., export
namespace qos { ... } or export inline const auto debug/... ) so the presets are
visible outside the module.
- Around line 15-29: The helper functions check_naming and naming_standard are
defined inside namespace prefix but not exported from the module; either export
them for external use or move them into an internal (unnamed) namespace if they
should be internal. To export, add the module export keyword (e.g., export
constexpr auto check_naming(...) noexcept -> bool and export constexpr auto
naming_standard = "...";) inside namespace prefix so callers can access them;
otherwise wrap their definitions in an anonymous namespace (or remove namespace
prefix and place them in unnamed namespace) to mark them internal. Ensure both
check_naming and naming_standard are handled the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d68694d6-7c4c-4805-b82c-70a200d1b2c9
⛔ Files ignored due to path filters (2)
maps/local_mock.pngis excluded by!**/*.pngmaps/rmul.pngis excluded by!**/*.png
📒 Files selected for processing (20)
CMakeLists.txtcmake/export.cmakeconfig/nav2.yamlconfig/plan.yamllaunch/custom.launch.pylaunch/follow_waypoints.launch.pymaps/local_mock.yamlmaps/rmul.yamlpackage.xmlsrc/component/brain/plan.ccsrc/component/brain/plan.hhsrc/component/brain/rule.hhsrc/component/component.ccsrc/component/util/fsm.hhsrc/component/util/pimpl.hhsrc/component/util/rclcpp/node.ccmsrc/component/util/status.hhsrc/decision/decision.pysrc/decision/follow_waypoints_runner.pysrc/decision/goal_topic_bridge.py
✅ Files skipped from review due to trivial changes (4)
- cmake/export.cmake
- maps/local_mock.yaml
- maps/rmul.yaml
- package.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- config/plan.yaml
- launch/follow_waypoints.launch.py
- config/nav2.yaml
Comment on lines
+152
to
+155
| bag_cmd = ["ros2", "bag", "play", bag_path] | ||
| if bag_use_clock.lower() in {"1", "true", "yes", "on"}: | ||
| bag_cmd.append("--clock") | ||
| result_actions.append(ExecuteProcess(cmd=bag_cmd, output="screen")) |
There was a problem hiding this comment.
bag_path 未正确转换为字符串
bag_path 是 LaunchConfiguration 对象，在第 57 行未调用 .perform(context)，但在第 152 行直接用于构建命令列表。这会导致命令中包含对象引用而非实际路径字符串。
🐛 建议修复
- bag_path = LaunchConfiguration("bag_path")
+ bag_path = LaunchConfiguration("bag_path").perform(context)或者使用 launch.substitutions 的方式处理：
if config["use_bag"]:
- bag_cmd = ["ros2", "bag", "play", bag_path]
+ bag_cmd = ["ros2", "bag", "play", LaunchConfiguration("bag_path")]
if bag_use_clock.lower() in {"1", "true", "yes", "on"}:
bag_cmd.append("--clock")
- result_actions.append(ExecuteProcess(cmd=bag_cmd, output="screen"))
+ result_actions.append(ExecuteProcess(
+ cmd=bag_cmd,
+ output="screen",
+ shell=False,
+ ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@launch/custom.launch.py` around lines 152 - 155, The LaunchConfiguration
object bag_path is being used directly when building bag_cmd, which embeds the
object reference instead of the actual string; convert LaunchConfiguration
values to strings before building the command by calling
bag_path.perform(context) (and similarly call bag_use_clock.perform(context)
before doing .lower()), or alternatively construct the ExecuteProcess cmd using
launch.substitutions (i.e., pass LaunchConfiguration/substitution objects
instead of raw Python strings) so that ExecuteProcess receives proper
substitutions; update the code around bag_cmd, bag_use_clock, and the
ExecuteProcess invocation (where result_actions is appended) to use one of these
two approaches.
src/component/brain/plan.cc
Outdated
Comment on lines
+43
to
+46
| auto goal_position() noexcept { | ||
| // ...... | ||
| return std::tuple{goal_x, goal_y}; | ||
| } |
There was a problem hiding this comment.
goal_position() 实现不完整
方法体包含占位符注释 // ......，且 goal_x/goal_y 始终返回 NaN。根据 plan.hh 中的公共接口，没有任何方法会更新这些坐标值，导致 goal_position() 永远返回无效数据。
需要我帮助实现目标位置更新逻辑吗？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/brain/plan.cc` around lines 43 - 46, The goal_position()
implementation is a placeholder returning NaN via goal_x/goal_y; replace the
stub with real logic that reads the plan's authoritative goal state (e.g., the
member variables or methods declared in plan.hh such as the goal fields or any
set_goal/update_goal functions), compute and assign goal_x and goal_y from that
state (or from the current path/goal index if the class stores path_ or
goal_index), validate they are finite (fallback to a safe default or
std::optional/empty result if not), and return the computed tuple; ensure you
update/initialize the underlying goal storage wherever the class exposes
mutators so goal_position() no longer returns NaN.
src/component/brain/plan.cc
Outdated
| return pimpl->goal_position(); | ||
| } | ||
|
|
||
| auto PlanBox::do_plan_() noexcept -> void { pimpl->do_plan(); } |
There was a problem hiding this comment.
do_plan() 返回值被丢弃
Impl::do_plan() 返回计划名称 (to_the_home 或 cruise_mode)，但 do_plan_() 调用时未使用该返回值。考虑到 plan_map 中存储了对应的 callable，返回值可能应该用于执行相应的计划操作。
💡 建议使用返回值执行计划
-auto PlanBox::do_plan_() noexcept -> void { pimpl->do_plan(); }
+auto PlanBox::do_plan_() noexcept -> void {
+ auto plan_name = pimpl->do_plan();
+ if (auto it = pimpl->plan_map.find(plan_name); it != pimpl->plan_map.end()) {
+ it->second();
+ }
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/brain/plan.cc` at line 62, PlanBox::do_plan_() currently calls
pimpl->do_plan() and discards its return value; Impl::do_plan() actually returns
a plan name (e.g. "to_the_home" or "cruise_mode") that should be used to run the
corresponding callable stored in plan_map. Change do_plan_() to capture the
returned plan key from Impl::do_plan(), look it up in PlanBox::plan_map (or
pimpl->plan_map if stored in Impl), and invoke the callable if found; add a safe
fallback (log or no-op) when the key is missing or callable is empty and
preserve noexcept behavior of do_plan_().
Comment on lines
+20
to
+23
| template <std::invocable<Information&> F> | ||
| auto update_information(F&& function) noexcept -> void { | ||
| std::forward<F>(function)(information_()); | ||
| do_plan_(); |
There was a problem hiding this comment.
set_rule() 之前调用 update_information() 会直接撞空指针。
src/component/brain/plan.cc 里的 Impl::do_plan() 直接读取 rule->limit.*，但 PlanBox 默认构造时 rule 还是空的；这里每次更新信息都无条件 do_plan_()，首次调用若早于 set_rule() 就会崩。请在规划前做 guard，或者把 rule 变成构造必需项。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/brain/plan.hh` around lines 20 - 23, update_information calls
do_plan_ unconditionally which lets Impl::do_plan access rule->limit.* while
PlanBox's rule is null by default; to fix, add a guard in update_information (or
in do_plan_) to check that rule is set before calling do_plan_ (e.g., return
early if rule == nullptr), or make rule a required constructor parameter so it
cannot be null; reference symbols: update_information, do_plan_(),
Impl::do_plan, PlanBox, rule, set_rule().
Comment on lines
+40
to
+75
| auto use(state_type state, on_begin_type&& on_begin, on_event_type&& on_event) { | ||
| static_assert( | ||
| std::is_invocable_r_v<void, on_begin_type>, "on_begin must be callable returning void"); | ||
| static_assert( | ||
| std::is_invocable_r_v<state_type, on_event_type>, | ||
| "on_event must be callable returning state_type"); | ||
|
|
||
| struct State final : IState { | ||
| [[no_unique_address]] std::decay_t<on_begin_type> impl_on_begin; | ||
| [[no_unique_address]] std::decay_t<on_event_type> impl_on_event; | ||
|
|
||
| explicit State(on_begin_type&& on_begin, on_event_type&& on_event) | ||
| : impl_on_begin{std::forward<on_begin_type>(on_begin)} | ||
| , impl_on_event{std::forward<on_event_type>(on_event)} {} | ||
|
|
||
| ~State() override = default; | ||
|
|
||
| auto on_begin() -> void override { impl_on_begin(); } | ||
| auto on_event() -> state_type override { return impl_on_event(); } | ||
| }; | ||
| states_map.at(std::to_underlying(state)) = std::make_unique<State>( | ||
| std::forward<on_begin_type>(on_begin), std::forward<on_event_type>(on_event)); | ||
| } | ||
|
|
||
| template <state_type state> | ||
| auto use(auto&& on_begin, auto&& on_event) { | ||
| using _1 = decltype(on_begin); | ||
| using _2 = decltype(on_event); | ||
| use(state, std::forward<_1>(on_begin), std::forward<_2>(on_event)); | ||
| } | ||
|
|
||
| template <state_type state> | ||
| auto use(auto&& on_event) { | ||
| using _1 = decltype(on_event); | ||
| use(state, [] {}, std::forward<_1>(on_event)); | ||
| } |
There was a problem hiding this comment.
请显式禁止将 END 当作可注册/可启动状态。
当前 use(...) 与 start_on(...) 没有拦截 END，会在后续索引 states_map 时触发越界访问路径（例如 Line 60/Line 92）。建议在运行时和编译期都加保护，避免把 sentinel 误当正常状态使用。
🔧 建议修复
+#include <stdexcept>
+
template <typename on_begin_type, typename on_event_type>
auto use(state_type state, on_begin_type&& on_begin, on_event_type&& on_event) {
+ if (state == StateKey::END) {
+ throw std::invalid_argument("END is a sentinel and cannot be registered");
+ }
static_assert(
std::is_invocable_r_v<void, on_begin_type>, "on_begin must be callable returning void");
@@
template <state_type state>
auto use(auto&& on_begin, auto&& on_event) {
+ static_assert(state != StateKey::END, "END is a sentinel and cannot be registered");
using _1 = decltype(on_begin);
using _2 = decltype(on_event);
use(state, std::forward<_1>(on_begin), std::forward<_2>(on_event));
}
@@
auto start_on(state_type state) {
+ if (state == StateKey::END) {
+ throw std::invalid_argument("cannot start on END");
+ }
current_state = state;
current_event = nullptr;
}Based on learnings: 在 rmcs-navigation 项目的 FSM 实现中，END 是 sentinel 终止标记，不是需要注册处理器的正常状态。
Also applies to: 84-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/util/fsm.hh` around lines 40 - 75, The code currently allows
registering/starting the sentinel END state and later indexing states_map out of
range; forbid END both at compile-time and runtime: add a compile-time check
(e.g., static_assert or std::enable_if) in the template overloads use<state>()
and start_on<state>() to ensure the template parameter is not state_type::END,
and add a runtime guard in the non-template use(state_type state, ...) and
start_on(state_type) functions that throws or asserts if state ==
state_type::END before touching states_map (and include a clear error message
referencing END and states_map). Ensure you update all overloads mentioned
(use(state_type,...), use<state>(), use<state>(on_event), and start_on variants)
so END cannot be registered or started.
src/component/util/status.hh
Outdated
Comment on lines
+19
to
+39
| auto to_string(rmcs_msgs::GameStage stage) noexcept -> std::string { | ||
| using rmcs_msgs::GameStage; | ||
| switch (stage) { | ||
| case GameStage::NOT_START: return "NotStart"; | ||
| case GameStage::PREPARATION: return "Preparation"; | ||
| case GameStage::REFEREE_CHECK: return "RefereeCheck"; | ||
| case GameStage::COUNTDOWN: return "Countdown"; | ||
| case GameStage::STARTED: return "Started"; | ||
| case GameStage::SETTLING: return "Settling"; | ||
| case GameStage::UNKNOWN: return "Unknown"; | ||
| } | ||
| return "Unknown"; | ||
| } | ||
| auto to_string(RobotStatus status) noexcept -> std::string { | ||
| switch (status) { | ||
| case RobotStatus::Unknown: return "Unknown"; | ||
| case RobotStatus::Normal: return "Normal"; | ||
| case RobotStatus::Invincible: return "Invincible"; | ||
| case RobotStatus::Deaded: return "Deaded"; | ||
| } | ||
| return "Unknown"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
C++ 头文件中定义非 inline的自由函数会不会导致 multiple definition？返回std::string的函数标记为noexcept是否安全，还是应改为返回字面量类型（如const char */std::string_view）？
💡 Result:
是的，在C++头文件中定义非inline的自由函数会导致multiple definition错误。这违反了One Definition Rule (ODR)，因为头文件被多个翻译单元包含时，非inline函数会有多个定义，而链接器无法处理重复符号。只有标记为inline的函数、类内定义的成员函数、模板等才允许在头文件中定义并在多个TU中出现，前提是所有定义相同。 对于返回std::string的函数标记为noexcept是安全的。std::string的构造函数、赋值、析构等操作在标准库实现中不抛异常（除了可能的前置条件违反，如length_error，但noexcept主要用于优化和传播不抛异常信息）。cppreference显示std::string的许多操作是noexcept的，返回std::string（通过RVO或move）也不会引入异常。标记noexcept可以启用更好的优化，如在noexcept函数中使用。 如果函数总是返回相同的内容（如常量字符串），使用std::string_view或const char*更高效，避免构造std::string的开销。但对于一般情况，返回std::string并标记noexcept是标准且安全的做法。
Citations:
- 1: https://en.cppreference.com/w/cpp/language/inline
- 2: https://en.cppreference.com/w/cpp/language/definition.html
- 3: https://stackoverflow.com/questions/68186806/why-does-defining-functions-in-header-files-create-multiple-definition-errors-b
- 4: https://stackoverflow.com/questions/212006/multiple-definition-error-including-c-header-file-with-inline-code-from-multip
- 5: https://stackoverflow.com/questions/34614523/multiple-definition-error-c
- 6: https://stackoverflow.com/questions/65460620/how-to-properly-create-a-header-for-functions-without-multiple-definitions-erro
- 7: https://www.cppreference.com/w/cpp/string/basic_string.html
- 8: https://www.cppreference.com/w/cpp/string/basic_string/basic_string.html
- 9: https://www.modernescpp.com/index.php/c-core-guidelines-the-noexcept-specifier-and-operator/
- 10: https://stackoverflow.com/questions/66603543/why-those-two-stdstring-find-functions-are-declared-as-noexcept
这两个 to_string() 函数需要在头文件中标记为 inline。
它们现在是头文件里的自由函数定义，多个翻译单元同时包含时会产生重复定义，违反单一定义规则（ODR）。需要加上 inline 关键字，或者将定义移到 .cc 文件中。
关于 noexcept 和返回 std::string：标准库中 std::string 的构造、赋值和移动操作都是安全的，不会在正常情况下抛异常，因此返回 std::string 并标记 noexcept 是安全且符合 C++ 最佳实践的。
如果函数总是返回相同内容，可以考虑改为返回 std::string_view 或 const char* 以避免构造开销，但这是可选的性能优化，不是正确性问题。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/util/status.hh` around lines 19 - 39, Both free functions
to_string(rmcs_msgs::GameStage) and to_string(RobotStatus) are defined in a
header and must be marked inline to avoid ODR violations when included in
multiple translation units; update the declarations/definitions of
to_string(rmcs_msgs::GameStage) and to_string(RobotStatus) in this header to be
inline (or alternatively move their definitions into a .cc file), keeping the
same logic and noexcept specifier, and consider returning std::string_view or
const char* later as an optional optimization.
src/decision/decision.py
Outdated
Comment on lines
+17
to
+19
| class CruiseMethod: | ||
| method_name: str = "" | ||
| waypoints: list[float, float] = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 内置泛型 list 是否只接受一个类型参数？list[float, float]在运行时会怎样？dataclass 字段为什么要用field(default_factory=list)而不是=[]？
💡 Result:
是，Python内置泛型list只接受一个类型参数。list[float, float]在运行时会引发TypeError: Too many parameters for typing.List; actual 2, expected 1（尽管是内置泛型，但泛型别名行为类似），因为list是单参数泛型（PEP 585和typing规范）。 dataclass字段使用field(default_factory=list)而不是=[]，是因为可变默认值问题：=[]创建单个共享列表实例，所有对象修改同一列表（Python类属性默认行为）。default_factory=list为每个实例调用list创建新列表，避免共享状态bug（dataclasses文档明确禁止直接可变默认，并强制使用default_factory）。
Citations:
- 1: https://stackoverflow.com/questions/53526516/why-cant-list-contain-multiple-types
- 2: https://docs.python.org/3/library/typing.html
- 3: https://docs.python.org/3.11/library/dataclasses.html
- 4: https://stackoverflow.com/questions/53632152/why-cant-dataclasses-have-mutable-defaults-in-their-class-attributes-declaratio
- 5: https://peps.python.org/pep-0557/
- 6: https://typing.python.org/en/latest/spec/generics.html
🏁 Script executed:
cat -n src/decision/decision.pyRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 1201
修复 CruiseMethod.waypoints 的类型注解和默认值。
list[float, float] 不是合法的泛型写法——list 只接受一个类型参数，代码将在导入时抛出 TypeError。同时 = [] 会导致所有实例共享同一个列表对象。请改为正确的类型注解（如 list[tuple[float, float]]），并使用 field(default_factory=list)。
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 19-19: Do not use mutable default values for dataclass attributes
(RUF008)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/decision/decision.py` around lines 17 - 19, The CruiseMethod class has an
invalid type annotation and a mutable default for waypoints; change the
annotation of waypoints to a sequence of coordinate pairs (e.g.
list[tuple[float, float]] or Sequence[Tuple[float, float]]) and replace the
shared empty list default with a dataclasses-safe default_factory (use
field(default_factory=list) from dataclasses); update imports to include
dataclasses.field (and typing.Tuple/Sequence if you choose those) and keep
method_name as-is.
Comment on lines
+27
to
+28
| def _quaternion_to_yaw(z: float, w: float) -> float: | ||
| return math.atan2(2.0 * w * z, 1.0 - 2.0 * z * z) |
There was a problem hiding this comment.
Yaw 计算少了四元数的 x/y 分量。
当前公式只在 x == 0 && y == 0 的平面四元数上成立；odometry 一旦带有非零 x/y，yaw_error 就会被算错，导致 waypoint 提前或延后判定到达。这里应使用完整四元数公式计算 yaw。
Also applies to: 209-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/decision/follow_waypoints_runner.py` around lines 27 - 28,
_quaternion_to_yaw currently computes yaw using only z and w components which is
only valid when x and y are zero; replace the implementation to use the full
quaternion-to-yaw formula that includes x and y (yaw = atan2(2*(w*z + x*y), 1 -
2*(y*y + z*z))) and update any other occurrences of the simplified formula (the
other location referenced around lines 209-212) to use the same full formula;
locate and modify the function named _quaternion_to_yaw and the other identical
snippet to accept x, y, z, w (or read x and y where available) and compute yaw
with the full expression so yaw_error is correct when x/y are nonzero.
Comment on lines
+31
to
+55
| def _on_goal(self, msg: PoseStamped) -> None: | ||
| threading.Thread( | ||
| target=self._send_goal, | ||
| args=(msg,), | ||
| daemon=True, | ||
| ).start() | ||
|
|
||
| def _send_goal(self, msg: PoseStamped) -> None: | ||
| self._client.wait_for_server() | ||
|
|
||
| with self._lock: | ||
| current_goal = self._current_goal | ||
|
|
||
| if current_goal is not None: | ||
| cancel_future = current_goal.cancel_goal_async() | ||
| cancel_future.add_done_callback(lambda _: None) | ||
|
|
||
| goal = NavigateToPose.Goal() | ||
| goal.pose = msg | ||
| goal.behavior_tree = "" | ||
| with self._lock: | ||
| self._last_goal = msg | ||
|
|
||
| send_future = self._client.send_goal_async(goal) | ||
| send_future.add_done_callback(self._handle_goal_response) |
There was a problem hiding this comment.
把 goal 发送串行化，别为每条消息新建一个线程。
两个目标几乎同时到达时，会并发执行 _send_goal()；哪个 response 最后回来，就会把 _current_goal 回写成哪个 handle，_last_goal 的日志也会对错目标，后续 cancel 很容易打到旧 goal。更糟的是 action server 未起来时，wait_for_server() 会让这些线程一直堆积。这里更适合单个 worker/队列，或者把整个 cancel → send → accept 生命周期串行化。
Also applies to: 57-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/decision/goal_topic_bridge.py` around lines 31 - 55, Replace the
per-message thread spawn in _on_goal/_send_goal with a single sequential worker:
stop creating a new Thread for every incoming PoseStamped and instead push msg
into a FIFO queue (e.g., collections.deque or queue.Queue) and have one
dedicated worker thread/loop that processes entries one-by-one; inside that
worker perform the entire cancel → wait_for_server → send_goal_async → handle
acceptance lifecycle while holding/using self._lock to update self._current_goal
and self._last_goal so they cannot be interleaved by concurrent sends, and
ensure wait_for_server is called once or from the worker (not blocking
per-message threads) and that _handle_goal_response is invoked from the
serialized worker context or safely updates shared state under the same lock.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/decision/decision.py (2)
17-21:⚠️ Potential issue | 🔴 Critical修复
waypoints的类型注解与默认值；当前写法会导致导入期异常和实例间共享状态。Line 20 的
list[float, float]不是合法写法（list只接受一个类型参数），且=[]是可变默认值，会在实例间共享。建议修改
-from dataclasses import dataclass +from dataclasses import dataclass, field `@dataclass` class CruiseMethod: method_name: str = "" - waypoints: list[float, float] = [] + waypoints: list[tuple[float, float]] = field(default_factory=list)#!/bin/bash # 验证当前代码是否命中问题模式，并验证 Python 对该泛型写法的行为 rg -n 'waypoints:\s*list\[float,\s*float\]\s*=\s*\[\]' src/decision/decision.py python - <<'PY' try: eval("list[float, float]") print("Unexpected: accepted") except TypeError as e: print("Expected TypeError:", e) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/decision/decision.py` around lines 17 - 21, The CruiseMethod dataclass has an invalid type annotation and a mutable default for waypoints: replace the annotation list[float, float] with a correct type (e.g., list[tuple[float, float]] or list[Tuple[float, float]]/Sequence[Tuple[float, float]]) and stop using a shared mutable default; set the default to None or use dataclasses.field(default_factory=list) instead. Update imports accordingly (from dataclasses import field and from typing import Tuple or Sequence if using those) and change the waypoints declaration in CruiseMethod to use the new type and safe default.
44-46:⚠️ Potential issue | 🟠 Major
entrypoint()的 busy loop 会持续占满 CPU。Line 45-46 当前实现会空转。建议改为阻塞等待或直接
raise NotImplementedError(...)。建议修改（fail-fast 占位）
def entrypoint() -> None: - while True: - continue + raise NotImplementedError("entrypoint is not implemented yet")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/decision/decision.py` around lines 44 - 46, entrypoint() currently contains a busy loop that spins CPU indefinitely; replace the while True: continue with a non-spin placeholder such as a blocking wait or a fail-fast stub by raising NotImplementedError("entrypoint not implemented") (or use an appropriate blocking primitive for your runtime) so the function no longer consumes CPU while unimplemented; locate the entrypoint function in decision.py and make this change.src/component/component.cc (1)
70-77:⚠️ Potential issue | 🔴 Critical订阅回调签名仍然不符合 rclcpp 的支持列表。
这和之前指出的问题一致。
AnySubscriptionCallbackPossibleTypes支持的是const MessageT&、std::shared_ptr<const MessageT>/const std::shared_ptr<const MessageT>&、以及按值接收的UniquePtr（及带MessageInfo的变体）；const std::unique_ptr<Twist>&不在列表里，所以这里还是会在模板实例化时失败。 (docs.ros.org)建议修改
subscription_twist = Node::create_subscription<Twist>( - "/cmd_vel_smoothed", 10, [&, this](const std::unique_ptr<Twist>& msg) { + "/cmd_vel_smoothed", 10, [this](Twist::ConstSharedPtr msg) { command_velocity->x() = msg->linear.x; command_velocity->y() = msg->linear.y; command_velocity->z() = msg->angular.z; command_received_timestamp = std::chrono::steady_clock::now(); has_warning_timeout = false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/component.cc` around lines 70 - 77, The subscription callback signature is invalid for rclcpp: replace the current parameter type "const std::unique_ptr<Twist>&" in the Node::create_subscription call (subscription_twist) with a supported type such as Twist::UniquePtr (or std::unique_ptr<Twist> by value) or std::shared_ptr<const Twist> or const Twist&; update the lambda parameter and its uses accordingly (e.g., use msg->linear.x when keeping a pointer type, or msg.linear.x if switching to const Twist&) so the callback matches rclcpp's AnySubscriptionCallbackPossibleTypes.
🧹 Nitpick comments (3)
src/decision/decision.py (2)
23-24: 占位函数建议显式失败（NotImplementedError），避免“静默成功”。
pass会让调用方误以为功能正常执行，调试成本更高。建议修改
def setup_cruise_methods(): - pass + raise NotImplementedError("setup_cruise_methods is not implemented yet") @@ def update_blackboard(combat_status: str): - pass + raise NotImplementedError("update_blackboard is not implemented yet")Also applies to: 32-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/decision/decision.py` around lines 23 - 24, The placeholder function setup_cruise_methods (and any other stubbed functions around lines 32-33) should raise NotImplementedError instead of using pass so callers fail fast; locate the setup_cruise_methods definition in decision.py (and the other empty stubs nearby), replace the body with a raised NotImplementedError including a short message like "setup_cruise_methods not implemented" to make missing implementation explicit and aid debugging.
27-29: 模块导入即写入Blackboard会产生全局副作用，建议改为显式初始化。当前导入该模块就会修改全局状态，不利于测试隔离与复用。
建议修改方向
-Blackboard.set("health", 0) -Blackboard.set("bullet", 0) -Blackboard.set("cruise_method", "occupation") +def initialize_blackboard_defaults() -> None: + Blackboard.set("health", 0) + Blackboard.set("bullet", 0) + Blackboard.set("cruise_method", "occupation")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/decision/decision.py` around lines 27 - 29, The module currently performs global writes to Blackboard at import time (Blackboard.set("health", 0), Blackboard.set("bullet", 0), Blackboard.set("cruise_method", "occupation")), causing side effects; refactor by removing those top-level Blackboard.set calls and add an explicit initializer function (e.g., initialize_blackboard_defaults or setup_blackboard_defaults) that performs those three Blackboard.set operations, then ensure callers (application entrypoint or relevant class constructor) invoke that initializer where appropriate; keep the same keys ("health", "bullet", "cruise_method") and values so behavior is unchanged but no writes happen on import.launch/nav2.launch.py (1)
109-109: 优化列表拼接写法根据静态分析工具建议，使用解包语法更符合 Python 惯例。
♻️ 建议修改
- remappings=remappings + [("cmd_vel", "cmd_vel_nav")], + remappings=[*remappings, ("cmd_vel", "cmd_vel_nav")],同样适用于第 125 行和第 149 行。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/nav2.launch.py` at line 109, 将使用列表加法拼接 remappings 的写法（出现于 remappings=remappings + [("cmd_vel", "cmd_vel_nav")] 以及类似第125行和第149行的表达式）替换为解包语法，将 remappings = remappings + [...] 改为 remappings = [*remappings, ("cmd_vel", "cmd_vel_nav")]（并对第125行和第149行对应的元组做同样修改），以遵循 Python 列表解包惯例并提高可读性。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@launch/custom.launch.py`:
- Around line 261-263: The _load_custom_config function currently opens
config_file without handling missing files; catch FileNotFoundError around the
open/read in _load_custom_config and either return an empty dict as a safe
default or re-raise a clearer exception using the config_file path in the
message so launch failures show a helpful error; update references inside
_load_custom_config (the open(...) call and the yaml.safe_load(...) return)
accordingly.
In `@launch/nav2.launch.py`:
- Around line 27-41: The RewrittenYaml param_rewrites use incorrect keys so the
nested map_topic entries in nav2.yaml aren't being matched; update the
controller_params and planner_params RewrittenYaml blocks to rewrite the full
dot-separated nested parameter paths instead of "map_topic" (e.g. use
"local_costmap.local_costmap.ros__parameters.static_layer.map_topic" for
controller_params and
"global_costmap.global_costmap.ros__parameters.static_layer.map_topic" for
planner_params), keeping convert_types=True and mapping each full path to the
corresponding launch-time topic variable (local_map_topic and global_map_topic).
In `@src/component/component.cc`:
- Around line 60-63: The output registration currently seeds command_velocity
with NaNs (using kNan) which causes downstream consumers to briefly observe NaN
before the first /cmd_vel_smoothed arrives; change the initial registered value
passed to Component::register_output for name "/{}/command_velocity" (and the
other occurrences at the noted locations) from Eigen::Vector3d{kNan, kNan, kNan}
to Eigen::Vector3d::Zero(), and keep a separate boolean/state (e.g., a
"has_command" flag tied to command_received_timestamp or a new member) to
represent "no command received yet", leaving update() logic to emit zeros until
a real command arrives or timeout occurs.
- Around line 1-15: 把原来带发行版路径的头改为可移植形式并补上缺失的标准头：在 component.cc 中把 `#include`
<eigen3/Eigen/Geometry> 改为 `#include` <Eigen/Geometry>，并显式添加所需的标准库头 `#include`
<format>, `#include` <chrono>, `#include` <atomic>, `#include` <string_view>, `#include`
<limits>（这些符号由 std::format/
std::format_string、std::chrono::steady_clock::time_point、std::atomic<bool>、std::string_view、std::numeric_limits
提供，kNan 使用了 numeric_limits）。同时在 CMake 列表中为该目标在 target_link_libraries 中加入
Eigen3::Eigen 以传播 Eigen 的 include 目录。确保修改后能编译且不依赖发行版特定 include 路径。
In `@src/decision/decision.py`:
- Around line 36-41: The MoveTo behaviour currently doesn't follow the
py_trees.Behaviour contract: update() must return a py_trees.common.Status and
__init__ must call the base constructor to set the node name. Fix __init__ in
class MoveTo by calling super().__init__(name="MoveTo") and initialize any
needed instance state there; implement update(self) to return one of
py_trees.common.Status values (e.g., Status.RUNNING, Status.SUCCESS or
Status.FAILURE) based on the node's logic (use py_trees.common.Status to
reference the enum) rather than returning None, and ensure any state transitions
(e.g., setting succeeded/failed conditions) are reflected by the returned
Status.
---
Duplicate comments:
In `@src/component/component.cc`:
- Around line 70-77: The subscription callback signature is invalid for rclcpp:
replace the current parameter type "const std::unique_ptr<Twist>&" in the
Node::create_subscription call (subscription_twist) with a supported type such
as Twist::UniquePtr (or std::unique_ptr<Twist> by value) or
std::shared_ptr<const Twist> or const Twist&; update the lambda parameter and
its uses accordingly (e.g., use msg->linear.x when keeping a pointer type, or
msg.linear.x if switching to const Twist&) so the callback matches rclcpp's
AnySubscriptionCallbackPossibleTypes.
In `@src/decision/decision.py`:
- Around line 17-21: The CruiseMethod dataclass has an invalid type annotation
and a mutable default for waypoints: replace the annotation list[float, float]
with a correct type (e.g., list[tuple[float, float]] or list[Tuple[float,
float]]/Sequence[Tuple[float, float]]) and stop using a shared mutable default;
set the default to None or use dataclasses.field(default_factory=list) instead.
Update imports accordingly (from dataclasses import field and from typing import
Tuple or Sequence if using those) and change the waypoints declaration in
CruiseMethod to use the new type and safe default.
- Around line 44-46: entrypoint() currently contains a busy loop that spins CPU
indefinitely; replace the while True: continue with a non-spin placeholder such
as a blocking wait or a fail-fast stub by raising
NotImplementedError("entrypoint not implemented") (or use an appropriate
blocking primitive for your runtime) so the function no longer consumes CPU
while unimplemented; locate the entrypoint function in decision.py and make this
change.
---
Nitpick comments:
In `@launch/nav2.launch.py`:
- Line 109: 将使用列表加法拼接 remappings 的写法（出现于 remappings=remappings + [("cmd_vel",
"cmd_vel_nav")] 以及类似第125行和第149行的表达式）替换为解包语法，将 remappings = remappings + [...] 改为
remappings = [*remappings, ("cmd_vel",
"cmd_vel_nav")]（并对第125行和第149行对应的元组做同样修改），以遵循 Python 列表解包惯例并提高可读性。
In `@src/decision/decision.py`:
- Around line 23-24: The placeholder function setup_cruise_methods (and any
other stubbed functions around lines 32-33) should raise NotImplementedError
instead of using pass so callers fail fast; locate the setup_cruise_methods
definition in decision.py (and the other empty stubs nearby), replace the body
with a raised NotImplementedError including a short message like
"setup_cruise_methods not implemented" to make missing implementation explicit
and aid debugging.
- Around line 27-29: The module currently performs global writes to Blackboard
at import time (Blackboard.set("health", 0), Blackboard.set("bullet", 0),
Blackboard.set("cruise_method", "occupation")), causing side effects; refactor
by removing those top-level Blackboard.set calls and add an explicit initializer
function (e.g., initialize_blackboard_defaults or setup_blackboard_defaults)
that performs those three Blackboard.set operations, then ensure callers
(application entrypoint or relevant class constructor) invoke that initializer
where appropriate; keep the same keys ("health", "bullet", "cruise_method") and
values so behavior is unchanged but no writes happen on import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40982dcd-e85f-468d-a067-79df4e614402
📒 Files selected for processing (5)
config/nav2.yamllaunch/custom.launch.pylaunch/nav2.launch.pysrc/component/component.ccsrc/decision/decision.py
✅ Files skipped from review due to trivial changes (1)
- config/nav2.yaml
Comment on lines
+261
to
+263
| def _load_custom_config(config_file): | ||
| with open(config_file, "r", encoding="utf-8") as stream: | ||
| return yaml.safe_load(stream) or {} |
There was a problem hiding this comment.
缺少文件不存在的错误处理
当配置文件不存在时，open() 会抛出 FileNotFoundError，导致 launch 过程失败且错误信息不够清晰。建议添加错误处理或更明确的错误提示。
🛡️ 建议修复
def _load_custom_config(config_file):
+ if not os.path.exists(config_file):
+ raise FileNotFoundError(
+ f"Configuration file not found: {config_file}"
+ )
with open(config_file, "r", encoding="utf-8") as stream:
return yaml.safe_load(stream) or {}或者返回默认空配置：
def _load_custom_config(config_file):
+ if not os.path.exists(config_file):
+ return {}
with open(config_file, "r", encoding="utf-8") as stream:
return yaml.safe_load(stream) or {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@launch/custom.launch.py` around lines 261 - 263, The _load_custom_config
function currently opens config_file without handling missing files; catch
FileNotFoundError around the open/read in _load_custom_config and either return
an empty dict as a safe default or re-raise a clearer exception using the
config_file path in the message so launch failures show a helpful error; update
references inside _load_custom_config (the open(...) call and the
yaml.safe_load(...) return) accordingly.
Comment on lines
+27
to
+41
| controller_params = RewrittenYaml( | ||
| source_file=params_file, | ||
| param_rewrites={ | ||
| "map_topic": local_map_topic, | ||
| }, | ||
| convert_types=True, | ||
| ) | ||
|
|
||
| planner_params = RewrittenYaml( | ||
| source_file=params_file, | ||
| param_rewrites={ | ||
| "map_topic": global_map_topic, | ||
| }, | ||
| convert_types=True, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Nav2 RewrittenYaml param_rewrites nested parameters
💡 Result:
In Nav2, RewrittenYaml from nav2_common.launch is a launch Substitution that modifies a YAML parameter file by overriding specific leaf values based on param_rewrites dict during launch. For nested parameters, use dot-separated full paths in param_rewrites keys matching the YAML structure. Key mechanism from source code: It loads the YAML, collects all leaf keys using getYamlLeafKeys (recursive traversal yielding dot-separated paths like 'local_costmap.local_costmap.obstacle_layer.scan.topic'), and in substitute_params/add_params, if a path matches a param_rewrites key, updates the leaf value. Example usage in launch file: from nav2_common.launch import RewrittenYaml params_file = 'path/to/nav2_params.yaml' rewritten_params = RewrittenYaml( source_file=params_file, param_rewrites={ 'use_sim_time': use_sim_time, # top-level 'local_costmap.local_costmap.obstacle_layer.scan.topic': 'robot_name/scan', # nested 'controller_server.rosparameters.controller_frequency': '20.0' }, convert_types=True ) Then pass rewritten_params to Node(parameters=[rewritten_params]). Common issues (from GitHub #4308): Top-level params like robot_base_frame work, but deeply nested ones fail if paths incorrect or root_key mis-set (root_key scopes substitution). Without root_key='', it searches entire YAML. For per-node params under 'node_name.rosparameters', use full path. This matches Nav2 bringup launches overriding use_sim_time etc. across nodes. Source confirms leaf-only replacement via paths.
Citations:
- 1: Rewritten Yaml nested parameters ros-navigation/navigation2#4308
- 2: RewrittenYaml - living in our launchfiles, waiting to show their full potential? ros-navigation/navigation2#2117
- 3: https://github.com/ros-navigation/navigation2/blob/main/nav2_common/nav2_common/launch/rewritten_yaml.py
- 4: https://api.nav2.org/nav2-humble/html/rewritten__yaml_8py_source.html
🏁 Script executed:
# Find the nav2.yaml file
find . -name "nav2.yaml" -o -name "nav2_params.yaml" | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 96
🏁 Script executed:
# Also search for the launch file mentioned
find . -name "nav2.launch.py" | head -10Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 101
🏁 Script executed:
# Search for RewrittenYaml usage patterns in launch files
rg "RewrittenYaml" --type py -B 5 -A 10 | head -100Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 2406
🏁 Script executed:
# Examine the nav2.yaml file structure
cat ./config/nav2.yaml | head -200Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 5673
🏁 Script executed:
# Search specifically for map_topic in the yaml file
rg "map_topic" ./config/nav2.yaml -B 3 -A 1Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 581
修复 RewrittenYaml 中的嵌套参数路径错误
RewrittenYaml 的 param_rewrites 支持嵌套参数，但需要使用完整的点分隔路径。当前使用 "map_topic" 作为 key 无法匹配 nav2.yaml 中的实际参数位置，因为 map_topic 位于深层嵌套结构中：
local_costmap.local_costmap.ros__parameters.static_layer.map_topicglobal_costmap.global_costmap.ros__parameters.static_layer.map_topic
当前配置会静默失败，导致 Nav2 节点的 costmap 订阅错误的地图话题。
🐛 修复方案
controller_params = RewrittenYaml(
source_file=params_file,
param_rewrites={
- "map_topic": local_map_topic,
+ "local_costmap.local_costmap.ros__parameters.static_layer.map_topic": local_map_topic,
},
convert_types=True,
)
planner_params = RewrittenYaml(
source_file=params_file,
param_rewrites={
- "map_topic": global_map_topic,
+ "global_costmap.global_costmap.ros__parameters.static_layer.map_topic": global_map_topic,
},
convert_types=True,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@launch/nav2.launch.py` around lines 27 - 41, The RewrittenYaml param_rewrites
use incorrect keys so the nested map_topic entries in nav2.yaml aren't being
matched; update the controller_params and planner_params RewrittenYaml blocks to
rewrite the full dot-separated nested parameter paths instead of "map_topic"
(e.g. use "local_costmap.local_costmap.ros__parameters.static_layer.map_topic"
for controller_params and
"global_costmap.global_costmap.ros__parameters.static_layer.map_topic" for
planner_params), keeping convert_types=True and mapping each full path to the
corresponding launch-time topic variable (local_map_topic and global_map_topic).
src/component/component.cc
Outdated
Comment on lines
+1
to
+15
| #include <eigen3/Eigen/Geometry> | ||
| #include <geometry_msgs/msg/twist.hpp> | ||
| #include <rclcpp/node.hpp> | ||
| #include <rclcpp/subscription.hpp> | ||
| #include <std_msgs/msg/string.hpp> | ||
|
|
||
| #include <rmcs_executor/component.hpp> | ||
| #include <rmcs_msgs/game_stage.hpp> | ||
|
|
||
| #include <yaml-cpp/yaml.h> | ||
|
|
||
| namespace rmcs { | ||
|
|
||
| constexpr auto kRosLabel = std::string_view{"rmcs_navigation"}; | ||
| constexpr auto kNan = std::numeric_limits<double>::quiet_NaN(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure and find the relevant files
git ls-files | grep -E "(CMakeLists|component\.cc)" | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 139
🏁 Script executed:
# Let's examine the actual src/component/component.cc file to see all includes and what's actually used
find . -name "component.cc" -type fRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 106
🏁 Script executed:
# Check if there are any other source files to understand the project structure
find . -name "*.cc" -o -name "*.cpp" | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 164
🏁 Script executed:
# Look for CMakeLists.txt to understand Eigen configuration
find . -name "CMakeLists.txt" -type fRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 116
🏁 Script executed:
# Read the component.cc file to see the includes and usage
wc -l ./src/component/component.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 109
🏁 Script executed:
# Read the full component.cc file
cat -n ./src/component/component.cc | head -150Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 4436
🏁 Script executed:
# Read the CMakeLists.txt to see how Eigen is configured
cat -n ./CMakeLists.txtRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 2016
🏁 Script executed:
# Verify if Eigen3 should be linked - check if it's header-only
# and check what the proper Eigen3 include convention is
rg "Eigen3::Eigen|eigen3" --type cmakeRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 60
🏁 Script executed:
# Check if there are any other files using Eigen to understand the convention
rg "#include.*[Ee]igen" --type cc --type hppRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 104
🏁 Script executed:
# Check other Eigen usage in the codebase
rg "#include.*[Ee]igen"Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 137
🏁 Script executed:
# Check the actual Eigen include convention in project
rg "Eigen"Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 439
🏁 Script executed:
# Let's verify the Eigen documentation convention by checking what modern CMake Eigen3 packages typically provide
# First, let's see if there are any build artifacts or configuration files
ls -la | grep -E "build|cmake"Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 299
🏁 Script executed:
# Check what include path Eigen3 modern CMake packages typically provide
# by looking at how other ROS2 projects might use it
web_search "Eigen3 CMake find_package include path convention"Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 128
🏁 Script executed:
# Let's also verify if Eigen3 needs to be linked explicitly
# Check if there's a linking issue
rg "target_link_libraries" CMakeLists.txt -A 10Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 365
🌐 Web query:
Eigen3 CMake find_package include path convention header-only
💡 Result:
The standard convention for using Eigen3 with CMake's find_package, particularly for its header-only nature, is: 1. Use find_package(Eigen3 REQUIRED NO_MODULE) (CMake 3.5+ required). 2. Link your target to the imported INTERFACE target Eigen3::Eigen via target_link_libraries(your_target Eigen3::Eigen). 3. Do NOT manually add include directories. The Eigen3::Eigen target has INTERFACE_INCLUDE_DIRECTORIES set to the Eigen headers directory (typically the parent of the 'Eigen/' subdirectory, e.g., /usr/include/eigen3), so linking propagates the include path automatically. This allows #include <Eigen/Dense> in your code. Eigen must be built/installed first to generate Eigen3Config.cmake (despite being header-only), usually placing headers in /include/eigen3/Eigen and config in /share/eigen3/cmake. If not installed, manual include_directories or FetchContent with manual INTERFACE target setup is an alternative, but find_package is the official convention per Eigen docs.
Citations:
- 1: https://eigen.tuxfamily.org/dox/TopicCMakeGuide.html
- 2: https://libeigen.gitlab.io/eigen/docs-nightly/TopicCMakeGuide.html
- 3: https://eigen.tuxfamily.org/dox-3.3/TopicCMakeGuide.html
- 4: http://eigen.tuxfamily.org/dox/TopicCMakeGuide.html
- 5: https://gitlab.com/libeigen/eigen/-/blob/3af8c262acdfb50fbfc88e6c640d5d1a582145a9/cmake/Eigen3Config.cmake.in
- 6: https://eigen.googlesource.com/mirror/+/943fdc71c60e8ac2f6eda9c990c50bca06841b9b/CMakeLists.txt
把头文件依赖改成可移植且自洽的形式。
#include <eigen3/Eigen/Geometry> 把发行版目录结构写进了源码，当前静态分析也已经在这里报了找不到头文件；Eigen 文档给出的标准做法是使用 CMake 的 find_package(Eigen3 REQUIRED) 后，在代码中用 #include <Eigen/Geometry> 而不是带 eigen3/ 前缀的路径。另外，这个 TU 直接用了 std::format、std::format_string、std::chrono::steady_clock::time_point、std::atomic<bool>、std::string_view 和 std::numeric_limits，它们分别在标准库的 <format>、<chrono>、<atomic>、<string_view> 和 <limits> 头里声明，当前文件没有显式包含。建议一起修掉，避免构建结果依赖特定安装布局和传递包含。
同时注意 CMakeLists.txt 第 47-55 行的 target_link_libraries 没有链接 Eigen3::Eigen，即使 Eigen 是纯头文件库，也需要通过链接目标来传播 CMake 的 include 目录配置。 (eigen.tuxfamily.org)
🧰 Tools
🪛 Clang (14.0.6)
[error] 1-1: 'eigen3/Eigen/Geometry' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/component.cc` around lines 1 - 15,
把原来带发行版路径的头改为可移植形式并补上缺失的标准头：在 component.cc 中把 `#include` <eigen3/Eigen/Geometry>
改为 `#include` <Eigen/Geometry>，并显式添加所需的标准库头 `#include` <format>, `#include` <chrono>,
`#include` <atomic>, `#include` <string_view>, `#include` <limits>（这些符号由 std::format/
std::format_string、std::chrono::steady_clock::time_point、std::atomic<bool>、std::string_view、std::numeric_limits
提供，kNan 使用了 numeric_limits）。同时在 CMake 列表中为该目标在 target_link_libraries 中加入
Eigen3::Eigen 以传播 Eigen 的 include 目录。确保修改后能编译且不依赖发行版特定 include 路径。
src/component/component.cc
Outdated
Comment on lines
+60
to
+63
| const auto name = std::format("/{}/command_velocity", kRosLabel); | ||
| const auto vec_nan = Eigen::Vector3d{kNan, kNan, kNan}; | ||
| Component::register_output(name, command_velocity, vec_nan); | ||
|
|
There was a problem hiding this comment.
启动阶段会短暂向下游暴露 NaN 速度。
command_velocity 在注册时被初始化成 NaN，但 command_received_timestamp 又在构造函数里被设为 now()。因此在首条 /cmd_vel_smoothed 到来前的前 100ms 内，update() 不会进入超时分支，下游读到的不是安全的 0 速度而是 NaN。建议把初始输出设为 Eigen::Vector3d::Zero()，并单独用状态位区分“尚未收到命令”。
Also applies to: 79-80, 82-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/component.cc` around lines 60 - 63, The output registration
currently seeds command_velocity with NaNs (using kNan) which causes downstream
consumers to briefly observe NaN before the first /cmd_vel_smoothed arrives;
change the initial registered value passed to Component::register_output for
name "/{}/command_velocity" (and the other occurrences at the noted locations)
from Eigen::Vector3d{kNan, kNan, kNan} to Eigen::Vector3d::Zero(), and keep a
separate boolean/state (e.g., a "has_command" flag tied to
command_received_timestamp or a new member) to represent "no command received
yet", leaving update() logic to emit zeros until a real command arrives or
timeout occurs.
src/decision/decision.py
Outdated
Comment on lines
+36
to
+41
| class MoveTo(Behaviour): | ||
| def __init__(self) -> None: | ||
| pass | ||
|
|
||
| def update(self) -> None: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
在 py_trees 中，继承 py_trees.behaviour.Behaviour 时，__init__ 需要如何调用父类？update() 的返回值类型和可用状态枚举是什么？
💡 Result:
在 py_trees 中，继承 py_trees.behaviour.Behaviour 时，init 方法需要调用 super.init(name)，其中 name 是字符串参数，用于设置行为名称。这是官方文档和示例中标准做法。 update 方法的返回值类型是 py_trees.common.Status，必须返回 RUNNING、SUCCESS 或 FAILURE（INVALID 是初始默认状态，不应由 update 返回）。 可用状态枚举 py_trees.common.Status 包括： - SUCCESS: 行为检查通过或动作成功完成。 - FAILURE: 行为检查失败或动作失败。 - RUNNING: 行为正在执行中，结果待定。 - INVALID: 未初始化或非活动状态。
Citations:
- 1: https://py-trees.readthedocs.io/en/devel/_modules/py_trees/behaviour.html
- 2: https://py-trees.readthedocs.io/en/devel/_modules/py_trees/behaviours.html
- 3: https://py-trees.readthedocs.io/en/devel/behaviours.html
- 4: http://docs.ros.org/en/melodic/api/py_trees/html/_modules/py_trees/common.html
- 5: https://py-trees.readthedocs.io/en/devel/_modules/py_trees/common.html
🏁 Script executed:
cat -n src/decision/decision.py | head -50Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 1209
修复 MoveTo 类违反行为树基类契约的问题
MoveTo 类未正确实现 py_trees.behaviour.Behaviour 接口：
__init__()未调用super().__init__(name="MoveTo")，行为节点名称无法初始化update()返回类型为None，但必须返回py_trees.common.Status（SUCCESS、FAILURE 或 RUNNING）
修改建议
+from py_trees.common import Status
@@
class MoveTo(Behaviour):
def __init__(self) -> None:
- pass
+ super().__init__(name="MoveTo")
- def update(self) -> None:
- pass
+ def update(self) -> Status:
+ return Status.RUNNING📝 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
| class MoveTo(Behaviour): | |
| def __init__(self) -> None: | |
| pass | |
| def update(self) -> None: | |
| pass | |
| from py_trees.common import Status | |
| class MoveTo(Behaviour): | |
| def __init__(self) -> None: | |
| super().__init__(name="MoveTo") | |
| def update(self) -> Status: | |
| return Status.RUNNING |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/decision/decision.py` around lines 36 - 41, The MoveTo behaviour
currently doesn't follow the py_trees.Behaviour contract: update() must return a
py_trees.common.Status and __init__ must call the base constructor to set the
node name. Fix __init__ in class MoveTo by calling
super().__init__(name="MoveTo") and initialize any needed instance state there;
implement update(self) to return one of py_trees.common.Status values (e.g.,
Status.RUNNING, Status.SUCCESS or Status.FAILURE) based on the node's logic (use
py_trees.common.Status to reference the enum) rather than returning None, and
ensure any state transitions (e.g., setting succeeded/failed conditions) are
reflected by the returned Status.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
CMakeLists.txt (3)
20-23: 建议为预留依赖添加用途说明。
Eigen3和PCL已声明但在当前组件代码中未见使用。根据项目的过往实践，若这些依赖是为未来功能预留，应添加注释说明其预期用途。💡 建议的修改
+# TODO: Eigen3 will be used for pose transformations and matrix operations find_package(Eigen3 REQUIRED) find_package(yaml-cpp REQUIRED) find_package(rclcpp REQUIRED) +# TODO: PCL will be used for point cloud processing in obstacle detection find_package(PCL REQUIRED)基于 learnings：在 CMakeLists.txt 文件中，当有意包含供将来使用的依赖项时，应添加简短注释说明理由或用 TODO 记录其预期用途。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 20 - 23, The CMakeLists.txt declares find_package(Eigen3 REQUIRED) and find_package(PCL REQUIRED) but the code currently doesn't use them; add a short inline comment next to these find_package lines (or a TODO above them) explaining their intended future use (e.g., "reserved for future linear algebra ops / sensor point-cloud processing") so reviewers know they are intentionally predeclared; update the lines referencing Eigen3 and PCL in CMakeLists.txt accordingly to include that explanatory comment.
59-62: 建议将find_package(pluginlib)与其他依赖声明放在一起。
find_package(pluginlib REQUIRED)位于库目标定义之后（第 59 行），而其他所有find_package调用都集中在第 20-30 行。虽然功能上没有问题，但将所有依赖查找语句放在一起可以提高可读性和可维护性。♻️ 建议的修改
将第 59 行移动到第 30 行之后：
find_package(rmcs_executor REQUIRED) +find_package(pluginlib REQUIRED) # RMCS Component add_library(${PROJECT_NAME} SHARED)并移除原来的位置：
) -find_package(pluginlib REQUIRED) pluginlib_export_plugin_description_file(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 59 - 62, Move the dependency discovery call find_package(pluginlib REQUIRED) to join the other find_package calls (grouped around the existing block at lines ~20-30) so all dependency declarations are together; keep the pluginlib_export_plugin_description_file(rmcs_executor plugins.xml) call where it is or immediately after, and remove the original find_package(pluginlib REQUIRED) from its current location so there is only one declaration. Ensure you reference the same package name (pluginlib) and keep rmcs_executor and plugins.xml unchanged.
7-8: 建议为 CMake 策略设置添加注释说明。
CMP0167和CMP0144策略设置缺少解释性注释。这些策略会影响FindBoost行为和find_package()大小写敏感性，添加注释可以帮助后续维护者理解为什么需要这些特定设置。💡 建议的修改
-set(CMAKE_POLICY_DEFAULT_CMP0167 OLD) -set(CMAKE_POLICY_DEFAULT_CMP0144 NEW) +# CMP0167: Use legacy FindBoost module behavior for compatibility +set(CMAKE_POLICY_DEFAULT_CMP0167 OLD) +# CMP0144: find_package() uses upper-case <PACKAGENAME>_ROOT variables +set(CMAKE_POLICY_DEFAULT_CMP0144 NEW)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 7 - 8, The two CMake policy settings set(CMAKE_POLICY_DEFAULT_CMP0167 OLD) and set(CMAKE_POLICY_DEFAULT_CMP0144 NEW) lack explanatory comments; update the CMakeLists to add brief comments next to these set(...) lines explaining what each policy controls (e.g., CMP0167 affects FindBoost target semantics/namespace behavior and why OLD is chosen, CMP0144 controls case-sensitivity of find_package() and why NEW is required) and include the rationale or link to the CMake policy documentation so future maintainers understand why these specific values were forced.launch/custom.launch.py (1)
180-197: 考虑将初始位姿参数化
world->base_link的初始位姿硬编码为(1.2, 6.3, 0)，这可能仅适用于特定环境。建议将其提取为 launch 参数，便于不同场景下复用。♻️ 建议修改
在
generate_launch_description()中添加参数：DeclareLaunchArgument("initial_x", default_value="1.2"), DeclareLaunchArgument("initial_y", default_value="6.3"),在
_build_mode_actions中使用：+ initial_x = LaunchConfiguration("initial_x").perform(context) + initial_y = LaunchConfiguration("initial_y").perform(context) ... arguments=[ - "1.2", "6.3", "0", + initial_x, initial_y, "0", "0", "0", "0", "world", "base_link" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/custom.launch.py` around lines 180 - 197, Extract the hardcoded world->base_link pose into launch arguments: add DeclareLaunchArgument entries (e.g., "initial_x" and "initial_y", with defaults "1.2" and "6.3") in generate_launch_description(), then read them where _build_mode_actions constructs initial_base_link and replace the literal "1.2" and "6.3" with the launch substitutions (use LaunchConfiguration for "initial_x" and "initial_y"); keep the TimerAction/initial_base_link node creation and arguments order unchanged and ensure types remain strings for the static_transform_publisher arguments.launch/nav2.launch.py (1)
115-115: 考虑使用列表解包语法替代列表拼接静态分析工具建议使用
[*remappings, ("cmd_vel", "cmd_vel_nav")]替代remappings + [...]，这种写法更符合 Python 惯用风格且性能略优。同样适用于第 131 行和第 155 行。♻️ 建议修改
- remappings=remappings + [("cmd_vel", "cmd_vel_nav")], + remappings=[*remappings, ("cmd_vel", "cmd_vel_nav")],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/nav2.launch.py` at line 115, Replace the list concatenations that build remappings (e.g., occurrences of remappings + [("cmd_vel", "cmd_vel_nav")] and the similar patterns used later) with list-unpacking style to follow Python idioms and improve performance; locate the expressions that append tuples to the existing remappings list (references: the variable remappings and the concatenation expressions) and change them to use [*remappings, ("cmd_vel", "cmd_vel_nav")] (and analogous replacements for the other two concatenations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@launch/custom.launch.py`:
- Around line 78-91: The launch sets enable_local_map_node to the same value as
use_local_mock, which starts the live local_map node when a static local mock is
intended; update the nav2_actions launch_arguments so enable_local_map_node is
the boolean inverse of use_local_mock (i.e., when use_local_mock is "true" set
enable_local_map_node to "false" and vice‑versa), ensuring you convert the
inverted value back to the expected string (e.g., "true"/"false") before
inserting it into the dict for IncludeLaunchDescription; adjust the code around
the symbols use_local_mock and enable_local_map_node inside nav2_actions
accordingly.
- Around line 245-247: The current assignment of bag_path_default uses
str(bag_config.get("path")) which turns None into the literal "None"; change it
to safely handle missing values by reading the raw value (via
bag_config.get("path", None) or checking if bag_config.get("path") is None) and
only convert to string when a non-None path exists (e.g., set bag_path_default
to "" or None when missing), updating related logic that consumes
bag_path_default to treat empty/None as "no bag file" instead of a filename;
refer to bag_config, bag_path_default, and bag_use_clock_default to locate and
adjust the code paths that expect a valid bag path.
In `@src/component/util/command.hh`:
- Around line 22-24: The Command::gimbal_mode member is not initialized in
explicit Command(const YAML::Node& yaml), causing undefined behavior when later
read and passed to set_gimbal_mode; fix by initializing gimbal_mode inside the
Command(const YAML::Node&) constructor (parse the YAML node to a valid
GimbalMode or set a safe default) and ensure any YAML parsing errors fallback to
that default or throw, so set_gimbal_mode always receives a well-defined enum
value.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 20-23: The CMakeLists.txt declares find_package(Eigen3 REQUIRED)
and find_package(PCL REQUIRED) but the code currently doesn't use them; add a
short inline comment next to these find_package lines (or a TODO above them)
explaining their intended future use (e.g., "reserved for future linear algebra
ops / sensor point-cloud processing") so reviewers know they are intentionally
predeclared; update the lines referencing Eigen3 and PCL in CMakeLists.txt
accordingly to include that explanatory comment.
- Around line 59-62: Move the dependency discovery call find_package(pluginlib
REQUIRED) to join the other find_package calls (grouped around the existing
block at lines ~20-30) so all dependency declarations are together; keep the
pluginlib_export_plugin_description_file(rmcs_executor plugins.xml) call where
it is or immediately after, and remove the original find_package(pluginlib
REQUIRED) from its current location so there is only one declaration. Ensure you
reference the same package name (pluginlib) and keep rmcs_executor and
plugins.xml unchanged.
- Around line 7-8: The two CMake policy settings
set(CMAKE_POLICY_DEFAULT_CMP0167 OLD) and set(CMAKE_POLICY_DEFAULT_CMP0144 NEW)
lack explanatory comments; update the CMakeLists to add brief comments next to
these set(...) lines explaining what each policy controls (e.g., CMP0167 affects
FindBoost target semantics/namespace behavior and why OLD is chosen, CMP0144
controls case-sensitivity of find_package() and why NEW is required) and include
the rationale or link to the CMake policy documentation so future maintainers
understand why these specific values were forced.
In `@launch/custom.launch.py`:
- Around line 180-197: Extract the hardcoded world->base_link pose into launch
arguments: add DeclareLaunchArgument entries (e.g., "initial_x" and "initial_y",
with defaults "1.2" and "6.3") in generate_launch_description(), then read them
where _build_mode_actions constructs initial_base_link and replace the literal
"1.2" and "6.3" with the launch substitutions (use LaunchConfiguration for
"initial_x" and "initial_y"); keep the TimerAction/initial_base_link node
creation and arguments order unchanged and ensure types remain strings for the
static_transform_publisher arguments.
In `@launch/nav2.launch.py`:
- Line 115: Replace the list concatenations that build remappings (e.g.,
occurrences of remappings + [("cmd_vel", "cmd_vel_nav")] and the similar
patterns used later) with list-unpacking style to follow Python idioms and
improve performance; locate the expressions that append tuples to the existing
remappings list (references: the variable remappings and the concatenation
expressions) and change them to use [*remappings, ("cmd_vel", "cmd_vel_nav")]
(and analogous replacements for the other two concatenations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6256561-2ac2-4f8c-8b3c-a394015c04aa
⛔ Files ignored due to path filters (1)
maps/empty.pngis excluded by!**/*.png
📒 Files selected for processing (8)
CMakeLists.txtconfig/nav2.yamllaunch/custom.launch.pylaunch/nav2.launch.pymaps/empty.yamlplugins.xmlsrc/component/component.ccsrc/component/util/command.hh
✅ Files skipped from review due to trivial changes (3)
- plugins.xml
- maps/empty.yaml
- config/nav2.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/component/component.cc
Comment on lines
+245
to
+247
| bag_config = custom_config.get("bag", {}) | ||
| bag_path_default = str(bag_config.get("path")) | ||
| bag_use_clock_default = str(bag_config.get("use_clock", True)).lower() |
There was a problem hiding this comment.
bag_path_default 可能变为 "None" 字符串
当 custom.yaml 中没有配置 bag.path 时，bag_config.get("path") 返回 None，str(None) 会变成字符串 "None"，导致 bag 模式下尝试播放名为 "None" 的文件。
🛡️ 建议修复
bag_config = custom_config.get("bag", {})
- bag_path_default = str(bag_config.get("path"))
+ bag_path_default = bag_config.get("path", "")
+ if bag_path_default is None:
+ bag_path_default = ""
bag_use_clock_default = str(bag_config.get("use_clock", True)).lower()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@launch/custom.launch.py` around lines 245 - 247, The current assignment of
bag_path_default uses str(bag_config.get("path")) which turns None into the
literal "None"; change it to safely handle missing values by reading the raw
value (via bag_config.get("path", None) or checking if bag_config.get("path") is
None) and only convert to string when a non-None path exists (e.g., set
bag_path_default to "" or None when missing), updating related logic that
consumes bag_path_default to treat empty/None as "no bag file" instead of a
filename; refer to bag_config, bag_path_default, and bag_use_clock_default to
locate and adjust the code paths that expect a valid bag path.
src/component/util/command.hh
Outdated
Comment on lines
+22
to
+24
| GimbalMode gimbal_mode; | ||
|
|
||
| explicit Command(const YAML::Node& yaml) {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "command.hh" | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 109
🏁 Script executed:
find . -type f -name "component.cc" | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 106
🏁 Script executed:
wc -l ./src/component/util/command.hh ./src/component/component.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 159
🏁 Script executed:
sed -n '15,35p' ./src/component/util/command.hhRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 327
🏁 Script executed:
sed -n '115,135p' ./src/component/component.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 589
🏁 Script executed:
rg "from_string" ./src/component/util/command.hh -A 3 -B 3Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 269
🏁 Script executed:
sed -n '1,30p' ./src/component/util/command.hhRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 614
gimbal_mode 未初始化即被读取，存在未定义行为。
Command(const YAML::Node&) 构造函数为空，gimbal_mode 成员没有初始化；而 src/component/component.cc 第 123 行立即读取该字段并传给 set_gimbal_mode。这会在运行时读取未初始化的枚举值，导致未定义行为。
建议修复
struct Command {
- GimbalMode gimbal_mode;
+ GimbalMode gimbal_mode = GimbalMode::Unknown;
- explicit Command(const YAML::Node& yaml) {}
+ explicit Command(const YAML::Node& yaml) {
+ if (const auto mode = yaml["gimbal_mode"]; mode && mode.IsScalar()) {
+ from_string(mode.as<std::string>(), gimbal_mode);
+ }
+ }
};📝 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
| GimbalMode gimbal_mode; | |
| explicit Command(const YAML::Node& yaml) {} | |
| GimbalMode gimbal_mode = GimbalMode::Unknown; | |
| explicit Command(const YAML::Node& yaml) { | |
| if (const auto mode = yaml["gimbal_mode"]; mode && mode.IsScalar()) { | |
| from_string(mode.as<std::string>(), gimbal_mode); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/util/command.hh` around lines 22 - 24, The Command::gimbal_mode
member is not initialized in explicit Command(const YAML::Node& yaml), causing
undefined behavior when later read and passed to set_gimbal_mode; fix by
initializing gimbal_mode inside the Command(const YAML::Node&) constructor
(parse the YAML node to a valid GimbalMode or set a safe default) and ensure any
YAML parsing errors fallback to that default or throw, so set_gimbal_mode always
receives a well-defined enum value.
creeper5820
force-pushed
the
wip/framework
branch
from
f48d380 to
a323f2c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RMCS 导航系统初始框架开发
项目构建与包元信息
核心 C++ 组件
ROS2 启动与 Nav2 集成
Python 决策与运行时节点
配置与资源
测试与示例
文档与开发规范
公共接口/导出变更（要点）