diagnostics

diagnostics copied to clipboard

Our robots react on the top-level state of the aggregator. But sometimes we have error which are relevant to know, but not critical at all for operation.

Therefor the DowngradeAnalyser, still report but just be happy.

Looks reasonable to me. Something similar we've experimented with is a folding analyzer— if error X is set, downgrade errors Y and Z, so that there isn't user confusion about which problem to resolve first.

Obviously the whole business of halting operation over diagnostics is in violation of the diagnostics REP, but it's a pretty common pattern, and awfully convenient given that there isn't any other commonly-supported system in ROS for exposing machine consumable component-level status: https://www.ros.org/reps/rep-0107.html#improper-usage

I get the concerns indicated in the REP. But doing it another way probably means implementing an pretty identical layout.

But regardless of that, we have a green/yellow/light on our robot which indicates the diagnostics status. If this is red because of known issues, users will become insensible for the red light and the whole idea is gone.

Does this PR need any more work to get accepted?

It looks good to me, but I'm not the maintainer of this repo, just an interested third party.

@trainman419 what do you think of this DowngradeAnalyser next to the DiscardAnalyser?

Also interested in this. @g-gemignani , since you are the new maintainer now, what do you think?

@Timple , maybe an idea to make the LEVEL configurable, OK, WARN?

Hi @Timple thank you for your PR. Could you provide a specific example of why would you want this analyzer? I understand changing the diagnostic status based on a given trigger but in your pull request, you are always treating the message as OK. Why would you have these diagnostics aggregated at all then? Just for logging? In such a case, wouldn't the standard logging mechanism be a better choice?

ROS nodes outside of our control generate diagnostic messages :+1: but they also control if it is a warning or error. For some nodes we 'disagree' that things should be a warning/error. However this is specific to our case so a PR to modify this does not make sense.

Therefor: DowngradeAnalyser. Still have all the useful information inside the diagnostic status (temperature/RPM/etc) but don't show errors. Especially since they propagate upward and the full robot is always in an error state, leading to people ignoring this.

@reinzor Making the level optional is actually a good idea. I'll see if we can pick this up if @g-gemignani is also onboard with the idea.

Yep, I do now understand why you would want that. I would agree on making the level configurable and if I am not asking too much, we would need a test added to the package

@Timple I rebased this feature branch onto noetic-devel. Would you change the target branch of this PR?