Feature/rosmon diag

[adrienbarral]

I did this small node that analyze data published by rosmon to produce ros diagnostics.
We use it on our projects to have a diagnostic on the CPU or memory usage of a node.

This modification was done as a new node in a new package of the rosmon metapackage.

[romainreignier]

@adrienbarral The build is failing because std_srvs dependency is missing in CMakeLists.txt and package.xml.

[xqms]

Hi! Thanks for the PR, this looks highly interesting. I'll have a deeper look next week when I have time.

In my mind it would make sense to integrate this into rosmon itself rather than having a separate node. What do you think?

[romainreignier]

I have successfully tested it on my project.
With a diagnostic_aggregator it is really nice to see all the infos in the rqt_rosmon_monitor.

@xqms About integrating it into rosmon itself, maybe it would be less clear how to setup the several parameters (prefix, limits...).

[adrienbarral]

As usual there are pro and cons to integrate this code directly into rosmon node, but I personally see more "cons" arguments

Cons :

• If user don't want this feature, he just have to not launch the node
• Integrate this feature in rosmon node will create a coupling between these two codes. When I developped this feature, I want to be sure to not break anything in rosmon. So developping my feature as a third part node allow this.

PRO :

• This is more efficient, because we don't need to serialize / deserialize datas

So if you want to integrate it in your node why not. But me, I prefer to put this kind of "accessories features" out of the main code. Because you masterize the rosmon code, maybe it will be straight forward for you to add it into the main node.

[xqms]

I would like to offer another pro: per-node configuration could be done directly in the launch file, like

<launch>
    <node name="xyz" pkg="xyz" type="xyz" rosmon-cpu-limit="0.5" rosmon-memory-limit="150M">
    </node>
</launch>

which I think feels nicer (and I need to keep less places in sync, e.g. when changing node names).

Regarding your cons: Is there any drawback to enabling the feature? Why not have it on by default?
And regarding the isolation: We could still have it as a separate class/method which takes the rosmon status message and publishes the diagnostics message (although directly using the NodeMonitor values is probably easier).

[adrienbarral]

I really like your way to define per-node resource usage directly in launch file.

I think I will have time to move my code into a class embedded into rosmon node this end of week. So I hope to be able to update the PR next Monday.

So I will :

• Add to "LaunchConfig" parsing of "resources" usage limits attributes.
• Add to the "ROSInterface" a class (or a method) to publish diagnostics.

[xqms]

Sounds good to me! 👍

One thing you should take care about is to use a rosmon-* prefix for the attributes you introduce in the launch file - that way we can be sure that there will be no conflict with future roslaunch attributes. It also helps people unfamiliar with rosmon to find out what these are about.

[xqms]

Closing in favor of #76, let's continue the discussion there.