Astra camera ROS2 support

ros2 / ros_astra_camera

ROS wrapper for Astra camera

9 stars 12 forks source link

Astra camera ROS2 support #3

Closed clalancette closed 7 years ago

clalancette commented 7 years ago

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

clalancette commented 7 years ago

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.

mikaelarguedas commented 7 years ago

Could you please rebase this on top of master so that the ros2 branch includes the latest changes from upstream? thanks!

clalancette commented 7 years ago

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:

Continue with this review, and when the review is done, reset ros2 back to an earlier version and push this version instead.
Rebase ros2 right now, doing similar fixes as I had to do for this one.
Comment ros_astra_camera out of turtlebot2_demo.repos until we get this review complete. Then we can do whatever we need without risk of breaking turtlebot2_demo.repos.

Thoughts?

mikaelarguedas commented 7 years ago

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.

clalancette commented 7 years ago

All right, so I'm going to go in and rebase the ros2 branch on top of master, and then things should be happier.

mikaelarguedas commented 7 years ago

thanks!

clalancette commented 7 years ago

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.

mikaelarguedas commented 7 years ago

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

mikaelarguedas commented 7 years ago

no need to review all changes since it has been forked, will address comments though

clalancette commented 7 years ago

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.