Skip to content

Style change follow up #13707

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.

Sign up for GitHub

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

Merged
kellerza merged 12 commits into from
Apr 11, 2018
Merged

Style change follow up #13707

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
03eb379
Style improvements
cdce8p Apr 5, 2018
5243038
Rewritten get_accessory
cdce8p Apr 9, 2018
8703cb2
Added log statement
cdce8p Apr 9, 2018
2b05f1e
Return None
cdce8p Apr 10, 2018
cb0aad4
Pass config instead of **a_kwargs
cdce8p Apr 10, 2018
dfa7191
Thermostat: Moved feature check to type file
cdce8p Apr 10, 2018
99b67df
Init_setup
cdce8p Apr 10, 2018
2f7ce59
Small changes
cdce8p Apr 10, 2018
2793753
Use super().__init__(*args, category=CATEGORY)
cdce8p Apr 11, 2018
d99f9df
Fix light set_color_temperature call
cdce8p Apr 11, 2018
0af42de
Added setup_char helper function
cdce8p Apr 11, 2018
e0eabb1
Improved type_thermostat optional chars
cdce8p Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 33 additions & 48 deletions homeassistant/components/homekit/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
SUPPORT_TARGET_TEMPERATURE_HIGH, SUPPORT_TARGET_TEMPERATURE_LOW)
from homeassistant.components.cover import SUPPORT_SET_POSITION
from homeassistant.const import (
ATTR_CODE, ATTR_SUPPORTED_FEATURES, ATTR_UNIT_OF_MEASUREMENT,
ATTR_SUPPORTED_FEATURES, ATTR_UNIT_OF_MEASUREMENT,
CONF_PORT, TEMP_CELSIUS, TEMP_FAHRENHEIT,
EVENT_HOMEASSISTANT_START, EVENT_HOMEASSISTANT_STOP)
import homeassistant.helpers.config_validation as cv
Expand Down Expand Up @@ -79,63 +79,48 @@ def get_accessory(hass, state, aid, config):
state.entity_id)
return None

if state.domain == 'sensor':
unit = state.attributes.get(ATTR_UNIT_OF_MEASUREMENT)
if unit == TEMP_CELSIUS or unit == TEMP_FAHRENHEIT:
_LOGGER.debug('Add "%s" as "%s"',
state.entity_id, 'TemperatureSensor')
return TYPES['TemperatureSensor'](hass, state.entity_id,
state.name, aid=aid)
elif unit == '%':
_LOGGER.debug('Add "%s" as %s"',
state.entity_id, 'HumiditySensor')
return TYPES['HumiditySensor'](hass, state.entity_id, state.name,
aid=aid)
if state.domain == 'alarm_control_panel':

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Insert a_type = None

a_type = 'SecuritySystem'

elif state.domain == 'binary_sensor' or state.domain == 'device_tracker':
_LOGGER.debug('Add "%s" as "%s"', state.entity_id, 'BinarySensor')
return TYPES['BinarySensor'](hass, state.entity_id,
state.name, aid=aid)

elif state.domain == 'cover':
# Only add covers that support set_cover_position
features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0)
if 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)

elif state.domain == 'alarm_control_panel':
_LOGGER.debug('Add "%s" as "%s"', state.entity_id, 'SecuritySystem')
return TYPES['SecuritySystem'](hass, state.entity_id, state.name,
alarm_code=config.get(ATTR_CODE),
aid=aid)
a_type = 'BinarySensor'

elif state.domain == 'climate':
features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0)
support_temp_range = SUPPORT_TARGET_TEMPERATURE_LOW | \
SUPPORT_TARGET_TEMPERATURE_HIGH
# Check if climate device supports auto mode
support_auto = bool(features & support_temp_range)
a_type = 'Thermostat'
config['support_auto'] = bool(features & support_temp_range)

_LOGGER.debug('Add "%s" as "%s"', state.entity_id, 'Thermostat')
return TYPES['Thermostat'](hass, state.entity_id,
state.name, support_auto, aid=aid)
elif state.domain == 'cover':
# Only add covers that support set_cover_position
features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0)
if features & SUPPORT_SET_POSITION:
a_type = 'WindowCovering'

elif state.domain == 'light':
_LOGGER.debug('Add "%s" as "%s"', state.entity_id, 'Light')
return TYPES['Light'](hass, state.entity_id, state.name, aid=aid)
a_type = 'Light'

elif state.domain == 'lock':
return TYPES['Lock'](hass, state.entity_id, state.name, aid=aid)
a_type = 'Lock'

elif state.domain == 'sensor':
unit = state.attributes.get(ATTR_UNIT_OF_MEASUREMENT)
if unit == TEMP_CELSIUS or unit == TEMP_FAHRENHEIT:
a_type = 'TemperatureSensor'
elif unit == '%':
a_type = 'HumiditySensor'

elif state.domain == 'switch' or state.domain == 'remote' \
or state.domain == 'input_boolean' or state.domain == 'script':
_LOGGER.debug('Add "%s" as "%s"', state.entity_id, 'Switch')
return TYPES['Switch'](hass, state.entity_id, state.name, aid=aid)
a_type = 'Switch'

else:

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change will cause issues....
state.domain == 'sensor' and unit is not %, or temp and a_type is undefined

Please revert to the original if a_type is None:

@cdce8p cdce8p Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right. Didn't thought about that.

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if a_type is None:

return None

return None
_LOGGER.debug('Add "%s" as "%s"', state.entity_id, a_type)
return TYPES[a_type](hass, state.name, state.entity_id, config, aid=aid)


def generate_aid(entity_id):
Expand All @@ -151,7 +136,7 @@ class HomeKit():

def __init__(self, hass, port, entity_filter, entity_config):
"""Initialize a HomeKit object."""
self._hass = hass
self.hass = hass
self._port = port
self._filter = entity_filter
self._config = entity_config
Expand All @@ -164,11 +149,11 @@ def setup(self):
"""Setup bridge and accessory driver."""
from .accessories import HomeBridge, HomeDriver

self._hass.bus.async_listen_once(
self.hass.bus.async_listen_once(
EVENT_HOMEASSISTANT_STOP, self.stop)

path = self._hass.config.path(HOMEKIT_FILE)
self.bridge = HomeBridge(self._hass)
path = self.hass.config.path(HOMEKIT_FILE)
self.bridge = HomeBridge(self.hass)
self.driver = HomeDriver(self.bridge, self._port, get_local_ip(), path)

def add_bridge_accessory(self, state):
Expand All @@ -177,7 +162,7 @@ def add_bridge_accessory(self, state):
return
aid = generate_aid(state.entity_id)
conf = self._config.pop(state.entity_id, {})
acc = get_accessory(self._hass, state, aid, conf)
acc = get_accessory(self.hass, state, aid, conf)
if acc is not None:
self.bridge.add_accessory(acc)

Expand All @@ -192,12 +177,12 @@ def start(self, *args):
type_covers, type_lights, type_locks, type_security_systems,
type_sensors, type_switches, type_thermostats)

for state in self._hass.states.all():
for state in self.hass.states.all():
self.add_bridge_accessory(state)
self.bridge.set_broker(self.driver)

if not self.bridge.paired:
show_setup_message(self.bridge, self._hass)
show_setup_message(self.hass, self.bridge)

_LOGGER.debug('Driver start')
self.driver.start()
Expand Down
39 changes: 26 additions & 13 deletions homeassistant/components/homekit/accessories.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
from homeassistant.util import dt as dt_util

from .const import (
DEBOUNCE_TIMEOUT, ACCESSORY_MODEL, ACCESSORY_NAME, BRIDGE_MODEL,
BRIDGE_NAME, MANUFACTURER, SERV_ACCESSORY_INFO, CHAR_MANUFACTURER,
DEBOUNCE_TIMEOUT, BRIDGE_MODEL, BRIDGE_NAME, MANUFACTURER,
SERV_ACCESSORY_INFO, CHAR_MANUFACTURER,
CHAR_MODEL, CHAR_NAME, CHAR_SERIAL_NUMBER)
from .util import (
show_setup_message, dismiss_setup_message)
Expand Down Expand Up @@ -85,34 +85,47 @@ def set_accessory_info(acc, name, model, manufacturer=MANUFACTURER,
class HomeAccessory(Accessory):
"""Adapter class for Accessory."""

# pylint: disable=no-member

def __init__(self, name=ACCESSORY_NAME, model=ACCESSORY_MODEL,
category='OTHER', **kwargs):
def __init__(self, hass, name, entity_id, category='OTHER', **kwargs):
"""Initialize a Accessory object."""
super().__init__(name, **kwargs)

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this **kwargs or the combination of config and aid that should be passed here?

@cdce8p cdce8p Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will change **kwargs to aid.

set_accessory_info(self, name, model)
set_accessory_info(self, name, model=entity_id)
self.category = getattr(Category, category, Category.OTHER)
self.entity_id = entity_id
self.hass = hass

def _set_services(self):
add_preload_service(self, SERV_ACCESSORY_INFO)

def run(self):
"""Method called by accessory after driver is started."""
state = self.hass.states.get(self.entity_id)
self.update_state(new_state=state)
self.update_state_callback(new_state=state)
async_track_state_change(
self.hass, self.entity_id, self.update_state)
self.hass, self.entity_id, self.update_state_callback)

def update_state_callback(self, entity_id=None, old_state=None,
new_state=None):
"""Callback from state change listener."""
_LOGGER.debug('New_state: %s', new_state)
if new_state is None:
return
self.update_state(new_state)

def update_state(self, new_state):

@kellerza kellerza Apr 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 This is a good idea and seem to simplify things as well

"""Method called on state change to update HomeKit value.

Overridden by accessory types.
"""
pass


class HomeBridge(Bridge):
"""Adapter class for Bridge."""

def __init__(self, hass, name=BRIDGE_NAME,
model=BRIDGE_MODEL, **kwargs):
def __init__(self, hass, name=BRIDGE_NAME, **kwargs):
"""Initialize a Bridge object."""
super().__init__(name, **kwargs)
set_accessory_info(self, name, model)
set_accessory_info(self, name, model=BRIDGE_MODEL)
self.hass = hass

def _set_services(self):
Expand All @@ -130,7 +143,7 @@ def add_paired_client(self, client_uuid, client_public):
def remove_paired_client(self, client_uuid):
"""Override super function to show setup message if unpaired."""
super().remove_paired_client(client_uuid)
show_setup_message(self, self.hass)
show_setup_message(self.hass, self)


class HomeDriver(AccessoryDriver):
Expand Down
2 changes: 0 additions & 2 deletions homeassistant/components/homekit/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
SERVICE_HOMEKIT_START = 'start'

# #### STRING CONSTANTS ####
ACCESSORY_MODEL = 'homekit.accessory'
ACCESSORY_NAME = 'Home Accessory'
BRIDGE_MODEL = 'homekit.bridge'
BRIDGE_NAME = 'Home Assistant'
MANUFACTURER = 'HomeAssistant'
Expand Down
15 changes: 4 additions & 11 deletions homeassistant/components/homekit/type_covers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
CATEGORY_WINDOW_COVERING, SERV_WINDOW_COVERING,
CHAR_CURRENT_POSITION, CHAR_TARGET_POSITION, CHAR_POSITION_STATE)


_LOGGER = logging.getLogger(__name__)


Expand All @@ -20,13 +19,10 @@ class WindowCovering(HomeAccessory):
The cover entity must support: set_cover_position.
"""

def __init__(self, hass, entity_id, display_name, **kwargs):
def __init__(self, hass, name, entity_id, config, **kwargs):
"""Initialize a WindowCovering accessory object."""
super().__init__(display_name, entity_id,
CATEGORY_WINDOW_COVERING, **kwargs)

self.hass = hass
self.entity_id = entity_id
super().__init__(hass, name, entity_id,
category=CATEGORY_WINDOW_COVERING, **kwargs)

@cdce8p cdce8p Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Personally I would really like to move these calls to the HomeAccessory class somehow. Any ideas who to improve that?

@cdce8p cdce8p Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On solution could be to use a decorator function for for the init method which calls super().__init__(). That would look like this:

def super_init(category):
    def category_wrapper(func_init):
        def wrapper(self, hass, name, entity_id, **kwargs):
            super(self.__class__, self).__init__(
                hass, name, entity_id, category=CATEGORY_LIGHT, **kwargs)
            func_init(self)
        return wrapper
    return category_wrapper


@TYPES.register('Light')
class Light(HomeAccessory):

    @super_init(CATEGORY_LIGHT)
    def __init__(self):

Should we do that or stay at the current:

    def __init__(self, hass, name, entity_id, **kwargs):
        super().__init__(hass, name, entity_id, category=CATEGORY_LIGHT, **kwargs)

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't like the decorator and not sure how that will interfere with __init__

You could do this (note: category= at the end so they look the same at first glance)

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs, category=CATEGORY_LIGHT)

but being explicit might give you some code editor hints, so I like this version more:

    def __init__(self, hass, name, entity_id, **kwargs):
        super().__init__(hass, name, entity_id, **kwargs,
                         category=CATEGORY_LIGHT)

@cdce8p cdce8p Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The idea behind the decorator was to limit code redundancy between different types. You're right the it might be more difficult for editors to read, however I don't think that is that big of a deal since those attributes are already assigned in accessories.py/HomeAccessory and most inexperience developers will only copy existing types without fully understanding them.

Regarding the interference with __init__, it should change anything. The call order is as follows:

  1. HA start
  2. Module is loaded and __init__ call is replaced by decorater
  3. Call to light.__init__
  4. super().__init__()
  5. __init__()

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you dont like the super rather add a bootstrap/postinit method in the child classes that you call from the parent’s init. Type could be a class var on the child

@kellerza kellerza Apr 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In HomeAccessory you then have:

    def __init__(self, hass, name, entity_id, **kwargs):
        """Initialize a Accessory object."""
        super().__init__(name, **kwargs)
        set_accessory_info(self, name, model=entity_id)
        self.entity_id = entity_id
        self.hass = hass
        cat = self.init_category()
        self.category = getattr(Category, cat, Category.OTHER)

with only def init_category(self) and no more __init__ in the rest, init_category then returns the category.

@cdce8p cdce8p Apr 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Extending on your idea what do you think about the following:

  • Making the category a static var of the type
  • Changing the __init__() of the types to something like init_setup() which is called from HomeAccessory.__init__()

@cdce8p cdce8p Apr 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

init_setup() might not be the best name. If you have something else in mind, I'm happy to change it.
The only caveat with this solution is, that I had to add # pylint: disable=attribute-defined-outside-init to every type file.


self.current_position = None
self.homekit_target = None
Expand Down Expand Up @@ -56,11 +52,8 @@ def move_cover(self, value):
self.hass.components.cover.set_cover_position(
value, self.entity_id)

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

current_position = new_state.attributes.get(ATTR_CURRENT_POSITION)
if isinstance(current_position, int):
self.current_position = current_position
Expand Down
12 changes: 4 additions & 8 deletions homeassistant/components/homekit/type_lights.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,11 @@ class Light(HomeAccessory):
Currently supports: state, brightness, color temperature, rgb_color.
"""

def __init__(self, hass, entity_id, name, **kwargs):
def __init__(self, hass, name, entity_id, config, **kwargs):
"""Initialize a new Light accessory object."""
super().__init__(name, entity_id, CATEGORY_LIGHT, **kwargs)
super().__init__(hass, name, entity_id,
category=CATEGORY_LIGHT, **kwargs)

self.hass = hass
self.entity_id = entity_id
self._flag = {CHAR_ON: False, CHAR_BRIGHTNESS: False,
CHAR_HUE: False, CHAR_SATURATION: False,
CHAR_COLOR_TEMPERATURE: False, RGB_COLOR: False}
Expand Down Expand Up @@ -136,11 +135,8 @@ def set_color(self):
self.hass.components.light.turn_on(
self.entity_id, hs_color=color)

def update_state(self, entity_id=None, old_state=None, new_state=None):
def update_state(self, new_state):
"""Update light after state change."""
if not new_state:
return

# Handle State
state = new_state.state
if state in (STATE_ON, STATE_OFF):
Expand Down
13 changes: 4 additions & 9 deletions homeassistant/components/homekit/type_locks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,10 @@ class Lock(HomeAccessory):
The lock entity must support: unlock and lock.
"""

def __init__(self, hass, entity_id, name, **kwargs):
def __init__(self, hass, name, entity_id, config, **kwargs):
"""Initialize a Lock accessory object."""
super().__init__(name, entity_id, CATEGORY_LOCK, **kwargs)

self.hass = hass
self.entity_id = entity_id
super().__init__(hass, name, entity_id,
category=CATEGORY_LOCK, **kwargs)

self.flag_target_state = False

Expand All @@ -58,11 +56,8 @@ def set_state(self, value):
params = {ATTR_ENTITY_ID: self.entity_id}
self.hass.services.call('lock', service, params)

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

hass_state = new_state.state
if hass_state in HASS_TO_HOMEKIT:
current_lock_state = HASS_TO_HOMEKIT[hass_state]
Expand Down
16 changes: 5 additions & 11 deletions homeassistant/components/homekit/type_security_systems.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,12 @@
class SecuritySystem(HomeAccessory):
"""Generate an SecuritySystem accessory for an alarm control panel."""

def __init__(self, hass, entity_id, display_name, alarm_code, **kwargs):
def __init__(self, hass, name, entity_id, config, **kwargs):
"""Initialize a SecuritySystem accessory object."""
super().__init__(display_name, entity_id,
CATEGORY_ALARM_SYSTEM, **kwargs)

self.hass = hass
self.entity_id = entity_id
self._alarm_code = alarm_code
super().__init__(hass, name, entity_id,
category=CATEGORY_ALARM_SYSTEM, **kwargs)

self._alarm_code = config[ATTR_CODE]
self.flag_target_state = False

serv_alarm = add_preload_service(self, SERV_SECURITY_SYSTEM)
Expand All @@ -61,11 +58,8 @@ def set_security_state(self, value):
params[ATTR_CODE] = self._alarm_code
self.hass.services.call('alarm_control_panel', service, params)

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

hass_state = new_state.state
if hass_state in HASS_TO_HOMEKIT:
current_security_state = HASS_TO_HOMEKIT[hass_state]
Expand Down
Loading