Contributing Apex.AI's ROS 2 Velodyne driver

Hi,

we at Apex.AI have contributed our ROS2-based Velodyne driver to the Autoware.Auto project

However, we’d like to contribute this driver to the ros-drivers initiative instead, since we believe that other people may benefit from it. Compared with the existing Velodyne driver, Autoware.Auto’s is lacking support for more models (it only supports VLP16), however the one we developed was done with real time constraints in mind (static memory allocation, deterministic lifecycle, etc.). Moreover, it contains an extensive test suite and has been audited with Silexica’s tools to improve its performance.

I wonder if there could be a way to merge the two efforts or contribute our improvements, we’re happy to maintain the Velodyne driver.

Thanks.

3 Likes

Hi Esteve!

I’m not the maintainer of https://github.com/ros-drivers/velodyne (that would be @JWhitleyWork) , but I did do the ROS 2 port there (on the dashing-devel branch). I think it would be awesome to combine the efforts so we don’t have too much duplication.

That being said, I think it is going to be hard to do without a significant rewrite of one of them (maybe both of them).

Personally, I’d try to approach this in an iterative fashion where pieces of the Autoware driver get slowly fed into the ros-drivers driver as they make sense. For example, one place that makes sense to combine is the low-level dissection of packets, since that should definitely be common code. Another thing that would probably make sense in the upstream velodyne repository is some of the tests.

However, I don’t know the Autoware version of the code, so I can’t say how far this approach will take us. Thoughts?

Actually, @JWhitleyWork is a member of the Autoware project as well and he advised me to chime in here so we all could discuss this in the open :slight_smile:

We had a few discussions and I agree with him that for a significant number of people, having support for as many models as possible is more important than the real time stuff or the tests.

So I wonder if by adding support for the rest of models in the Autoware.Auto driver would be easier than breaking it into smaller pieces and adding it to the existing ROS 2 driver in ros-drivers, since it’d require structural changes to accommodate the features we developed.

There are two major reasons not to use the Autoware driver at the moment:

  1. It only supports the VLP-16
  2. It is dependent on a bunch of the Autoware infrastructure (that is, you can’t easily use it or even compile it without bringing in Autoware)

If we can solve both of those issues, then bringing support for additional devices into the Autoware driver is fine by me. Unfortunately, I only have a VLP-16 to test with, so I can’t really help with testing other devices at present.

Yeah, that’s one of the concerns I raised and I woudn’t pursue merging it until more models are supported.

The Velodyne driver in Autoware only depends on lidar_utils and udp_driver, the former can be just integrated into the driver, the latter is a consequence of how we developed the driver, where we separated concerns into smaller packages (e.g. reading from a UDP source, translating packets into sensor_msgs messages, etc.) Besides that, there’s no Autoware-specific code, the driver is largely separate from the rest of the Autoware code base.

Sure. What I’m trying to get at is that users who are not Autoware (or automotive at all) want to just be able to pull a small source repository, build it, and launch it. While I agree that the Autoware Velodyne driver doesn’t have a heavy dependency on Autoware, it does have some dependencies (Files · master · AutowareAuto / AutowareAuto · GitLab and the lidar_utils and udp_driver are all examples).

I think that the Velodyne driver should be totally independent from Autoware. It should have its own source repository and not be dependent on any Autoware-specific bits. How many packages and how it is split up is less important to me. But that kind of gets us back to my original proposal to put it in GitHub - ros-drivers/velodyne: ROS support for Velodyne 3D LIDARs :smile:

Sorry, what I meant is that we also want to contribute all those other packages as well, so that the driver does not to pull anything from the Autoware.Auto repository and stays completely independent. Also the the way we separated each part of the driver may benefit other drivers. For example, the udp_driver package can be used to write drivers for any other sensor that spits out UDP packets. Additionally, it’s fairly straightforward to port the udp_driver to TCP or serial ports.

Perfect! That works for me. There’s a minor issue in that we’ll probably have a time period where both the old velodyne repository and the new one you are working on are both active, and it may be hard for users to figure out which one to use. But we can fix that with documentation.

Thanks for the discussion.

Awesome :slight_smile: I’m going to spin off the driver into its own GitHub repository to ensure that it can be made completely independent from Autoware and then we can start working on integrating it into ros-drivers.

This sounds like an excellent plan, gentlemen! I also believe that a seperate, modular TCP/UDP stack is the best way to go and the principals used to develop the AutowareAuto driver are the right approach. @esteve - would it be possible for you to split the TCP/UDP layer into its own repository and then have another for the Velodyne driver that relies on it? I’d be happy to create the network driver repo under ros-drivers.

Sure thing. I’m currently setting up GitHub - ApexAI/velodyne_driver: A ROS 2 driver for Velodyne 3D LiDARs and GitHub - ApexAI/velodyne_driver at initial-checkin, but once I get the CI working, I’ll merge the latter and will create a separate repository for the udp_driver package.

3 Likes

I’ve put all the packages in a separate repository in https://github.com/ApexAI/velodyne_driver/tree/initial-checkin, but for some reason CI fails with an error in ament_cppcheck. I filed an ticket in https://github.com/ament/ament_lint/issues/177 but I’m unsure where the issue is.

Let me know when you’ve created the repository, and I’ll submit a pull request to add the udp_driver package.

@tfoote / @dirk-thomas / @Vincent_Rabaud / @Mike_Purvis / @wjwwood - You guys are owners of the ros-drivers organization on GH. Can one of you create a new repo called something like transport or transport_drivers or transport_layers and we will import the udp_driver as the first package in the stack? That or promote me to an owner and I can set it up. We’ll need a -release repo as well under ros2-gbp eventually.

I’ve created https://github.com/ros-drivers/transport_drivers and https://github.com/ros-drivers-gbp/transport_drivers-release and have given access to the Velodyne Maintainers team to both.

PS We might want to review the naming but I went with the most descriptive one I saw on the list above.

2 Likes

Thanks, @tfoote. However, I think these will only be targeting ROS2. Can you create https://github.com/ros2-gbp/transport_drivers-release or does only @wjwwood have access for that organization?

I have access, but in general as we’re moving forward we’re decreasing the differentiation between ROS 1 and ROS 2 hosting.

We started with ROS 2 in a separate organizations to make sure not to disrupt ongoing development in ROS 1 and to avoid miscommunications about people grabbing the wrong version of things while they were early in the development cycle.

However now that we’re mature and working on transitioning things from ROS 1 to ROS 2 many are going to be maintained in development branches on the original upstream repositories. Duplicating every repository and every team and maintaining all of that in parallel on multiple github organizations doesn’t make sense in the bigger picture. In general we’re working to converge development back into the main repository even for things that we forked for ROS 2 since most of the codebase will be overlapping and bug fixes and issue tracking can apply to both versions as well as it’s common for maintainership to be shared.

2 Likes

Thanks for the clarification, @tfoote.

1 Like

@tfoote Could you add me to the Velodyne team as well? I’d like to upload the udp_driver from the Autoware.Auto Velodyne driver. Thanks :slight_smile:

Sure, invitation sent.

1 Like