Add HomeKit support for fans #14351

schmittx · 2018-05-09T03:45:40Z

Description:

Adds support for fans in HomeKit. In addition to basic on/off it uses supported_features to add additional characteristics for direction and oscillation (in HomeKit, oscillation = swing_mode).

Support for speed is not being implemented yet due to the current architecture of speed_list. Refer to home-assistant/architecture#27 for more details. Once an improved architecture for speed_list is set and entities updated, HomeKit will be updated to support speed.

Related issue (if applicable): N/A

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

Example entry for `configuration.yaml` (if applicable):

homekit:
  filter:
    include_domains:
      - fan

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 communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

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

cdce8p

This PR is already looking quite good. I didn't had time to test it yet, but nevertheless added a few small comments.

cdce8p · 2018-05-09T07:09:14Z

homeassistant/components/homekit/type_fans.py

+    def __init__(self, *args, config):
+        """Initialize a new Light accessory object."""
+        super().__init__(*args, category=CATEGORY_FAN)
+        self._domain = split_entity_id(self.entity_id)[0]


Is their a reason you use split_entity_id instead of DOMAIN imported from components.fan?
As I see it, it should be enough to use the later one.

No reason, just copied the base code from type_switches. I agree though, I've updated to import from components.fan

cdce8p · 2018-05-09T07:12:31Z

homeassistant/components/homekit/type_fans.py

+            CHAR_ACTIVE, value=self._state, setter_callback=self.set_state)
+
+        if CHAR_ROTATION_SPEED in self.chars:
+            speed_list = self.hass.states.get(self.entity_id) \


Does this change over time? At the moment no homekit type accounts for chnages in supported features, so I think changing this to self.speed_list would work.

No, you're correct in that it doesn't change after being initialized at startup.

cdce8p · 2018-05-09T07:19:37Z

homeassistant/components/homekit/type_fans.py

+                .attributes.get(ATTR_SPEED_LIST)
+            speed = speed_list[int(round(value / self.min_step)) - 1]
+            self.hass.components.fan.turn_on(
+                self.entity_id, speed=speed)


Can you use hass.services.call instead. Also for the ones below.
I just haven't gotten around to harmonize that for the other types.

No problem, I can help with updating the other types accordingly too.

No need to do that now. I'm planing to start converting the first methods to async, maybe as soon as this weekend. I can do it then.

That in mind it would be great if we could finish up on #14159 first. As a heads up: I'm rewriting almost all tests at the moment to use pytest and async. Should get the PR out in the next few hours. It might require a rebase then.

Sounds good.

I'll get #14159 wrapped up tonight so that can be closed out.

Regarding tests for this PR, should I wait for your new PR, then rebase and add/update for fans?

I just opened the PR #14377

The foundations stayed the same. It might be best if you take a look at one of the new type tests and write the new tests based on that structure. They should work even without the PR being merged.

However, it is likely that #14159 needs to be rebased once the PR is merged.

Understood, I'll look at the new tests tonight and update this PR.

I just merged the test PR, should be good to rebase now.

cdce8p · 2018-05-09T07:21:39Z

homeassistant/components/homekit/type_fans.py

+        """Set state if call came from HomeKit."""
+        _LOGGER.debug('%s: Set direction to %d', self.entity_id, value)
+        self._flag[CHAR_ROTATION_DIRECTION] = True
+        direction = HOMEKIT_TO_HASS[value]


Can we use if instead? Dictionary mapping is a bit much for just to values.

No problem, updated.

cdce8p · 2018-05-09T07:52:22Z

homeassistant/components/homekit/type_fans.py

+            .attributes.get(ATTR_SUPPORTED_FEATURES)
+        if self._features & SUPPORT_SET_SPEED:
+            self.chars.append(CHAR_ROTATION_SPEED)
+            self._speed = None


What reason is their to define it as class variable? As I see it, it's only used in update_state. Same for self._direction and self._oscillating.

Sorry, I just copied the base code from type_lights. It's not needed so I removed.

cdce8p · 2018-05-09T07:53:47Z

homeassistant/components/homekit/type_fans.py

+            direction = new_state.attributes.get(ATTR_DIRECTION)
+            if not self._flag[CHAR_ROTATION_DIRECTION] and \
+                    direction in HASS_TO_HOMEKIT:
+                self._direction = HASS_TO_HOMEKIT[direction]


Same as before. I think if is more than enough here.

No problem, updated.

houndci-bot · 2018-05-11T01:50:17Z

tests/components/homekit/test_type_fans.py

+        acc.char_direction.client_update_value(1)
+        self.hass.block_till_done()
+        self.assertEqual(self.events[1].data[ATTR_DOMAIN], DOMAIN)
+        self.assertEqual(self.events[1].data[ATTR_SERVICE], SERVICE_SET_DIRECTION)


line too long (82 > 79 characters)

houndci-bot · 2018-05-11T01:50:17Z

tests/components/homekit/test_type_fans.py

+        acc.char_direction.client_update_value(0)
+        self.hass.block_till_done()
+        self.assertEqual(self.events[0].data[ATTR_DOMAIN], DOMAIN)
+        self.assertEqual(self.events[0].data[ATTR_SERVICE], SERVICE_SET_DIRECTION)


line too long (82 > 79 characters)

tests/components/homekit/test_type_fans.py

+import unittest
+
+from homeassistant.core import callback
+from homeassistant.components.fan import (


schmittx · 2018-05-11T02:22:13Z

@cdce8p Please merge #14159 first, I'll rebase/update this PR afterwards.

cdce8p · 2018-05-12T14:15:06Z

homeassistant/components/homekit/type_fans.py

+        if CHAR_ROTATION_DIRECTION in self.chars:
+            direction = new_state.attributes.get(ATTR_DIRECTION)
+            if not self._flag[CHAR_ROTATION_DIRECTION] and \
+                    direction in (DIRECTION_FORWARD, DIRECTION_REVERSE):


This currently doesn't work if set from the frontend. See home-assistant/frontend#1158

schmittx · 2018-05-15T01:35:41Z

@cdce8p I tested the latest commit, all good here. I also updated description to explain lack of speed support. PR for docs is updated as well. Ready to merge.

schmittx requested a review from cdce8p as a code owner May 9, 2018 03:45

homeassistant added cla-signed integration: homekit new-platform labels May 9, 2018

cdce8p self-assigned this May 9, 2018

cdce8p added new-feature and removed new-platform labels May 9, 2018

cdce8p requested changes May 9, 2018

View reviewed changes

homeassistant added the cla-signed label May 10, 2018

cdce8p mentioned this pull request May 10, 2018

Rewritten HomeKit tests #14377

Merged

2 tasks

houndci-bot reviewed May 11, 2018

View reviewed changes

schmittx mentioned this pull request May 11, 2018

Add HomeKit support for fans home-assistant/home-assistant.io#5346

Merged

2 tasks

cdce8p mentioned this pull request May 11, 2018

Async conversion HomeKit Types #14399

Closed

2 tasks

schmittx force-pushed the homekit-fan-support branch from 566d839 to 81aa101 Compare May 12, 2018 00:15

homeassistant added the cla-signed label May 12, 2018

schmittx changed the title ~~[WIP] Add HomeKit support for fans~~ Add HomeKit support for fans May 12, 2018

homeassistant added the cla-signed label May 12, 2018

cdce8p reviewed May 12, 2018

View reviewed changes

cdce8p mentioned this pull request May 14, 2018

Fan entity speed settings home-assistant/architecture#27

Closed

schmittx and others added 10 commits May 14, 2018 21:12

Initial commit

824a34c

Add support for rotation speed

aae3162

Clean up rotation speed

eefeb14

Clean up optional characteristics

92afdfc

Update direction characteristic

439c1d5

Update per comments

2c866c7

Add tests

d62095b

Hound fixes

8e8acc1

Update tests

e4641d3

Style changes

ff6c8a2

cdce8p added 6 commits May 14, 2018 21:12

Fix test

f1af5d3

Change speed assignment

669daa8

pytest.fixture yield

208720e

Readded minStep

5ef2ce5

Removed speed settings

b782083

Small change

14197ad

schmittx force-pushed the homekit-fan-support branch from 014e350 to 14197ad Compare May 15, 2018 01:12

homeassistant added the cla-signed label May 15, 2018

cdce8p approved these changes May 16, 2018

View reviewed changes

cdce8p merged commit 25dcddf into home-assistant:dev May 16, 2018

balloob mentioned this pull request May 28, 2018

0.70.0 #14657

Merged

schmittx deleted the homekit-fan-support branch June 2, 2018 00:04

cdce8p mentioned this pull request Jun 12, 2018

Added HomeKit fan speed #14942

Closed

2 tasks

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018

Add HomeKit support for fans (home-assistant#14351)

fd12304

home-assistant locked and limited conversation to collaborators Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HomeKit support for fans #14351

Add HomeKit support for fans #14351

schmittx commented May 9, 2018 •

edited

Loading

cdce8p left a comment

cdce8p May 9, 2018

schmittx May 10, 2018

cdce8p May 9, 2018

schmittx May 10, 2018

cdce8p May 9, 2018

schmittx May 10, 2018

cdce8p May 10, 2018

schmittx May 10, 2018

cdce8p May 10, 2018

schmittx May 10, 2018

cdce8p May 10, 2018

cdce8p May 9, 2018

schmittx May 10, 2018

cdce8p May 9, 2018

schmittx May 10, 2018

cdce8p May 9, 2018

schmittx May 10, 2018

houndci-bot May 11, 2018

houndci-bot May 11, 2018

This comment was marked as outdated.

schmittx commented May 11, 2018 •

edited

Loading

cdce8p May 12, 2018

schmittx commented May 15, 2018 •

edited

Loading

Add HomeKit support for fans #14351

Add HomeKit support for fans #14351

Conversation

schmittx commented May 9, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

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

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

This comment was marked as outdated.

schmittx commented May 11, 2018 • edited Loading

Choose a reason for hiding this comment

schmittx commented May 15, 2018 • edited Loading

schmittx commented May 9, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

schmittx commented May 11, 2018 •

edited

Loading

schmittx commented May 15, 2018 •

edited

Loading