Fix: Ensure server_timeout Respects Default Values Using getInputPortOrBlackboard()

[alanxuefei]

Basic Info

Info	Details
Ticket(s) this addresses	#4618
Primary OS tested on	Ubuntu 22.04
Robotic platform tested on	Gazebo simulation, Turtlebot3 hardware
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

• Updated bt_action_node, bt_cancel_action_node, and bt_service_node to use getInputPortOrBlackboard() for initializing server_timeout.
• Ensured that the server_timeout parameter now respects the default value from the blackboard if not explicitly set in the BT action node.
• This change addresses the issue where the default_server_timeout was always overridden, leading to more consistent behavior and reducing configuration overhead.

Description of documentation updates required from your changes

• No new parameters were added, so no updates to default configs are necessary.

---

Future work that may be required in bullet points

• Further testing on different robotic platforms to ensure compatibility.
• Potential optimizations in the BT nodes to enhance performance.
• Documentation updates in case of additional related changes in the future.

---

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 are 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]

@alanxuefei, 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]

@alanxuefei, 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]

@alanxuefei isn't this still true?

We'd have the change all the BT nodes not to have defaults in that case then so that getInput would return false when unset to use the global timeout.

[alanxuefei]

@alanxuefei isn't this still true?

We'd have the change all the BT nodes not to have defaults in that case then so that getInput would return false when unset to use the global timeout.

Yes, the statement is correct.

In the default behavior trees (e.g., navigate_to_pose_w_replanning_goal_patience_and_recovery.xml), there is no server_timeout parameter.

After second analysis, the original code actually works:

// Check ROS2 parameter.
server_timeout_ = config().blackboard->template get<std::chrono::milliseconds>("server_timeout");
// Check BT port (if the BT port is unset, server_timeout_ remains unchanged).    
getInput<std::chrono::milliseconds>("server_timeout", server_timeout_); 

However, the new code is more concise and efficient (inline):

template<typename T> inline
bool getInputPortOrBlackboard(
  const BT::TreeNode & bt_node,
  const BT::Blackboard & blackboard,
  const std::string & param_name,
  T & value)
{
  if (bt_node.getInput<T>(param_name, value)) {       //check BT port first
    return true;
  }
  if (blackboard.get<T>(param_name, value)) {           // check ROS2 param
    return true;
  }
  return false;
}

[SteveMacenski]

So this doesn't change the behavior? I agree this is cleaner so happy to merge either way, but what sparked this PR / work if you weren't running into a problem (and do you still have a problem)?

[alanxuefei]

So this doesn't change the behavior? I agree this is cleaner so happy to merge either way, but what sparked this PR / work if you weren't running into a problem (and do you still have a problem)?

We mistakenly thought that once a BT port is declared in a BT node, it must also be included in the XML file. In fact, if the port is not defined in the XML file, getInput will return false without causing any system errors and remain server_timeout unchanged. For server_timeout, we can simply remove it from xml and it will use default value.

[SteveMacenski]

So we're good to merge this? I assume so, just wanting to verify there's nothing else you want to include here

[alanxuefei]

Since it is a base function for each BT node, it's good to double-check its functionality. The tests have passed, and the logic has been verified. I believe it's ready to be merged.