adding timeout for action client initialization

[vinnnyr]

Signed-off-by: Vinny Ruia [email protected]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#3027
Primary OS tested on	N/A*
Robotic platform tested on	N/A*

• I am currently having trouble compiling from main on my system. I started this PR so that I can see if CI can do it.

---

Description of contribution in a few bullet points

Adding timeout to the action server initialization

Description of documentation updates required from your changes

N/A

---

Future work that may be required in bullet points

N/A

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]

@vinnnyr, please properly fill in PR template in the future. @SteveMacenski, use this instead.

• 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

[vinnnyr]

Ok I think I have to rethink this a small bit. The default server_timeout is 20ms, which is not a lot of time to wait by default.

When trying to launch the system:

[lifecycle_manager-3] [INFO] [1655777607.945443640] [lifecycle_manager_navigation]: Activating bt_navigator
[bt_navigator-2] [INFO] [1655777607.945732127] [bt_navigator]: Activating
[bt_navigator-2] [ERROR] [1655777607.968145727] [bt_navigatornavigate_through_poses_rclcpp_node]: "compute_path_through_poses" action server not available after waiting for 20 ms
[bt_navigator-2] [ERROR] [1655777607.976364703] []: Caught exception in callback for transition 13
[bt_navigator-2] [ERROR] [1655777607.976390295] []: Original error: Action server not available
[bt_navigator-2] [WARN] [1655777607.976438741] []: Error occurred while doing error handling.
[bt_navigator-2] [FATAL] [1655777607.976449793] [bt_navigator]: Lifecycle node bt_navigator does not have error state implemented
[lifecycle_manager-3] [ERROR] [1655777607.976858429] [lifecycle_manager_navigation]: Failed to change state for node: bt_navigator
[lifecycle_manager-3] [ERROR] [1655777607.976902588] [lifecycle_manager_navigation]: Failed to bring up all requested nodes. Aborting bringup.

Should I declare a new parameter for this timeout?

[SteveMacenski]

I'm hesitant to have a new timeout since that might confuse people, even if well named - but maybe there's a good name that would be clear so its moot? wait_for_action_server_timeout server_initialization_timeout?

Otherwise, if we can't come up with a good & specific name, I think just hardcoding it at some value would be fine. I'd say 1s just to be nice and round. We have a few like that (only a few) in the stack where it didn't make much sense to parameterize it for various reasons and since they were just initialization operations, it wasn't super important for them to be configurable for 99.8% of users.

[vinnnyr]

I'd say 1s just to be nice and round

I think I agree

[SteveMacenski]

Where is this exception caught? I don't see in the loading process where there's anything catching exceptions so that this wouldn't bring down the process (unless its being caught at the lifecycle node level?).

If it happens in the lifecycle node process for starting up the node, where would it be caught then in steady state? We can specify BTs for use for every navigation request, so we could change trees anything during execution. In that case, there wouldn't be a lifecycle node transition to catch the exception so it wouldn't crash - when a different BT is specified in a navigation request and thus is being constructed

[vinnnyr]

Good point.

At least on startup, the exception is caught by the Parent Lifecycle Node

server_name | [action_server_1] [ERROR] [1655837597.993815093] [action_name_rclcpp_node]: "follow_path" action server not available after waiting for 2000 ms
server_name | [action_server_1] [ERROR] [1655837598.076770031] []: Caught exception in callback for transition 13
server_name | [action_server_1] [ERROR] [1655837598.076800292] []: Original error: Action server not available
server_name | [action_server_1] [WARN] [1655837598.076856566] []: Error occurred while doing error handling.
server_name | [ERROR] [launch_ros.actions.lifecycle_node]: Failed to make transition 'TRANSITION_ACTIVATE' for LifecycleNode '/action'

I can look into how it gets caught in steady state too.

[SteveMacenski]

Prob just need another try/catch somewhere in the chain

[vinnnyr]

hardcoded to a reasonable 1s now

[vinnnyr]

@SteveMacenski Take a look at your convenience. I believe I addressed your concerns, including handling the "switch at runtime" case.

[vinnnyr]

As an aside, I am unsure which tests are actually failing here.
Atleast on my browser / machine, the CircleCI tab for tests seems to be hanging indefinitely
https://app.circleci.com/pipelines/github/ros-planning/navigation2/7517/workflows/12f61cb3-ffeb-414c-8722-b47d4e422afa/jobs/26329/tests

[SteveMacenski]

I mean std::chrono::milliseconds wait_for_server_timeout = std::chrono::milliseconds(1000); works, but we can just do action_client_->wait_for_action_server(1s) if you just include chrono literals.

[SteveMacenski]

The tests came through, it did have 1 failure that is unusual so retriggering, but I don't think its related to this PR. I'll let it spin though and check back in the morning