Add consider_home and source_type to device_tracker.see service #12849
Conversation
|
Travis failure is unrelated to this PR. Also fails locally without any changes. Please restart the build. See also: https://travis-ci.org/home-assistant/home-assistant/jobs/348381098#L1027 |
|
|
||
| try: | ||
| consider_home = call.data[ATTR_CONSIDER_HOME] | ||
| except KeyError: |
There was a problem hiding this comment.
Please use dict.get and set the default timedelta if the key is missing.
There was a problem hiding this comment.
The other arguments/keys are only added if they are present in call.data, so what is the reason you prefer a default for consider_home? This would overwrite any consider_home stored in a Device already. If consider_home isn't passed, then the value previously stored in Device.consider_home is used (see Device.async_seen).
For example, if you create a device tracker in your config like so:
device_tracker:
consider_home: 300
your proposal would result in any service call without consider_home to overwrite this value with 180. If the functionality of this PR is desired and likely to be merged, I could also write some tests that ensure this doesn't happen.
If your issue is solely with the try/except block (which I admit has a lot of cruft), I can of course rewrite it differently, e.g.
if ATTR_CONSIDER_HOME in call.data:
args[ATTR_CONSIDER_HOME] = timedelta(seconds=int(call.data[ATTR_CONSIDER_HOME]))
There was a problem hiding this comment.
Yes, sorry, I misread the try... else. Just switch from try... else to what you write above.
There was a problem hiding this comment.
Alright. Commits are squashed before merging right? So I don't have to force-push to my fork?
One more thing might be to not just allow seconds in the timedelta creation, but to parse times such as "0:03", "0:03:00" correctly.
There was a problem hiding this comment.
Yes, instead add a service schema that validates all the allowed key - value pairs. Then you can also just add the ATTR_CONSIDER_HOME key in the iteration of call.data above.
There was a problem hiding this comment.
That's awesome, I didn't know that possibility existed.
| hass.services.async_register(DOMAIN, SERVICE_SEE, async_see_service) | ||
| hass.services.async_register( | ||
| DOMAIN, SERVICE_SEE, async_see_service, | ||
| schema=vol.Schema({ATTR_CONSIDER_HOME: cv.time_period}, |
There was a problem hiding this comment.
Please define the schema as a constant at the module level. Also please validate all possible service attributes in the schema. The schema should not allow extra.
There was a problem hiding this comment.
Previously none of the arguments were validated, so this just enforces validation on only ATTR_CONSIDER_HOME, while chaning nothing else. But yeah, I agree, I initially wanted to do what you said, but did this to keep changes minimal.
I'll add a general schema for the service.
| ATTR_ATTRIBUTES: dict, | ||
| ATTR_SOURCE_TYPE: vol.In(SOURCE_TYPES), | ||
| ATTR_CONSIDER_HOME: cv.time_period, | ||
| })) |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_BATTERY: cv.string, | ||
| ATTR_ATTRIBUTES: dict, | ||
| ATTR_SOURCE_TYPE: vol.In(SOURCE_TYPES), | ||
| ATTR_CONSIDER_HOME: cv.time_period, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_GPS_ACCURACY: cv.positive_int, | ||
| ATTR_BATTERY: cv.string, | ||
| ATTR_ATTRIBUTES: dict, | ||
| ATTR_SOURCE_TYPE: vol.In(SOURCE_TYPES), |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_GPS: cv.gps, | ||
| ATTR_GPS_ACCURACY: cv.positive_int, | ||
| ATTR_BATTERY: cv.string, | ||
| ATTR_ATTRIBUTES: dict, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_LOCATION_NAME: cv.string, | ||
| ATTR_GPS: cv.gps, | ||
| ATTR_GPS_ACCURACY: cv.positive_int, | ||
| ATTR_BATTERY: cv.string, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_DEV_ID: cv.string, | ||
| ATTR_HOST_NAME: cv.string, | ||
| ATTR_LOCATION_NAME: cv.string, | ||
| ATTR_GPS: cv.gps, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_MAC: cv.string, | ||
| ATTR_DEV_ID: cv.string, | ||
| ATTR_HOST_NAME: cv.string, | ||
| ATTR_LOCATION_NAME: cv.string, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| cv.has_at_least_one_key(ATTR_MAC, ATTR_DEV_ID), { | ||
| ATTR_MAC: cv.string, | ||
| ATTR_DEV_ID: cv.string, | ||
| ATTR_HOST_NAME: cv.string, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| SERVICE_SEE_PAYLOAD_SCHEMA = vol.Schema(vol.All( | ||
| cv.has_at_least_one_key(ATTR_MAC, ATTR_DEV_ID), { | ||
| ATTR_MAC: cv.string, | ||
| ATTR_DEV_ID: cv.string, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
|
|
||
| SERVICE_SEE_PAYLOAD_SCHEMA = vol.Schema(vol.All( | ||
| cv.has_at_least_one_key(ATTR_MAC, ATTR_DEV_ID), { | ||
| ATTR_MAC: cv.string, |
There was a problem hiding this comment.
continuation line missing indentation or outdented
| ATTR_LOCATION_NAME: cv.string, | ||
| ATTR_GPS: cv.gps, | ||
| ATTR_GPS_ACCURACY: cv.positive_int, | ||
| ATTR_BATTERY: cv.string, |
There was a problem hiding this comment.
Should this be a string or an int?
There was a problem hiding this comment.
Both should be possible, as it turns out. Some platforms use int, others str. @balloob said it should usually be an int, though. I initially used str because the typechecking on DeviceTracker and Device expects str.
There was a problem hiding this comment.
Ok, well a test failed due to it expecting an int, it seems. If we coerce to int platforms should be able to provide both, I think.
|
🎉 thanks! |
|
You might want to link your script to push things to Home Assistant on the docs page for the OpenWRT device tracker integration. |
|
@balloob yep, will do so! Wasn't sure whether this was going to be merged. Adding documentation now! |
* Add consider_home and source_type to device_tracker.see service * Use schema instead of manual validation * Extend schema to validate all keys * Fix style * Set battery level to int
Description:
Exposes
consider_home(in seconds)andsource_typeto thedevice_tracker.seeservice.As a prerequisite, this also adds
consider_hometodevice_tracker.DeviceTracker.async_see. This sets theconsider_homeattribute ofdevice_tracker.Device.This is useful for the following reasons:
source_type: the caller of the service can identify the source of the informationconsider_home: the caller of the service may have more/dynamic information on how long this event means the device is likely to be present.As an example, this allows a completely event-driven device tracker for OpenWRT which I implemented, i.e. without any scanners or cron jobs. Based on the association event, one of two timeouts is chosen. This drastically reduces the amount of calls made to keep an up-to-date mirror of device presence, typical wifi association events occur very infrequently if a device stays connected. The event-driven design completely removes the delay of newly-connected devices due to scanning taking place only every 12 seconds. When a device connects to an OpenWRT access point running the above package it is registered in Home Assistant in less than 1 second.
Also adds
voluptuousvalidation todevice_tracker.seeservice calls and fixes thebatteryattribute being treated asstrin some places. There may be in issue withgpslogger, as it treats it asfloat, but it does not have any tests so I can't check it as the calls made by the mobile apps to thegpsloggerendpoint are unknown to me.Checklist:
toxrun successfully. Your PR cannot be merged unless tests pass