refactor: pose estimator#43
Merged
creeper5820 merged 4 commits intomainfrom May 1, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (13)
Walkthrough该PR进行了广泛的系统重构，重命名了核心类型（ Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime as Runtime Loop
participant Feishu as Feishu<br/>(Comm)
participant Adapter as Adapter<br/>(TF)
participant Detector as Detector
participant PoseEst as PoseEstimator
participant YawOpt as YawOptimizer
participant FireCtrl as FireControl
participant Tracker as Tracker
participant Viz as Visualization
Note over Runtime: Main Loop Iteration
Runtime->>Feishu: heartbeat()
Feishu-->>Runtime: ok
Runtime->>Feishu: search(image_ts, 50ms)
Feishu-->>Runtime: SystemContext
Runtime->>Adapter: ready()
Adapter-->>Runtime: bool
Runtime->>Adapter: camera_transform()
Adapter-->>Runtime: Transform
Runtime->>Detector: detect(image)
Detector-->>Runtime: Armor2D[]
Runtime->>PoseEst: estimate_armor(Armor2D[])
PoseEst->>YawOpt: solve(camera_feature, armor_shape, corners)
YawOpt-->>PoseEst: Orientation (optimized yaw)
PoseEst-->>Runtime: Armor3D[]
Runtime->>PoseEst: into_odom_link(Armor3D[])
PoseEst-->>Runtime: Armor3D[] (transformed)
Runtime->>Tracker: update(Armor3D[], context)
Tracker-->>Runtime: Snapshot
Runtime->>FireCtrl: solve(Snapshot, context)
FireCtrl-->>Runtime: (control_dir, should_shoot)
Runtime->>Viz: update_camera_pose(context.camera_transform)
Runtime->>Viz: update_visible_armors(Armor3D[])
Runtime->>Viz: update_aiming_direction(yaw, pitch)
Runtime->>Viz: update_image(image)
Note over Runtime: Publish control output
sequenceDiagram
participant Tool as hikcamera Tool
participant Capturer as Capturer
participant ImageRec as ImageRecorder
participant Queue as Frame Queue
participant Worker as Record Worker
Note over Tool: Recording Flow (Async)
Tool->>ImageRec: set_saving_location(path)
Tool->>ImageRec: set_framerate(30)
Tool->>Worker: request_start_recording()
activate Worker
loop Per Frame
Capturer->>Capturer: fetch_image()
Note over Capturer: Drain queue,<br/>return latest
Tool->>Queue: push(cloned_frame)
Worker->>Queue: dequeue_burst()
Worker->>ImageRec: write_frame(frame)
ImageRec->>ImageRec: encode & save
end
Tool->>Worker: request_stop_recording()
deactivate Worker
Worker->>ImageRec: stop()
ImageRec-->>Worker: last_saved_path
Tool->>Tool: display saved path & stats
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 19 seconds.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: 10
🧹 Nitpick comments (3)
AGENTS.md (1)
17-25: ⚡ Quick win建议补充“指令优先级/冲突处理”小节，减少执行歧义。
当前文档同时定义了 SOP、代码风格和协作规范，但未明确冲突时的优先级，后续多 Agent/多人协作下容易出现执行不一致。建议加一个简短规则段落。
📌 建议补充示例
## Agent 协作开发规范（Auto Aim） 本文件用于约束 Agent 在本仓库中的行为，目标是： @@ - 让 Agent 在可控范围内稳定提供辅助价值 + +### 指令优先级与冲突处理 + +- 若出现冲突，按“系统/平台指令 > 开发者当前明确指令 > 本文档 > 其他参考资料”执行 +- 发生冲突时在回复中显式说明采用了哪条高优先级指令🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 17 - 25, 在 AGENTS.md 中在 “核心原则：算法开发由人类主导” 下新增一个名为 “指令优先级/冲突处理” 的短段落，明确列出冲突决策顺序（例如：人类指令 > 项目SOP/安全策略 > 代码风格/约定 > Agent 自动建议），并说明遇到指令或规范冲突时应如何记录（谁做出决定、理由、变更记录位置）和如何通知协作成员（PR/Issue/日志）；引用现有标题文本（“核心原则：算法开发由人类主导”）来定位插入点并保证语言简洁可执行。src/utility/math/solve_pnp/yaw_optimizer.hpp (1)
3-8: ⚡ Quick win建议统一头文件分组顺序，避免风格漂移。
这里建议按“本地 → 标准库 → 第三方”分组，和仓库其余头文件保持一致。
♻️ 建议修改
-#include <array> - `#include` "utility/math/camera.hpp" `#include` "utility/math/linear.hpp" `#include` "utility/robot/id.hpp" + +#include <array>As per coding guidelines:
**/*.{h,hpp,hxx}: Header files should follow the order: local headers, standard library headers, third-party library headers, with blank lines separating each section (can merge into one block when headers are few).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/math/solve_pnp/yaw_optimizer.hpp` around lines 3 - 8, The include order in yaw_optimizer.hpp is inconsistent with project style; reorder the includes so local headers come first (utility/math/camera.hpp, utility/math/linear.hpp, utility/robot/id.hpp), then the standard library header (array), and ensure a blank line separates the local block from the standard-library block; update the include block in yaw_optimizer.hpp accordingly to match the "local → standard → third-party" grouping.src/utility/math/camera.hpp (1)
3-7: ⚡ Quick win请按仓库规范调整头文件分组顺序。
当前顺序是“标准库 → 第三方 → 本地”，与仓库约定不一致。建议改为“本地 → 标准库 → 第三方”，并保留分组空行。
♻️ 建议修改
-#include <array> -#include <opencv2/core/mat.hpp> - `#include` "utility/math/linear.hpp" + +#include <array> +#include <opencv2/core/mat.hpp>As per coding guidelines:
**/*.{h,hpp,hxx}: Header files should follow the order: local headers, standard library headers, third-party library headers, with blank lines separating each section (can merge into one block when headers are few).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/math/camera.hpp` around lines 3 - 7, In src/utility/math/camera.hpp reorder the `#include` blocks to match repository convention: place local headers first (e.g. `#include` "utility/math/linear.hpp"), then standard library headers (e.g. `#include` <array>), then third‑party headers (e.g. `#include` <opencv2/core/mat.hpp>), keeping blank lines between each group; update the three include lines shown so they appear in the order: "utility/math/linear.hpp", then "<array>", then "<opencv2/core/mat.hpp>".
🤖 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 54-60: The timeout check using Clock::now() and kAutoAimTimeout
currently sets *should_control and *should_shoot to false but is immediately
overwritten by unguarded assignments from command.should_control and
command.should_shoot; fix by preventing the later assignments when the timeout
condition is true — either return early after setting
*should_control/*should_shoot to false or make the assignments conditional (e.g.
only assign from command.should_control/command.should_shoot when (Clock::now()
- command.timestamp) <= kAutoAimTimeout) so the timeout protection is not
overwritten.
- Around line 51-52: When feishu.heartbeat() returns false you must explicitly
disable or reset any active control outputs instead of simply returning; update
the branch around the feishu.heartbeat() call so that on failure you (1) disable
the control path (e.g., clear/send-off to actuators or call the component's
control-disable method), (2) reset any persistent output state/flags (e.g.,
control_enabled = false or zero outputs), and (3) then return. Locate the
heartbeat check (feishu.heartbeat()) in component.cpp and invoke the
module/utility that turns off outputs or zeroes commands before returning to
avoid leaving previous outputs active.
In `@src/kernel/capturer.cpp`:
- Around line 90-97: fetch_image() currently drains the entire capture_queue and
returns only the newest frame, which breaks the semantics of
local_video.allow_skipping; change fetch_image() (and/or add a Capturer member
like allow_skipping_) so that when allow_skipping is false it returns the next
frame in FIFO order (pop a single frame and return it) preserving sequence, and
only when allow_skipping is true perform the current drain-to-latest behavior;
update any call sites relying on fetch_image() (e.g., the main loop in runtime
that expects no-skipping behavior) to read the new flag from the Capturer
instance.
In `@src/kernel/pose_estimator.hpp`:
- Around line 15-21: The header pose_estimator.hpp exposes Transform, std::span
and std::vector via the APIs (update_camera_transform, estimate_armor,
into_odom_link) but does not explicitly include the corresponding headers; add
explicit includes for <span> and <vector> and also include the local math header
"utility/math/linear.hpp" to make Transform/type dependencies clear; finally
reorder the include block so local project headers (e.g.,
"utility/math/linear.hpp", "utility/robot/armor.hpp") appear before
standard/third-party headers to follow the project include-order guideline.
In `@src/utility/image/recorder.cpp`:
- Line 205: The ImageRecorder currently allows invalid configuration (e.g.,
framerate == 0, negative durations, min_duration > max_duration) which leads to
silent non-recording; update ImageRecorder to perform fail-fast validation when
configuration is applied (e.g., in the configuration setter or initialization
path that mutates pimpl), validate fields like framerate, duration, min_duration
and max_duration and if any are invalid: return an error / throw an exception
(or log and set an explicit error state) rather than silently accepting them;
ensure the validation lives next to the existing setter
(ImageRecorder::set_saving_location and the pimpl-backed config setters) and
references pimpl->framerate, pimpl->min_duration, pimpl->max_duration (or their
actual names) so invalid values are rejected immediately with a clear error
message.
In `@src/utility/math/linear.hpp`:
- Line 68: The three uses of static_assert(false, ...) in the template functions
(e.g., inside clone_scalar2d and the other two branches) are non-dependent and
fire at template definition time; add a dependent-false helper template (e.g.,
template<class...> inline constexpr bool dependent_false_v = false;) and replace
each static_assert(false, "...") with static_assert(dependent_false_v<Src, Dst>,
"...") (or use the actual template parameter pack for that function) so the
assertion becomes dependent on template parameters and is evaluated only at
instantiation.
In `@src/utility/math/solve_pnp/yaw_optimizer.cpp`:
- Around line 76-77: The yaw search loop is coupling the iteration count to a
hardcoded assumption that kSearchStepDeg == 1 and misses the +70° boundary;
change the loop to compute the number of samples from the range and step (e.g.
samples = round(kSearchRangeDeg / kSearchStepDeg) + 1) and iterate i from 0 to
samples-1 so candidate_yaw = yaw_start + i * deg2rad(kSearchStepDeg) covers both
endpoints; update the loop that produces candidate_yaw (references: yaw_start,
kSearchRangeDeg, kSearchStepDeg, deg2rad, candidate_yaw) to use this computed
sample count instead of static_cast<int>(kSearchRangeDeg).
In `@src/utility/rclcpp/visual/arrow.hpp`:
- Around line 1-5: arrow.hpp currently relies on indirect includes but uses
std::string and std::unique_ptr; explicitly include the missing standard headers
by adding <string> and <memory> at the top of the header so usages of
std::string and std::unique_ptr are guaranteed (update the include list near the
existing includes "utility/rclcpp/node.hpp" and
"utility/rclcpp/visual/movable.hpp").
In `@tool/hikcamera.cpp`:
- Around line 149-153: shutdown() writes shared flag enabled without locking
while the worker reads/writes enabled/stop_pending under a mutex, causing a data
race; update shutdown() (and the similar code at the other occurrence) to
acquire the same mutex used by the worker before modifying enabled/stop_pending,
set the flag(s) while holding the lock, then release and call cv.notify_one()
(or notify after unlocking) and join the worker; reference the member names
enabled, stop_pending, the shutdown() method, the worker thread lambda/function,
and the cv/mutex pair to locate and fix the accesses.
- Around line 68-130: The worker recreation can deadlock because assigning a new
std::jthread to member worker will join the old thread while that thread may be
blocked and not exiting; before creating or assigning a new worker in start(),
request the old thread to stop and wake it: set stop_requested = true (or
enabled = false), call cv.notify_all(), and call worker.request_stop() if used;
then wait for the old worker to finish without holding mutex (e.g., release
locks, then let the old jthread be destroyed/assigned) before constructing the
new std::jthread; update start() to ensure you don't hold mutex across the
worker reassignment and use worker/request_stop()/cv.notify_all() to guarantee
the old loop sees the stop signal and exits.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 17-25: 在 AGENTS.md 中在 “核心原则：算法开发由人类主导” 下新增一个名为 “指令优先级/冲突处理”
的短段落，明确列出冲突决策顺序（例如：人类指令 > 项目SOP/安全策略 > 代码风格/约定 > Agent
自动建议），并说明遇到指令或规范冲突时应如何记录（谁做出决定、理由、变更记录位置）和如何通知协作成员（PR/Issue/日志）；引用现有标题文本（“核心原则：算法开发由人类主导”）来定位插入点并保证语言简洁可执行。
In `@src/utility/math/camera.hpp`:
- Around line 3-7: In src/utility/math/camera.hpp reorder the `#include` blocks to
match repository convention: place local headers first (e.g. `#include`
"utility/math/linear.hpp"), then standard library headers (e.g. `#include`
<array>), then third‑party headers (e.g. `#include` <opencv2/core/mat.hpp>),
keeping blank lines between each group; update the three include lines shown so
they appear in the order: "utility/math/linear.hpp", then "<array>", then
"<opencv2/core/mat.hpp>".
In `@src/utility/math/solve_pnp/yaw_optimizer.hpp`:
- Around line 3-8: The include order in yaw_optimizer.hpp is inconsistent with
project style; reorder the includes so local headers come first
(utility/math/camera.hpp, utility/math/linear.hpp, utility/robot/id.hpp), then
the standard library header (array), and ensure a blank line separates the local
block from the standard-library block; update the include block in
yaw_optimizer.hpp accordingly to match the "local → standard → third-party"
grouping.
🪄 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: 9b8b7c45-93c6-4cfd-b610-cad7afb9ed6d
📒 Files selected for processing (56)
AGENTS.mdconfig/config.yamlsrc/adapter/adapter.hppsrc/adapter/sentry.hppsrc/component.cppsrc/kernel/capturer.cppsrc/kernel/fire_control.cppsrc/kernel/fire_control.hppsrc/kernel/pose_estimator.cppsrc/kernel/pose_estimator.hppsrc/kernel/visualization.cppsrc/kernel/visualization.hppsrc/module/debug/recorder.hppsrc/module/debug/visualization/armor_visualizer.cppsrc/module/debug/visualization/armor_visualizer.hppsrc/module/debug/visualization/stream_session.cppsrc/module/fire_control/aim_point_chooser.cppsrc/module/fire_control/aim_point_chooser.hppsrc/module/predictor/outpost/robot_state.cppsrc/module/predictor/regular/ekf_parameter.hppsrc/module/predictor/regular/robot_state.cppsrc/module/predictor/regular/robot_state.hppsrc/module/predictor/robot_state.cppsrc/module/predictor/robot_state.hppsrc/module/predictor/snapshot.hppsrc/module/tracker/decider.cppsrc/module/tracker/decider.hppsrc/runtime.cppsrc/utility/image/image.cppsrc/utility/image/image.hppsrc/utility/image/recorder.cppsrc/utility/image/recorder.hppsrc/utility/math/camera.cppsrc/utility/math/camera.hppsrc/utility/math/conversion.hppsrc/utility/math/kalman_filter/ekf.hppsrc/utility/math/linear.hppsrc/utility/math/mahalanobis.hppsrc/utility/math/point.hppsrc/utility/math/solve_pnp/pnp_solution.cppsrc/utility/math/solve_pnp/pnp_solution.hppsrc/utility/math/solve_pnp/yaw_optimizer.cppsrc/utility/math/solve_pnp/yaw_optimizer.hppsrc/utility/rclcpp/visual/armor.cppsrc/utility/rclcpp/visual/arrow.cppsrc/utility/rclcpp/visual/arrow.hppsrc/utility/rclcpp/visual/movable.hppsrc/utility/robot/armor.hppsrc/utility/shared/context.hpptest/CMakeLists.txttest/feishu_test.cpptest/image_recorder.cpptest/solve_pnp.cpptest/transform_communication.cpptool/CMakeLists.txttool/hikcamera.cpp
💤 Files with no reviewable changes (5)
- src/kernel/fire_control.hpp
- src/module/predictor/robot_state.cpp
- src/module/debug/visualization/armor_visualizer.hpp
- src/module/debug/recorder.hpp
- src/utility/math/point.hpp
Comment on lines
+54
to
+60
| if (Clock::now() - command.timestamp > kAutoAimTimeout) { | ||
| *should_control = false; | ||
| *should_shoot = false; | ||
| } | ||
|
|
||
| const auto& [yaw, pitch] = std::tie(state.yaw, state.pitch); | ||
| *should_control = command.should_control; | ||
| *should_shoot = command.should_shoot; |
Contributor
There was a problem hiding this comment.
超时保护被后续赋值覆盖，导致失效。
Line 54-57 置 false 后，Line 59-60 又无条件回写 command.should_*，超时命令仍可生效。
建议修复（示例）
auto command = *feishu.latest();
if (Clock::now() - command.timestamp > kAutoAimTimeout) {
*should_control = false;
*should_shoot = false;
+ return;
}
*should_control = command.should_control;
*should_shoot = command.should_shoot;📝 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
| if (Clock::now() - command.timestamp > kAutoAimTimeout) { | |
| *should_control = false; | |
| *should_shoot = false; | |
| } | |
| const auto& [yaw, pitch] = std::tie(state.yaw, state.pitch); | |
| *should_control = command.should_control; | |
| *should_shoot = command.should_shoot; | |
| if (Clock::now() - command.timestamp > kAutoAimTimeout) { | |
| *should_control = false; | |
| *should_shoot = false; | |
| return; | |
| } | |
| *should_control = command.should_control; | |
| *should_shoot = command.should_shoot; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component.cpp` around lines 54 - 60, The timeout check using Clock::now()
and kAutoAimTimeout currently sets *should_control and *should_shoot to false
but is immediately overwritten by unguarded assignments from
command.should_control and command.should_shoot; fix by preventing the later
assignments when the timeout condition is true — either return early after
setting *should_control/*should_shoot to false or make the assignments
conditional (e.g. only assign from command.should_control/command.should_shoot
when (Clock::now() - command.timestamp) <= kAutoAimTimeout) so the timeout
protection is not overwritten.
Comment on lines
+90
to
+97
| // 为了实时性，一般取最新的帧 | ||
| auto fetch_image() noexcept -> ImageUnique { | ||
| auto raw = RawImage { nullptr }; | ||
| capture_queue.pop(raw); | ||
| return std::unique_ptr<Image> { raw }; | ||
| auto result = ImageUnique { nullptr }; | ||
| auto image = RawImage { }; | ||
| while (capture_queue.pop(image)) { | ||
| result = ImageUnique { image }; | ||
| } | ||
| return result; |
Contributor
There was a problem hiding this comment.
不要在 fetch_image() 里无条件丢弃中间帧。
这里把队列一次性 drain 到最新帧后，local_video.allow_skipping: false 的语义就失效了。只要 src/runtime.cpp:110-114 的主循环一轮处理慢于采集线程，就会直接跳过中间视频帧，无法再保证按顺序完整播放。建议把“只取最新帧”的策略限制在允许跳帧的来源/模式下，或者把丢帧策略显式做成 Capturer 级配置。
Based on learnings: "when allow_skipping is false, the intended behavior is to play all frames strictly in sequence without caring about actual elapsed time; when allow_skipping is true, the intended behavior is to maintain the expected playback rate and catch up when falling behind."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/kernel/capturer.cpp` around lines 90 - 97, fetch_image() currently drains
the entire capture_queue and returns only the newest frame, which breaks the
semantics of local_video.allow_skipping; change fetch_image() (and/or add a
Capturer member like allow_skipping_) so that when allow_skipping is false it
returns the next frame in FIFO order (pop a single frame and return it)
preserving sequence, and only when allow_skipping is true perform the current
drain-to-latest behavior; update any call sites relying on fetch_image() (e.g.,
the main loop in runtime that expects no-skipping behavior) to read the new flag
from the Capturer instance.
Comment on lines
15
to
+21
| auto initialize(const YAML::Node&) noexcept -> std::expected<void, std::string>; | ||
| auto update_camera_transform(Transform const& transform) -> void; | ||
|
|
||
| auto visualize(RclcppNode& visual_node) -> void; | ||
|
|
||
| auto solve_pnp(std::vector<Armor2D> const&) const -> std::optional<std::vector<Armor3D>>; | ||
|
|
||
| auto set_odom_to_camera_transform(Transform const& transform) -> void; | ||
| auto estimate_armor(std::vector<Armor2D> const&) const -> std::vector<Armor3D>; | ||
|
|
||
| auto odom_to_camera(std::span<Armor3D const> armors) const -> std::vector<Armor3D>; | ||
| auto odom_to_camera(Armor3D const& armor) const -> Armor3D; | ||
|
|
||
| auto update_imu_link(const Orientation&) noexcept -> void; | ||
| auto into_odom_link(std::span<Armor3D const> armors) const -> std::vector<Armor3D>; | ||
| auto into_odom_link(Armor3D const& armor) const -> Armor3D; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/kernel/pose_estimator.hpp | head -40Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 871
🏁 Script executed:
# Check the current includes in the file
rg "#include" src/kernel/pose_estimator.hpp -A 1 -B 1Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 193
🏁 Script executed:
# Search for Transform definition and its header
fd -t f -e hpp -e h | xargs rg "class Transform|struct Transform" | head -20Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 195
🏁 Script executed:
# Check if utility/math/linear.hpp is already included indirectly through other headers
rg "#include.*linear" src/kernel/pose_estimator.hppRepository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 61
🏁 Script executed:
# Check what's in utility/robot/armor.hpp - see if it includes the Transform or vector/span
cat -n src/utility/robot/armor.hpp | head -50Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 1843
🏁 Script executed:
# Check what's in utility/pimpl.hpp
cat -n src/utility/pimpl.hpp | head -50Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 1366
🏁 Script executed:
# Check the implementation file to see which Transform is imported
fd -t f -e cpp -e cxx | xargs rg "pose_estimator" -lRepository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 125
🏁 Script executed:
# Search for Transform usage in pose_estimator implementation or related files
rg "Transform" src/kernel/ -B 2 -A 2Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 1844
🏁 Script executed:
# Verify the include order guideline by checking if other header files in the codebase follow the local-standard-third-party order
fd -t f -e hpp | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 2304
🏁 Script executed:
# Check what Armor2D and Armor3D are to ensure we need vector
rg "struct Armor3D|class Armor3D" -A 3Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 264
🏁 Script executed:
# Check the correct include order more carefully by looking at capturer.hpp which seems to follow it
cat -n src/kernel/capturer.hpp | head -15Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 453
补回 PoseEstimator 新 API 的显式依赖，并修正头文件包含顺序。
update_camera_transform() / estimate_armor() / into_odom_link() 直接暴露了 Transform、std::span 和 std::vector，但这个头文件缺少 <span> 和 <vector> 的显式包含。虽然 Transform 可通过 "utility/robot/armor.hpp" 传递获得，但建议显式包含 "utility/math/linear.hpp" 以提高清晰度。
此外，当前包含顺序不符合编码规范。根据指南，本地头文件应先于标准库和第三方头文件。
🔧 建议修正
`#pragma` once
+#include "utility/math/linear.hpp"
+#include "utility/pimpl.hpp"
+#include "utility/robot/armor.hpp"
+
`#include` <expected>
+#include <span>
+#include <vector>
`#include` <yaml-cpp/yaml.h>
-
-#include "utility/pimpl.hpp"
-#include "utility/robot/armor.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/kernel/pose_estimator.hpp` around lines 15 - 21, The header
pose_estimator.hpp exposes Transform, std::span and std::vector via the APIs
(update_camera_transform, estimate_armor, into_odom_link) but does not
explicitly include the corresponding headers; add explicit includes for <span>
and <vector> and also include the local math header "utility/math/linear.hpp" to
make Transform/type dependencies clear; finally reorder the include block so
local project headers (e.g., "utility/math/linear.hpp",
"utility/robot/armor.hpp") appear before standard/third-party headers to follow
the project include-order guideline.
| } | ||
| }; | ||
|
|
||
| auto ImageRecorder::set_saving_location(const std::string& path) -> void { pimpl->location = path; } |
Contributor
There was a problem hiding this comment.
在配置入口增加 fail-fast 参数校验，避免“静默不录制”。
Line [207]、Line [213]、Line [217] 当前允许无效配置（如 framerate=0、负时长、min_duration > max_duration），后续会表现为录制失败或文件总被丢弃，但没有明确错误反馈。
🔧 建议修复
`#include` "recorder.hpp"
+ `#include` "utility/panic.hpp"
@@
auto ImageRecorder::set_saving_location(const std::string& path) -> void { pimpl->location = path; }
-auto ImageRecorder::set_framerate(std::size_t rate) -> void { pimpl->framerate = rate; }
+auto ImageRecorder::set_framerate(std::size_t rate) -> void {
+ if (rate == 0) {
+ util::panic("ImageRecorder: framerate must be greater than 0");
+ }
+ pimpl->framerate = rate;
+}
@@
auto ImageRecorder::set_max_recording_duration(std::chrono::seconds duration) -> void {
+ if (duration.count() < 0 || duration < pimpl->min_duration) {
+ util::panic("ImageRecorder: max_recording_duration is invalid");
+ }
pimpl->max_duration = duration;
}
auto ImageRecorder::set_min_recording_duration(std::chrono::seconds duration) -> void {
+ if (duration.count() < 0 || duration > pimpl->max_duration) {
+ util::panic("ImageRecorder: min_recording_duration is invalid");
+ }
pimpl->min_duration = duration;
}Based on learnings: "Default strategy for error handling: fail fast to avoid silent fallback ... report errors explicitly for invalid configuration/parameters without defaults."
Also applies to: 207-207, 213-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utility/image/recorder.cpp` at line 205, The ImageRecorder currently
allows invalid configuration (e.g., framerate == 0, negative durations,
min_duration > max_duration) which leads to silent non-recording; update
ImageRecorder to perform fail-fast validation when configuration is applied
(e.g., in the configuration setter or initialization path that mutates pimpl),
validate fields like framerate, duration, min_duration and max_duration and if
any are invalid: return an error / throw an exception (or log and set an
explicit error state) rather than silently accepting them; ensure the validation
lives next to the existing setter (ImageRecorder::set_saving_location and the
pimpl-backed config setters) and references pimpl->framerate,
pimpl->min_duration, pimpl->max_duration (or their actual names) so invalid
values are rejected immediately with a clear error message.
| dst.x() = src.x; | ||
| dst.y() = src.y; | ||
| } else { | ||
| static_assert(false, "clone_scalar2d: unsupported trait combination"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 验证源码中是否存在会导致该问题的写法
rg -n 'static_assert\(false' src/utility/math/linear.hppRepository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 338
🏁 Script executed:
# Read the relevant file to understand the code context and template structure
cat -n src/utility/math/linear.hpp | head -130Repository: Alliance-Algorithm/rmcs_auto_aim_v2
Length of output: 5625
模板中的 static_assert(false) 会导致编译直接失败，需要修复为依赖表达式。
第 68、91、118 行的 static_assert(false, ...) 在模板定义阶段直接被评估，因为它是非依赖表达式。即使 if constexpr 分支在实例化时不会被执行，非依赖的 static_assert 仍会在模板定义时触发，导致编译无法进行。
需要引入依赖于模板参数的 false 值，使断言延迟到模板实例化时评估：
template <class...>
inline constexpr bool dependent_false_v = false;然后将三处 static_assert(false, ...) 改为 static_assert(dependent_false_v<Src, Dst>, ...)。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utility/math/linear.hpp` at line 68, The three uses of
static_assert(false, ...) in the template functions (e.g., inside clone_scalar2d
and the other two branches) are non-dependent and fire at template definition
time; add a dependent-false helper template (e.g., template<class...> inline
constexpr bool dependent_false_v = false;) and replace each static_assert(false,
"...") with static_assert(dependent_false_v<Src, Dst>, "...") (or use the actual
template parameter pack for that function) so the assertion becomes dependent on
template parameters and is evaluated only at instantiation.
Comment on lines
+76
to
+77
| for (auto i = int { }; i < static_cast<int>(kSearchRangeDeg); ++i) { | ||
| auto candidate_yaw = double { yaw_start + i * deg2rad(kSearchStepDeg) }; |
Contributor
There was a problem hiding this comment.
Yaw 搜索循环有边界与步长耦合问题。
当前实现不会覆盖 +70° 边界，且循环次数写死依赖“步长=1°”；建议用 range/step 计算采样数并包含上下边界。
建议修复（示例）
- for (auto i = int { }; i < static_cast<int>(kSearchRangeDeg); ++i) {
+ const auto sample_count = static_cast<int>(kSearchRangeDeg / kSearchStepDeg);
+ for (auto i = int { }; i <= sample_count; ++i) {
auto candidate_yaw = double { yaw_start + i * deg2rad(kSearchStepDeg) };📝 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
| for (auto i = int { }; i < static_cast<int>(kSearchRangeDeg); ++i) { | |
| auto candidate_yaw = double { yaw_start + i * deg2rad(kSearchStepDeg) }; | |
| const auto sample_count = static_cast<int>(kSearchRangeDeg / kSearchStepDeg); | |
| for (auto i = int { }; i <= sample_count; ++i) { | |
| auto candidate_yaw = double { yaw_start + i * deg2rad(kSearchStepDeg) }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utility/math/solve_pnp/yaw_optimizer.cpp` around lines 76 - 77, The yaw
search loop is coupling the iteration count to a hardcoded assumption that
kSearchStepDeg == 1 and misses the +70° boundary; change the loop to compute the
number of samples from the range and step (e.g. samples = round(kSearchRangeDeg
/ kSearchStepDeg) + 1) and iterate i from 0 to samples-1 so candidate_yaw =
yaw_start + i * deg2rad(kSearchStepDeg) covers both endpoints; update the loop
that produces candidate_yaw (references: yaw_start, kSearchRangeDeg,
kSearchStepDeg, deg2rad, candidate_yaw) to use this computed sample count
instead of static_cast<int>(kSearchRangeDeg).
Comment on lines
+1
to
+5
| #pragma once | ||
|
|
||
| #include "utility/rclcpp/node.hpp" | ||
| #include "utility/rclcpp/visual/movable.hpp" | ||
|
|
Contributor
There was a problem hiding this comment.
头文件缺少直接依赖的标准库包含，存在编译脆弱性。
该文件直接使用了 std::string 和 std::unique_ptr，建议显式包含 <string> 与 <memory>，避免依赖间接包含。
🐛 建议修改
`#pragma` once
+#include <memory>
+#include <string>
+
`#include` "utility/rclcpp/node.hpp"
`#include` "utility/rclcpp/visual/movable.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utility/rclcpp/visual/arrow.hpp` around lines 1 - 5, arrow.hpp currently
relies on indirect includes but uses std::string and std::unique_ptr; explicitly
include the missing standard headers by adding <string> and <memory> at the top
of the header so usages of std::string and std::unique_ptr are guaranteed
(update the include list near the existing includes "utility/rclcpp/node.hpp"
and "utility/rclcpp/visual/movable.hpp").
Comment on lines
+149
to
+153
| auto shutdown() -> void { | ||
| enabled = false; | ||
| cv.notify_one(); | ||
| if (worker.joinable()) worker.join(); | ||
| } |
Contributor
There was a problem hiding this comment.
共享状态存在数据竞争（enabled/stop_pending 跨线程无同步访问）。
shutdown() 无锁写 enabled，主线程无锁读 stop_pending，而这些字段在 worker 内是受互斥锁保护读写的；当前混用会导致未定义行为。
建议修复（示例）
struct Recording {
@@
+ auto is_stop_pending() -> bool {
+ auto lock = std::lock_guard { mutex };
+ return stop_pending;
+ }
@@
auto shutdown() -> void {
- enabled = false;
+ {
+ auto lock = std::lock_guard { mutex };
+ enabled = false;
+ }
cv.notify_one();
if (worker.joinable()) worker.join();
}
};
@@
- if (recording.stop_pending) {
+ if (recording.is_stop_pending()) {
std::println("[main] Stop in progress, please wait");
continue;
}Also applies to: 254-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tool/hikcamera.cpp` around lines 149 - 153, shutdown() writes shared flag
enabled without locking while the worker reads/writes enabled/stop_pending under
a mutex, causing a data race; update shutdown() (and the similar code at the
other occurrence) to acquire the same mutex used by the worker before modifying
enabled/stop_pending, set the flag(s) while holding the lock, then release and
call cv.notify_one() (or notify after unlocking) and join the worker; reference
the member names enabled, stop_pending, the shutdown() method, the worker thread
lambda/function, and the cv/mutex pair to locate and fix the accesses.
creeper5820
force-pushed
the
refactor/pose_estimator
branch
from
5a8663b to
ffe7bfe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.