Skip to content

Implement time tracking in templates#41147

Merged
balloob merged 3 commits intohome-assistant:devfrom
bdraco:tpl_time_pattern
Oct 19, 2020
Merged

Implement time tracking in templates#41147
balloob merged 3 commits intohome-assistant:devfrom
bdraco:tpl_time_pattern

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Oct 3, 2020

Completes home-assistant/architecture#206

Breaking change

It is no longer necessary to reference sensor.time, sensor.date, or manually update template entities that do not listen for state changes when now() or utcnow() is present in the template.

The template will automatically be updated when:

  • A referenced entity changes state.
  • At the start of each minute when now() or utcnow() is present in the template.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration (homeassistant) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 3, 2020

Looks like we need tests for

  • a template group that has some using state tracking and some using time pattern tracking
  • a time pattern that changes
  • returning the time_patterns in the listeners

@frenck
Copy link
Copy Markdown
Member

frenck commented Oct 4, 2020

This is functionality that should not be in the template engine. I feel like we are going down a rabbit hole we should not.

I think we should put a halt to this design we are heading towards, it makes things unneeded complex, for reasons I fail to see at this point.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 4, 2020

This is functionality that should not be in the template engine. I feel like we are going down a rabbit hole we should not.

I think we should put a halt to this design we are heading towards, it makes things unneeded complex, for reasons I fail to see at this point.

Without a way for users to take control of the update frequency, we leave cases like this #40621 (comment) without a solution.

Admittedly, it appears to be a rare case, and this does seem to be heading for feature overload so, I'll put this on ice until we can show that we have a larger set of affected users.

@bdraco bdraco closed this Oct 4, 2020
@bdraco bdraco reopened this Oct 10, 2020
@bdraco bdraco closed this Oct 11, 2020
@bdraco bdraco reopened this Oct 11, 2020
@bdraco bdraco marked this pull request as draft October 11, 2020 00:03
@bdraco bdraco changed the title Implement track_time_pattern in templates Implement time tracking in templates Oct 11, 2020
@bdraco bdraco removed the invalid label Oct 11, 2020
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 11, 2020

Offline discussion: do away with the configurability and just call_later(60) after each update (cancelled on the next update).

That should have minimal performance impact and also avoid a CPU spike at xx:yy:00.

websocketapi

fix test

tweaks

add time pattern skip if update inside the time pattern

reverts

reverts

reverts and switch to interval

adjust tests

handle now going away and coming back

update tests

cover
@bdraco bdraco requested a review from amelchio October 15, 2020 18:37
@amelchio
Copy link
Copy Markdown
Contributor

I think it is more consistent (and thus easier to explain/understand) to always delay 60 seconds after the last update. I have tried to adjust to that and it actually simplified things: amelchio@e08a598 ... but you said this would not work so maybe I missed something?

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 16, 2020

I think it is more consistent (and thus easier to explain/understand) to always delay 60 seconds after the last update. I have tried to adjust to that and it actually simplified things: amelchio@e08a598 ... but you said this would not work so maybe I missed something?

What you have will work.

There are two caveats we would need to be ok with:

  • We don't cancel the time based refresh if there is state changed event (not sure if that is an issue).
    - If now() is no longer in scope in the template, the time based refreshes will continue.

@amelchio
Copy link
Copy Markdown
Contributor

I don't think those caveats apply. The running async_call_later() is cancelled in both of these cases and a new one is installed iff now() is still in scope.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 16, 2020

I don't think those caveats apply. The running async_call_later() is cancelled in both of these cases and a new one is installed iff now() is still in scope.

I've stared at this and I can't see where the async_call_later is canceled? What am I missing?

@amelchio
Copy link
Copy Markdown
Contributor

It's self._time_listeners.pop(template)() from your commit, so not a part of my diff.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 16, 2020

I misunderstood. So the intent is to always update every 60 seconds even if there is a state changed event in the time window (taking out the _skip_next_time_update)?

@amelchio
Copy link
Copy Markdown
Contributor

No, my intent is to force an update if the template is idle for 60 seconds.

A state change at time 17 will schedule an update for time 77. Another state change at time 33 will cancel the first timer and instead schedule a new update for time 93 (always 60 seconds later).

With the same state change at time 33, your implementation might skip a time update at time 77 and then refresh at time 137 (104 seconds after the last state change).

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 16, 2020

No, my intent is to force an update if the template is idle for 60 seconds.

I checked this out on my desktop instead of trying to look at it on github and parse the changes on top of changes in my head and I'll all on board now.

It looks good to me. 👍 Do you want to push it on top of the existing branch in this PR?

@bdraco bdraco marked this pull request as ready for review October 16, 2020 16:17
@amelchio
Copy link
Copy Markdown
Contributor

Pushed it now. I am obviously cool with this change but I'll refrain from marking as approved since I think we should get feedback from at least @frenck as well so we can avoid any more emergency brakes :)

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 16, 2020

  • I will adjust the frontend PR
  • retest later today

@amelchio
Copy link
Copy Markdown
Contributor

Using sensor.time to align the update with the change of a minute will race with the automatic update. Thus, some minutes will have two updates right after each other. I wonder if we should change the delay to 61 or 65 seconds to avoid the redundant refresh in this (probably common) case? 🤔

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 17, 2020

Using sensor.time to align the update with the change of a minute will race with the automatic update. Thus, some minutes will have two updates right after each other. I wonder if we should change the delay to 61 or 65 seconds to avoid the redundant refresh in this (probably common) case? 🤔

Good call. Since we have microsecond precision, how about 60.45? We could probably even go lower.

@amelchio
Copy link
Copy Markdown
Contributor

Whatever you think is fine, I'm in ...

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

🎉

@amelchio
Copy link
Copy Markdown
Contributor

Grammar question to Breaking change: ... when an referenced entity changes state or at the start of each minute ...

Should that "or" be "and"? As it is, I understand that one will not get updates from state changes if now() is used. But it could be my reading comprehension that is the problem.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 22, 2020

It is a bit unwieldy. I'll rework the phrasing.

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Oct 22, 2020

I adjusted the phrasing. Hopefully that makes more sense.

basnijholt added a commit to basnijholt/home-assistant-config that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Binary sensor using now() never updates

6 participants