Add configurable launch parameters

[mm318]

The launch parameters may now be specified in the launch file, and consequently on the command line as well. Node names, measurement period, publish period, and publish topic are things that are configurable.

Example usage and output:

# ros2 launch system_metrics_collector system_cpu_and_memory.launch.py 
[INFO] [launch]: All log files can be found below /root/.ros/log/2020-01-22-22-04-56-835420-u0fc025e8e7ed53.ant.amazon.com-10207
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [linux_cpu_collector-1]: process started with pid [10218]
[INFO] [linux_memory_collector-2]: process started with pid [10219]
[linux_cpu_collector-1] [ERROR] [1579730697.973499051] [ComputeCpuActivePercentage]: a measurement was empty, unable to compute cpu percentage
...

[thomas-moulard]

Launch files should not contain any logic, let's chat about this pr

[mm318]

Launch files should not contain any logic, let's chat about this pr

Finished chatting. Please re-review, @dabonnie and @thomas-moulard.

[emersonknapp]

this looks fine

[thomas-moulard]

This LGTM - could you check quickly if there isn't any kind of builtin way to rename nodes to make sure we're not reimplementing something that already exists? It was the case in ROS2, I'm surprised this hasn't been ported.

[thomas-moulard]

variables containing a scientific value should include the unit default_measurement_period -> default_measurement_period_in_ms

[emersonknapp]

@thomas-moulard if i understand correctly, you are asking about this https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#name-remapping

[thomas-moulard]

@emersonknapp wrong username :p Yes exactly, but does it work with roslaunch2? I know we can rename nodes if we invoke them directly on the command line, but not sure if there is a "default" roslaunch2 arg for that.

[mm318]

I think you mean something like overriding a builtin __node parameter, but I'm not sure how that will behave for a launch file that's launching multiple nodes (e.g.: which node's name will it change?)

[emersonknapp]

Hm, yes, I'm not sure. I'm pretty sure if you know the name, you can remap any ROS2 name from the launch command line, but I'm not sure how

[thomas-moulard]

^ what I'm talking about - if we could educate ourselves on this now, that would solve us a lot of work in the future.

[mm318]

After doing some testing, this is my understanding.

__node is a parameter that you may override when doing ros2 run to change the name of the node. In the context of a launch file, when you do launch_ros.actions.Node(node_name=<something>), it is essentially doing the same thing under the hood (i.e.: adding -r __node:=<something> to the run parameters).

However, if you try to override __node by doing ros2 launch <package> <launch_file> __node:=<something>, it is actually the launch_ros_[xxxx] node (so apparently ros2 launch spawns an extra node) that gets renamed to <something> (and you'll get a warning that this is a deprecated syntax, suggesting you to do --ros-args --remap __node:=<something> instead, but doing that has no effect whatsoever given how our launch file is written).

I'm not sure what's the right way to override parameters at the ros2 launch command line as it gives the following warnings:

# ros2 launch system_metrics_collector system_cpu_and_memory.launch.py linux_cpu_collector_node_name:=something1 linux_memory_collector_node_name:=something2
[WARN] [1579792466.558805964] [rcl]: Found remap rule 'linux_cpu_collector_node_name:=something1'. This syntax is deprecated. Use '--ros-args --remap linux_cpu_collector_node_name:=something1' instead.
[WARN] [1579792466.558831749] [rcl]: Found remap rule 'linux_memory_collector_node_name:=something2'. This syntax is deprecated. Use '--ros-args --remap linux_memory_collector_node_name:=something2' instead.

But it works. I actually think for ros2 launch to produce this warning is a bug, as I think what's happening is the parameter override has both taken effect at the ros2 launch / launch file level as well as propagated down to the child ros2 run calls level.

[mm318]

In short, I believe what we're doing in this pull request is the right approach.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@791fc37). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master     #80   +/-   ##
========================================
  Coverage          ?   39.3%           
========================================
  Files             ?      33           
  Lines             ?    1346           
  Branches          ?     809           
========================================
  Hits              ?     529           
  Misses            ?      62           
  Partials          ?     755

Flag	Coverage Δ
#unittests	39.3% <ø> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 791fc37...0c1c99b. Read the comment docs.