Add composition::LifecycleTalker component#746
Add composition::LifecycleTalker component#746SuperJappie08 wants to merge 3 commits intoros2:rollingfrom
Conversation
|
I think this is ready for review. |
|
This requires additional changes in order to align with #674 EDIT: This has been resolved |
Signed-off-by: SuperJappie08 <36795178+SuperJappie08@users.noreply.github.com>
Signed-off-by: SuperJappie08 <36795178+SuperJappie08@users.noreply.github.com>
Signed-off-by: SuperJappie08 <36795178+SuperJappie08@users.noreply.github.com>
SuperJappie08
force-pushed
the
rolling-lifecycle-component
branch
from
c439c57 to
4df76db
Compare
| const rclcpp_lifecycle::State & state) override; | ||
|
|
||
| rclcpp_lifecycle::node_interfaces::LifecycleNodeInterface::CallbackReturn on_shutdown( | ||
| const rclcpp_lifecycle::State & state) override; |
There was a problem hiding this comment.
Should we exercise the other transitions on_activate, etc?
There was a problem hiding this comment.
I'm unsure if I understand correctly, but do you mean 'Should we add the other transitions?'.
I haven't added them right now, as I intended this to be a minimal example for creating lifecycle components.
It isn't a full copy of lifecycle's LifecycleTalker as I wanted to keep this simple for testing (for example in ros2/launch_ros#479), because currently there is no suitable lifecycle component to test with.
I'm open to adding these extra transitions, but we should probably consider the 'difficulty order' or complexity of both lifecycle nodes and node components. (Thinking about 'the (fictitious) ROS 2 textbook chapter ordering'.)
For example, I would imagine most users (or at least beginners) would first learn about lifecycle nodes before node components.
This would make it safe to assume they are familiar with lifecycle nodes before looking at this, which would greatly limit the benefit of these nearly empty transitions while introducing more 'clutter'.
(Having written this, I'm second-guessing the ordering of my ROS 2 journey.)
|
|
||
| // We hold an instance of a timer which periodically triggers the publish function. | ||
| // As for the beta version, this is a regular timer. In a future version, a | ||
| // lifecycle timer will be created which obeys the same lifecycle management as the |
There was a problem hiding this comment.
Would definitely appreciate lifecycle timer contribution
mjcarroll
left a comment
mjcarroll
left a comment
There was a problem hiding this comment.
LGTM, one comment about extra state transitions.
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
For a demo package, the experience of running it matters a lot.
someone should be able to launch it and immediately see it working, or at minimum know exactly what commands to run.
Looking at how the existing lifecycle demo handles this, it ships a lifecycle_service_client node that automatically drives the state transitions, plus a launch file that brings everything up together. A new user can just ros2 launch and watch the full lifecycle play out.
For this composition::LifecycleTalker, if you just load the component, it starts in the unconfigured state and does nothing visible. Without either an automated driver (a companion node or launch file that loads the component and then transitions it through configure → activate), or clear documentation explaining the exact commands needed (e.g., ros2 component load ..., then ros2 lifecycle set ... configure, then ros2 lifecycle set ... activate) ???
3 participants
Description
Adds a lifecycle component to
composition(composition::LifecycleTalker)Fixes #745
Is this user-facing behavior change?
Yes, a new component is added.
Did you use Generative AI?
No.
Additional Information
Currently I have put it in the
compositionpackage, since it is more of a note that a lifecycle component can exist (and therefore does not extensively focus on the lifecycle part).