Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 2 commits
02901df
988b77b
b2c73ec
469f5d7
0035757
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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()
and 'activated' foron_deactivate()
.on_shutdown()
/on_configure()
/on_error()
transitions.on_configure()
and destroy them onon_shutdown()
if those methods were available.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.
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.
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.
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.
I think we could do that.
The logic would be:
This applies when a on_configure callback errors, or when an on_shutdown callback errors after a previous failure.
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 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.