Add SteadyTimer #1014
Add SteadyTimer #1014
Conversation
There was a problem hiding this comment.
Beside the inline comments I do have a few more:
-
Why should the boost headers be pulled into this repo?
-
steady_clockseems to be only used in the.cppfiles but not being exposed thorugh any idea. If that is the case why not use the native type available as of C++11? -
Please add unit tests covering the new functionality.
-
The CI tests failed for what looks like a simple compiler error.
clients/roscpp/CMakeLists.txt
Outdated
| @@ -70,7 +70,7 @@ CHECK_FUNCTION_EXISTS(trunc HAVE_TRUNC) | |||
| configure_file(${CMAKE_CURRENT_SOURCE_DIR}/src/libros/config.h.in ${CMAKE_CURRENT_BINARY_DIR}/config.h) | |||
| include_directories(${CMAKE_CURRENT_BINARY_DIR}) | |||
|
|
|||
| add_library(roscpp | |||
| add_library(roscpp | |||
There was a problem hiding this comment.
Please avoid any unrelated whitespace changes in your PR.
| @@ -0,0 +1,127 @@ | |||
| /* | |||
| * Copyright (C) 2009, Willow Garage, Inc. | |||
There was a problem hiding this comment.
This should not be "2009, Willow Garage, Inc.".
There was a problem hiding this comment.
Help @flixr out, what should it be (at least some guidelines)?
There was a problem hiding this comment.
The year the code snippet was written and the author / company who wrote it.
That's probably my fault. I suggested in another pr that we might not want to use C++11 features in the core yet, but that was mostly because I didn't know how you felt about it. For instance would we need to add the |
|
If the C++11 specific features are only used in |
|
What I meant is that you can have ABI problems if roscpp is compiled with c++11 but other libraries are not and they both use STL types across the boundary. This usually isn't an issue for gcc/clang on unix systems, but it's not guaranteed either (at least as far as I know). |
The only working implementation I could find was the one in boost > 1.61, see description. I'm not particularly fond of this kludge to include the boost headers from 1.61 if the system version is not recent enough, but I couldn't think of another way (short of providing our own condition variable implementation). Any better ideas?
Sorry, don't quite understand which part you mean here... Regarding C++11:
The CI tests obviously fail because ros/roscpp_core#57 is not yet merged and hence |
flixr
force-pushed
the
steady_timer
branch
2 times, most recently
from
daee44c to
48169e5
Compare
based on SteadyTime (which uses the CLOCK_MONOTONIC). This timer is not influenced by time jumps of the system time, so ideal for things like periodic checks of timeout/heartbeat, etc...
This is a bit of a hack, since up to boost version 1.61 the time of the steady clock is always converted to system clock, which is then used for the wait... which obviously doesn't help if we explicitly want the steady clock. So as a workaround, include the backported versions of the boost condition variable if boost version is not recent enough.
not sure if this is only need in my case on ROS indigo...
|
Added some unit tests, but to make (the existing) tests compile I had to add the |
|
Also please note that the added unit tests (basically the same as for WallTimer) don't cover the actually important aspect of the SteadyTimer: changing the system time doesn't/shouldn't affect it. |
Sorry, I missed this obvious reason. It indeed requires
Let me try to rephrase my question and ask a few more specific ones:
|
I think @flixr said that these don't work in some version of gcc's standard library (that presumably we're using in Indigo?):
|
Yes.
Basically yes... but since this code is the generic timer_manager.h (which is actually also installed), the boost headers also need to be exposed/installed...
That is actually what I implemented at first, but it didn't work: The boost implementation from 1.61 simply always uses CLOCK_MONOTONIC for initializing the condition variable if you have Also I added another commit that actually uses SteadyTime for some timeouts and the same fix for using a steady/monotonic timer for the internal timer manager. |
|
@flixr Thank you for the quick answers.
If we want to target this for Lunar we need to put the headers in this repo since Xenial doesn't have a new enough boost version (they could be placed in a location though which isn't being installed / exposed to downstream packages). The alternative would be to only target ROS M-turtle for this change (which is over a year out, May 2018, I know a long time). @ros/ros_team @ros/ros_comm-maintainers Please let me know what you think. |
That would work if
I would strongly recommend to backport this to all active ROS versions as IMHO this makes it impossible to build robust real world ROS systems (where the clock might be adjusted by NTP, PTP, etc..). There are probably also a couple of other places where we should really use SteadyTime or a SteadyTimer instead of WallTime(r). |
|
The implementation of SteadyTimer will be very useful to prevent nodelets from crashing on time jumps (c.f. ros/bond_core#16). So it would be awesome to get this in before ROS-M. |
| // we have to distinguish between SteadyTime and WallTime here, | ||
| // because boost timed_wait with duration just adds the duration to current system time | ||
| // this however requires boost 1.61, see: https://svn.boost.org/trac/boost/ticket/6377 | ||
| if (typeid(T) == typeid(SteadyTime)) |
There was a problem hiding this comment.
This check prevents subclassing SteadyTime. I am not sure if that is a valid use case but directly comparing typeid here looks problematic to me.
There was a problem hiding this comment.
Yeah, but I don't have a better idea.... anyone else?
There was a problem hiding this comment.
If you want to check the type T and only accept exactly SteadyTime. That could be done with some template meta programming since the function is already templated on T.
Or the SteadyTime class could have a method which return true for that class (and false for the other variations). It might not be possible to introduce that in an ABI compatible fashion which would prevent backporting the change.
There was a problem hiding this comment.
Thanks, added a specialization for the SteadyTimer in the cpp... also fixed it for the case when the SteadyTimer is created via the node handle (somehow didn't use the backported boost version in that case before)
to avoid the typeid check and make really sure the correct boost condition wait_until implementation is used
|
Given that this is merged and will be released only for Lunar, ros/bond_core#18 will need some extra work to support it |
|
ping, what is the holdup here? Would be great to finally get this merged... |
|
I just need to find the time to do ROS 1 specific work. Go through PRs, merge them, and release a new version. But currently I have to focus on other projects. I can't tell when I will get to this - hopefully within a month. |
|
I finally was able to get back to this. I created flixr#1 to update the patch to specify the minimum version of Thank you for you effort on this great improvement. |
The important change is the minimum version for the dependency on rostime to ensure the version contains SteadyTime. The other changes are mostly spaces.
|
At least one of the tests ( |
* add SteadyTimer based on SteadyTime (which uses the CLOCK_MONOTONIC). This timer is not influenced by time jumps of the system time, so ideal for things like periodic checks of timeout/heartbeat, etc... * fix timer_manager to really use a steady clock when needed This is a bit of a hack, since up to boost version 1.61 the time of the steady clock is always converted to system clock, which is then used for the wait... which obviously doesn't help if we explicitly want the steady clock. So as a workaround, include the backported versions of the boost condition variable if boost version is not recent enough. * add tests for SteadyTimer * [test] add -pthread to make some tests compile not sure if this is only need in my case on ROS indigo... * use SteadyTime for some timeouts * add some checks to make sure the backported boost headers are included if needed * specialize TimerManager threadFunc for SteadyTimer to avoid the typeid check and make really sure the correct boost condition wait_until implementation is used * Revert "[test] add -pthread to make some tests compile" This reverts commit f62a3f2. * set minimum version for rostime * mostly spaces
|
@mikaelarguedas @dirk-thomas how about porting this back to at least kinetic and preferably even indigo? |
|
Backporting to Kinetic sounds good to me. I don't think this needs to go into Indigo. Indigo has been released 3.5 yrs ago, new features are not necessary to be backported to it. |
|
Same thing for the corresponding PR on bond_core, I'm fine backporting it to kinetic because it touches only private functions signature. I agree with @dirk-thomas and wouldn't plan on backporting it to indigo |
* add SteadyTimer based on SteadyTime (which uses the CLOCK_MONOTONIC). This timer is not influenced by time jumps of the system time, so ideal for things like periodic checks of timeout/heartbeat, etc... * fix timer_manager to really use a steady clock when needed This is a bit of a hack, since up to boost version 1.61 the time of the steady clock is always converted to system clock, which is then used for the wait... which obviously doesn't help if we explicitly want the steady clock. So as a workaround, include the backported versions of the boost condition variable if boost version is not recent enough. * add tests for SteadyTimer * [test] add -pthread to make some tests compile not sure if this is only need in my case on ROS indigo... * use SteadyTime for some timeouts * add some checks to make sure the backported boost headers are included if needed * specialize TimerManager threadFunc for SteadyTimer to avoid the typeid check and make really sure the correct boost condition wait_until implementation is used * Revert "[test] add -pthread to make some tests compile" This reverts commit f62a3f2. * set minimum version for rostime * mostly spaces
* fix open mode on Windows * Fix BZip2 inclusion * Respect if/unless for roslaunch-check. * fix rosmsg show from bag * fix rosbag::View::iterator copy assignment operator (ros#1017) * refactor test_rosbag_storage * fix rosbag::View::iterator copy assignment operator the compiler-generated copy assignment operator did lead to segfault and memory leaks. * Add subscriber to connection log messages. (ros#1023) * ensure cwd exists * Sleep in rospy wait_for_service even if exceptions raised * Avoid deleting XmlRpcClient's while they are being still in use on another thread (ros#1013) * Avoid deleting XmlRpcClient's while they are being still in use on another thread * Acquire clients_mutex_ before deleting the clients * Remove the timed wait for the clients to become not in use * Only delete and erase clients that are not in use * Clients that would still be in use would delete themselves on release * Wait for clients that are in use to finish in XmlRpcManager::shutdown * Abort topic lookup on connection refused In a multimaster environment where a topic has multiple publishers, when a node drops out abruptly (host is shutdown), a single subscriber update on that topic will cause multiple threads to be created (one for each host) in order to resolve the topic location. This cause a thread leak as host which are turned off will not respond and when they come back online, the xmlrpc URI is changed causing a connection refused error at the socket layer. This fix catches the connection refused error and terminate the thread with the understanding that if the connection is refused, the rosnode cannot be reached now or never. This effectively prevents thread leak. Note: if the remote host where the rosnode is thought to be never comes back up, then the thread will still be leaked as the exception received is a host unreachable type. This is intentional to avoid abruptly terminating the thread in case of a temporary DNS failure. * Fix bug in transport_tcp (ros#1050) * Fix bug in transport_tcp It assumes that the `connect` method of non-blocking scoket should return -1 and `last_socket_error()` should return `ROS_SOCKETS_ASYNCHRONOUS_CONNECT_RETURN`(=`EINPROGRESS`). But a non-blocking `connect` can return 0 when TCP connection to 127.0.0.1 (localhost). [http://stackoverflow.com/questions/14027326/can-connect-return-0-with-non-blocing-socket](http://stackoverflow.com/questions/14027326/can-connect-return-0-with-non-blocing-socket) * Modify code format Modify code format * Fix race condition that lead to miss first message (lunar) (ros#1058) * Fix race condition that lead to miss first message Callback queue waits for callback from "callOne" method. When internal queue is not empty this last method succeeded even if id info mapping does not contains related callback's id. In this case, first callback (for one id) is never called since "addCallback" method first push callback into internal queue and *then* set info mapping. So id info mapping has to be set before push callback into internal queue. Otherwise first message could be ignored. * Added test for addCallback race condition * ensure pid file is removed on exit * Changed the check command output to be a little bit more specific. * [roslaunch] Fix pid file removing condition * [rospy] Add option to reset timer when time moved backwards (ros#1083) * Add option to reset timer when time moved backwards * refactor logic * add missing mutex lock for publisher links * [rospy] Improve rospy.logXXX_throttle performance * Added logging output when `roslogging` cannot change permissions (ros#1068) * Added logging output when `roslogging` cannot change permissions Added better differentiated logging output to `roslogging` so that problems with permission is made clear to the user. Accompanying test have also been added. * Removed testing, updated warning message and fixed formatting Removed testing since test folder should not be stored together with tests. Since testing group permission requires intervention outside the test harness the test it self is also removed. Updated the warning message to include `WARNING` and updated to using `%` formatting. * [rostest] Check /clock publication neatly in publishtest (ros#973) * Check /clock publication neatly in publishtest - Use time.sleep because rospy.sleep(0.1) hangs if /clock is not published - Add timeout for clock publication * Add comment explaining use of time.sleep. * Use logwarn_throttle not to flood the console * A fix to a critical stack buffer overflow vulnerability which leads to direct control flow hijacking (ros#1092) * A fix to a critical stack buffer overflow vulnerability which leads to control flow hi-jacking. * Much more simple fix for the stack overflow bug * only launch core nodes if master was launched by roslaunch * Made copying rosbag::Bag a compiler error to prevent crashes and added a swap function instead (ros#1000) * Made copying rosbag::Bag a compiler error to prevent crashes * Added Bag::swap(Bag&) and rosbag::swap(Bag&, Bag&) * Fixed bugs in Bag::swap * Added tests for Bag::swap * [roscpp] add missing header for writev(). After an update of gcc and glibc roscpp started to fail builds with the error: /home/rojkov/work/ros/build/tmp-glibc/work/i586-oe-linux/roscpp/1.11.21-r0/ros_comm-1.11.21/clients/roscpp/src/libros/transport/transport_udp.cpp:579:25: error: 'writev' was not declared in this scope ssize_t num_bytes = writev(sock_, iov, 2); ^~~~~~ According to POSIX.1-2001 the function writev() is declared in sys/uio.h. The patch includes the missing header for POSIX compliant systems. * Add SteadyTimer (ros#1014) * add SteadyTimer based on SteadyTime (which uses the CLOCK_MONOTONIC). This timer is not influenced by time jumps of the system time, so ideal for things like periodic checks of timeout/heartbeat, etc... * fix timer_manager to really use a steady clock when needed This is a bit of a hack, since up to boost version 1.61 the time of the steady clock is always converted to system clock, which is then used for the wait... which obviously doesn't help if we explicitly want the steady clock. So as a workaround, include the backported versions of the boost condition variable if boost version is not recent enough. * add tests for SteadyTimer * [test] add -pthread to make some tests compile not sure if this is only need in my case on ROS indigo... * use SteadyTime for some timeouts * add some checks to make sure the backported boost headers are included if needed * specialize TimerManager threadFunc for SteadyTimer to avoid the typeid check and make really sure the correct boost condition wait_until implementation is used * Revert "[test] add -pthread to make some tests compile" This reverts commit f62a3f2. * set minimum version for rostime * mostly spaces * Close CLOSE_WAIT sockets by default (ros#1104) * Add close_half_closed_sockets function * Call close_half_closed_sockets in xmlrpcapi by default * fix handling connections without indices * fix rostopic prining long integers * update tests to match stringify changes * ignore headers with zero stamp in statistics * Improves the stability of SteadyTimerHelper. Due to scheduling / resource contention, `sleep`s and `wait_until`s may be delayed. The `SteadyTimerHelper` test class was not robust to these delays, which was likely the cause of a failing test (`multipleSteadyTimeCallbacks` in `timer_callbacks.cpp`:220). * Improve steady timer tests (ros#1132) * improve SteadyTimer tests instead of checking when the callback was actually called, check for when it was added to the callback queue. This *should* make the test more reliable. * more tolerant and unified timer tests * xmlrpc_manager: use SteadyTime for timeout * Replaced deprecated lz4 function call * Removed deprecated dynamic exception specifications * only use CLOCK_MONOTONIC if not apple * Improved whitespace to fix g++ 7 warning .../ros_comm/tools/rosbag_storage/src/view.cpp:249:5: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] if ((bag.getMode() & bagmode::Read) != bagmode::Read) ^~ .../ros_comm/tools/rosbag_storage/src/view.cpp:252:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’ boost::function<bool(ConnectionInfo const*)> query = TrueQuery(); ^~~~~ * Using poll() in favor of select() in the XmlRPCDispatcher (ros#833) * Using poll() in favor of select() in the XmlRPCDispatcher * ros#832: initialize pollfd event with 0. Added check for POLLHUP and POLLNVAL * Fix syntax * poll flags emulate select, verify requests, sync/init sources/fds This commit makes sure that the poll flags emulate select (which it replaces). It also double-checks event types to make sure they were requested (e.g. POLLERR might trigger both). It keeps track of fd/src relationship through two parallel arrays, instead of an iterator / array hybrid. * Fix rostopic hz and bw in Python 3 (ros#1126) * string.atoi is not present in Python3, just use int(x) * rostopic bw: set default value of window_size arg to -1 instead of None * Check for window_size < 0 when constructing ROSTopicBandwidth object * Revert "Check for window_size < 0 when constructing ROSTopicBandwidth object" This reverts commit 86a2a29. * Revert "rostopic bw: set default value of window_size arg to -1 instead of None" This reverts commit 4c74df9. * Check for argument != None before calling int(options.window_size) * Properly check for options.window_size != None before converting to int * Don't direct users to build rosout with rosmake. (ros#1140) * Don't direct users to build rosout with rosmake. * Eliminate special case for rosout being missing. * use not deprecated console_bridge macros and undefine the deprecated ones * Fix rosbag API for Python 3 Closes ros#1047 * Sort the output of rosnode info. * Minor fixes for compatibility with both Python 2 & 3 * [bug] fixes ros#1158 (ros#1159) * [bug] fixes ros#1158 XmlLoader and LoaderContext no longer share the param list to child 'node' contexts. This was causing the creation of unintended private parameters when the tilde notation was used. * added test cases for tilde param in launch * added test cases for tilde param in python * fixed tilde param issue for group tags Issue ros#1158 manifested for group tags that appear before (but not containing) node tags. * added one more test case for issue ros#1158 used param tag to make sure we test the proposed fix * Added negative tests for extra parameters Some parameters should not be present at all. * rosconsole: replaced 'while(0)' by 'while(false)' * Change rospy.Rate hz type from int to float (ros#1177) * Don't try to set unknown socket options (ros#1172) * Don't try to set unknown socket options These are not avaible on FreeBSD, for example * individualize ifdefs * fix whitespace * rosnode: Return exit code 1 if there is an error. (ros#1178) * Fixed an out of bounds read in void rosbag::View::iterator::increment() (ros#1191) - Only triggered if reduce_overlap_ = true - When iters_.size() == 1 and iters_.pop_back() gets called in the loop, the next loop condition check would read from iters_.back(), but iters_ would be empty by then. * Test bzfile_ and lz4s_ before reading/writing/closing (ros#1183) * Test bzfile_ before reading/writing/closing * Test lz4stream before reading/writing * More agile demux. (ros#1196) * More agile demux. Publishers in demux are no longer destroyed and recreated when switching, which results in much faster switching behavior. The previous version took even 10 seconds to start publishing on the newly selected topic (all on localhost). Please, comment if you feel the default behavior should stay as the old was, and this new behavior should be triggered by a parameter. * update style * catch exception with `socket.TCP_INFO` on WSL (ros#1212) * catch exception with `socket.TCP_INFO` on WSL fixes issue ros#1207 this only catches socket error 92 * Update util.py * Update util.py * avoid unnecessary changes, change import order * fix path check * fix error message to mention what was actually tested * fix roswtf test when rosdep is not initialized * update changelogs * 1.12.8 * backward compatibility with libconsole-bridge in Jessie * update changelogs * 1.12.9 * backward compatibility with libconsole-bridge in Jessie (take 2) * update changelogs * 1.12.10 * Revert "Replaced deprecated lz4 function call" This reverts commit a31ddd0. * update changelogs * 1.12.11 * backward compatibility with libconsole-bridge in Jessie (take 3) (ros#1235) * backward compatibility with libconsole-bridge in Jessie (take 3) * copy-n-paste error * remove fragile undef * dont rely on existence of short version of the macros, define the long with API calls instead * remove empty line * update changelogs * 1.12.12 * place console_bridge macros definition in header and use it everywhere console_bridge is included (ros#1238)
…tion_variable (fix ros#1343) ros#1014 and ros#1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process.
…tion_variable (fix ros#1343) ros#1014 and ros#1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process.
…tion_variable (fix ros/ros_comm#1343) ros/ros_comm#1014 and ros/ros_comm#1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process.
…tion_variable (fix ros#1343) ros#1014 and ros#1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process.
…ait spinning (#1878) * roscpp: fix potential busy-wait loop caused by backported Boost condition_variable (fix #1343) #1014 and #1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process. * roscpp: use boost::condition_variable::wait_for() instead of deprecated timed_wait() This fixes ROS timers in combination with 2c18b9f. The timer callbacks were not called because the TimerManager's thread function blocked indefinitely on boost::condition_variable::timed_wait(). Relative timed_wait() uses the system clock (boost::get_system_time()) unconditionally to calculate the absolute timestamp for do_wait_until(). If the condition variable has been initialized with BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC, it compares this timestamp with the monotonic clock and therefore blocks. This issue has been reported in https://svn.boost.org/trac10/ticket/12728 and will not be fixed. The timed_wait interface is apparently deprecated. * roscpp: do not use boost_161_condition_variable.h on Windows (untested) * roscpp: remove specialized implementation of TimerManager<T,D,E>::threadFunc() in steady_timer.cpp The updated generic definition in timer_manager.h should do the same with a minor update. In all cases we can call boost::condition_variable::wait_until() with an absolute time_point of the respective clock. The conversion from system_clock to steady_clock for Time and WallTime is done internally in boost::condition_variable::wait_until(lock_type& lock, const chrono::time_point<Clock, Duration>& t). * fix namespaces * add more explicit namespaces * add missing ns * roscpp: fixed Boost version check in CMakeLists.txt find_package(Boost) has to come before checking the Boost version. Otherwise BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC was not defined which triggered the assertion in timer_manager.h:240. Since Boost 1.67 BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC became the default if the platform supports it and the macro is not defined anymore. Instead, check for BOOST_THREAD_INTERNAL_CLOCK_IS_MONO. * roscpp: replace ROSCPP_BOOST_CONDITION_VARIABLE and ROSCPP_BOOST_CONDITION_VARIABLE_HEADER macros by a typedef in internal_condition_variable.h * Remove copy of boost::condition_variable implementation from Boost 1.61 in namespace boost_161 * Revert some changes in include directives and in CMakeLists.txt to minimize the diff to melodic-devel Addresses #1878 (review). * use wait_for(), remove TimerManagerTraits * Revert "use wait_for(), remove TimerManagerTraits" This reverts commit 2a67cf6. Co-authored-by: Antoine Hoarau <[email protected]> Co-authored-by: Dirk Thomas <[email protected]>
…ait spinning (ros#1878) * roscpp: fix potential busy-wait loop caused by backported Boost condition_variable (fix ros#1343) ros#1014 and ros#1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process. * roscpp: use boost::condition_variable::wait_for() instead of deprecated timed_wait() This fixes ROS timers in combination with 2c18b9f. The timer callbacks were not called because the TimerManager's thread function blocked indefinitely on boost::condition_variable::timed_wait(). Relative timed_wait() uses the system clock (boost::get_system_time()) unconditionally to calculate the absolute timestamp for do_wait_until(). If the condition variable has been initialized with BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC, it compares this timestamp with the monotonic clock and therefore blocks. This issue has been reported in https://svn.boost.org/trac10/ticket/12728 and will not be fixed. The timed_wait interface is apparently deprecated. * roscpp: do not use boost_161_condition_variable.h on Windows (untested) * roscpp: remove specialized implementation of TimerManager<T,D,E>::threadFunc() in steady_timer.cpp The updated generic definition in timer_manager.h should do the same with a minor update. In all cases we can call boost::condition_variable::wait_until() with an absolute time_point of the respective clock. The conversion from system_clock to steady_clock for Time and WallTime is done internally in boost::condition_variable::wait_until(lock_type& lock, const chrono::time_point<Clock, Duration>& t). * fix namespaces * add more explicit namespaces * add missing ns * roscpp: fixed Boost version check in CMakeLists.txt find_package(Boost) has to come before checking the Boost version. Otherwise BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC was not defined which triggered the assertion in timer_manager.h:240. Since Boost 1.67 BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC became the default if the platform supports it and the macro is not defined anymore. Instead, check for BOOST_THREAD_INTERNAL_CLOCK_IS_MONO. * roscpp: replace ROSCPP_BOOST_CONDITION_VARIABLE and ROSCPP_BOOST_CONDITION_VARIABLE_HEADER macros by a typedef in internal_condition_variable.h * Remove copy of boost::condition_variable implementation from Boost 1.61 in namespace boost_161 * Revert some changes in include directives and in CMakeLists.txt to minimize the diff to melodic-devel Addresses ros#1878 (review). * use wait_for(), remove TimerManagerTraits * Revert "use wait_for(), remove TimerManagerTraits" This reverts commit 2a67cf6. Co-authored-by: Antoine Hoarau <[email protected]> Co-authored-by: Dirk Thomas <[email protected]>
…tonic clock [kinetic-devel] (#2011) * Drop custom implementation of boost::condition_variable to fix busy-wait spinning (#1878) * roscpp: fix potential busy-wait loop caused by backported Boost condition_variable (fix #1343) #1014 and #1250 introduced a backported version of boost::condition_variable, where support for steady (monotonic) clocks has been added in version 1.61. But the namespace of the backported version was not changed and the symbol names might clash with the original version. Because the underlying clock used for the condition_variable is set in the constructor and must be consistent with the the expectations within member variables. The compiler might choose to inline one or the other or both, and is more likely to do so for optimized Release builds. But if it does not, the symbol ends up in the symbol table of roscpp and depending on which other libraries will be linked into the process it is unpredictable which of the two versions will be actually called at the end. In case the constructor defined in `/usr/include/boost/thread/pthread/condition_variable.hpp` was called and did not configure the internal pthread condition variable for monotonic clock, each call to the backported do_wait_until() method with a monotonic timestamp will return immediately and hence causes `CallbackQueue::callOne(timeout)` or `CallbackQueue::callAvailable(timeout)` to return immediately. This patch changes the namespace of the backported condition_variable implementation to boost_161. This removes the ambiguity with the original definition if both are used in the same process. * roscpp: use boost::condition_variable::wait_for() instead of deprecated timed_wait() This fixes ROS timers in combination with 2c18b9f. The timer callbacks were not called because the TimerManager's thread function blocked indefinitely on boost::condition_variable::timed_wait(). Relative timed_wait() uses the system clock (boost::get_system_time()) unconditionally to calculate the absolute timestamp for do_wait_until(). If the condition variable has been initialized with BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC, it compares this timestamp with the monotonic clock and therefore blocks. This issue has been reported in https://svn.boost.org/trac10/ticket/12728 and will not be fixed. The timed_wait interface is apparently deprecated. * roscpp: do not use boost_161_condition_variable.h on Windows (untested) * roscpp: remove specialized implementation of TimerManager<T,D,E>::threadFunc() in steady_timer.cpp The updated generic definition in timer_manager.h should do the same with a minor update. In all cases we can call boost::condition_variable::wait_until() with an absolute time_point of the respective clock. The conversion from system_clock to steady_clock for Time and WallTime is done internally in boost::condition_variable::wait_until(lock_type& lock, const chrono::time_point<Clock, Duration>& t). * fix namespaces * add more explicit namespaces * add missing ns * roscpp: fixed Boost version check in CMakeLists.txt find_package(Boost) has to come before checking the Boost version. Otherwise BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC was not defined which triggered the assertion in timer_manager.h:240. Since Boost 1.67 BOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONIC became the default if the platform supports it and the macro is not defined anymore. Instead, check for BOOST_THREAD_INTERNAL_CLOCK_IS_MONO. * roscpp: replace ROSCPP_BOOST_CONDITION_VARIABLE and ROSCPP_BOOST_CONDITION_VARIABLE_HEADER macros by a typedef in internal_condition_variable.h * Remove copy of boost::condition_variable implementation from Boost 1.61 in namespace boost_161 * Revert some changes in include directives and in CMakeLists.txt to minimize the diff to melodic-devel Addresses #1878 (review). * use wait_for(), remove TimerManagerTraits * Revert "use wait_for(), remove TimerManagerTraits" This reverts commit 2a67cf6. Co-authored-by: Antoine Hoarau <[email protected]> Co-authored-by: Dirk Thomas <[email protected]> * Use an internal implementation of boost::condition_variable with monotonic clock (#1932) * Use an internal implementation of condition_variable with monotonic clock to avoid ODR violation * Fix build of ros/internal/condition_variable.h for Boost <1.65 * Fix "static_assert with no message is a C++17 extension" warning in ros/internal/condition_variable.h * roscpp: fix ros/internal/condition_variable.h for pre-C++11 compilers Co-authored-by: Antoine Hoarau <[email protected]> Co-authored-by: Dirk Thomas <[email protected]>
Add a SteadyTimer which is based on SteadyTime (which uses the CLOCK_MONOTONIC).
This timer is not influenced by time jumps of the system time, so ideal for things like periodic checks of timeout/heartbeat, etc...
The difficulty is that using
wait_fororwait_untilon a boost condition variable always uses the system clock until version 1.61 (see https://svn.boost.org/trac/boost/ticket/6377).Which obviously doesn't help for the SteadyTimer where we explicitly want the steady clock.
So as a workaround, include the backported versions of the boost condition variable if boost version is not recent enough.
This depends on ros/roscpp_core#57 where SteadyTime was added.
This also replaces #1003