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

Add DowngradeAnalyser for reporting non-critical errors #126

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

Timple
Copy link
Contributor

@Timple Timple commented Oct 16, 2019

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.

@mikepurvis
Copy link
Member

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

@Timple
Copy link
Contributor Author

Timple commented Oct 25, 2019

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?

@mikepurvis
Copy link
Member

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

@Timple
Copy link
Contributor Author

Timple commented Oct 28, 2019

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

@reinzor
Copy link
Contributor

reinzor commented Jul 29, 2020

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?

@g-gemignani
Copy link
Collaborator

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?

@Timple
Copy link
Contributor Author

Timple commented Jul 30, 2020

ROS nodes outside of our control generate diagnostic messages 👍 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.

@g-gemignani
Copy link
Collaborator

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

@rokusottervanger
Copy link

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

@Timple Timple changed the base branch from indigo-devel to noetic-devel April 8, 2022 09:59
@ralph-lange ralph-lange added the ros1 PR tackling a ROS1 branch label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ros1 PR tackling a ROS1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants