search

ethz-asl / ethzasl_xsens_driver

Driver for xsens IMUs
BSD 2-Clause "Simplified" License
102 stars 111 forks source link

[wip] Make diagnostics more reasonable #43

Open mjcarroll opened 7 years ago

mjcarroll commented 7 years ago

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

fcolas commented 7 years ago

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.

mjcarroll commented 7 years ago

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 commented 7 years ago

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

fcolas commented 7 years ago

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 commented 6 years ago

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

mjcarroll commented 6 years ago

yes thanks for reminder

fcolas commented 6 years ago

@mjcarroll Ping?

skohlbr commented 6 years ago

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

mjcarroll commented 6 years ago

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 commented 6 years ago

@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.