search

ethz-asl / ethzasl_xsens_driver

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

Fix conversion from UTC to epoch time for /time_reference #56

Closed anaiman closed 6 years ago

anaiman commented 7 years ago

Chasing some confusion with time syncing, I found what I think is a small bug in filling the /time_reference topic message from a UTC time.

The Xsens provides a UTC time stamp. To convert this to a ROS /time_reference (which I think is defined as time since epoch), we should use calendar.timegm() instead of mktime() which expects a tuple in local time as its argument.

fcolas commented 7 years ago

Indeed, you're right. But then I believe it should be the case also with the conversion from iTOW, right? Could you please check stamp_from_itow and update your PR if it does need fixing as well? Thanks again.

anaiman commented 6 years ago

Yes, I agree, I'll make the change there as well.

anaiman commented 6 years ago

I'm convinced that this is correct for the UTC time reference, but I'm not sure that stamp_from_itow will do the right thing if the interval since start_of_week includes a daylight savings time change because the timedelta won't take it into account. But that fix might require some refactoring beyond the scope of this PR (and I don't have time to figure it out at the moment, sorry).

Thanks for your hard work on this driver, it's very useful!

fcolas commented 6 years ago

Sorry for the delay. I agree with you that there might be some issue with DST change. However, I couldn't find a good reference to find a more proper conversion. I'll merge it in because right now it's wrong and hopefully issues shouldn't be to frequent if any. Thanks again for your contribution,