BT nodes not able to accept parameter values

[papertowel123]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	(add tickets here #1)
Primary OS tested on	Ubuntu
Robotic platform tested on	gazebo simulation
Does this PR contain AI generated software?	No

---

Reasong for change

There are BT nodes that read the input ports from the constructor. This causes issues when we want to pass the variables as input.

Description of contribution in a few bullet points

• move every getInput() usage from constructor to tick() method

Description of documentation updates required from your changes

---

Future work that may be required in bullet points

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

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

This pull request is in conflict. Could you fix it @maksymdidukh?

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

I'm happy to review this in detail once you can actually build it 😉

From a quick glance, you're moving input getting from the constructors to the ticks, which I assume is because you're varying some or all of those inputs (eg non-const)? I'm kind of surprised for many of them that you'd actually change them mid-operation, but who am I to argue.

The only ones I have somewhat substantial beef with are those that are moving the creation of subscriptions in tick. Those should be existing already and its impractical to do what you're doing. Those nodes just won't work if you're recreating subscriptions every 100 hz tick.

[papertowel123]

I'm happy to review this in detail once you can actually build it 😉

From a quick glance, you're moving input getting from the constructors to the ticks, which I assume is because you're varying some or all of those inputs (eg non-const)? I'm kind of surprised for many of them that you'd actually change them mid-operation, but who am I to argue.

The only ones I have somewhat substantial beef with are those that are moving the creation of subscriptions in tick. Those should be existing already and its impractical to do what you're doing. Those nodes just won't work if you're recreating subscriptions every 100 hz tick.

sure! I'll remove PR from the draft when it's ready for review :)

[SteveMacenski]

OK. Much of your linting issues (though I'm sure you have a few of your own) will be fixed by rebasing or pulling in main. Some other things large-number of linting problems were fixed recently.

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

This pull request is in conflict. Could you fix it @maksymdidukh?

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@maksymdidukh, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[SteveMacenski]

I'm on the fence about the action / service nodes.

On one hand it would be nice to actually resolve some subtree issues if we didn't get those ports in the constructor. It also then makes it no different from the other BT nodes.

On the other hand, it delays the construction of the action client to give very little time for discovery to occur before calling the action and pushing the wait_for_action_server into the BT's execution context -- which seems like a poor choice on both counts especially since we know users have action servers on other computers in the network pretty regularly.

I'm leaning towards asking you to revert the changes in those two files, but I could be convinced if someone has a strong outside reason why we should enact that change given the trade off. I'd say for now to revert those two files so that we can merge in all the rest. Then I suppose we could have a follow up PR or discussion about it if someone disagrees. We don't have to have the controversial stuff happen right now to get 80% of the benefit immediately.

[papertowel123]

I'm on the fence about the action / service nodes.

On one hand it would be nice to actually resolve some subtree issues if we didn't get those ports in the constructor. It also then makes it no different from the other BT nodes.

On the other hand, it delays the construction of the action client to give very little time for discovery to occur before calling the action and pushing the wait_for_action_server into the BT's execution context -- which seems like a poor choice on both counts especially since we know users have action servers on other computers in the network pretty regularly.

I'm leaning towards asking you to revert the changes in those two files, but I could be convinced if someone has a strong outside reason why we should enact that change given the trade off. I'd say for now to revert those two files so that we can merge in all the rest. Then I suppose we could have a follow up PR or discussion about it if someone disagrees. We don't have to have the controversial stuff happen right now to get 80% of the benefit immediately.

I'm on the fence about the action / service nodes.

On one hand it would be nice to actually resolve some subtree issues if we didn't get those ports in the constructor. It also then makes it no different from the other BT nodes.

On the other hand, it delays the construction of the action client to give very little time for discovery to occur before calling the action and pushing the wait_for_action_server into the BT's execution context -- which seems like a poor choice on both counts especially since we know users have action servers on other computers in the network pretty regularly.

I'm leaning towards asking you to revert the changes in those two files, but I could be convinced if someone has a strong outside reason why we should enact that change given the trade off. I'd say for now to revert those two files so that we can merge in all the rest. Then I suppose we could have a follow up PR or discussion about it if someone disagrees. We don't have to have the controversial stuff happen right now to get 80% of the benefit immediately.

reverted)

[papertowel123]

When this PR is merged, will we be able to make a backport to iron?

[SteveMacenski]

@maksymdidukh this change breaks ABI so unfortunately it cannot be backported to any existing distributions. I looked into some policy requirements for ABI stability and its pretty clear cut that this could cause issue if you attempted to recompile the C++ files from the old installed headers since the initialize function is new. I tried to think of a way to make this work, but I think this falls on the wrong side of stability policies to be backport-able.

However, Iron is not very different from this, so you should be able to cherry-pick the merge commit here and apply to Iron on your own fork (for the next ~5 months before Jazzy) and you'll get all this included in Jazzy and newer! Or, you could even just use main from Nav2 with Iron and that should also work.

@jwallace42 any additional comments?

Edit: you have a test failure:

/opt/overlay_ws/src/navigation2/nav2_behavior_tree/test/plugins/action/test_drive_on_heading_cancel_node.cpp:153
Expected equality of these values:
  tree_->rootNode()->status()
    Which is: FAILURE
  BT::NodeStatus::SUCCESS
    Which is: SUCCESS/opt/overlay_ws/src/navigation2/nav2_behavior_tree/test/plugins/action/test_drive_on_heading_cancel_node.cpp:156
Expected equality of these values:
  action_server_->isGoalCancelled()
    Which is: false
  true

[jwallace42]

LGTM!

[papertowel123]

Edit: you have a test failure:

Cannot reproduce test failure locally, I'll rerun the tests, maybe it was an accident

[SteveMacenski]

Thanks for the contribution! This is a really nice one

[redvinaa]

A bit late to the party, but I think there's some problems with this solution.
IDLE (aka first tick since last halted) is not the same as not initialized (aka first tick since construction). And if we load the blackboard from the navigator at the start of every new task (which can happen), it could cause problems. For example, if one wants to create a task that includes in the goal a DriveOnHeading distance (that changes per navigation task), the value won't change properly, in fact it will always have the first value it was created with.

I think the proper solution would be to pass the IDLE state to the on_tick callback so every implementation can check for themselves. Though this probably breaks ABI even more.

diff --git a/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp b/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp
index 0a89dcfb..40ddfeb9 100644
--- a/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp
+++ b/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp
@@ -194,15 +194,15 @@ public:
   {
     // first step to be done only at the beginning of the Action
     if (status() == BT::NodeStatus::IDLE) {
-      // setting the status to RUNNING to notify the BT Loggers (if any)
-      setStatus(BT::NodeStatus::RUNNING);
-
       // reset the flag to send the goal or not, allowing the user the option to set it in on_tick
       should_send_goal_ = true;
 
       // user defined callback, may modify "should_send_goal_".
       on_tick();
 
+      // setting the status to RUNNING to notify the BT Loggers (if any)
+      setStatus(BT::NodeStatus::RUNNING);
+
       if (!should_send_goal_) {
         return BT::NodeStatus::FAILURE;
       }

[SteveMacenski]

Hi, sorry for the delay, ROSCon, ROS-I, and a short vacation left me away from my desk to think about complicated technical problems :-)

@redvinaa I appreciate the point and agree.

Another option: we could set the initialization to false on exit conditions or halt so that we reinitialize when re-called again. I agree that the initialize vs IDLE is a bit messy and that just doubles down on it. I think it be better to re-do this using the state IDLE rather than an initialization process (ie we call initialize()) to get our values each time we start in the IDLE state. I believe this is also what you suggested.

We have some examples of doing such things https://github.com/ros-navigation/navigation2/blob/main/nav2_behavior_tree/plugins/decorator/goal_updated_controller.cpp#L35-L45 that we could duplicate here. Is that the right move in conjunction with your suggestion to move the status setting?

[redvinaa]

We have some examples of doing such things https://github.com/ros-navigation/navigation2/blob/main/nav2_behavior_tree/plugins/decorator/goal_updated_controller.cpp#L35-L45 that we could duplicate here. Is that the right move in conjunction with your suggestion to move the status setting?

Yes, this is the same solution as what I suggested. As this is a decorator node, and doesn't inherit from BtActionNode, it does actually get the IDLE state.

Pro for the initialization fix is that it can't break existing nodes. But also it has to be implemented separately in every single bt node. And this quick fix just takes us deeper down into the rabbit hole.

[SteveMacenski]

@redvinaa can you file a ticket about this so we can discuss further? I don't want this to get forgotten in the merged PR body.

But also it has to be implemented separately in every single bt node.

Don't both solutions involve a bit of repetition for every Action BT Node?

I don't think this would break existing nodes either way, both solutions seem backportable to me on the face. I like the use of status() and moving the RUNNING as you suggest.

Is this something you're open to working on?