Add Homekit locks support #13625

philk · 2018-04-01T23:05:27Z

Description:

Adding lock support for Homekit. It's largely a copy and modify from the covers device, but feel free to point out if I missed something.

Future work might include integrating LockLastKnownAction but that would require upstream lock components to report their status in a standard way (feels way out of scope for this).

cc: @cdce8p

I'll can do the minor PR for docs (add to list of supported components) if you're good with the way this looks.

Will depend on #13534 for the naming changes to self._hass and self._entity_id.

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

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

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

houndci-bot · 2018-04-01T23:05:48Z

homeassistant/components/homekit/__init__.py

@@ -121,6 +121,9 @@ def get_accessory(hass, state, aid, config):

    elif state.domain == 'light':
        return TYPES['Light'](hass, state.entity_id, state.name, aid=aid)
+


blank line contains whitespace

cdce8p

I'm looking forward to adding this to Home Assistant. A small home-assistant.github PR would be great. Left a few comments.

cdce8p · 2018-04-01T23:46:19Z

homeassistant/components/homekit/__init__.py

@@ -182,7 +185,7 @@ def start(self, *args):
        # pylint: disable=unused-variable
        from . import (  # noqa F401
            type_covers, type_lights, type_security_systems, type_sensors,
-            type_switches, type_thermostats)
+            type_switches, type_thermostats, type_locks)


Just order it alphabetically, like the rest is already.

cdce8p · 2018-04-01T23:50:29Z

homeassistant/components/homekit/type_locks.py

+
+    def __init__(self, hass, entity_id, name, *args, **kwargs):
+        """Initialize a Lock accessory object."""
+        super().__init__(name, entity_id, CATEGORY_LOCK, *args, **kwargs)


Don't pass *args. I changed that in #13534 as well. For init as well. It should be enough if additional parameter are passed via **kwargs. That mainly applies to aid.

cdce8p · 2018-04-01T23:51:50Z

homeassistant/components/homekit/type_locks.py

+    CATEGORY_LOCK, SERV_LOCK,
+    CHAR_LOCK_CURRENT_STATE, CHAR_LOCK_TARGET_STATE)
+
+


One empty line would be enough here, for consistency.

cdce8p · 2018-04-01T23:54:04Z

homeassistant/components/homekit/type_locks.py

+
+        self.current_state = None
+
+        self.serv_lock_mechanism = add_preload_service(self, SERV_LOCK)


The service doesn't need to be an instance attribute, since it is only used in __init__.

cdce8p · 2018-04-01T23:56:03Z

homeassistant/components/homekit/type_locks.py

+            get_characteristic(CHAR_LOCK_TARGET_STATE)
+
+        self.char_current_state.value = 3  # unknown
+        self.char_target_state.value = 1  # unsecured


Unsecured should be 0.

cdce8p · 2018-04-02T00:02:57Z

homeassistant/components/homekit/type_locks.py

+    def set_state(self, value):
+        """Set lock state to value if call came from HomeKit."""
+        self.char_target_state.set_value(value, should_callback=False)
+        if value != self.current_state:


This if statement isn't necessary, since a call from HomeKit implies that it is different. type_cover might not be the best example, since it tries to set a continuous value (and I didn't had the chance to optimize it yet). A good example is type_lights.

cdce8p · 2018-04-02T00:03:07Z

homeassistant/components/homekit/type_locks.py

+        self.char_target_state.set_value(value, should_callback=False)
+        if value != self.current_state:
+            _LOGGER.debug("%s: Set state to %d", self._entity_id, value)
+            print(self._hass.components)


Print statement

cdce8p · 2018-04-02T00:03:58Z

homeassistant/components/homekit/type_locks.py

+                self._hass.components.lock.unlock(self._entity_id)
+            else:
+                _LOGGER.warning(
+                    "unknown lock value %s for %s", value, self._entity_id)


Target State can only be set to 0 or 1 from HomeKit. The else clause will never be called.

cdce8p · 2018-04-02T00:12:04Z

homeassistant/components/homekit/type_locks.py

+            self.char_target_state.set_value(0, should_callback=False)
+        elif new_state.state == STATE_UNKNOWN:
+            self.char_current_state.set_value(3)
+            # self.char_target_state.set_value(0, should_callback=False)


This block needs improvement. HomeKit uses transition states, like opening and closing for covers and I would guess locking and unlocking for locks. To use them, I think the the best way is to use a flag variable (e.g. self.flag_target_state). Initialized in init to false, set to true in the set_state and reset in update_state. You might want to take a look at type_security_systems.

If you have questions about that, I'm happy to help.

Unless I'm looking in the wrong place LockCurrentState doesn't have transitional states. The only one I see with opening/closing is CurrentDoorState

It isn't really a state. The Home app does its own interpretation when current_state != target_state, at least for a short moment.

cdce8p · 2018-04-02T00:14:34Z

homeassistant/components/homekit/type_locks.py

+        if new_state is None:
+            return
+
+        # TODO: handle various lock states beyond just lock unlock


Please remove that.

philk · 2018-04-03T06:13:05Z

Sorry for the kind of sloppy initial PR, forget to push all my rebased changes originally. Thanks for the advice on security_system, I used a lot of that and the code feels much cleaner now.

I'll do the _hass, _entity_id, and args* fixes then squash rebase once #13534 is merged. Doc PR is done as well.

cdce8p

Just some minor suggestions. Otherwise looking good

cdce8p · 2018-04-03T10:45:27Z

homeassistant/components/homekit/type_locks.py

+
+from homeassistant.components.lock import (
+    ATTR_ENTITY_ID,
+    STATE_LOCKED, STATE_UNLOCKED, STATE_UNKNOWN)


You don't need an extra line here

cdce8p · 2018-04-03T10:45:35Z

homeassistant/components/homekit/type_locks.py

+from .accessories import HomeAccessory, add_preload_service
+from .const import (
+    CATEGORY_LOCK, SERV_LOCK,
+    CHAR_LOCK_CURRENT_STATE, CHAR_LOCK_TARGET_STATE)


cdce8p · 2018-04-03T10:59:30Z

homeassistant/components/homekit/type_locks.py

+            self.char_target_state.set_value(current_lock_state,
+                                             should_callback=False)
+        if self.char_target_state.value == self.char_current_state.value:
+            self.flag_target_state = False


L71-73: Nobody will see the debug statement.
L85: Shouldn't be necessary.

Maybe that should work:

hass_state = new_state.state if HASS_TO_HOMEKIT.get(hass_state) is not None: current_lock_state = HASS_TO_HOMEKIT[hass_state] self.char_current_state.set_value(current_lock_state, should_callback=False) _LOGGER.debug('%s: Updated current state to %s (%d)', self._entity_id, hass_state, current_lock_state) if not self.flag_target_state: self.char_target_state.set_value(current_lock_state, should_callback=False) self.flag_target_state = False

I have that check in case additional STATE_* are added to the lock component in the future, that seems like a reasonable possibility to me? (STATE_JAMMED for instance).

Right on L85, curious why security_system does it now that I think about it. Taking a look at that (and turning up logs in test) did lead me to noticing an exception (that for some reason doesn't cause tests to fail even though they cause it?):

ERROR:homeassistant.core:Error doing job: Future exception was never retrieved Traceback (most recent call last): File "/Users/pkates/.pyenv/versions/3.6.4/lib/python3.6/concurrent/futures/thread.py", line 56, in run result = self.fn(*self.args, **self.kwargs) File "/Users/pkates/pksrc/home-assistant/homeassistant/components/homekit/type_locks.py", line 84, in update_state should_callback=False) File "/Users/pkates/.pyenv/versions/3.6.4/envs/hass/lib/python3.6/site-packages/pyhap/characteristic.py", line 150, in set_value raise ValueError ValueError

This happens when Hass sets STATE_UNKNOWN which target_state doesn't support as a valid value.

My first thought was just if state == STATE_UNLOCKED but that feels lazy and brittle, I went with a try/except but I definitely willing to hear alternate opinions.

Maybe try: if hass_state in (STATE_UNLOCKED, STATE_LOCKED). I wouldn't agree that it's lazy, just practical and easier to read.

You don't need any debug statements for state not supported. That would get out of hand otherwise.

cdce8p · 2018-04-03T11:01:43Z

tests/components/homekit/test_locks.py

+        self.assertEqual(acc.char_current_state.value, 3)
+        self.assertEqual(acc.char_target_state.value, 0)
+
+        # # Set from HomeKit


# Set from HomeKit

cdce8p · 2018-04-03T11:10:31Z

Personally I'm not that big a fan of using HASS_TO_HOMEKIT and HOMEKIT_TO_HASS. It might make it a bit harder to read then an if clause. It works nevertheless so I will leave it up to you what you prefer.

cdce8p · 2018-04-03T12:42:22Z

I extracted the style changes from #13534 to #13654
If you want, take a quick look at it. It should be ready for merging.

philk · 2018-04-04T02:16:33Z

Rebased this on #13654 if you notice the commits look slightly weird.

cdce8p · 2018-04-09T14:13:37Z

Hi, sorry that I haven't replied. I somehow missed that you had already updated the PR.
I did some last changes:

rebase to resolve a merge conflict (in homekit/const.py)
Replace if HASS_TO_HOMEKIT.get(hass_state) is None: with a positive if statement. That will make upgrading the type much easier in the future. I will update security_types.py in another PR.

Only waiting for travis before I'm merging this.

* homekit: Add locks support * Improved upgradeability

philk requested a review from cdce8p as a code owner April 1, 2018 23:05

homeassistant added cla-signed integration: homekit new-platform labels Apr 1, 2018

houndci-bot reviewed Apr 1, 2018

View reviewed changes

cdce8p self-assigned this Apr 1, 2018

cdce8p requested changes Apr 2, 2018

View reviewed changes

fabaff changed the title ~~homekit: Add locks support~~ Add Homekit locks support Apr 2, 2018

homeassistant added the cla-signed label Apr 3, 2018

philk force-pushed the homekit_locks branch from 5adf548 to 3ece992 Compare April 3, 2018 05:59

homeassistant added the cla-signed label Apr 3, 2018

philk force-pushed the homekit_locks branch from 3ece992 to 319cf45 Compare April 3, 2018 06:01

homeassistant added the cla-signed label Apr 3, 2018

philk mentioned this pull request Apr 3, 2018

homekit/locks: Add locks home-assistant/home-assistant.io#5093

Merged

2 tasks

cdce8p reviewed Apr 3, 2018

View reviewed changes

cdce8p mentioned this pull request Apr 3, 2018

Minor style changes, cleanup #13654

Merged

2 tasks

philk force-pushed the homekit_locks branch from 319cf45 to 26ca98b Compare April 4, 2018 02:13

homeassistant added the cla-signed label Apr 4, 2018

philk force-pushed the homekit_locks branch from 26ca98b to ba01e62 Compare April 4, 2018 02:15

homeassistant added the cla-signed label Apr 4, 2018

philk force-pushed the homekit_locks branch from ba01e62 to e9c3ab4 Compare April 6, 2018 17:18

homeassistant added the cla-signed label Apr 6, 2018

philk force-pushed the homekit_locks branch from e9c3ab4 to 4da55cf Compare April 6, 2018 17:21

homeassistant added the cla-signed label Apr 6, 2018

philk force-pushed the homekit_locks branch from 4da55cf to 3333965 Compare April 6, 2018 17:34

homeassistant added the cla-signed label Apr 6, 2018

philk force-pushed the homekit_locks branch from 3333965 to 9b8f8f0 Compare April 6, 2018 21:30

homeassistant added the cla-signed label Apr 6, 2018

philk force-pushed the homekit_locks branch from 9b8f8f0 to 711804c Compare April 6, 2018 21:30

homeassistant added the cla-signed label Apr 6, 2018

philk force-pushed the homekit_locks branch from 711804c to 2b960f5 Compare April 6, 2018 21:31

homeassistant added the cla-signed label Apr 6, 2018

philk and others added 2 commits April 9, 2018 16:02

homekit: Add locks support

b27e7ce

Improved upgradeability

7afe863

cdce8p force-pushed the homekit_locks branch from 2b960f5 to 7afe863 Compare April 9, 2018 14:08

homeassistant added the cla-signed label Apr 9, 2018

cdce8p approved these changes Apr 9, 2018

View reviewed changes

cdce8p merged commit c61611d into home-assistant:dev Apr 9, 2018

cdce8p mentioned this pull request Apr 9, 2018

Improved upgradeability HomeKit type #13783

Merged

2 tasks

philk deleted the homekit_locks branch April 9, 2018 18:48

cdce8p added new-feature and removed new-platform labels Apr 11, 2018

balloob mentioned this pull request Apr 27, 2018

0.68 #14124

Merged

Adminiuga pushed a commit to Adminiuga/home-assistant that referenced this pull request Jun 25, 2018

Add Homekit locks support (home-assistant#13625)

064d55b

* homekit: Add locks support * Improved upgradeability

home-assistant locked and limited conversation to collaborators Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Homekit locks support #13625

Add Homekit locks support #13625

philk commented Apr 1, 2018 •

edited

Loading

houndci-bot Apr 1, 2018

cdce8p left a comment

cdce8p Apr 1, 2018

cdce8p Apr 1, 2018

cdce8p Apr 1, 2018

cdce8p Apr 1, 2018

cdce8p Apr 1, 2018

cdce8p Apr 2, 2018

cdce8p Apr 2, 2018

cdce8p Apr 2, 2018

cdce8p Apr 2, 2018

philk Apr 3, 2018

cdce8p Apr 3, 2018

cdce8p Apr 2, 2018

philk commented Apr 3, 2018

cdce8p left a comment

cdce8p Apr 3, 2018

cdce8p Apr 3, 2018

cdce8p Apr 3, 2018

philk Apr 4, 2018

cdce8p Apr 4, 2018

cdce8p Apr 3, 2018

cdce8p commented Apr 3, 2018

cdce8p commented Apr 3, 2018

philk commented Apr 4, 2018

cdce8p commented Apr 9, 2018

		@@ -121,6 +121,9 @@ def get_accessory(hass, state, aid, config):

		elif state.domain == 'light':
		return TYPES['Light'](hass, state.entity_id, state.name, aid=aid)

		CATEGORY_LOCK, SERV_LOCK,
		CHAR_LOCK_CURRENT_STATE, CHAR_LOCK_TARGET_STATE)


		self.current_state = None

		self.serv_lock_mechanism = add_preload_service(self, SERV_LOCK)

Add Homekit locks support #13625

Add Homekit locks support #13625

Conversation

philk commented Apr 1, 2018 • edited Loading

Description:

Checklist:

Choose a reason for hiding this comment

cdce8p left a 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

philk commented Apr 3, 2018

cdce8p left a 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

cdce8p commented Apr 3, 2018

cdce8p commented Apr 3, 2018

philk commented Apr 4, 2018

cdce8p commented Apr 9, 2018

philk commented Apr 1, 2018 •

edited

Loading