Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the FAST-Calib calibration system from ROS1 to ROS2 (Jazzy/Humble), updating the build system, core APIs, and all dependencies to their ROS2 equivalents.
- Converted build system from catkin to ament_cmake with updated package format
- Migrated core node structure to use rclcpp with shared pointers and modern parameter handling
- Updated rosbag reading to use rosbag2_cpp API with support for both Livox CustomMsg and standard PointCloud2 formats
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| package.xml | Upgraded to format 3 with ROS2 dependencies (rclcpp, tf2_ros, livox_ros_driver2, rosbag2_cpp) |
| CMakeLists.txt | Converted to ament_cmake build system with ROS2 find_package and installation patterns |
| src/main.cpp | Updated to use rclcpp node initialization, shared pointers, ROS2 publishers, and spin patterns |
| src/qr_detect.hpp | Migrated to rclcpp publishers, updated OpenCV aruco API, added ROS2-compatible cv_bridge include guards |
| src/lidar_detect.hpp | Updated to use rclcpp publishers and ROS2 message types for point cloud publishing |
| src/data_preprocess.hpp | Implemented rosbag2_cpp reader with deserialization for Livox CustomMsg and PointCloud2 formats |
| include/common_lib.h | Migrated parameter loading to ROS2 declare/get pattern, updated PCL and tf2 includes |
| include/CustomMsg.h | Removed ROS1-generated message headers (now using livox_ros_driver2 package) |
| include/CustomPoint.h | Removed ROS1-generated message headers (now using livox_ros_driver2 package) |
| launch/calib.launch.py | Added ROS2 Python launch file with node and RViz2 configuration |
| config/qr_params.yaml | Updated to ROS2 hierarchical parameter format with node namespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Determine if the message is a Livox custom message or PointCloud2 | ||
| // Note: In ROS2, message types are often checked via the metadata in the bag, | ||
| // but here we try to deserialize. |
There was a problem hiding this comment.
The comment on lines 61-62 is outdated and potentially confusing. In ROS2, the message type metadata is available through the bag API, specifically via bag_message->topic_name and the serialization format. The current try-catch approach works but the comment suggests uncertainty about the implementation. Consider clarifying that this is a deliberate fallback strategy for handling both Livox CustomMsg and standard PointCloud2 formats.
| // Determine if the message is a Livox custom message or PointCloud2 | |
| // Note: In ROS2, message types are often checked via the metadata in the bag, | |
| // but here we try to deserialize. | |
| // Determine if the message is a Livox CustomMsg or a standard PointCloud2. | |
| // In ROS2, message type metadata is available via the bag API (e.g. topic_name and | |
| // serialization format), but here we intentionally use a deserialization-based | |
| // fallback: first try Livox CustomMsg, then fall back to PointCloud2. |
| img_input_ = cv::imread(params.image_path, cv::IMREAD_UNCHANGED); | ||
| if (img_input_.empty()) | ||
| { | ||
| std::string msg = "Loading the image " + image_path + " failed"; | ||
| ROS_ERROR_STREAM(msg.c_str()); | ||
| RCLCPP_ERROR(rclcpp::get_logger("data_preprocess"), "Loading the image %s failed", image_path.c_str()); | ||
| return; | ||
| } | ||
|
|
||
| std::fstream file_; | ||
| file_.open(bag_path, ios::in); | ||
| if (!file_) | ||
| { | ||
| std::string msg = "Loading the rosbag " + bag_path + " failed"; | ||
| ROS_ERROR_STREAM(msg.c_str()); | ||
| return; | ||
| } | ||
| ROS_INFO("Loading the rosbag %s", bag_path.c_str()); | ||
| RCLCPP_INFO(rclcpp::get_logger("data_preprocess"), "Loading the rosbag %s", bag_path.c_str()); | ||
|
|
||
| rosbag::Bag bag; | ||
| rosbag2_cpp::Reader reader; | ||
| try { | ||
| bag.open(bag_path, rosbag::bagmode::Read); | ||
| } catch (rosbag::BagException &e) { | ||
| ROS_ERROR_STREAM("LOADING BAG FAILED: " << e.what()); | ||
| reader.open(bag_path); | ||
| } catch (const std::exception &e) { | ||
| RCLCPP_ERROR(rclcpp::get_logger("data_preprocess"), "LOADING BAG FAILED: %s", e.what()); | ||
| return; |
There was a problem hiding this comment.
The constructor returns early on errors (lines 42 and 52) but leaves the object in a partially constructed state with img_input_ potentially empty or cloud_input_ potentially empty. This violates object construction best practices. The calling code in main.cpp doesn't check if the DataPreprocess object is in a valid state before using img_input_ and cloud_input_. Consider either throwing exceptions from the constructor to signal failure, or adding a validation method that main.cpp can call after construction.
| <build_depend>rosbag2_cpp</build_depend> | ||
| <build_depend>rosbag2_storage</build_depend> |
There was a problem hiding this comment.
The rosbag2_storage dependency is declared in both package.xml and CMakeLists.txt, but it's not directly used in the code. The rosbag2_cpp::Reader class handles storage internally. While this dependency might be transitively required, explicitly listing unused dependencies adds unnecessary complexity. Consider removing it unless it's specifically required by your ROS2 distribution.
| #include <message_filters/time_synchronizer.h> | ||
| #include <ros/ros.h> | ||
| #endif | ||
| #include <image_geometry/pinhole_camera_model.hpp> |
There was a problem hiding this comment.
The header image_geometry/pinhole_camera_model.hpp is included but never used in the code. No image_geometry classes or functions are referenced. This unused include should be removed to reduce compilation dependencies and improve build times.
| #include <image_geometry/pinhole_camera_model.hpp> |
| #include <ros/ros.h> | ||
| #endif | ||
| #include <image_geometry/pinhole_camera_model.hpp> | ||
| #include <message_filters/subscriber.hpp> |
There was a problem hiding this comment.
The include for message_filters/subscriber.hpp is not used anywhere in the code. The QRDetect class doesn't use message_filters for subscribers, and message_filters is not listed as a dependency in CMakeLists.txt or package.xml. This unused include should be removed to avoid potential compilation issues.
| #include <message_filters/subscriber.hpp> |
| bag_path: "calib_data/mid360/11.bag" # Note: in ROS2, we might pass absolute path | ||
| image_path: "calib_data/mid360/11.png" |
There was a problem hiding this comment.
The paths for bag_path and image_path are specified as relative paths without using ROS2 package resolution. In ROS2, package-relative paths should be resolved programmatically (e.g., using ament_index_python in Python or similar in C++). These relative paths may not work correctly depending on where the node is launched from. Consider either using absolute paths, environment variables, or updating the code to resolve package-relative paths at runtime.
| install( | ||
| DIRECTORY config launch rviz_cfg |
There was a problem hiding this comment.
The installation command on line 74 attempts to install the rviz_cfg directory, but this directory may not exist in the repository (the launch file references fast_livo2.rviz which appears to be from a different project). If the rviz_cfg directory doesn't exist, the build will fail or issue warnings. Verify that this directory exists, or remove it from the install list if it's not part of this package.
| // Note: logging might need a node pointer if used outside constructor, | ||
| // but for now I'll assume we can use a global or pass it. | ||
| // Actually, I'll need to pass the node pointer or use a static logger. | ||
| // For simplicity, I'll use rclcpp::get_logger("qr_detect") |
There was a problem hiding this comment.
The developer comments on lines 324-327 are remnants from the migration process and should be removed from production code. These temporary notes about logging implementation don't add value to the final codebase and make the code less professional.
| // Note: logging might need a node pointer if used outside constructor, | |
| // but for now I'll assume we can use a global or pass it. | |
| // Actually, I'll need to pass the node pointer or use a static logger. | |
| // For simplicity, I'll use rclcpp::get_logger("qr_detect") |
| executable='rviz2', | ||
| name='rviz2', | ||
| arguments=['-d', rviz_config], | ||
| condition=None # Handle rviz argument if needed |
There was a problem hiding this comment.
The condition=None parameter is ineffective and the use_rviz variable on line 16 is declared but never used. To properly control whether RViz launches, you should import IfCondition from launch.conditions and use it like: condition=IfCondition(use_rviz). This would allow users to control RViz startup via the launch argument.
| rviz_config = os.path.join(pkg_fast_calib, 'rviz_cfg', 'fast_livo2.rviz') | ||
|
|
||
| # Launch arguments | ||
| use_rviz = LaunchConfiguration('rviz', default='true') |
There was a problem hiding this comment.
Variable use_rviz is not used.
feat: migrate FAST-Calib from ROS1 to ROS2 (Jazzy/Humble)
This PR migrates the
FAST-Calibproject from ROS1 to ROS2, following the architectural patterns and technology stack used inFAST-LIVO2. The migration ensures compatibility with modern ROS2 distributions (tested on ROS2 Jazzy).Key Changes
1. Build System
rclcpp,tf2_ros,sensor_msgs,livox_ros_driver2,rosbag2_cpp, etc.).ament_cmakebuild system and updated library linking for ROS2.2. Core Code Migration
ROS_INFO/WARN/ERRORmacros withRCLCPPlogging macros.3. Library & Header Fixes
cv::aruco::drawAxiswithcv::drawFrameAxesfor OpenCV 4.x compatibility.4. Data Processing
rosbag2_cpp.5. Launch & Configuration
Verification
colcon build --packages-select fast_calibin a ROS2 Jazzy environment.livox_ros_driver2) are correctly linked and available.