NetworkClock: unblock all waiting threads if a network reset is detected

[Giulero]

This PR implements the suggestions of @traversaro (see #800 (comment)) to handle the time reset.

This fix should avoid the phenomena of threads stopping and resuming after the time at which the clock was previously reset has passed.

[traversaro]

Thanks @Giulero ! Can you add a file in https://github.com/robotology/yarp/tree/yarp-3.4/doc/release/yarp_3_4 to document this change? Thanks!

[traversaro]

Some improvements suggestions.

[traversaro]

@xEnVrE if you use the YARP_CLOCK for Gazebo simulations, I guess you could be interested also in this.

[Giulero]

Thanks @traversaro for the suggestions!

[drdanz]

I have a very small concern about this patch.

If, for example, someone defines a clock, similar to what @PeterBowman did in #2488 (comment) which is based on a non-monotonic clock, and the clock adjusts of a few microseconds, this patch will notice that _time < oldTime, and will unlock all threads. This might lead to unexpected behaviours, which may be very hard to debug and to reproduce.

I'd rather keep the thread blocked, and update the wake up time in this way: waiter_it->first = _time + (waiter_it->first - oldTime).

@Giulero and @traversaro what do you think?

[traversaro]

I think this make sense @drdanz ! @Giulero could you try this modification?

[Giulero]

I agree! I'll try the modification.

[Giulero]

@drdanz @traversaro I tried the modification but maybe I did something wrong.

Correct me if I'm wrong. From what I understood, a delay (of 1) is requested (waiter_it->first evolves in a quantized fashion) every time the condition waiter_it->first - _time < 1E-12 is fulfilled, checking which threads are sleeping.
If a reset occurs, waiter_if->first remains blocked on the last value (for example 100) until _time (starting now from 0) reaches 100.

Updating waiter_it->first = _time + (waiter_it->first - oldTime) will close the gap between waiter_it->first and the "new" _time when the reset occurs. However, waiter_it->first will grow by a delta=_time - oldTime every cycle (not in a quantized way as before) The condition waiter_it->first - _time < 1E-12 will not be met since waiter_it->first is delayed always by 1 w.r.t _time.

Even if it does not work, I commit the modification (f216964), since I probably just misunderstood the suggestion.

[traversaro]

Even if it does not work, I commit the modification (f216964), since I probably just misunderstood the suggestion.

Should't we simply modify first if a network reset is detected, and leave it as it is in the rest of the cycles?

[Giulero]

Should't we simply modify first if a network reset is detected, and leave it as it is in the rest of the cycles?

I think the same! Even if it seems that the condition clockResetDetected is just moved somewhere else, in this way.
In any case, we should define what network reset means. _time < oldTime doesn't work for non-monotonic clocks. Maybe something more generic as abs(_time - oldTime) > eps?

I also tried to modify first if a reset is detected.
What I checked is that first is modified when a reset is detected, but the cycle after goes back to the previous value (jumping from 0 to 100, for instance). I guess that another process is setting the value of first and we are modifying it just in that scope.

[drdanz]

@Giulero Friendly ping 😜
I think this patch does not require much work in order to be merged, please do not forget about this.

[drdanz]

The description here should also be updated...

[Giulero]

Thanks @drdanz :)
Done in ab17f0f.

[Giulero]

@drdanz sorry for the delay! Your suggestions implemented in 97ce64d.

[drdanz]

LGTM Now, thanks.

[drdanz]

Merged, thanks.