[wip] Make diagnostics more reasonable #43
Conversation
|
Thanks a lot for your contribution, indeed it's probably better. I've made a few comments that should be fixed. |
nodes/mtnode.py
Outdated
| @@ -11,6 +11,9 @@ | |||
| FluidPressure, Temperature, TimeReference | |||
| from geometry_msgs.msg import TwistStamped, PointStamped | |||
| from diagnostic_msgs.msg import DiagnosticArray, DiagnosticStatus | |||
|
|
|||
There was a problem hiding this comment.
please remove this empty line
nodes/mtnode.py
Outdated
| 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 = { |
There was a problem hiding this comment.
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).
nodes/mtnode.py
Outdated
| self.device_info = { | ||
| 'product_code': self.mt.GetProductCode(), | ||
| 'device_id': self.mt.GetDeviceID(), | ||
| 'firmware_rev': self.mt.GetFirmwareRev() |
There was a problem hiding this comment.
these three lines are over indented: please add only one indentation level
nodes/mtnode.py
Outdated
|
|
||
| output_config = self.mt.GetConfiguration() | ||
| for (k,v) in output_config.iteritems(): | ||
| self.device_info[k] = v |
There was a problem hiding this comment.
unless there is a reason, update is probably better
these two lines could just be: self.device_info.update(output_config)
nodes/mtnode.py
Outdated
| 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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
nodes/mtnode.py
Outdated
| 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, |
There was a problem hiding this comment.
Why checking only imu/data and imu/mag?
|
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 |
|
I think that fixes the PEP8 violations, must have been using a misconfigured editor. |
|
Many thanks for the PEP8 corrections. It looks great. Also do you think it could make sense to provide launch and yaml files in order to launch |
|
@mjcarroll Do you think you would have time to finish this? |
|
yes thanks for reminder |
|
@mjcarroll Ping? |
|
Just ran into diagnostics publishing at high rate, so another vote for finishing this PR from me :) |
mjcarroll
force-pushed
the
add_diagnostics
branch
from
df188ab
to
b7dd960
Compare
|
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. |
|
@mjcarroll Thanks for fixing the conflicts and issues. 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. |
The diagnostics topic currently publishes at the rate of the IMU, this should really only publish at a fixed frequency, using
diagnostic_updater