Try to address #25 unstable frequency#30
Try to address #25 unstable frequency#30versatran01 wants to merge 9 commits intoKumarRobotics:masterfrom versatran01:master
Conversation
|
@versatran01 Thanks for the change! I was on vacation last week, but I'll take a look at it this week. |
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
I've got quite a few comments in here, but my last one is the biggest and most important.
| pnh_.param("compensated", compensated_, false); | ||
| pnh_.param("time_alpha", time_alpha_, 0.0); | ||
| ROS_INFO("time_alpha: %f", time_alpha_); | ||
| time_alpha_ = std::max(0.0, std::min(time_alpha_, 1.0)); |
There was a problem hiding this comment.
So, I understand that time_alpha needs to be between 0.0 and 1.0, but it is annoying to the user to quietly change the data behind their back. If they entered in 2.0, then they'll just quietly get 1.0, which may not be what they are expecting. I think it would be better to raise an exception and fail the constructor here.
| // delta time from ros | ||
| const int64_t dt_ros = (ros_time_now - ros_time_last_).toNSec(); | ||
| // filtered delta time | ||
| const int64_t dt_filtered = dt_ros * time_alpha_ + dt_dev * (1 - time_alpha_); |
There was a problem hiding this comment.
So time_alpha seems to be a measure of the relative "goodness" of each of the time sources. If time_alpha == 0, then we trust the delta from the IMU 100%. If time_alpha == 1, then we trust the delta from ROS time 100%. This gives us flexibility, but the two problems I see with it are:
- ROS time is definitely going to be wrong at times. USB can cause batching behavior where we get significantly shorter time between callbacks interrupts than we are expecting (I've seen 5ms, rather than the expected 10ms), and latency in the kernel can cause the callback to be delayed for significant amounts of time (I've seen 16ms instead of the expected 10ms). Thus, I don't think
time_alphacan accurately capture these arbitrary differences. - Following on from the above point, I don't see how a user would be able to tune this for accurate measurements. Other than the delta reported from the IMU, there is no data available for them to see how to tune it. And if we are depending on the delta reported by the IMU, we may as well just use that for our dt anyway.
I have a totally different suggestion here. Instead of trying to do this tuning, what if we periodically re-sync the "zero" time of both the ROS time and the IMU delta? That is, every 5 seconds (say), we re-zero them both. That means that they should never drift too far apart, but it also means that for the more common case of the delta, we are trusting the device to give us good data. Thoughts on that approach?
There was a problem hiding this comment.
For the re-sync idea, wouldn't this still cause jumps in timestamps (at the point of resyncing)?
From the plot in #25, it shows a time drift of 2ms / 500s. This means the drift will be 16ms after about an hour.
And since the time zero is from ros time, this is already wrong in the first place, so I don't see how one could fix this.
2 participants
Use the idea from Use the relative timestamp from the device. #28 to fix the unstable timestamp, see Unstable generated frequency #25 for details.
Add a parameter
time_alphato compute final timestamp ast1 = a * dt_ros + (1-a) * dt_dev + t0
time_alpha defaults to 0 which is the fix from Use the relative timestamp from the device. #28.
User can check
/imu/dtfor the difference between 2 consecutive time stamps and/imu/dnowfor the deviation from ros time.Various cleanup.
Use binary group 1 only.
Remove ypr publisher
Normally I would just merge this, but since there are other people using this driver, it would be nice if someone could review this PR.
@IanTheEngineer @clalancette @Triocrossing
Thanks.