Why is the /diagnostics_toplevel_state ERROR when one of the diagnostics is STALE about diagnostics HOT 7 OPEN

The toplevel state is not just the maximum of all the levels. Its calculated with the following algorithm

if maximum_level > ERROR and minimum_level <= ERROR
	# one or more STALE, but not all of them
	level = ERROR
else:
	level = maximum_level

I would propose to change this to the following, because I think it's more logical.

if maximum_level == STALE and maximum_level_without_stale < ERROR
	# one or more STALE, but no errors
	level = STALE
else:
	level = maximum_level_without_stale

This will be the difference between the two algorithms:

from diagnostics.

Hi @Rayman and thank you for your comment. I agree with you that this makes more sense but, as you stated, this is a breaking change so for Noetic I do not believe it can be merged as is. I am not sure how the ros2 maintainers would see this change for the ros2 version...

from diagnostics.

Hi @Rayman. Thanks for your suggestion. Just to be clear: The stale state is set by the generic analyzer if no message was received within a given timeout:

If I get it correctly, your suggestion is to treat it in aggregation like the other levels and aggregate it in the group. There, I would honestly have a hard time to rate it in severity between the other levels. Currently, it is level 3 which reads as the highest priority. This means you would only see stale on the highest level, even if another item in that group is in the error state. This can not be the intended behavior. Changing these levels would be a SERIOUS breaking change.

What is your take on that?

from diagnostics.

What I find counter-intuitive about the current behavior is that if you have three leaf diagnostics rolled up into a group, the discard_stale doesn't seem to have an impact on the parent status. For example, if bar and baz in the example below go stale, but foo is OK, I would intuitively think that the part group should also be OK. However, what I'm seeing is that foo currently gets marked as ERROR.

diagnostics_aggregator:
  ros__parameters:
    pub_rate: 1.0
    path: 'robot'
    analyzers:
      part:
        type: 'diagnostic_aggregator/AnalyzerGroup'
        path: 'part'
        foo:
          type: 'diagnostic_aggregator/GenericAnalyzer'
          path: 'foo'
          find_and_remove_prefix: ['/foo:']
          num_items: 1
        bar:
          type: 'diagnostic_aggregator/GenericAnalyzer'
          path: 'bar'
          find_and_remove_prefix: ['/foo:']
          discard_stale: true
        baz:
          type: 'diagnostic_aggregator/GenericAnalyzer'
          path: 'baz'
          find_and_remove_prefix: ['/baz:']
          discard_stale: true

from diagnostics.

I did not want to propose to merge a breaking change in noetic, so I've implemented my proposed change in our fork: https://github.com/nobleo/diagnostics. Feel free to use it

I've implemented the change for the toplevel diagnostics and for the AnalyserGroup.

from diagnostics.

I also added my implementation here: #315

from diagnostics.

Since this was a breaking change for Noetic, it probably also is for Humble and Iron?

Does it make sense to put some effort into this before ROS Jazzy?

from diagnostics.