Add waypoint plugin for taking photos at arrivals

[jediofgever]

Hi ,

https://github.com/ros-planning/navigation2/commit/eff208ef5aa1b26356bda8f5fa8f304c7884327c was introduced to execute tasks at waypoint arrivals. We now want to add a tutorial to help people understand how to build their own plugins using provided interface.

For this purpose the tutorial plugin does following; subscribes to specified image topic takes photos at waypoint arrivals and saves taken photos to a specified directory with stamp.

An initial work has been committed https://github.com/jediofgever/navigation2_tutorials/commit/9b774af1339e93a45e1e6fb6e3839ceb0955c3a1

I would like to get an rough feedback on code and styling then I will be happy to submit a PR with requested modifications

[SteveMacenski]

This also overlaps the primary ticket: https://github.com/ros-planning/navigation.ros.org/issues/89

[SteveMacenski]

Rough review:

• The name... we should work on that. Also this plugin might be better suited to be in the waypoint follower directly, I think this is sufficiently useful to have as a plugin in the pkg.
• You have some cmake issues but we can deal with that in the real PR
• tabs... remove all tabs.... you should change your settings on your editor to never use tabs. I don't think any open source project in the ROS community will accept tabs
• The common.h stuff, isn't that stuff taken care of for your by image transport or image_common?
• Default paths / topic names should be more standardized, but we can clean that up quickly in the PR
• C++ contains proper filepath tools, you should use them
• Use image transport subscriber to allow people to use compression streams and it handles alot of the boiler plate for you

[jediofgever]

The name...

• Sure no problem, also welcome to suggest a name if there are any on your mind

You have some cmake issues

• Will be dealt with in PR

tabs... remove all tabs....

• Noted, I will use the auto formatting provided in ament_code_style.cfg

The common.h stuff

• I did saw these functions being embedded somewhere in image_pipeline. But in my case the simulated realsense camera encoding type is rgb8, a type which they don't cover in original implementation, so I had to add this modified function, I believe most of gazebo simulated cameras will have same encoding in default setting, so IMO it is better to keep functions here and have control over them.

  Default paths / topic names

• Yes my username isn't the best default path, they will be modified

C++ contains proper filepath tools

• I need more info on this, for example at which lines we need to replace the existing code with native C++ path tools ?

Use image transport subscriber to allow

• This was my original ideal, but seems like we cannot do that at the moment, the parent node(waypoint_follower) is a managed LifeCycleNode, a type of Node image_transport does not support yet , relevant issue; https://github.com/ros-perception/image_common/issues/108

So I will do modifications and submit a PR to navigation2 , does that mean the plugin wont be added to this repo ?

[SteveMacenski]

OK - on image_transport, it wouldn't take that long to make that all work. We have many examples of supporting different node types in Navigation2.

[jediofgever]

I suggest that we stick with the current state on that, to be able to make image_transport work with LifecycleNode will require modification image_transport API itself. I believe some time in future they will add that support. Also I haven't come across any package that use image_transport with a LifecycleNode at the moment

[SteveMacenski]

Images should always use image_transport, its really important. The efficiency gains of having access to compression is substantial. We can continue to a PR without it, but before merging, I'll want to have the image_transport stuff patched in.

[jediofgever]

I see , just an idea ,how about if we make this plugin to have double inheritance as such;

class WaypointPaparazzi : public nav2_core::WaypointTaskExecutor, public rclcpp::Node 
{
};

then I believe we will be able to use image_transport, but the parent LifecycleNode will be ghosted in that case ..

[SteveMacenski]

This will involve making image_transport lifecycle node compatible

[SteveMacenski]

We actually did this already :-)