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

Let XML executables/nodes be "required" (like in ROS 1) #751

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

m-elwin
Copy link
Contributor

@m-elwin m-elwin commented Jan 2, 2024

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit= mechanism.

Right now "shutdown" is the only action accepted by on_exit, but theoretically more "on_exit" actions could be added later.

Example:

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
@moriarty
Copy link

moriarty commented Jan 2, 2024

👍

This feature is implemented using the python launchfile on_exit= mechanism.

Linking related issue #174 and PR #179 added similar support for Python launch files via on_exit=Shutdow()

Py Example:

    nodes_to_launch.append(
        Node(
            package="my_example_pkg",
            executable="example_node_main",
            name="example_node",
            ...
            on_exit=Shutdown(),
        )
    )

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution.

Besides the fix listed inline, would you also please add this support to YAML? That way we keep feature parity between XML and YAML.

if 'on_exit' not in ignore:
on_exit = entity.get_attr('on_exit', optional=True)
if on_exit is not None:
if on_exit == "shutdown":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our style prefers single-quotes:

Suggested change
if on_exit == "shutdown":
if on_exit == 'shutdown':

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
@m-elwin
Copy link
Contributor Author

m-elwin commented Jan 3, 2024

Thanks. Fixed the quotes and added a unit test for the yaml support. I think the code I added automatically covers both xml and yaml cases, but please let me know if I'm missing something.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for iterating! This looks great. I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

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

@clalancette clalancette merged commit 986ea83 into ros2:rolling Jan 3, 2024
3 checks passed
Timple pushed a commit to nobleo/launch that referenced this pull request Mar 5, 2024
* Let XML nodes be "required"

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

* Added tests for yaml

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
Timple pushed a commit to nobleo/launch that referenced this pull request Mar 5, 2024
* Let XML nodes be "required"

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

* Added tests for yaml

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
mjcarroll pushed a commit that referenced this pull request Mar 20, 2024
#764)

Backport #751 for Iron

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

* Added tests for yaml

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
Co-authored-by: Matthew Elwin <10161574+m-elwin@users.noreply.github.com>
jplapp pushed a commit to pixel-robotics/launch that referenced this pull request May 11, 2024
ros2#764)

Backport ros2#751 for Iron

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

* Added tests for yaml

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
Signed-off-by: Tim Clephas <tim.clephas@nobleo.nl>
Co-authored-by: Matthew Elwin <10161574+m-elwin@users.noreply.github.com>
@23pointsNorth
Copy link

I can see this being backported to iron. Is there a plan to move this to humble as well?

monicaperezserrano pushed a commit to monicaperezserrano/launch that referenced this pull request Dec 20, 2024
* Let XML nodes be "required"

Essentially on_exit="shutdown" is equivalent to ROS 1 required="true".

This feature is implemented using the python launchfile on_exit mechanism.

Right now "shutdown" is the only action accepted by on_exit,
but theoretically more "on_exit" actions could be added later.

Example:
<executable cmd="ls" on_exit="shutdown"/>

* Added tests for yaml

Signed-off-by: Matthew Elwin <elwin@northwestern.edu>
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.

4 participants