Reapply "Yuyuyu/relocation"#16
Open
vhjcgja-789 wants to merge 11 commits intomainfrom
Open
Conversation
概述该拉取请求通过ROS2服务集成了 更改重定位服务集成
序列图sequenceDiagram
participant Lua as Lua脚本
participant Action as action模块
participant Component as C++组件
participant Localization as Localization
participant Service as rmcs_relocation<br/>服务
Lua->>Action: relocalize_initial(x,y,yaw)
Action->>Action: send_and_await()检查在途门控
Action->>Component: api.relocalize_initial()
Component->>Localization: relocalize(mode,x,y,yaw)
Localization->>Service: 发送异步服务请求
Service-->>Localization: 异步响应回调
Localization->>Localization: 映射响应→RelocalizeStatus
loop 轮询直至终止
Action->>Component: api.relocalize_status()
Component->>Localization: relocalize_status()
Localization-->>Component: 返回当前状态快照
Component-->>Action: 状态表
alt 状态==SUCCEEDED或失败
Action->>Action: 返回(ok, status)
else 状态==IN_FLIGHT
Action->>Action: 继续轮询
end
end
Action-->>Lua: (success_bool, status_table)
评估代码审查工作量🎯 3 (中等) | ⏱️ ~20 分钟 诗
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (7)
package.xml (1)
31-33: 💤 Low value
<depend>顺序与缩进微调建议新增的
<depend>rmcs_relocation</depend>被放在<exec_depend>组之后，与上方其他<depend>项分隔开了；同时 Line 33 末尾有多余空格。建议把它移回到上方<depend>组（紧挨rmcs_msgs/nav2_msgs），以保持一致的分组顺序。♻️ 建议改动
<depend>rmcs_msgs</depend> <depend>nav2_msgs</depend> + <depend>rmcs_relocation</depend> <exec_depend>launch</exec_depend> ... <exec_depend>rmcs_local_map</exec_depend> - <depend>rmcs_relocation</depend> - <export>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.xml` around lines 31 - 33, The new <depend>rmcs_relocation</depend> entry is misplaced after the <exec_depend>rmcs_local_map</exec_depend> and has a trailing space; move the <depend>rmcs_relocation</depend> line back into the main <depend> group next to rmcs_msgs and nav2_msgs to preserve grouping order and remove the extra trailing whitespace at the end of the line; ensure indentation matches the other <depend> entries..github/workflows/ci.yml (1)
70-76: 💤 Low valueCI 跟随
rmcs_relocation的main分支，非确定性构建这里
--branch main --depth 1直接拉取上游rmcs_relocation/main。一旦上游有非兼容改动，与本仓库无关的 PR 也会突然在 CI 里失败。短期可以接受，长期建议改成 pin 一个稳定的 tag/commit，并在env:里以RMCS_RELOCATION_REF暴露便于后续升级。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 70 - 76, The CI step "Clone rmcs_relocation dependency" currently clones the upstream main branch causing non-deterministic builds; change it to use an environment variable RMCS_RELOCATION_REF (exposed in the workflow env:) and pin the clone to that ref (a stable tag or commit) instead of hardcoding --branch main; update the step to use RMCS_RELOCATION_REF when cloning (or fetch+checkout that ref) so maintainers can update the pinned version intentionally and CI remains deterministic.src/lua/action.lua (2)
8-13: 💤 Low valueLua 与 C++ 的
RelocalizeState数值需保持手工同步这里硬编码了
IDLE=0/IN_FLIGHT=1/SUCCEEDED=2/FAILED=3，与engine.hh的enum class RelocalizeState : std::uint8_t对齐。两侧任何一边调整顺序都会让send_and_await误判终态而不会有任何警告。建议在 C++ 侧通过make_api_injection把这几个常量也注入到 Lua（例如api.RelocalizeState.SUCCEEDED），让 Lua 这里直接消费而不是各写一份。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lua/action.lua` around lines 8 - 13, The RelocalizeState numeric values are hardcoded in Lua and must instead consume the constants injected from C++ to avoid drift; remove the local RelocalizeState table and replace its usages in this file (e.g., comparisons inside send_and_await and any references to RelocalizeState.IDLE/IN_FLIGHT/SUCCEEDED/FAILED) with the injected api.RelocalizeState.* symbols (ensure the C++ side provides these via make_api_injection so api.RelocalizeState.SUCCEEDED etc. exist), and update any local references or imports so the code reads from api.RelocalizeState rather than duplicating the enum.
100-114: ⚡ Quick win轮询缺少上限，依赖 C++ 端必须按时退出
IN_FLIGHT
while true do ... request:sleep(0.1) end在 Lua 侧没有任何兜底超时。一旦 C++ 端的session因为engine.cc的 timer/race 问题卡在IN_FLIGHT（参见对engine.cc的另一条评论），这个协程会永远占着调度。建议加一个 wall-clock 上限（例如request_timeout_sec * 2 + 安全余量），到时强制返回false, st。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lua/action.lua` around lines 100 - 114, The loop polling api.relocalize_status() has no wall-clock timeout and can spin forever if state stays IN_FLIGHT; update the relocalization loop in action.lua (the while true block that calls api.relocalize_status(), checks RelocalizeState, and uses request:sleep) to track elapsed real time and break after a configured limit (for example request_timeout_sec * 2 plus a small safety margin), returning false, st when the timeout is reached and preserving the existing self:warn logging (include the timeout reason in the message). Ensure the timeout uses a monotonic/wall-clock timer available to the coroutine and does not rely on the C++ side to flip IN_FLIGHT.src/cxx/util/localization/engine.hh (1)
10-21: 💤 Low value两个枚举的命名风格不一致
RelocalizeState用IDLE/IN_FLIGHT/SUCCEEDED/FAILED（全大写下划线），而紧挨着的RelocalizeMode用Initial/Local/Wide（PascalCase）。同一文件、同一前缀的两个枚举类风格不一致，跨文件使用时容易写错。建议统一其中一种。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cxx/util/localization/engine.hh` around lines 10 - 21, The two enum classes RelocalizeState and RelocalizeMode use inconsistent naming styles; pick a single style (either ALL_CAPS_WITH_UNDERSCORES or PascalCase) and rename the enum members in one of them to match the other (e.g., change RelocalizeMode::Initial/Local/Wide to INITIAL/LOCAL/WIDE to match RelocalizeState, or change RelocalizeState::IDLE/IN_FLIGHT/SUCCEEDED/FAILED to Idle/InFlight/Succeeded/Failed to match RelocalizeMode); update all references/usages of these symbols (RelocalizeState, RelocalizeMode and their members) across the codebase to the chosen style to avoid build errors and keep naming consistent.src/cxx/component.cc (1)
277-280: 💤 Low value
has_parameter+get_parameter_or二选一即可
get_parameter_or<bool>("enable_relocalization", true)本身就在参数缺失时返回默认值，不需要再额外has_parameter三目；现在的写法逻辑等价，但读起来像在做不同的判断。♻️ 建议简化
- relocalization_enabled = - has_parameter("enable_relocalization") - ? get_parameter_or<bool>("enable_relocalization", true) - : true; + relocalization_enabled = get_parameter_or<bool>("enable_relocalization", true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cxx/component.cc` around lines 277 - 280, The ternary using has_parameter with get_parameter_or for relocalization_enabled is redundant; replace the expression with a single call to get_parameter_or<bool>("enable_relocalization", true) so relocalization_enabled is set directly from get_parameter_or (remove has_parameter and the conditional) — update the assignment for relocalization_enabled in component.cc to rely solely on get_parameter_or.src/cxx/util/localization/engine.cc (1)
168-173: 💤 Low value
service_is_ready+wait_for_service(0s)的双重判断基本等价
wait_for_service(std::chrono::seconds(0))实际上是一次非阻塞探测，与service_is_ready()在效果上重叠。这里写成!ready && !wait(0)并没有比单独!ready多检测什么状态。如果想要"短超时再等一次"，建议给wait_for_service一个非零的小窗口（例如 50–200ms），否则可以直接简化成service_is_ready()。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cxx/util/localization/engine.cc` around lines 168 - 173, The current check uses both session->client->service_is_ready() and wait_for_service(std::chrono::seconds(0)), which overlap because wait_for_service(0) is a non-blocking probe; simplify by using a single check or make the wait non-zero: either replace the condition with if (!session->client->service_is_ready()) { ... } to keep the fast non-blocking check, or call session->client->wait_for_service(std::chrono::milliseconds(100)) (or another small 50–200ms value) instead of seconds(0) to allow a brief wait before failing, keeping the same error path that calls warn(...) and session->end(failed_status("service unavailable: " + config.service_name)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cxx/component.cc`:
- Around line 134-146: The disabled branch in the
api.set_function("relocalize_status") lambda returns RelocalizeStatus{.message =
"disabled"} which leaves state at the default IDLE; update that branch to return
a status with an explicit terminal state (e.g., RelocalizeState::FAILED or add a
new RelocalizeState::DISABLED) and a suitable message so callers of
relocalize_status() see a consistent terminal state; change the returned
RelocalizeStatus construction in the disabled path inside the relocalize_status
lambda (or add a new enum value if choosing DISABLED) so the "state" field is
not left as IDLE.
In `@src/cxx/util/localization/engine.cc`:
- Around line 148-202: The timeout timer can repeatedly fire and pending_id
races because arm_timeout is called before we obtain and track the actual
request_id; fix send by calling session->client->async_send_request(...) first,
capture result.request_id, call session->track(result.request_id), assign
*pending_id = result.request_id, then call arm_timeout(session, pending_id) so
the timer sees the real id; inside arm_timeout's lambda capture the timer (e.g.,
store the created timer in a shared_ptr and capture it) and call timer->cancel()
at the start of the callback (in addition to s->timeout_timer cancel handling)
to ensure the timer is stopped even if s->end(...) returns false, referencing
functions/symbols: send, async_send_request, result.request_id, session->track,
arm_timeout, create_wall_timer, timeout_timer, pending_id.
In `@src/lua/action.lua`:
- Around line 95-99: The false return from fn(...) in send_and_await conflates
"in-flight" and "service unavailable" cases; modify send_and_await to call
api.relocalize_status() when fn(x,y,yaw) returns false, inspect the returned
status/state from that API, and log a distinct message (e.g., "reloc skip <mode>
(service_unavailable: ...)" vs the existing "reloc skip <mode> (in_flight)") and
propagate the same false,nil return; use the function send_and_await and
api.relocalize_status() as the insertion points to read the accurate state and
include it in the process warning.
In `@src/lua/api.lua`:
- Line 23: relocalize_status 的注解缺少组件实际返回的 estimated_* 字段：在声明
relocalize_status（Lua 注解的 fun() 返回表）中补充 estimated_x、estimated_y、estimated_z 和
estimated_qx、estimated_qy、estimated_qz、estimated_qw 字段并把类型设为 number，以与
src/cxx/component.cc 中实际返回的 table 匹配，从而让 LSP 补全与静态检查识别这些字段。
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 70-76: The CI step "Clone rmcs_relocation dependency" currently
clones the upstream main branch causing non-deterministic builds; change it to
use an environment variable RMCS_RELOCATION_REF (exposed in the workflow env:)
and pin the clone to that ref (a stable tag or commit) instead of hardcoding
--branch main; update the step to use RMCS_RELOCATION_REF when cloning (or
fetch+checkout that ref) so maintainers can update the pinned version
intentionally and CI remains deterministic.
In `@package.xml`:
- Around line 31-33: The new <depend>rmcs_relocation</depend> entry is misplaced
after the <exec_depend>rmcs_local_map</exec_depend> and has a trailing space;
move the <depend>rmcs_relocation</depend> line back into the main <depend> group
next to rmcs_msgs and nav2_msgs to preserve grouping order and remove the extra
trailing whitespace at the end of the line; ensure indentation matches the other
<depend> entries.
In `@src/cxx/component.cc`:
- Around line 277-280: The ternary using has_parameter with get_parameter_or for
relocalization_enabled is redundant; replace the expression with a single call
to get_parameter_or<bool>("enable_relocalization", true) so
relocalization_enabled is set directly from get_parameter_or (remove
has_parameter and the conditional) — update the assignment for
relocalization_enabled in component.cc to rely solely on get_parameter_or.
In `@src/cxx/util/localization/engine.cc`:
- Around line 168-173: The current check uses both
session->client->service_is_ready() and
wait_for_service(std::chrono::seconds(0)), which overlap because
wait_for_service(0) is a non-blocking probe; simplify by using a single check or
make the wait non-zero: either replace the condition with if
(!session->client->service_is_ready()) { ... } to keep the fast non-blocking
check, or call session->client->wait_for_service(std::chrono::milliseconds(100))
(or another small 50–200ms value) instead of seconds(0) to allow a brief wait
before failing, keeping the same error path that calls warn(...) and
session->end(failed_status("service unavailable: " + config.service_name)).
In `@src/cxx/util/localization/engine.hh`:
- Around line 10-21: The two enum classes RelocalizeState and RelocalizeMode use
inconsistent naming styles; pick a single style (either
ALL_CAPS_WITH_UNDERSCORES or PascalCase) and rename the enum members in one of
them to match the other (e.g., change RelocalizeMode::Initial/Local/Wide to
INITIAL/LOCAL/WIDE to match RelocalizeState, or change
RelocalizeState::IDLE/IN_FLIGHT/SUCCEEDED/FAILED to
Idle/InFlight/Succeeded/Failed to match RelocalizeMode); update all
references/usages of these symbols (RelocalizeState, RelocalizeMode and their
members) across the codebase to the chosen style to avoid build errors and keep
naming consistent.
In `@src/lua/action.lua`:
- Around line 8-13: The RelocalizeState numeric values are hardcoded in Lua and
must instead consume the constants injected from C++ to avoid drift; remove the
local RelocalizeState table and replace its usages in this file (e.g.,
comparisons inside send_and_await and any references to
RelocalizeState.IDLE/IN_FLIGHT/SUCCEEDED/FAILED) with the injected
api.RelocalizeState.* symbols (ensure the C++ side provides these via
make_api_injection so api.RelocalizeState.SUCCEEDED etc. exist), and update any
local references or imports so the code reads from api.RelocalizeState rather
than duplicating the enum.
- Around line 100-114: The loop polling api.relocalize_status() has no
wall-clock timeout and can spin forever if state stays IN_FLIGHT; update the
relocalization loop in action.lua (the while true block that calls
api.relocalize_status(), checks RelocalizeState, and uses request:sleep) to
track elapsed real time and break after a configured limit (for example
request_timeout_sec * 2 plus a small safety margin), returning false, st when
the timeout is reached and preserving the existing self:warn logging (include
the timeout reason in the message). Ensure the timeout uses a
monotonic/wall-clock timer available to the coroutine and does not rely on the
C++ side to flip IN_FLIGHT.
🪄 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: 2a58e101-2f11-4c70-a3cf-6e205a570397
📒 Files selected for processing (8)
.github/workflows/ci.ymlCMakeLists.txtpackage.xmlsrc/cxx/component.ccsrc/cxx/util/localization/engine.ccsrc/cxx/util/localization/engine.hhsrc/lua/action.luasrc/lua/api.lua
Comment on lines
+134
to
+146
| // status 转 lua table：disabled/enabled。 | ||
| api.set_function("relocalize_status", [this]() { | ||
| const auto status = (relocalization_enabled && localization) | ||
| ? localization->relocalize_status() | ||
| : RelocalizeStatus{.message = "disabled"}; | ||
| return lua->create_table_with( | ||
| "state", static_cast<int>(status.state), "success", status.success, | ||
| "message", status.message, "fitness_score", status.fitness_score, "confidence", | ||
| status.confidence, "estimated_x", status.estimated_x, "estimated_y", | ||
| status.estimated_y, "estimated_z", status.estimated_z, "estimated_qx", | ||
| status.estimated_qx, "estimated_qy", status.estimated_qy, "estimated_qz", | ||
| status.estimated_qz, "estimated_qw", status.estimated_qw); | ||
| }); |
There was a problem hiding this comment.
disabled 分支返回的 state 是 IDLE 而非终态
RelocalizeStatus{.message = "disabled"} 会得到默认的 state = IDLE，但语义上"功能被关闭"应该是一个终态（FAILED 或新增的 DISABLED）。Lua 端 send_and_await 不会走到这里（disabled 时 relocalize_* 已直接返回 false），但任何只查询 relocalize_status() 的调用方会看到 state==IDLE / message=="disabled" 的奇怪组合。
🛠️ 建议
- const auto status = (relocalization_enabled && localization)
- ? localization->relocalize_status()
- : RelocalizeStatus{.message = "disabled"};
+ const auto status = (relocalization_enabled && localization)
+ ? localization->relocalize_status()
+ : RelocalizeStatus{
+ .state = RelocalizeState::FAILED,
+ .success = false,
+ .message = "disabled",
+ };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cxx/component.cc` around lines 134 - 146, The disabled branch in the
api.set_function("relocalize_status") lambda returns RelocalizeStatus{.message =
"disabled"} which leaves state at the default IDLE; update that branch to return
a status with an explicit terminal state (e.g., RelocalizeState::FAILED or add a
new RelocalizeState::DISABLED) and a suitable message so callers of
relocalize_status() see a consistent terminal state; change the returned
RelocalizeStatus construction in the disabled path inside the relocalize_status
lambda (or add a new enum value if choosing DISABLED) so the "state" field is
not left as IDLE.
Comment on lines
+148
to
+202
| auto arm_timeout(const std::shared_ptr<Session>& s, const std::shared_ptr<std::int64_t>& pending_id) | ||
| -> void { | ||
| const auto timeout = std::chrono::duration_cast<std::chrono::nanoseconds>( | ||
| std::chrono::duration<double>{std::max(0.1, config.request_timeout_sec)}); | ||
| auto timer = node.create_wall_timer(timeout, [s, pending_id]() { | ||
| if (s->end(failed_status("request timeout: " + s->service_name), *pending_id)) | ||
| RCLCPP_WARN(s->logger, "relocalize request timeout: %s", s->service_name.c_str()); | ||
| }); | ||
| auto lock = std::scoped_lock{s->mutex}; | ||
| if (s->timeout_timer) | ||
| s->timeout_timer->cancel(); | ||
| s->timeout_timer = std::move(timer); | ||
| } | ||
|
|
||
| auto send(RelocalizeMode mode, double x, double y, double yaw) -> bool { | ||
| if (!session->begin()) { | ||
| warn("relocalize skipped: previous request is still in flight"); | ||
| return false; | ||
| } | ||
|
|
||
| if (!session->client->service_is_ready() | ||
| && !session->client->wait_for_service(std::chrono::seconds(0))) { | ||
| warn("relocalize service unavailable: {}", config.service_name); | ||
| session->end(failed_status("service unavailable: " + config.service_name)); | ||
| return false; | ||
| } | ||
|
|
||
| auto request = std::make_shared<Relocalize::Request>(); | ||
| request->mode = mode_to_msg(mode); | ||
| request->initial_guess_world_base = yaw_to_pose(x, y, yaw); | ||
|
|
||
| // executor 异步触发 callback。 | ||
| auto pending_id = std::make_shared<std::int64_t>(-1); | ||
| arm_timeout(session, pending_id); | ||
| auto s = session; | ||
| auto result = session->client->async_send_request( | ||
| std::move(request), [s, pending_id](RelocalizeClient::SharedFuture future) { | ||
| auto response = future.get(); | ||
| if (!response) { | ||
| RCLCPP_WARN(s->logger, "relocalize response is null"); | ||
| s->end(failed_status("response is null"), *pending_id); | ||
| return; | ||
| } | ||
| if (!response->success) { | ||
| RCLCPP_WARN(s->logger, "relocalize failed: %s", response->message.c_str()); | ||
| s->end(status_from_response(*response, false), *pending_id); | ||
| return; | ||
| } | ||
| RCLCPP_INFO( | ||
| s->logger, "relocalize success: score=%.4f, confidence=%.3f", | ||
| response->fitness_score, response->confidence); | ||
| s->end(status_from_response(*response, true), *pending_id); | ||
| }); | ||
| *pending_id = result.request_id; | ||
| session->track(result.request_id); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does rclcpp Node::create_wall_timer create a one-shot timer or a recurring timer in ROS 2 Jazzy?
💡 Result:
In ROS 2 Jazzy, rclcpp::Node::create_wall_timer creates a recurring (periodic) timer, not a one-shot timer. Evidence: the rclcpp timer that create_wall_timer builds on is described as a “Generic timer. Periodically executes a user-specified callback” [1]. The create_wall_timer API takes a “period – period to execute callback” [2], and the WallTimer docs describe the callback as “to execute every interval” [3]. If you want one-shot behavior, you typically cancel the timer after the first callback (or use cancel/reset appropriately); rclcpp timers expose cancel and reset APIs [4]. So, create_wall_timer’s default behavior is periodic/recurring; it does not automatically fire only once.
Citations:
- 1: http://docs.ros.org/en/jazzy/p/rclcpp/generated/classrclcpp_1_1GenericTimer.html
- 2: https://docs.ros.org/en/jazzy/p/rclcpp/generated/function_namespacerclcpp_1aa3ce62ebcda2516e5017b0eab01b4169.html
- 3: https://api.nav2.org/jazzy/html/classrclcpp_1_1WallTimer.html
- 4: https://api.nav2.org/jazzy/html/classrclcpp_1_1TimerBase.html
🏁 Script executed:
# Find the Session struct and examine end() method
rg -A 30 "struct Session" src/cxx/util/localization/engine.cc | head -100Repository: Alliance-Algorithm/rmcs-navigation
Length of output: 1182
🏁 Script executed:
# Search for the end() method implementation
rg -B 5 -A 20 "bool.*end\(" src/cxx/util/localization/engine.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 60
🏁 Script executed:
# Search for clear_locked implementation
rg -B 5 -A 15 "clear_locked" src/cxx/util/localization/engine.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 1969
🏁 Script executed:
# Search for track() method
rg -B 3 -A 10 "void.*track\(" src/cxx/util/localization/engine.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 60
🏁 Script executed:
# Check the full arm_timeout and send method context
sed -n '140,210p' src/cxx/util/localization/engine.ccRepository: Alliance-Algorithm/rmcs-navigation
Length of output: 3161
超时定时器存在重复触发与 pending_id 的竞态隐患
代码确实存在两个紧密相关的问题：
-
create_wall_timer是周期定时器，当end()失败时不会取消。 定时器回调调用s->end(failed_status(...), *pending_id)，若expected_id与pending_id不匹配，end()直接返回false而不调用clear_locked()，定时器会持续按request_timeout_sec周期触发。 -
*pending_id与result.request_id间存在竞态。 L180 初始化pending_id = -1，L181 已将其传入arm_timeout的 lambda 和 L183 响应回调，但真正赋值是 L201。若任何回调在 L202 前执行（快速服务、同进程调用）：- 回调捕获的
*pending_id == -1 - 此时
session->pending_id仍为空（track()在 L202） end()的expected_id检查失败：if (expected_id && pending_id != *expected_id)返回falseclear_locked()不被调用，定时器持续触发- 响应或超时状态被丢弃，
session永远卡在IN_FLIGHT状态
- 回调捕获的
建议先调用 async_send_request 并获取 result.request_id，随后再调用 track() 和 arm_timeout()；同时在定时器回调内显式调用 timer->cancel() 确保不会重复触发。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cxx/util/localization/engine.cc` around lines 148 - 202, The timeout
timer can repeatedly fire and pending_id races because arm_timeout is called
before we obtain and track the actual request_id; fix send by calling
session->client->async_send_request(...) first, capture result.request_id, call
session->track(result.request_id), assign *pending_id = result.request_id, then
call arm_timeout(session, pending_id) so the timer sees the real id; inside
arm_timeout's lambda capture the timer (e.g., store the created timer in a
shared_ptr and capture it) and call timer->cancel() at the start of the callback
(in addition to s->timeout_timer cancel handling) to ensure the timer is stopped
even if s->end(...) returns false, referencing functions/symbols: send,
async_send_request, result.request_id, session->track, arm_timeout,
create_wall_timer, timeout_timer, pending_id.
Comment on lines
+95
to
+99
| local function send_and_await(self, mode, fn, x, y, yaw) | ||
| if not fn(x, y, yaw) then | ||
| self:warn(string.format("reloc skip %s (in_flight)", mode)) | ||
| return false, nil | ||
| end |
There was a problem hiding this comment.
fn(...)==false 的两种语义被合并成同一条日志
C++ 侧 Localization::send 返回 false 有两种情况：
- 已有请求在飞 (
session->begin()失败)； - 服务不可用 (
service_is_ready()检查失败)。
第二种情况下 session->end(failed_status("service unavailable: ...")) 已经把状态写成 FAILED 了，但这里只输出 reloc skip <mode> (in_flight)，会误导排障。建议在 false 分支再读一次 api.relocalize_status()，按 state 输出更准确的原因。
🛠️ 建议
- if not fn(x, y, yaw) then
- self:warn(string.format("reloc skip %s (in_flight)", mode))
- return false, nil
- end
+ if not fn(x, y, yaw) then
+ local st = api.relocalize_status()
+ self:warn(string.format("reloc skip %s | state=%d msg=%s",
+ mode, st.state, tostring(st.message)))
+ return false, st
+ end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lua/action.lua` around lines 95 - 99, The false return from fn(...) in
send_and_await conflates "in-flight" and "service unavailable" cases; modify
send_and_await to call api.relocalize_status() when fn(x,y,yaw) returns false,
inspect the returned status/state from that API, and log a distinct message
(e.g., "reloc skip <mode> (service_unavailable: ...)" vs the existing "reloc
skip <mode> (in_flight)") and propagate the same false,nil return; use the
function send_and_await and api.relocalize_status() as the insertion points to
read the accurate state and include it in the process warning.
| --- @field relocalize_initial fun(x: number, y: number, yaw: number): boolean | ||
| --- @field relocalize_local fun(x: number, y: number, yaw: number): boolean | ||
| --- @field relocalize_wide fun(x: number, y: number, yaw: number): boolean | ||
| --- @field relocalize_status fun(): { state: integer, success: boolean, message: string, fitness_score: number, confidence: number } |
There was a problem hiding this comment.
relocalize_status 注解漏了 estimated_* 字段
src/cxx/component.cc 中 relocalize_status 实际返回的 table 还包含 estimated_x/y/z 与 estimated_qx/qy/qz/qw，但这里只声明了 state/success/message/fitness_score/confidence。LSP 补全和静态检查都会少识别 7 个字段。
🛠️ 建议补全注解
---- `@field` relocalize_status fun(): { state: integer, success: boolean, message: string, fitness_score: number, confidence: number }
+--- `@field` relocalize_status fun(): { state: integer, success: boolean, message: string, fitness_score: number, confidence: number, estimated_x: number, estimated_y: number, estimated_z: number, estimated_qx: number, estimated_qy: number, estimated_qz: number, estimated_qw: number }📝 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
| --- @field relocalize_status fun(): { state: integer, success: boolean, message: string, fitness_score: number, confidence: number } | |
| --- `@field` relocalize_status fun(): { state: integer, success: boolean, message: string, fitness_score: number, confidence: number, estimated_x: number, estimated_y: number, estimated_z: number, estimated_qx: number, estimated_qy: number, estimated_qz: number, estimated_qw: number } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lua/api.lua` at line 23, relocalize_status 的注解缺少组件实际返回的 estimated_*
字段：在声明 relocalize_status（Lua 注解的 fun() 返回表）中补充
estimated_x、estimated_y、estimated_z 和
estimated_qx、estimated_qy、estimated_qz、estimated_qw 字段并把类型设为 number，以与
src/cxx/component.cc 中实际返回的 table 匹配，从而让 LSP 补全与静态检查识别这些字段。
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.
接入重定位——重新提了PR
重定位功能集成 PR 重新提交
概述
本 PR 重新应用了重定位功能集成，涉及与
rmcs_relocation依赖库的接入。项目中 main 分支之前的错误操作已被撤销。主要变更
依赖项和构建系统
CI 工作流 (
.github/workflows/ci.yml)：在测试步骤中新增一个步骤，克隆rmcs_relocation依赖到 RMCS 工作区 (/tmp/RMCS/rmcs_ws/src/skills/rmcs_relocation)，使用浅克隆 (--depth 1)。CMakeLists.txt：
rmcs_relocation依赖并配置其包含目录和链接库package.xml：添加了
rmcs_relocation的运行时依赖声明C++ 实现层面
本地化引擎重构 (
src/cxx/util/localization/)RelocalizeState和RelocalizeMode枚举RelocalizeStatus结构体，包含位置、旋转、适应度和置信度信息Localization::Config为仅包含服务名称和超时配置start_collecting()和start_localizing()方法relocalize(mode, x, y, yaw)和relocalize_status()接口导航组件 (
src/cxx/component.cc)Localization实例和relocalization_enabled参数enable_relocalization参数（默认启用）初始化重定位功能relocalize_initial、relocalize_local、relocalize_widerelocalize_status()Lua API，用于查询重定位状态Lua 脚本层面
动作层 (
src/lua/action.lua)RelocalizeState枚举send_and_await辅助函数）relocalize_initial(x, y, yaw)：执行初始重定位relocalize_local()：进行本地重定位，使用黑板中的用户位置作为初始值relocalize_wide()：进行全局重定位，如无有效位置则使用原点relocalize_status()方法用于查询重定位状态API 层 (
src/lua/api.lua)Api类型注解，新增重定位相关字段：relocalize_initial、relocalize_local、relocalize_wide、relocalize_status主要特点