ros-navigation / navigation2

ROS 2 Navigation Framework and System
https://nav2.org/
Other
2.62k stars 1.31k forks source link

Docking backwards as plugin param #4695

Open cgasconb opened 2 months ago

cgasconb commented 2 months ago

Feature request

The dock_backwards parameter as a dock_plugin parameter

Feature description

The dock_backwards param is a general param of the node, so it is fixed once the node starts. This way, the dock_plugins and dock instances are always forward or backward. In the case of needing a dock_plugin as forward and another one as backward, it is necessary to start 2 nodes changing this parameter. As this param is called each time the velocity is calculated, it would be great if this param is part of the dock_plugin params, so we can have 2 plugins for forward and backward in one node.

SteveMacenski commented 1 month ago

This makes sense and you are not the first to bring this up, so I think this is worthwhile.

I took a look and it appears that most of the uses of dock_backwards_ are trivial to replace with a dock->plugin->dockDirection() or so forth. Then in the simple docking plugins, I can make the dock_direction a parameter so you can specify for each plugin instance which to use. Thus, you could have a forward and reverse plugin instance within a single server instance.

A couple of APIs would need to change (resetApproach to pass in the direction or the current plugin; move the parameter off the server) but nothing technically complicated.

I would propose the following:

I think that should work. Then the open question I have is:

We could still get the parameter and log a warning that this is depreciated and to use the new dock plugin API. However in that case if we get it, we need to deconflict it when we use it in dockRobot and undockRobot.

Alternatively, we could have the simple plugins have their own <dock name>.dock_direction parameter, and if that's not set, look for the global dock_backwards parameter instead to use. Then also log that its depreciated and use <dock name>.dock_direction instead. The deconfliction is that if its set on the plugin-level, the global-level deprecated parameter is never obtained.

I think the latter is cleaner, but works primarily for users of the simple charging / non-charging plugins provided. I suppose that is 'fine' as long as we don't backport these changes and only available in Rolling and newer. I'm not sure we could backport the changes anyway since we change the API of resetApproach.

What do you think? Interest in contributing this or something I should work on on the way to ROSCon?