Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
漫步该PR重构了导航系统的架构，将单一的launch/config组织方式分解为分层的传感器（sensor）和运动规划（motion）子系统。移除了旧的Python辅助脚本和样例配置，并将导航API从 变更
序列图sequenceDiagram
participant Lua as Lua端点
participant API as Navigation API
participant Nav2 as Nav2导航栈
participant TF as TF变换
Lua->>API: send_target(x,y)
API->>Nav2: 发送NavigateToPose动作目标
Nav2->>Nav2: 规划+控制+速度平滑
Lua->>API: check_position()
API->>TF: 查询world→base_link变换
TF-->>API: 返回位置/四元数
API->>API: 计算偏航角
API-->>Lua: 返回(x,y,yaw)
Lua->>API: switch_topic_forward(true)
API->>API: 订阅/move_base_simple/goal及/goal_pose
Note over API: 转发PoseStamped至send_target()
sequenceDiagram
participant Lua as Lua端点
participant Sensor as sensor.launch
participant Motion as motion.launch
participant Nav2 as Nav2生命周期
Lua->>Lua: 解析config表(launch_livox,launch_odin1,...)
Lua->>Sensor: 启动传感器launch(参数传入)
Sensor->>Sensor: 条件启动驱动、地图服务器
Sensor->>Nav2: 生命周期管理器监控
Lua->>Motion: 启动运动launch(参数传入)
Motion->>Nav2: 启动BT导航、规划器、控制器
Motion->>Nav2: 加载motion.yaml/motion.xml配置
预计代码审查工作量🎯 4 (复杂) | ⏱️ ~60 分钟 可能相关的PR
诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🧹 Nitpick comments (11)
CMakeLists.txt (1)
72-87: 建议：使用 CMake 属性替代-rdynamic硬编码
-rdynamic是 GCC/Clang 特有的链接器标志，直接放在target_link_libraries中可能影响跨平台兼容性。♻️ 建议使用 CMake 原生方式
target_link_libraries( ${PROJECT_NAME} PRIVATE ${LUA_LIBRARIES} sol2::sol2 - -rdynamic rclcpp::rclcpp # ... ) + +set_target_properties(${PROJECT_NAME} PROPERTIES ENABLE_EXPORTS TRUE)或使用
target_link_options：target_link_options(${PROJECT_NAME} PRIVATE $<$<CXX_COMPILER_ID:GNU,Clang>:-rdynamic> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 72 - 87, The hardcoded linker flag "-rdynamic" in the target_link_libraries PRIVATE list breaks cross-platform portability; remove "-rdynamic" from that list and instead add a target_link_options call for the same target (e.g., the target referenced by ${PROJECT_NAME} or the same target used with target_link_libraries) that conditionally applies $<$<CXX_COMPILER_ID:GNU,Clang>:-rdynamic> so the option is only set for GCC/Clang compilers.src/lua/util/native.lua (1)
33-41: 建议：检查run_command的返回值从
src/lua/api.lua:37-68的调用可以看到，api.restart_navigation中调用util.run_command(packed_command)时未检查返回值。如果ros2 launch命令失败，错误将被静默忽略，screen 会话可能处于不完整状态。单引号转义逻辑 (
'"'"') 是正确的 shell 技术，bash -lc也确保了登录 shell 环境被加载。♻️ 建议在调用处检查返回值
在
src/lua/api.lua中：- util.run_command(packed_command) + local success, msg = util.run_command(packed_command) + if not success then + api.warn("restart_navigation failed: " .. msg) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/util/native.lua` around lines 33 - 41, The call site for util.run_command in api.restart_navigation must check its return values so failing `ros2 launch` isn't silently ignored: update the call in api.restart_navigation to capture the returned boolean and message from M.run_command (or util.run_command), if the boolean is false log the error message (or return an error to the caller) and avoid proceeding with assuming the screen session started; also consider cleaning up or killing any partially created screen session on failure. Ensure you reference the M.run_command return contract (true/"ok" on success, false/<message> on failure) when implementing the check and error handling.src/cxx/util/node_mixin.hh (2)
34-35: 建议：改进错误消息的可读性异常消息
"lack of param"可以包含更多上下文信息以便调试。♻️ 建议改进异常消息
self.fuck("param [ {} ] for {} is needed", name, self.get_name()); - throw std::runtime_error{"lack of param"}; + throw std::runtime_error{std::format("param [ {} ] for {} is needed", name, self.get_name())};需要在文件顶部添加
#include <stdexcept>(如果尚未包含)。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cxx/util/node_mixin.hh` around lines 34 - 35, 将抛出的 std::runtime_error 错误信息改为包含参数名和节点名以便调试：在抛出位置（当前的 throw std::runtime_error{"lack of param"};）构建更有上下文的消息（例如把 name 和 self.get_name() 拼入错误字符串），并确保文件头部包含 `#include` <stdexcept> 如果尚未包含；保持现有的 self.fuck 调用不变，只替换 runtime_error 的消息内容以包含详细上下文。
29-36: 建议：简化param<T>()逻辑当前实现中
get_parameter_or<T>(name, T{})的默认值T{}永远不会被使用，因为has_parameter(name)为false时会直接抛出异常。♻️ 建议简化实现
template <typename T> auto param(this const auto& self, const std::string& name) -> T { - if (self.has_parameter(name)) - return self.template get_parameter_or<T>(name, T{}); + if (self.has_parameter(name)) { + T value{}; + self.get_parameter(name, value); + return value; + } self.fuck("param [ {} ] for {} is needed", name, self.get_name()); throw std::runtime_error{"lack of param"}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cxx/util/node_mixin.hh` around lines 29 - 36, 当前 param<T>() 在 has_parameter(name) 为 true 时仍调用 get_parameter_or<T>(name, T{})，导致传入的默认值 T{} 永远不会被使用；请将实现简化：在模板函数 param(this const auto& self, const std::string& name) 中，当 self.has_parameter(name) 为 true 时直接返回 self.template get_parameter<T>(name)，否则调用 self.fuck("param [ {} ] for {} is needed", name, self.get_name()) 然后抛出 std::runtime_error；替换掉对 get_parameter_or 的调用以清晰表达意图（保留函数名 param、has_parameter、get_parameter_or/get_parameter、fuck、get_name 以便定位）。src/lua/launch/navigation.lua (1)
5-13: 提醒：处理 TODO 注释并移除未使用的导入当前实现为占位符代码：
lyaml已导入但未使用basic_launch表已定义但未使用- 函数始终返回空字符串
建议在完成实现前添加
-- luacheck: ignore注释，或考虑延迟合并此文件。需要我帮助生成使用
lyaml构建 ROS 2 launch YAML 的实现吗？或者创建一个 issue 来跟踪此任务？🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/launch/navigation.lua` around lines 5 - 13, The TODO and unused imports must be resolved: either implement the returned function to build and return a ROS 2 launch YAML string using the lyaml library (use lyaml.emit on a Lua table you construct from the provided args inside the anonymous function returned at the bottom, and reuse or populate the basic_launch table as a template), or if you cannot implement it now remove the unused basic_launch and lyaml imports or add a top-of-file comment "-- luacheck: ignore 621,211" to silence warnings; ensure the final change either uses the symbols lyaml and basic_launch (by converting the launch table to YAML and returning that string) or removes them so there are no unused identifiers..luarc.json (1)
5-6: 平台特定路径可能影响跨平台开发体验。系统库路径
/usr/lib/x86_64-linux-gnu/lua/5.4仅适用于 x86_64 架构的 Debian/Ubuntu 系统。在 ARM 架构或其他 Linux 发行版上，路径会有所不同。这不会影响运行时功能，但可能导致其他开发者的 IDE 提示不完整。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.luarc.json around lines 5 - 6, The entry "/usr/lib/x86_64-linux-gnu/lua/5.4" is platform/architecture-specific and can break IDE tooling for non-x86_64 contributors; replace it with more portable alternatives or a set of common paths (e.g., use "/usr/lib/lua/5.4", "/usr/local/lib/lua/5.4", or remove the x86_64-specific string) or make loading conditional via environment detection so the array containing "/usr/share/lua/5.4" and the other lua paths is not tied to x86_64-only locations.src/cxx/navigation.cc (1)
139-146: 可选：简化条件判断
should_cancel的检查逻辑可以合并：- auto should_cancel = request_id != latest_request_id; - if (!should_cancel) - current_goal_handle = handle; - - if (should_cancel) { + if (request_id != latest_request_id) { client->async_cancel_goal(handle); return; } + current_goal_handle = handle;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cxx/navigation.cc` around lines 139 - 146, 当前代码先计算 should_cancel = request_id != latest_request_id，再用 if (!should_cancel) current_goal_handle = handle; 然后 if (should_cancel) { client->async_cancel_goal(handle); return; }，请将这两个分支合并为一个清晰的条件分支：直接判断 request_id == latest_request_id 时设置 current_goal_handle = handle，否则调用 client->async_cancel_goal(handle) 并 return。引用的标识符：request_id、latest_request_id、should_cancel、current_goal_handle、handle、client->async_cancel_goal。src/lua/api.lua (1)
43-46: 考虑验证配置字段
tostring()对nil值会返回字符串"nil"，这可能导致 launch 参数意外变为global_map:=nil。建议添加字段验证或默认值处理。local global_map = config.global_map or "rmul"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lua/api.lua` around lines 43 - 46, The current code calls tostring(config.<field>) for launch_livox, launch_odin1, global_map and use_sim_time which turns nil into the literal string "nil"; update these to validate and provide sensible defaults from config before conversion (e.g. check config.global_map and fallback to a default like "rmul", ensure boolean flags like launch_livox/launch_odin1 are coerced to "true"/"false" only when not nil) — modify the assignments for launch_livox, launch_odin1, global_map and use_sim_time to read from config with nil checks or `or` defaults and then call tostring only on the resulting safe value.config/motion.yaml (1)
76-87: 速度限制不一致
DWBLocalPlanner配置的速度限制（max_vel_x/y: ±5.0,max_speed_xy: 8.0）与velocity_smoother的max_velocity: [3.5, 3.5, 3.0]存在明显差异。这意味着 DWB 规划的高速轨迹会被 velocity_smoother 大幅削减。建议考虑对齐这两处配置，或确认这是预期的限速行为。
Also applies to: 155-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/motion.yaml` around lines 76 - 87, The DWBLocalPlanner velocity limits (max_vel_x, min_vel_x, max_vel_y, min_vel_y, max_speed_xy, max_speed_theta) are inconsistent with the velocity_smoother max_velocity ([3.5,3.5,3.0]); either align these values or document/accept the intentional reduction. Update the config so DWBLocalPlanner keys (max_vel_x/max_vel_y/max_speed_xy/max_speed_theta) match the velocity_smoother.max_velocity vector, or adjust velocity_smoother.max_velocity to reflect DWB’s higher limits, and ensure the same change is applied for the second occurrence referenced (also applies to the block at 155-162). Include a short comment explaining the design choice.launch/sensor.launch.yaml (1)
79-84: 提醒：处理 TODO 注释此处标注了
(remove after testing)的静态变换发布器。请确保在测试完成后移除此节点，或者创建 issue 跟踪此任务。是否需要我帮您创建一个 issue 来跟踪此待办事项？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@launch/sensor.launch.yaml` around lines 79 - 84, The TODO comment indicates the temporary static transform publisher node (pkg: "tf2_ros", exec: "static_transform_publisher", name: "initial_transform") should not remain in production; either remove this node block from launch/sensor.launch.yaml after testing or create and link an issue tracking the follow-up to remove it—update the file by deleting the node entry or add an ISSUE_ID/URL comment referencing the created issue so the TODO is tracked.src/cxx/component.cc (1)
36-36: 使用大括号初始化根据编码规范，应使用大括号初始化：
- std::atomic<std::uint16_t> lua_tick_count = 0; + std::atomic<std::uint16_t> lua_tick_count = std::uint16_t{0};As per coding guidelines: "Use brace initialization with type postfix for constructing and declaring C++ variables:
auto var = T { };"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cxx/component.cc` at line 36, The declaration of lua_tick_count uses equals-style initialization; change the declaration of lua_tick_count (std::atomic<std::uint16_t> lua_tick_count = 0) to use brace initialization with a type-postfix and auto (i.e., construct a std::atomic<std::uint16_t> with braces and assign to auto lua_tick_count) so it follows the project's "auto var = T { }" pattern.
🤖 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/cxx/component.cc`:
- Around line 149-157: The lua_sync() function is injecting NaN values returned
by navigation.check_position() into lua_blackboard["user"] (user["x"],
user["y"], user["yaw"]) without validation; add a NaN check (e.g., std::isnan on
x/y/yaw) after calling navigation.check_position() and only assign the position
fields into lua_blackboard["user"] when all required values are finite/valid,
leaving health/bullet updates unchanged; alternatively, clear or set the Lua
fields to nil when the position is invalid so downstream Lua code (like
crossing-road-zone.lua) cannot accidentally consume NaN values.
In `@src/cxx/navigation.hh`:
- Around line 32-33: The header comment in navigation.hh incorrectly states TF
query failure returns {0.0, 0.0, 0.0}; update that documentation to state it
returns {NaN, NaN, NaN} on TF lookup failure to match the implementation that
sets NaN values when the TF query fails (i.e., the TF lookup function used by
the navigation code). Ensure the updated comment replaces the example return
value and briefly notes that callers must check for NaN results.
In `@src/lua/api.lua`:
- Line 67: The call to util.run_command(packed_command) ignores its (success,
message) return values; update the caller to capture those values (e.g., local
ok, err = util.run_command(packed_command)) and handle failures by logging the
error via the module logger (or util.log) and returning/propagating a failure to
the caller (or raising an error) so callers are notified when starting the
screen session or running the launch file fails; make sure to reference
util.run_command and the surrounding function that invokes it when making the
change.
In `@src/lua/endpoint/main.lua`:
- Around line 49-50: 在记录 `api.info("use decision: '" .. option.decision .. "'")`
时要防护 `option.decision` 为 nil 的情况；修改点为在使用 `option.decision` 前进行空值检查或转换（例如使用
tostring(option.decision) 或判断后记录默认/占位字符串），或者用条件分支只在非 nil 时拼接并记录，这样可避免在函数/代码段中因
`option.decision` 为 nil 而引发运行时错误；请定位并更新含有 api.info 调用的代码段以实施该检查/转换。
In `@test/lua/runable.lua`:
- Around line 91-94: Replace the old string-call to api.restart_navigation with
a call that passes a configuration table/object containing the four properties
expected by the new API: set launch_livox to false, launch_odin1 to false,
global_map to "rmul", and use_sim_time to false so that the call to
api.restart_navigation supplies {launch_livox=false, launch_odin1=false,
global_map="rmul", use_sim_time=false} and the existing test assertions against
calls.restart_navigation[1].<property> will match.
---
Nitpick comments:
In @.luarc.json:
- Around line 5-6: The entry "/usr/lib/x86_64-linux-gnu/lua/5.4" is
platform/architecture-specific and can break IDE tooling for non-x86_64
contributors; replace it with more portable alternatives or a set of common
paths (e.g., use "/usr/lib/lua/5.4", "/usr/local/lib/lua/5.4", or remove the
x86_64-specific string) or make loading conditional via environment detection so
the array containing "/usr/share/lua/5.4" and the other lua paths is not tied to
x86_64-only locations.
In `@CMakeLists.txt`:
- Around line 72-87: The hardcoded linker flag "-rdynamic" in the
target_link_libraries PRIVATE list breaks cross-platform portability; remove
"-rdynamic" from that list and instead add a target_link_options call for the
same target (e.g., the target referenced by ${PROJECT_NAME} or the same target
used with target_link_libraries) that conditionally applies
$<$<CXX_COMPILER_ID:GNU,Clang>:-rdynamic> so the option is only set for
GCC/Clang compilers.
In `@config/motion.yaml`:
- Around line 76-87: The DWBLocalPlanner velocity limits (max_vel_x, min_vel_x,
max_vel_y, min_vel_y, max_speed_xy, max_speed_theta) are inconsistent with the
velocity_smoother max_velocity ([3.5,3.5,3.0]); either align these values or
document/accept the intentional reduction. Update the config so DWBLocalPlanner
keys (max_vel_x/max_vel_y/max_speed_xy/max_speed_theta) match the
velocity_smoother.max_velocity vector, or adjust velocity_smoother.max_velocity
to reflect DWB’s higher limits, and ensure the same change is applied for the
second occurrence referenced (also applies to the block at 155-162). Include a
short comment explaining the design choice.
In `@launch/sensor.launch.yaml`:
- Around line 79-84: The TODO comment indicates the temporary static transform
publisher node (pkg: "tf2_ros", exec: "static_transform_publisher", name:
"initial_transform") should not remain in production; either remove this node
block from launch/sensor.launch.yaml after testing or create and link an issue
tracking the follow-up to remove it—update the file by deleting the node entry
or add an ISSUE_ID/URL comment referencing the created issue so the TODO is
tracked.
In `@src/cxx/component.cc`:
- Line 36: The declaration of lua_tick_count uses equals-style initialization;
change the declaration of lua_tick_count (std::atomic<std::uint16_t>
lua_tick_count = 0) to use brace initialization with a type-postfix and auto
(i.e., construct a std::atomic<std::uint16_t> with braces and assign to auto
lua_tick_count) so it follows the project's "auto var = T { }" pattern.
In `@src/cxx/navigation.cc`:
- Around line 139-146: 当前代码先计算 should_cancel = request_id !=
latest_request_id，再用 if (!should_cancel) current_goal_handle = handle; 然后 if
(should_cancel) { client->async_cancel_goal(handle); return;
}，请将这两个分支合并为一个清晰的条件分支：直接判断 request_id == latest_request_id 时设置
current_goal_handle = handle，否则调用 client->async_cancel_goal(handle) 并
return。引用的标识符：request_id、latest_request_id、should_cancel、current_goal_handle、handle、client->async_cancel_goal。
In `@src/cxx/util/node_mixin.hh`:
- Around line 34-35: 将抛出的 std::runtime_error 错误信息改为包含参数名和节点名以便调试：在抛出位置（当前的 throw
std::runtime_error{"lack of param"};）构建更有上下文的消息（例如把 name 和 self.get_name()
拼入错误字符串），并确保文件头部包含 `#include` <stdexcept> 如果尚未包含；保持现有的 self.fuck 调用不变，只替换
runtime_error 的消息内容以包含详细上下文。
- Around line 29-36: 当前 param<T>() 在 has_parameter(name) 为 true 时仍调用
get_parameter_or<T>(name, T{})，导致传入的默认值 T{} 永远不会被使用；请将实现简化：在模板函数 param(this
const auto& self, const std::string& name) 中，当 self.has_parameter(name) 为 true
时直接返回 self.template get_parameter<T>(name)，否则调用 self.fuck("param [ {} ] for {}
is needed", name, self.get_name()) 然后抛出 std::runtime_error；替换掉对 get_parameter_or
的调用以清晰表达意图（保留函数名
param、has_parameter、get_parameter_or/get_parameter、fuck、get_name 以便定位）。
In `@src/lua/api.lua`:
- Around line 43-46: The current code calls tostring(config.<field>) for
launch_livox, launch_odin1, global_map and use_sim_time which turns nil into the
literal string "nil"; update these to validate and provide sensible defaults
from config before conversion (e.g. check config.global_map and fallback to a
default like "rmul", ensure boolean flags like launch_livox/launch_odin1 are
coerced to "true"/"false" only when not nil) — modify the assignments for
launch_livox, launch_odin1, global_map and use_sim_time to read from config with
nil checks or `or` defaults and then call tostring only on the resulting safe
value.
In `@src/lua/launch/navigation.lua`:
- Around line 5-13: The TODO and unused imports must be resolved: either
implement the returned function to build and return a ROS 2 launch YAML string
using the lyaml library (use lyaml.emit on a Lua table you construct from the
provided args inside the anonymous function returned at the bottom, and reuse or
populate the basic_launch table as a template), or if you cannot implement it
now remove the unused basic_launch and lyaml imports or add a top-of-file
comment "-- luacheck: ignore 621,211" to silence warnings; ensure the final
change either uses the symbols lyaml and basic_launch (by converting the launch
table to YAML and returning that string) or removes them so there are no unused
identifiers.
In `@src/lua/util/native.lua`:
- Around line 33-41: The call site for util.run_command in
api.restart_navigation must check its return values so failing `ros2 launch`
isn't silently ignored: update the call in api.restart_navigation to capture the
returned boolean and message from M.run_command (or util.run_command), if the
boolean is false log the error message (or return an error to the caller) and
avoid proceeding with assuming the screen session started; also consider
cleaning up or killing any partially created screen session on failure. Ensure
you reference the M.run_command return contract (true/"ok" on success,
false/<message> on failure) when implementing the check and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1908da44-8a99-49c7-8341-6c873196fba3
⛔ Files ignored due to path filters (1)
maps/rmuc.pngis excluded by!**/*.png
📒 Files selected for processing (36)
.luarc.jsonCMakeLists.txtREADME.mdcmake/export.cmakeconfig/custom.yamlconfig/follow_waypoints.yamlconfig/motion.xmlconfig/motion.yamlconfig/nav2.yamlconfig/rmuc.yamlconfig/rmul.yamlconfig/sensor.yamllaunch/custom.launch.pylaunch/follow_waypoints.launch.pylaunch/motion.launch.yamllaunch/nav2.launch.pylaunch/online.launch.pylaunch/sensor.launch.yamlmaps/rmuc.yamlmaps/rmul.yamlsrc/cxx/component.ccsrc/cxx/navigation.ccsrc/cxx/navigation.hhsrc/cxx/util/node_mixin.hhsrc/lua/api.luasrc/lua/blackboard.luasrc/lua/endpoint/main.luasrc/lua/endpoint/mock.luasrc/lua/endpoint/test.luasrc/lua/launch/navigation.luasrc/lua/option.luasrc/lua/util/ascii_art.luasrc/lua/util/native.luasrc/script/follow_waypoints_runner.pysrc/script/goal_topic_bridge.pytest/lua/runable.lua
💤 Files with no reviewable changes (12)
- config/custom.yaml
- cmake/export.cmake
- config/follow_waypoints.yaml
- config/rmul.yaml
- config/rmuc.yaml
- launch/follow_waypoints.launch.py
- config/nav2.yaml
- launch/nav2.launch.py
- launch/online.launch.py
- src/script/goal_topic_bridge.py
- launch/custom.launch.py
- src/script/follow_waypoints_runner.py
Comment on lines
149
to
+157
| auto lua_sync() { | ||
| const auto [x, y, yaw] = navigation.check_position(); | ||
|
|
||
| auto user = lua_blackboard["user"].get<sol::table>(); | ||
| user["health"] = *context.robot_health; | ||
| user["bullet"] = *context.robot_bullet; | ||
| user["x"] = x; | ||
| user["y"] = y; | ||
| user["yaw"] = yaw; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查 Lua 代码中是否有对 x, y, yaw 的 NaN 处理
rg -n 'blackboard.*user' --type lua -A3 -B1
rg -n '\bx\b.*\by\b.*\byaw\b' --type luaRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 491
🏁 Script executed:
# Find and examine the check_position() implementation
fd -e cc -e cpp -e h -e hpp | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 159
🏁 Script executed:
# Search for check_position definition
rg -n 'check_position' --type cpp -B2 -A10Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 3017
🏁 Script executed:
# Search for all Lua code using blackboard.user
rg -n 'blackboard\.user' --type lua -B2 -A2Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 504
🏁 Script executed:
# Get full check_position() implementation including catch block
sed -n '230,255p' src/cxx/navigation.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 1133
🏁 Script executed:
# Find all Lua files
find . -name "*.lua" -type fRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 620
🏁 Script executed:
# Search comprehensively for all blackboard.user usage in Lua
rg -n 'blackboard\.user\.' --type luaRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 222
🏁 Script executed:
# Check for any NaN handling or validation
rg -n 'nan|NaN|is_nan|isnan' --type cpp --type lua -iRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 726
🏁 Script executed:
# Check the full crossing-road-zone.lua to see how x, y are used
cat -n src/lua/mini-task/crossing-road-zone.luaRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 1215
🏁 Script executed:
# Check context.lua to see the initialization and any NaN handling
cat -n src/lua/context.lua | head -40Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 512
🏁 Script executed:
# Check if there's any validation logic around position usage
rg -n 'crossing-road-zone|position' --type lua -B2 -A5Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 610
在注入 Lua 前添加 NaN 值检查
check_position() 在 TF 查询失败时会返回 NaN 值（navigation.cc:244），这些 NaN 值被直接注入到 blackboard.user 中而未进行任何验证。虽然 Lua 的 context:set_target() 中存在 NaN 检查逻辑（context.lua:14-16，使用 if x ~= x or y ~= y 的方式），但 crossing-road-zone.lua 直接访问 blackboard.user.x 和 blackboard.user.y 而不进行验证，会导致这些 NaN 值被直接使用。
建议在 C++ 侧的 lua_sync() 中添加 NaN 检查，仅在位置有效时才更新 blackboard，或者确保所有访问这些值的 Lua 代码都添加防护逻辑。测试用例 (test/lua/runable.lua:71) 已标注此为已知问题。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cxx/component.cc` around lines 149 - 157, The lua_sync() function is
injecting NaN values returned by navigation.check_position() into
lua_blackboard["user"] (user["x"], user["y"], user["yaw"]) without validation;
add a NaN check (e.g., std::isnan on x/y/yaw) after calling
navigation.check_position() and only assign the position fields into
lua_blackboard["user"] when all required values are finite/valid, leaving
health/bullet updates unchanged; alternatively, clear or set the Lua fields to
nil when the position is invalid so downstream Lua code (like
crossing-road-zone.lua) cannot accidentally consume NaN values.
Contributor
Author
There was a problem hiding this comment.
cxx 侧不做任何逻辑处理，都抛到 Lua 侧做，这是未来的开发事项
There was a problem hiding this comment.
@creeper5820，了解，C++ 侧保持纯粹的数据桥接，不引入业务逻辑，后续在 Lua 侧统一处理位置有效性校验。已记录此设计原则。
✏️ Learnings added
Learnt from: creeper5820
Repo: Alliance-Algorithm/rmcs-navigation PR: 5
File: src/cxx/component.cc:149-157
Timestamp: 2026-04-14T12:44:24.591Z
Learning: In rmcs-navigation (Alliance-Algorithm/rmcs-navigation), the C++ side (src/cxx/component.cc) is intentionally a thin bridge with no business logic processing. All logic, including NaN/invalid-value handling for position data (x, y, yaw from check_position()), is deferred to the Lua side. Do not suggest adding validation or guards in C++ component code; such concerns belong in Lua.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: Alliance-Algorithm/rmcs-navigation PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-09T05:13:19.413Z
Learning: Applies to rmcs_ws/src/skills/rmcs-navigation/src/lua/main.lua : Lua entry point is at rmcs_ws/src/skills/rmcs-navigation/src/lua/main.lua; implement on_init() for initialization of edge detection, periodic tasks and runtime state; implement on_tick() as the stable per-frame entry point
Learnt from: CR
Repo: Alliance-Algorithm/rmcs-navigation PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-09T05:13:19.413Z
Learning: Applies to rmcs_ws/src/skills/rmcs-navigation/src/lua/**/*.lua : Use rmcs_ws/src/lua/util/clock.lua as the unified time source; provide time through blackboard.meta.timestamp; only update time via clock:reset() and clock:update() in on_init() and on_tick(); do not use os.clock() directly in business logic, coroutine primitives, or scheduler internals
| ]] | ||
| local packed_command = string.format(command, filename, sensor_config, motion_config) | ||
|
|
||
| util.run_command(packed_command) |
There was a problem hiding this comment.
考虑检查 run_command 的返回值
util.run_command() 返回 (success, message)，但此处未检查返回值。如果 screen 会话启动或 launch 文件执行失败，调用方不会收到任何错误通知。
建议的修复
- util.run_command(packed_command)
+ local success, message = util.run_command(packed_command)
+ if not success then
+ api.warn("restart_navigation failed: " .. message)
+ end📝 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
| util.run_command(packed_command) | |
| local success, message = util.run_command(packed_command) | |
| if not success then | |
| api.warn("restart_navigation failed: " .. message) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lua/api.lua` at line 67, The call to util.run_command(packed_command)
ignores its (success, message) return values; update the caller to capture those
values (e.g., local ok, err = util.run_command(packed_command)) and handle
failures by logging the error via the module logger (or util.log) and
returning/propagating a failure to the caller (or raising an error) so callers
are notified when starting the screen session or running the launch file fails;
make sure to reference util.run_command and the surrounding function that
invokes it when making the change.
Comment on lines
+49
to
+50
| api.info("use decision: '" .. option.decision .. "'") | ||
|
|
There was a problem hiding this comment.
建议对 option.decision 进行空值检查。
如果 ROS 参数中未配置 decision，option.decision 将为 nil，导致字符串拼接失败并抛出运行时错误。
🛡️ 建议的防御性修改
on_init = function()
clock:reset(blackboard.meta.timestamp)
- api.info("use decision: '" .. option.decision .. "'")
+ local decision = option.decision or "unknown"
+ api.info("use decision: '" .. decision .. "'")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lua/endpoint/main.lua` around lines 49 - 50, 在记录 `api.info("use decision:
'" .. option.decision .. "'")` 时要防护 `option.decision` 为 nil 的情况；修改点为在使用
`option.decision` 前进行空值检查或转换（例如使用 tostring(option.decision)
或判断后记录默认/占位字符串），或者用条件分支只在非 nil 时拼接并记录，这样可避免在函数/代码段中因 `option.decision` 为 nil
而引发运行时错误；请定位并更新含有 api.info 调用的代码段以实施该检查/转换。
Comment on lines
+91
to
+94
| assert_eq(calls.restart_navigation[1].launch_livox, false, "restart config launch_livox") | ||
| assert_eq(calls.restart_navigation[1].launch_odin1, false, "restart config launch_odin1") | ||
| assert_eq(calls.restart_navigation[1].global_map, "rmul", "restart config global_map") | ||
| assert_eq(calls.restart_navigation[1].use_sim_time, false, "restart config use_sim_time") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查 restart_navigation.lua 是否使用了旧的字符串参数 API
rg -n 'restart_navigation\s*\(' test/lua/restart_navigation.luaRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 132
🏁 Script executed:
# Check all calls to restart_navigation in the test file
rg -n 'restart_navigation' test/lua/restart_navigation.lua -B 2 -A 2Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 1079
🏁 Script executed:
# Check if there are other test files that might call restart_navigation
fd -e lua test/ | xargs rg -l 'restart_navigation' | head -20Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 541
🏁 Script executed:
# Read the restart_navigation.lua file to understand its full content
wc -l test/lua/restart_navigation.luaRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 112
🏁 Script executed:
# Read the entire restart_navigation.lua file
cat -n test/lua/restart_navigation.luaRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 3195
🏁 Script executed:
# Check the api.lua file to understand the restart_navigation API signature
rg -A 20 'function.*restart_navigation' src/lua/api.lua | head -40Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 678
更新 test/lua/restart_navigation.lua 第 70 行以使用新的 API 签名。
当前代码在第 70 行使用了旧的字符串参数 api.restart_navigation("rmul")，但新的 API 要求传入配置表。应更新为传入包含 launch_livox、launch_odin1、global_map 和 use_sim_time 属性的配置表，其中 global_map 值为 "rmul"。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/lua/runable.lua` around lines 91 - 94, Replace the old string-call to
api.restart_navigation with a call that passes a configuration table/object
containing the four properties expected by the new API: set launch_livox to
false, launch_odin1 to false, global_map to "rmul", and use_sim_time to false so
that the call to api.restart_navigation supplies {launch_livox=false,
launch_odin1=false, global_map="rmul", use_sim_time=false} and the existing test
assertions against calls.restart_navigation[1].<property> will match.
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.
No description provided.