Skip to content
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

Resampled timestamps don't have constant interval #170

Closed
shsms opened this issue Jan 20, 2023 · 3 comments · Fixed by #255
Closed

Resampled timestamps don't have constant interval #170

shsms opened this issue Jan 20, 2023 · 3 comments · Fixed by #255
Assignees
Labels
part:data-pipeline Affects the data pipeline priority:high Address this as soon as possible type:bug Something isn't working
Milestone

Comments

@shsms
Copy link
Contributor

shsms commented Jan 20, 2023

What happened?

The resampler uses the trigger times from a frequenz.channels.Timer as the timestamp for resampled messages. But the Timer returns the clock time at which it triggered, and not a calculated timestamp that always has a constant interval, adjusting for any delays from the rest of the program, as seen below:

>>> import asyncio
>>> from frequenz.channels.util import Timer
>>> async def test():
...   async for t in Timer(1.0): print(t)
... 
>>> asyncio.run(test())
2023-01-20 15:43:30.898930+00:00
2023-01-20 15:43:31.900304+00:00
2023-01-20 15:43:32.901836+00:00
2023-01-20 15:43:33.903355+00:00
2023-01-20 15:43:34.904885+00:00
2023-01-20 15:43:35.906479+00:00

What did you expect instead?

Timestamps in subsequent messages from the resampler should all have a constant interval, equal to the given resampling interval.

Affected version(s)

v0.16.0

Affected part(s)

Data pipeline (part:data-pipeline)

Extra information

No response

@shsms shsms added priority:❓ We need to figure out how soon this should be addressed type:bug Something isn't working labels Jan 20, 2023
@keywordlabeler keywordlabeler bot added the part:data-pipeline Affects the data pipeline label Jan 20, 2023
@sahas-subramanian-frequenz sahas-subramanian-frequenz added priority:high Address this as soon as possible and removed priority:❓ We need to figure out how soon this should be addressed labels Jan 20, 2023
@shsms
Copy link
Contributor Author

shsms commented Jan 20, 2023

The Timer's behaviour of returning the clock time should ideally not change.

An easy fix would be to replace the use of Timer in the resampler with a custom implementation, that adjusts the time it sleeps based on how much time has elapsed since the last tick. But it should also be possible to add an optional setting to the Timer to implement the logic there instead.

@leandro-lucarella-frequenz
Copy link
Contributor

An easy fix would be to replace the use of Timer in the resampler with a custom implementation, that adjusts the time it sleeps based on how much time has elapsed since the last tick. But it should also be possible to add an optional setting to the Timer to implement the logic there instead.

There are 2 problems here, I think:

  1. The Timer doesn't adjust the next tick (I think it really should, why would you want to have a purposely drifting timer? I would be fine to keep that as an option but the default should be not to drift)
  2. The resampler doesn't keep track of the window size, so even if the timer doesn't drift, the timer timestamp will probably be always slightly in the future, so the resampler should always stick its own timestamp (based on the resampling period) to generated samples, not the timestamp provided by the timer. This one is fixed by Make resampling window size constant #236.

@leandro-lucarella-frequenz
Copy link
Contributor

OK, looking at the Timer implementation, I see that the timer is not started (or advanced) until you call ready() (or receive() or any derived method). I guess this makes sense, and what we really need is a periodic timer that will start when it is created and advance automatically after the timer has fired.

leandro-lucarella-frequenz pushed a commit that referenced this issue Mar 10, 2023
The resampler was using the timer timestamp to do the resampling, but
this is wrong because the timer could (and probably always will) fire in
the *future* (after the resampling period has passed).

The effect was that samples produced by the resampler were not perfectly
spread using an exact resampling period, but they included the error
produced by the timer firing

This PR fixes this issue and also fixes another bug found, samples in
the future could have been included in a resampling window. Now we only
consider relevant samples that fit to the exact resampling window.

Part of #170.
ela-kotulska-frequenz added a commit that referenced this issue Mar 15, 2023
Previous solution used Timer, that was fired after the specific time.
Also it didn't take time difference spent on resampling into account
when sleeping.
In result the time difference between resampling period was constantly
increasing. And there was no option to catch up with resampling.

Fixes #170.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:data-pipeline Affects the data pipeline priority:high Address this as soon as possible type:bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

4 participants