UniversalTimeScaleClock now() and conversion from unix time

[ahundt]

It seems the UniversalTimeScaleClock doesn't have a now() function. Additionally, it isn't clear to me how to convert the std::chrono::steady_clock::time_point from std::chrono::steady_clock::now() to cartographer::common::Time. I saw that there are ROS conversions in cartographer_ros, but that requires ROS as a dependency,

What is the best way to convert std::chrono::steady_clock::time_point and get the current time in cartographer::common::Time format with high precision?

Perhaps an addition for now() such as https://stackoverflow.com/a/17138183/99379 would be appropriate?

struct UniversalTimeScaleClock {
  using rep = int64;
  using period = std::ratio<1, 10000000>;
  using duration = std::chrono::duration<rep, period>;
  using time_point = std::chrono::time_point<UniversalTimeScaleClock>;
  static constexpr bool is_steady = true;

    static time_point now() noexcept
    {
        using namespace std::chrono;
        return time_point
          (
            duration_cast<duration>(system_clock::now().time_since_epoch()) +
            seconds(kUtsEpochOffsetFromUnixEpochInSeconds)
          );
    }
};

I'm not 100% sure if I'm supposed to subtract or add kUtsEpochOffsetFromUnixEpochInSeconds though...

update: adding is the correct behavior

[wohe]

Can you explain a bit what you are trying to do?

The reason that there is no now() function is that timing is best provided with the sensor data and that should not get its time from the system clock for best accuracy. Accurate timing information matters a lot, so ideally this would be hardware timestamps included in the sensor data.

Regarding a conversion from steady_clock, I think the answer is that this should not be done. There intentionally is no fixed relationship and steady_clock::time_points should only be used relative to each other, e.g. to compute durations.

[ahundt]

Unfortunately my sensors can't do hardware time synchronization so I'm trying to estimate the skew between multiple hardware times and local machine time in software across multiple hardware devices. Some of the reasons timing/synchronization is important are discussed in #242.

This involves timestamping before making data requests and after they are received on multiple sensors so I can better estimate when in local machine time the data was collected, so I can then determine a corrected hardware time stamp that most accurately matches between two data sources and correctly sample in the pose interpolation buffer.

An example of this sort of thing is TicSync/TriggerSync (pdf). Other approaches to time synchronization are implemented in https://github.com/ethz-asl/kalibr with various tradeoffs.

ROS code:
https://github.com/englishar/trigger_sync

[ahundt]

Regarding a conversion from steady_clock, I think the answer is that this should not be done. There intentionally is no fixed relationship and steady_clock::time_points should only be used relative to each other, e.g. to compute durations.

I disagree on this point because I have found fusing multiple data sources according to a common time frame to be a very sound and well studied approach to reducing noise/error and improving the accuracy of world state estimates.

[wohe]

Thanks for the context.

There is no fixed relationship between steady_clock and UniversalTimeScaleClock. It is for example allowed by the C++ standard for steady_clock to count time since boot. Likewise, the epoch of system_clock is not specified even though it might be the Unix epoch in practice. clock_gettime's CLOCK_REALTIME makes this guarantee. Do you disagree?

Trying to get the best possible estimate for sensor timing as you suggest is indeed a very good idea. These timestamps have to be accurate relative to each other. Shifting all sensor timestamps by a fixed offset to the correct date and time has no influence on the quality of the resulting trajectory. An approach could be: Computing the offset once at the very beginning, add this constant offset to all sensor data. If you need to convert from a Unix epoch timestamp to UniversalTimeScaleClock for this, you'd have to add kUtsEpochOffsetFromUnixEpochInSeconds, not subtract.

[ahundt]

There is no fixed relationship between steady_clock and UniversalTimeScaleClock. It is for example allowed by the C++ standard for steady_clock to count time since boot. Likewise, the epoch of system_clock is not specified even though it might be the Unix epoch in practice. clock_gettime's CLOCK_REALTIME makes this guarantee. Do you disagree?

Yes, I agree. This makes sense now that we are on the same page, thanks. I think in practice every major OS, compiler, and standard library implements unix time. However, now() on the UniversalTimeScaleClock would definitely break in the unusual case a clock does not conform to this ad-hoc unix time convention.

Considering that, would you consider accepting a PR with either of the following or should I just keep things in user code?

1. now() with the known assumption and an accompanying warning in the comments
2. An additional helper function that achieves a similar result, much like the ROS helpers

I'd prefer to rely on the ad-hoc convention over runtime estimation when it is feasible and correct on my platform, unless there's another approach that would have no limitations?

If you need to convert from a Unix epoch timestamp to UniversalTimeScaleClock for this, you'd have to add kUtsEpochOffsetFromUnixEpochInSeconds, not subtract.

Thanks!

[BrannonKing]

I'm going to vote with @wohe on this and say that now() does not belong in the core library; before I understood what was happing with the time stamps, I would have accidentally abused it (if it was there), and that would have given me the wrong behavior. Just keep the clock handling where you are handling/converting your messages.

[ahundt]

Ok fair enough, thanks!