Skip to content
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 support for HomeKit #12488

Merged
merged 9 commits into from Feb 19, 2018

Conversation

Projects
None yet
8 participants
@cdce8p
Copy link
Member

commented Feb 18, 2018

Description:

After reading about HAP-Python in the forum, I tried a few things and ended up with this. It should add the bare bone support for homekit to Home Assistant and is quite easy expendable. For now I just added support for two components: temperature sensors and covers (with set_position support).

Tested with: HAP-python==1.1.2

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

Example entry for configuration.yaml (if applicable):

homekit:

cover:
  - platform: demo
sensor:
  - platform: yr
    monitored_conditions:
      - temperature

Checklist:

  • The code change is tested and works locally.

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

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • 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.
Basic Homekit support
* Added Temperatur Sensor
* Added Window Cover
Code refactored
* Added class HomeAccessory(Accessory)
* Added class HomeBridge(Bridge)
* Changed homekit imports to relative, to enable use in custom_components
* Updated requirements
* Added docs
* Other smaller changes

import voluptuous as vol

from pyhap.accessory_driver import AccessoryDriver

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Imports of dependencies should happen inside functions. They are not allowed to be in the main level because of how dependency installation works.

To work around this, you can move it to new files (like you already did), but then also only import those files inside functions.

def async_setup(hass, config):
    from .accessories import HomeBridge
CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.All({
vol.Optional(CONF_NAME): cv.string,
vol.Optional(CONF_IP_ADDRESS): cv.string,

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

No need for a config. Use homeassistant.util.get_local_ip()

types = conf.get(CONF_TYPES)

yield from component.async_add_entities([
Homekit(hass, name, ip_address, port, pin, types)])

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

This shouldn't be an entity. This should just be a class.


CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.All({
vol.Optional(CONF_NAME): cv.string,

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

No need for a name as we don't need an entity for it.

name = conf.get(CONF_NAME, 'homeassistant')
ip_address = conf.get(CONF_IP_ADDRESS, '127.0.0.1')
port = conf.get(CONF_PORT, 51826)
pin = conf.get(CONF_PIN_CODE, '123-45-678')

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Put the defaults inside the schema by passing the default keyword to vol.Optional or vol.Required

CONF_PIN_CODE = 'pincode'
CONF_TYPES = 'types'

ALL_TYPES = {

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Instead of registering it like this, follow the example in Alexa and use our registry:

# In __init__.py
from homeassistant.util.decorator import Registry

TYPES = Registry()
# Then in cover.py
from . import TYPES

@TYPES.register('Cover')
class Window(HomeAccessory):
    …

You only have to import the cover file once in your setup and they will be added to your TYPES constant, to be used whenever you want.

vol.Optional(CONF_PIN_CODE): cv.string,
vol.Required(CONF_TYPES): vol.Schema({
vol.Required(valid_typ): vol.Schema({
vol.Required(cv.entity_id): valid_string,

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Instead of valid_string, do vol.Any(cv.string, None)

vol.Optional(CONF_IP_ADDRESS): cv.string,
vol.Optional(CONF_PORT): vol.Coerce(int),
vol.Optional(CONF_PIN_CODE): cv.string,
vol.Required(CONF_TYPES): vol.Schema({

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

It doesn't make much sense to have the user define that a cover entity needs to be exposed as a cover in homekit. We should be able to figure that out ourselves.

By default we should expose everything we know and allow the user to configure a filter, just like we do for Alexa or Google Home. Alexa example code

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

For the initial PR, I think it is totally fine to leave filtering out and just expose all covers and temp sensors by name as they are known in Home Assistant.

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Add this to async_setup to just loop through all entities:

for state in hass.states.async_all():
    if state.domain == 'cover':
        # Register cover entity
    elif# etc
@asyncio.coroutine
def async_added_to_hass(self):
"""Register callbacks."""
@callback

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Move all this code to the setup method.

_LOGGER.debug('Driver start')
self.driver = AccessoryDriver(self.bridge, self._port,
self._ip_address, self.path)
self.driver.start()

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Is this starting a thread? or is this doing I/O ? This is an async context (because of @callback decorator) and thus you cannot do I/O

def homeassistant_stop(event):
"""Stop the accessory drive after HA stop is called."""
_LOGGER.debug('Driver stop')
self.driver.stop()

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Same.

'cover', 'set_cover_position',
{'entity_id': self._entity_id, 'position': value})

@callback

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Only annotate things with callback if you know that no I/O is done inside it. So for example, the cal to set_value() below, is that blocking or is that instant?

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 18, 2018

Off to a great start! Will need some tweaking to get it working with Home Assistant. I think that once the foundation is in place, it will be easier for people to extend. Please don't add extra device support to this PR (beyond temp sensor and cover) and let's make sure the foundation is right first.

Changed Homekit from entity to class
* Changes based on feedback
* Updated config schema
* Add only covers that support set_cover_position
@cdce8p

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2018

I just pushed a new version which should address all your comments.

def set_services(self, services):
"""Define the services to be available for the accessory."""
from pyhap.loader import get_serv_loader
self.add_service(get_serv_loader().get(services))

This comment has been minimized.

Copy link
@ikalchev

ikalchev Feb 18, 2018

loader gets a single Service, maybe remove plural?

This comment has been minimized.

Copy link
@cdce8p

cdce8p Feb 18, 2018

Author Member

The idea was to handle all service loading from this method. Maybe later extend the functionality to support lists as input, hence services. For know service should be enough.

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 18, 2018

As long as those files are never imported, it's indeed not high priority as it is not consuming extra memory.

I wonder how many accessories you'll need once the Home Assistant integration is done and you can just forward any Home Assistant integration over Homekit?

Addressed comments, updated to pyhap==1.1.5
* For lint: added files to gen_requirements_all
* Added codeowner
self.bridge.add_accessory(acc)

ip_address = get_local_ip()
# ip_address = yield from self._hass.async_add_job(get_local_ip)

This comment has been minimized.

Copy link
@cdce8p

cdce8p Feb 18, 2018

Author Member

ip_address = yield from self._hass.async_add_job(get_local_ip) didn't work.

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Correct, because that's only valid inside async context. Inside a sync context, you can just call the method.

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

You can remove the comment

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 18, 2018

This is ok to merge when some tests get added.

"""Start the accessory driver."""
from pyhap.accessory_driver import AccessoryDriver
_LOGGER.debug('Start driver')
self._hass.bus.async_listen_once(

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 18, 2018

Member

Since this is a sync context, you should call self._hass.bus.listen_once.

EVENT_HOMEASSISTANT_STOP, self.stop_driver)

_LOGGER.debug('Start adding accessories.')
for state in self._hass.states.async_all():

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 18, 2018

Member

Sync context.

if temperature != STATE_UNKNOWN:
self.char_temp.set_value(float(temperature))

if self._entity_id == 'sensor.weather_1':

This comment has been minimized.

Copy link
@amelchio

amelchio Feb 18, 2018

Contributor

I'm not sure but this looks like debugging code?

This comment has been minimized.

Copy link
@cdce8p

cdce8p Feb 18, 2018

Author Member

Year... Will be removed with the next push.

@cdce8p cdce8p referenced this pull request Feb 18, 2018

Closed

Accessory Object parameters #45

cdce8p added some commits Feb 18, 2018

Small changes, added tests
* Homekit class: removed add_accessory since it's already covered by pyhap
* Added test requirement: HAP-python
* Added test suit for homekit setup and interaction between HA and pyhap
* Added test suit for get_accessories function

@balloob balloob changed the title RFC: New Homekit component Add support for HomeKit Feb 19, 2018


def move_cover(self, value):
"""Move cover to value if call came from homekit."""
if value != self.current_position:

This comment has been minimized.

Copy link
@balloob

balloob Feb 19, 2018

Member

Is this up to us to filter out? What if a cover has an assumed state and our assumption is off?

(we can change this in a future PR if we want)

This comment has been minimized.

Copy link
@cdce8p

cdce8p Feb 19, 2018

Author Member

self.current_position isn't the same as the state attribute current_position. Unfortunatly right now the way pyhap handles changes in HomeKit is by calling the same method as HomeAssistant is calling for changes to the Homekit Status. Without this if clause something like this could happen:

  • HomeKit sets cover position form 0 to 100
  • HomeAssistant state listener triggers the same method, after an initial move of the cover and sets its position to the current_position attribute of the new_state, which almost always won't be 100 (more like 10 or so)

Their might be a better way to do it though, but thats sertainly a topic for another PR.

@balloob balloob merged commit eec3bad into home-assistant:dev Feb 19, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
coverage/coveralls Coverage increased (+0.06%) to 94.072%
Details
hound No violations found. Woof!
@balloob

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

Oh snap, I just saw that the Py35 test failures actually were Homekit failures. @cdce8p can you have a look what is going on? Wouldn't want to add flaky tests

https://travis-ci.org/home-assistant/home-assistant/jobs/343531242#L962

ERROR:homeassistant.core:Error doing job: Future exception was never retrieved
Traceback (most recent call last):
  File "/opt/python/3.5.3/lib/python3.5/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/travis/build/home-assistant/home-assistant/homeassistant/components/homekit/__init__.py", line 133, in stop_driver
    self.driver.stop()
  File "/home/travis/build/home-assistant/home-assistant/.tox/py35/lib/python3.5/site-packages/pyhap/accessory_driver.py", line 491, in stop
    self.run_sentinel.set()
AttributeError: 'NoneType' object has no attribute 'set'
@cdce8p

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

I'm on it. Missed that one, since I'm running 3.6.

@arsaboo

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

@cdce8p we will also need docs for this.

@cdce8p cdce8p deleted the cdce8p:homekit branch Feb 20, 2018

@cdce8p

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@arsaboo Will work on that next

@simse

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

Just be clear, this doesn't add support for lights and switches yet, or am I mistaken?

@arsaboo

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

Indeed, only cover and temperature sensors are supported right now.

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

@fattdev PR welcome 👍

@simse

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

@balloob Would love to!

@cdce8p cdce8p referenced this pull request Feb 20, 2018

Merged

Added doc for homekit #4720

2 of 2 tasks complete

@balloob balloob referenced this pull request Feb 22, 2018

Merged

0.64.0 #12609

@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.