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

Re-implement HomematicIP cloud to async #13468

Merged
merged 15 commits into from Apr 25, 2018
220 changes: 132 additions & 88 deletions homeassistant/components/homematicip_cloud.py
Expand Up @@ -5,137 +5,186 @@
https://home-assistant.io/components/homematicip_cloud/
"""

import asyncio
import logging
from socket import timeout

import voluptuous as vol

from homeassistant.core import callback
from homeassistant.const import EVENT_HOMEASSISTANT_STOP
from homeassistant.helpers.aiohttp_client import async_get_clientsession
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.dispatcher import (dispatcher_send,
async_dispatcher_connect)
from homeassistant.helpers.discovery import load_platform
from homeassistant.helpers.discovery import async_load_platform
from homeassistant.helpers.entity import Entity

REQUIREMENTS = ['homematicip==0.8']
REQUIREMENTS = ['homematicip==0.9.1']

_LOGGER = logging.getLogger(__name__)

DOMAIN = 'homematicip_cloud'

COMPONTENTS = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a typo:
COMPONTENTS -> COMPONENTS.

'sensor',
# 'binary_sensor',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you want to implement these in the future, but we usually don't want to have commented code.

# 'climate',
# 'switch',
# 'light',
# 'alarm_control_panel'
]

CONF_NAME = 'name'
CONF_ACCESSPOINT = 'accesspoint'
CONF_AUTHTOKEN = 'authtoken'

CONFIG_SCHEMA = vol.Schema({
vol.Optional(DOMAIN): [vol.Schema({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an empty list as default value, and add cv.ensure_list to the validation.

vol.Optional(DOMAIN, default=[]): vol.All(cv.ensure_list, [vol.Schema({...})]),

vol.Optional(CONF_NAME, default=''): cv.string,
vol.Optional(CONF_NAME, default=None): vol.Any(None, cv.string),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't default to None. You can revert this change. Either default to some cool string, or leave it not set.

vol.Required(CONF_ACCESSPOINT): cv.string,
vol.Required(CONF_AUTHTOKEN): cv.string,
})],
}, extra=vol.ALLOW_EXTRA)

EVENT_HOME_CHANGED = 'homematicip_home_changed'
EVENT_DEVICE_CHANGED = 'homematicip_device_changed'
EVENT_GROUP_CHANGED = 'homematicip_group_changed'
EVENT_SECURITY_CHANGED = 'homematicip_security_changed'
EVENT_JOURNAL_CHANGED = 'homematicip_journal_changed'
HMIP_ACCESS_POINT = 'Access Point'
HMIP_HUB = 'HmIP-HUB'

ATTR_HOME_ID = 'home_id'
ATTR_HOME_LABEL = 'home_label'
ATTR_HOME_NAME = 'home_name'
ATTR_DEVICE_ID = 'device_id'
ATTR_DEVICE_LABEL = 'device_label'
ATTR_STATUS_UPDATE = 'status_update'
ATTR_FIRMWARE_STATE = 'firmware_state'
ATTR_UNREACHABLE = 'unreachable'
ATTR_LOW_BATTERY = 'low_battery'
ATTR_MODEL_TYPE = 'model_type'
ATTR_GROUP_TYPE = 'group_type'
ATTR_DEVICE_RSSI = 'device_rssi'
ATTR_DUTY_CYCLE = 'duty_cycle'
ATTR_CONNECTED = 'connected'
ATTR_SABOTAGE = 'sabotage'
ATTR_RSSI = 'rssi'
ATTR_TYPE = 'type'
ATTR_OPERATION_LOCK = 'operation_lock'


class HomematicipConnector:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this below async_setup to follow our normal pattern.

"""Manages HomematicIP http and websocket connection."""

def __init__(self, config, websession, hass):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest putting hass first in the signature. That's an existing pattern.

"""Initialize HomematicIP cloud connection."""
from homematicip.async.home import AsyncHome
self._hass = hass
self._ws_close_requested = False
self._retry_task = None
self._tries = 0
self._accesspoint = config.get(CONF_ACCESSPOINT)
self._authtoken = config.get(CONF_AUTHTOKEN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This instance attribute is never used outside this method. Please just store it in a local variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mxworm I guess what @MartinHjelmare means it is not necessary to store this as a instance attribute as you only use it once two lines lower. So no real need to store it. In your last change you've made it a class attribute which is not what we want: That makes the _authtoken the same across all instances of HomematicipConnector possibly resulting in errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sander76 so more like:
self.home.set_auth_token(config.get(CONF_AUTHTOKEN))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the current diff is good. It's not a class attribute as long as it's just defined inside init.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man ! what am I thinking. @MartinHjelmare is absolutely correct. Must be the hot weather ;-)


self.home = AsyncHome(hass.loop, websession)
self.home.set_auth_token(self._authtoken)

def setup(hass, config):
hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, self.close())

async def init(self):
"""Initial connection."""
await self.home.init(self._accesspoint)
await self.home.get_current_state()

async def _handle_connection(self):
"""Handle websocket connection."""
from homematicip.base.base_connection import HmipConnectionError

await self.home.get_current_state()
hmip_events = await self.home.enable_events()
try:
await hmip_events
except HmipConnectionError:
return
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return is not needed.


async def connect(self):
"""Start websocket connection."""
self._tries = 0
while True:
try:
await self._handle_connection()
except Exception: # pylint: disable=broad-except
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the broad except? You don't take any action here, so it will try again and probably fail again and keep spamming the log.

_LOGGER.exception("Unexpected error")
if self._ws_close_requested:
break
self._ws_close_requested = False
self._tries += 1
try:
self._retry_task = self._hass.async_add_job(asyncio.sleep(
2 ** min(9, self._tries), loop=self._hass.loop))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to wait the _retry_task otherwise the sleep command just wanders off and does not make the loop wait.

await self._retry_task

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

except asyncio.CancelledError:
break
_LOGGER.info('Reconnect (%s) to the HomematicIP cloud server.',
self._tries)

async def close(self):
"""Close the websocket connection."""
self._ws_close_requested = True
if self._retry_task is not None:
self._retry_task.cancel()
await self.home.disable_events()
_LOGGER.info("Closed connection to HomematicIP cloud server.")


async def async_setup(hass, config):
"""Set up the HomematicIP component."""
# pylint: disable=import-error, no-name-in-module
from homematicip.home import Home
from homematicip.base.base_connection import HmipConnectionError

hass.data.setdefault(DOMAIN, {})
homes = hass.data[DOMAIN]
_LOGGER.info('Loading config for HomematicIP cloud connection.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this. The core will already log the setup at info level.

accesspoints = config.get(DOMAIN, [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can put the default empty list in the config schema and then not use dict.get but just dict[key].


def _update_event(events):
"""Handle incoming HomeMaticIP events."""
for event in events:
etype = event['eventType']
edata = event['data']
if etype == 'DEVICE_CHANGED':
dispatcher_send(hass, EVENT_DEVICE_CHANGED, edata.id)
elif etype == 'GROUP_CHANGED':
dispatcher_send(hass, EVENT_GROUP_CHANGED, edata.id)
elif etype == 'HOME_CHANGED':
dispatcher_send(hass, EVENT_HOME_CHANGED, edata.id)
elif etype == 'JOURNAL_CHANGED':
dispatcher_send(hass, EVENT_SECURITY_CHANGED, edata.id)
return True

for device in accesspoints:
name = device.get(CONF_NAME)
accesspoint = device.get(CONF_ACCESSPOINT)
authtoken = device.get(CONF_AUTHTOKEN)

home = Home()
if name.lower() == 'none':
name = ''
home.label = name
for conf in accesspoints:
_websession = async_get_clientsession(hass)
_hmip = HomematicipConnector(conf, _websession, hass)
try:
home.set_auth_token(authtoken)
home.init(accesspoint)
if home.get_current_state():
_LOGGER.info("Connection to HMIP established")
else:
_LOGGER.warning("Connection to HMIP could not be established")
return False
except timeout:
_LOGGER.warning("Connection to HMIP could not be established")
await _hmip.init()
except HmipConnectionError:
_LOGGER.error('Failed to connect to the HomematicIP cloud server.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the access point info stored in CONF_ACCESSPOINT. That's not sensitive info, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense :-)

return False
homes[home.id] = home
home.onEvent += _update_event
home.enable_events()
_LOGGER.info('HUB name: %s, id: %s', home.label, home.id)

for component in ['sensor']:
load_platform(hass, component, DOMAIN, {'homeid': home.id}, config)
home = _hmip.home
home.name = conf.get(CONF_NAME)
home.label = HMIP_ACCESS_POINT
home.modelType = HMIP_HUB

hass.data[DOMAIN][home.id] = home
_LOGGER.info('Connected to the HomematicIP cloud server.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

homeid = {ATTR_HOME_ID: home.id}
for component in COMPONTENTS:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct typo.

hass.async_add_job(async_load_platform(hass, component, DOMAIN,
homeid, config))

hass.loop.create_task(_hmip.connect())
return True


class HomematicipGenericDevice(Entity):
"""Representation of an HomematicIP generic device."""

def __init__(self, home, device):
def __init__(self, home, device, post=None):
"""Initialize the generic device."""
self._home = home
self._device = device
self.post = post
_LOGGER.info('Setting up %s (%s)', self._name(),
self._device.modelType)

async def async_added_to_hass(self):
"""Register callbacks."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ever called ?

async_dispatcher_connect(
self.hass, EVENT_DEVICE_CHANGED, self._device_changed)
self._device.on_update(self._device_changed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just add this to the init method.


@callback
def _device_changed(self, deviceid):
def _device_changed(self, json, **kwargs):
"""Handle device state changes."""
if deviceid is None or deviceid == self._device.id:
_LOGGER.debug('Event device %s', self._device.label)
self.async_schedule_update_ha_state()

def _name(self, addon=''):
"""Return the name of the device."""
name = ''
if self._home.label != '':
name += self._home.label + ' '
name += self._device.label
if addon != '':
name += ' ' + addon
_LOGGER.debug('Event %s (%s)', self._name(), self._device.modelType)
self.async_schedule_update_ha_state()

def _name(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't need to pass an argument anymore, you can make this a property, that is inherited in the child classes:

@property
def name(self):

"""Return the name of the generic device."""
name = self._device.label
if self._home.name is not None:
name = self._home.name + ' ' + name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use str.format and not string concatenation.

if self.post is not None:
name = name + ' ' + self.post
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

return name

@property
Expand All @@ -153,24 +202,19 @@ def available(self):
"""Device available."""
return not self._device.unreach

def _generic_state_attributes(self):
@property
def device_state_attributes(self):
"""Return the state attributes of the generic device."""
laststatus = ''
if self._device.lastStatusUpdate is not None:
laststatus = self._device.lastStatusUpdate.isoformat()
return {
ATTR_HOME_LABEL: self._home.label,
ATTR_HOME_NAME: self._home.name,
ATTR_DEVICE_LABEL: self._device.label,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We re storing way too many things in the attributes here. Attributes should only be used to store things that describe the STATE of a device. I am not sure how all these other things came in here, but they should be removed.

Example of a good attribute: a light has the state "on". The attributes explain that the color is red.

Anything that does not explain the state, should be removed. Examples are unreachable, firmware state or status update (this one is already described by looking at the last_updated attribute of the state).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove all unnecessary things from the sensors as well as the status sensor for components. You are right it might confuse users.

ATTR_HOME_ID: self._device.homeId,
ATTR_DEVICE_ID: self._device.id.lower(),
ATTR_STATUS_UPDATE: laststatus,
ATTR_FIRMWARE_STATE: self._device.updateState.lower(),
ATTR_UNREACHABLE: self._device.unreach,
ATTR_LOW_BATTERY: self._device.lowBat,
ATTR_RSSI: self._device.rssiDeviceValue,
ATTR_TYPE: self._device.modelType
ATTR_DEVICE_RSSI: self._device.rssiDeviceValue,
ATTR_MODEL_TYPE: self._device.modelType
}

@property
def device_state_attributes(self):
"""Return the state attributes of the generic device."""
return self._generic_state_attributes()