[rostime] add SteadyTime #57

flixr · 2017-03-03T18:11:21Z

Add a new SteadyTime wich uses a steady/monotonic clock to prevent issues with time jumps.

Replacement for #55 with MonotonicTime renamed to SteadyTime and added Windows implementation.

flixr · 2017-03-03T18:14:08Z

I didn't want to use the C++11 features here as I'm not sure if we can/should use them for roscpp yet.
And I'm hoping this will also applied to indigo (we are currently stuck on Ubuntu 14.04 on the Nvidia Jetson, as there are no cuda drivers for newer Ubuntu versions), where a C++11 compatible compiler is not yet required.

dirk-thomas

Please add unit tests covering the new functionality.

dirk-thomas · 2017-03-07T22:57:30Z

rostime/include/ros/time.h

+  {
+    public:
+      SteadyTime()
+              : TimeBase<SteadyTime, WallDuration>()


Please use two space indentation to be consistent with the existing code.

dirk-thomas · 2017-03-07T22:57:37Z

rostime/src/time.cpp

@@ -93,8 +93,8 @@ namespace ros
   * These have only internal linkage to this translation unit.
   * (i.e. not exposed to users of the time classes)
   */
-  void ros_walltime(uint32_t& sec, uint32_t& nsec) 
-#ifndef WIN32    
+  void ros_walltime(uint32_t& sec, uint32_t& nsec)


Please avoid any unrelated whitespace changes in your PR.

flixr · 2017-03-08T15:43:24Z

Fixed the whitespace stuff... will look into creating unit tests, although I have no idea on how to check if time is really monotonic, since I don't think we can reset the system time in the tests...

Add a new SteadyTime wich uses a steady/monotonic clock to prevent issues with time jumps.

flixr · 2017-03-20T11:02:51Z

Anything else to do here?

flixr · 2017-03-20T11:04:21Z

Is it OK to use WallDuration with SteadyTime or should we add a SteadyDuration as well? There would be no difference between WallDuration and SteadyDuration though...

dirk-thomas · 2017-03-20T17:41:36Z

I think most importantly this needs to be tested in context of the full stack up to bond_core to make sure it satisfies the desired use case. @mikaelarguedas FYI

mikaelarguedas · 2017-03-25T02:40:31Z

I tested the changes to use SteadyTimer all the way up to bond_core. It works as expected (bonds don't break on forward time jumps, but if you jump back in time the bond will stay alive until the deadline is reached). I did notice a few unexpected nodelet crashes when using this so I'll do some more testing on Monday before +1 ing this.

mikaelarguedas · 2017-03-27T19:01:13Z

Update:
Looks like there are still situations where time jumps cause this to fail:

[DEBUG] [1490639010.653929481]: SteadyTimer deregistering callbacks.
[DEBUG] [1490643600.000426555]: Time jumped forward by [25.143854] for timer of period [1.000000], resetting timer (current=1524518.175054, next_expected=1524493.031200)
[DEBUG] [1490643600.000492923]: Time jumped forward by [25.042610] for timer of period [1.000000], resetting timer (current=1524518.175106, next_expected=1524493.132497)
[ INFO] [1490643600.087787003]: Bond broken, exiting

It shows up only with nodelets but not with simple processes tied together with a bond. Needs more investigation.

Can be reproduced by running:

roslaunch nodelet_tutorial_math plus.launch

and jumping system time forward:

sudo date --set="2017-03-28 14:00:00.000"

flixr · 2017-03-29T14:24:22Z

@mikaelarguedas thanks a lot for testing!

The SteadyTime class of this PR should be ok, but I added a fix for the SteadyTimer in ros/ros_comm#1014

flixr · 2017-04-04T15:45:41Z

@mikaelarguedas any updates here?
We have these 3 patchsets running here on all our devices and haven't encountered any problems so far.

mikaelarguedas · 2017-04-04T15:49:30Z

@flixr Sorry didn't have time to do another round of testing on this. Did you perform tests with nodelets as well or only on simple bond connection ? (IIRC I faced problem only with nodelets and multiple forward time jumps)

I'll try to test this today or tomorrow and will report here

flixr · 2017-04-04T18:39:14Z

I tested the SteadyTimer callbacks directly and nodelets...

mikaelarguedas · 2017-04-06T23:36:25Z

I tested the same here (cb, bonds only and nodelets) and couldn't make if fail. 👏 🎉 thanks for pounding on this that's a great improvement! 🚀
@dirk-thomas What is the porting strategy you consider here ? Do you consider back-porting this to all active distros ?

dirk-thomas · 2017-04-07T16:48:39Z

rostime/src/time.cpp

@@ -167,6 +167,40 @@ namespace ros
    nsec = nsec_sum;
 #endif
  }
+
+  void ros_steadytime(uint32_t& sec, uint32_t& nsec)
+#ifndef WIN32


This condition seems to be inverted. The exception is thrown in the Windows block.

I just copied that whole part from the implemtation of ros_walltime...

dirk-thomas · 2017-04-07T16:49:19Z

rostime/src/time.cpp

+    sec  = start.tv_sec;
+    nsec = start.tv_nsec;
+#else
+    static LARGE_INTEGER cpu_frequency, performance_count;


Why should these variables be static? A local variable would avoid problems of the static variable being used by multiple concurrent callers.

Same as above: basically just copied that from ros_walltime...

dirk-thomas · 2017-04-07T16:49:52Z

rostime/src/time.cpp

+    //      https://msdn.microsoft.com/en-us/library/windows/desktop/ms644904(v=vs.85).aspx:
+    // "On systems that run Windows XP or later, the function will always succeed and will
+    //  thus never return zero."
+    QueryPerformanceFrequency(&cpu_frequency);


The return value of the function should be checked.

Same as above: basically just copied that from ros_walltime...

dirk-thomas · 2017-04-07T16:50:09Z

rostime/src/time.cpp

+    if (cpu_frequency.QuadPart == 0) {
+      throw NoHighPerformanceTimersException();
+    }
+    QueryPerformanceCounter(&performance_count);


The return value of the function should be checked.

dirk-thomas · 2017-04-07T16:57:13Z

The change in this repo is easier since it only adds API. The PRs in other repos building on top of this are more challenging since they change behavior. Therefore I plan to roll them out distro by distro. So first Lunar and Kinetic, waiting for PRs on top to also be released, wait until all changes are in the public repo and let it soak there for some time. Then consider doing the backports for Indigo and Jade.

flixr · 2017-04-10T19:43:19Z

@dirk-thomas I would propose to merge this as is...
If you want further changes in the windows implementation, it should IMHO be done in a separate PR as the same should be applied to the already existing ros_walltime function and tested by someone on Windows...

flixr · 2017-04-22T13:30:21Z

@dirk-thomas any updates here?
I can't really test the windows code... but we have these patches running on our systems for a while now without any problems, so is there any reason to not merge this yet?

flixr · 2017-05-02T18:50:34Z

@dirk-thomas @mikaelarguedas ping...

dirk-thomas · 2017-05-15T16:53:08Z

Sorry for the delay - very busy times 😟

I will merge this and release a new version into Lunar in order to test the ros_comm PR.

dirk-thomas · 2017-05-15T16:54:41Z

@flixr Thank you for working on this feature!

flixr mentioned this pull request Mar 3, 2017

add MonotonicTime for use with timeouts #55

Closed

This was referenced Mar 6, 2017

Add SteadyTimer ros/ros_comm#1014

Merged

Use SteadyTime and SteadyTimer for bond timeouts ros/bond_core#18

Merged

dirk-thomas reviewed Mar 7, 2017

View reviewed changes

dirk-thomas added the requires-changes label Mar 7, 2017

flixr force-pushed the steady_time branch from 762341a to 681ce9d Compare March 8, 2017 15:41

flixr added 2 commits March 8, 2017 18:23

[rostime] add SteadyTime

426fc3f

Add a new SteadyTime wich uses a steady/monotonic clock to prevent issues with time jumps.

add some unit tests for SteadyTime

3126a28

flixr force-pushed the steady_time branch from 681ce9d to 3126a28 Compare March 8, 2017 17:24

dirk-thomas removed the requires-changes label Mar 8, 2017

dirk-thomas reviewed Apr 7, 2017

View reviewed changes

try to fix ros_steadytime for windows

96865f3

flixr mentioned this pull request Apr 15, 2017

Wall time should use CLOCK_MONOTONIC instead of CLOCK_REALTIME which is vulnerable to leapseconds #33

Closed

dirk-thomas added the enhancement label May 15, 2017

dirk-thomas merged commit 089d553 into ros:kinetic-devel May 15, 2017

wjwwood mentioned this pull request Jun 6, 2017

add logic to support steady time on macOS #59

Merged

flixr deleted the steady_time branch December 13, 2018 18:00

mathias-luedtke mentioned this pull request Feb 12, 2019

returned PREEMPTED or RECALLED if the system time is rolled back with… ros/actionlib#118

Closed

alvarovillena mentioned this pull request Feb 27, 2023

Use SteadyTime instead of Time to avoid jumps back in time when using NTP clock RobotnikAutomation/vectornav#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rostime] add SteadyTime #57

[rostime] add SteadyTime #57

flixr commented Mar 3, 2017

flixr commented Mar 3, 2017

dirk-thomas left a comment

dirk-thomas Mar 7, 2017

dirk-thomas Mar 7, 2017

flixr commented Mar 8, 2017

flixr commented Mar 20, 2017

flixr commented Mar 20, 2017

dirk-thomas commented Mar 20, 2017

mikaelarguedas commented Mar 25, 2017

mikaelarguedas commented Mar 27, 2017

flixr commented Mar 29, 2017

flixr commented Apr 4, 2017

mikaelarguedas commented Apr 4, 2017

flixr commented Apr 4, 2017

mikaelarguedas commented Apr 6, 2017

dirk-thomas Apr 7, 2017

flixr Apr 7, 2017

dirk-thomas Apr 7, 2017

flixr Apr 7, 2017

dirk-thomas Apr 7, 2017

flixr Apr 7, 2017

dirk-thomas Apr 7, 2017

dirk-thomas commented Apr 7, 2017

flixr commented Apr 10, 2017

flixr commented Apr 22, 2017

flixr commented May 2, 2017

dirk-thomas commented May 15, 2017

dirk-thomas commented May 15, 2017

[rostime] add SteadyTime #57

[rostime] add SteadyTime #57

Conversation

flixr commented Mar 3, 2017

flixr commented Mar 3, 2017

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flixr commented Mar 8, 2017

flixr commented Mar 20, 2017

flixr commented Mar 20, 2017

dirk-thomas commented Mar 20, 2017

mikaelarguedas commented Mar 25, 2017

mikaelarguedas commented Mar 27, 2017

flixr commented Mar 29, 2017

flixr commented Apr 4, 2017

mikaelarguedas commented Apr 4, 2017

flixr commented Apr 4, 2017

mikaelarguedas commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Apr 7, 2017

flixr commented Apr 10, 2017

flixr commented Apr 22, 2017

flixr commented May 2, 2017

dirk-thomas commented May 15, 2017

dirk-thomas commented May 15, 2017