[wip] Make diagnostics more reasonable #43

mjcarroll · 2017-05-16T17:52:17Z

The diagnostics topic currently publishes at the rate of the IMU, this should really only publish at a fixed frequency, using diagnostic_updater

fcolas · 2017-05-19T12:55:38Z

Thanks a lot for your contribution, indeed it's probably better. I've made a few comments that should be fixed.
Moreover maybe we should discuss exactly what kinds of diagnostics we should publish. For instance, checking the frequency of the topics doesn't really make sense if we don't know which topics should be published and at which frequency.

fcolas · 2017-05-19T12:00:12Z

nodes/mtnode.py

@@ -11,6 +11,9 @@
    FluidPressure, Temperature, TimeReference
 from geometry_msgs.msg import TwistStamped, PointStamped
 from diagnostic_msgs.msg import DiagnosticArray, DiagnosticStatus
+


please remove this empty line

nodes/mtnode.py

fcolas · 2017-05-19T12:04:41Z

nodes/mtnode.py

        self.stest_stat = DiagnosticStatus(name='mtnode: Self Test', level=1,
                                           message='No status information')
        self.xkf_stat = DiagnosticStatus(name='mtnode: XKF Valid', level=1,
                                         message='No status information')
        self.gps_stat = DiagnosticStatus(name='mtnode: GPS Fix', level=1,
                                         message='No status information')
-        self.diag_msg.status = [self.stest_stat, self.xkf_stat, self.gps_stat]
+
+	self.device_info = {


Never mix tab and space in Python code!
This code uses four spaces for indentation (as advised in PEP 8 please comply to this (you have several lines with tabulations).

fcolas · 2017-05-19T12:09:08Z

nodes/mtnode.py

+	self.device_info = {
+		'product_code': self.mt.GetProductCode(),
+                'device_id': self.mt.GetDeviceID(),
+                'firmware_rev': self.mt.GetFirmwareRev()


these three lines are over indented: please add only one indentation level

fcolas · 2017-05-19T12:12:55Z

nodes/mtnode.py

+
+        output_config = self.mt.GetConfiguration()
+        for (k,v) in output_config.iteritems():
+            self.device_info[k] = v


unless there is a reason, update is probably better
these two lines could just be: self.device_info.update(output_config)

nodes/mtnode.py

fcolas · 2017-05-19T12:29:24Z

nodes/mtnode.py

+        self.updater.add("Device Info", self.diagnostic_device)
+
+        self.imu_freq = diagnostic_updater.TopicDiagnostic("imu/data", self.updater,
+		diagnostic_updater.FrequencyStatusParam({'min': 0, 'max': 100}, 0.1, 10),


Frequency of certain field in certain configurations can be up to 2000 Hz.
Does it really make sense to check that the frequency is somewhere between 0 and 2000? Wouldn't it be more useful to check the expected frequency?

I think it would make the most sense to check within a certain tolerance of the desired frequency, which is what I've done elsewhere (+/- 1% or +/- 5% depending on the hardware)

fcolas · 2017-05-19T12:30:45Z

nodes/mtnode.py

+        self.imu_freq = diagnostic_updater.TopicDiagnostic("imu/data", self.updater,
+		diagnostic_updater.FrequencyStatusParam({'min': 0, 'max': 100}, 0.1, 10),
+		diagnostic_updater.TimeStampStatusParam())
+        self.mag_freq = diagnostic_updater.TopicDiagnostic("imu/mag", self.updater,


Why checking only imu/data and imu/mag?

nodes/mtnode.py

mjcarroll · 2017-05-23T15:22:21Z

Thanks for the feedback.

This was a very quick stab as I needed it in the field for IMU and mag data, and it was flooding out the rest of my /diagnostics messages. Give me a few days for things to settle down here, and I'll address these comments and take the WIP tag off of this PR.

mjcarroll · 2017-05-23T15:37:39Z

I think that fixes the PEP8 violations, must have been using a misconfigured editor.

fcolas · 2017-06-12T16:12:07Z

Many thanks for the PEP8 corrections. It looks great.
As for the questions about the frequency values and the topics for which we check the frequency, should it be done by checking the configuration, or do you believe that should be parameters?

Also do you think it could make sense to provide launch and yaml files in order to launch rqt_robot_monitor directly? (might be easier if the solution above is to use parameters since some of them might be in common)
What do you think?

fcolas · 2017-11-28T13:55:20Z

@mjcarroll Do you think you would have time to finish this?

mjcarroll · 2017-12-01T00:30:29Z

yes thanks for reminder

fcolas · 2018-07-16T08:10:16Z

@mjcarroll Ping?

skohlbr · 2018-10-15T13:52:39Z

Just ran into diagnostics publishing at high rate, so another vote for finishing this PR from me :)

mjcarroll · 2018-10-15T15:54:55Z

So the conflicts are now gone, but I no longer have this hardware.

I think it would make sense to have the updater use the frequency that the IMU is set to, but since that's currently not handled in the node, I'm not sure what the best way to do that would be.

fcolas · 2018-10-26T12:15:16Z

@mjcarroll Thanks for fixing the conflicts and issues.
We could have an expected parameter for each topic. But then people would have to specify it twice: for the node, and for the diagnostics configuration. Giving access in the node to the configuration might be feasible, but as it's currently supporting several kinds of devices with their different configuration specification, it's not a trivial change.

In the meantime, if you don't have much time, the easiest would be to exclude the frequency check in the pull request to have the reasonable aspect and open an issue calling for this functionality.
It'd be great if you could do that.

fcolas suggested changes May 19, 2017

View reviewed changes

Michael Carroll added 2 commits October 15, 2018 10:52

Make diagnostics more reasonable.

635f473

Fix worst of PEP8 violations.

b7dd960

mjcarroll force-pushed the add_diagnostics branch from df188ab to b7dd960 Compare October 15, 2018 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Make diagnostics more reasonable #43

[wip] Make diagnostics more reasonable #43

mjcarroll commented May 16, 2017

fcolas commented May 19, 2017

fcolas May 19, 2017

fcolas May 19, 2017

fcolas May 19, 2017

fcolas May 19, 2017

fcolas May 19, 2017

mjcarroll May 23, 2017

fcolas May 19, 2017

mjcarroll commented May 23, 2017

mjcarroll commented May 23, 2017

fcolas commented Jun 12, 2017

fcolas commented Nov 28, 2017

mjcarroll commented Dec 1, 2017

fcolas commented Jul 16, 2018

skohlbr commented Oct 15, 2018

mjcarroll commented Oct 15, 2018

fcolas commented Oct 26, 2018

[wip] Make diagnostics more reasonable #43

Are you sure you want to change the base?

[wip] Make diagnostics more reasonable #43

Conversation

mjcarroll commented May 16, 2017

fcolas commented May 19, 2017

fcolas May 19, 2017

Choose a reason for hiding this comment

fcolas May 19, 2017

Choose a reason for hiding this comment

fcolas May 19, 2017

Choose a reason for hiding this comment

fcolas May 19, 2017

Choose a reason for hiding this comment

fcolas May 19, 2017

Choose a reason for hiding this comment

mjcarroll May 23, 2017

Choose a reason for hiding this comment

fcolas May 19, 2017

Choose a reason for hiding this comment

mjcarroll commented May 23, 2017

mjcarroll commented May 23, 2017

fcolas commented Jun 12, 2017

fcolas commented Nov 28, 2017

mjcarroll commented Dec 1, 2017

fcolas commented Jul 16, 2018

skohlbr commented Oct 15, 2018

mjcarroll commented Oct 15, 2018

fcolas commented Oct 26, 2018