remove messages that are newer than the newly added message #1438
Conversation
There was a problem hiding this comment.
Logic to clear the filter when looping bag playback might make sense but I don't think that this is the right approach. It assumes all messages arrive in order except for a loop which is not guarenteed. There could easily be multiple publishers and publishers with higher latency.
There is logic in tf2_ros to reset it's buffers if the clock jumps backwards. I would suggest looking at that sort of logic as that is much more clearly a time discontinuity and behavior should change. Rather than possibly a delayed message arriving into a synchronizer.
|
I agree that |
|
That looks better to me. Two comments though. With the additional use of And the logic should provide a warning about the reset. Otherwise this could be triggered regularly and all data would potentially be silently dropped. |
|
The message about the reset is printed as info, not as warning. From my point of view, clearing the buffer on jumps backward in time is an expected behaviour while the alternative of blocking the synchronisation (by not clearing the buffers) is unexpected. |
|
info output is fine too. It looks like this is being triggered in the current tests and causing a failure. And related to that we should add a unit test to explicitly test that this is working and won't regress in the future. |
|
The test fails because it not only scrambles the sequence of messages, but also the clock time that is supposed to be continuous: |
|
@tfoote Does this look all right to you now? |
|
@ros-pull-request-builder retest this please |
|
The C++ equivalent will likely only be part of the next ROS distro (see #1518 (comment)) I am not sure if you still want to wait with this patch to release them in sync or merge this into Melodic now? |
|
We can merge this now. Then at least people which are using the python interface will benefit from this. |
|
Sounds good to me. Thanks for the patch. |
* remove messages that are newer than the newly added message * use /clock as reference for detecting backward jumps * set time to initialised after constructing ApproximateTimeSynchronizer * notify user about clearing buffer * use continuous time for testing header-less message synchronisation * test buffer clearing when jumping back in time * avoid unrelated changes, remove double space
In some circumstances, when replaying a log in loop mode (-l) the synchronisation blocks because messages jump back in time when restarting the log. This commit removes messages that are newer than the newly added message, enabling to synchronise looped log files. This could optionally enabled via a parameter that gives the maximum tolerated backward jump in time.