BehaviorTree issue when SubTrees are used

[facontidavide]

Bug report

This problem was originally reported here by @Rid0n

BehaviorTree/BehaviorTree.CPP#563

The issue is here.

https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#LL139C1-L144C97

The problem is that each subree has its own blackboard, unless the Subtree was created with "__shared_blackboard=true".

The SubtreePlus / _autoremap works differently and, instead of using the blackboard of the parent, it remap the single ports.

But this means that Subtrees can not see the entries node, server_timeout and bt_loop_duration.

There are two solutions:

1. The correct one (RECOMMENDED). Stop doing that 😄 . You are abusing the blackboard and expect it to do things that is not supposed to do. The solution is to follow the pattern suggested in this tutorial. There is a more concrete example here: https://github.com/BehaviorTree/BehaviorTree.ROS2

2. The quick and dirty solution:

Change this: https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#L203

For

tree_ = bt_->createTreeFromFile(filename);
for(auto& blackboard: tree.blackboard_stack)
{
  blackboard->set<rclcpp::Node::SharedPtr>("node", client_node_);  
  blackboard->set<std::chrono::milliseconds>("server_timeout", default_server_timeout_);  
  blackboard->set<std::chrono::milliseconds>("bt_loop_duration", bt_loop_duration_);   
}

[facontidavide]

My goal is to have a preliminary discussion on this thread, and submit a PR later, once we agreed how to proceed.

[SteveMacenski]

That suggestion looks more or less the same to me, just moved around in a way that doesn't impact end-use. I've got no problem with it.

I don't see how this is abusing the blackboard though. The blackboard is to store information that needs to be shared with other BT nodes in the context of execution. Throwing in the shared resources like the goal pose or tools needed to obtain results seems totally in line with its intent.

But that's perhaps here nor there.

[facontidavide]

I don't see how this is abusing the blackboard though.

Let me explain 😃

The recommended abstraction to use is not the blackboard, but the input/output ports.
It is at that level that things are tested, including:

• type checking,
• conversions from/to strings
• manual or automatic remapping.

When using the blackboard directly, you are on your own. Things may work or not (as in this case, they don't), I don't offer any guaranty that things will work as you may expect they should.

I am very careful in my documentation/tutorials about not using the blackboard directly for that reasons.

But Nav2 created a precedent, and I find myself fixing issues like these that are derived from using the "wrong" API.

Said that, I will propose a PR that finally get rid of this!

[SteveMacenski]

When nodes are passing data to each other or params specific to a particular BT node, yes I agree. And we do that broadly.

However you must be able to insert information onto the blackboard when starting a BT because you need to be able to communicate to the BT template the request specifics (e.g. the goal pose requested or other task-specific request parameters). These are read into the blackboard and then used as input ports to the appropriate BT nodes that need them (e.g. goal={goal_pose}, where goal_pose was inserted onto the blackboard at startup). That's a straight up requirement and I don't know how someone could argue against that need. If you have a BT doing a task template (e.g. go to pose, pick up a box, whatever), you need to give it the request specific inputs somehow and that seems to me to be the role of the blackboard.

Separately, these items you see added are global parameters that are needed by virtually all BT nodes as a shared resource. It makes little practical sense to have every BT node have an input port of a node or timeouts -- those XML files would just get massive and it would be silly to see node={node} server_timeout={server_timeout} ... written over and over again on every line. I see the argument for why when people use it, it should be an input port which points to the blackboard variable. But given every single node needs this same shared resource, its a shorthand in Nav2's internal nodes that we just grab it from the blackboard because we know its there in all the BT factories. I don't advocate that as a general solution, only because I control the BT factory and the BT node source files I can do that. I don't think your issue is how we're putting shared resources onto the blackboard (as the previous paragraph points out, that is required for task templates), I think your issue is how the BT nodes themselves are grabbing them directly instead of using the port/blackboard grabbing with {}. I can appreciate that as being a best practice, however subtrees literally didn't exist when much of that was written. And certainly we can adjust.

Do port defaults allow you to point to a blackboard variable? That could be a relatively easy solution so that every BT node could have the timeouts / node added as an input port but not have to specify over and over at the XML level

But Nav2 created a precedent, and I find myself fixing issues like these that are derived from using the "wrong" API.

I'm sorry that you're frustrated by this. However you have a community of folks that point out potential problems, you may well served by asking them if they'd like to contribute solutions once you've identified the problem as a way to make best use of your time / limited resources. I've been quite successful with looking into problems to the degree which I find the solution and throw the ball back into their court to implement the solution. You don't need to do it all alone!

[SteveMacenski]

Making the change

tree_ = bt_->createTreeFromFile(filename);
for(auto& blackboard: tree.blackboard_stack)
{
  blackboard->set<rclcpp::Node::SharedPtr>("node", client_node_);  
  blackboard->set<std::chrono::milliseconds>("server_timeout", default_server_timeout_);  
  blackboard->set<std::chrono::milliseconds>("bt_loop_duration", bt_loop_duration_);   
}

Is an easy fix that I'm happy to make -- is there something else you wanted to change as well?

[SteveMacenski]

@facontidavide how should we proceed? I don't want to leave this one hanging open if we have a solution. If that's the only change you want me to make, I'm happy to do it, but I need to know 😄

[SteveMacenski]

@facontidavide pinging -- I'm going to execute on the solution above if I don't hear otherwise

[SteveMacenski]

#3911

[facontidavide]

well done! Sorry for the high latency in responding the ping