Create subscriber on constructor

[Tacha-S]

---

Basic Info

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

---

Description of contribution in a few bullet points

• I modified it to create a subscriber during initialization while maintaining the functionality to switch topics based on the input port status.

Description of documentation updates required from your changes

• There are no changes to the inputs, outputs, parameters, or overall behavior, so there is no need to update the documentation.

Description of how this change was tested

• Create a tree with IsBatteryLow as the top-level condition and invoke the tree when the battery level is low.

---

Future work that may be required in bullet points

For Maintainers:

• Check that any new parameters added are updated in docs.nav2.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]

@Tacha-S, 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 approve of these changes, but I think this should be made for all BT nodes so that this behavior is consistent across the board. Plus, I'm sure you'll use other BT nodes and want this done for them anyway, so may as well save yourself the headache and batch it out now :-)

[Tacha-S]

Understood. I will check and fix the other BT nodes as well.

[mergify]

@Tacha-S, 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 not entirely sure if this PR is done / needing review, can you let me know?

I'm not seeing that all the nodes updated have the initialize implementation so we create on construction to resolve your issue and set properly if the port is updated like in IsBatteryLow.

[Tacha-S]

In the node with Initialize(), there were no other nodes updating topics, actions, or services except for IsBatteryLow, so it was unclear what was expected.
As a result, I ended up adding them to all the components that create topics, actions, and services in the constructor, but I don’t think this is what was intended.

[SteveMacenski]

Got it, I'm sorry that I was not clear.

The pattern that is battery low established was good; checking for changes and creating the updated subscription if/when the topic changed. For BT nodes that derive from the BTServiceNode and BTActionNode, we can leave these alone because they don't have their topics as a port, so they don't change at runtime ever. But, we need to have them dynamic for ones were the topic itself is a port, like the isBatteryLow since it has:

getInput("battery_topic", battery_topic_new);

The other updates you made to things like SmootherSelector, ProgressCheckerSelector, etc all also have their own input port topics that need to be checked not just on construction, but when running an initialization when ticking from IDLE. This is because these ports may be blackboard variables that are not known at construction time, not hard-coded values in the XML which is known at construction time.

So, what I was asking was to create that same pattern you made for isBatteryLow for all BT nodes with subscriptions/publishers with topic input ports:

MyNode::MyNode(...)
{
  createROSInterfaces()
}

MyNode::tick()
{
  if (!BT::isStatusActive(status())) {
    initialize();
  }
 
  // ... continues
}

Whereas initialize calls createROSInterfaces and createROSInterfaces checks if the topic has changed or if the subscriber isn't yet initialized for creating the new subscription.

Does that make sense?

[Tacha-S]

Thank you for the explanation, I understand.
The current scope of applying initialize is still insufficient,
so for cases where the topic or service name is specified through an input port, I should introduce initialize and createROSInterfaces to enable dynamic updates.

Once the changes are complete, I’ll reopen and reply again.

[SteveMacenski]

Correct! I went through the files and you did identify all of the files that would need to be updated, so you don't need to go through every file again. The ones that you touched in this PR are all of them :-)

Thanks for the help!

[mergify]

@Tacha-S, 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).

[Tacha-S]

I have made the corrections as you pointed out.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
...ree/include/nav2_behavior_tree/bt_service_node.hpp	84.72% <100.00%> (+1.64%)	⬆️
...r_tree/plugins/action/controller_selector_node.hpp	100.00% <ø> (ø)
...tree/plugins/action/goal_checker_selector_node.hpp	100.00% <ø> (ø)
...vior_tree/plugins/action/planner_selector_node.hpp	100.00% <ø> (ø)
.../plugins/action/progress_checker_selector_node.hpp	100.00% <ø> (ø)
...ior_tree/plugins/action/smoother_selector_node.hpp	100.00% <ø> (ø)
...lugins/condition/is_battery_charging_condition.hpp	100.00% <ø> (ø)
...ree/plugins/condition/is_battery_low_condition.hpp	100.00% <ø> (ø)
...avior_tree/plugins/decorator/goal_updater_node.hpp	100.00% <ø> (ø)
...r_tree/plugins/action/controller_selector_node.cpp	100.00% <100.00%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SteveMacenski]

Small changes and LGTM!

[SteveMacenski]

Shouldn't we do initialize() here because initialize gets the input ports

[SteveMacenski]

In general, should we change them all from createROSInterfaces() in the constructor to initalize() so that if there are other non-ros-interface making initialization steps that might be important for the interface definitions, we perform them here too?

[Tacha-S]

I think consistently calling initialize() in the constructor is a good idea, and I agree with that approach.
However, limiting createROSInterfaces() to only within initialize() and not calling either createROSInterfaces() or initialize() from the constructor would not resolve the issue mentioned in the issue, where the behavior during the first execution becomes undefined. Therefore, I would like to avoid this.

[SteveMacenski]

I think consistently calling initialize() in the constructor is a good idea, and I agree with that approach.

OK! I see you updated that in this case on battery low, but not in the other cases. Can you update all the BT nodes for that change so they're all consistent? I think with that, this is ready to merge.

However, limiting createROSInterfaces() to only within initialize() and not calling either createROSInterfaces() or initialize() from the constructor would not resolve the issue mentioned in the issue, where the behavior during the first execution becomes undefined. Therefore, I would like to avoid this.

I'm saying that we should call initialize() in the constructor and also in the pattern below we've added to all of the tick()'s. Shouldn't that handle the situation?

  if (!BT::isStatusActive(status())) {
    initialize();
  }

I'm basically just saying I want you to replace createROSInterfaces() in the constructor with initialize(), who itself calls createROSInterfaces(). That way, if there are additional steps to initialization beyond just creating ROS interfaces, we call all of that stuff in the constructor as well as on the first tick() so the behavior is totally consistent between them.

Basically, use initialize() in both locations and createROSInterfaces() should only ever be called by initialize() and no one else as a general design pattern across all nodes.

[SteveMacenski]

I approve! Is there anything left here to change or anything else you plan to do before I merge?

[Tacha-S]

This is all for now. Thank you.

[SteveMacenski]

Do you mind rebasing or pulling in main? You have a couple of CI tests that are failing which seem... odd. We recently fixed some things in these areas and I want to make sure that your PR doesn't somehow cause breakages (which I highly doubt it does).

[Tacha-S]

I have merged the latest changes, and the CI tests have passed as well.

[SteveMacenski]

Merged, sorry for the delay, I was on vacation then got sick!