Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retrying configure for planner/controller is not possible #4678

Closed
bektaskemal opened this issue Sep 16, 2024 · 6 comments · Fixed by #4708
Closed

Retrying configure for planner/controller is not possible #4678

bektaskemal opened this issue Sep 16, 2024 · 6 comments · Fixed by #4708
Labels
enhancement New feature or request in progress

Comments

@bektaskemal
Copy link
Contributor

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

@SteveMacenski
Copy link
Member

SteveMacenski commented Sep 16, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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?

@SteveMacenski SteveMacenski reopened this Sep 23, 2024
@bektaskemal
Copy link
Contributor Author

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

@SteveMacenski
Copy link
Member

Revieewing your PR now!

@SteveMacenski
Copy link
Member

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants