error_code_names parameter for BT Navigator plugins

[RBT22]

Feature request

Feature description

It would be nice to have the option to give different error_code_names parameters to BT Navigator plugins. Currently you can pass only one list to the parent node and that is used in every navigator. This new customization option would help to handle structually more different navigators.

Implementation considerations

In the config file the error_code_name parameter for the navigator could be placed near the plugin:

bt_navigator:
  ros__parameters:
    navigators: ["navigate_to_pose", "navigate_through_poses"]
    navigate_to_pose:
      plugin: "nav2_bt_navigator/NavigateToPoseNavigator"
      error_code_names:
        - compute_path_error_code
        - follow_path_error_code

We can reach the parent node's parameters in the BtActionServer implementation, so my proposal would be to use these new values as default and fallback on the parent node's values, and if those are not set also, we could use the default.

This way the parameter can be declared as "<navigator_name>.error_code_names". One question is how to get the first part of this string in the BtActionServer. We have action_name, which is "task/<navigator_name>" so technically we could take the second part of it, but maybe there is a better way.

[SteveMacenski]

Is there a reason not to include them all? If there are some of the error code IDs which are not used in a particular navigation task, that wouldn't cause any issues as they'd continue to be zeros https://github.com/ros-planning/navigation2/blob/01b84136493a47c2fe6ffa9d0b1132cbf132b189/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#L272-L291

Also, for a given NavigateToPose navigation task, there could be many different BTs that could be sent to use for the task, each of which could have their own unique cross section of error codes. I don't think specifying on a per-navigator basis makes any sense. It would either need to be global or on a per-action basis and needs to be somehow parsed from the BT itself (perhaps metadata?).

Though, I don't think that level of optimization benefits anyone really given the overhead of having N error code IDs checked on the blackboard is eq. to an unordered map O(1) lookup, there's not much computational reason to do so.

So I'd ask why you want this feature and what it accomplishes for you? Though, we may desire to change the ERROR message to a DEBUG message. That was a point of contention when we were reviewing this work that I could go either way on. You bring up a good point here that perhaps it should not be an error message since different navigators might have different cross sections of error codes specified. That could be an actionable outcome!

[RBT22]

Yeah if we can change the error message to debug, that is also fine by me. :D

Although one reason for using specialized parameters could be exactly the ability to keep the ERROR level messages. Logging the missing value as an error could be a good feature. Using one big list for the parent and logging the differences all the time might be confusing and not that helpful for debugging, but it's not that big of a deal.

[SteveMacenski]

Although one reason for using specialized parameters could be exactly the ability to keep the ERROR level messages.

Definitely get that and I'd generally agree. I think thought it would have to be either on a per-server basis for all or on a per-tree basis so we know exactly what to do expect from each BT since the different navigator types can take in N BTs that may have different cross sections of error codes.

DEBUG seems like the best way to me! Maybe also INFO log on initialization about which are being used to help debug problems if not all are correctly specified since the ERROR warnings aren't being shown anymore so there's still some tracability from logs. What do you think?

Would you be open to submitting that PR? Always more happy to have others share in the open-source glory 😆

[RBT22]

I can agree with you after I gave some thoughts to it and read the code again. I will definitely open a PR about it this week.

[SteveMacenski]

#3642 to be probably merged this week, closing ticket as complete