Skip to content
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

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

Merged
merged 2 commits into from Nov 28, 2017

Conversation

Projects
None yet
2 participants
@anaiman
Copy link
Contributor

commented Sep 8, 2017

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

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2017

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

@anaiman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2017

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

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

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,

@fcolas fcolas merged commit 1842757 into ethz-asl:master Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.