Timer: Add support for rearming timer with new interval #321
Conversation
github-actions
bot
added
part:docs
src/frequenz/channels/timer.py
Outdated
| self._next_tick_time = self._now() + start_delay_ms + self._interval | ||
|
|
||
| if prev_next_tick_time and prev_next_tick_time > self._next_tick_time: |
There was a problem hiding this comment.
This check is to know if this is the first time the timer is started? I think it is worth a comment. Also for the > condition, as it wasn't obvious for me (I hope I'm getting it right).
| if prev_next_tick_time and prev_next_tick_time > self._next_tick_time: | |
| # If there was no previous `next_tick_time`, it means the timer wasn't started yet, so no need to | |
| # fire the reset event. If the new `_next_tick_time` is bigger than the previous one, we also don't | |
| # need to fire the reset even, as `ready()` will keep looping until the new `_next_tick_time` is | |
| # reached. | |
| if prev_next_tick_time and prev_next_tick_time > self._next_tick_time: |
There was a problem hiding this comment.
Funny enough, I didn't even think about it, I was just making mypy happy 😆
There was a problem hiding this comment.
I was mainly checking if the new time is ealier or not like you described.
But I think we can simplify this.
src/frequenz/channels/timer.py
Outdated
| @@ -664,7 +683,12 @@ async def ready(self) -> bool: # noqa: DOC502 | |||
| # could be reset while we are sleeping, in which case we need to recalculate | |||
| # the time to the next tick and try again. | |||
| while time_to_next_tick > 0: | |||
| await asyncio.sleep(time_to_next_tick / 1_000_000) | |||
| for fin in asyncio.as_completed( | |||
There was a problem hiding this comment.
What does fin stands for? Why not completed or something more obvious?
src/frequenz/channels/timer.py
Outdated
| for fin in asyncio.as_completed( | ||
| [asyncio.sleep(time_to_next_tick / 1_000_000), self._reset_event.wait()] | ||
| ): | ||
| await fin | ||
| break |
There was a problem hiding this comment.
I was a bit confused by this break too (I somehow thought it was breaking the outer while instead of the for).
Maybe it could be a bit more clear like this?
| for fin in asyncio.as_completed( | |
| [asyncio.sleep(time_to_next_tick / 1_000_000), self._reset_event.wait()] | |
| ): | |
| await fin | |
| break | |
| await next(asyncio.as_completed( | |
| [asyncio.sleep(time_to_next_tick / 1_000_000), self._reset_event.wait()] | |
| )) |
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
github-actions
bot
added
the
part:tests
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
| if self.is_running: | ||
| self._reset_event.set() |
There was a problem hiding this comment.
Just out of curiosity, was the if necessary or is it just an optimization? Going through it quickly it looks like it should work without it too (if the timer is not running, it will just break the wait loop but it should resume normally, afterwards, right?), but maybe I'm missing some edge case.
There was a problem hiding this comment.
We would probably be fine if we just set it every time, yes.
|
Not sure how strict we should be about this, but the bug fix should be applied to v1.1.x, but the new feature should go to v1.2.0. I think to have it both and not being that bureaucratic, you could split it in 2 PRs, bug fix and feature, and we merge first the bug fix, release v1.1.3, then merge the feature and release v1.2.0. |
|
For simplicity I think we can release all of this in v1.2.0 and that's it. |
Ugly alternative to #320