[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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
Merged

Conversation

kyrofa
Copy link
Member
@kyrofa 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 feature/174/required_nodes branch 2 times, most recently from 3e34dbf to 039644e Compare February 7, 2019 21:58
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 ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@kyrofa kyrofa force-pushed the feature/174/required_nodes branch from 039644e to 6fdc5c9 Compare February 7, 2019 22:01
@kyrofa kyrofa mentioned this pull request Feb 7, 2019
@tfoote tfoote added the in review Waiting for review (Kanban column) label Feb 7, 2019
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Copy link
Member
@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Other than a style nitpick, lgtm.

launch/launch/actions/execute_process.py Outdated Show resolved Hide resolved
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@wjwwood
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 Waiting for review (Kanban column) label Feb 8, 2019
@kyrofa kyrofa deleted the feature/174/required_nodes branch February 8, 2019 00:01
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.

Support required nodes
3 participants