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

Expose lifecycle_node #327

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

esteve
Copy link
Member

@esteve esteve commented Oct 4, 2022

Original author: @kenji-miyake

Closes #236

Signed-off-by: Kenji Miyake <[email protected]>
@kenji-miyake
Copy link
Contributor

@esteve Thank you for your work!
But actually, since it's still a limited part, I think it won't "close" the issue.

@esteve
Copy link
Member Author

esteve commented Oct 4, 2022

@kenji-miyake thanks. I created ros2/launch#658 to address event handlers

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
It would be nice to add some tests before merging.

If we only expose this, and not event handlers, there's no much advantage of using lifecycle_node over node in XML launch files.
The only advantage is that the StateTransition and ChangeState events will be emited.
So if you include a python launch file from the XML launch file, you can take advantages of those using event handlers.
Though that seems fine IMO

@ivanpauno
Copy link
Member

@esteve @kenji-miyake could you add a test case for this?
Thanks!

@adityapande-1995 adityapande-1995 added the tests Failing or missing tests label Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog tests Failing or missing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support LifecycleNode and EventHandler in frontend
4 participants