New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scenes as switches HomeKit #17799

Merged
merged 3 commits into from Nov 5, 2018

Conversation

Projects
None yet
6 participants
@quthla
Contributor

quthla commented Oct 25, 2018

Description:

Adds scenes as switches to HomeKit

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

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • Documentation added/updated in home-assistant.io
  • Tests have been added to verify that the new code works.
@@ -170,7 +170,8 @@ def get_accessory(hass, driver, state, aid, config):
switch_type = config.get(CONF_TYPE, TYPE_SWITCH)
a_type = SWITCH_TYPES[switch_type]
elif state.domain in ('automation', 'input_boolean', 'remote', 'script'):
elif state.domain in ('automation', 'input_boolean', 'remote', 'script',
'scene'):

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

continuation line over-indented for visual indent

@cdce8p cdce8p self-assigned this Oct 25, 2018

@cdce8p

This comment has been minimized.

Member

cdce8p commented Oct 25, 2018

I'll do a full review and testing on this PR and #17793 Sunday. This weekend will be a busy one for me.

@quthla quthla force-pushed the quthla:homekit_scenes branch from b0ee299 to 9032c47 Oct 25, 2018

@quthla

This comment has been minimized.

Contributor

quthla commented Oct 25, 2018

Yeah no worries. Scenes have the same issue as scripts before #17793

Maybe we need the first variant of #17793 for that

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

I don't like this. Why can't we expose scenes as scenes?

@cdce8p

This comment has been minimized.

Member

cdce8p commented Oct 26, 2018

@balloob A scene accessory doesn't exist for HomeKit. They can only be created within the Home App. The only way is to expose it is as a switch witch calls scene.turn_on if activated and turns off automatically.

@quthla

This comment has been minimized.

Contributor

quthla commented Oct 28, 2018

@cdce8p shall I add the code for automatic turn off back and limit it to scenes?

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 3, 2018

As described here: #17793 (comment)
I believe that we shouldn't add the automatic turn_off feature. However adding scenes like we do with not cancel_able scripts should be fine. In the end it's more a hack then a correct implementation. Maybe we can add a note about that behavior in the documentation?

@cdce8p

To finish this PR, you would just need to add a test case here: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/homekit/test_get_accessories.py#L138-L139
and update the documentation.

@@ -170,7 +170,8 @@ def get_accessory(hass, driver, state, aid, config):
switch_type = config.get(CONF_TYPE, TYPE_SWITCH)
a_type = SWITCH_TYPES[switch_type]
elif state.domain in ('automation', 'input_boolean', 'remote', 'script'):
elif state.domain in ('automation', 'input_boolean', 'remote', 'script',
'scene'):

This comment has been minimized.

@cdce8p

cdce8p Nov 3, 2018

Member

Can you move 'scene' before 'script' to be sorted alphabetically?

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 4, 2018

We would also need to integrate a check inside the set_state method not to call scene.turn_off.

        if self._domain == 'scene' and value == 0:
            _LOGGER.debug('%s: Ignoring turn_off call', self.entity_id)
            return

Adding this right after the debug statement should work: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/homekit/type_switches.py#L81-L88

@quthla quthla force-pushed the quthla:homekit_scenes branch from 9032c47 to ca00c9d Nov 5, 2018

@wafflebot wafflebot bot added the in progress label Nov 5, 2018

@quthla

This comment has been minimized.

Contributor

quthla commented Nov 5, 2018

@cdce8p Is this ready for merge then?

@cdce8p

Since I added extra tests for reset_switch, you would need to add the scene to it as well: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/homekit/test_type_switches.py#L182-L183
Lastly you would need to open a doc PR, to add scenes to the list of supported components (inside the HomeKit docs).

@@ -86,7 +86,7 @@ def __init__(self, *args):
def is_activate(self, state):
"""Check if entity is activate only."""
can_cancel = state.attributes.get(ATTR_CAN_CANCEL)
if self._domain == 'script' and not can_cancel:
if self._domain in ['script', 'scene'] and not can_cancel:

This comment has been minimized.

@cdce8p

cdce8p Nov 5, 2018

Member

scenes don't have the can_cancel attribute. So although this would technical work, I would like it better to be a separate if statement below the first (to just check if the domain is scene).

This comment has been minimized.

@quthla

quthla Nov 5, 2018

Contributor

I did it so it can be in one line but will change it

@quthla quthla referenced this pull request Nov 5, 2018

Merged

Add scenes as switches #7371

2 of 2 tasks complete

@quthla quthla force-pushed the quthla:homekit_scenes branch from ca00c9d to adc3ba3 Nov 5, 2018

@quthla

This comment has been minimized.

Contributor

quthla commented Nov 5, 2018

@quthla quthla force-pushed the quthla:homekit_scenes branch from adc3ba3 to ca4be34 Nov 5, 2018

cdce8p added some commits Nov 5, 2018

@cdce8p cdce8p added the new-feature label Nov 5, 2018

@cdce8p cdce8p changed the title from Add scenes as switches to Add scenes as switches HomeKit Nov 5, 2018

@cdce8p

cdce8p approved these changes Nov 5, 2018

@cdce8p cdce8p merged commit c59b038 into home-assistant:dev Nov 5, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 93.1%
Details

@wafflebot wafflebot bot removed the in progress label Nov 5, 2018

@cdce8p

This comment has been minimized.

Member

cdce8p commented Nov 5, 2018

@quthla Thanks for the patience with getting this done 🐬

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment