Changed source priority for Person #21479

gorynychzmey · 2019-02-27T07:34:25Z

Description:

Added GPS accuracy to Person.
Changed source priority for Person. Status of Person determined next way:

If there are sources with status 'home', than latest of this sources will be taken.
If there are sources with source type 'gps', than latest of this sources will be taken.
Other wise will be taken latest source with status 'not_home'.

Docs PR:

home-assistant/home-assistant.io#8773

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:

Documentation added/updated in home-assistant.io

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.

homeassistant/components/person/__init__.py

balloob

Add a test and this will be ok to merge

Changed test for rounding of coordinates.

tests/components/person/test_init.py

balloob · 2019-02-28T00:40:53Z

Awesome 🎉

Please make a PR to update the docs to describe the new behavior.

MartinHjelmare · 2019-02-28T07:47:46Z

homeassistant/components/person/__init__.py

        for entity_id in self._config.get(CONF_DEVICE_TRACKERS, []):
            state = self.hass.states.get(entity_id)

            if not state or state.state in IGNORE_STATES:
                continue

-            if latest is None or state.last_updated > latest.last_updated:
-                latest = state
+            if state.attributes.get(ATTR_SOURCE_TYPE) == SOURCE_TYPE_GPS:


Checking source GPS first, before state home, doesn't match the PR description. Is the logic correct?

@MartinHjelmare, yes. If there are other non-GPS devices, which shown 'home', they'll be got prior to GPS in next conditions block.

I think this should be clarified in the PR description and in the docs, if it's not done already.

Also, it would help to add examples that explain why this logic is better than the previous logic.

Done in home-assistant/home-assistant.io#8779

MartinHjelmare · 2019-02-28T07:59:42Z

homeassistant/components/person/__init__.py

+            if state.attributes.get(ATTR_SOURCE_TYPE) == SOURCE_TYPE_GPS:
+                latest_gps = self._get_latest(latest_gps, state)
+            elif state.state == STATE_HOME:
+                latest_home = self._get_latest(latest_home, state)


Shouldn't we call this latest_non_gps_home or something? Since we're excluding GPS from this group.

Done in #21528

arsaboo · 2019-02-28T13:07:29Z

@balloob @MartinHjelmare The PR was merged before I could comment. Maybe I am missing something, but it looks like this is the opposite of what we should be doing. GPS trackers are more accurate and should be prioritized over wifi and BT trackers. However, it seems, we are doing the opposite here. Based on the examples, we are using the less accurate trackers (tracker_ble and tracker_router) to determine state. Given the consider_home property, they will always be slow to update and we should not prioritize them over GPS trackers. I follow this logic in my python_script that works 100% with 5 device_trackers. I did not want to add comments on the docs PR as the comments are related to the underlying logic and not the docs.

So, when we are home, what happens when tracker_router goes not_home due to wifi sleeping?

MartinHjelmare · 2019-02-28T13:18:09Z

We should open a new architecture issue to discuss this. I definitely think this needs more discussion. But I don't know myself that the new logic is worse. But I think we should discuss it. There were some comments mentioning a similar logic as is now implemented in the old architecture issue.

gorynychzmey · 2019-02-28T14:03:21Z

@arsaboo, when tracker_router goes not_home, then priority receives one of the other trackers. If tracker_ble still shows home, then it will be primary source, otherwise tracker_gps. So wifi sleeping is not an issue.
I have frequently the situation, when my Google Maps tracker gives location outside of my home while I'm definitely at home. So I think that stationary trackers are more accurate in question of detecting home (with adjstment to 'consider_home'). On the contrary GPS is more accurate in question of detecting zones outside of home.

arsaboo · 2019-02-28T14:10:18Z

I am happy to test the new logic out in the beta, as I am still using the meta tracker script for all my presence and can compare the two. However, with the logic in this PR, the tracker will behave only in the most optimistic case. In the more realistic scenario or if any of the trackers misbehave, we will certainly have worse outcomes.

For example, based on the docs PR, You're going out. 'tracker_gps' shows status 'not_home', but other two trackers show status 'home' according to their setting 'consider_home'. You are still considered to be at home., is definitely incorrect.

I have not played with Google Maps tracker and may be that is an issue with that specific tracker. I am currently using iOS app, Owntracks, Geofency, Life360, and Unifi trackers. Overall, in my experience and in discussions with other users, GPS trackers are significantly more accurate.

We should not penalize all GPS trackers just because one tracker is misbehaving. In fact, your experience raises another issue. May be we need to let users decide the priority based on how well these trackers work for them. I, for one, would certainly like to prioritize GPS over others.

gorynychzmey · 2019-02-28T14:21:59Z

@arsaboo, in new logic we prioritize not all stationary trackers against GPS, but only in question of detecting home. But you've given me an idea - specific warning needs to be added in documentation, than with multi-tracker configuration, especially when it contains GPS devices, consider_homemust be set as low as possible. In this case prioritized stationary trackers will show homeonly when device indeed at home and connected. When such device will disconnected, GPS will receive priority.

MartinHjelmare · 2019-02-28T14:36:50Z

It would be better if we can continue discussion in an architecture issue. Please open one.

arsaboo · 2019-02-28T14:38:31Z

I created one to get our discussion started home-assistant/architecture#163

sbienia · 2019-10-16T20:37:44Z

Hey
I have beacon in my office, I use OwnTracks as device tracker in person. When owntracks publish (source bluetooth_le) that I’m in office zone, person is not updated, only device tracker. It happens because gps has priority and in else block you have only home and not_home. Can you fix it?

frenck · 2019-10-16T20:46:57Z

Hi there, @sbienia, please don't comment on merged/closed PR's.

If you suspect an issue, please create one on our main GitHub repository. That will allow us to track it. Thanks! 👍

gorynychzmey added 2 commits February 27, 2019 08:17

Added gps accuracy to Person

aa462f6

Corrected GPS accuracy for Person

1499b93

gorynychzmey requested review from andrewsayre, andrey-git, bachya, cdce8p, Danielhiversen, dgomes, fabaff, kellerza, ludeeus, OttoWinter, rytilahti, scop, syssi and a team as code owners February 27, 2019 07:34

ghost added the in progress label Feb 27, 2019

homeassistant added the cla-signed label Feb 27, 2019

fabaff changed the base branch from master to dev February 27, 2019 08:19

Added priority of sources to Person

637c668

houndci-bot reviewed Feb 27, 2019

View reviewed changes

Fixed formatting

9361695

gorynychzmey changed the title ~~Added GPS accuracy to Person~~ Changed source priority for Person Feb 27, 2019

balloob reviewed Feb 27, 2019

View reviewed changes

homeassistant/components/person/__init__.py Outdated Show resolved Hide resolved

Removed rounding of coordinates.

4cac23d

balloob reviewed Feb 27, 2019

View reviewed changes

Added test for source priority.

a9c4625

Changed test for rounding of coordinates.

houndci-bot reviewed Feb 27, 2019

View reviewed changes

tests/components/person/test_init.py Outdated Show resolved Hide resolved

tests/components/person/test_init.py Outdated Show resolved Hide resolved

tests/components/person/test_init.py Outdated Show resolved Hide resolved

tests/components/person/test_init.py Outdated Show resolved Hide resolved

Improved code style

acdf86d

gorynychzmey added 5 commits February 27, 2019 19:49

Code style cleanup

1369123

Code style cleanup

1bfbc87

Code style cleanup

6c0c580

Code style cleanup

d35b9fe

Code style cleanup

2f9149e

balloob approved these changes Feb 27, 2019

View reviewed changes

Lint

4be7a7e

ghost assigned balloob Feb 28, 2019

Lint

33256de

balloob merged commit 4a45510 into home-assistant:dev Feb 28, 2019

ghost removed the in progress label Feb 28, 2019

gorynychzmey mentioned this pull request Feb 28, 2019

Added description for source priority. home-assistant/home-assistant.io#8773

Merged

2 tasks

gorynychzmey deleted the patch-3 branch February 28, 2019 07:10

MartinHjelmare reviewed Feb 28, 2019

View reviewed changes

balloob mentioned this pull request Mar 6, 2019

0.89.0 #21712

Merged

jjlawren mentioned this pull request Mar 12, 2019

Person tracker logic issues in 0.89 #21980

Closed

home-assistant locked as resolved and limited conversation to collaborators Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed source priority for Person #21479

Changed source priority for Person #21479

gorynychzmey commented Feb 27, 2019 •

edited by MartinHjelmare

Loading

balloob left a comment

balloob commented Feb 28, 2019

MartinHjelmare Feb 28, 2019

gorynychzmey Feb 28, 2019

MartinHjelmare Feb 28, 2019 •

edited

Loading

gorynychzmey Feb 28, 2019

MartinHjelmare Feb 28, 2019

gorynychzmey Feb 28, 2019

arsaboo commented Feb 28, 2019

MartinHjelmare commented Feb 28, 2019

gorynychzmey commented Feb 28, 2019 •

edited

Loading

arsaboo commented Feb 28, 2019 •

edited

Loading

gorynychzmey commented Feb 28, 2019

MartinHjelmare commented Feb 28, 2019

arsaboo commented Feb 28, 2019

sbienia commented Oct 16, 2019 •

edited

Loading

frenck commented Oct 16, 2019

Changed source priority for Person #21479

Changed source priority for Person #21479

Conversation

gorynychzmey commented Feb 27, 2019 • edited by MartinHjelmare Loading

Description:

Docs PR:

Checklist:

balloob left a comment

Choose a reason for hiding this comment

balloob commented Feb 28, 2019

MartinHjelmare Feb 28, 2019

Choose a reason for hiding this comment

gorynychzmey Feb 28, 2019

Choose a reason for hiding this comment

MartinHjelmare Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

gorynychzmey Feb 28, 2019

Choose a reason for hiding this comment

MartinHjelmare Feb 28, 2019

Choose a reason for hiding this comment

gorynychzmey Feb 28, 2019

Choose a reason for hiding this comment

arsaboo commented Feb 28, 2019

MartinHjelmare commented Feb 28, 2019

gorynychzmey commented Feb 28, 2019 • edited Loading

arsaboo commented Feb 28, 2019 • edited Loading

gorynychzmey commented Feb 28, 2019

MartinHjelmare commented Feb 28, 2019

arsaboo commented Feb 28, 2019

sbienia commented Oct 16, 2019 • edited Loading

frenck commented Oct 16, 2019

gorynychzmey commented Feb 27, 2019 •

edited by MartinHjelmare

Loading

MartinHjelmare Feb 28, 2019 •

edited

Loading

gorynychzmey commented Feb 28, 2019 •

edited

Loading

arsaboo commented Feb 28, 2019 •

edited

Loading

sbienia commented Oct 16, 2019 •

edited

Loading