Exception thrown when multiple nodes have unremapped outputs

[schactus]

When a tree contains multiple nodes with unremapped outputs of different types, behaviortree_cpp will throw an exception when the second node attempts to write to it's unremapped output port. The message looks something like this:

what(): Blackboard entry []: once declared, the type of a port shall not change. Previously declared type [geometry_msgs::Point_<std::allocator<void> >], current type [std_msgs::Bool_<std::allocator<void> >]

In my case, it's preceeded by this suspicious looking line in the print out of what keys and types are on the blackboard:
(geometry_msgs::Point_<std::allocator<void> >)

Note the whitespace at the beginning of that line that to indicates that the blackboard entry with key "" (empty string) has a type geometry_msgs::Point.

[schactus]

This appears to be because the setOutput method of the TreeNode class does not check if the remapped key of a port is actually remapped to a blackboard topic before it attempts to set it. This means that if the output is unremapped, calling setOutput will put that value in the blackboard with an empty string as the key. When the second node does this (if it is attempting to set a different type), it will throw.

[schactus]

I forked the repo and took a pass at a fix in this PR, but that fails a lot of the other unit tests, so I don't think my PR is the right solution. I also don't think I fully understand the intent of the assignDefaultRemapping method.

I considered checking if the remapped output was an empty string and returning early from setOutput if it is, but that wouldn't handle the case where an output is erroneously set to a non-empty string without {} around it. It would fix the issue that we're currently running into though (have to assign dummy blackboard entries to unused node outputs to prevent exceptions). Happy to PR y'all's preferred fix to this if you have one, or to come up with something if you don't.

[facontidavide]

Please provide a motivating example.

I looked at your unit test but it is conceptually wrong.
This is expected instead:

TEST(BlackboardTest, NullOutputRemapping)
{
  auto bb = Blackboard::create();

  NodeConfig config;

  config.blackboard = bb;
  config.input_ports["in_port"] = "{my_input_port}";
  config.output_ports["out_port"] = "";
  bb->set("my_input_port", 11);

  BB_TestNode node("good_one", config);

  // This will throw because setOutput should fail in BB_TestNode::tick()
  ASSERT_ANY_THROW(node.executeTick());
}

But maybe I am missing something. Investigating

[facontidavide]

fixed.
The correct behavior in your case is that an empty output port or an output port that is not pointing to the blackboard must fail.

[schactus]

Thanks for fixing. You're right that the unit test that I wrote was incorrect.

Our use case is that we have some re-usable nodes that have outputs that may or may not be used depending on where they are in the tree. Things like "ExecuteArmGoal" has an output "final_tcp_position" that is sometimes used by subsequent nodes, and sometimes not used. In cases where it isn't used, we've been leaving it's output set to an empty string (so it doesn't point to the blackboard). This fix makes sense and will work for us because we can just choose to ignore failing setOutput calls in nodes where the output may not be remapped.