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

Person: Ignore unavailable states #21058

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
3 participants
@balloob
Copy link
Member

commented Feb 14, 2019

Description:

Ignore unavailable/unknown states when determining state of a person.

If the code does not interact with devices:

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

@ghost ghost assigned balloob Feb 14, 2019

@ghost ghost added the in progress label Feb 14, 2019

@balloob balloob force-pushed the person-ignore-unavailable branch from 38371bc to 30ac199 Feb 14, 2019

@balloob balloob force-pushed the person-ignore-unavailable branch from 30ac199 to 873b7a3 Feb 14, 2019

@balloob balloob merged commit 81d2ec9 into dev Feb 14, 2019

3 of 5 checks passed

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

@ghost ghost removed the in progress label Feb 14, 2019

@delete-merged-branch delete-merged-branch bot deleted the person-ignore-unavailable branch Feb 14, 2019


# Unknown, as only 1 device tracker has a state, but we ignore that one
state = hass.states.get('person.tracked_person')
assert state.state == STATE_UNKNOWN

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 14, 2019

Member

Shouldn't this be state 'home' since the device tracker had that state first, before becoming unknown state?

This comment has been minimized.

Copy link
@balloob

balloob Feb 14, 2019

Author Member

The moment device tracker updates to unavailable, it's no longer home. So person should not rely on a no-longer available state in current implementation. I guess we can tweak in future.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 14, 2019

Member

But we don't set the person state explicitly to unknown. We just ignore unknown or unavailable device tracker states when reacting to the device tracker change. I don't understand why this test works.

This comment has been minimized.

Copy link
@balloob

balloob Feb 14, 2019

Author Member

Because I changed how the person entity determines state. It will fetch all new states for each entity and decides which one to take. So by overriding the only state that was there to one that is ignored, no state will be found.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 14, 2019

Member

Yeah, I forgot that we do set state explicitly to None after check at line 380.

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

Merge branch 'dev' into current
* dev: (213 commits)
  Bump pyHik library to 0.2.2, improve connections, add sensors (home-assistant#21086)
  Check against unlinked user (home-assistant#21081)
  Rename CONF_ATTRIBUTION to ATTRIBUTION (home-assistant#21069)
  Fix pushover schema
  Climate const.py move (home-assistant#20945)
  Update file header
  Update file header (home-assistant#21061)
  Météo-France platform for the weather component (home-assistant#18404)
  Bumped version to 0.89.0.dev0
  Don't directly update config entries (home-assistant#20877)
  Update file header (home-assistant#21054)
  Upgrade ruamel.yaml to 0.15.88 (home-assistant#21055)
  fix webhook update (home-assistant#21048)
  Add integration method to sensor.integration (home-assistant#21050)
  Person: Ignore unavailable states (home-assistant#21058)
  Person checks (home-assistant#21056)
  Create a person during onboarding (home-assistant#21057)
  Add template support to Bayesian sensor (home-assistant#20757)
  Add Lock capability to SmartThings platform (home-assistant#20977)
  Update translations
  ...

# Conflicts:
#	homeassistant/components/homematicip_cloud/climate.py

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

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.