Astra camera ROS2 support #3
Conversation
clalancette
added
in progress
|
Note that in order to not break the turtlebot2 demo, I ended up pushing the older work that Brian, Daniel, and myself did back onto the ros2 branch, without review. However, I will still take review comments that come from here and fix up any problems pointed out. |
|
Could you please rebase this on top of master so that the ros2 branch includes the latest changes from upstream? thanks! |
|
So, I rebased this. The problem is that it now conflicts with what is on the ros2 branch. However, I don't want to break the existing turtlebot2_demo.repos file, so I don't want to reset the ros2 branch either. Options:
Thoughts? |
|
The build has never succeeded with the the turtlebot2_demos.repos file anyway so I think it's worth getting the repos in sync with upstream before adding new code. |
|
All right, so I'm going to go in and rebase the ros2 branch on top of master, and then things should be happier. |
|
thanks! |
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
|
All right, I rebased ros2, and I rebased this one on top of ros2. It means we only see the diff for my latest code, unfortunately. Not quite sure how to address that, but I guess we'll get back to it at some point. |
|
I'd personally suggest resetting the ros2 branch to be even with master and ask for review for this PR with all the changes. We should review and iterate on this in priority if other PRs are relying on it. Maybe @ros2/team has a different feeling about this though |
CMakeLists.txt
Outdated
| @@ -61,7 +61,8 @@ add_library(astra_driver_lib src/astra_driver.cpp) | |||
| ament_target_dependencies(astra_driver_lib | |||
| "rclcpp" | |||
| "builtin_interfaces" | |||
| "sensor_msgs") | |||
| "sensor_msgs" | |||
| "image_geometry") | |||
There was a problem hiding this comment.
Nit: list dependencies alphabetically
There was a problem hiding this comment.
Nit: put parenthesis on next line so that it's easy to add dependencies without having to deal with the closing parenthesis
package.xml
Outdated
| @@ -13,6 +13,7 @@ | |||
| <depend>sensor_msgs</depend> | |||
| <depend>builtin_interfaces</depend> | |||
| <depend>rclcpp</depend> | |||
| <depend>boost</depend> | |||
There was a problem hiding this comment.
Nit: please list dependencies alphabetically
src/astra_driver.cpp
Outdated
| @@ -42,6 +42,8 @@ | |||
| #include "astra_camera/image_encodings.h" | |||
| #include "astra_camera/distortion_models.h" | |||
|
|
|||
| #include "sensor_msgs/msg/camera_info.hpp" | |||
There was a problem hiding this comment.
Nit: include as system header and alphabetically
There was a problem hiding this comment.
Isn't this already included by astra_driver.h ?
There was a problem hiding this comment.
Yeah, it does get pulled in by astra_driver.h. However, I tend to list all of the headers needed for each particular file anyway. For now, I'll include as a system header, you can let me know if you still want me to remove it.
| @@ -717,6 +720,7 @@ sensor_msgs::msg::CameraInfo::SharedPtr AstraDriver::getDepthCameraInfo(int widt | |||
|
|
|||
| void AstraDriver::readConfigFromParameterServer() | |||
| { | |||
| depth_frame_id_ = std::string("openni_depth_optical_frame"); | |||
There was a problem hiding this comment.
why use openni here ? this package doesn't depend on openni in any way does it ? Maybe "depth_optical_frame" or using "orbec" or "astra" prefix would be more appropriate
There was a problem hiding this comment.
I got that particular name out of the ROS1 astra camera driver, where they attempt to get the name for the depth_frame_id_ out of the parameter server and fall back to to "/openni_depth_optical_frame" if it doesn't exist. I don't have any particular affinity for that name, so I can change it if you feel like we should.
There was a problem hiding this comment.
I'd say leave it as upstream has it.
There was a problem hiding this comment.
Yeah, I'm going to leave it as-is for now for upstream compatibility. We can change it later if we want.
|
no need to review all changes since it has been forked, will address comments though |
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
Signed-off-by: Chris Lalancette <clalancette@osrfoundation.org>
|
All right, I think I've now addressed all review comments here. The only open question is what to name the depth_frame. Right now I've left it the same as it is in ROS1, but I'm happy to change it if we want to do something different. This is ready for review and merge otherwise. |
This is a large-ish review of all of the code necessary to make the astra camera work on ROS2. This includes the work that Brian and Daniel Stonier did on it earlier, as well as my current work on it.
connects to ros2/ros2#342