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 light.group platform #12229

Merged
merged 19 commits into from
Mar 2, 2018
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
1 change: 1 addition & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ omit =
homeassistant/components/light/decora_wifi.py
homeassistant/components/light/flux_led.py
homeassistant/components/light/greenwave.py
homeassistant/components/light/group.py
homeassistant/components/light/hue.py
homeassistant/components/light/hyperion.py
homeassistant/components/light/iglo.py
Expand Down
12 changes: 10 additions & 2 deletions homeassistant/components/light/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,9 @@ def async_turn_off(hass, entity_id=None, transition=None):
DOMAIN, SERVICE_TURN_OFF, data))


@callback
@bind_hass
def toggle(hass, entity_id=None, transition=None):
def async_toggle(hass, entity_id=None, transition=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

I added async_toggle so that there was an appropriate method to be called from within the event loop (for the tests). It's almost identical to async_turn_on and async_turn_off.

"""Toggle all or specified light."""
data = {
key: value for key, value in [
Expand All @@ -216,7 +217,14 @@ def toggle(hass, entity_id=None, transition=None):
] if value is not None
}

hass.services.call(DOMAIN, SERVICE_TOGGLE, data)
hass.async_add_job(hass.services.async_call(
DOMAIN, SERVICE_TOGGLE, data))


@bind_hass
def toggle(hass, entity_id=None, transition=None):
"""Toggle all or specified light."""
hass.add_job(async_toggle, hass, entity_id, transition)


def preprocess_turn_on_alternatives(params):
Expand Down
280 changes: 280 additions & 0 deletions homeassistant/components/light/group.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,280 @@
"""
This component allows several lights to be grouped into one light.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/light.group/
"""
import logging
import itertools
from typing import List, Tuple, Optional, Iterator, Any, Callable
from collections import Counter
from copy import deepcopy

import voluptuous as vol

from homeassistant.core import State, callback
from homeassistant.components import light
from homeassistant.const import (STATE_OFF, STATE_ON, SERVICE_TURN_ON,
SERVICE_TURN_OFF, ATTR_ENTITY_ID, CONF_NAME,
CONF_ENTITIES, STATE_UNAVAILABLE,
STATE_UNKNOWN, ATTR_SUPPORTED_FEATURES)
from homeassistant.helpers.event import async_track_state_change
from homeassistant.helpers.typing import HomeAssistantType, ConfigType
from homeassistant.components.light import (
SUPPORT_BRIGHTNESS, SUPPORT_RGB_COLOR, SUPPORT_COLOR_TEMP,
SUPPORT_TRANSITION, SUPPORT_EFFECT, SUPPORT_FLASH, SUPPORT_XY_COLOR,
SUPPORT_WHITE_VALUE, PLATFORM_SCHEMA, ATTR_BRIGHTNESS, ATTR_XY_COLOR,
ATTR_RGB_COLOR, ATTR_WHITE_VALUE, ATTR_COLOR_TEMP, ATTR_MIN_MIREDS,
ATTR_MAX_MIREDS, ATTR_EFFECT_LIST, ATTR_EFFECT)
import homeassistant.helpers.config_validation as cv

_LOGGER = logging.getLogger(__name__)

DOMAIN = 'group'
Copy link
Member

Choose a reason for hiding this comment

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

Only components have a domain. This file is a platform the light component.

Copy link
Member

Choose a reason for hiding this comment

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

Also doesn't seem to be used?


DEFAULT_NAME = 'Group Light'

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Required(CONF_ENTITIES): cv.entities_domain('light')
})

SUPPORT_GROUP_LIGHT = (SUPPORT_BRIGHTNESS | SUPPORT_COLOR_TEMP | SUPPORT_EFFECT
| SUPPORT_FLASH | SUPPORT_RGB_COLOR | SUPPORT_TRANSITION
| SUPPORT_XY_COLOR | SUPPORT_WHITE_VALUE)


async def async_setup_platform(hass: HomeAssistantType, config: ConfigType,
async_add_devices, discovery_info=None) -> None:
"""Initialize light.group platform."""
async_add_devices([GroupLight(config.get(CONF_NAME),
config[CONF_ENTITIES])])


class GroupLight(light.Light):
"""Representation of a group light."""

def __init__(self, name: str, entity_ids: List[str]) -> None:
"""Initialize a group light."""
self._name = name # type: str
self._entity_ids = entity_ids # type: List[str]
self._state = STATE_UNAVAILABLE # type: str
self._brightness = None # type: Optional[int]
self._xy_color = None # type: Optional[Tuple[float, float]]
self._rgb_color = None # type: Optional[Tuple[int, int, int]]
self._color_temp = None # type: Optional[int]
self._min_mireds = 154 # type: Optional[int]
self._max_mireds = 500 # type: Optional[int]
self._white_value = None # type: Optional[int]
self._effect_list = None # type: Optional[List[str]]
self._effect = None # type: Optional[str]
self._supported_features = 0 # type: int
self._async_unsub_state_changed = None

async def async_added_to_hass(self) -> None:
"""Register callbacks."""
@callback
def async_state_changed_listener(entity_id: str, old_state: State,
new_state: State):
"""Handle child updates."""
self.async_schedule_update_ha_state(True)

self._async_unsub_state_changed = async_track_state_change(
self.hass, self._entity_ids, async_state_changed_listener)

async def async_will_remove_from_hass(self):
"""Callback when removed from HASS."""
if self._async_unsub_state_changed:
self._async_unsub_state_changed()
self._async_unsub_state_changed = None

@property
def name(self) -> str:
"""Return the name of the entity."""
return self._name

@property
def state(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Just embed it inside is_on. Function calls add overhead. Also, we can just store a boolean since we don't ever use the value of self.state

Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of because of my own way of using Home Assistant, but: Don't we want to also support the light reporting an STATE_UNAVAILABLE state when all child lights are unavailable?

Just embed it inside is_on. Function calls add overhead.

Do you mean writing this? (usage of _)

@property
def is_on(self) -> bool:
    """Return True if entity is on."""
    return self._state == STATE_ON

Copy link
Member

Choose a reason for hiding this comment

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

return self._is_on

Copy link
Member Author

Choose a reason for hiding this comment

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

(sorry if this is outdated: we'd still like to report unavailable/unknown state right? Then AFAIK we can't just implement the property is_on. We need Light#state for that.)

Copy link
Member

Choose a reason for hiding this comment

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

You are never allowed to override Light#state or any state property of any abstract base class for that matter. You should always implement properties and what gets exposed as state is up to the ABC to decide. You implement is_on and available, the ABC decides how that will be represented.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

"""Return the state."""
return self._state

@property
def is_on(self) -> bool:
"""Return True if entity is on."""
return self._state == STATE_ON

@property
def brightness(self) -> Optional[int]:
"""Return the brightness of this light between 0..255."""
return self._brightness

@property
def xy_color(self) -> Optional[Tuple[float, float]]:
"""Return the XY color value [float, float]."""
return self._xy_color

@property
def rgb_color(self) -> Optional[Tuple[int, int, int]]:
"""Return the RGB color value [int, int, int]."""
return self._rgb_color

@property
def color_temp(self) -> Optional[int]:
"""Return the CT color value in mireds."""
return self._color_temp

@property
def min_mireds(self) -> Optional[int]:
"""Return the coldest color_temp that this light supports."""
return self._min_mireds

@property
def max_mireds(self) -> Optional[int]:
"""Return the warmest color_temp that this light supports."""
return self._max_mireds

@property
def white_value(self) -> Optional[int]:
"""Return the white value of this light between 0..255."""
return self._white_value

@property
def effect_list(self) -> Optional[List[str]]:
"""Return the list of supported effects."""
return self._effect_list

@property
def effect(self) -> Optional[str]:
"""Return the current effect."""
return self._effect

@property
def supported_features(self) -> int:
"""Flag supported features."""
return self._supported_features

@property
def should_poll(self) -> bool:
"""No polling needed for a group light."""
return False

async def _async_send_message(self, service, **kwargs):
"""Send a message to all entities in the group."""
for entity_id in self._entity_ids:
payload = deepcopy(kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

No need to do a deepcopy if you only mutate the outer layer. Just dict(kwargs) is enough.

Also, are all kwargs for turn_on and turn_off the same as service attributes? This becomes an implicit rule with this method and I'm not sure if this is good.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean as far as I saw, they're the same, but you're right in that we shouldn't rely on that. So I now changed the code to use light.async_turn_on directly. I mean then we also need to rely on that method having the same kwargs, but that's much more maintainable.

We could also manually specify the arguments in our own self.async_turn_on, but that'd be a lot of arguments (and a bit ugly 😅).

Copy link
Member

Choose a reason for hiding this comment

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

Problem with light.async_turn_on is that it's non blocking (see other comment)

payload[ATTR_ENTITY_ID] = entity_id
await self.hass.services.async_call(light.DOMAIN, service, payload)
Copy link
Member

Choose a reason for hiding this comment

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

Should be blocking=True

Copy link
Member Author

Choose a reason for hiding this comment

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

Why should we be blocking? I mean one potential problem would be that maybe some component takes its time sending the turn_* command. Then we shouldn't need to wait for the it before sending the payload to the others, as that would create a weird staggered light on effect if I'm correct.

Copy link
Member

Choose a reason for hiding this comment

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

If I would interact with the normal device, it would take the time to finish the command. If I interact with it via proxy, it should take that time to finish the command.

Also, Home Assistant expects commands to be done when the function is done calling. That's why for polling devices it knows it can call update right away.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about storing every service call as a task (list) and the use asyncio.gather?

Copy link
Member

Choose a reason for hiding this comment

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

ehr, you can do it with 1 service call. Just pass the list of entities as ATTR_ENTITY_ID and the light component will figure it all out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pass the list of entities as ATTR_ENTITY_ID and the light component will figure it all out.

Oh, that's certainly good to know, will do.

Problem with light.async_turn_on is that it's non blocking (see other comment)
Also, are all kwargs for turn_on and turn_off the same as service attributes?

Ok, so 1. we need to be able to do the service call with blocking mode + 2. we shouldn't simply pass the kwargs to the service call, as that could break in the future (bc we want to maximize maintainability).

So ideally I think we would be using something like light.async_turn_on for the 2. problem, but that doesn't support blocking mode. In my view there are 2 possibilities now (of which I prefer the former):

  • Extend light.async_turn_on to accept blocking kwarg.
    • Upside: we have 1 place where the kwargs -> service call logic sits (DRY... 🤓)
    • Downside: we'd need to add the optional argument + probably also return the task object
    • Or we create a coroutine similar to async_turn_on that is called from the existing async_turn_on and that provides a blocking arg.
  • We "bodge" the kwarg -> service call logic into the group platform.
    • Downside: not nice 😥

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 it's nicer to just keep this logic inside the group platform. It's the platforms responsibility to keep working.

Otherwise we add the constraint that light.async_turn_on has the same args as Entity.turn_on.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍


async def async_turn_on(self, **kwargs):
"""Forward the turn_on command to all lights in the group."""
await self._async_send_message(SERVICE_TURN_ON, **kwargs)

async def async_turn_off(self, **kwargs):
"""Forward the turn_off command to all lights in the group."""
await self._async_send_message(SERVICE_TURN_OFF, **kwargs)

async def async_update(self):
"""Query all members and determine the group state."""
states = self._child_states()
on_states = [state for state in states if state.state == STATE_ON]

self._state = _determine_on_off_state(states)
Copy link
Member

Choose a reason for hiding this comment

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

All these functions will do their own iteration over all items in the list. That is very inefficient.

states = []
min_mireds = 154

for entity_id in self._entity_ids:
    state = self.hass.states.get(entity_id)
    if state is None:
        continue

    # In case you still need to map over all states for some checks
    states.append(state)

    # Etc…
    min_mireds = min(min_mireds, state.attributes[MIN_MIREDS])

Copy link
Member

Choose a reason for hiding this comment

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

Nice thing of doing it like this is that you can also not do a bunch of checks when the state is off.

Copy link
Member Author

Choose a reason for hiding this comment

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

All these functions will do their own iteration over all items in the list.

Technically they only go through the iteration until they find a state that matches - that's also why I'm using the generator syntax and next(..., default) in _reduce_attribute. Next, complexity-wise isn't having a global for-loop over all states just the same as having a local for-loop for all attributes? I'm not a python expert, but I mean maybe python has a bit of overhead with starting for-loops, but would that tiny speed benefit really be worth it? I mean this function will probably not be called too often.

min_mireds = min(min_mireds, state.attributes[MIN_MIREDS])

I mean if I understand correctly this kinda touches on home-assistant/architecture#13; do we want to report the first light state or the union of all supported features/states?

Copy link
Member Author

Choose a reason for hiding this comment

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

The example with min_mireds is quite, but with brightness for example where one has to handle None I've come up with these two solutions with using average:

brightnesses = []

for state in states:
    if state.state != STATE_ON:
        continue

    if state.attributes.get('brightness') is not None:
        brightnesses.append(state.attributes['brightness'])
        
if brightnesses:
    self._brightness = sum(brightnesses) / len(brightnesses)
else:
    self._brightness = None
self._brightness = None
num_brightness = 0

for state in states:
    if state.state != STATE_ON:
        continue

    if state.attributes.get('brightness') is not None:
        self._brightness = (self.brightness or 0) + state.attributes['brightness']
        num_brightness += 1

if num_brightness > 0:
    self._brightness = self._brightness / num_brightness

I mean one could probably also design helper methods around these statements, but is the speed benefit really worth the messier code? Also: at least some of the slowness of the previous code has now been fixed by pre-calculating a list of states that are on.

Copy link
Member Author

@OttoWinter OttoWinter Feb 21, 2018

Choose a reason for hiding this comment

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

Ok, I went ahead and created a small speed benchmark for both methods. And the current way with using a helper actually seems faster: (Scroll to the bottom; f is current method, g would be new method)

https://gist.github.com/OttoWinter/58ce0fd4c7743d233af8a10ed7ff940a

(I might be doing some things here a bit inefficiently, especially concerning object creation.)


self._brightness = _reduce_attribute(on_states, ATTR_BRIGHTNESS)

self._xy_color = _reduce_attribute(
on_states, ATTR_XY_COLOR, reduce=_mean_tuple)

self._rgb_color = _reduce_attribute(
on_states, ATTR_RGB_COLOR, reduce=_mean_tuple)
if self._rgb_color is not None:
self._rgb_color = tuple(map(int, self._rgb_color))

self._white_value = _reduce_attribute(on_states, ATTR_WHITE_VALUE)

self._color_temp = _reduce_attribute(on_states, ATTR_COLOR_TEMP)
self._min_mireds = _reduce_attribute(
states, ATTR_MIN_MIREDS, default=154, reduce=min)
self._max_mireds = _reduce_attribute(
states, ATTR_MAX_MIREDS, default=500, reduce=max)

self._effect_list = None
all_effect_lists = list(
_find_state_attributes(states, ATTR_EFFECT_LIST))
if all_effect_lists:
# Merge all effects from all effect_lists with a union merge.
self._effect_list = list(set().union(*all_effect_lists))

self._effect = None
all_effects = list(_find_state_attributes(on_states, ATTR_EFFECT))
if all_effects:
# Report the most common effect.
effects_count = Counter(itertools.chain(all_effects))
self._effect = effects_count.most_common(1)[0][0]

self._supported_features = 0
for support in _find_state_attributes(states, ATTR_SUPPORTED_FEATURES):
# Merge supported features by emulating support for every feature
# we find.
self._supported_features |= support
Copy link
Member

Choose a reason for hiding this comment

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

I would expect &= ?

Copy link
Contributor

@NovapaX NovapaX Feb 20, 2018

Choose a reason for hiding this comment

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

I would expect that too.
De default should be to only support features that all child entities support (bitwise &=)

But there may be instances where you have some lights that do color, and some do only color temperature. If you group those you want to at least be able to set the color temperature. (And let the platform of the colored light figure out how to handle that further.)
To accomplish that you need to be able to manually override supported_features. But that might be food for another PR.... let's not make this one too complicated maybe

Copy link
Member Author

@OttoWinter OttoWinter Feb 20, 2018

Choose a reason for hiding this comment

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

I'm creating a architecture post right now, as I think we should first define how users expect these platforms to behave.

Edit: home-assistant/architecture#13

# Bitwise-and the supported features with the GroupedLight's features
# so that we don't break in the future when a new feature is added.
self._supported_features &= SUPPORT_GROUP_LIGHT
Copy link
Member

Choose a reason for hiding this comment

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

SUPPORT_GROUP_LIGHT is a confusing name since SUPPORT_ constants are used to describe the features of an entity, while here it is describing what support this entity has, if one of its children has that support.

I think that there is no need for SUPPORT_GROUP_LIGHT and this line at all ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that there is no need for SUPPORT_GROUP_LIGHT and this line at all ?

I mean the reason was that if some entity decided to add support for some weird supported effect, that wouldn't be reported to the front-end through our reduce_attribute code, we shouldn't just fail silently. But I'm open to changing this back.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Let's be conservative and only support the features we know to support.


def _child_states(self) -> List[State]:
Copy link
Member

Choose a reason for hiding this comment

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

Why not inline this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure :)

"""The states that the group is tracking."""
states = [self.hass.states.get(x) for x in self._entity_ids]
return list(filter(None, states))


def _find_state_attributes(states: List[State],
key: str) -> Iterator[Any]:
"""Find attributes with matching key from states."""
for state in states:
value = state.attributes.get(key)
if value is not None:
Copy link
Member

Choose a reason for hiding this comment

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

If attributes is equal to { 'bla': None }, it would not yield the value. Is this on purpose and would it ever make sense to return None?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I once saw some platforms using state attributes as None to indicate that they didn't exist. I replaced the key in state.attributes check in the last commit so that we don't break the _reduce_attribute code if that happens.

Copy link
Member

Choose a reason for hiding this comment

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

👍

yield value


def _mean_int(*args):
"""Return the mean of the supplied values."""
return int(sum(args) / len(args))


def _mean_tuple(*args):
"""Return the mean values along the columns of the supplied values."""
return tuple(sum(l) / len(l) for l in zip(*args))


# https://github.com/PyCQA/pylint/issues/1831
# pylint: disable=bad-whitespace
def _reduce_attribute(states: List[State],
key: str,
default: Optional[Any] = None,
reduce: Callable[..., Any] = _mean_int) -> Any:
"""Find the first attribute matching key from states.

If none are found, return default.
"""
attrs = list(_find_state_attributes(states, key))

if not attrs:
return default

if len(attrs) == 1:
return attrs[0]

return reduce(*attrs)


def _determine_on_off_state(states: List[State]) -> str:
"""Helper method to determine the ON/OFF/... state of a light."""
s_states = [state.state for state in states]

if not s_states or all(state == STATE_UNAVAILABLE for state in s_states):
Copy link
Member

Choose a reason for hiding this comment

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

This function is iterating 3 times over all the states. Could be done in 1?

return STATE_UNAVAILABLE
elif any(state == STATE_ON for state in s_states):
return STATE_ON
elif all(state == STATE_UNKNOWN for state in s_states):
return STATE_UNKNOWN
return STATE_OFF
Loading