Skip to content

Commit

Permalink
use wait_for(), remove TimerManagerTraits
Browse files Browse the repository at this point in the history
  • Loading branch information
dirk-thomas committed Feb 6, 2020
1 parent 7d470f3 commit 2a67cf6
Showing 1 changed file with 2 additions and 18 deletions.
20 changes: 2 additions & 18 deletions clients/roscpp/include/ros/timer_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,6 @@
namespace ros
{

namespace {
template<class T>
class TimerManagerTraits
{
public:
typedef boost::chrono::system_clock::time_point time_point;
};

template<>
class TimerManagerTraits<SteadyTime>
{
public:
typedef boost::chrono::steady_clock::time_point time_point;
};
}

template<class T, class D, class E>
class TimerManager
{
Expand Down Expand Up @@ -592,8 +576,8 @@ void TimerManager<T, D, E>::threadFunc()
{
// On system time we can simply sleep for the rest of the wait time, since anything else requiring processing will
// signal the condition variable
typename TimerManagerTraits<T>::time_point end_tp(boost::chrono::nanoseconds(sleep_end.toNSec()));
timers_cond_.wait_until(lock, end_tp);
int64_t remaining_time = std::max<int64_t>((sleep_end - current).toSec() * 1000.0f, 1);
timers_cond_.wait_for(lock, boost::chrono::milliseconds(remaining_time));

This comment has been minimized.

Copy link
@flixr

flixr Feb 6, 2020

Contributor

To be clear, replacing wait_until(sleep_end) with wait_for(sleep_end - current) is not exactly the same.
But I guess for how these Timers are supposed to be used it's ok.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Feb 6, 2020

Author Member

Can you elaborate how they are different?

This comment has been minimized.

Copy link
@flixr

flixr Feb 6, 2020

Contributor

You end up with a slightly different (steady) timepoint that you wait until...
Because wait_for calls wait_until again under the hood, using wait_for here means it waits a tiny bit longer than strictly wanted, as it basically creates a new sleep end timepoint from the duration (sleep_end - current) created a tiny bit earlier.
But I think we can assume that it is very unlikely that there are significant delays (something else might be scheduled) in between when current is set and wait_for is called with (sleep_end - current).

Also of course it is not guaranteed how quickly after the end timepoint it will actually wake up and since these timers are not (and can't be expected to) used for high precision timing, this seems to be fine four our case here.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Feb 7, 2020

Author Member

Ok, I think I understand the difference now. Calling now() again with the chance of letting the end tp drift even a tiny bit isn't nice. I will just revert this commit and keep the wait_until() call previously proposed.

}
}

Expand Down

0 comments on commit 2a67cf6

Please sign in to comment.