Make transition event topic reliable to avoid topic lost

[kjjpc]

This PR is related to #1166.
Lifecycle action of launch_ros occasionally fails to transit because of topic event lost.
This PR make transition event topic reliable.
After the marge of this PR, I will make a PR for launch_ros.

[mjcarroll]

@kjjpc Thanks for reporting. We discussed this at the recent PMC maintainers' meeting and came to the following conclusion.

In this case, changing the durability of the transition event could have far reaching impacts on the system that are undesirable. The main concern is that if you had a late-joining lifecycle node, that it could be transitioned inadvertently by the latched topic. This would be pretty un-intuitive and could cause issues in larger systems.

The real issue here stems from the fact that we are triggering the transition only when the process becomes available, rather than waiting for the node to be in the unconfigured state.

I think that the real solution here would be detect when the node is started in the unconfigured state in your launch file, like:

    emit_active = RegisterEventHandler(
        launch_ros.event_handlers.OnStateTransition(
            target_lifecycle_node=lc_node,
            goal_state='unconfigured',
            ...
        ))

The other alternative would be to add on OnNodeReady event to launch_ros so that we are more intelligently waiting for the node to start.

[fujitatomoya]

@mjcarroll

i might be mistaken, but i was thinking opposite...

The main concern is that if you had a late-joining lifecycle node, that it could be transitioned inadvertently by the latched topic. This would be pretty un-intuitive and could cause issues in larger systems.

either that is lifecycle node or just a subscription, delivering the latest state change event via latched topic to the subscription that does specify the QoS with durability by the user application makes sense?
this is something user application wants to do that, so is specified by user.
if they do not want to receive the latest state change event via latched topic, they just do not need to specify the QoS durability on the subscription side? without this change, system cannot provide the choice for the user application.
besides, this does not break the communication between them, https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Quality-of-Service-Settings.html#qos-compatibilities

[mjcarroll]

either that is lifecycle node or just a subscription, delivering the latest state change event via latched topic to the subscription that does specify the QoS with durability by the user application makes sense?

Hmm, maybe my understanding of the issue here was wrong. I thought we were making the subscription transient_local, which would cause the issues I described.

[kjjpc]

@mjcarroll
Thanks for the discussion. The issue is that the transition event topic is volatile in the current implementation.
launch_ros occasionally fails to receive the change event topic because the topic is not latched.

[Wiktor-99]

Hey @mjcarroll @mjcarroll, is this change going to be merged or discussion is ongoing? I'd like this feature to be reliable. If any additional work needs to be done I can help

[wjwwood]

Pulls: #1171
Gist: https://gist.githubusercontent.com/wjwwood/ed8b269a34311280adcfd2e8586e7643/raw/caf75df9e5aa18e9d0289b229a5e4b76c4377935/ros2.repos
BUILD args: --packages-above-and-dependencies rcl_lifecycle
TEST args: --packages-above rcl_lifecycle
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/20087

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[wjwwood]

I think @fujitatomoya is right that the behavior change would not occur unless the subscriber also requests transient_local, otherwise the subscriber will only get new messages when subscribing with volatile. So I think this should be safe. If we also change some common code that makes the subscription transient_local, then I think we could be in some danger of undesired behavior change.

Now whether or not this is the right fix for the original problem, that I'm not sure about. If it's a race between the state changing and the subscription being established, then this would fix the problem if you also change the subscription, but it has the side-effect that late joiners (really late joiners) will get a message on this topic indicating the state changed, even though the state change happened long in the past.

Instead, I think it would make more sense to establish a volatile pub-sub pair for event notification, but also use a service call to query the state explicitly when first starting to listen to the state of a node. That way you never get stale messages on the event topic, but also don't rely on racing that topic for the first state observed when starting a monitoring process.

I started a CI just to see what the status of the code is, but I think this needs more discussion. @fujitatomoya or @mjcarroll what do you think?

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[fujitatomoya]

@wjwwood i agree, maybe we can discuss what the original problem is before considering the fixes.

[fujitatomoya]

@Mergifyio rebase

[mergify]

rebase

🛑 The pull request rule doesn't match anymore

Details

This action has been cancelled.

[fujitatomoya]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows