-
Notifications
You must be signed in to change notification settings - Fork 422
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
Automatically transition lifecycle entities when node transitions #1863
Automatically transition lifecycle entities when node transitions #1863
Conversation
…inactive Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
{ | ||
|
||
/// Base class for lifecycle entities, like `LifecyclePublisher`. | ||
class ManagedEntityInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface in rclpy is a bit different (see here):
- on_activate()/on_deactivate() receives the previous state as a parameter and can fail.
- I think in most of the cases on_activate()/on_deactivate() shouldn't fail, the on_shutdown()/on_configure() transitions should be doing the fallible work.
- Receiving the previous state is not important for this transitions, as it's always 'inactive' for
on_activate()
and 'activated' foron_deactivate()
.
- The rclpy interface allows you to also provide
on_shutdown()
/on_configure()
/on_error()
transitions.- LifecyclePublisher is not using it, though entities could lazely create resources on
on_configure()
and destroy them onon_shutdown()
if those methods were available. - The default on_configure/on_shutdown() implementation in Node might be less useful, as it's not clear what to do if some of the callbacks fail (see this and this).
In practice, I think people usually want to manually write on_configure()/on_shutdown().
- LifecyclePublisher is not using it, though entities could lazely create resources on
We could either modify this PR to do exactly what rclpy does or reduce rclpy flexibility to match more closely what this PR is adding.
I prefer a bit this less flexible version, which is what we actually seem to need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in most of the cases on_activate()/on_deactivate() shouldn't fail
Having less failure points sounds nice to me. I haven't thought through scenarios where someone may want to fail during activation/deactivation, but we could add it if the need arises.
The default on_configure/on_shutdown() implementation in Node might be less useful, as it's not clear what to do if some of the callbacks fail
If one or more of the callbacks fail, I would think we would want to transition all entities to the unconfigued
state (as indicated by the design doc diagram). What do you think?
I agree that in most cases people will probably provide their own on_configure/on_shutdown logic. I think that it still might be nice if we call the transitions on all managed entities though. E.g. call any user-defined callback for the node and then, if that is successful, call the transition callbacks for managed entities. Or do you see something wrong with that behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If one or more of the callbacks fail, I would think we would want to transition all entities to the unconfigued state (as indicated by the design doc diagram). What do you think?
I think we could do that.
The logic would be:
- configure
- Try to transition all managed entities to configured
- If transition fails (TRANSITION_FAILURE), shut down all the managed entities that already transitioned to go back to the unconfigured state.
- If any callback errors (TRANSITION_ERROR), then we immediately return error without doing any more transitions.
This applies when a on_configure callback errors, or when an on_shutdown callback errors after a previous failure.
- shutdown
- Similar, but on failures we do nothing, and continue trying to move the rest of the entities to the shutdown state.
The only problem I see is that the logic is a bit complex and might not be actually useful in practice (we don't have any lifecycle entity that do work on configure/shutdown currently).
I agree that in most cases people will probably provide their own on_configure/on_shutdown logic. I think that it still might be nice if we call the transitions on all managed entities though. E.g. call any user-defined callback for the node and then, if that is successful, call the transition callbacks for managed entities. Or do you see something wrong with that behavior?
I think this has been resolved in the discussion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I'm fine keeping the implementation simple for now; we can add complexity if the need arises.
{ | ||
|
||
/// Base class for lifecycle entities, like `LifecyclePublisher`. | ||
class ManagedEntityInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in most of the cases on_activate()/on_deactivate() shouldn't fail
Having less failure points sounds nice to me. I haven't thought through scenarios where someone may want to fail during activation/deactivation, but we could add it if the need arises.
The default on_configure/on_shutdown() implementation in Node might be less useful, as it's not clear what to do if some of the callbacks fail
If one or more of the callbacks fail, I would think we would want to transition all entities to the unconfigued
state (as indicated by the design doc diagram). What do you think?
I agree that in most cases people will probably provide their own on_configure/on_shutdown logic. I think that it still might be nice if we call the transitions on all managed entities though. E.g. call any user-defined callback for the node and then, if that is successful, call the transition callbacks for managed entities. Or do you see something wrong with that behavior?
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Fixes #1846.
Similar to what was done in rclpy: ros2/rclpy#865.
Demo: ros2/demos#552.