ROS2 version #3
ROS2 version #3
Conversation
There was a problem hiding this comment.
Thank you so much for this port. I am pleased that we are providing a valuable package for the open-source community.
I would like to express some concerns about the potential merger of i2c_device_ros into this package, as I mentioned in the header file. In light of these concerns, I have decided to pause my current revision and would like to propose further discussion on this matter after I have had the opportunity to continue with my review.
| # add_definitions(-Wall -Werror) | ||
| # Default to C++14 | ||
| if(NOT CMAKE_CXX_STANDARD) | ||
| set(CMAKE_CXX_STANDARD 14) |
There was a problem hiding this comment.
I suggest we put C++17 here to comply with the REP 2000 for Humble release
| # Link against ROS 2 libraries | ||
| # target_link_libraries(mpu6050_node_lib | ||
| # ${rclcpp_LIBRARIES} | ||
| # ) |
There was a problem hiding this comment.
Please, remove commented lines
| # Install targets | ||
| # install(TARGETS | ||
| # mpu6050_node | ||
| # mpu6050_calibration_node | ||
| # mpu6050_node_lib | ||
| # DESTINATION lib/${PROJECT_NAME}) |
There was a problem hiding this comment.
Please, remove commented lines
| **Maintainer:** Mateus Meneses, [email protected] | ||
|
|
||
| The MPU6050 Driver package has been tested under [ROS] Kinetic and [Raspbian Jessie]. | ||
| The barnch `ros2_humble` has been tested under [ROS] `humble` and [Raspberry Pi Bookworm] . |
There was a problem hiding this comment.
| The barnch `ros2_humble` has been tested under [ROS] `humble` and [Raspberry Pi Bookworm] . | |
| The branch `ros2_humble` has been tested under [ROS] `Humble Hawksbill` and [Raspberry Pi Bookworm] . |
| ```sh | ||
| $ cd <YOUR_WS>/src | ||
| $ git clone https://github.com/mateusmenezes95/mpu6050_driver.git | ||
| $ git clone -b ros2_humble https://github.com/mateusmenezes95/mpu6050_driver.git |
There was a problem hiding this comment.
As this PR target the master branch, I suggest remove the branch option from this command
| // void MPU6050Node::loadParameters() { | ||
| // this->declare_parameter("bus_uri", "/dev/i2c-1"); | ||
| // this->declare_parameter("mpu_address", 0x68); | ||
| // this->declare_parameter("pub_rate", 30.0f); | ||
| // this->declare_parameter("frame_id", "imu_link"); | ||
| // this->declare_parameter("axes_offsets", std::vector<int>{0, 0, 0, 0, 0, 0}); | ||
|
|
||
| imu_data.linear_acceleration.x = mpu_data.accel.x * gravity_value; | ||
| imu_data.linear_acceleration.y = mpu_data.accel.y * gravity_value; | ||
| imu_data.linear_acceleration.z = mpu_data.accel.z * gravity_value; | ||
| // this->get_parameter("bus_uri", i2c_bus_uri_); | ||
| // this->get_parameter("mpu_address", mpu6050_addr_); | ||
| // this->get_parameter("pub_rate", pub_rate_); | ||
| // this->get_parameter("frame_id", imu_frame_id_); | ||
| // this->get_parameter("axes_offsets", axes_offsets_); | ||
| // } |
There was a problem hiding this comment.
Please remove the commented code
| add_executable(mpu6050_node src/main.cpp) | ||
| ament_target_dependencies(mpu6050_node rclcpp sensor_msgs) | ||
| target_link_libraries(mpu6050_node mpu6050_node_lib) |
There was a problem hiding this comment.
| add_executable(mpu6050_node src/main.cpp) | |
| ament_target_dependencies(mpu6050_node rclcpp sensor_msgs) | |
| target_link_libraries(mpu6050_node mpu6050_node_lib) | |
| add_executable(mpu6050_node | |
| src/main.cpp | |
| ) | |
| ament_target_dependencies(mpu6050_node | |
| rclcpp | |
| sensor_msgs | |
| ) | |
| target_link_libraries(mpu6050_node | |
| mpu6050_node_lib | |
| ) |
Nitpick to comply with what we do in our internal repositories. Please apply this to the following lines, too.
| install(DIRECTORY include/ | ||
| DESTINATION include) |
There was a problem hiding this comment.
| install(DIRECTORY include/ | |
| DESTINATION include) | |
| install( | |
| DIRECTORY include/ | |
| DESTINATION include | |
| ) |
There was a problem hiding this comment.
This file is almost the same for the node. Why should we have both of these two files?
There was a problem hiding this comment.
So, I'm afraid I have to disagree with bringing this library to this package. The i2c_device_ros library is intended to be used in devices beyond the MPU6050, e, g. to be a generic class for I2C devices connected to Raspberries. Hence, if the package for this library receives improvements, it will not enjoy these improvements unless we bring it manually, which is undesirable. Could you please evaluate if you can port the i2c_device_ros to ROS 2 thus, we use here ament_target_dependencies
I modified this package to work with ROS2. It was tested successfully on ROS humble on raspberry pi 4B with OS Bookworm.
I removed the dependency on the i2c package by including the source code in this package so it can be self contained.
I hope this is useful for others.
Thank you.