Skip to content

Separate tools and tests - #9

Closed
creeper5820 wants to merge 1 commit into
mainfrom
refactor/separate-test-and-tool
Closed

Separate tools and tests#9
creeper5820 wants to merge 1 commit into
mainfrom
refactor/separate-test-and-tool

Conversation

@creeper5820

@creeper5820 creeper5820 commented Dec 15, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

Release Notes

  • Chores
    • Refactored build system configuration for development tools and utilities
    • Reorganized hardware integration support with improved conditional availability based on detected system dependencies
    • Visualization tool is always available
    • Streaming and camera tools are available only when required dependencies are detected
    • Build system provides status messages when tools cannot be built

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Dec 15, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The pull request relocates visualization and HikCamera-related build targets from the test directory to a newly established tool CMakeLists.txt. Build configuration for three executables (visualization, hikcamera, streaming) is consolidated in the tool directory, with conditional logic based on HikCamera package availability.

Changes

Cohort / File(s) Summary
Test build target removal
test/CMakeLists.txt
Removed HikCamera integration logic, package detection, and three executable targets (example_visualization, example_hikcamera, example_streaming) with their associated linking configurations.
Tool build configuration
tool/CMakeLists.txt
New CMake configuration for rmcs_auto_aim_v2_tools project. Declares dependencies (ament_cmake, rclcpp, OpenVINO, OpenCV, hikcamera), establishes HIKCAMERA_AVAILABLE flag, and defines three executables: visualization (unconditional), hikcamera and streaming (conditional on hikcamera availability). Each executable is linked against appropriate libraries and source modules.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Straightforward build configuration reorganization with no functional logic changes
  • Removal and addition patterns are consistent and repetitive
  • Conditional HikCamera logic is straightforward; verify variable definitions and target dependencies match between files
  • Ensure all source file paths referenced in tool/CMakeLists.txt are valid and TOOL_DIR/RMCS_SRC_DIR variables are properly defined

Poem

🐰 Hark! The build targets hop and play,
From test to tool, they've moved away!
HikCamera's journey, now complete,
Tools and visualization, oh how sweet! 📹✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Separate tools and tests' directly reflects the main purpose of the pull request: moving tool-related CMake configuration from test/CMakeLists.txt to tool/CMakeLists.txt, effectively separating build configuration for tools from tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/separate-test-and-tool

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
tool/CMakeLists.txt (2)

29-36: Conditionally set hikcamera include directories.

Line 31 includes ${hikcamera_INCLUDE_DIRS} unconditionally, but the hikcamera package is searched with QUIET (line 20) and may not be available. While CMake will safely expand undefined variables to empty strings, this pattern is fragile and doesn't clearly communicate the intent that this include path is optional.

Consider moving the hikcamera include directory inside the conditional block (lines 57–87) where HIKCAMERA_AVAILABLE is checked:

 include_directories(
     ${RMCS_SRC_DIR}
-    ${hikcamera_INCLUDE_DIRS}
     ${rclcpp_INCLUDE_DIRS}
     ${visualization_msgs_INCLUDE_DIRS}
     ${geometry_msgs_INCLUDE_DIRS}
     ${OpenCV_INCLUDE_DIRS}
 )
 
 set(TOOL_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 
 # Visualization
 add_executable(
     visualization
     ${TOOL_DIR}/visualization.cpp
     ${RMCS_SRC_DIR}/utility/rclcpp/visual/armor.cpp
     ${RMCS_SRC_DIR}/utility/rclcpp/visual/posture.cpp
     ${RMCS_SRC_DIR}/utility/panic.cpp
     ${RMCS_SRC_DIR}/utility/math/solve_armors.cpp
     ${RMCS_SRC_DIR}/utility/rclcpp/node.cpp
 )
 target_link_libraries(
     visualization
     rclcpp::rclcpp
     ${visualization_msgs_LIBRARIES}
     ${geometry_msgs_LIBRARIES}
 )
 
 if(HIKCAMERA_AVAILABLE)
+    include_directories(${hikcamera_INCLUDE_DIRS})
     # Hikcamera
     add_executable(

85-87: Use English in status messages for consistency.

The fallback status message at line 86 uses Chinese ("hikcamera 未检测到,跳过相关构建"), but the rest of the codebase uses English for comments, variable names, and messages. Consider translating this to English for consistency.

 else()
-    message(STATUS "hikcamera 未检测到,跳过相关构建")
+    message(STATUS "hikcamera not detected; skipping related builds")
 endif()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b6fda31 and b1ddddd.

📒 Files selected for processing (2)
  • test/CMakeLists.txt (0 hunks)
  • tool/CMakeLists.txt (1 hunks)
💤 Files with no reviewable changes (1)
  • test/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (1)
tool/CMakeLists.txt (1)

41-55: The visualization target has all required dependencies. Verification of the actual source and header files included in this target shows no usage of OpenCV or yaml-cpp. The implementation uses Eigen3 for mathematics and ROS2 messaging libraries (visualization_msgs and geometry_msgs) for rendering, all of which are already properly linked.

Likely an incorrect or invalid review comment.

@github-project-automation github-project-automation Bot moved this from Todo to Done in RMCS Auto Aim V2 Dec 15, 2025
@creeper5820

Copy link
Copy Markdown
Collaborator Author

修正 Coderabbit 的设定

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant