-
Notifications
You must be signed in to change notification settings - Fork 15
Added first commit for diagnostics for the phidgets. #20
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
Conversation
@@ -17,5 +17,10 @@ | |||
<author email="[email protected]">Copyright (C) 2010 Phidgets Inc. </author> | |||
|
|||
<buildtool_depend>catkin</buildtool_depend> | |||
<build_depend>diagnostics_updater<build_depend> | |||
<build_depend>diagnostics_msgs<build_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing slash ("/") in the two closing tags above.
OK, will fix these soon and update pull request. Thanks for the corrections. |
Don't merge yet. I've added a couple of comments; they need to be fixed to make it compile. Also, the last diagnostics message needs to be published periodically (let's say once a second). Otherwise the diagnostics don't show up until the state changes, and then they will be marked stale in how to test:Launch file:
analyzers.yaml:
Then |
* Added "/" for closing tags in package.xml * Moved lines to non-static function as required.
Ah, I saw that you already added a commit. Yep, that's what I meant. So now what's missing before we can merge is the periodic publishing. Could you add a comment once you've done that? I only get notifications for comments, not new commits added to the PR. |
Yes, I will comment once I am done. |
So far the periodic publishing, I just need to add the aggregator and add the analyzers.yaml with the pub_rate: 1.0 #Hertz ? |
Sorry for the late reply - family obligations! :)
No, you need to call |
Thanks for the help! I'll get this done this week and comment with an update. |
@kkiyenga It's an a good feature to have. Have you had a chance to update the diagnostics code? I volunteer to continue this development if you are not working on it anymore. |
@mani-monaj I can work on it later this month or if you really interested you can definitely finish it up. |
@kkiyenga Thanks for your quick reply. I will most likely start doing it later this week. I think I can finish it up. |
- Remove all diagnostics logic from phidget_api - Add diagnostics to phidgets_imu - Connect/disconnect/error states are propogated from callbacks - Periodic updates for diagnostics through processImuData() - Add Frequency and Timestamp (drift) diagnostics for imu/data_raw topic
* Added "/" for closing tags in package.xml * Moved lines to non-static function as required.
- Remove all diagnostics logic from phidget_api - Add diagnostics to phidgets_imu - Connect/disconnect/error states are propogated from callbacks - Periodic updates for diagnostics through processImuData() - Add Frequency and Timestamp (drift) diagnostics for imu/data_raw topic
- Remove all diagnostics logic from phidget_api - Add diagnostics to phidgets_imu - Connect/disconnect/error states are propogated from callbacks - Periodic updates for diagnostics through processImuData() - Add Frequency and Timestamp (drift) diagnostics for imu/data_raw topic
- Remove all diagnostics logic from phidget_api - Add diagnostics to phidgets_imu - Connect/disconnect/error states are propogated from callbacks - Periodic updates for diagnostics through processImuData() - Add Frequency and Timestamp (drift) diagnostics for imu/data_raw topic
This PR can be closed (moved to #24 ). |
Add IMU diagnostics (related to #20)
Hi,
This just my first commit for the diagnostics. Check and see if this works on the imu (should be working). I can also add other information if you would like.
Thanks,