-
Notifications
You must be signed in to change notification settings - Fork 116
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
[rostime] add SteadyTime #57
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests covering the new functionality.
rostime/include/ros/time.h
Outdated
{ | ||
public: | ||
SteadyTime() | ||
: TimeBase<SteadyTime, WallDuration>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use two space indentation to be consistent with the existing code.
rostime/src/time.cpp
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid any unrelated whitespace changes in your PR.
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.
Anything else to do here? |
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... |
I think most importantly this needs to be tested in context of the full stack up to |
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. |
Update:
It shows up only with nodelets but not with simple processes tied together with a bond. Needs more investigation. Can be reproduced by running:
and jumping system time forward:
|
@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 |
@mikaelarguedas any updates here? |
@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 |
I tested the SteadyTimer callbacks directly and nodelets... |
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! 🚀 |
@@ -167,6 +167,40 @@ namespace ros | |||
nsec = nsec_sum; | |||
#endif | |||
} | |||
|
|||
void ros_steadytime(uint32_t& sec, uint32_t& nsec) | |||
#ifndef WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition seems to be inverted. The exception is thrown in the Windows block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied that whole part from the implemtation of ros_walltime...
sec = start.tv_sec; | ||
nsec = start.tv_nsec; | ||
#else | ||
static LARGE_INTEGER cpu_frequency, performance_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should these variables be static? A local variable would avoid problems of the static variable being used by multiple concurrent callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: basically just copied that from ros_walltime...
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of the function should be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: basically just copied that from ros_walltime...
if (cpu_frequency.QuadPart == 0) { | ||
throw NoHighPerformanceTimersException(); | ||
} | ||
QueryPerformanceCounter(&performance_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of the function should be checked.
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. |
@dirk-thomas I would propose to merge this as is... |
@dirk-thomas any updates here? |
@dirk-thomas @mikaelarguedas ping... |
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. |
@flixr Thank you for working on this feature! |
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.