sccn / liblsl

C++ lsl library for multi-modal time-synched data transmission over the local network
Other
108 stars 63 forks source link

Change the C++11 clock source #104

Closed tstenner closed 3 years ago

tstenner commented 3 years ago

Quoting from the docs for std::chrono::high_resolution_clock:

The high_resolution_clock is not implemented consistently across different standard library implementations, and its use should be avoided. It is often just an alias for std::chrono::steady_clock or std::chrono::system_clock, but which one it is depends on the library or configuration. When it is a system_clock, it is not monotonic (e.g., the time can go backwards). For example, for gcc's libstdc++ it is system_clock, for MSVC it is steady_clock, and for clang's libc++ it depends on configuration.

This PR switches the clock source to std::chrono::steady_clock and converts the timestamps to double in two steps to avoid a precision loss (double has 53 bits of precision, but nanoseconds since 1970 are already in beyond 2**60).

cboulay commented 3 years ago

I tested the artifacts on Win/Ubuntu 18.04/MacOS Big Sur. They all ran (but not without some effort to bypass Mac security settings). Even after a cold boot, Windows timestamps are still around 645000 range but the value does seem to be seconds (multiple calls change the value at the expected rate). So that puts its reference to about now - 7 days. All other tested platforms seemed to be seconds since boot.

I also built on RPi4 buster and it works there. liblsl-1.14.0-buster_arm.gz

cboulay commented 3 years ago

I couldn't install the package for ubuntu 20.04 on WSL2. That's something we can look into later.

chkothe commented 3 years ago

I think it's good that we're scrutinizing this area very closely, but I agree that it's subtle. A couple of q's:

In case we were to switch the epoch, that would not be a decision to make lightly, because, while the time correction would take care of it (ignoring numeric precision issues with large time deltas), the previously valid assumption that LSL apps running on the same machine are already synchronous without having to invoke time correction features will no longer apply when mixing different lib versions (eg older vendor software and apps pulled from github releases).

Not that it would break the world, but there are pretty sure a lot of user codes and lab setups out in the wild that assume so (perhaps unknowingly), and which will break when they update an app to a newer version (and in the worst case they'll only find out after a whole study has been collected).

cboulay commented 3 years ago

In Windows, I looked at the timestamps produced by this PR's ./lslver and by pylsl.local_clock() and they were using the same epoch despite pylsl using a previous version of liblsl lacking this PR's change. This doesn't guarantee they use the same clock of course.

chkothe commented 3 years ago

@cboulay ok that's good to know, I guess the way reboots are handled in recent Win versions must have gotten a bit more funky than it used to be. But if behavior is consistent with old lib vers, that's a good sign (could also compare to an archived lib build, e.g. from the old ftp).

Wasn't it the case that some recent liblsl build used time since 1970 on Mac OS? (I might have misread that) If so and if we're back to time since boot, and the 1970 phase was just a short blip, that's perhaps a good thing for compatibility

tstenner commented 3 years ago

I couldn't install the package for ubuntu 20.04 on WSL2. That's something we can look into later.

Do you have pugixml installed? The newer debian packages use the system pugixml and dpkg doesn't install the dependency by itself (but apt does).

Do we have an idea of what's the epoch of this clock on the respective platforms (Win/Linux/Mac/some ARMs)? Do we use/support any compilers that make inconsistent choices there? For reference, older LSL lib vers had time since boot on Windows (using QueryPerformanceCounter), Linux (using CLOCK_MONOTONIC), and Mac (using mach_absolute_time()) IIRC.

Ever since Boost.Chrono it has always been implementation defined, but mostly as in the list above.

Is there an effective change in epoch from previous lib versions?

Hopefully not, but if it is it's a change for the better in case the old clock was using the system clock, i.e. time since the unix epoch, as it's affected by changes in the clock, e.g. by NTP or time zone changes.

Also, for Windows specifically, can we confirm that it's not using the clock that has 10ms resolution (I guess by diffing successive clock readouts in a tight loop)? (I'm pretty sure it's not using that one, since that's the system / non-steady clock but best to be sure)

I have added a unit test for it. On Linux it's below 70ns, for OS X and Windows the CI will complain if it's above 1ms.

tstenner commented 3 years ago

@cboulay @chkothe Any objections to me merging this?

cboulay commented 3 years ago

Just found this link.

Beginning in Visual Studio 2015, the implementation of steady_clock has changed to meet the C++ Standard requirements for steadiness and monotonicity. steady_clock is now based on QueryPerformanceCounter() and high_resolution_clock is now a typedef for steady_clock. As a result, in the Microsoft C++ compiler steady_clock::time_point is now a typedef for chrono::time_point; however, this rule isn't necessarily the case for other implementations.

So on Windows from VS 2015 and above, this change will not make any difference except maybe improve precision slightly due to the improved conversion from nanoseconds to seconds. I believe the same is for Xcode.

I guess whether or not we have to worry about the pre VS 2015 issue, and maybe old gcc vs new clang, depends on what liblsl does when both applications are running on the same host. For example, the network stack will take some shortcuts for localhost vs a real ip-address. Do we do the same with clock_offsets? If clock_offsets are always calculated naively regardless of if the 2 streams are on the same host then it should be fine to use different clocks in different instances of liblsl on the same machine, right?

tstenner commented 3 years ago

Do we do the same with clock_offsets?

No, not even when both streams are coming from the same process. That's unfortunate when two streams belong together (e.g. one stream for EEG data and one for markers), but in this case it's actually nice this isn't done.

chkothe commented 3 years ago

@tstenner as you say it sounds like it's an improvement from some earlier versions. Re VS2013 or earlier, we could add an exclusion criterion there to prevent those specific compilers from being used.

tstenner commented 3 years ago

Re VS2013 or earlier, we could add an exclusion criterion there to prevent those specific compilers from being used.

I haven't tested it, but I think Boost / liblsl uses parts of C++11 VS2013 doesn't support. The docs list VS2015 as the oldest supported compiler version, but I have added a check just to be sure.

chkothe commented 3 years ago

OK, looks good to go to to me!

cboulay commented 3 years ago

@tstenner What happened with this?

tstenner commented 3 years ago

Github didn't acknowledge that I merged it from the command line so I closed the PR manually.