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.

Sign up for GitHub

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

Jump to bottom

Implement managed nodes #865

Merged
merged 34 commits into from
Dec 23, 2021
Merged

Implement managed nodes #865

merged 34 commits into from
Dec 23, 2021

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Dec 15, 2021 •
edited
Loading

Fixes #604.

I took into account the feedback provided here ros2/rclcpp#1846, so you don't need to manually call on_activate/on_deactivate for managed entities like LifecyclePublisher.
I'm open to revert that bit, though I think it's a good idea.

I have only manually tested that the lifecycle node services work correctly.
I have not tested the lifecycle publisher at all.
This PR has tests now.
I could write some automated tests for the lifecycle builtin services though.
Done as well.

This is not ready to be merged, though early feedback is appreciated anyway.
This is ready now.

I'm working to add a demo equivalent to demos/lifecycle, but that shouldn't block this from being reviewed.
PR with demo ready: ros2/demos#547.

@ivanpauno ivanpauno added the enhancement New feature or request label Dec 15, 2021
@ivanpauno ivanpauno self-assigned this Dec 15, 2021
@ivanpauno ivanpauno mentioned this pull request Dec 15, 2021
Closed
@scpeters scpeters mentioned this pull request Dec 15, 2021
Closed
bpwilcox
bpwilcox reviewed Dec 17, 2021
View reviewed changes
rclpy/rclpy/lifecycle.py Outdated
return TransitionCallbackReturn.SUCCESS


class SimpleManagedEntity(ManagedEntity):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there motivation to have the SimpleManagedEntity inherit from a ManagedEntity other than that they share two intended transitions (activate/deactivate). Presumably the other transitions do not do anything.

Copy link
Member Author

@ivanpauno ivanpauno Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought in some cases it could be useful to override some of the others, so I left all of them.

e.g. you can inherit from LifecyclePublisher and override on_error().
I'm not sure if that's really useful or not.

If the only use-case we see is something like SimpleManagedEntity, i.e. setting/clearing a flag in on_active/on_deactive, maybe it would be simpler to just keep a reference of the lifecycle node state machine in all managed entities and query if the current state is active or not.
That might be a bit slower, as there a few extra indirections and comparisons.

@ivanpauno ivanpauno mentioned this pull request Dec 17, 2021
Merged
@ivanpauno
wip
72c3ff6 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
wip 2
d72dc29 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Fix possible ownership issue
fb17060 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
make basic example work
b0bf636 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Make all the services work, checked by doing manual testing
19d5234 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
More consistent style
c01fe6f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Add some docs, add ManagedEntity concept
0c3b02b 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Make all linters happy
8efe11c 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Quick lifecycle publisher implementation
581d030 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Minor style change
d8451be 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Fix issues introduced when implementing LifecyclePublisher, automatic…
f5967cf 
…ally manage child entities

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Fixes to make it work again
ed57e05 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Modularize the implementation
30cb7a0 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Improve SimpleManagedEntity.when_enabled
b53bb36 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Add some docs to lifecycle publisher
d1ff708 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Register all necessary callbacks, add State abstraction, other minor …
2ac7da1 
…improvements

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Minor style changes, add local API to trigger transitions
33157f1 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Test LifecycleNode constructor, fix when enable_communication_interfa…
ebd1d9a 
…ce=False

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
test transitions + fixes
c6c4d73 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Add tests for lifecycle publisher
7517641 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Please flake8
ce0d256 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno marked this pull request as ready for review December 17, 2021 21:03
audrow
audrow reviewed Dec 17, 2021
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. It will be very nice to have lifecycle nodes in rclpy! I've commented on a few TODOs and made a few minor suggestions.

It would also be good to have a tutorial written for this at the same time that we merge in this PR.

Also, do you still intend to add more tests, or does that about cover it?

rclpy/test/test_lifecycle.py
node = LifecycleNode(
'test_lifecycle_state_transitions_1', enable_communication_interface=False)
# normal transitions
assert node.trigger_configure() == TransitionCallbackReturn.SUCCESS
Copy link
Member

Choose a reason for hiding this comment

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

Maybe have a for loop put the node through many transitions, just to confirm that they work back-to-back.

Copy link
Member Author

Choose a reason for hiding this comment

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

IIUC after 4ca2a36 I'm testing all the possible transitions back to back.
I find it clear to do them manually instead of in a for loop, but if you have a code snippet that you find better feel free to share it.

@ivanpauno
Minor style change after writting the demos
bd253ac 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno mentioned this pull request Dec 20, 2021
Merged
ivanpauno and others added 11 commits December 20, 2021 17:30
@ivanpauno @audrow
Fix misspelled word
324d4ef 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Audrow Nash <[email protected]>
@ivanpauno
Make some attributes in Node "protected" instead of private for easie…
2616315 
…r reuse in LifecycleNode and other derived classes

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Remove TODO, add better docs
b57a779 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Remove TODO in __register_callback
041ea6d 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Remove TODO
a72645b 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Check transition callbacks return code type. Check type of added mana…
bc98247 
…ged entitities. Share implementation of default transtion callbacks

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Check that state is 'unconfigured' also before transition
6c20f80 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Test lifecycle services
928ee79 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Delete early debugging code
59fcc4c 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Please linters
c31b4f6 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Test the two remaining shutdown transitions. Correct error handling w…
4ca2a36 
…hen shutdown transition is not possible

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno requested a review from audrow December 22, 2021 20:20
audrow
audrow approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@ivanpauno
Address peer review comments
c693398 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Copy link
Member Author

ivanpauno commented Dec 23, 2021 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status (unrelated failure)
  • Windows Build Status

@ivanpauno
Copy link
Member Author

Looks good to me with green CI.

I will request more feedback about the API, particularly considering it doesn't match exactly the rclcpp one (following the suggestion in ros2/rclcpp#1846).
I won't block this PR on that though, we still have time to make API changes until the Humble release.

@ivanpauno
Copy link
Member Author

Going in, thanks for the review @audrow !!

@ivanpauno ivanpauno merged commit 48c7850 into master Dec 23, 2021
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/lifecycle branch December 23, 2021 16:47
@ivanpauno ivanpauno mentioned this pull request Jan 13, 2022
Merged
@clalancette clalancette mentioned this pull request Apr 29, 2022
Closed
87 tasks
@SteveMacenski
Copy link

@ivanpauno any plans for adding docs that this exists? This came up here: ros-navigation/navigation2#3259 and while trying to show them documentation for the API, I came up with lint in my pockets 😄

@ivanpauno
Copy link
Member Author

@ivanpauno any plans for adding docs that this exists? This came up here: ros-planning/navigation2#3259 and while trying to show them documentation for the API, I came up with lint in my pockets smile

Yes sorry, there's not much more than the demo added in this PR up to now.
I will try to get some time to add better documentation.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/lifecycle-nodes-in-python/14547/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpwilcox bpwilcox bpwilcox left review comments

@audrow audrow audrow approved these changes

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: Managed Nodes
5 participants
@ivanpauno @SteveMacenski @ros-discourse @audrow @bpwilcox