changing resetStatus to public

[SteveMacenski]

Needed for unit test coverage to reset the root nodes. I can appreciate that this is to prevent users from doing unwise things, but I think at least a reset option is a necessary public method even if setStatus is not.

At the absolute least, it needs to be a protected member so I can access it from a shim node for testing - that would be fine as well!

[SteveMacenski]

@facontidavide let me know if you have thoughts (I'm sure you will 😉)

[facontidavide]

If you want to reset the root node, why not using Tree::haltTree.

https://github.com/BehaviorTree/BehaviorTree.CPP/blob/master/include/behaviortree_cpp/bt_factory.h#L127

The name is not that intuitive, but it will do exactly what you want to do.

[SteveMacenski]

Because there is no tree. I'm trying to write unit tests of nodes. For example: https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/test/plugins/decorator/test_single_trigger_node.cpp

[facontidavide]

About the PR, if you make it protected, I will accept it, since it is protected in BT.CPP 4.X

I know it is not the subject of this PR, but I find this very confusing

https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/plugins/decorator/single_trigger_node.cpp#L31

It is hard for me to see how this has ANY effect in practice. The parent of SingleTrigger will almost certainly set it to IDLE almost immediately, making the entire logic pointless.

The only place where it MAY make sense if in a ReactiveSequence, but you are basically converting it into a regular Sequence.
I don't want to waste your time, but can you show me a simple use case where SingleTrigger does ... something?

[SteveMacenski]

On the PR: done!

On otherwise: In case you want to do something like "no replanning" so you place the planner BT node under this "only once" so that its skipped for all over traversals of the tree during a particular run while its parent node is RUNNING. There are potentially several things like this that for a given task you need to insert a task somewhere in the middle of a BT for its logical use, but it should only do so one time within that particular context.

I agree it has some limited use due to the IDLE reset of the parent, but for the example below, if we replaced the Rate Controller (orange) with this node, then it would trigger one time only for planning. The IDLE reset actually works in our favor because the parent of that node is always RUNNING except when there's a system level failure exiting the "navigation" side of the tree for the "recovery" side of the tree. Then once we come back, the reset to IDLE makes it so that we retrigger planning 1 time again to continue the task in the new state.

So basically, its mostly useful on the context that its parent is the main control node of the branch or BT. So if it leaves that context, it'll retrigger once, but not repeatedly within the "normal" bounds of operation of that particular branch. Might also be worth noting the custom control flow nodes we use.

[facontidavide]

so you place the planner BT node under this "only once" so that its skipped for all over traversals of the tree during a particular run while its parent node is RUNNING.

I am confused... the most basic nodes, Sequence or Fallback, do that by default.

Now, what I understand is that probably you are trying to deal with a different logic introduced by your custom Control Nodes which don't follow the same rules as the "official ones" in BT.CPP.

In your example, you should just replace the PipelineSequence (parent of RateController) with a regular Sequence and that's it (this is my understanding). In that way you ComputePathToPose once, and never tick that again while FollowPath is RUNNING

But probably I am missing something, so... I guess you know what you are doing!

[SteveMacenski]

But probably I am missing something, so... I guess you know what you are doing!

Perhaps, but I also didn't write the PipelineSequence node but I did have a section of the website written up explaining it https://navigation.ros.org/behavior_trees/overview/nav2_specific_nodes.html#control-pipelinesequence

It might be that there was a different / better way to do it, but its been working for so long I haven't had a reason to argue with it. I think the same could be useful with default control nodes, but certainly this is a quirk that is most interesting to us due to it.

In your example, you should just replace the PipelineSequence (parent of RateController) with a regular Sequence and that's it (this is my understanding). In that way you ComputePathToPose once, and never tick that again while FollowPath is RUNNING

True. But its still a general primitive that has utility. There were some users asking for it, but I don't recall the exact context at this moment in their applications.

---

@facontidavide can you do a 3.8 release so we can get this into binary format & I can merge the test fix into Nav2?

[SteveMacenski]

@facontidavide can you cut a release for 3.8.4 and release to bloom so these changes can be reflected?

Its the last thing to get our CI green https://app.circleci.com/pipelines/github/ros-planning/navigation2/9624/workflows/32358c53-3f64-44a8-a92e-75bdf30b0547/jobs/30813/tests