[Discussion] preparing migration to BehaviorTree.CPP 4.x about navigation2 HOT 17 CLOSED

See here: https://github.com/BehaviorTree/BehaviorTree.ROS2/blob/humble/behaviortree_ros2/include/behaviortree_ros2/plugins.hpp

Problem solved already 😄

from navigation2.

Very stupid question first: does the main compile only in Rolling?

Yes :-). If it happens to compile elsewhere, thats a nice to have, but not a requirement. We move fast.

Some methods in BT.CPP are now [[nodiscard]] (modern c++, yay) . The changes to address that can be merged earlier and are backward compatible with BT.CPP 3.8 : #4060

I'll take a look at that right after this

I see that Nav2 broadly use "hard-coded" access to the blackboard, basically skipping the entire InputPort and OutputPort, Subtree remapping, mechanism (that exists for a reason). I would love to discuss in the next community meeting how to address this bad practice. I will try to write a small article about why and how that can be solved the next week.

I'd say "one thing at a time", but I'd have to see what things you're talking about. I think we use blackboards for almost everything except a few globals like node and a few timeouts which are set as attributes of the entire system and not of specific BT nodes. Certainly you could specify node={node} in every single BT node, but that makes the XML file unwieldy and redundant.

But, open to critique. I would say though that we should table that until after the BT.CPP migration is done just not to muddle things up

I need to improve the automated XML converter to upgrade from version 3.X tyo 4.X here. Hopefully, that should takle care of most of the migration pain for many users.

Nice! We should add a link to that (once done) in the Nav2 migration guide, right at the top, to help expose that clearly.

from navigation2.

Note the thread: #4059 on BT.CPPv4 migration. I'm moving the discussion here, I guess 😉

from navigation2.

By the way: why does Nav2 use plugins so much to implement BehaviorTree nodes?

I don't see a reason. It would be simpler to just link statically the "built-in" Nodes, and let the user add their custom ones as plugins.

Not an issue, I just don't see any value for maintainers or users.

from navigation2.

Roadmap in terms of PRs:

1. First, get #4059 merged. That will fix a lot of errors when trying to migrate
2. I would like to discuss the use of the blackboard instead of Input/Output ports. That is a bad practice that needs to be fixed (I will write a different article about that soon). I can make changes in BT.CPP to change the default behavior when the port is omitted in the XML.
3. The actual PR where the migration to version 4.X is done

from navigation2.

There's value in showing examples out of the box how users would do it.

I can make changes in BT.CPP to change the default behavior when the port is omitted in the XML.

To what? We still need the optional ports like we have now for checking different modes

I would like to discuss the use of the blackboard instead of Input/Output ports. That is a bad practice that needs to be fixed (I will write a different article about that soon).

For example? I think I highlighted why we have some as non-ports. There are certain resources like nodes and timeouts which all BT nodes share that would be incredibly redundant to make a port for every instance and make the XML files unwieldy to modify and read. Those are the only places I believe we directly just use the blackboard

from navigation2.

Initialization parameters

There are certain resources like nodes and timeouts which all BT nodes share that would be incredibly redundant to make a port for every instance

I absolutely agree!!!! This is the reason why there is an entire tutorial to address that pattern:
https://www.behaviortree.dev/docs/tutorial-basics/tutorial_08_additional_args

What I think you should do:

https://github.com/BehaviorTree/BehaviorTree.ROS2/blob/humble/behaviortree_ros2/include/behaviortree_ros2/bt_action_node.hpp#L92-L94

https://github.com/BehaviorTree/BehaviorTree.ROS2/blob/humble/behaviortree_ros2/include/behaviortree_ros2/ros_node_params.hpp#L25-L43

Default / not initialized ports

We still need the optional ports like we have now for checking different modes

Let's pretend that I convinced you that this is an antipatterns (postponed to future interactions). Moving into the solution space.

Example with the "goal" entry.

option A:

config().blackboard->get("goals", current_goals);
config().blackboard->get("goal", current_goal);

option B:

// we are assuming the ports are declared in GoalUpdatedController::providedPorts()
getInput("goals", current_goals);
getInput("goal", current_goal);

The two codes are equivalent only if we write this in the XML:

<GoalUpdated goals="{goals}" goal="{goal}"/>

But this code is tedious to write.

Currently, if the port is omitted in the XML we fallback to either:

• the default value of the port, if it was defined in providedPorts()
• or getInput fails (may or may not be desirable)

Proposed solution

New API:

  static BT::PortsList providedPorts()
  {
     return {
        BT::InputPort<std::vector<geometry_msgs::msg::PoseStamped>>(
          "goals", BlackboardKey("goals"), "Vector of navigation goals"),
        BT::InputPort<geometry_msgs::msg::PoseStamped>(
          "goal", BlackboardKey("goal"), "Navigation goal"),
      };
  }

When using BlackboardKey("foo") , we mean that: if the port is omitted in the XML, assume that the value in the port is:
"{foo}".

The advantage of this approach is that you are exposing your intention in the manifest: your intention is to read the PoseStampedfrom a specific key, unless specified otherwise.

from navigation2.

What I think you should do:

Honestly though, how is that different than the blackboard? It still assumes that we're statically typing in the code the path to the content. Whether its my_timeout = ros_obj.timeout or my_timeout = blackboard->get("timeout")

config().blackboard->get("goal", current_goal);
getInput("goal", current_goal);

The problem is that we don't necessarily know what the string value of goal or goals are. The BT navigator can handle plugin-based navigators with their own Action APIs. NavigateToPose uses goal, NavigateThroughPoses uses goals, CoverageNavigator doesn't even contain a goal pose(s) and instead injects field into the BT system. Moreover, there's no statement that the string goal or goals have to be used, they can be set by their blackboard_id parameters so that you can see them dynamically dependent on your application. Though, right now, we mostly use it for the user to tell us what their goal/goals ids are for the use in generating action feedback to obtain the goal(s), which thus must be searchable on the blackboard from the BT's parent object. Example.

BlackboardKey("foo")

We don't know the blackboard key at compile time. See https://navigation.ros.org/configuration/packages/configuring-bt-navigator.html

But overall, we have a bunch of things going on at once that are affecting the BT system. Lets not address this until we have other things squared away. I only have so much mental bandwidth

from navigation2.

Honestly though, how is that different than the blackboard?

YES. The difference is that:

• this pattern "pollutes" the blackboard with names that are not visible in the manifest, creating potential name collisions and errors that are hard to debug.
• it creates a problem when using Subtree (we discussed that in the past).

I have only arguments against your approach and I don't see any advantage, excluding "I can technically do it, so I will do it".

We don't know the blackboard key at compile time.

You are literally compiling your code with the hardcoded name "goal" here. I don't understand what you mean

config().blackboard->get("goal", current_goal);

About this code you mentioned:

blackboard->get<nav_msgs::msg::Path>(path_blackboard_id_, current_path);

This is a different pattern and I am not going to say anything about it ... yet!
Let's just fix what we know how to fix. But I have soooo many questions 😄

from navigation2.

One (or two) things at a time my friend 😉

from navigation2.

Actually lets stop discussing this.

Those "best practice" changes are NOT required to migrate to BT.CPP 4, if I don't mistake, therefore I will refocus the discussion here to the bare minimum to upgrade the code, and nothing else.

from navigation2.

Initialization parameters

There are certain resources like nodes and timeouts which all BT nodes share that would be incredibly redundant to make a port for every instance

I absolutely agree!!!! This is the reason why there is an entire tutorial to address that pattern: https://www.behaviortree.dev/docs/tutorial-basics/tutorial_08_additional_args

What I think you should do:

https://github.com/BehaviorTree/BehaviorTree.ROS2/blob/humble/behaviortree_ros2/include/behaviortree_ros2/bt_action_node.hpp#L92-L94

https://github.com/BehaviorTree/BehaviorTree.ROS2/blob/humble/behaviortree_ros2/include/behaviortree_ros2/ros_node_params.hpp#L25-L43

Can you have nodes with custom constructors be exported/imported as plugins? At least as far as I can see, I haven't found a good way of passing these additional arguments through the registerFromPlugin() function

from navigation2.

@facontidavide now that its in, want to complain to me about my use of blackboards or other things you take issue with that we could address? 😉

from navigation2.

@facontidavide Oh crap, also please put in an entry in the migration guide https://navigation.ros.org/migration/Iron.html

I think this should be a little lengthy.

• We moved to V4
• Some key features of V4 why this is useful
• Mention the migration scripts for the BTs
• Link to V4 documentation to learn more

This is a key change, so right up at top of that page is where it belongs 👑

I think this is also worth writing up a Discourse post about this migration too for Nav2. (both for users to know, a chance for you to take it up, and a call for supporters for the library)

from navigation2.

I will work during the weekend on the migration guide.

Also, I prepared this article: https://www.behaviortree.dev/docs/guides/ports_vs_blackboard

This explains the drawback of accessing the blackboard directly and why it should be avoidable, if possible.
Anyway... if you don't want to, nothing changes from my point of view!

I will let you know once the migration guide is ready

from navigation2.

Can you point out specifically where you think we're using it wrong? I can review them with that in mind. I think there were a couple of places we wanted to directly access the BT's blackboard externally (and thus required) or we populate it with global values in our BT Action factory that never change. I think we should only be doing that with things that are either populated or grabbed externally -- but I'm not opposed to reconsidering that setup. I generally try to be a "best practices" example for the libraries we use.

from navigation2.

Migration is complete. Happy to continue discussion @facontidavide on further improvements for BT.CPP-land. Either continue here and we can re-open for a file a new ticket (or PR). Closing this to get the completed BT.CPPv4 migration off of the issue queue now that its complete for the scope of this ticket.

from navigation2.