Port stereo_image_proc to ROS 2 #486

jacobperron · 2019-12-13T23:32:26Z

Connects to #360.

I think the majority of the port is complete. Opening this PR for visibility/early feedback. I've removed OpenCV 2 support in an attempt to simplify the code.

In addition to the port, I've added some simple launch tests that can be expanded upon in the future.
Test data taken from OpenCV (from here).

Here's what's missing:

Monitoring the status of connected publishers to dynamically subscribe/unsubscribe as needed

image_pipeline/stereo_image_proc/src/stereo_image_proc/disparity_node.cpp

Lines 128 to 132 in a0a361b

    
           // TODO(jacobperron): Monitoring subscriber status is currently not possible in ROS 2 
        
           // std::lock_guard<std::mutex> lock(connect_mutex_); 
        
           // Monitor whether anyone is subscribed to the output 
        
           // ros::SubscriberStatusCallback connect_cb = std::bind(&DisparityNode::connectCb, this);

This feature doesn't exist in ROS 2, so I've left TODOs in the code (similar to what was done in other image_pipeline packages).

Reconfigurable callbacks for parameters. This should be achievable since parameters are configurable by default. I'll proceed replacing the use of dynamic reconfigure using the rclcpp API. I can add it to this PR or after it is merged, whatever comes first.
I should also double-check QoS compatibility with other parts of image_pipeline (mentioned in Port Image_pipeline on ROS2 #360 (comment)).
Windows support. E.g add missing visibility flags, cmake commands, etc.

I've been building/testing with ROS 2 Eloquent and the latest ROS 2 source code.

jacobperron · 2019-12-13T23:49:18Z

rclcpp_components_register_node was recently backported to Dashing. It should be available in the next patch release and then hopefully CI will be happy.

SteveMacenski

LGTM, just a couple comments

stereo_image_proc/launch/stereo_image_proc.launch.py

SteveMacenski · 2019-12-16T18:04:24Z

stereo_image_proc/src/stereo_image_proc/disparity_node.cpp

+  int left = block_matcher_.getDisparityRange() + block_matcher_.getMinDisparity() + border - 1;
+  int wtf;
+  if (block_matcher_.getMinDisparity() >= 0) {
+    wtf = border + block_matcher_.getMinDisparity();


What does wtf represent? I think that could be named clearer

I wish I knew, haha. I didn't touch the logic that was here since ROS 1. Any suggestions are appreciated.

I... don't even know what that is doing (or why there isn't one of those for the height). I'm going to just back away slowly before I go too far down a rabbithole

stereo_image_proc/src/stereo_image_proc/point_cloud_node.cpp

jacobperron · 2019-12-16T19:51:47Z

Note, I also did some minor refactoring a25372d

SteveMacenski

Any other changes you'd like to make?

We should wait until we pass CI though,

jacobperron · 2019-12-16T20:13:54Z

I want to add the reconfigurable parameters, and figure out what wtf is.. but I can make changes in separate PRs.

It'd be nice if CI was green though before this one goes in. I figured since the latest dashing sync it should be working... 🤔

JWhitleyWork · 2019-12-16T20:14:28Z

@jacobperron / @SteveMacenski - They have to manually re-build the Docker containers with the updated packages for CI to pass.

SteveMacenski · 2019-12-16T21:25:06Z

Looks like it was updated 3 days ago https://hub.docker.com/_/ros?tab=tags

SteveMacenski · 2019-12-16T21:25:44Z

Potentially do it the more nasty way to get CI to build?

JWhitleyWork · 2019-12-16T21:33:09Z

@SteveMacenski - You're right. I checked the SHA hash of the image it pulled and CI must be using a cached copy somewhere. I don't know that there is anyway to update this from our end.

jacobperron · 2019-12-16T21:40:26Z

I don't think the image has been updated yet. If I pull ros:dashing-ros-base locally, it doesn't contain the latest version of ros_base (0.7.3).

SteveMacenski · 2019-12-16T21:40:32Z

https://circleci.com/docs/2.0/caching/#clearing-cache

@jacobperron try adding a cache variable we can increment, ei CACHE_VERSION=v1

Edit: ah, ok

jacobperron · 2019-12-16T22:07:58Z

Also, I just noticed that my tests aren't compatible with Dashing, so I'll update those in the meantime.

jacobperron · 2019-12-16T22:52:05Z

@SteveMacenski I've updated the tests so they're backwards compatible with Dashing (ecce41b). PTAL.

jacobperron · 2019-12-16T23:39:12Z

TL;DR It doesn't seem like the Docker image will be updated any time soon. Not until the underlying Ubuntu image gets rebuilt: osrf/docker_images#112

It's a bit annoying, but I can refactor the CMakeLists.txt to see if we can make CI happy.

jacobperron · 2019-12-17T17:47:44Z

See #487 for a proposed fix for failing CI.

SteveMacenski · 2019-12-17T18:01:48Z

Merged.

More descriptive names that will match the classes contained inside. Also moved all source files associated with the library into a directory of the same name. Signed-off-by: Jacob Perron <[email protected]>

Made some minimum changes required for compilation. Not feature-complete yet. Signed-off-by: Jacob Perron <[email protected]>

The test simply checks if the disparity node publishes to the expected topic when it received input. Made some fixes added to CMakeLists.txt to get node to run probably and pass the test. Signed-off-by: Jacob Perron <[email protected]>

Similar to the test for the disparity node. Fix bug in PointCloudNode. Signed-off-by: Jacob Perron <[email protected]>

Signed-off-by: Jacob Perron <[email protected]>

Replaces the old ROS 1 XML launch file. Signed-off-by: Jacob Perron <[email protected]>

This form of manual composition seems redundant to me and can be done with a launch file. The only thing it appeared to add was the 'advertisement checker', which was not ported. If there is strong objection to removing this executable, we can add it back. Signed-off-by: Jacob Perron <[email protected]>

There are a few cases where we discard the const-qualifier for the image data. This happens because OpenCV won't accend const pointers to data. Rather than changing the API, I've introduced a const_cast instead. Signed-off-by: Jacob Perron <[email protected]>

Signed-off-by: Jacob Perron <[email protected]>

Fix lint error. Signed-off-by: Jacob Perron <[email protected]>

The code removed was never being used. Added a TODO regarding the use of an unset variable. This has been here since the ROS 1 implementation, I'm not sure what the logic is intended to do so I've left it for now. Signed-off-by: Jacob Perron <[email protected]>

Signed-off-by: Jacob Perron <[email protected]>

Save on some vertical space. Signed-off-by: Jacob Perron <[email protected]>

Remove commented code related to subscriber status since this may look different in ROS 2. Signed-off-by: Jacob Perron <[email protected]>

Now the launch tests should work for both Dashing and Eloquent. I left some TODO's where we can simplify once we diverge from Dashing. Signed-off-by: Jacob Perron <[email protected]>

Signed-off-by: Jacob Perron <[email protected]>

jacobperron · 2019-12-17T18:05:23Z

Rebased 🤞

jacobperron added the ros2 label Dec 13, 2019

jacobperron mentioned this pull request Dec 13, 2019

Port Image_pipeline on ROS2 #360

Closed

7 tasks

SteveMacenski requested changes Dec 16, 2019

View reviewed changes

SteveMacenski approved these changes Dec 16, 2019

View reviewed changes

jacobperron mentioned this pull request Dec 17, 2019

Install latest package versions during CI #487

Merged

jacobperron added 11 commits December 17, 2019 10:04

Rename source files

c94a86a

More descriptive names that will match the classes contained inside. Also moved all source files associated with the library into a directory of the same name. Signed-off-by: Jacob Perron <[email protected]>

Get stereo_image_proc compiling with ROS 2

2c4da05

Made some minimum changes required for compilation. Not feature-complete yet. Signed-off-by: Jacob Perron <[email protected]>

Add launch test for disparity node

e35ddac

The test simply checks if the disparity node publishes to the expected topic when it received input. Made some fixes added to CMakeLists.txt to get node to run probably and pass the test. Signed-off-by: Jacob Perron <[email protected]>

Add launch test for PointCloudNode

cedfbd6

Similar to the test for the disparity node. Fix bug in PointCloudNode. Signed-off-by: Jacob Perron <[email protected]>

Remove unused files

aa6f728

Signed-off-by: Jacob Perron <[email protected]>

Add Python launch file

c3d74d0

Replaces the old ROS 1 XML launch file. Signed-off-by: Jacob Perron <[email protected]>

Add linter tests and lint

09af1f8

There are a few cases where we discard the const-qualifier for the image data. This happens because OpenCV won't accend const pointers to data. Rather than changing the API, I've introduced a const_cast instead. Signed-off-by: Jacob Perron <[email protected]>

Remove OpenCV 2 support

6cf9a8d

Signed-off-by: Jacob Perron <[email protected]>

Expand macros

b0e43c3

Signed-off-by: Jacob Perron <[email protected]>

Enable uncrustify test

5e802d8

Fix lint error. Signed-off-by: Jacob Perron <[email protected]>

jacobperron added 10 commits December 17, 2019 10:04

Update package.xml version

9cb1aa5

Signed-off-by: Jacob Perron <[email protected]>

Minor refactor

887ecf9

Signed-off-by: Jacob Perron <[email protected]>

Fix typo

5dab497

Signed-off-by: Jacob Perron <[email protected]>

Minor: update include syntax and order for consistency

a95bf88

Signed-off-by: Jacob Perron <[email protected]>

Fix rosdep key typo

b6a1b7b

Signed-off-by: Jacob Perron <[email protected]>

Move inline implementation into class

bd8dc7a

Save on some vertical space. Signed-off-by: Jacob Perron <[email protected]>

Avoid extra computation if there are no matching subscriptions

12db28a

Remove commented code related to subscriber status since this may look different in ROS 2. Signed-off-by: Jacob Perron <[email protected]>

Refactor tests for compatbility with Dashing

6e80e2f

Now the launch tests should work for both Dashing and Eloquent. I left some TODO's where we can simplify once we diverge from Dashing. Signed-off-by: Jacob Perron <[email protected]>

Use default context

09e922a

Signed-off-by: Jacob Perron <[email protected]>

jacobperron force-pushed the jacob/port_stereo_image_proc branch from 7c477b7 to 09e922a Compare December 17, 2019 18:05

JWhitleyWork approved these changes Dec 17, 2019

View reviewed changes

JWhitleyWork merged commit 144c704 into ros2 Dec 17, 2019

JWhitleyWork deleted the jacob/port_stereo_image_proc branch December 17, 2019 22:00

This was referenced Jan 7, 2020

Add reconfigurable parameters to disparity node #490

Merged

Fix get/set speckle size methods #491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port stereo_image_proc to ROS 2 #486

Port stereo_image_proc to ROS 2 #486

jacobperron commented Dec 13, 2019

jacobperron commented Dec 13, 2019

SteveMacenski left a comment

SteveMacenski Dec 16, 2019

jacobperron Dec 16, 2019

SteveMacenski Dec 16, 2019 •

edited

Loading

jacobperron commented Dec 16, 2019

SteveMacenski left a comment •

edited

Loading

jacobperron commented Dec 16, 2019

JWhitleyWork commented Dec 16, 2019

SteveMacenski commented Dec 16, 2019

SteveMacenski commented Dec 16, 2019

JWhitleyWork commented Dec 16, 2019

jacobperron commented Dec 16, 2019

SteveMacenski commented Dec 16, 2019 •

edited

Loading

jacobperron commented Dec 16, 2019

jacobperron commented Dec 16, 2019

jacobperron commented Dec 16, 2019

jacobperron commented Dec 17, 2019

SteveMacenski commented Dec 17, 2019 •

edited

Loading

jacobperron commented Dec 17, 2019

	// TODO(jacobperron): Monitoring subscriber status is currently not possible in ROS 2
	// std::lock_guard<std::mutex> lock(connect_mutex_);

	// Monitor whether anyone is subscribed to the output
	// ros::SubscriberStatusCallback connect_cb = std::bind(&DisparityNode::connectCb, this);

Port stereo_image_proc to ROS 2 #486

Port stereo_image_proc to ROS 2 #486

Conversation

jacobperron commented Dec 13, 2019

jacobperron commented Dec 13, 2019

SteveMacenski left a comment

Choose a reason for hiding this comment

SteveMacenski Dec 16, 2019

Choose a reason for hiding this comment

jacobperron Dec 16, 2019

Choose a reason for hiding this comment

SteveMacenski Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

jacobperron commented Dec 16, 2019

SteveMacenski left a comment • edited Loading

Choose a reason for hiding this comment

jacobperron commented Dec 16, 2019

JWhitleyWork commented Dec 16, 2019

SteveMacenski commented Dec 16, 2019

SteveMacenski commented Dec 16, 2019

JWhitleyWork commented Dec 16, 2019

jacobperron commented Dec 16, 2019

SteveMacenski commented Dec 16, 2019 • edited Loading

jacobperron commented Dec 16, 2019

jacobperron commented Dec 16, 2019

jacobperron commented Dec 16, 2019

jacobperron commented Dec 17, 2019

SteveMacenski commented Dec 17, 2019 • edited Loading

jacobperron commented Dec 17, 2019

SteveMacenski Dec 16, 2019 •

edited

Loading

SteveMacenski left a comment •

edited

Loading

SteveMacenski commented Dec 16, 2019 •

edited

Loading

SteveMacenski commented Dec 17, 2019 •

edited

Loading