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

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

Merged
merged 13 commits into from Mar 9, 2019

Conversation

Projects
None yet
6 participants
@emontnemery
Copy link
Contributor

emontnemery commented Mar 2, 2019

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 emontnemery requested a review from home-assistant/core as a code owner Mar 2, 2019

@wafflebot wafflebot bot added the in progress label Mar 2, 2019

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 2, 2019

Related: #10498

_LOGGER.exception("Update for %s fails", self.entity_id)
return

state, attr = stateattr or self._get_state_and_attributes()

This comment has been minimized.

@balloob

balloob Mar 4, 2019

Member

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

This comment has been minimized.

Copy link
Member

balloob commented Mar 4, 2019

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
Copy link
Member

amelchio left a comment

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).

@@ -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):

This comment has been minimized.

@amelchio

amelchio Mar 4, 2019

Member

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

This comment has been minimized.

@emontnemery

emontnemery Mar 4, 2019

Author Contributor

Fixed.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 4, 2019

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

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Member

balloob commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Member

balloob commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 5, 2019

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

This comment has been minimized.

Copy link
Member

balloob commented Mar 5, 2019

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

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Mar 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 6, 2019

consider if we should allow async entities to call update_ha_state directly

async_update_ha_state is already called directly, for example here:

@callback
def async_register_callbacks(self):
"""Register callbacks to update hass after device was changed."""
async def after_update_callback(device):
"""Call after device was updated."""
await self.async_update_ha_state()
self.device.register_device_updated_cb(after_update_callback)

Do you mean to make a non coroutine version?

@emontnemery emontnemery force-pushed the emontnemery:fix_schedule_update_ha_state branch from 66070ae to 02ff5e7 Mar 6, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Mar 7, 2019

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

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Mar 7, 2019

@balloob That's much cleaner!

I still think it wouldn't hurt to update the docstrings for the schedule functions though:

def schedule_update_ha_state(self, force_refresh=False):
"""Schedule an update ha state change task from other thread.
If state is changed again before the ha state change task has been
executed, state transitions will be missed.
"""
self.hass.add_job(self.async_update_ha_state(force_refresh))
@callback
def async_schedule_update_ha_state(self, force_refresh=False):
"""Schedule an update ha state change task.
Scheduling the update avoids executor dead looks.
If state is changed again before the ha state change task has been
executed, state transitions will be missed.
This method must be run in the event loop.
"""
self.hass.async_create_task(self.async_update_ha_state(force_refresh))

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Mar 7, 2019

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_* ...

@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Mar 7, 2019

@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

This comment has been minimized.

Copy link
Member

balloob commented Mar 7, 2019

@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

This comment has been minimized.

Copy link
Member

amelchio commented Mar 7, 2019

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.

@emontnemery 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

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

balloob added some commits Mar 7, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Mar 9, 2019

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

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Mar 9, 2019

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

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

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.008%) to 92.75%
Details

@wafflebot wafflebot bot removed the in progress label Mar 9, 2019

mxworm added a commit to mxworm/home-assistant that referenced this pull request Mar 9, 2019

Merge branch 'dev' into current
* dev: (82 commits)
  Add ClearPass Policy Manger device tracker (home-assistant#21673)
  Better cloud check (home-assistant#21875)
  Bump quirks for ZHA and handle resulting battery % change (home-assistant#21869)
  Only commit if need. (home-assistant#21848)
  Fix authorization header in cors (home-assistant#21662)
  Introduce Entity.async_write_ha_state() to not miss state transition (home-assistant#21590)
  Fix TypeError (home-assistant#21734)
  Update honeywell.py to read current humidity for US Thermostats (home-assistant#21728)
  Override http.trusted_networks by auth_provider.trusted_networks (home-assistant#21844)
  mobile_app improvements (home-assistant#21607)
  Fixed a misspelling in a docstring (home-assistant#21846)
  Update dependencies to receive data on webhook callbacks (home-assistant#21838)
  Fix config entry exception in Ambient PWS (home-assistant#21836)
  Updated to pyeconet 0.0.10 (home-assistant#21837)
  fix empty TOPIC_BASE issue (home-assistant#21740)
  Synology sensor quick return if attr is null (home-assistant#21709)
  Add support for Cisco Mobility Express (home-assistant#21531)
  Log if aiohttp hits error during IndieAuth (home-assistant#21780)
  Resolve auth_store loading race condition (home-assistant#21794)
  Load logger and system_log components as soon as possible (home-assistant#21799)
  ...

@emontnemery emontnemery referenced this pull request Mar 12, 2019

Merged

Write state directly in all MQTT platforms #21971

2 of 3 tasks complete

@balloob balloob referenced this pull request Mar 20, 2019

Merged

0.90.0 #22216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.