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

Support for respawn in ExecuteProcess #287

Closed
ivanpauno opened this issue Jul 25, 2019 · 18 comments
Closed

Support for respawn in ExecuteProcess #287

ivanpauno opened this issue Jul 25, 2019 · 18 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ivanpauno
Copy link
Member

Feature request

Add support for respawn and respawn_delay.

Feature description

ROS 1 supported those attributes in node tag. We could have something similar in ExecuteProcess action. The behavior could be emulated with event handlers, but having an argument in the constructor for that may be easier to use.

There are some point of discussions:

  • We could just implement it for the frontend implementations (XML, YAML), and avoid adding an argument to ExecuteProcess constructor.
  • Should event handlers targeting the original process target the new one?

Previous discussion: ros2/ros2_documentation#302 (comment)

@ivanpauno ivanpauno added the enhancement New feature or request label Jul 25, 2019
@ivanpauno
Copy link
Member Author

ivanpauno commented Jul 25, 2019

@wjwwood @hidmic FYI.

We could just implement it for the frontend implementations (XML, YAML), and avoid adding an argument to ExecuteProcess constructor.

I prefer implementing it in ExecuteProcess.

Should event handlers targeting the original process target the new one?

I think they should. I don't see an use case where that isn't desired.

@mkhansenbot
Copy link

+1 for getting this feature, is it planned for anytime soon?

@wjwwood
Copy link
Member

wjwwood commented Oct 10, 2019

+1 for getting this feature, is it planned for anytime soon?

I think the best assumption is no. Maybe @hidmic or @ivanpauno were planning on doing it, but I'm full booked up until well after ROSCon.

@mkhansenbot
Copy link

Thanks for the quick response, I'll see if either of them are planning to do it. It's not critical per se but it would help with occasional crashes we see in navigation2. It would also be nice if we could signal somehow that the node has been respawned to the lifecycle_manager node so that it could re-activate any node that crashes. Not to digress too far, but is there a mechanism for doing something like that?

@wjwwood
Copy link
Member

wjwwood commented Oct 10, 2019

It would also be nice if we could signal somehow that the node has been respawned to the lifecycle_manager node so that it could re-activate any node that crashes. Not to digress too far, but is there a mechanism for doing something like that?

Assuming it knows the node's name, it could look for the node to enter the unconfigured state (or whatever the first lifecycle event published is) on the event topic for that node. Launch does the same thing in order to produce launch events for each state transition.

@ivanpauno
Copy link
Member Author

+1 for getting this feature, is it planned for anytime soon?

I'm not planning to work on this in a near future.
If someone wants to work on it, I can help doing reviews.

@mkhansenbot
Copy link

Assuming it knows the node's name, it could look for the node to enter the unconfigured state (or whatever the first lifecycle event published is) on the event topic for that node. Launch does the same thing in order to produce launch events for each state transition.

That makes sense. We'll discuss that as a potential way of doing this. It won't matter until the 'respawn' capability is available though.

@Jconn
Copy link

Jconn commented Feb 14, 2020

has anything changed here? This is a nice to have for us

@hidmic hidmic added the help wanted Extra attention is needed label Apr 9, 2020
@iuhilnehc-ynos
Copy link
Contributor

@ivanpauno @hidmic
I'd like to contribute to this feature.

@ivanpauno
Copy link
Member Author

Thanks @iuhilnehc-ynos !
Let us know if you have any question or when you need a review.

@fujitatomoya
Copy link

@iuhilnehc-ynos

can you make PR against this? so that they can start review.

@iuhilnehc-ynos
Copy link
Contributor

@ivanpauno @fujitatomoya

I have made a PR for this feature, please review #426 .

@fujitatomoya
Copy link

@ivanpauno

friendly ping, i think that you could close this one.

@ivanpauno
Copy link
Member Author

Fixed in #426, thanks for the contribution @iuhilnehc-ynos !

@tuxnese
Copy link

tuxnese commented Jan 21, 2021

Is there a plan to port to Foxy?

@fujitatomoya
Copy link

i think currently no. this is feature, not bug fix. so it is unlikely to happen, i think.

@tuxnese
Copy link

tuxnese commented Jan 22, 2021

I have found the PR from blacksoul000:foxy . I have tried the blacksoul000:foxy fork and it seems to work fine, although I have not yet tested it a lot.

@tonynajjar
Copy link
Contributor

Hi @mkhansenbot, @wjwwood

That makes sense. We'll discuss that as a potential way of doing this. It won't matter until the 'respawn' capability is available though.

Now that respawn is available, was respawning + activating ever discussed again? Would be useful for me. Workarounds are also appreciated

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

No branches or pull requests

9 participants