search

ros2 / ros_astra_camera

ROS wrapper for Astra camera
9 stars 12 forks source link

Ros2 cleanup #1

Closed clalancette closed 7 years ago

clalancette commented 7 years ago

This PR gets the astra camera working a bit better with ROS2. In particular, it makes it compile again (though only against Fast-RTPS), it turns the color and IR camera code back on, and it publishes the depth data on a separate topic from color on a separate topic from IR. Note that in order for this to really work well, you need to edit rmw_fastrtps/rmw_fastrtps_cpp/src/functions.cpp to make the heartbeat go every 10ms; see https://github.com/ros2/rmw_fastrtps/issues/81 for more information.

mikaelarguedas commented 7 years ago

I don't know how much of this should be code cleanup. If you think it's relevant here, you could run the linters on this package that seems to have a lot of linter violations by looking at it. This can also be done in a follow-up PR if you think it's more appropriate

wjwwood commented 7 years ago

I didn't mention the linters because this is technically a fork of an existing ROS 1 package (not an original ROS 2 creation). I don't mind either way though.

clalancette commented 7 years ago

I don't want to drift too far from the original package if we can help it. It seems to me that we should be able to support ROS1 and ROS2 on the same branch of the original package, given a bit more work[1]. So for now, I'd rather not go crazy linting it. I definitely will fix the cmake/ament problems, though.

[1] Does ament support building a package with parameters? From a brief glance I didn't see how to do this, but I didn't look very closely.

mikaelarguedas commented 7 years ago

Sounds good, given that it was using ament specific macros etc I lost track that it was a ros1 package. +1 for not diverging too much then

ament allows to pass cmake options / arguments using --cmake-args, is that what you mean by "parameters" ?

clalancette commented 7 years ago

Using --cmake-args is a start, though it would be better if it could somehow detect the "ROS2" version of the package.xml and use that as appropriate. That way the ROS1 and ROS2 code could live side-by-side, and the build systems would pick the right one. I didn't (and won't) push too hard on this until we figure out whether we are going with ament, catkin, or something else going forward with ROS2.