search

ros-navigation / navigation2

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

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

Closed jcarlosgm30 closed 2 weeks ago

jcarlosgm30 commented 3 weeks ago

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:

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 2 weeks ago

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.