Retrying configure for planner/controller is not possible

[bektaskemal]

Bug report

Required Info:

• Operating System: Ubuntu 24.04
• ROS2 Version: Jazzy
• Version or commit hash: main
• DDS implementation: CycloneDDS

Steps to reproduce issue

- Have a planner that will fail on configure(wrong config, params etc.).
- Call lifecycle transition to configure for planner server. It will fail since planner will fail.
- Call lifecycle transition to configure again.

Expected behavior

• The transition should succeed if you fixed the issue(config etc.) or transition should fail again.

Actual behavior

• Planner server crashes with what(): Node '/global_costmap/global_costmap' has already been added to an executor.

Additional information

• I think the issue is valid also for controller server.
• https://github.com/ros-navigation/navigation2/blob/jazzy/nav2_planner/src/planner_server.cpp#L85 Here we call configure on costmap which adds it to executor, so even if transition fails it is still in configured state.
• So I think either costmap should return early if it is already in configured state or planner server should unconfigure(deactivate?) here https://github.com/ros-navigation/navigation2/blob/jazzy/nav2_planner/src/planner_server.cpp#L122

[SteveMacenski]

You need to set the lifecycle state back to inactive before trying again via the cleanup transition. Error states that happen from failed transitions need to be explicitly recovered from by transitioning back down to a lower state and trying again

See the ROS 2 lifecycle node design docs: https://design.ros2.org/articles/node_lifecycle.html

[bektaskemal]

I can't see clearly where it says we should cleanup after onConfigure[FAILURE].

And If I try cleanup after configure failure, I get No transition matching 2 found for current state unconfigured because the transition failed and we are still in Unconfigured.

[SteveMacenski]

Ah, I see. We don't enter the failure state in that situation, we simply fail the transition.

The only semi-clean way I see of handling this is if we reset/unconfigure anything setup in prior to returning the failure in https://github.com/ros-navigation/navigation2/blob/jazzy/nav2_planner/src/planner_server.cpp#L122. That way subsequent attempt's state will be back to what it should be from an unconfigured state.

It is a little fragile with edits in the future though. I suppose we could also try manually calling on_cleanup before that return as well which should reset everything from the configure state transition -- that seems more general to me and something we could apply to the Controller server and others that have the same lifecycle node pattern.

What do you think?

[bektaskemal]

Yeah, it makes sense. I'll test it.

[SteveMacenski]

Revieewing your PR now!

[SteveMacenski]

Merged