ROS specific launch

[wjwwood]

Requires #74

Things to have a look at:

• Simple pub/sub example:
  • https://github.com/ros2/launch/pull/75/files#diff-65d815fff2f1a19178fc422135bb3b66
• Lifecycle pub/sub example:
  • https://github.com/ros2/launch/pull/75/files#diff-2086d091e24f813cafb9e343fb009c64

[dejanpan]

@sumanth-nirmal

[sumanth-nirmal]

Lifecycle pub/sub example:

Copy/Paste mistake, correct link is https://github.com/ros2/launch/pull/75/files#diff-2086d091e24f813cafb9e343fb009c64

[wjwwood]

Rebased to one commit after #74 was merged. I'll start prepping this for review now.

[dhood]

partial review

[dhood]

I would suggest naming this ExecuteNode instead. To me, seeing something trigger an action of "Node" somewhere like here can mislead people, and I don't see the need to risk the confusion (but there might be one)

[wjwwood]

The reason I departed from Execute in these is because of how I planed to reuse them as arguments to the "node container", basically something like this:

LaunchDescription([
  ComponentNodeContainer(name='whatever', nodes=[
     Node(...),
     Node(...), 
  ]),
  Node(...),
])

Where the first two Node would never be visited, and instead would be introspected by the ComponentNodeContainer to figure out how to load them dynamically, and the last Node would actually get visited and therefore executed.

[wjwwood]

Maybe there's a better way to organize the names, with that in mind. I don't know.

I also thought about an Execute action which would look like:

LaunchDescription([
  ComponentNodeContainer(name='whatever', nodes=[
     Node(...),
     Node(...), 
  ]),
  Execute(executable=Node(...), cwd='something'),
])

In which case Node would no longer inherit from ExecuteProcess.

[dhood]

Even if Node doesn't inherit from ExecuteProcess we'd still probably benefit from distinguishing it from something that is actually a node (it is not, in any of these cases). For now, it's namespaced enough that I suppose we can lean on that.

[wjwwood]

That's part of the reason I use launch.x.Y in most of the demos, to reinforce those differences.

I also somewhat inconsistently use Launch<thing> and <thing>, e.g. LaunchDescription versus Action (and not LaunchAction). I don't have a good reason for the inconsistency, other than I felt that in context launch.Action was specific enough, but launch.Description was not (mostly due to where and how it's used in the examples.

It might be something we want to revisit in the near future.

Do you want me to change anything for this pr related to this conversation?

[dhood]

I think for this PR we're good, and then there's a chance we'll rename things in the future.

[dhood]

we should make a todo (if we haven't already, apologies if this is something you've already tracked) to add an AlreadyInitializedException to rclpy.

[wjwwood]

Already made one 😁: ros2/rclpy#190

[dhood]

I think the usage will be clearer if this is renamed to something like launcher_node because otherwise being used as context.locals.rclpy_node you don't really know where it came from/who put it there.

[wjwwood]

Sounds good 431853f

[dhood]

Aren't there cases where the specific order of the remapping rules has to be preserved, because applying them in different orders causes different results? If so, another type would be more appropriate e.g. OrderedDict or list of tuples.

[wjwwood]

That's true, let me look into that.

[wjwwood]

So, I can't use OrderedDict as a type annotation with subscripted types for key and value, unfortunately. I don't like using a list of tuples because of how it looks/feels in the examples, but I don't see another way, and creating an ordered dict also uses a list of tuples. So I guess I'll do that, but the demos/examples need to be updated.

[wjwwood]

See: https://stackoverflow.com/questions/41207128/how-do-i-specify-ordereddict-k-v-types-for-mypy-type-annotation

[wjwwood]

313bd70

[dhood]

for readability of this file I recommend reversing the creation of these actions so we read about them in the order that they would occur

[wjwwood]

The event handler must be registered before sending the event. Do you have a suggestion for how to reorder it that works?

[dhood]

I am not sure if we are talking about the same scale. What I meant was add the action that creates the node; add the action that configures the node; add the action that activates the node; add the action that starts the listener. But I think it wasn't clear so you thought I was referring to this individual add_action call.

[wjwwood]

Checkout 46bd3e1

[dhood]

what is the point of adding a leading forward slash if the next line does lstrip('/')?

[wjwwood]

No idea, 03e9d07

[dhood]

this also depends on rclpy, lifecycle_msgs, ament_index_python and osrf_pycommon

[wjwwood]

Yes, but I don't think osrf_pycommon directly. cd0ebe2

[dhood]

osrf_pycommon's which is used: https://github.com/ros2/launch/pull/75/files#diff-5faadcc2201b3e21ba8aee1be2d7d50fR30

[wjwwood]

You were right, I was wrong, here's the fixup: 15ac0c3

[dhood]

I think this should be more generic than referencing nodes since it will prefix all processes' output right?

[wjwwood]

Correct b4b83da

[dhood]

what's the intended use of this?

[wjwwood]

Future use, nothing for now.

[dhood]

it seems like this isn't lifecycle specific (maybe it was at some point but it doesn't look like it's used anymore), and the module could be moved/renamed (maybe in followup)

[wjwwood]

Well, not lifecycle specific, but there are no events that target Node right now (there are some that target ExecuteProcess however), so I put it with the lifecycle events which it is used with. We may need to move it later.

[dhood]

seems like it'd be more appropriate to explicitly set self.__current_state = ChangeState.valid_states[lifecycle_msgs.msg.State.PRIMARY_STATE_UNKNOWN] (because we want to do this regardless of dict ordering)

[wjwwood]

Fair enough, I think I was imagining that the "first" state might change and this would be more future proof, but that's silly (it was probably late): 6955bc8

[dhood]

maybe this should be an error instead of just a warning.

[wjwwood]

65c2b53

[dhood]

this is not being used FWICT

[wjwwood]

Right 838242e

[dhood]

🚢

[wjwwood]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

[wjwwood]

Retroactive summary for this feature:

add ROS specific features to launch system

Purpose / Use Cases (Why implement this feature?)

This was implemented to provide ROS specific actions, substitutions, events, etc... to the launch system and to add default actions which allow monitoring of child nodes with lifecycle.

Design (How does it work?)

It provides new actions (like Node or LifecycleNode) and other related new entities like substitutions and events, which a user my use in their launch description. These new items are often just syntactic sugar, building entirely on existing features in launch. This is by far not a complete set of new features for ROS, but just the first iteration.

It also adds some "default" ROS related actions that are (will be) included in the ros2 launch command. These default actions do things like create a rclpy node for launch, which is used to monitor the state of child nodes which have lifecycle. It uses this information to generate events in launch when these child nodes change state.

Inputs / Outputs / API (How to use it?)

The API is anything in the launch_ros package:

https://github.com/ros2/launch/tree/baefc077b062ba5cfaff8a4dbecda7aaaeb4a05a/launch_ros/launch_ros