Introduce Entity.async_write_ha_state() to not miss state transition #21590

emontnemery · 2019-03-02T12:55:12Z

Description:

Introduce Entity.async_write_ha_state() which can be called instead of Entity.schedule_update_ha_state / Entity.async_schedule_update_ha_state to immediately update hass state and thus make sure state transitions are never missed.
By calling Entity.async_write_ha_state(), state triggers have a deterministic behavior.

As an example, if an open event from a door sensor is delayed and arrives to HA at the same time as the closed event, the state may be back to closed once Entity.async_update_ha_state() is called. In such a case, any automation triggering on an open state event from the door sensor will never be executed.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

emontnemery · 2019-03-02T13:05:07Z

Related: #10498

balloob · 2019-03-04T05:31:24Z

homeassistant/helpers/entity.py

+                _LOGGER.exception("Update for %s fails", self.entity_id)
+                return
+
+        state, attr = stateattr or self._get_state_and_attributes()


I know that stateattr is passed in as None when force refresh, but it is still confusing to read the code as such. Please put an explicit state, attr = self._get_state_and_attributes() inside the if force_refresh. You probably also want to safe guard and check if stateattr is None too. I know you do all that now, but it's hard to follow and this is important code for people to understand.

balloob · 2019-03-04T05:36:03Z

A quick state change should most likely really be an event in Home Assistant. As usual MQTT is pushing our infra to it's limits 😉

I think that this is an ok change. Had one comment.

amelchio

I never managed to find a reference that it is safe to assume that tasks are executed round-robin in the order they are added. Lacking that, it seems like this change has the potential to set states in the wrong order (though I don't believe it can happen with current Python).

amelchio · 2019-03-04T06:31:22Z

homeassistant/helpers/entity.py

@@ -281,6 +261,32 @@ def async_set_context(self, context):
                            "https://goo.gl/Nvioub", self.entity_id,
                            type(self), end - start)

+        return (state, attr)
+
+    async def async_update_ha_state(self, force_refresh=False, stateattr=None):


Do we want stateattr to be a part of the external interface? It seems like it would only be used internally.

MartinHjelmare · 2019-03-04T08:56:57Z

We're changing what we mean with a state update with this PR. Currently we mean that it's current state that will be used in the update. With this PR it will be the passed state, which might be old, that will be used in the update.

I think this makes the state update unpredictable.

emontnemery · 2019-03-04T14:11:14Z

@amelchio Thanks for review, now updated such that the order is preserved.

emontnemery · 2019-03-04T14:25:58Z

I think this makes the state update unpredictable.

My take is the state update is more predictable with this change.

Without change:

schedule_update_ha_state called with force_refresh=False
- Multiple state changes happening before async_update_ha_state are lost, meaning it's not guaranteed that a STATE_CHANGED event is generated for a state change.
- This is confusing, because the Entity state is already known when calling schedule_update_ha_state
- As a result, any automation with a state event triggers is unreliable and may never be executed

After change:

schedule_update_ha_state called with force_refresh=False
- Every state change will generate a STATE_CHANGED events, with preserved order.

For the case where schedule_update_ha_state is called with force_refresh=True, behavior is unchanged

Multiple state changes happening before async_update_ha_state are lost
This is probably OK, because the Entity state is not yet updated when calling schedule_update_ha_state

MartinHjelmare · 2019-03-04T15:46:32Z

I think it's more important to preserve the integrity of the state, ie that the state value and time are true, than ensuring that the update generates a state change event. If we can't trust the state, the state change event is useless.

Automations that need the very short time resolution should use events and not states changes. Integrations should follow this when implementing support for devices and services.

emontnemery · 2019-03-04T16:38:20Z

it's more important to preserve the integrity of the state, ie that the state value and time are true, than ensuring that the update generates a state change event. If we can't trust the state, the state change event is useless.

In which way can't we trust the state with this change, are you worried about processing a long backlog of old states?

balloob · 2019-03-04T16:49:34Z

After thinking about it more, I agree with @MartinHjelmare . We schedule a state update, and when that update happens, that's when we write the current state. Otherwise the name of the function no longer covers what it does.

balloob · 2019-03-04T17:01:24Z

I think I am mainly worried about state changes no longer representing the current state. If we say that at time X the state was Y, it should be Y. It should not have been Y between previous state change and now.

emontnemery · 2019-03-04T18:00:04Z

OK, but it means the STATE_CHANGED event really is unreliable if used as a trigger for automations, with no guarantee that an event is generated for state transitions if: the states happen too close in time, the event loop is blocked, updates from the HW are delayed due to network issues and arrive together, etc.

I think I am mainly worried about state changes no longer representing the current state.

Yes, it means we would replay instead of discard state changes if we for whichever reason can't keep up. Is replaying really worse than discarding?

emontnemery · 2019-03-05T07:11:58Z

We schedule a state update, and when that update happens, that's when we write the current state.

How about making sure we generate a STATE_CHANGED event for every state transition, while taking care not to build a queue of outdated state writes?

Not sure how to do that though..

balloob · 2019-03-05T19:40:06Z

That's impossible because state change events are fired by the state machine, the entity is not responsible for that.

balloob · 2019-03-05T19:50:08Z

The only thing I can think of, is to consider if we should allow async entities to call update_ha_state directly.

CC @pvizeli

emontnemery · 2019-03-06T15:22:07Z

consider if we should allow async entities to call update_ha_state directly

async_update_ha_state is already called directly, for example here:
https://github.com/home-assistant/home-assistant/blob/8e9a4960025fd1b422c5a6dbd1c7c28af86853b5/homeassistant/components/knx/binary_sensor.py#L100-L106

Do you mean to make a non coroutine version?

balloob · 2019-03-07T06:11:19Z

I've updated your PR @emontnemery with what I think would be ok. It's a lot simpler, and should not be confusing.

emontnemery · 2019-03-07T08:16:12Z

@balloob That's much cleaner!

I still think it wouldn't hurt to update the docstrings for the schedule functions though:
https://github.com/home-assistant/home-assistant/blob/02ff5e79cc421a3eab873991c48a09c45d76a63f/homeassistant/helpers/entity.py#L337-L355

amelchio · 2019-03-07T08:35:03Z

Speaking of confusing, should these methods be named *_hass_*? I realize it's a big rename but if we want to do it eventually, probably we should not introduce more methods called *_ha_* ...

homeassistant/helpers/entity.py

pvizeli · 2019-03-07T14:54:52Z

@balloob well, the changes on helper/entity.py are okay. I prefer to use schedule_update_ha_state as we do now. So it's for every one clean what they need call. I'll remember everyone here, that mqtt was slow (also other libraries) until we abstract it to work like now. This speed cost us that we can't process noise with a smaller time frame as 100 - 150ms. But for home automation, I don't see an issue do not trigger another device in that time frame with automation.

If we want to go away from that pratic to old slow one that records any state change in any time, we should change schedule_update_ha_state as we want and all is working in the same way. We should not build a new building lot.

balloob · 2019-03-07T15:00:04Z

@amelchio let's keep it ha. It's a huge breaking change and provides no extra clarification

@pvizeli we can't update the original method because it's async. If we want to write now, we need to call a callback

amelchio · 2019-03-07T15:11:38Z

I recall inconsistent naming being quite a barrier to entry for a new developer and these things have a tendency to get worse over time because there is not a single right way to copy.

Anyway, this is not the place for an extended discussion about this – just thought I would mention it.

homeassistant/helpers/entity.py

balloob · 2019-03-09T00:38:19Z

@pvizeli @MartinHjelmare @amelchio I am fine with the PR in its current state. Any objects on merging this?

amelchio · 2019-03-09T06:56:52Z

I have not really formed an opinion, so no objections from me.

Copy state in schedule_update_ha_state

9db7b87

emontnemery requested a review from a team as a code owner March 2, 2019 12:55

homeassistant added cla-signed core labels Mar 2, 2019

ghost assigned emontnemery Mar 2, 2019

ghost added the in progress label Mar 2, 2019

Lint

1176f15

emontnemery mentioned this pull request Mar 2, 2019

What is the correct way to utilise auto-discovered buttons? Koenkk/zigbee2mqtt#959

Closed

Fix broken test

8b5ea22

balloob reviewed Mar 4, 2019

View reviewed changes

Review comment, improve docstring

d6d5d76

amelchio reviewed Mar 4, 2019

View reviewed changes

Preserve order of state updates

260e3b0

Rewrite

02ff5e7

emontnemery force-pushed the fix_schedule_update_ha_state branch from 66070ae to 02ff5e7 Compare March 6, 2019 15:39

Break up async_update_ha_state

fc477bc

ghost assigned balloob Mar 7, 2019

Update binary_sensor.py

c4b5789

MartinHjelmare reviewed Mar 7, 2019

View reviewed changes

homeassistant/helpers/entity.py Outdated Show resolved Hide resolved

Review comments

9459170

Update docstring

cf641cd

hass -> ha

83bd8ae

emontnemery changed the title ~~Copy state in schedule_update_ha_state~~ Introduce Entity.async_write_ha_state() to not miss state transition Mar 7, 2019

balloob reviewed Mar 7, 2019

View reviewed changes

homeassistant/helpers/entity.py Outdated Show resolved Hide resolved

balloob added 2 commits March 7, 2019 09:58

Update entity.py

a37f23b

Update entity.py

6c00020

balloob merged commit fc81826 into home-assistant:dev Mar 9, 2019

ghost removed the in progress label Mar 9, 2019

emontnemery mentioned this pull request Mar 12, 2019

Write state directly in all MQTT platforms #21971

Merged

3 tasks

balloob mentioned this pull request Mar 20, 2019

0.90.0 #22216

Merged

emontnemery mentioned this pull request Apr 18, 2019

Fix clearing error message for MQTT vacuum #23206

Merged

4 tasks

basilfx mentioned this pull request Jul 26, 2023

Use async_write_ha_state to update switch state basilfx/homeassistant-biketrax#103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Entity.async_write_ha_state() to not miss state transition #21590

Introduce Entity.async_write_ha_state() to not miss state transition #21590

emontnemery commented Mar 2, 2019 •

edited

Loading

emontnemery commented Mar 2, 2019

balloob Mar 4, 2019

balloob commented Mar 4, 2019

amelchio left a comment

amelchio Mar 4, 2019

emontnemery Mar 4, 2019

MartinHjelmare commented Mar 4, 2019

emontnemery commented Mar 4, 2019

emontnemery commented Mar 4, 2019 •

edited

Loading

MartinHjelmare commented Mar 4, 2019

emontnemery commented Mar 4, 2019

balloob commented Mar 4, 2019

balloob commented Mar 4, 2019

emontnemery commented Mar 4, 2019 •

edited

Loading

emontnemery commented Mar 5, 2019

balloob commented Mar 5, 2019

balloob commented Mar 5, 2019

emontnemery commented Mar 6, 2019

balloob commented Mar 7, 2019

emontnemery commented Mar 7, 2019

amelchio commented Mar 7, 2019

pvizeli commented Mar 7, 2019 •

edited

Loading

balloob commented Mar 7, 2019

amelchio commented Mar 7, 2019

balloob commented Mar 9, 2019

amelchio commented Mar 9, 2019

Introduce Entity.async_write_ha_state() to not miss state transition #21590

Introduce Entity.async_write_ha_state() to not miss state transition #21590

Conversation

emontnemery commented Mar 2, 2019 • edited Loading

Description:

Checklist:

emontnemery commented Mar 2, 2019

balloob Mar 4, 2019

Choose a reason for hiding this comment

balloob commented Mar 4, 2019

amelchio left a comment

Choose a reason for hiding this comment

amelchio Mar 4, 2019

Choose a reason for hiding this comment

emontnemery Mar 4, 2019

Choose a reason for hiding this comment

MartinHjelmare commented Mar 4, 2019

emontnemery commented Mar 4, 2019

emontnemery commented Mar 4, 2019 • edited Loading

MartinHjelmare commented Mar 4, 2019

emontnemery commented Mar 4, 2019

balloob commented Mar 4, 2019

balloob commented Mar 4, 2019

emontnemery commented Mar 4, 2019 • edited Loading

emontnemery commented Mar 5, 2019

balloob commented Mar 5, 2019

balloob commented Mar 5, 2019

emontnemery commented Mar 6, 2019

balloob commented Mar 7, 2019

emontnemery commented Mar 7, 2019

amelchio commented Mar 7, 2019

pvizeli commented Mar 7, 2019 • edited Loading

balloob commented Mar 7, 2019

amelchio commented Mar 7, 2019

balloob commented Mar 9, 2019

amelchio commented Mar 9, 2019

emontnemery commented Mar 2, 2019 •

edited

Loading

emontnemery commented Mar 4, 2019 •

edited

Loading

emontnemery commented Mar 4, 2019 •

edited

Loading

pvizeli commented Mar 7, 2019 •

edited

Loading