Support Garage Doors in HomeKit

[marthoc]

Description:

Add support for garage doors in HomeKit:

• Cover needs to be in device_class: garage (added via customize:).
• Cover also needs to support at least OPEN and CLOSE.

Related issue (if applicable): #13782

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

Example entry for configuration.yaml (if applicable):

customize:
  cover.test_garage_door:
    device_class: garage

cover:
  - platform: mqtt
    name: test_garage_door
    command_topic: garage/door/cmd
    state_topic: garage/door/state

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]

'homeassistant.const.STATE_OPEN' imported but unused

[houndci-bot]

line too long (84 > 79 characters)

[cdce8p]

You don't need to change the comment for `domain == 'cover'. That's just for covers, since they aren't fully supported yet.

Regarding the if statement, maybe use:

        if device_class == 'garage' and \
                features & (SUPPORT_OPEN | SUPPORT_CLOSE):
            _LOGGER.debug('Add "%s" as "%s"',
                          state.entity_id, 'GarageDoorOpener')
            return TYPES['GarageDoorOpener'](hass, state.entity_id, state.name,
                                             aid=aid)
        elif features & SUPPORT_SET_POSITION:
            _LOGGER.debug('Add "%s" as "%s"',
                          state.entity_id, 'WindowCovering')
            return TYPES['WindowCovering'](hass, state.entity_id, state.name,
                                           aid=aid)

That way you also catch the corner case that a garage door somehow supports set_cover_position.

[cdce8p]

You can combine those if statements:

        if value == 0:
            self.char_current_state.set_value(2)
            self.hass.components.cover.open_cover(self.entity_id)
        elif value == 1:
            self.char_current_state.set_value(3)
            self.hass.components.cover.close_cover(self.entity_id)

[cdce8p]

Use this instead:

    def update_state(self, entity_id=None, old_state=None, new_state=None):
        """Update cover state after state changed."""
        if new_state is None:
            return

        hass_state = new_state.state
        if hass_state in (STATE_OPEN, STATE_CLOSED):
            current_state = 0 if hass_state == STATE_OPEN else 1
            self.char_current_state.set_value(current_state)
            if not self.flag_target_state:
                self.char_target_state.set_value(current_state)
            self.flag_target_state = False

You can remove self.current_state, since only the states open and closed will be handled that way. Opening, Closing, etc. will be ignored.

[cdce8p]

Isn't needed

[cdce8p]

You don't need to add this char if it's not used.

[houndci-bot]

'homeassistant.components.cover.SUPPORT_STOP' imported but unused

[marthoc]

Thanks @cdce8p for the review comments! I pushed a bunch of fixes to address your comments and the Hound issues.

Two questions:

1. Regarding this changed section in init.py:

if device_class == 'garage' and \
                features & (SUPPORT_OPEN | SUPPORT_CLOSE):

Are there covers that don't support OPEN and CLOSE? Just wondering if the bitwise AND is necessary here or if device_class garage is sufficient; not that including the SUPPORT_OPEN | SUPPORT_CLOSE section is harmful in any way (and I suppose it does protect against some strange cover implementation from breaking HomeKit). Issue #13782 raises the problem of some garage door covers that support set_position being wrongly added to HomeKit now as WindowCoverings so the reordering in this section should close that issue.

2. The reason that in update_state I included the catch-all to treat states other than closed as open is because I noticed in testing that when the Hass state changed to 'unavailable' (as can happen with MQTT covers) the Home app acted strangely (got stuck on Opening...). Unfortunately the Garage Door implementation in HomeKit doesn't appear to have an Unknown state (like for example the lock implementation does). Is there a way to set the accessory as "Unavailable" or something else when new_state is UNKNOWN or UNAVAILABLE? We could include an else: to the if hass_state in (STATE_OPEN, STATE_CLOSED): block in the update_state method that would set that state in HomeKit. Thoughts?

[cdce8p]

To 1.: It's just a fail safe. I'm not sure if there are garage doors that only support set_cover_position.

To 2.: We are working on a solution for the unavailable case. The issue with HomeKit is because HomeKit generates those states based on the difference between target and current state. If e.g. you open the garage in the Home app, the target state wilk be set to open will the current state remains at closed. That is resolved when we set the current state to open as well.
Treating all other states except open as closed could result in a situation where a open cover will be displayed as closed just because it's currently unavailable.

[cdce8p]

Can you add tests and the GitHub doc?

[marthoc]

Just pushed some tests; will open a doc PR shortly.

Here's my thinking on the update_state method: with the code as it currently stands, if the door's new_state is UNKNOWN or UNAVAILABLE, then the if statement will be missed and the door will stay in it's previous open/closed state in HomeKit.

My thinking was that it would be better to at least catch the UNKNOWN/UNAVAILABLE states and change the state in HomeKit to open (given the lack of a better HomeKit state like Unknown) to at least alert the user that something had gone wrong. But, I realize that this may not be the best way to do it.

Also: would you like me to remove CHAR_OBSTRUCTION_DETECTED from .const given that we aren't using it, or should I leave it in in the event we can support it in the future?

Edit: I suppose I'll need to remove it since it's causing the linting to fail.

[marthoc]

Doc PR opened and I removed CHAR_OBSTRUCTION_DETECTED - this should solve the linting issue.

[cdce8p]

My thinking was that it would be better to at least catch the UNKNOWN/UNAVAILABLE states and change the state in HomeKit to open (given the lack of a better HomeKit state like Unknown) to at least alert the user that something had gone wrong. But, I realize that this may not be the best way to do it.

I understand your point. Until there is a good solution (see ikalchev/HAP-python#75), I would like to stay consistent over all different types and the way they are handling those cases is by not reacting to them.

---

During #13707 we introduced some changes to the homekit core. If it's ok with you, I would rebase your branch and push the required changes before merging this PR.

[marthoc]

@cdce8p Re: the not reacting to UNKNOWN/UNAVAILABLE states, I agree with you. Just wanted to make sure we are on the same page.

Do you want me to do the rebase or are you saying you'll do it? If you want to do it, please go ahead.

[cdce8p]

Rebase

I already started ;) Should be done soon.

[cdce8p]

Only waiting for travis to finish, before I merge it.

[cdce8p]

Thanks 🥇

[ybizeul]

Spent. 3 days. working. on exactly this. Rebase nightmare until I understand @marthoc did it!

Life is a b*tch !

Thanks !