ROS 2 Dashing port #251

clalancette · 2019-06-24T13:41:02Z

This PR is a fairly complete port of the velodyne driver to ROS 2 Dashing. I started with the work of @klintan in #237, and did quite a bit of work to finish up the port; hence, this PR would supersede #237. Note that it is currently targeted at "master" because I rebased the work of #237 on master; we'd probably want to make a "dashing-devel" branch instead. I've tested the code on VLP16, and confirmed the following to work:

Getting "raw" data packets out of a Velodyne and publishing to ROS 2 (the velodyne_driver)
Taking the VelodyneScan and converting it to a pointcloud (the velodyne_pointcloud)
Taking a published point cloud and get a laser scan line out of it (the velodyne_laserscan)

There are still some things that need to be worked on:

Add the ability to change parameters on the fly (right now they can only be set at startup)
Make the nodes composable (right now they are separate nodes; I'm waiting for Added cmake macro to create targets for shared library and executable ros2/rclcpp#758 to land in Dashing before doing this)
Port the integration tests
Re-enable parts of the logging that I've disabled
Test the "transform" velodyne_pointcloud node
Do some static analysis on the code and fix things up
Test on more Velodyne devices (though I don't have access to anything but a VLP16)
Port to Crystal (this should be possible, but I didn't bother since it will make the PR bigger and I don't know if there is interest in it)
Get the CI setup (I haven't played with this at all, it is probably broken)
Get diagnostics working (this is blocked waiting on diagnostics to be ported to ROS 2)

However, before I go any further making changes, I'd like to discuss how to land this monstrosity. This PR is probably too big to review in one go, so I'd be curious to hear from the maintainers (@JWhitleyAStuff , etc) how we want to proceed. Any thoughts appreciated; thanks!

No changes to the code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This is strictly a style cleanup, making all of the code in the repository have roughly the same style. There are no functional changes to the code at all. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

…lity clalancette: updates: 1. Heavily clean up CMakeLists.txt 2. Cleanup package.xml 3. Disable diagnostics 4. Address warnings 5. Use a single clock instance 6. Declare parameters for Dashing 7. Add "root scope" to Unix APIs 8. Use a thread for polling so the node is composable

At the moment, this only enables the velodyne_msgs and the velodyne_driver. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

klintan · 2019-06-24T13:49:37Z

Awesome work! thanks for taking this up, as you've gotten further than me, I just started taking it up again after doing some work on perception_pcl, I think we can continue from this PR maybe.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2019-06-24T14:29:24Z

Awesome work! thanks for taking this up, as you've gotten further than me, I just started taking it up again after doing some work on perception_pcl, I think we can continue from this PR maybe.

Excellent! Yeah, I'm happy to do some more work and get this in, I just wanted to pause here so I can get a feeling of what direction to go in next.

JWhitleyWork · 2019-06-24T14:33:47Z

@clalancette - Very nice work! I've re-targeted the PR to a new dashing-devel branch. Since I knew that the ROS2 port would be a nearly complete re-write, I wanted to suggest a couple of feature edits:

Make the "cut_at_specified_angle" feature the default with an angle of 2*pi radians. The current version defaults to a specific number of packets per revolution (npackets). Since the rotation rate of the sensor can vary, this causes over- and under-scanning in the generated point clouds. I think we should just do away with the npackets value and calculate some max estimated value for possible number of packets for pre-allocation purposes, then use the cut_at_specified_angle feature always.
Set "organize_point_cloud" defaulted to true.

Hopefully these aren't too much of a disruption but they should be fairly simple to implement. Thoughts?

velodyne_pointcloud/src/conversions/convert.cc

velodyne_pointcloud/src/conversions/transform.cc

klintan · 2019-06-24T14:03:32Z

velodyne_pointcloud/src/conversions/transform_node.cc


  // handle callbacks until shut down
-  ros::spin();
+  rclcpp::spin(std::make_shared<velodyne_pointcloud::Transform>());


If you change to the components approach you would need to do

rclcpp::NodeOptions options; auto conv = std::make_shared<velodyne_pointcloud::Transform>(options);

klintan · 2019-06-24T14:27:14Z

velodyne_driver/include/velodyne_driver/driver.h

-                 ros::NodeHandle private_nh);
-  ~VelodyneDriver() {}
+public :
+  VelodyneDriver();


I guess this could also be a component based node.

klintan · 2019-06-24T14:35:26Z

velodyne_driver/mainpage.dox

@@ -3,7 +3,7 @@

 \htmlinclude manifest.html

-ROS device driver for Velodyne 3D LIDARs. 
+ROS device driver for Velodyne 3D LIDARs.


"typo": ROS2

klintan · 2019-06-24T14:38:17Z

velodyne_laserscan/src/node.cpp


  // handle callbacks until shut down
-  ros::spin();
+  rclcpp::spin(std::make_shared<velodyne_laserscan::VelodyneLaserScan>());


Not a biggie, but maybe here also follow how its done in the velodyne_pointcloud package

auto laserscan = std::make_shared<velodyne_laserscan::VelodyneLaserScan>() rclcpp::sping(laserscan

or similar.
(in addition, as mentioned on other this should probably be written as the new component thingy in Dashing and use NodeOptions)

clalancette · 2019-06-24T14:42:57Z

Hopefully these aren't too much of a disruption but they should be fairly simple to implement. Thoughts?

Both of those sound like fine changes to me, and I'm happy to do them. However, I'll suggest that we use a separate PR to implement them. Otherwise, those relatively small changes will be swamped by the size of the port, and thus it may be harder for users to see why the behavior on this branch is subtly different than on master. Does that make sense?

JWhitleyWork · 2019-06-24T14:43:30Z

@clalancette - Yes, absolutely. Thanks for the suggestion.

velodyne_driver/src/lib/input.cc

clalancette · 2019-06-25T13:59:38Z

I've opened #252 to track some of the additional things we want to fix after we get the initial port in. Feel free to add to that as necessary.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This essentially fixes the problem that is currently on master and reported in ros-drivers#249. Here, we take an empty target frame (the default) to mean that we should use the frame_id incoming from the VelodyneScan; any other value (which can be set via a parameter), and we override the frame_id with that. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This gets rid of an error, and should work fine on its own. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2019-06-25T14:45:19Z

Nice. Thanks to both of you, and a slight adjustment to the circleci config, CI is now passing. I also think I've addressed all of the pending review comments (the ones that I haven't replied to yet basically all revolve around composability, which I think we should do later). I also added commit 6b17e46 to try and fix the fixed_frame/target_frame bug that is present on master and reported in #249 ; we ran into this bug in our testing as well.

Are there any blockers to getting this merged? I'm happy to make more changes, I just want to keep up the momentum.

As discussed in ros-drivers#251, we should make the default node names a lot more consistent. Here, rename the nodes to all be velodyne_{driver,convert,transform,laserscan}_node. I also renamed the "cloud_node.cc" file to be "convert_node.cc", so it matches the rest of the code. Finally, I removed the unused nodelets.xml file from velodyne_pointcloud. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

As discussed in #251, we should make the default node names a lot more consistent. Here, rename the nodes to all be velodyne_{driver,convert,transform,laserscan}_node. I also renamed the "cloud_node.cc" file to be "convert_node.cc", so it matches the rest of the code. Finally, I removed the unused nodelets.xml file from velodyne_pointcloud. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette and others added 14 commits June 17, 2019 18:02

Delete trailing whitespace.

276c19d

No changes to the code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Style cleanup.

26f8e32

This is strictly a style cleanup, making all of the code in the repository have roughly the same style. There are no functional changes to the code at all. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove unused ring_sequence.h header file.

935b2ee

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Use the global scope for Unix system calls.

0003df9

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add in cmath include for ::rint.

85d2df6

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Add in COLCON_IGNORE for everything to start ROS 2 port.

b50a295

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

custom messages in ROS2

f21f99e

CI: Testing ROS2 CI.

d866ce6

CI: Adding colcon.

813cbf1

CI: Forgot to source setup.bash again.

a3979ad

Enable the velodyne metapackage.

d1e5c35

At the moment, this only enables the velodyne_msgs and the velodyne_driver. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

ROS 2 port of velodyne_pointcloud.

cd47706

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Port of velodyne_laserscan to ROS 2.

88ab6b0

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Make sure to install the calibration files.

3eecd1d

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

JWhitleyWork changed the base branch from master to dashing-devel June 24, 2019 13:56

klintan reviewed Jun 24, 2019

View reviewed changes

velodyne_driver/src/lib/input.cc Show resolved Hide resolved

klintan reviewed Jun 24, 2019

View reviewed changes

velodyne_driver/src/lib/input.cc Outdated Show resolved Hide resolved

clalancette mentioned this pull request Jun 25, 2019

ROS 2 Port improvements #252

Closed

8 tasks

clalancette added 3 commits June 25, 2019 14:09

Fix htons problem.

beb5f96

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Simplify colcon test.

6ae01a6

This gets rid of an error, and should work fine on its own. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

JWhitleyWork merged commit 08b2866 into ros-drivers:dashing-devel Jun 25, 2019

clalancette mentioned this pull request Jun 26, 2019

Dashing: Rename nodes. #254

Merged

clalancette deleted the dashing-devel3 branch June 26, 2019 12:25

clalancette mentioned this pull request Jun 26, 2019

ROS2 #237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROS 2 Dashing port #251

ROS 2 Dashing port #251

clalancette commented Jun 24, 2019 •

edited

Loading

klintan commented Jun 24, 2019

clalancette commented Jun 24, 2019

JWhitleyWork commented Jun 24, 2019

klintan Jun 24, 2019

klintan Jun 24, 2019

klintan Jun 24, 2019 •

edited

Loading

klintan Jun 24, 2019

clalancette commented Jun 24, 2019

JWhitleyWork commented Jun 24, 2019

clalancette commented Jun 25, 2019

clalancette commented Jun 25, 2019

ROS 2 Dashing port #251

ROS 2 Dashing port #251

Conversation

clalancette commented Jun 24, 2019 • edited Loading

klintan commented Jun 24, 2019

clalancette commented Jun 24, 2019

JWhitleyWork commented Jun 24, 2019

klintan Jun 24, 2019

Choose a reason for hiding this comment

klintan Jun 24, 2019

Choose a reason for hiding this comment

klintan Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

klintan Jun 24, 2019

Choose a reason for hiding this comment

clalancette commented Jun 24, 2019

JWhitleyWork commented Jun 24, 2019

clalancette commented Jun 25, 2019

clalancette commented Jun 25, 2019

clalancette commented Jun 24, 2019 •

edited

Loading

klintan Jun 24, 2019 •

edited

Loading