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

Correctly detect devices, which went offline during HA restart #20933

Merged
merged 2 commits into from Feb 21, 2019

Conversation

Projects
None yet
7 participants
@OleksandrBerchenko
Copy link
Contributor

OleksandrBerchenko commented Feb 10, 2019

Description:

Related issue: fixes #18698

This fix addresses the following scenario:

  1. Device is online (home)
  2. Stop Home Assistant
  3. Power off device
  4. Start Home Assistant
  5. Home Assistant thinks that the device is home forever

So, what happens?

  1. Device object is created with state = not home and last seen = None.
  2. async_added_to_hass loads the previous state of the entity and updates state = home. last_seen is still None.
  3. Sooner or later async_update_stale correctly detects that the device is stale and triggers async_update_ha_state.
  4. It calls async_update and we expect that the following code will run:
        elif self.stale():
            self.mark_stale()
    
    Nope. Nothing happens because:
        if not self.last_seen:
            return
    

The cause of the problem is a situation when state = home and last seen = None: a device was never seen, but is treated online.

The idea of the fix is to set some last_seen when we loaded the previous state and it's home. That allows to correctly handle the scenario above and shouldn't break any other scenarios.

Example entry for configuration.yaml:

device_tracker:
  - platform: ping
    consider_home: 120
    hosts:
      slinex: 192.168.1.83

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.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Feb 10, 2019

IMHO the sensor-type devices should not restore the previous state.
Would not it be a more appropriate behavior for the device to be detected again independently of the previous state? Why set a fake date with previous state better than detect state again?
I do not know the internal behavior of device_tracker but I thinkit is the behavior that should be followed for those devices that can update the status periodically.

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Feb 10, 2019

I have seen that restore state is a old PR (PR #6150).
I'm sure it's not something you want to modify now.

Maybe last_seen attribute can be saved too and restored like the others attributes

        for attr, var in (
                (ATTR_SOURCE_TYPE, 'source_type'),
                (ATTR_GPS_ACCURACY, 'gps_accuracy'),
                (ATTR_BATTERY, 'battery'),
        ):
            if attr in state.attributes:
                setattr(self, var, state.attributes[attr])
@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

OleksandrBerchenko commented Feb 10, 2019

So, should we put all devices into not home state for up to a minute after HA restart (and to trigger a dozen of monitoring alerts in my case)?

I'm ok if we would have a state that means state detection is pending, be patient, but for device_tracker we have only home/not home (and I don't think any other platform has something like that).

Personally, I'm fine to read all previous states (in most cases nothing changes) and give HA another minute to update them.

Does it make sense?

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

OleksandrBerchenko commented Feb 10, 2019

Maybe last_seen attribute can be saved too and restored like the others attributes

What if HA was stopped at least for several minutes? Then all restored last seen values will be outdated and all devices will be marked as not home immediately.

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Feb 10, 2019

...for device_tracker we have only home/not home ...

I see your point.
I thought that maybe the state was not initialized until having a reading, but I have seen at code that (as you say) it is initialized in the not home state, with all the problems it entails.

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Feb 10, 2019

What if HA was stopped at least for several minutes? Then all restored last seen values will be outdated and all devices will be marked as not home immediately.

But in the opposite case, your proposal would also be a good solution?. For example, a device goes to not home during a several minutes restart. Can the fake date cause an unexpected behavior?

I hope you do not misunderstand me, I do not know this type of device or its implementation. Also I do not doubt the proposed solution. But it is something that I have raised myself in other situations (RFLink sensors) and I'm glad to see your point of view about it.

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

OleksandrBerchenko commented Feb 10, 2019

With my fake date fix we have the following situation after HA restart:

  • We assume that all devices, which were home before the restart are still home and we have just seen all of them (my fake last seen value).
    In a minute:
    • If there was no seen event, the device is marked as not home (and fake date here helps to avoid if not self.last_seen early exit).
    • If there was a seen event, last seen is updated with a real date and the device remains home.
  • We assume that all devices, which were not home before the restart are still not home and we have never seen any of them (I didn't change anything here).
    In a minute:
    • If there was no seen event, the device remains not home and last seen remains None.
    • If there was a seen event, last seen is updated with a real date and the device becomes home.

No, your questions are fine :)

Thanks!

@javicalle

This comment has been minimized.

Copy link
Contributor

javicalle commented Feb 10, 2019

Now I think I can see the full picture.

  1. Restore states prevents from undesired trigger events (default state not home)
  2. Fake date also prevents from undesired trigger events if too much time passes ((now or dt_util.utcnow()) - self.last_seen > self.consider_home)
  3. If in the middle of restarting a device changes its state, next update (at SCAN_INTERVAL) triggers events and changes states to the real one

Thank you for your patience and understanding.

@OleksandrBerchenko

This comment has been minimized.

Copy link
Contributor Author

OleksandrBerchenko commented Feb 10, 2019

Yes, I see it the same way.

Thanks!

@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Feb 21, 2019

The solution is like you say before, restore last_seen.

The magic is my code on:

async def async_setup_tracked_device(self):
"""Set up all not exists tracked devices.
This method is a coroutine.
"""
async def async_init_single_device(dev):
"""Init a single device_tracker entity."""
await dev.async_added_to_hass()
await dev.async_update_ha_state()
tasks = []
for device in self.devices.values():
if device.track and not device.last_seen:
tasks.append(self.hass.async_create_task(
async_init_single_device(device)))
if tasks:
await asyncio.wait(tasks, loop=self.hass.loop)

Only device they are not available during setup will be restored.

@pvizeli pvizeli self-assigned this Feb 21, 2019

@pvizeli pvizeli merged commit 0969214 into home-assistant:dev Feb 21, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA

@wafflebot wafflebot bot removed the in progress label Feb 21, 2019

@tjorim

This comment has been minimized.

Copy link
Contributor

tjorim commented Feb 23, 2019

Should this fix #19998 too? It's a similar issue, just other platform.
Maybe it's worth it to tag for the next 0.88 release?
Will try out if this fixes my issue as well.

@quthla

This comment has been minimized.

Copy link
Contributor

quthla commented Mar 25, 2019

Unfortunately this seems to make device_tracker disregard consider_home so devices are set not_home when they should actually still be considered home. I'd suggest reverting this PR until there is a better solution which doesn't break existing integrations.

#22387

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Mar 25, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 25, 2019

Further discussion is directed to the issue linked above.

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