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

Automatically transition lifecycle entities when node transitions #1863

Merged

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Jan 13, 2022

Fixes #1846.
Similar to what was done in rclpy: ros2/rclpy#865.
Demo: ros2/demos#552.

…inactive

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the enhancement New feature or request label Jan 13, 2022
@ivanpauno ivanpauno self-assigned this Jan 13, 2022
{

/// Base class for lifecycle entities, like `LifecyclePublisher`.
class ManagedEntityInterface
Copy link
Member Author

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' for on_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 on on_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().

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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

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>
@ivanpauno
Copy link
Member Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno closed this Jan 14, 2022
@ivanpauno ivanpauno reopened this Jan 14, 2022
@ivanpauno
Copy link
Member Author

  • Windows Build Status

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

Successfully merging this pull request may close these issues.

Allow LifecycleNode to automatically transition Lifecycle-enabled ROS2 entities
2 participants