Skip to content

Conversation

@mergify
Copy link

@mergify mergify bot commented Feb 19, 2025

This resolves #418 which implements autostarting lifecycle nodes. This is complete and ready for a review

You'll notice a couple of key changes worth explaining:

  1. There is a new is_lifecycle_node attribute of the node and lifecycle nodes which is needed to remove a circular dependency on the LifecycleNode within the LifecycleTransition class which only is type checking. I replace this type check with checking if a non-None object (1) has the attribute at all and (2) has a lifecycle attribute with appropriate logging between the difference of a non-lifecycle node being attempted vs a non-node. This has been tested as well to function using the demos.

  2. You will also notice that I refactored lifecycle node to have a separate util LifecycleEventManager, who handles the event emitting, handling, and topic listening for lifecycle nodes. This way, this can be used in lifecycle-components as well! No changes were made here except removing the self.__current_state variable which was completely unused. The LifecycleEventManager is only created if the node requests autostart at Launch time and otherwise has no overhead nor exposes the interfaces/event handler/emitter if its a non-lifecycle node.

  3. The component nodes don't add a / before the node names with the namespaces. This messes with the node event matcher. I add in a leading / so that the action.node_name == node_name which has the / forcably applied in the node event matcher

  4. I tested LifecycleNode, ComposableNodeContainer, LoadCompoableNodes for all cases: autostart=True autostart=False, and autostart field not supplied. You can also run any experiments you like using the 2 extra launch files I provide in the examples directory which shows all the features at work/


    This is an automatic backport of pull request Autostarting lifecycle nodes and example launch file demo #430 done by Mergify.

@wjwwood
Copy link
Member

wjwwood commented Feb 19, 2025

@methylDragon if you have some time, could you review this backport with an eye towards likelihood of regressions in jazzy?

@SteveMacenski fyi

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2025

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@audrow
Copy link
Member

audrow commented Feb 27, 2025

Friendly ping @methylDragon

@wjwwood wjwwood removed their assignment Feb 28, 2025
@SteveMacenski
Copy link
Contributor

Thanks! I look forward to this!

@christophebedard christophebedard self-requested a review March 13, 2025 17:37
@christophebedard christophebedard self-assigned this Mar 13, 2025
@christophebedard
Copy link
Member

#449 should be backported after this, too.

@christophebedard
Copy link
Member

@Mergifyio rebase

* autostarting lifecycle nodes and example

Signed-off-by: Steve Macenski <[email protected]>

* fix linting

Signed-off-by: Steve Macenski <[email protected]>

* fix linting

Signed-off-by: Steve Macenski <[email protected]>

* removing an unused variable

Signed-off-by: Steve Macenski <[email protected]>

* initializing a member client as none like sub

Signed-off-by: Steve Macenski <[email protected]>

* completing auto-start feature for composition nodes

Signed-off-by: Steve Macenski <[email protected]>

* linting

Signed-off-by: Steve Macenski <[email protected]>

* final linting fix

Signed-off-by: Steve Macenski <[email protected]>

* Resolving issue with StateTransition messages

Signed-off-by: Steve Macenski <[email protected]>

* removing Node's autostart

Signed-off-by: Steve Macenski <[email protected]>

* Fixing lifecyclenode type

Signed-off-by: Steve Macenski <[email protected]>

* update docstring

Signed-off-by: Steve Macenski <[email protected]>

* adding in composable lifecycle node

Signed-off-by: Steve Macenski <[email protected]>

* Adding docstring

Signed-off-by: Steve Macenski <[email protected]>

* require autostart non-None

Signed-off-by: Steve Macenski <[email protected]>

* remove unused imports

Signed-off-by: Steve Macenski <[email protected]>

* review comments continued

Signed-off-by: Steve Macenski <[email protected]>

* is lifecycle node and autostart to lifecycle-only classes

Signed-off-by: Steve Macenski <[email protected]>

* final bits

Signed-off-by: Steve Macenski <[email protected]>

* whoops, remove the Avatar-inspired printout

Signed-off-by: Steve Macenski <[email protected]>

* finish linting

Signed-off-by: Steve Macenski <[email protected]>

* review fix

Signed-off-by: Steve Macenski <[email protected]>

* remove old import

Signed-off-by: Steve Macenski <[email protected]>

---------

Signed-off-by: Steve Macenski <[email protected]>
(cherry picked from commit 3569f0d)
@mergify
Copy link
Author

mergify bot commented Mar 25, 2025

rebase

✅ Branch has been successfully rebased

@christophebedard christophebedard force-pushed the mergify/bp/jazzy/pr-430 branch from 733c3e4 to 271691d Compare March 25, 2025 00:39
@christophebedard
Copy link
Member

I've taken a look. Regression-wise, there aren't many actual tests for this, but it looks fine. The autostart feature is fairly separate.

I rebased and I'll trigger a fresh round of CI.

@christophebedard
Copy link
Member

Pulls: #438
Gist: https://gist.githubusercontent.com/christophebedard/268da2541a058cc2da3ffbae02102bb5/raw/5454692502baa3ff2fa71754dd131842e7f1471d/ros2.repos
BUILD args: --packages-above-and-dependencies launch_ros test_launch_ros
TEST args: --packages-above launch_ros test_launch_ros
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15456

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@SteveMacenski
Copy link
Contributor

@christophebedard I'd make sure to also merge in the fix from #449 as that's an important fix

@christophebedard
Copy link
Member

I was going to process them separately to keep the (backporting) history clean, but yeah. I've created #453 to backport #449 and rebased it on this commit. I triggered CI for the combination of both backports in #453 (comment). Once that looks good, we can merge them both: #438 then #453.

@christophebedard christophebedard merged commit e2983a1 into jazzy Mar 25, 2025
2 checks passed
@christophebedard christophebedard deleted the mergify/bp/jazzy/pr-430 branch March 25, 2025 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants