refactor: feishu and cleaning namespace, with a agents.md - #40
Merged
Conversation
Move clock/context aliases to rmcs-level TimePoint usage and remove redundant rmcs-prefixed type aliases to make predictor and IPC interfaces cleaner and more consistent.
Contributor
|
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 22 minutes and 58 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 (3)
Walkthrough统一引入顶层时间类型 Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as AutoAim\nComponent
participant Feishu as Feishu\nClient
participant SHM as Shared\nMemory
Note over Comp,SHM: 类型化 Feishu 双向流程
Comp->>Feishu: start()
activate Feishu
Feishu->>SHM: 打开发送/接收通道 (基于 SendT::kLabel / RecvT::kLabel)
deactivate Feishu
loop 每次主循环
Comp->>Feishu: heartbeat()
Feishu-->>Comp: boolean (是否有新 recv)
alt 有新数据
Comp->>Feishu: latest()
Feishu->>SHM: 从缓冲读取最近 RecvT
Feishu-->>Comp: RecvT (ControlState)
Comp->>Comp: 条件覆盖/更新 AutoAimState
else 无新数据
Comp->>Comp: 保持或使用上次状态
end
Comp->>Comp: 计算 ControlState（基于追踪/火控/接管逻辑）
Comp->>Feishu: with_write(lambda -> send SendT)
Feishu->>SHM: 写入 SendT (ControlState) 且更新发送端状态
Feishu-->>Comp: 写入完成（无布尔 commit 分支）
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 Possibly related PRs
Suggested labels
诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/module/predictor/regular/snapshot.hpp (1)
1-13:⚠️ Potential issue | 🟡 Minor添加显式的
utility/clock.hpp包含以提高代码健壮性。虽然
TimePoint通过module/predictor/snapshot.hpp的传递包含可见，但此模块内存在不一致的模式：robot_state.hpp等文件显式包含utility/clock.hpp，而同样使用TimePoint的快照声明文件（本文件及outpost/snapshot.hpp）则依赖传递包含。建议添加
#include "utility/clock.hpp"来：
- 明确依赖关系
- 避免
snapshot.hpp头文件变化导致的编译失败风险- 与同模块其他文件的包含策略保持一致
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module/predictor/regular/snapshot.hpp` around lines 1 - 13, The header is relying on a transitive definition of TimePoint; add an explicit include of utility/clock.hpp at the top of src/module/predictor/regular/snapshot.hpp so TimePoint is defined without depending on module/predictor/snapshot.hpp changes—update the file containing the declaration of make_regular_snapshot(Snapshot::NormalEKF::XVec ekf_x, DeviceId device, CampColor color, int armor_num, TimePoint stamp) noexcept -> Snapshot to include "utility/clock.hpp" alongside the existing includes to make the dependency explicit and consistent with other files like robot_state.hpp.src/module/predictor/backend/robot_state_backend.cpp (1)
47-47:⚠️ Potential issue | 🟡 Minor命名空间结束注释与实际作用域不匹配。
第 9 行打开的是
namespace rmcs::predictor,本文件并未嵌套detail子命名空间,结束注释应改为// namespace rmcs::predictor,以免阅读时产生误导。✏️ 修复建议
-} // namespace rmcs::predictor::detail +} // namespace rmcs::predictor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module/predictor/backend/robot_state_backend.cpp` at line 47, 结束注释与实际命名空间不匹配：当前文件在第 9 行打开的是 namespace rmcs::predictor 但文件尾部的注释写成了 // namespace rmcs::predictor::detail，找到文件末尾闭合的右大括号对应的命名空间结束注释并将注释改为 // namespace rmcs::predictor 以匹配实际作用域（定位参考符号：namespace rmcs::predictor）。
🧹 Nitpick comments (8)
src/module/predictor/outpost/snapshot.hpp (1)
1-10: 建议直接#include "utility/clock.hpp"以显式暴露TimePoint依赖。现在签名里直接使用了未限定的
TimePoint，但本头文件并未直接包含utility/clock.hpp，依赖于module/predictor/snapshot.hpp等间接传递。按 IWYU（include-what-you-use）原则，显式包含可避免未来上游头文件精简时本文件编译失败，属于可选但推荐的小重构。♻️ 建议修改
`#include` "module/predictor/outpost/armor_layout.hpp" `#include` "module/predictor/snapshot.hpp" +#include "utility/clock.hpp" `#include` "utility/robot/color.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module/predictor/outpost/snapshot.hpp` around lines 1 - 10, 在头文件中显式包含 utility/clock.hpp 以暴露 TimePoint 的定义：在 src/module/predictor/outpost/snapshot.hpp 顶部新增 `#include` "utility/clock.hpp"（或等价头），这样 make_outpost_snapshot 的签名中使用的 TimePoint 不再依赖间接包含；保持其他 include 顺序不变，确保编译器能直接解析 TimePoint。src/module/capturer/local_video.cpp (1)
15-21: 可考虑直接复用rmcs::TimePoint，避免本地再定义。本 PR 在
src/utility/clock.hpp中新增了顶层rmcs::Clock与rmcs::TimePoint统一别名。此处Impl内部新加的Clock/TimePoint与其完全等价，可直接#include "utility/clock.hpp"后使用rmcs::TimePoint，与仓库内其他模块保持一致。当然，保留本地别名也不影响功能，属于可选重构。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module/capturer/local_video.cpp` around lines 15 - 21, Replace the locally defined Clock/TimePoint aliases in Impl with the shared aliases from utility/clock.hpp: include "utility/clock.hpp" and use rmcs::Clock and rmcs::TimePoint for the types currently declared as Clock and TimePoint (which affect symbols like capturer, interval_duration, last_read_time). This keeps behavior identical but aligns with project-wide typedefs and removes the duplicate local aliases.src/utility/clock.hpp (1)
6-9: 建议移除冗余的Timestamp别名，仅保留TimePoint。当前定义中
using Timestamp = TimePoint;与using TimePoint = Clock::time_point;完全重复。整个代码库中广泛使用TimePoint（tracker、predictor、image 等模块均采用），而Timestamp全局别名未被任何其他文件导入或使用（feishu.hpp 中的Timestamp是本地重定义，不依赖该全局别名）。保留两个相同的别名会增加命名空间噪声，易引发后续维护时的混用。建议删除Timestamp这一行，统一使用TimePoint。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/clock.hpp` around lines 6 - 9, Remove the redundant alias by deleting the line "using Timestamp = TimePoint;" from src/utility/clock.hpp so only Clock, TimePoint and Duration remain; search the repo for any uses of the global Timestamp alias and, if any are found, replace them with TimePoint (note feishu.hpp already has its own local Timestamp and does not depend on the removed alias) to avoid namespace noise and keep all time types unified around TimePoint.src/component.cpp (1)
42-43: 清理已失效的commit_control_state_failed节流项。原先
commit(control_state)存在失败分支，通过action_throttler.dispatch("commit_control_state_failed", ...)做节流日志；现在改为无条件feishu.send(...)，该 action 只剩下每帧一次的reset(...)，相当于死代码。建议要么移除注册与reset，要么在send中引入明确的失败返回并恢复 dispatch 分支，保持错误可观测性。♻️ 建议的最小清理
action_throttler.register_action("tf_not_ready"); - action_throttler.register_action("commit_control_state_failed"); } ... - feishu.send([&](auto& data) { data = control_state; }); - action_throttler.reset("commit_control_state_failed"); + feishu.send([&](auto& data) { data = control_state; }); }Also applies to: 138-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component.cpp` around lines 42 - 43, 当前已注册的节流项 "commit_control_state_failed" 已无实际 dispatch 使用，只剩下每帧调用的 reset，等同死代码；请清理或恢复错误分支：要么删除 action_throttler.register_action("commit_control_state_failed") 并移除对应的 reset 调用（清理死代码），要么在 commit(control_state) 的 feishu.send 路径中恢复明确的失败返回并重新调用 action_throttler.dispatch("commit_control_state_failed", ...) 以保持错误可观测性；定位相关符号：action_throttler.register_action, action_throttler.dispatch, feishu.send, reset, 以及原先触发 dispatch 的 commit(control_state) 分支，任选其一实施并相应更新注释。src/module/predictor/regular/robot_state.hpp (1)
1-19: 建议直接 includeutility/clock.hpp，与同级头文件保持一致。本文件在公共签名中使用
TimePoint（Line 16/18/19），但没有直接 include"utility/clock.hpp"，当前依赖module/predictor/snapshot.hpp的传递性可见性。同目录下src/module/predictor/robot_state.hpp:7和src/module/predictor/outpost/robot_state.hpp:8都显式包含了该头，建议这里也按 IWYU 对齐，避免未来重构snapshot.hpp时破坏编译。♻️ 建议改动
`#include` "module/predictor/regular/ekf_parameter.hpp" `#include` "module/predictor/snapshot.hpp" +#include "utility/clock.hpp" `#include` "utility/pimpl.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module/predictor/regular/robot_state.hpp` around lines 1 - 19, The header RegularRobotState uses TimePoint in its public API (constructor RegularRobotState(TimePoint), initialize and predict) but relies on transitive inclusion via module/predictor/snapshot.hpp; add a direct include of "utility/clock.hpp" at the top of src/module/predictor/regular/robot_state.hpp to satisfy IWYU and avoid breakage if snapshot.hpp changes, keeping the rest of the file unchanged.src/kernel/feishu.hpp (2)
108-108: 补一个命名空间结束注释(nit)。文件末尾
}缺少// namespace rmcs::kernel,和仓库其它文件风格一致会更好维护。✏️ 建议
-} +} // namespace rmcs::kernel🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernel/feishu.hpp` at line 108, Add a namespace end comment after the final closing brace to match project style: locate the closing brace that ends namespace rmcs::kernel in feishu.hpp and append a trailing comment like "// namespace rmcs::kernel" so the file's namespace terminator is explicitly annotated and consistent with other headers.
16-45:recv直接访问data.timestamp,但类型约束里没有保证它存在。类模板仅对
RecvT要求context_trait(只校验kLabel/kLength),而recv在第 41 行无条件读取data.timestamp。若将来有调用方用一个没有timestamp字段的类型实例化Feishu,recv会在实例化点爆出一大段模板错误,而不是在声明处给出清晰的约束诊断。建议把
timestamp_trait<RecvT>合并进类模板约束(或至少作为recv的requires约束),与第 80 行search中的static_assert(timestamp_trait<RecvT>)形成一致的契约:♻️ 建议改法
-template <context_trait SendT, context_trait RecvT> +template <context_trait SendT, context_trait RecvT> + requires timestamp_trait<RecvT> class Feishu {或者更局部地约束在
recv/search上。既然search已经依赖该 trait,把它提升到类层级更自然,并可移除第 80 行冗余的static_assert。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernel/feishu.hpp` around lines 16 - 45, The recv method in class template Feishu reads data.timestamp but the class-level context_trait only ensures kLabel/kLength for RecvT; add the timestamp requirement to the template constraints so instantiations fail with a clear diagnostic: require timestamp_trait<RecvT> (or include timestamp_trait in the class template parameter list) or add a requires clause on recv referencing timestamp_trait<RecvT>; update the class-level constraint (template <context_trait SendT, context_trait RecvT> -> include timestamp_trait for RecvT) so it matches the existing static_assert in search, and then remove the redundant static_assert; ensure references: Feishu, RecvT, recv, search, latest_timestamp, and recv_client.with_read are satisfied by the new trait.src/utility/shared/context.hpp (1)
25-26:kLength的语义略有歧义,建议考虑更明确的命名。当前
kLength在src/kernel/feishu.hppheartbeat 中被用作recv_buffer的条目数上限(deque 最大长度),但名字也可能被读者解读为"共享内存缓冲区字节长度"或"单条消息字节长度"。在跨进程消息 + 环形缓冲的语境下,这种歧义容易放大。若不涉及 ABI 兼容,推荐改为
kBufferCapacity/kHistoryCapacity一类的命名,同时在context_trait里也同步更新。若保留现名,建议在结构体上方注释里写清楚单位与用途。Also applies to: 77-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/shared/context.hpp` around lines 25 - 26, Rename the ambiguous static constexpr kLength to a clearer name (e.g., kBufferCapacity or kHistoryCapacity) and update all usages (including in context_trait and the heartbeat recv_buffer/deque size logic in kernel::feishu heartbeat) to the new identifier; alternatively, if you must keep the name, add a concise comment above the constant explaining its unit and purpose (that it denotes maximum number of entries in the recv_buffer/deque, not byte length) and mirror that comment in context_trait to avoid ambiguity.
🤖 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/kernel/feishu.hpp`:
- Around line 78-82: search 函数当前在遇到 target > latest_timestamp 时直接返回
std::nullopt，过于激进；请改为先计算差值并比较 max 容差：当 (target - latest_timestamp) > max（或等价的
Duration 比较）才短路返回 std::nullopt，否则不要早退，继续按现有逻辑将 latest_timestamp
对应的样本视为合法最近邻并返回（参考函数名 search、参数 target、latest_timestamp、max 的比较与 Duration
类型转换以确保类型安全）。
In `@src/runtime.cpp`:
- Around line 110-113: The empty if around feishu.heartbeat() in the main loop
(while (util::get_running())) ignores its meaningful boolean return; replace the
empty conditional with explicit intent: either (A) treat false as an error by
adding handling inside the if (e.g., log the failure via your logger, attempt
feishu.reconnect() or other recovery and/or break/stop the loop) or (B) if only
the side effects of heartbeat() are needed, remove the if and call
feishu.heartbeat(); (or explicitly cast to void to indicate ignored result).
Locate the call to feishu.heartbeat() and implement one of these two options so
the return value is no longer silently discarded.
- Around line 124-133: The code currently creates a new ControlState
(control_state) each frame and only calls feishu.recv(...) when feishu.updated()
is true, so the else branch's "use last cached value" message is misleading and
the default ControlState is silently sent downstream; fix by either (A) lifting
the ControlState instance (ControlState control_state) out of the per-frame loop
so it truly caches the last received value across frames and keep
feishu.recv(...) only when updated(), or (B) always call feishu.recv(...) each
frame to read the latest shared-memory value and use feishu.updated() solely to
throttle/log via action_throttler (control_state_label) without skipping the
recv; adjust action_throttler.reset/dispatch usage accordingly to match the
chosen approach.
---
Outside diff comments:
In `@src/module/predictor/backend/robot_state_backend.cpp`:
- Line 47: 结束注释与实际命名空间不匹配：当前文件在第 9 行打开的是 namespace rmcs::predictor 但文件尾部的注释写成了
// namespace rmcs::predictor::detail，找到文件末尾闭合的右大括号对应的命名空间结束注释并将注释改为 // namespace
rmcs::predictor 以匹配实际作用域（定位参考符号：namespace rmcs::predictor）。
In `@src/module/predictor/regular/snapshot.hpp`:
- Around line 1-13: The header is relying on a transitive definition of
TimePoint; add an explicit include of utility/clock.hpp at the top of
src/module/predictor/regular/snapshot.hpp so TimePoint is defined without
depending on module/predictor/snapshot.hpp changes—update the file containing
the declaration of make_regular_snapshot(Snapshot::NormalEKF::XVec ekf_x,
DeviceId device, CampColor color, int armor_num, TimePoint stamp) noexcept ->
Snapshot to include "utility/clock.hpp" alongside the existing includes to make
the dependency explicit and consistent with other files like robot_state.hpp.
---
Nitpick comments:
In `@src/component.cpp`:
- Around line 42-43: 当前已注册的节流项 "commit_control_state_failed" 已无实际 dispatch
使用，只剩下每帧调用的 reset，等同死代码；请清理或恢复错误分支：要么删除
action_throttler.register_action("commit_control_state_failed") 并移除对应的 reset
调用（清理死代码），要么在 commit(control_state) 的 feishu.send 路径中恢复明确的失败返回并重新调用
action_throttler.dispatch("commit_control_state_failed", ...)
以保持错误可观测性；定位相关符号：action_throttler.register_action, action_throttler.dispatch,
feishu.send, reset, 以及原先触发 dispatch 的 commit(control_state) 分支，任选其一实施并相应更新注释。
In `@src/kernel/feishu.hpp`:
- Line 108: Add a namespace end comment after the final closing brace to match
project style: locate the closing brace that ends namespace rmcs::kernel in
feishu.hpp and append a trailing comment like "// namespace rmcs::kernel" so the
file's namespace terminator is explicitly annotated and consistent with other
headers.
- Around line 16-45: The recv method in class template Feishu reads
data.timestamp but the class-level context_trait only ensures kLabel/kLength for
RecvT; add the timestamp requirement to the template constraints so
instantiations fail with a clear diagnostic: require timestamp_trait<RecvT> (or
include timestamp_trait in the class template parameter list) or add a requires
clause on recv referencing timestamp_trait<RecvT>; update the class-level
constraint (template <context_trait SendT, context_trait RecvT> -> include
timestamp_trait for RecvT) so it matches the existing static_assert in search,
and then remove the redundant static_assert; ensure references: Feishu, RecvT,
recv, search, latest_timestamp, and recv_client.with_read are satisfied by the
new trait.
In `@src/module/capturer/local_video.cpp`:
- Around line 15-21: Replace the locally defined Clock/TimePoint aliases in Impl
with the shared aliases from utility/clock.hpp: include "utility/clock.hpp" and
use rmcs::Clock and rmcs::TimePoint for the types currently declared as Clock
and TimePoint (which affect symbols like capturer, interval_duration,
last_read_time). This keeps behavior identical but aligns with project-wide
typedefs and removes the duplicate local aliases.
In `@src/module/predictor/outpost/snapshot.hpp`:
- Around line 1-10: 在头文件中显式包含 utility/clock.hpp 以暴露 TimePoint 的定义：在
src/module/predictor/outpost/snapshot.hpp 顶部新增 `#include`
"utility/clock.hpp"（或等价头），这样 make_outpost_snapshot 的签名中使用的 TimePoint
不再依赖间接包含；保持其他 include 顺序不变，确保编译器能直接解析 TimePoint。
In `@src/module/predictor/regular/robot_state.hpp`:
- Around line 1-19: The header RegularRobotState uses TimePoint in its public
API (constructor RegularRobotState(TimePoint), initialize and predict) but
relies on transitive inclusion via module/predictor/snapshot.hpp; add a direct
include of "utility/clock.hpp" at the top of
src/module/predictor/regular/robot_state.hpp to satisfy IWYU and avoid breakage
if snapshot.hpp changes, keeping the rest of the file unchanged.
In `@src/utility/clock.hpp`:
- Around line 6-9: Remove the redundant alias by deleting the line "using
Timestamp = TimePoint;" from src/utility/clock.hpp so only Clock, TimePoint and
Duration remain; search the repo for any uses of the global Timestamp alias and,
if any are found, replace them with TimePoint (note feishu.hpp already has its
own local Timestamp and does not depend on the removed alias) to avoid namespace
noise and keep all time types unified around TimePoint.
In `@src/utility/shared/context.hpp`:
- Around line 25-26: Rename the ambiguous static constexpr kLength to a clearer
name (e.g., kBufferCapacity or kHistoryCapacity) and update all usages
(including in context_trait and the heartbeat recv_buffer/deque size logic in
kernel::feishu heartbeat) to the new identifier; alternatively, if you must keep
the name, add a concise comment above the constant explaining its unit and
purpose (that it denotes maximum number of entries in the recv_buffer/deque, not
byte length) and mirror that comment in context_trait to avoid ambiguity.
🪄 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: 4accaa93-3267-47dc-804b-0bb8bbf8d66e
📒 Files selected for processing (35)
AGENTS.mdsrc/component.cppsrc/kernel/common.hppsrc/kernel/feishu.hppsrc/kernel/fire_control.hppsrc/kernel/pose_estimator.hppsrc/kernel/tracker.cppsrc/kernel/tracker.hppsrc/module/capturer/local_video.cppsrc/module/debug/framerate.hppsrc/module/identifier/armor_detection.cppsrc/module/predictor/backend/robot_state_backend.cppsrc/module/predictor/backend/robot_state_backend.hppsrc/module/predictor/backend/snapshot_backend.hppsrc/module/predictor/outpost/robot_state.cppsrc/module/predictor/outpost/robot_state.hppsrc/module/predictor/outpost/snapshot.cppsrc/module/predictor/outpost/snapshot.hppsrc/module/predictor/regular/robot_state.cppsrc/module/predictor/regular/robot_state.hppsrc/module/predictor/regular/snapshot.cppsrc/module/predictor/regular/snapshot.hppsrc/module/predictor/robot_state.cppsrc/module/predictor/robot_state.hppsrc/module/predictor/snapshot.cppsrc/module/predictor/snapshot.hppsrc/module/tracker/decider.cppsrc/module/tracker/decider.hppsrc/runtime.cppsrc/utility/clock.hppsrc/utility/image/image.cppsrc/utility/image/image.hppsrc/utility/shared/context.hpptest/feishu_test.cpptest/transform_communication.cpp
💤 Files with no reviewable changes (1)
- src/kernel/fire_control.hpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/runtime.cpp (1)
106-106:⚠️ Potential issue | 🟡 Minor
updated变量被赋值后未使用。
auto updated = feishu.heartbeat();之后整个循环中再无对updated的引用，heartbeat()的返回值被静默丢弃。鉴于过去评审已就同一调用点的"返回值未被处理"提过类似问题，建议明确意图：要么feishu.heartbeat();（直接丢弃），要么真正用updated做新鲜度门控（例如只在新一帧 control_state 到达时刷新received，避免后面的feishu.latest()在长时间无新数据时持续返回陈旧值）。🛠 简单修复
- auto updated = feishu.heartbeat(); + feishu.heartbeat();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime.cpp` at line 106, The assignment auto updated = feishu.heartbeat(); currently discards the return value; either drop the assignment and call feishu.heartbeat(); directly if its return is intentionally ignored, or use updated to gate freshness: inside the loop check updated (the bool/flag returned by feishu.heartbeat()) and only refresh received from feishu.latest() when updated is true or when control_state changes, so you avoid returning stale values—update the code paths around feishu.heartbeat(), feishu.latest(), control_state and received accordingly to reflect the chosen intent.
🧹 Nitpick comments (5)
src/component.cpp (1)
44-44:commit_control_state_failed节流器已经无写失败路径，建议清理。
feishu.with_write(...)不再返回 commit 成功/失败，Line 140 的action_throttler.reset("commit_control_state_failed")没有任何配对的dispatch，而 Line 44 注册的同名 action 也只在此处被 reset。建议把这条注册和 reset 一并删除，避免遗留死代码误导后续维护者。♻️ 建议删除
action_throttler.register_action("tf_not_ready"); - action_throttler.register_action("commit_control_state_failed"); }feishu.with_write([&](auto& data) { data = control_state; }); - action_throttler.reset("commit_control_state_failed"); }Also applies to: 140-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component.cpp` at line 44, Remove the dead throttler action for "commit_control_state_failed": delete the register_action("commit_control_state_failed") call and the matching action_throttler.reset("commit_control_state_failed") since feishu.with_write(...) no longer returns commit success/failure and there is no dispatch of this action; verify no other references to "commit_control_state_failed" remain and remove them to avoid leftover dead code.src/utility/shared/context.hpp (2)
68-68:static_assert(context_trait<...>)没有验证kLabel/kLength—— 这点值得显式补充。由于本文件的
context_trait只校验"trivially copyable"，而 Feishu 模板真正依赖的kLabel/kLength概念是rmcs::kernel::context_trait，这两个static_assert实际上漏检了 IPC 必备元数据。如果未来误删了kLabel/kLength，本文件不会报错，错误会推迟到 Feishu 实例化才暴露。可以考虑直接static_assert(rmcs::kernel::context_trait<AutoAimState>)加固。Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/shared/context.hpp` at line 68, 当前对 AutoAimState 的静态断言仅使用本文件的 context_trait（只检查 trivially copyable），未保证 IPC 元数据 kLabel/kLength 存在；请将对应断言改为使用 rmcs::kernel::context_trait（例如 static_assert(rmcs::kernel::context_trait<AutoAimState>)）以强制验证 kLabel/kLength 等 IPC 必需概念，并对文件中所有类似位置（包括第105行对应断言）一并替换，确保在编译期就能捕获缺失元数据的问题。
11-12:context_trait与rmcs::kernel::context_trait同名但语义完全不同，建议重命名以避免误导。本文件在
namespace rmcs中定义的context_trait<T>仅检查std::is_trivially_copyable_v<T>；而src/kernel/feishu.hpp在namespace rmcs::kernel中定义的同名context_trait要求T::kLabel与T::kLength。两个 concept 的字面名一样但约束完全不同，仅靠所在命名空间区分。在阅读static_assert(context_trait<AutoAimState>)时（Line 68 / 105），不结合 ADL/namespace 上下文很容易误以为这里也在校验kLabel/kLength。建议把本文件内的概念改名为更直白的
trivially_shareable/shm_trivial之类，或者反之让 feishu.hpp 那个改名为feishu_message，避免后续维护者踩坑。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/shared/context.hpp` around lines 11 - 12, The concept context_trait in namespace rmcs conflicts semantically with rmcs::kernel::context_trait (one checks std::is_trivially_copyable_v<T>, the other requires kLabel/kLength); rename the trivial copy concept (e.g., to trivially_shareable or shm_trivial) in the header where context_trait is defined, update all local uses (e.g., static_assert(context_trait<AutoAimState>) and any templates or constraints referencing context_trait) to the new name, and run a quick project-wide search for context_trait to ensure only the intended kernel concept remains under rmcs::kernel::context_trait or alternatively rename the kernel concept (e.g., feishu_message) if that fits your design.src/utility/logging_util.hpp (1)
17-17:std::string_view作为 key 存在悬垂风险。
store的 key 是std::string_view，目前调用点（logging.reset("receive", 5)等）都是字符串字面量、生命周期为静态存储期，所以暂时无问题。但如果未来有调用方传入std::string的临时对象或局部string，键将悬垂导致 UB。建议要么把 key 改为std::string，要么在文档/契约中明确"name 必须具有静态存储期"。Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/logging_util.hpp` at line 17, The unordered_map member store uses std::string_view keys which can dangle; change the map key to std::string (i.e., std::unordered_map<std::string, std::int16_t> store) and update any call sites (e.g., logging.reset("receive", 5)) to rely on implicit conversion or explicitly construct std::string where necessary so stored keys own their data; adjust constructors/insert/emplace usages in the class (look for store and the reset method) to copy names into the map and remove any requirement that callers provide static-lifetime strings.src/kernel/feishu.hpp (1)
49-53: 每次heartbeat()调用都会尝试open()，在对端未启动时反复执行系统调用。
heartbeat()在主循环中每帧调用一次（src/runtime.cpp:106），而start()使用opened() || open(...)模式——当连接未建立时，每次都会执行：
- Send 端：
shm_open()+ftruncate()+mmap()- Recv 端：
shm_open()+mmap()这意味着主循环中每帧都会触发多个系统调用。在对端尚未启动的场景下，这会导致连续的系统调用堆积。建议添加失败重试的退避机制（如失败后短时间内不再重试）或在外部控制调度频率。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernel/feishu.hpp` around lines 49 - 53, start() currently calls send_client.open(...) and recv_client.open(...) on every heartbeat (via heartbeat()), causing repeated shm_open/ftruncate/mmap system calls when the peer isn't ready; add a retry/backoff so failed opens are not retried every frame. Concretely: add per-endpoint state (e.g., last_send_open_attempt, last_recv_open_attempt timestamps or a failed_until deadline) inside the Feishu class and in start() only call send_client.open(SendT::kLabel) or recv_client.open(RecvT::kLabel) if the corresponding opened() is false AND enough time has elapsed since the last failed attempt; on failure update the timestamp/failed_until to impose a short delay (e.g., 100–500ms) before the next open() attempt so heartbeat() no longer triggers system calls every frame. Ensure you reference send_client.opened(), send_client.open(...), recv_client.opened(), recv_client.open(...) and heartbeat() when making the change.
🤖 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/component.cpp`:
- Around line 129-133: 在 handle_tf_not_ready 中当前对 control_state 进行 control_state
= ControlState::kInvalid() 会被后续 update_control_state 覆盖且不会触发
publish_control_state()，所以请根据意图处理：如果希望在 tf 未就绪期间通知对端发送 invalid 状态，则在该分支内调用
feishu.with_write(...)（用现有写入/发布路径直接写出并发送包含 kInvalid 的 control_state 或调用
publish_control_state() 的等价写入操作）；如果只是想清空内部状态且不通知对端，则删除 control_state =
ControlState::kInvalid() 这一行。保持现有对
publish_auto_aim_outputs(make_invalid_auto_aim_state()) 的调用不变或与写入操作配对发送。
In `@src/runtime.cpp`:
- Around line 117-125: feishu.latest() can return stale ControlState from the
circular buffer, so add a freshness check against received.timestamp using the
same timeout logic as kAutoAimTimeout/has_fresh_auto_aim_state(): after calling
feishu.latest() in the receive block, compare now - received.timestamp to the
timeout and if expired set received = ControlState::kInvalid() (or skip invoking
tracker.set_invincible_armors, pose_estimator.set_odom_to_camera_transform and
fire_control.solve with received.yaw) and log a warning; ensure you reference
feishu.latest(), received.timestamp, ControlState::kInvalid(), kAutoAimTimeout,
has_fresh_auto_aim_state(), tracker.set_invincible_armors,
pose_estimator.set_odom_to_camera_transform and fire_control.solve when making
the change.
In `@src/utility/logging_util.hpp`:
- Around line 35-48: The lambdas used in info/warn/error (inside the templated
methods info, warn, error) capture Args by value with “[=, this]” but are const
by default, so using std::forward<Args>(args) can fail for non-reference deduced
Args; either mark the lambda mutable (e.g., exec(name, [=, this] mutable { ...
})) so std::forward is valid, or stop using std::forward and pass args by value
into rclcpp.* (e.g., rclcpp.info(fmt, args...)); update all three methods
consistently and keep the call through exec unchanged.
In `@src/utility/shared/context.hpp`:
- Around line 57-66: AutoAimState::kInvalid() and ControlState::kInvalid()
currently set timestamp via Clock::now(), which makes "invalid" states appear
fresh to has_fresh_auto_aim_state() (which compares auto_aim_state.timestamp
against kAutoAimTimeout); change both kInvalid() implementations to initialize
timestamp to a zero/epoch value (e.g., TimePoint{} or TimePoint::min()) instead
of Clock::now(), and ensure ControlState::kInvalid() does not default-initialize
capture_signals with Clock::now() (construct them with epoch/empty timestamps)
so that invalid states are unambiguously treated as stale by downstream
freshness checks like has_fresh_auto_aim_state().
---
Duplicate comments:
In `@src/runtime.cpp`:
- Line 106: The assignment auto updated = feishu.heartbeat(); currently discards
the return value; either drop the assignment and call feishu.heartbeat();
directly if its return is intentionally ignored, or use updated to gate
freshness: inside the loop check updated (the bool/flag returned by
feishu.heartbeat()) and only refresh received from feishu.latest() when updated
is true or when control_state changes, so you avoid returning stale
values—update the code paths around feishu.heartbeat(), feishu.latest(),
control_state and received accordingly to reflect the chosen intent.
---
Nitpick comments:
In `@src/component.cpp`:
- Line 44: Remove the dead throttler action for "commit_control_state_failed":
delete the register_action("commit_control_state_failed") call and the matching
action_throttler.reset("commit_control_state_failed") since
feishu.with_write(...) no longer returns commit success/failure and there is no
dispatch of this action; verify no other references to
"commit_control_state_failed" remain and remove them to avoid leftover dead
code.
In `@src/kernel/feishu.hpp`:
- Around line 49-53: start() currently calls send_client.open(...) and
recv_client.open(...) on every heartbeat (via heartbeat()), causing repeated
shm_open/ftruncate/mmap system calls when the peer isn't ready; add a
retry/backoff so failed opens are not retried every frame. Concretely: add
per-endpoint state (e.g., last_send_open_attempt, last_recv_open_attempt
timestamps or a failed_until deadline) inside the Feishu class and in start()
only call send_client.open(SendT::kLabel) or recv_client.open(RecvT::kLabel) if
the corresponding opened() is false AND enough time has elapsed since the last
failed attempt; on failure update the timestamp/failed_until to impose a short
delay (e.g., 100–500ms) before the next open() attempt so heartbeat() no longer
triggers system calls every frame. Ensure you reference send_client.opened(),
send_client.open(...), recv_client.opened(), recv_client.open(...) and
heartbeat() when making the change.
In `@src/utility/logging_util.hpp`:
- Line 17: The unordered_map member store uses std::string_view keys which can
dangle; change the map key to std::string (i.e., std::unordered_map<std::string,
std::int16_t> store) and update any call sites (e.g., logging.reset("receive",
5)) to rely on implicit conversion or explicitly construct std::string where
necessary so stored keys own their data; adjust constructors/insert/emplace
usages in the class (look for store and the reset method) to copy names into the
map and remove any requirement that callers provide static-lifetime strings.
In `@src/utility/shared/context.hpp`:
- Line 68: 当前对 AutoAimState 的静态断言仅使用本文件的 context_trait（只检查 trivially
copyable），未保证 IPC 元数据 kLabel/kLength 存在；请将对应断言改为使用
rmcs::kernel::context_trait（例如
static_assert(rmcs::kernel::context_trait<AutoAimState>)）以强制验证 kLabel/kLength 等
IPC 必需概念，并对文件中所有类似位置（包括第105行对应断言）一并替换，确保在编译期就能捕获缺失元数据的问题。
- Around line 11-12: The concept context_trait in namespace rmcs conflicts
semantically with rmcs::kernel::context_trait (one checks
std::is_trivially_copyable_v<T>, the other requires kLabel/kLength); rename the
trivial copy concept (e.g., to trivially_shareable or shm_trivial) in the header
where context_trait is defined, update all local uses (e.g.,
static_assert(context_trait<AutoAimState>) and any templates or constraints
referencing context_trait) to the new name, and run a quick project-wide search
for context_trait to ensure only the intended kernel concept remains under
rmcs::kernel::context_trait or alternatively rename the kernel concept (e.g.,
feishu_message) if that fits your design.
🪄 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: 6c00119f-c605-4d40-ad14-92b633c8f04a
📒 Files selected for processing (8)
src/component.cppsrc/kernel/feishu.hppsrc/runtime.cppsrc/utility/logging_util.hppsrc/utility/shared/context.hpptest/CMakeLists.txttest/action_throttler.cpptest/feishu_test.cpp
💤 Files with no reviewable changes (2)
- test/action_throttler.cpp
- test/CMakeLists.txt
Comment on lines
129
to
133
| auto handle_tf_not_ready() -> void { | ||
| action_throttler.dispatch("tf_not_ready", [&] { rclcpp.warn("rmcs_tf is not ready"); }); | ||
| control_state.reset(); | ||
| control_state = ControlState::kInvalid(); | ||
| publish_auto_aim_outputs(make_invalid_auto_aim_state()); | ||
| } |
Contributor
There was a problem hiding this comment.
handle_tf_not_ready 中 control_state = ControlState::kInvalid() 实际上是死赋值。
该分支在 Line 50 直接 return，并不会走到 publish_control_state()；下一帧 tf 就绪后，update_control_state() 会把 timestamp/yaw/pitch/odom_to_camera_transform/invincible_devices 等字段全部覆盖。也就是说这次 kInvalid() 赋值不会被发送出去，也不会被下一次发送看到。
如果意图是"tf 未就绪期间也要让对端收到 invalid 信号"，应改为在此分支 feishu.with_write(...) 一次；如果意图只是把内部状态清空，那么这一行可以直接删掉。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component.cpp` around lines 129 - 133, 在 handle_tf_not_ready 中当前对
control_state 进行 control_state = ControlState::kInvalid() 会被后续
update_control_state 覆盖且不会触发 publish_control_state()，所以请根据意图处理：如果希望在 tf
未就绪期间通知对端发送 invalid 状态，则在该分支内调用 feishu.with_write(...)（用现有写入/发布路径直接写出并发送包含
kInvalid 的 control_state 或调用 publish_control_state()
的等价写入操作）；如果只是想清空内部状态且不通知对端，则删除 control_state = ControlState::kInvalid()
这一行。保持现有对 publish_auto_aim_outputs(make_invalid_auto_aim_state())
的调用不变或与写入操作配对发送。
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/runtime.cpp (1)
51-51: 变量名without_rmcs与配置键is_local_runtime语义反向，建议统一。读取的是
configuration["is_local_runtime"]，但局部变量改名为without_rmcs。两者在布尔语义上确实等价（无 RMCS ↔ 本地运行），但命名反向会让 Line 118 的if (!without_rmcs && updated)多一层心智负担——读者需先把 "without_rmcs == is_local_runtime" 在脑内换算一次，然后再加一个否定。建议直接命名为
is_local_runtime，与配置 key 保持一致：♻️ 命名对齐
- auto without_rmcs = configuration["is_local_runtime"].as<bool>(); + auto is_local_runtime = configuration["is_local_runtime"].as<bool>(); @@ - if (!without_rmcs && updated) { + if (!is_local_runtime && updated) { received = *feishu.latest(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime.cpp` at line 51, The local variable name without_rmcs is confusing relative to the config key configuration["is_local_runtime"]; rename the variable to is_local_runtime to match the config key (e.g., change auto without_rmcs = configuration["is_local_runtime"].as<bool>(); to auto is_local_runtime = ...), then update all usages of without_rmcs throughout the file (for example change if (!without_rmcs && updated) to if (!is_local_runtime && updated) so behavior stays identical while names align and you remove the mental inversion.
🤖 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/runtime.cpp`:
- Around line 34-36: The "receive" logging channel is registered via
logging.reset("receive", 5) but never used later, indicating either missing log
calls for ControlState receive failures/timeouts or dead code; update the
ControlState freshness/receive handling (where ControlState is validated in the
main loop) to emit logging.warn("receive", ...) or logging.error("receive", ...)
on stale/missing/timeout conditions, or remove the logging.reset("receive", 5)
call if you decide the receive logs are not needed—ensure you modify the code
paths that process ControlState (the main loop that checks freshness) so they
call the "receive" channel when appropriate to keep registrations consistent.
- Around line 117-120: The current logic discards buffered ControlState when
updated==false and uses ControlState::kInvalid(), causing NaNs to flow into
pose_estimator.set_odom_to_camera_transform, fire_control.solve and feishu.send;
instead always read feishu.latest() (regardless of feishu.heartbeat()/updated),
check the latest state's timestamp against a freshness threshold (reuse the
kAutoAimTimeout/has_fresh_auto_aim_state() pattern from src/component.cpp) and
only fallback to ControlState::kInvalid() when the buffer is empty or the latest
entry is stale; if stale/empty, explicitly skip downstream consumers or set a
clear “no-fresh-control” flag so pose_estimator, fire_control.solve and
feishu.send do not consume NaN values.
---
Nitpick comments:
In `@src/runtime.cpp`:
- Line 51: The local variable name without_rmcs is confusing relative to the
config key configuration["is_local_runtime"]; rename the variable to
is_local_runtime to match the config key (e.g., change auto without_rmcs =
configuration["is_local_runtime"].as<bool>(); to auto is_local_runtime = ...),
then update all usages of without_rmcs throughout the file (for example change
if (!without_rmcs && updated) to if (!is_local_runtime && updated) so behavior
stays identical while names align and you remove the mental inversion.
🪄 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: cebff857-adbe-4e51-9236-8c46f1b66e4b
📒 Files selected for processing (2)
src/runtime.cppsrc/utility/tf/static_tf.hpp
✅ Files skipped from review due to trivial changes (1)
- src/utility/tf/static_tf.hpp
Merged
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 包含三类主要工作：新增代理行为文档（AGENTS.md）、重构 Feishu 共享内存通信抽象，以及将一系列时间/工具类型与共享上下文从 rmcs::util 收拢并清理为顶级 rmcs 命名空间。变更范围广泛，既有架构/API 层面的公开接口调整，也有大量类型别名替换与格式化净化。
主要改动
新增：AGENTS.md
Feishu 库重构（重大）
时间类型与命名空间统一
共享上下文与状态结构重构（高风险、API 改变）
组件与运行时适配
新增工具与清理
对公开 API / 导出实体的影响（要点）
其它说明与代码质量