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

Add ForLoop action to repeat entities with an index #802

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Oct 13, 2024

Relates #499

Being able to repeat entities (e.g., nodes) N times based on the value of a launch argument is a pretty commonly-requested feature. Example: #499 and https://discourse.ros.org/t/poll-interest-in-ros2-launch-action-to-support-for-loops-e-g-includenlaunchdescriptions/20026. Some current solutions/workarounds include:

  1. OpaqueFunction: Launch Loop Support? [Feature Request] #499 (comment)
  2. Recursion: https://github.com/MetroRobots/rosetta_launch#12---recursion

While these work, they involve some boilerplate code that has to be repeated in each launch file, and they make launch files more complex.

This PR aims to simplify this by adding a new ForLoop action. It takes in something that defines the number of for-loop iterations, which can be a launch argument (LaunchConfiguration). It also takes in a function that gets called with each for-loop index value (0 to N, exclusive). This function should return a list of entities using the index value to differentiate entities between each for-loop iteration.

This is basically a nicer alternative to using an OpaqueFunction directly in Python, and supports frontends.


Example:

import launch
import launch_ros


def for_i(i: int):
    return [
        launch_ros.actions.Node(
            package='test_tracetools',
            executable='test_ping',
            output='screen',
            namespace=['/pingpong_', str(i)],
        ),
        launch_ros.actions.Node(
            package='test_tracetools',
            executable='test_pong',
            output='screen',
            namespace=['/pingpong_', str(i)],
        ),
    ]


def generate_launch_description():
    return launch.LaunchDescription([
        launch.actions.DeclareLaunchArgument('num_robots', default_value='2'),
        launch.actions.ForLoop(launch.substitutions.LaunchConfiguration('num_robots'), function=for_i),
    ])

Frontends are also supported. In this case, the callback function is created internally by the parser using the child entities and an $(index) substitution referencing the (index) name of the for-loop:

<launch>
    <arg name="num_robots" default="2" />
    <for len="$(var num_robots)" name="i" >
        <node pkg="test_tracetools" exec="test_ping" namespace="/pingpong_$(index i)" output="screen" />
        <node pkg="test_tracetools" exec="test_pong" namespace="/pingpong_$(index i)" output="screen" />
    </for>
</launch>
launch:
    - arg:
        name: num_robots
        default: '2'
    - for:
        len: $(var num_robots)
        name: i
        children:
            - node:
                pkg: test_tracetools
                exec: test_ping
                namespace: /pingpong_$(index i)
                output: screen
            - node:
                pkg: test_tracetools
                exec: test_pong
                namespace: /pingpong_$(index i)
                output: screen

If any of these launch files were launched without any launch arguments, they would create 2 pairs of ping & pong nodes, each pair in its own namespace. With num_robots:=5, they would create 5 pairs.


Open questions:

  1. Is this worth having in launch? I think so given that this is a common feature request (and undoubtedly a common Google search), but I'm open to other opinions.
  2. Any better name than ForLoop? Repeat, maybe?
  3. For frontends, is there another mechanism (other than locals) for defining and using the index variable in each loop iteration?
  4. Anything else? Any limitations or anything that would break this?

I can add tests once this looks good enough. For now, it's a draft. Also, once it looks good enough, I'd like to post on Discourse to request feedback from users.

We could also add support for start/stop like Python's range().


Bonus: you can even nest <for> loops:

<launch>
    <arg name="num_i" default="2" />
    <arg name="num_j" default="3" />
    <for len="$(var num_i)" name="i" >
        <for len="$(var num_j)" name="j" >
            <log message="i=$(index i), j=$(index j)" />
        </for>
    </for>
</launch>

Output:

[INFO] [launch.user]: i=0, j=0
[INFO] [launch.user]: i=0, j=1
[INFO] [launch.user]: i=0, j=2
[INFO] [launch.user]: i=1, j=0
[INFO] [launch.user]: i=1, j=1
[INFO] [launch.user]: i=1, j=2

@christophebedard christophebedard changed the title Add ForLoop action to repeat entities with a substitutable index Add ForLoop action to repeat entities with an index Nov 2, 2024
@christophebedard
Copy link
Member Author

After some quick initial feedback, I changed the implementation to instead take a callback function, similar to OpaqueFunction.

@Timple
Copy link

Timple commented Nov 3, 2024

Are you planning to support xml and yaml syntax as well?

@christophebedard
Copy link
Member Author

Yeah, I think XML and YAML support are very important. I managed to get a working hacky implementation, but it could probably be improved.

@christophebedard christophebedard self-assigned this Nov 4, 2024
@christophebedard christophebedard force-pushed the christophebedard/for-loop-action branch 4 times, most recently from 50a7b35 to 65dfa86 Compare November 5, 2024 16:37
@christophebedard christophebedard marked this pull request as ready for review November 5, 2024 17:07
@christophebedard
Copy link
Member Author

Well, with frontend support, this feels complete enough, so I think this is ready for a real review.

@christophebedard
Copy link
Member Author

Are you planning to support xml and yaml syntax as well?

@Timple feel free to provide feedback and/or just say whether you think this would be useful

@Timple
Copy link

Timple commented Nov 5, 2024

I don't know how this would look like in xml. So an example or test for that would be nice to show that.

Having the ability to support N lidar nodes is nice 👍

@christophebedard
Copy link
Member Author

I will work on adding tests, but there are XML and YAML examples in the PR description above.

@Timple
Copy link

Timple commented Nov 5, 2024

Tjeez... I totally overlooked those. That works great!

@christophebedard christophebedard force-pushed the christophebedard/for-loop-action branch 2 times, most recently from 593554a to 6b35a08 Compare November 6, 2024 18:57
@christophebedard
Copy link
Member Author

I've now added tests (Python, XML, YAML).

@christophebedard
Copy link
Member Author

We discussed this PR at a high level in the November 12th ROS PMC meeting. In short, this is pushing us towards less declarative launch files, which makes writing potential linters/checkers harder. However, it was still possible to do this before using OpaqueFunction (only in Python), so this reduces boilerplate code, and it allows us to use this functionality from the XML & YAML frontends. Therefore, pragmatically, it was deemed to be acceptable.

Now the PR just needs to be reviewed.

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.

2 participants