The first-time failure in the IsBatteryLow condition check

[Tacha-S]

Bug report

Required Info:

• Operating System:
  • Ubuntu 24.04
• ROS2 Version:
  • Jazzy binaries
• Version or commit hash:
  • ros-jazzy-navigation2 1.3.3-1noble.20241115.195529
• DDS implementation:
  • Cyclone DDS

When performing the check with the IsBatteryLow condition for the first time, it almost always results in FAILURE because a subscriber is created and then spin_some() is executed shortly thereafter.

Steps to reproduce issue

Expected behavior

Actual behavior

Additional information

[SteveMacenski]

What is your suggested solution? This seems like a reasonable choice to me and the design pattern we do in multiple BT nodes. I can see how it takes some time to 'warm up' to get the first message since its just starting, but I don't know how actionable a change would be.

If your application is sensitive to that, you could use a service call instead of a topic to obtain the battery information or possibly tick and have a delay action node to tick again to wait for a message on startup. There are probably other solutions too. If this is being performed in a reactive control flow node, then it'll trigger on soon after the first message is actually received, the delay should be negligible proportional to the publication rate of your battery state information.

I'm definitely open for suggestions :-)

[Tacha-S]

Constructing a behavior tree while considering whether the nodes will function correctly on the first run can easily become a source of bugs and make the behavior tree itself difficult to read.

Even if sufficient time is allocated for warmup, subscribers won't be created unless the nodes are executed at least once. Therefore, ensuring sufficient time between launching the node and triggering the behavior tree execution is completely ineffective.

How about calling initialize() once in the constructor?
Alternatively, using wait_for_message() to retrieve the message might be a good idea, but it would likely be rejected as it wouldn't work on distros where this functionality is not implemented.

[SteveMacenski]

Some thoughts before responding to your items:

I'm noticing that the BT Action Nodes and BT Service Nodes construct their service/action clients in the constructor of those nodes. These are special cases since the action names are set by the BT node name.

We could very easily just create all ROS interfaces for BT nodes (like isBatteryLow) in the constructor as well. The only downside of this is that the topic themselves are parameters. The other parameters being dynamically adjustable in initialize() make sense, which can continue to be called in tick() on status checks.

So, we could create the ROS interfaces, with the current input port status, in the constructor and also leave the creation in initialize() when the topic has changed only in tick(). We do that very similarly in the isBatteryLow BT node. I think we'd just want to branch out a new createROSInterfaces() method that is called in both the constructor and initialize, so we don't duplicate code.

I think that would solve the issue and a pattern that could be repeated in all BT nodes that have internal ROS interfaces

---

How about calling initialize() once in the constructor?

We likely cannot, since they get parameters that need to be gotten in the tick due to problems discussed in #3988 -- this is actually how we used to handle it. Perhaps @maksymdidukh can also chime in his thoughts. But also see above.

Alternatively, using wait_for_message() to retrieve the message might be a good idea, but it would likely be rejected as it wouldn't work on distros where this functionality is not implemented.

We don't need to backport everywhere if we cannot, that could be a viable solution. Waiting for message would need to block for no more than the tick rate, the same way we do for services and actions that return the RUNNING state while we're waiting for a service/action timeout and respecting the tick rate for reactive behavior (example).

Though, waiting for a message for, lets say 250ms, every time we retick a tree would be blocking all other productive action frequently. I'm not sure exactly this is the best way.

Constructing a behavior tree while considering whether the nodes will function correctly on the first run can easily become a source of bugs and make the behavior tree itself difficult to read.

This could just pedantic due to the nature of the isBatteryLow condition and the related logic around autonomous docking -- but I don't see that much of a problem. If you have a BT branch that is under a reactive control node, it'll be checked frequently for state changes. Waiting a couple of ticks to get the message to realize that its low seems pretty minimal. With other nodes, I could see this being higher-impact, which is also why its interesting to discuss this since the pattern would change for many BT nodes if we changed this one.

[SteveMacenski]

@Tacha-S thoughts / updates?

[Tacha-S]

While studying the tick rate of behavior trees, I forgot to reply.
Upon reconsideration, the following suggestion seems very promising.

So, we could create the ROS interfaces, with the current input port status, in the constructor and also leave the creation in initialize() when the topic has changed only in tick(). We do that very similarly in the isBatteryLow BT node. I think we'd just want to branch out a new createROSInterfaces() method that is called in both the constructor and initialize, so we don't duplicate code.

[SteveMacenski]

Is this something you'd be open to contributing? 😄

[Tacha-S]

Of course, I will create a PR with the proposed changes.