[behavior_tree] support both ports and blackboard

[facontidavide]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4059
Primary OS tested on	Ubuntu 22.04
Robotic platform tested on	None
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

• cosmetic: use haltTree() more consistently.

• Blackboard::get is marked [[nodiscard]] in BT.CPP 4.x. To prevent future warnings, I added EXPECT_TRUE in the test where the compiler would complain.

• Simplified deconflictPortAndParamFrame. T2 will always be a const BT::TreeNode*, so why writing more verbose code?

• Accessing directly the blackboard instead of using inputPort is an anti-pattern. getInputPortOrBlackboard is a temporary workaround that doesn't change that radically. A good solution must be discussed more broadly

Ports are defined for few BT nodes. If the port is defined, we will use that, otherwise we fallback to reading the blackboard, as we are currently doing.

Description of documentation updates required from your changes

Comments added in the modified/added functions

---

Future work that may be required in bullet points

None of this change should affect the current behavior of the existing code.
In the future, we shoulkd consider how to avoid accessing the blackboard directly.

For Maintainers:

• Check that any new parameters added are updated in navigation.ros.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[SteveMacenski]

Generally seems good to me. These ports need to be added to navigation.ros.org for each of the BT nodes that were added & to the migration guide explaining that these now have the option to take in an input port for goal/goals

[facontidavide]

@SteveMacenski I added a small utility to generate automatically the nav2_tree_node.xml file (as I originally intended).

See this commit, that also automates the creation of a list of plugins.

The auto-generated XML is radically different. See attachment.

nav2_tree_nodes.zip

[SteveMacenski]

@SteveMacenski I added a small utility to generate automatically the nav2_tree_node.xml file (as I originally intended).

Remove that. PRs should be single feaetures with single implementations. Open another PR if you plan to add in additional features unrelated to this one please. That's how we get megaPRs with never ending review cycles. One thing at a time, if they're not innately coupled. Good process saves us all time and effort 😉

[facontidavide]

Done: #4073

[SteveMacenski]

These ports need to be added to navigation.ros.org for each of the BT nodes that were added & to the migration guide explaining that these now have the option to take in an input port for goal/goals

[facontidavide]

ok, I will check what to change in navigation.ros.org

[SteveMacenski]

How's this going? I think its just peanuts and this can be merged :-). You did the hard stuff already

[facontidavide]

Added the fixes as discussed. Ready to merge?

[SteveMacenski]

LGTM on this code, but missing the documentation updates

• The BT nodes with a new port need to have their ports described https://navigation.ros.org/configuration/packages/configuring-bt-xml.html
• The migration guide needs to mention that you can do either or now, https://navigation.ros.org/migration/index.html

[facontidavide]

@SteveMacenski I will submit the change to the documentation today.

Heads up! Some of the changes in this PR might be short-lived, once we migrate to BT.CPP 4.6 (to be released soon),

This new syntax is possible (4 different ways to express the default value of a port):

  static PortsList providedPorts()
  {
    return {BT::InputPort<Point2D>("input", "no default value"),
            BT::InputPort<Point2D>("pointA", Point2D{1, 2}, "default value is [1,2]"),
            BT::InputPort<Point2D>("pointB", "{point}", "default value inside blackboard {point}"),
            BT::InputPort<Point2D>("pointC", "5,6", "default value is [5,6]"),
            BT::InputPort<Point2D>("pointD", "{=}", "default value inside blackboard {pointD}")};
  }

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/tests/gtest_ports.cpp#L423-L482
This means that the new code in NAV2 will be:

BT::InputPort<std::vector<geometry_msgs::msg::PoseStamped>>("goals","{=}", "Vector of navigation goals"),
BT::InputPort<geometry_msgs::msg::PoseStamped>("goal", "{=}, "Navigation goal"),

That means: "If I am lazy and don't specify the port in the XML, search in the blackboard using the SAME name of the port".
But this specific PR tries to stay compatible with BT.CPP 3.8, therefore I will postpone this change.

[facontidavide]

Aaaaand... done! @SteveMacenski
ros-navigation/docs.nav2.org#524

Note that I am not adding anything to the migration guide because it is TOO early, in my opinion

1. There will be more changes
2. The changes in this specific PR are backward compatible and no action should be taken.

So, let me postpone that documentation improvement to future PRs!

[SteveMacenski]

So, let me postpone that documentation improvement to future PRs!

Perfectly fine, this is just for the fact that ports so API changed and its easier to keep those always in sync than remember in the back of my mind the places where I know it needs to change later. The migration guide for the larger v4 migration can certainly wait, but mentioning that the blackboard or port can be used for goal/goals is worth mentioning.

once we migrate to BT.CPP 4.6

Whats your stable v4 version? We're not going to be able to be on your bleeding edge version of v4, its too much risk for too large an audience to go there until its had baking time with early adopters

[facontidavide]

Whats your stable v4 version? We're not going to be able to be on your bleeding edge version of v4, its too much risk for too large an audience to go there until its had baking time with early adopters

Technically speaking all the versions are stable... I have unit tests 😄

But from a behavioral point of view (pun not intended), after 4.6 I have no plan to add features. Only bugfixes until Jazzy.

And I will release 4.6 the next week. My proposed changes to Nav2 will depend on that version.
Also, I wan to point out that some of the features in 4.6 are being created specifically for Nav2, to help migration 😉

[SteveMacenski]

This is just pending on a minor docs update if you can do that, we can have this merged in an hour.

And I will release 4.6 the next week. My proposed changes to Nav2 will depend on that version. Also, I wan to point out that some of the features in 4.6 are being created specifically for Nav2, to help migration 😉

I guess my conservative "I don't want to break 100+ companies" mind says not to use 4.6 and instead use 4.5 until 4.6 has been around for at least a quarter and the top ~5 bugs have been reported/fixed. But, I also don't know how many core changes were made in that subversion update. Was it major or minor updates (and/or major to existing features or adding new features that wouldn't effect existing ones)? I'm more open to potential breaks in new features we don't use yet in the core of Nav2 than I would be for potential breaks in core things in Nav2 folks rely on. ROS 2 has not-so-good reputation due to RWM vendor breakages and issues, I want to make sure Nav2 doesn't fall into the same camp.

4.X still gets folks into groot2 😉

[mikeferguson]

"I don't want to break 100+ companies"

Oh come on! That strategy seems to work fine for MoveIt!

[SteveMacenski]

It'd be a shame if something happened to your robot, see..

[facontidavide]

I think you are making some assumptions about "cutting edge" but.cpp that are not correct, and I need to disagree.

I can only agree with you about the importance of stability, but let me repeat myself: the latest changes are specifically created to help migration to Nav2.

Once I have the change log, we can discuss it further.