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

Support required nodes #179

Merged
merged 4 commits into from Feb 8, 2019

Conversation

Projects
None yet
3 participants
@kyrofa
Copy link
Contributor

kyrofa commented Feb 7, 2019

Currently, the only way to specify that a given node is required for a given LaunchDescription is to register an OnProcessExit handler for the exit event of the required node that then emits a Shutdown event. Instead of requiring this boilerplate for a common use-case, This PR resolves #174 by adding an on_exit parameter to ExecuteProcess that can take actions, and adding a new action called Shutdown that immediately causes the launched system to shut down. This allows for the concept of a required node to be expressed simply with:

launch_ros.actions.Node(
    # ...
    on_exit=Shutdown()
)

@kyrofa kyrofa force-pushed the kyrofa:feature/174/required_nodes branch 2 times, most recently from 3e34dbf to 039644e Feb 7, 2019

Support required nodes
Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve #174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

@kyrofa kyrofa force-pushed the kyrofa:feature/174/required_nodes branch from 039644e to 6fdc5c9 Feb 7, 2019

@kyrofa kyrofa referenced this pull request Feb 7, 2019

Closed

Support required nodes #174

@tfoote tfoote added the in review label Feb 7, 2019

Inherit from EmitEvent
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@wjwwood

wjwwood approved these changes Feb 7, 2019

Copy link
Member

wjwwood left a comment

Other than a style nitpick, lgtm.

Show resolved Hide resolved launch/launch/actions/execute_process.py Outdated
Properly align type
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@wjwwood

wjwwood approved these changes Feb 7, 2019

Expose reason
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@wjwwood

wjwwood approved these changes Feb 7, 2019

@wjwwood

This comment has been minimized.

Copy link
Member

wjwwood commented Feb 7, 2019

CI:

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

@wjwwood wjwwood merged commit b665104 into ros2:master Feb 8, 2019

@wjwwood wjwwood removed the in review label Feb 8, 2019

@kyrofa kyrofa deleted the kyrofa:feature/174/required_nodes branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.