WIP Add support for Life360 device tracker #23405

pnbruckner · 2019-04-25T20:54:16Z

Breaking Change:

Although there are breaking changes from the custom component from which this is derived (see below), there are really no breaking changes, per se, since this is effectively a new integration.

Description:

Initial implementation derived from existing custom component.

Changes from custom component

The following changes were made to improve on the original implementation, to fit the new architecture and to remove features that didn't seem appropriate for a standard component/integration.

Configuration variable time_as and attribute time_zone removed so as to remove requirement for timezonefinderL and numpy, which are both rather large.
Change service from device_tracker.life360_zones_from_places to life360.zones_from_places, and add services.yaml.
Move more config processing to schema.
Configuration variable show_as_state must be list (comma separated values no longer accepted. I.e., change validator from cv.ensure_list_csv to cv.ensure_list.)
Change add_zones to accept either once or a timedelta (and remove original zone_interval key.)
Change warning_threshold and error_threshold so that error/warning will be logged when number of errors reach these levels (as opposed to going above them.)
Make default prefix life360 instead of none.
Change members configuration variable to include/exclude list.
Add circles and places configuration variables (similar to members.)
Add warning when members and/or circles configuration leads to no members being updated.
Bump life360 to 3.2.0. Use new life360-specific exceptions, and specify max_retries=2 to reduce number of communication errors.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#9302

Example entry for `configuration.yaml`:

device_tracker:
  - platform: life360
    username: LIFE360_USERNAME
    password: LIFE360_PASSWORD

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:

The manifest file has all fields filled out correctly (example).
New dependencies have been added to requirements in the manifest (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.

pnbruckner · 2019-04-25T20:56:19Z

I did run script/gen_requirements_all.py, but it didn't change requirements_all.txt. Instead it output these errors:

******* ERROR
Errors while importing:  The manifest for component api_streams is invalid., The manifest for component aws_lambda is invalid., The manifest for component aws_sns is invalid., The manifest for component aws_sqs is invalid., The manifest for component firetv is invalid., The manifest for component insteon_local is invalid., The manifest for component insteon_plm is invalid., The manifest for component introduction is invalid., The manifest for component pollen is invalid.
Make sure you import 3rd party libraries inside methods.

I wasn't sure how to manually edit requirements_all.txt, or if I even should.

awarecan · 2019-04-26T01:04:27Z

remove your api_stream folder, the component has been deleted for a while, but py_cache still exist in your local disk.

pnbruckner · 2019-04-26T01:29:17Z

Thanks! Still relatively new to git. I ran the following, after which script/gen_requirements_all.py worked!

git clean -fdx homeassistant

I'll amend the commit and force push.

pnbruckner · 2019-05-06T22:57:39Z

Ok, all changes I wanted to make are committed. This integration is ready for review. Removing WIP from title...

homeassistant/components/life360/device_tracker.py

MartinHjelmare · 2019-05-06T23:25:32Z

homeassistant/components/life360/device_tracker.py

+            '; '.join(['{}: {}, {}, {}'.format(*place) for place
+                       in sorted(places, key=lambda x: x.name.lower())]))
+
+    def zones_from_places(api, circles_filter, places_filter, home_place_name,


The zone handling needs a core review, I think. Is this something we want platforms to do?

@home-assistant/core

It may worth to open an architecture issue. Right now, user seems don't have way to create a zone through services or any api call, only way is going through config_flow, and no way to edit/delete them.

I think zone need rewrite, I propose moving zone into the core, like *_registry, not a component.

FWIW, this will only create zones during the current session, just like owntracks does. (They are therefore effectively not "permanent", at least as far as HA is concerned.) And if the Life360 Place is removed, the corresponding zone is automatically removed, too. So these are really, to some extent, separate from zones created via configuration.yaml or config_flow, although of course, unique entity_id's will always be created.

I can certainly understand a need to improve zone handling overall. But this works today (and has for quite some time.) Is it ok to use this current implementation and then come back and adjust as needed if/when the zone infrastructure is changed as a separate task? Just a thought...

Creating zones is fine.

It would be better if we would expose an appropriate API from the zone integration instead of having integrations dive into the Zone integration and take it's entity. I guess in a "perfect" world, the zone integration would have platforms and life360 and OwnTracks would be two of them.

For this PR, I suggest to add a async_create_zone(hass, name, lat, long, radius) to the Zone integration and call that from Life360.

So a few issues/questions that now come to mind:

Should async_create_zone return the Zone object? Or maybe the entity_id?

Should async_create_zone keep track of the created Zone object, and if so, where?

Should there also be an async_remove_zone function added to remove a zone that was created via the first function? And, if so, what should it take as a parameter -- the entity_id, name, Zone object returned by the first function, ...?

What is the best way, assuming the questions above have been answered/addressed, to update zone.home?

Regarding this last point, I now realize that I'm probably not handling the updating of zone.home as well as I should. First, I'm not using the home icon. Second, and possibly more important, I'm creating a Zone object for home, but there already is one, so now there would be two. I guess it's "worked" up until now because zone entities on their own never update after they've been created, and to the rest of the system it only matters what's in the state machine. But if we're going to do this in a reasonable manner, maybe updating zone.home should actually update the existing Zone object in some way.

If we all don't have time right now to work through all these questions (so as to come up with an appropriate enhancement to the zone integration), is it ok to go with what I have now, assuming that last point above isn't too bad of a situation. (I can probably address the icon issue fairly easily.)

Don't bother reviewing this just yet. I've given it more thought and I want to tweak it a bit more.

Sorry for the delay. I wanted to make sure not to change any of the original behavior. I think it's ready for a review. Also let me know if I need to add any tests.

Home zone is created based off the core configuration. We can't have integrations start overriding it, because it won't be clear who "owns" the data and there is no longer a single source of truth.

Sorry, but I don't understand this.

First, as I already mentioned, the need came from practical experience. If HA and Life360 have different definitions of home then they can, and will, disagree. This has been noticed by the users of my existing custom component and has caused confusion.

Second, to avoid that bad situation the two systems need to come into agreement as to where home is. Either this needs to be done manually, which is error prone and not as easy as it might seem (HA defines via GPS coordinates, whereas Life360 does it via address or a map), or this can be done automatically for the user. At least making HA agree with Life360 can be done automatically. (See this on the proposed doc page for more details.)

Third, who's to say whether HA's or Life360's definition of home is correct? I think this should be up to the user. Updating HA's home zone from Life360 info is a configurable feature -- the user can decide whether that's what they want or not.

And lastly, even in HA there is no "single source of truth", because it can come from either configuration.yaml or from an IP address lookup. And even if it comes from the config, there's two choices: under homeassistant or zone. So that's three different places it can come from in HA.

The bottom line to me is, it should be up to the user, and having a way to automatically keep the two systems in sync is better than forcing a user to manually copy data from one system to the other.

Oh, and BTW, if debug is enabled it tells you that it's doing this:

2019-05-13 12:58:59 DEBUG (SyncWorker_29) [homeassistant.components.life360.device_tracker] Updating zone.home from Place: Home: LATITUDE, LONGITUDE, RADIUS

@balloob so how can we make some progress on this? Maybe I should back out the changes to the zone and owntracks components and just put the life360 code back the way it was, which as I've said, has been not only working well in my custom component that many, many people use, but it also solves the real problems I've outlined. Then you can take your time to work out what you want to do with zones, and then this, as well as owntracks, etc., can be updated accordingly. But in the meantime I can retire my custom component and people can start using a standard component.

Initial implementation derived from existing custom component. Configuration variable time_as and attribute time_zone removed so as to remove requirement for timezonefinderL and numpy, which are both rather large. Change service from device_tracker.life360_zones_from_places to life360.zones_from_places, and add services.yaml. Move more config processing to schema. Configuration variable show_as_state must be list (comma separated values no longer accepted.) Change add_zones to accept either 'once' or a timedelta (and remove original zone_interval key.) Change warning_threshold and error_threshold so that error/warning will be logged when number of errors reach these levels (as opposed to going above them.) Make default prefix life360 instead of none. Change members configuration variable to include/exclude list. Add circles and places configuration variables (similar to members.) Add warning when members and/or circles configuration leads to no members being updated. Bump life360 to 3.2.0. Use new life360-specific exceptions, and specify max_retries=2 to reduce number of communication errors.

Use new zone maintenance functions in life360 & owntracks integrations.

balloob · 2019-05-13T08:32:40Z

homeassistant/components/life360/services.yaml

@@ -0,0 +1,2 @@
+zones_from_places:


Why is this a service?

If a user doesn't update Places in Life360 very often, or maybe never at all, then HA zones can be created/updated just once at startup. However, if/when they add, remove or change Places in Life360, they need a way to update HA accordingly. I suppose that could be done by forcing the user to restart HA. Rather than require a restart I provided two optional, alternative ways: they can set up HA to periodically poll the Life360 Places and update HA accordingly. Since this is something that probably doesn't happen very often it's probably best to set a fairly large poll period. Of course, that means when Places are changed it will take a while before HA updates. The other method is via a service. This way the user can update Life360 Places, then update HA immediately. Which way a user decides to go is based on their needs and inclinations.

balloob · 2019-05-13T08:33:05Z

homeassistant/components/zone/__init__.py

@@ -37,27 +39,69 @@
 }, extra=vol.ALLOW_EXTRA)


+def create_zone(hass, name, latitude, longitude, radius, icon=None,


Let's not create sync versions.

I did that for two reasons. First, life360 is sync and needed it. Second, I saw it is very common for other code, even core code, to provide both async and sync versions. It makes sense to me to provide a proper sync version rather than relying on people writing sync code, who may or may not understand async, to do it correctly. Also, why force duplication of code? I guess ultimately it doesn't matter to me. If I remove it from here I'll just have to add it to life360.

We only provide sync versions if it is an old API that was used in sync components. Life360 can just inline whatever this function does.

balloob · 2019-05-13T08:34:50Z

homeassistant/components/zone/__init__.py

+    return entity_id
+
+
+def remove_zone(hass, entity_id):


The sync version of remove, or the code that removes zones entirely?

sync version

balloob · 2019-05-13T08:36:09Z

homeassistant/components/life360/device_tracker.py

+
+_LOGGER = logging.getLogger(__name__)
+
+DEFAULT_FILENAME = 'life360.conf'


Use storage helper instead of creating your own files.

The file management is done in the pypi package which doesn't know about HA's storage helpers, similar to the way googe_maps does it. But since I own the pypi package, I suppose I can find a way to make this happen.

In that case, prefix the file with a . so it's hidden. However, by having your own file, it will not be part of our future backup/restore system.

balloob · 2019-05-13T08:39:49Z

homeassistant/components/life360/device_tracker.py

+    if not add_zones:
+        return
+
+    Place = namedtuple(


Move this outside of this function

Can you explain why? It's only used inside this function.

because you are defining a new class, and now are doing this each time this function is called.

balloob · 2019-05-13T08:40:37Z

homeassistant/components/life360/device_tracker.py

+
+    def get_places(api, circles_filter, places_filter):
+        errs = 0
+        while True:


Change this to while errs < 3: and drop the if check in the error handler.

balloob · 2019-05-13T08:41:55Z

homeassistant/components/life360/device_tracker.py

+
+        # If a "Home Place" was found and it is different from the current
+        # zone.home, then update zone.home with it.
+        if home_place:


Let's drop this and open an arch issue to discuss updating home zone.

Honestly, if I drop this then I'd rather drop the entire feature of creating/updating HA zones based on Life360 Places. This is the main reason for the feature existing in the first place.

I can guarantee, based on experience with the existing custom component, without this feature there's going to be lots of confusion when HA and Life360 don't agree on the user being home. It's happened many times already, and it will just happen more if/when this is introduced as a standard integration.

Actually, I'd rather not make it a standard integration without this feature and just live with maintaining the custom component.

It's up to you. If you want to discuss this in an arch issue, I'll just wait for that outcome. I personally see nothing wrong with an integration updating HA's data. That's basically what HA does -- take data from other systems, and in the process, make it more useful. I don't see why this should be any different.

Okay, in that case let's keep it a custom integration and close this PR.

@balloob ok, I'll go back to the drawing board and try to come up with something simpler.

balloob · 2019-05-13T08:46:43Z

homeassistant/components/life360/device_tracker.py

+
+_THRESHOLD = vol.All(vol.Coerce(int), vol.Range(min=1))
+
+LIFE360_SCHEMA = PLATFORM_SCHEMA.extend({


Instead of configuring it as a platform, let's configure it under the DOMAIN, so it will be easier to transition to a config entry in the future.

How about the work has already been done, let's just use it as-is and stop trying to put more work on me? It works like most other device trackers. It fits into the existing system just fine. If you want to do it some other way, then how about adding an issue to change it as part of another task? E.g., how about doing that as part of changing device_tracker to a platform component?

pnbruckner · 2019-05-22T15:24:42Z

While we're trying to come to some consensus on the zone questions, I think I want to take the time to reconsider the configuration variables. Real life scenarios are probably more complex than the current configuration options allow for. E.g., Circle names may not be unique, and between Circles, Member and Place names may not be unique either. There are unique IDs for everything, but they are not visible to the average user. What probably should be done is to use config flow, which, at least based on my limited understanding, might be able to retrieve all the entities from the Life360 server and present them in the UI so that users could pick which Circles, etc. they want to include, then internally the HA integration can use the unique IDs. I might even be able to do away with YAML config altogether. Not sure since I really haven't used config flow before. This will take some time...

pnbruckner · 2019-05-28T15:42:31Z

Apparently the current feature set doesn't fit. I'll close this PR and try to create something simpler that does fit and submit that in a new PR.

homeassistant added cla-signed integration: life360 new-integration new-platform labels Apr 25, 2019

pnbruckner mentioned this pull request Apr 25, 2019

Add Life360 integration home-assistant/home-assistant.io#9302

Closed

2 tasks

pnbruckner force-pushed the life360 branch from f6c23c9 to 9c89bea Compare April 26, 2019 01:35

pnbruckner changed the title ~~Add support for Life360 device tracker~~ WIP: Add support for Life360 device tracker May 1, 2019

pnbruckner force-pushed the life360 branch 4 times, most recently from 626adb3 to 9d8ff5f Compare May 6, 2019 19:45

pnbruckner changed the title ~~WIP: Add support for Life360 device tracker~~ Add support for Life360 device tracker May 6, 2019

MartinHjelmare reviewed May 6, 2019

View reviewed changes

pnbruckner changed the title ~~Add support for Life360 device tracker~~ WIP: Add support for Life360 device tracker May 10, 2019

pnbruckner force-pushed the life360 branch from 9d8ff5f to af69b9a Compare May 10, 2019 21:34

pnbruckner requested a review from a team as a code owner May 10, 2019 21:34

pnbruckner force-pushed the life360 branch from af69b9a to cfe2d9c Compare May 11, 2019 04:39

pnbruckner force-pushed the life360 branch 2 times, most recently from 908b554 to 25f30e0 Compare May 11, 2019 15:29

Add functions to zone component to create/update & remove zones

46ed612

Use new zone maintenance functions in life360 & owntracks integrations.

pnbruckner force-pushed the life360 branch from 25f30e0 to 46ed612 Compare May 12, 2019 13:40

pnbruckner changed the title ~~WIP: Add support for Life360 device tracker~~ Add support for Life360 device tracker May 12, 2019

balloob reviewed May 13, 2019

View reviewed changes

pnbruckner changed the title ~~Add support for Life360 device tracker~~ WIP Add support for Life360 device tracker May 22, 2019

pnbruckner closed this May 28, 2019

pnbruckner mentioned this pull request May 31, 2019

Life360 integration #24227

Merged

8 tasks

pnbruckner deleted the life360 branch June 7, 2019 13:24

		@@ -37,27 +39,69 @@
		}, extra=vol.ALLOW_EXTRA)


		def create_zone(hass, name, latitude, longitude, radius, icon=None,


		_LOGGER = logging.getLogger(__name__)

		DEFAULT_FILENAME = 'life360.conf'


		_THRESHOLD = vol.All(vol.Coerce(int), vol.Range(min=1))

		LIFE360_SCHEMA = PLATFORM_SCHEMA.extend({

WIP Add support for Life360 device tracker #23405

WIP Add support for Life360 device tracker #23405

Conversation

pnbruckner commented Apr 25, 2019 • edited

Breaking Change:

Description:

Changes from custom component

Example entry for configuration.yaml:

Checklist:

pnbruckner commented Apr 25, 2019

awarecan commented Apr 26, 2019

pnbruckner commented Apr 26, 2019

pnbruckner commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnbruckner May 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnbruckner commented May 22, 2019

pnbruckner commented May 28, 2019

pnbruckner commented Apr 25, 2019 •

edited

Example entry for `configuration.yaml`:

pnbruckner May 13, 2019 •

edited