Nav2 Docking: The docking_server does not work if you do not define instances of docks using the DockDatabase

[jcarlosgm30]

Hi @SteveMacenski ,

As far as I understood from nav2_docking README.md, it is not necessary to know the dock's localization in advance if you set the pose (and use_dock_id to false) in the docking request:

The docking action can either operate on a dock in the DockDatabase or from a dock specified in the docking request. This second option is useful for testing or when dock's locales are not necessarily known in advance. If use_dock_id = true, it uses the dock_id field to specify which dock in the database to use. Else, you must populate the dock_pose and dock_type fields.

Extracted from: https://github.com/ros-navigation/navigation2/tree/main/nav2_docking#docking-action.

However, if you have not defined the dock's instances using the DockDatabase, the docking server is going to fail when it initializes:

https://github.com/ros-navigation/navigation2/blob/main/nav2_docking/opennav_docking/src/docking_server.cpp#L91

I am not sure if this is a bug or I didn't understand well the documentation.

BR,
José Carlos.

Bug report

Required Info:

• Operating System:
  • Ubuntu 22.04 LTS
• ROS2 Version:
  • ROS2 Humble
• Version or commit hash:
  • 7737a2f
• DDS implementation:
  • Eclipse Cyclone DDS

Steps to reproduce issue

1. Do not set the dock instances in the .yaml parameter file to be taken by DockDatabase
2. Run the docking_server.
3. An error appears:
   [opennav_docking-2] [ERROR] [1718100300.110008781] [docking_server]: Dock database filepath nor dock parameters set. Unable to perform docking actions.

Expected behavior

Allows to use the docking_server using directly the action request, without the need to define the dock instances using the DockDatabase

Actual behavior

It is necessary to define at least one instance of the dock in the .yaml parameter file to be taken by the DockDatabase.

Additional information

[SteveMacenski]

Seems like a good improvement to make it fully optional in case you want to not specify dock instances. You are required to load all the dock plugins required - since those must be known by the server for use in the database or via the action request. But, instances could be made fully optional. Note that specifying the full dock information at request-time is really meant more as a mechanism for testing and if you're setting up a facility with new docks live. If you have a predefined set of docks, you should really load them either via the Nav2 configuration file or linking the dock's list file as laid out in the tutorial https://docs.nav2.org/tutorials/docs/using_docking.html

Open to submitting a PR?

[jcarlosgm30]

@SteveMacenski Yes, I agree. This functionality would provide flexibility to set up a facility with new docks live, and even facilitate integration with other higher-level systems that manage the storage of robot data via database or other mechanisms.

Open to submitting a PR?

I am open to submitting it. Just to discuss the approach, I think the functionality could be achieved by logging a good warning message here and returning true. And also, adding some log messages elsewhere to be more informative to the user.

What do you think? Do you think there are any additional requirements or considerations in order to add this feature?

[SteveMacenski]

I think the functionality could be achieved by logging a good warning message here and returning true

I agree, I think also having the reloading service work in either case (https://github.com/ros-navigation/navigation2/blob/main/nav2_docking/opennav_docking/src/dock_database.cpp#L45) would be a positive improvement in this part of the code as well (I'm not 100% sure what I was thinking there). But, if that function always returns true when validly configured (i.e. no corrupted yaml files) and the dock plugins has to have at least 1 valid option, your change would virtually eliminate that issue.

How I get around this now is just having a dummy_dock that we ignore, but it would be great not to require such a thing. I'm of 2 minds if I should ask you to remove that from our YAML file. I think its useful to have an example nonetheless. Perhaps you could comment it out so its just there for illustration purposes in your PR?

[jcarlosgm30]

I agree, I think also having the reloading service work in either case (https://github.com/ros-navigation/navigation2/blob/main/nav2_docking/opennav_docking/src/dock_database.cpp#L45) would be a positive improvement in this part of the code as well (I'm not 100% sure what I was thinking there). But, if that function always returns true when validly configured (i.e. no corrupted yaml files) and the dock plugins has to have at least 1 valid option, your change would virtually eliminate that issue.

I agree. I think the reloading service should be available as long as there is at least 1 valid dock plugin

How I get around this now is just having a dummy_dock that we ignore, but it would be great not to require such a thing. I'm of 2 minds if I should ask you to remove that from our YAML file. I think its useful to have an example nonetheless. Perhaps you could comment it out so its just there for illustration purposes in your PR?

I think it is important to keep it at least as an example to make the configuration easier for users. In the PR below, I have commented it out.

I have opened a PR-4442 with these changes. We can further discuss changes in it if that's okay with you.