refactor: single process#59
Conversation
Walkthrough该PR重构RMCS自瞄系统的运行时架构：移除独立的 ChangesAutoAim架构重构与运行时迁移
Sequence DiagramsequenceDiagram
participant Component as AutoAimComponent (主线程)
participant AutoAim as AutoAim (worker 线程)
participant Modules as Capturer/Identifier/Tracker/PoseEstimator/FireControl
Component->>AutoAim: with_context(SystemContext)
Note over AutoAim: 写入相机变换、炮管方向、时间戳
AutoAim->>Modules: 执行循环：抓图→识别→估计→跟踪→火控
Modules-->>AutoAim: 识别/3D/Target → AutoAimState
AutoAim->>AutoAim: mutex 写 current_command；atomic 设置 unread_command
Component->>AutoAim: command_updated()
alt 有命令且时间戳有效
Component->>AutoAim: with_command(读取状态)
AutoAim-->>Component: 返回 AutoAimState
Component->>Component: 应用 yaw/pitch/射击 输出
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/component.cppIn file included from src/component.cpp:1: ... [truncated 2200 characters] ... src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 src/utility/image/drawable.cppIn file included from src/utility/image/drawable.cpp:1: ... [truncated 2200 characters] ... langFrontend__CTrans.CTrans_funct.instruction_log.(fun) in file "src/clang/cTrans.ml", line 4784, characters 10-1023 src/kernel/auto_aim.cppIn file included from src/kernel/auto_aim.cpp:1: ... [truncated 1120 characters] ... ang/install/lib/clang/18/include" 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: 1
🧹 Nitpick comments (4)
src/component.cpp (1)
73-73: 💤 Low value可选：记录过时指令的丢弃情况
当指令时间戳超过 100ms 时会被静默丢弃。如果指令频繁过时，可能表明性能瓶颈。建议添加周期性日志或计数器以便诊断：
if (Clock::now() - cmd.timestamp > 100ms) { // 可选：添加速率限制的警告日志 // static auto last_warn = Clock::now(); // if (Clock::now() - last_warn > 5s) { // logger.warn("Discarding stale command (age: {}ms)", ...); // last_warn = Clock::now(); // } return; }这有助于在调试时发现组件更新频率不匹配的问题。
🤖 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/component.cpp` at line 73, Currently stale commands are silently dropped when Clock::now() - cmd.timestamp > 100ms; add a rate-limited diagnostic to surface frequent drops by incrementing a counter or emitting a periodic warning. Locate the check using Clock::now() and cmd.timestamp in the handler (the if that returns on >100ms) and implement either a static timestamp (e.g., last_warn) or a counter with a time window to call logger.warn("Discarding stale command (age: {}ms)", age) only every few seconds (or when a threshold count is reached), or increment a metrics counter so operators can detect frequent stale-command drops.src/kernel/auto_aim.cpp (1)
159-159: ⚡ Quick win建议显式转换信号处理函数
当前使用无捕获 lambda 作为信号处理器，可以隐式转换为函数指针。为提高可读性和明确意图，建议使用一元
+运算符显式转换：-std::signal(SIGINT, [](int) { util::set_running(false); }); +std::signal(SIGINT, +[](int) { util::set_running(false); });或者使用
extern "C"函数以确保 C 链接：extern "C" void sigint_handler(int) { util::set_running(false); } std::signal(SIGINT, sigint_handler);🤖 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/kernel/auto_aim.cpp` at line 159, The current call std::signal(SIGINT, [](int){ util::set_running(false); }) relies on implicit conversion of a no-capture lambda to a function pointer; change it to an explicit conversion or a C-linkage handler to make intent clear and safe: either use the unary + operator to convert the lambda to a function pointer when passing to std::signal, or replace the lambda with an extern "C" function (e.g., sigint_handler) that calls util::set_running(false) and pass that to std::signal; update the signal registration accordingly to reference std::signal, SIGINT, the converted lambda or sigint_handler.src/kernel/auto_aim.hpp (1)
15-27: 💤 Low value可选：考虑允许回调返回值
当前
with_context和with_command的返回类型为auto（推导为void），回调函数的返回值被丢弃。如果希望支持从回调中提取数据的用例，可以改为返回回调的结果：template <typename WithFunc> requires std::invocable<WithFunc, SystemContext&> auto with_context(WithFunc&& func) -> decltype(auto) { std::lock_guard lock(context_mutex); return func(current_context); } template <typename WithFunc> requires std::invocable<WithFunc, const AutoAimState&> auto with_command(WithFunc&& func) -> decltype(auto) { std::lock_guard lock(command_mutex); return func(current_command); }这样调用方可以选择返回值或仅执行副作用。当前设计仅用于副作用也是合理的，此建议为可选改进。
🤖 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/kernel/auto_aim.hpp` around lines 15 - 27, with_context and with_command currently discard the callback return value (they deduce to void); change both to return the callback result by using a trailing return type -> decltype(auto) and returning func(...) while still holding the lock (for with_context: use decltype(auto) return, lock context_mutex, return func(current_context); for with_command: use decltype(auto) return, lock command_mutex, return func(current_command)); keep the existing requires constraints (std::invocable<WithFunc, SystemContext&> and std::invocable<WithFunc, const AutoAimState&>) and the same mutex/variable names to locate the change.src/module/fire_control/aim_point_chooser.hpp (1)
3-13: ⚡ Quick win多个头文件存在同一根因：include 分组顺序不符合仓库约定。
以上位置都应统一为“本地头文件 → 标准库 → 第三方库”，并用空行分组；这是同一规范问题，建议一次性批量修正，避免后续冲突和风格漂移。
As per coding guidelines, “Header files should follow the order: local headers, standard library, third-party libraries, with blank lines separating these groups”。
🤖 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/module/fire_control/aim_point_chooser.hpp` around lines 3 - 13, The include order in aim_point_chooser.hpp violates the project's grouping convention; reorder the `#include` lines so local headers (e.g. "utility/pimpl.hpp", "utility/robot/armor.hpp") come first, then C++ standard library headers (e.g. <expected>, <optional>, <span>, <string>), and finally third‑party/externals (e.g. <eigen3/Eigen/Core>, <yaml-cpp/yaml.h>), with a blank line between each group; apply the same grouped ordering consistently to any other files that use these headers to avoid style drift.Source: Coding guidelines
🤖 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/component.cpp`:
- Around line 59-60: The pitch sign is inconsistent between visualization and
control: ensure you document or align the convention by either removing the
negation in visual.update_aiming_direction(-cmd.pitch) or explicitly explaining
the reason for the flip; add a clear comment in src/component.cpp around
ctx.pitch and where ctx.yaw is passed to fire.solve describing the chosen
Z-axis/pitch sign convention, and add matching comments in
trajectory_solution.cpp (where pitch = std::atan2(target_h, target_d) produces
result->pitch) and in src/kernel/auto_aim.cpp
(visual.update_aiming_direction(cmd.yaw, -cmd.pitch)) that state how
/auto_aim/barrel_direction and aim_point/target_position.z define positive Z and
why visualization negates pitch, so future readers know whether to flip or not
and avoid semantic mismatches.
---
Nitpick comments:
In `@src/component.cpp`:
- Line 73: Currently stale commands are silently dropped when Clock::now() -
cmd.timestamp > 100ms; add a rate-limited diagnostic to surface frequent drops
by incrementing a counter or emitting a periodic warning. Locate the check using
Clock::now() and cmd.timestamp in the handler (the if that returns on >100ms)
and implement either a static timestamp (e.g., last_warn) or a counter with a
time window to call logger.warn("Discarding stale command (age: {}ms)", age)
only every few seconds (or when a threshold count is reached), or increment a
metrics counter so operators can detect frequent stale-command drops.
In `@src/kernel/auto_aim.cpp`:
- Line 159: The current call std::signal(SIGINT, [](int){
util::set_running(false); }) relies on implicit conversion of a no-capture
lambda to a function pointer; change it to an explicit conversion or a C-linkage
handler to make intent clear and safe: either use the unary + operator to
convert the lambda to a function pointer when passing to std::signal, or replace
the lambda with an extern "C" function (e.g., sigint_handler) that calls
util::set_running(false) and pass that to std::signal; update the signal
registration accordingly to reference std::signal, SIGINT, the converted lambda
or sigint_handler.
In `@src/kernel/auto_aim.hpp`:
- Around line 15-27: with_context and with_command currently discard the
callback return value (they deduce to void); change both to return the callback
result by using a trailing return type -> decltype(auto) and returning func(...)
while still holding the lock (for with_context: use decltype(auto) return, lock
context_mutex, return func(current_context); for with_command: use
decltype(auto) return, lock command_mutex, return func(current_command)); keep
the existing requires constraints (std::invocable<WithFunc, SystemContext&> and
std::invocable<WithFunc, const AutoAimState&>) and the same mutex/variable names
to locate the change.
In `@src/module/fire_control/aim_point_chooser.hpp`:
- Around line 3-13: The include order in aim_point_chooser.hpp violates the
project's grouping convention; reorder the `#include` lines so local headers (e.g.
"utility/pimpl.hpp", "utility/robot/armor.hpp") come first, then C++ standard
library headers (e.g. <expected>, <optional>, <span>, <string>), and finally
third‑party/externals (e.g. <eigen3/Eigen/Core>, <yaml-cpp/yaml.h>), with a
blank line between each group; apply the same grouped ordering consistently to
any other files that use these headers to avoid style drift.
🪄 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: 295d6e36-cdf2-4618-aca9-c902c2065afe
📒 Files selected for processing (28)
CMakeLists.txtREADME.mdconfig/config.yamldoc/streaming.mdlaunch/launch.pysrc/adapter/adapter.hppsrc/adapter/deformable-infantry.hppsrc/adapter/sentry.hppsrc/component.cppsrc/kernel/auto_aim.cppsrc/kernel/auto_aim.hppsrc/kernel/fire_control.cppsrc/kernel/fire_control.hppsrc/module/fire_control/aim_point_chooser.hppsrc/module/fire_control/shoot_evaluator.hppsrc/module/fire_control/solver/aim_point_sampling.cppsrc/module/fire_control/solver/aim_point_sampling.hppsrc/module/fire_control/solver/target_solution_solver.cppsrc/module/fire_control/solver/target_solution_solver.hppsrc/module/fire_control/trajectory_solution.hppsrc/module/predictor/outpost/snapshot.cppsrc/module/predictor/outpost/snapshot.hppsrc/module/predictor/regular/snapshot.cppsrc/module/predictor/regular/snapshot.hppsrc/module/predictor/snapshot.cppsrc/module/predictor/snapshot.hppsrc/runtime.cppsrc/utility/math/solve_armors.cpp
💤 Files with no reviewable changes (6)
- src/adapter/sentry.hpp
- launch/launch.py
- src/runtime.cpp
- src/adapter/deformable-infantry.hpp
- src/adapter/adapter.hpp
- CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CMakeLists.txt`:
- Line 35: 在 CMakeLists.txt 中为目标 ${PROJECT_NAME}_component 明确添加 pluginlib
的传递依赖：因为 src/component.cpp 引入了 <pluginlib/class_list_macros.hpp> 并使用
PLUGINLIB_EXPORT_CLASS(...)，所以在定义 ${PROJECT_NAME}_component 的地方（当前只设置了
${rmcs_description_INCLUDE_DIRS}/${rmcs_executor_INCLUDE_DIRS}/${rmcs_executor_LIBRARIES}）需要把
pluginlib 的 include 和链接依赖补上；修改方法为对该 target 使用 ament_target_dependencies(...
pluginlib) 或在 target_include_directories/target_link_libraries 中加入
${pluginlib_INCLUDE_DIRS}/${pluginlib_LIBRARIES} 以确保编译和链接时可见 pluginlib 符号。
🪄 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: 1b0b8a36-69a5-4fa2-88e4-6e08f16bd24e
📒 Files selected for processing (5)
CMakeLists.txtsrc/component.cppsrc/kernel/auto_aim.cppsrc/utility/image/drawable.cppsrc/utility/image/drawable.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/component.cpp
- src/kernel/auto_aim.cpp
2 participants
单进程架构重构
概述
本 PR 将项目架构从原先的“Component 与 Runtime 两进程”模式重构为“单进程多线程”模式：引入独立的 AutoAim 后台工作线程（AutoAim）并由 AutoAimComponent 主线程通过显式输入/输出接口与之交互，移除独立 runtime 进程/可执行体与对应的 launch 脚本。
核心变更
运行时与启动逻辑
新增与线程化 AutoAim
组件侧重构
适配器移除
数据类型与接口调整
依赖与构建变更
文档与配置
图像与可视化 API 调整
影响与注意事项
行数与审查工作量