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.

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

Unexpected value of launch argument in 'on_exit' clause #501

Open
siteks opened this issue Apr 3, 2021 · 4 comments
Open

Unexpected value of launch argument in 'on_exit' clause #501

siteks opened this issue Apr 3, 2021 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@siteks
Copy link

siteks commented Apr 3, 2021

Bug report

  • Ubuntu 20.04 ROS2 Foxy
  • Installed from package binaries:

Steps to reproduce issue

# test1.launch.py
from launch import LaunchDescription
from launch.actions import IncludeLaunchDescription
from launch.launch_description_sources import PythonLaunchDescriptionSource

def generate_launch_description():
    ld = LaunchDescription()
    for r in ['r1', 'r2']:
        ld.add_action(IncludeLaunchDescription(
            PythonLaunchDescriptionSource('test2.launch.py'),
                launch_arguments = {'r' : r}.items()
        ))
    return ld

# test2.launch.py
from launch import LaunchDescription
from launch.actions import ExecuteProcess
from launch.actions import DeclareLaunchArgument
from launch.substitutions import LaunchConfiguration

def generate_launch_description():
    ld = LaunchDescription()
    ld.add_action(DeclareLaunchArgument('r'))
    ld.add_action(
        ExecuteProcess(
            cmd     = ['echo', 'first', LaunchConfiguration('r')],
            output  = 'screen',
            on_exit = ExecuteProcess(
                cmd     = ['echo', 'second', LaunchConfiguration('r')],
                output  = 'screen'
            )
        )
    )
    return ld


ros2 launch test1.launch.py

Expected behavior

echo commands print:

first r1
first r2
second r1
second r2

Actual behavior

[INFO] [echo-1]: process started with pid [1686]
[INFO] [echo-1]: process has finished cleanly [pid 1686]
[INFO] [echo-2]: process started with pid [1688]
[INFO] [echo-2]: process has finished cleanly [pid 1688]
[echo-1] first r1
[echo-2] first r2
[INFO] [echo-3]: process started with pid [1690]
[INFO] [echo-3]: process has finished cleanly [pid 1690]
[echo-3] second r2
[INFO] [echo-4]: process started with pid [1692]
[INFO] [echo-4]: process has finished cleanly [pid 1692]
[echo-4] second r2

Additional information

Somewhat related to ros/robot_state_publisher#144 I wish to run a robot_state_publisher per robot in a multi robot system where all the robot are identical, and they all get the URDF model from a topic published by the owner of the model, which in this case is a docker container running Gazebo. Since robot_state_publisher does not support receiving a model via topic, my workaround was to have a helper node receive the model and write it to a temporary local file, after which robot_state_publisher could be started.

A launch file is invoked per robot with a different value launch parameter uniquely naming the robot and the temporary file. Using the style of test2.launch.py to achieve this was unsuccessful, because the value of the launch configuration variable 'r' is always 'r2' in the 'on_exit' ExecuteProcess.

I'm quite new to the Python launch system and would welcome any pointers to what I might be doing wrong here.

@clalancette
Copy link
Contributor

So you are talking about two different things here, I believe.

One is that we never made progress on ros/robot_state_publisher#144 . As far as I can tell from the end of that conversation, there is no fundamental objection to that, it's just that nobody spent the time to write the code to allow the robot description to come from a topic. If you are interested in that, please consider submitting a pull request for it which we'd be happy to review.

The other thing you are talking about is making the current system work with robot_description as a parameter. My understanding is that something like you've shown above should work in the launch system, but there may be some subtlety that I'm missing. Hopefully one of the launch maintainers can steer you in the correct direction.

@hidmic
Copy link
Contributor

hidmic commented Apr 5, 2021

This is related to #313. Launch files are included within their parent scope, and thus both launch files in the example above refer to the same r variable. This is handy when you have deep hierarchies and do not want to re-declare arguments, but it is sometimes surprising.

Short term, you may explicitly push and pop scopes before and after including a launch file with the PushLaunchConfigurations and PopLaunchConfigurations actions. Long term, we probably need a way for users to specify if they want scoped inclusion or not. CC @ivanpauno @wjwwood.

@hidmic hidmic added the enhancement New feature or request label Apr 5, 2021
@siteks
Copy link
Author

siteks commented Apr 6, 2021

@clalancette yes, apologies, reference to #144 was more meant as background to the particular problem I was trying to solve. I'll see if I can work up a PR though.

@hidmic I read #313 but couldn't see exactly how this relates though I may be missing something. Surely, whether 'r' is within a separate scope or not, both of the ExecuteProcess actions in test2.launch.py should see the same value of 'r' in a given invocation of the launch file?

I tried changing the calling launch file to:

def generate_launch_description():
    ld = LaunchDescription()
    for r in ['r1', 'r2']:
        ld.add_action(PushLaunchConfigurations())
        ld.add_action(IncludeLaunchDescription(
            PythonLaunchDescriptionSource('test2.launch.py'),
                launch_arguments = {'r' : r}.items()
        ))
        ld.add_action(PopLaunchConfigurations())
    return ld

But this gives:

[echo-1] first r1
Task exception was never retrieved
future: <Task finished name='Task-9' coro=<LaunchService._process_one_event() done, defined at /opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py:271> exception=SubstitutionFailure("launch configuration 'r' does not exist")>
...
[echo-2] first r2
Task exception was never retrieved
future: <Task finished name='Task-12' coro=<LaunchService._process_one_event() done, defined at /opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py:271> exception=SubstitutionFailure("launch configuration 'r' does not exist")>
...

As with the original example, I get the expected output for the first ExecuteProcess but it seems that the second ExecuteProcess, attached to the 'on_exit' is not in the local scope and does not see the local 'r'.

I also tried doing RegisterEventHandler(OnProcessExit(...)) attached to the first ExecuteProcess and saw the same thing. Is this to do with the scope of an event handler?

@hidmic
Copy link
Contributor

hidmic commented Apr 6, 2021

whether 'r' is within a separate scope or not, both of the ExecuteProcess actions in test2.launch.py should see the same value of 'r' in a given invocation of the launch file?

The lack of scope means arguments, just like variables, can be overridden. First include sets r to 'r1', second include sets r to 'r2'. Everything that follows, above or below in the hierarchy, sees r equals 'r2'.

I get the expected output for the first ExecuteProcess but it seems that the second ExecuteProcess, attached to the 'on_exit' is not in the local scope and does not see the local 'r'.

Hmm, may be onto something here. Event handlers do not carry a closure of the local launch configurations in the scope they were registered. That's true, and bad surprising. Bottom line, your use case requires lexical scoping and closures, and launch does neither right now. Should be more or less straightforward to implement though: tracking local launch configurations on event handler registration and adding an attribute to IncludeLaunchDescription to optionally scope what's within. Contributions are much welcome.

CC @ivanpauno @wjwwood for feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants