feat(velodyne): enable dynamic reconfigure of launch_hw parameter

[mbharatheesha]

PR Type

• New Feature

Description

This PR introduces changes to allow for dynamically reconfiguring the launch_hw parameter for the velodyne driver so that the driver can be started at run time instead of always at startup

Review Procedure

1. I guess the first step would be to establish consensus on whether the support for making launch_hw parameter reconfigurable is desired.
2. Then it would be good to agree on whether the approach taken in this PR is ok to go ahead with for other lidar types or a discussion on alternate approaches.
3. Reproduce the test results that are listed below

I tested these changes by running the driver with an actual VLP16 connected by doing the following tests:

• Start the driver with launch_hw set to false. Then change the parameter to true. The driver successfully starts communicating with connected hardware and publishes pointclouds
• Start the driver with launch_hw set to true. The driver publishes pointclouds as expected. Then change the parameter to false. The driver stops publishing pointclouds.
• If the parameter was already true and it is set to true again, nothing happens. Same if the parameter was false and set to false again.

Remarks

1. I changed the C++ standard to C++ 20 because jthread support is integrated into stl. And jthread simplifies the stopping and starting of threads when launch_hw is changed at run time. The behavior in this PR could also be achieved with std::thread and C++ 17, but I just felt the possibility to use request_stop and the stop_token features made for better readability.
2. To avoid errors regarding redeclaration of already declared parameters on dynamic reconfigure, I added conditionals to only declare a parameter if it didn't already exist.

While the desired dynamic reconfiguration works without any errors, there is one unexplained behavior that I would like some feedback on:

• It seems like the while loop in the thread gets stuck on packet_queue_.pop() and doesn't allow the thread to finish. For example, if I try to do a decoder_thread_.join() in the clean_up_on_reconfigure() function before deleting the hw interface and hw monitor objects, the call to clean_up_on_reconfigure() never finishes.

Pre-Review Checklist for the PR Author

• Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

• Commits are properly organized and messages are according to the guideline
• (Optional) Unit tests have been written for new behavior
• PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

• All open points are addressed and tracked via issues or tickets

CI Checks

• Build and test for PR: Required to pass before the merge.

[mbharatheesha]

@mojomex I was working on this feature for our application that required us to enable or disable lidar hardware at runtime. I thought this might also be interesting to other users and hence thought of creating this PR. If this is a desired feature, I can also work on similar features for the supported lidar types.

[mbharatheesha]

I made improvements to the decoder thread management. Now, the decoder thread is monitored and confirmed it exits gracefully before reconfiguration happens. I checked this by giving the decoder thread a custom name and monitor it on htop when launch_hw is false and launch_hw is true.

launch_hw is false

We can see that the VelDecThread is running and is sorted to be at the lowest when sorted on % %CPU use. In this configuration, the thread was basically doing nothing as I wasn't publishing anything on velodyne_packets.

Reconfiguring phase after setting launch_hw to true

During the reconfiguration process, the thread is exited and joined to the calling thread. And it can also be seen on htop that the VelDecThread disappears from the htop list.

After reconfiguration complete on setting launch_hw to true

Once the reconfiguration is complete, the VelDecThread re-appears on the htop list. And now, it is on the top of the pile when sorted on % CPU use. This is natural because it is actually doing the work of processing lidar packets and publishing ROS pointcloud messages.

[mojomex]

Hi @mbharatheesha, thanks for the PR!

We've indeed been thinking about this but haven't gotten around to it yet.
This functionality is pretty close to what ROS2 Lifecycle Nodes are aiming to do: launch a node (possibly allocate buffers, validate parameters etc.) and on a service call activate it (connect to sensor etc.).
By implementing this feature using lifecycle nodes, standard tooling could interact with Nebula as well.

While I share your enthusiasm for C++20, there are still some Nebula users (e.g. users of some ECUs) whose compilers don't support C++20. So at the moment, as much as I'd want to move to C++20, we'll have to wait for another bit of time.

Otherwise, super nice PR! Please let me know if you would be up for making those changes or if that is too much work at the moment.

[mbharatheesha]

@mojomex Thanks for your feedback.

We've indeed been thinking about this but haven't gotten around to it yet.
This functionality is pretty close to what ROS2 Lifecycle Nodes are aiming to do: launch a node (possibly allocate buffers, validate parameters etc.) and on a service call activate it (connect to sensor etc.).
By implementing this feature using lifecycle nodes, standard tooling could interact with Nebula as well.We've indeed been thinking about this but haven't gotten around to it yet.

I understand. Initially that was something I thought too looking at the recent implementations in Ouster drivers with LifecycleNodes. But, I thought it would make the delta really big and indeed, I wasn't aware of the direction you guys were thinking about. So, I decided against it.

While I share your enthusiasm for C++20, there are still some Nebula users (e.g. users of some ECUs) whose compilers don't support C++20. So at the moment, as much as I'd want to move to C++20, we'll have to wait for another bit of time.

Sure, I can revert the implementation to C++17 and use std::thread based approach as was the case before.

Otherwise, super nice PR! Please let me know if you would be up for making those changes or if that is too much work at the moment.

As of the next three weeks, I don't have time to look into the lifecycle implementation approach. After that, I can take a look at this again. In the meanwhile, if you guys think this can be a stand-in, let me know. Otherwise, I am fine to close this PR and re-open a new one later when I get a chance to work on LifeCycle Node approaches.

[mojomex]

@drwnz What do you think?

[drwnz]

@mbharatheesha @mojomex thanks for proposing this!
As @mojomex has said, long term we definitely want to propose a LifeCycle node approach. However, as this requires some alignment with Autoware, it's not going to be fast. So if this PR can easily be supported in C++17, then I would be in favour of testing and merging it as I think it would also be helpful in some of our use cases.

[mbharatheesha]

@drwnz Alright. I will make that change over the weekend. Also, I think similar PRs for hesai and robosense would be nice. I will also make separate PRs for them to keep focused deltas per supported lidar type.