Add composition::LifecycleTalker component #746
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.
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).