Skip to content

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented Feb 14, 2025

When True, the timer will trigger immediately after starting, and then wait for the interval before triggering again.

@shsms shsms requested a review from a team as a code owner February 14, 2025 10:26
@shsms shsms requested a review from llucax February 14, 2025 10:26
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) labels Feb 14, 2025
@shsms
Copy link
Contributor Author

shsms commented Feb 14, 2025

FYI @cwasicki

triggering for the first time. If `auto_start` is `False` and
this is `True`, an exception is raised. If a `start_delay` is
specified and this is `True`, the first trigger will be immediately
after the `start_delay`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One use-case we have is that you want to trigger immediately, then wait for start_delay and from that point onward trigger each interval. This is not possible still with this addition, correct?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether it should be tick_on_init and the combination of tick_at_init=True and start_delay=X should function like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not possible still with this addition, correct?

No, I remember that use case and this should reduce its complexity and make it readable.

But I feel that this is the right balance, and tick_on_init + tick_on_start would make the API for the Timer too complex.

But we can come back to it. And if we are going to add more tick_on_* policies, maybe we should consider an enum instead, describing when to tick.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is replace tick_on_start with tick_on_init because AFAIU the behavior here can be achieved with a tick_on_init but what I outlined above is not possible with a tick_on_start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have only tick_on_init, then someone who wants just tick_on_start will get too many ticks and would need more complicated workarounds than someone who needs tick_on_init but only has tick_on_start.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which case do you mean here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in person, we take it in for now and keep potential changes for v2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an item describing the 2.0 plan here: #365

Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise.

Comment on lines +504 to +505
triggering for the first time. If `auto_start` is `False` and
this is `True`, an exception is raised. If a `start_delay` is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why raising if auto_start is False? What is the issue with making this first tick delayed to when the timer actually starts, like when using start_delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restricted by how it is implemented. But if they need it, users can specify tick_on_start=True in the call to reset, just like with start_delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the issue with making this first tick delayed to when the timer actually starts, like when using start_delay?

No, that's not how start_delay is implemented. It also doesn't work with auto_start=False.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, OK.

@shsms shsms added this pull request to the merge queue Feb 17, 2025
Merged via the queue into frequenz-floss:v1.x.x with commit fb83a3f Feb 17, 2025
14 checks passed
@shsms shsms deleted the tick-at-start branch February 17, 2025 08:45
@llucax llucax added this to the v1.7.0 milestone Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants