Add light.group platform #12229

OttoWinter · 2018-02-07T17:24:32Z

Description:

Adds supports for grouping several lights into one combined light.

For example, in my home, I have 4 RGBW light strips along the upper circumference of the living room and they each appear as their own component in Home Assistant. However, I would like them to appear as the same light in the front end so that I don't have to manually control each of them, or use scripts/template lights with light.turn_on and a group. Most platforms, like Philips Hue, can already do this on their own through their own Apps, but for other platforms such as MQTT or if you're mixing platforms this is currently not possible.

In a (pretty old) discussion in the community forums, @balloob pointed out that this could also be done in the front end. In a nutshell, if the front end encounters a group consisting only of lights, it would display the group as a light. But I would argue that this is not ideal, especially if you're using something like AppDaemon, which would then have to support that too. Additionally, this would also fix some of the issues #11548 tries to fix.

As for the GroupedLight component/platform itself - it only really tracks the child entities for state changes and (in most cases) reports the state of the first active light encountered. And when a turn_on/turn_off is received, the command is basically just forwarded to all children.

As said, there are several ways to approach solving this issue, and this is only one of the possible ones. This PR is far from complete (tests, documentation). Please feel free to comment on whether this is the right approach to fix this issue.

(Based off of https://gist.github.com/jjensn/83dd60d96330bbf66e58dfae336187bf)

Related issue/PR/discussion (if applicable): andrey-git/home-assistant-custom-ui#5 | https://community.home-assistant.io/t/grouped-light-control/1034/56 | #11548, fixes #12130

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

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

light:
  - platform: group
    name: Cool Light Group
    entities:
      - light.amazing_light
      - light.foobar
      - light.sun

Checklist:

The code change is tested and works locally.

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

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

lrnoble · 2018-02-07T17:33:59Z

Very interesting, as I have this issue when grouping Xiaomi and LimitlessLED(Mi-light)

#11913

lrnoble · 2018-02-07T18:03:00Z

What are lint issues?

arsaboo · 2018-02-07T19:03:27Z

@lrnoble Please limit the discussion to the PR. If you are not sure what is going on...wait until the PR gets merged and released.

lrnoble · 2018-02-07T20:04:36Z

Apologies, I snap looked at this and didn't see that the text was in relation to the commit. I can see the lint issues that have been fixed.

tboyce021 · 2018-02-08T19:45:04Z

Can this not already be done with a template light? It would need to be updated to add things like color control though. It's also a little more work to set up since you have to write the services.

tboyce021 · 2018-02-08T19:54:54Z

I'm not against adding this, I just want to make sure it's worth the effort. If the template light could work with a little improvement, then I vote for spending the time improving that component. If the template light is just too much work to configure for this use case and we want this as a shortcut, then that seems fine too. It's not really my decision to make though.

OttoWinter · 2018-02-08T19:55:09Z

@tboyce021 I mean theoretically yes. But that would 1) be terribly ugly and user unfriendly, especially since this is quite useful and 2) some of the logic in this PR like merging all effects would be quite difficult to do in a template light.

OttoWinter · 2018-02-10T10:24:12Z

Closing because this should probably rather be done in the frontend.

NovapaX · 2018-02-19T11:58:28Z

I disagree that this is a front-end issue as entities are not only exposed through the frond-end, but also through services like Homebridge.

Currently you need to group multiple bulbs that act as a single light (like spotlights in a single fixture).
The frontend indeed does some smart things with exposing controls if all members of the group support these.
But Homebridge for example exposes groups as switches. So I'm not able to set brightness or color-temperature on such a group.

And besides: this way that light (with multiple bulbs in a single fixture) would sit nicely in the light domain, where it belongs.

OttoWinter · 2018-02-19T12:57:30Z

@NovapaX In the end, I don't really care about how this is solved, all I would like is that this "feature" is implemented some time. I do get your point about this not being really that much of a front-end concern. The issue with the backend is that there, we already have group.light_*, which achieves similar things (just without the state reporting).

I myself am using this in my own setup, and I think it's really neat to have in general, as there are many use cases for this platform. If the core team (CC @balloob, @MartinHjelmare) want this feature as they stated in #11323, I would be happy to write documentation, tests, ... :)

I think the code in this PR is done in such a way that it shouldn't cause any issues down the line (" all of a sudden group becomes tightly coupled with other implementations which is something we should try to avoid.")

MartinHjelmare · 2018-02-19T18:54:48Z

I'll try to have a look at the code sometime soon, if nobody beats me to it. I think in general we have a go ahead. The platform should be named group.

OttoWinter · 2018-02-19T18:56:29Z

The platform should be named group.

Sure! Opening this PR again then, I will try to do the rest of the work tomorrow. I now noticed there are a few things I'd change to make the code a bit cleaner, so I think reviewing is not necessary at this point (dunno whether I should leave the PR closed in that case...)

bieniu · 2018-02-19T20:12:39Z

Guys if you will work on this component, can you consider adding the template turn_on and turn_off service like in template light? It would be an ideal solution for my problem - two smart Xiaomi bulbs that are powered only when the Z-Wave switch is on. So I turn on and off this bulbs through the switch, and change the brightness/colors/color temperature/effects through the group of bulbs. So far, I haven't found way to combine this switch and bulbs into one entity and to be able to control all the possibilities of bulbs.

MartinHjelmare · 2018-02-19T20:35:05Z

We should not add more complexity in this PR. Let's start with the basics on/off and attributes. It will be enough complexity just that.

bieniu · 2018-02-19T20:39:21Z

@MartinHjelmare Yes of course I understand. Maybe in the future...

OttoWinter · 2018-02-19T20:44:37Z

@bieniu I would say the same as MartinHjelmare; this is a grouped_light (or now light.group), not a template light. Besides, if I understand correctly you could 1) just listen for state updates from this platform and update your Z-Wave switch when the grouped light is turned on/off, 2) Have a template switch that turns on/off both the light and the switch or 3) list the entity_id of your switch in the config of your grouped light (we're not currently checking whether the entities are lights)

About that last part: @MartinHjelmare, should we be checking whether all child entities are actual lights? If so, would you know how (short of checking the entity id string I can't think of anything off the top of my head)?

OttoWinter · 2018-02-19T23:10:37Z

A few questions about how we would except this platform to behave:

If no child entity exists yet, I'm currently publishing a STATE_UNAVAILABLE state, since this also signals to the user that they can't control this light. This is also the default at startup.
Reporting of effects: I currently report all effects merged as a union in effect_list and the most common effect in effect; Is this the behavior users would expect?
What should we do when the user requests light.toggle? Do we use our reported is_on state to turn all lights on/off (current behavior), or do we toggle each individual child?
See "Alarm group" platform #11323 (comment), do we want this config schema? (with entities:)

balloob · 2018-02-20T07:38:07Z

I think that this platform is a great idea. We might even add frontend support for "group" platforms to show the child entities.

balloob · 2018-02-20T07:46:02Z

homeassistant/components/light/group.py

+        """Query all members and determine the group state."""
+        states = self._child_states()
+
+        self._state = _determine_on_off_state(states)


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])

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

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?

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.

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.)

balloob · 2018-02-28T02:24:43Z

homeassistant/components/light/group.py

+    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)


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.

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 😅).

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

balloob · 2018-02-28T02:25:26Z

homeassistant/components/light/group.py

+        for entity_id in self._entity_ids:
+            payload = deepcopy(kwargs)
+            payload[ATTR_ENTITY_ID] = entity_id
+            await self.hass.services.async_call(light.DOMAIN, service, payload)


Should be blocking=True

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.

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.

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

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.

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 😥

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.

balloob · 2018-02-28T02:27:31Z

homeassistant/components/light/group.py

+            self._supported_features |= support
+        # 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


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 ?

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.

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

balloob · 2018-02-28T02:28:18Z

homeassistant/components/light/group.py

+        # so that we don't break in the future when a new feature is added.
+        self._supported_features &= SUPPORT_GROUP_LIGHT
+
+    def _child_states(self) -> List[State]:


Why not inline this method?

balloob · 2018-02-28T02:29:06Z

homeassistant/components/light/group.py

+    """Find attributes with matching key from states."""
+    for state in states:
+        value = state.attributes.get(key)
+        if value is not None:


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?

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.

balloob · 2018-02-28T02:32:10Z

homeassistant/components/light/group.py

+    """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):


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

OttoWinter · 2018-02-28T21:29:37Z

homeassistant/components/light/group.py

+    async def async_turn_on(self, **kwargs):
+        """Forward the turn_on command to all lights in the group."""
+        kwargs[ATTR_ENTITY_ID] = self._entity_ids
+        await light.async_turn_on(self.hass, blocking=True, **kwargs)


Theoretically (and this is a big hypothetical) we could receive a blocking inside our own kwargs, then this would cause an exception TypeError: function got multiple values for keyword argument blocking, but I suppose it's better to just break if that happens.

Like I stated above, call the service directly and make sure you know how the turn_on kwargs map to the service attributes.

balloob · 2018-03-01T06:28:37Z

homeassistant/components/light/__init__.py

@@ -182,7 +186,8 @@ def async_turn_on(hass, entity_id=None, transition=None, brightness=None,
        ] if value is not None
    }

-    hass.async_add_job(hass.services.async_call(DOMAIN, SERVICE_TURN_ON, data))
+    return hass.async_add_job(hass.services.async_call(


This is not good. A callback method should not return coroutine objects to await. We used to do it and it is a) confusing and b) leads to bugs. It also breaks with every other such service helper methods all over the code base. Please revert this change.

It looks like light.group will have to keep track of which turn_on keywords map to turn_on attributes and have tests in place to make sure that they are not broken.

The fact that a platform would cause such a major change to a component is a good red flag that it's not a good idea.

Please revert all changes to light/__init__.py

Oh Sorry, I misread that previous comment.

(Should I also revert the addition of async_toggle? I mean it's just making toggle more easily available from an async context)

The fact that a platform would cause such a major change to a component is a good red flag that it's not a good idea.

That's also why I asked the question. But I misunderstood your answer 😅

Done, except for the async_togglè and the group import that I found no way around.

Let's drop async_toggle. I don't even know if we should keep the top level functions to call services. Like, who uses them?

Also, since there hasn't been a need for it, let's remove it. We should only add code that is going to be used.

balloob · 2018-03-01T06:32:10Z

homeassistant/components/light/group.py

+    async def async_turn_off(self, **kwargs):
+        """Forward the turn_off command to all lights in the group."""
+        kwargs[ATTR_ENTITY_ID] = self._entity_ids
+        await light.async_turn_off(self.hass, blocking=True, **kwargs)


This reverts commit 9e83198.

OttoWinter · 2018-03-01T13:15:01Z

homeassistant/components/light/__init__.py

@@ -12,7 +12,8 @@

 import voluptuous as vol

-from homeassistant.components import group
+from homeassistant.components.group import \
+    ENTITY_ID_FORMAT as GROUP_ENTITY_ID_FORMAT


By importing components.group into the light namespace I was unable to import light.group (the Group Light platform) in the tests, so I now import the "ENTITY_ID_FORMAT" directly with a GROUP_* prefix. Let me know if there's a better way.

that's fine.

OttoWinter · 2018-03-01T13:17:59Z

homeassistant/components/light/__init__.py

@@ -182,7 +186,8 @@ def async_turn_on(hass, entity_id=None, transition=None, brightness=None,
        ] if value is not None
    }

-    hass.async_add_job(hass.services.async_call(DOMAIN, SERVICE_TURN_ON, data))
+    return hass.async_add_job(hass.services.async_call(


The fact that a platform would cause such a major change to a component is a good red flag that it's not a good idea.

That's also why I asked the question. But I misunderstood your answer 😅

Done, except for the async_togglè and the group import that I found no way around.

balloob · 2018-03-01T23:03:34Z

homeassistant/components/light/group.py

+
+_LOGGER = logging.getLogger(__name__)
+
+DOMAIN = 'group'


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

Also doesn't seem to be used?

balloob · 2018-03-02T18:07:35Z

This has introduced a flaky test.

https://travis-ci.org/home-assistant/home-assistant/jobs/348247842#L996

Since your callback is a callback, you will need to block till done twice.

homeassistant added cla-signed new-platform platform: light.grouped_light labels Feb 7, 2018

homeassistant added the cla-signed label Feb 7, 2018

homeassistant added the cla-signed label Feb 9, 2018

OttoWinter closed this Feb 10, 2018

OttoWinter mentioned this pull request Feb 11, 2018

Added cover group platform #12303

Closed

3 tasks

MartinHjelmare mentioned this pull request Feb 12, 2018

"Alarm group" platform #11323

Closed

4 tasks

OttoWinter reopened this Feb 19, 2018

OttoWinter force-pushed the grouped-light branch from 4f631a1 to 5f68f60 Compare February 19, 2018 23:04

homeassistant added the cla-signed label Feb 19, 2018

balloob reviewed Feb 20, 2018

View reviewed changes

homeassistant added the cla-signed label Feb 27, 2018

balloob reviewed Feb 28, 2018

View reviewed changes

OttoWinter added 2 commits February 28, 2018 15:44

Changes according to Review Comments

ff0386d

Use blocking light.async_turn_*

9e83198

OttoWinter commented Feb 28, 2018

View reviewed changes

balloob reviewed Mar 1, 2018

View reviewed changes

OttoWinter added 3 commits March 1, 2018 12:48

Revert "Use blocking light.async_turn_*"

2d37f05

This reverts commit 9e83198.

Update service calls and state reporting

c1f71e1

Add group service call tests

6d4f4f8

OttoWinter commented Mar 1, 2018

View reviewed changes

balloob reviewed Mar 1, 2018

View reviewed changes

Remove unused constant.

9c1553f

balloob approved these changes Mar 1, 2018

View reviewed changes

balloob merged commit 0397076 into home-assistant:dev Mar 2, 2018

balloob mentioned this pull request Mar 2, 2018

Fix flaky light group test #12843

Closed

2 tasks

Martinvdm mentioned this pull request Mar 2, 2018

Limitlessled not working since 0.64.1 #12842

Closed

OttoWinter mentioned this pull request Mar 7, 2018

Clean up Light Groups #12962

Merged

6 tasks

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

NovapaX mentioned this pull request Mar 9, 2018

Nested group icon does not change colors home-assistant/frontend#186

Closed

OttoWinter deleted the grouped-light branch March 13, 2018 20:19

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: grouped_light and removed platform: light.grouped_light labels Mar 21, 2019


		_LOGGER = logging.getLogger(__name__)

		DOMAIN = 'group'

Add light.group platform #12229

Add light.group platform #12229

Conversation

OttoWinter commented Feb 7, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

lrnoble commented Feb 7, 2018

lrnoble commented Feb 7, 2018

arsaboo commented Feb 7, 2018

lrnoble commented Feb 7, 2018

tboyce021 commented Feb 8, 2018

tboyce021 commented Feb 8, 2018

OttoWinter commented Feb 8, 2018

OttoWinter commented Feb 10, 2018

NovapaX commented Feb 19, 2018

OttoWinter commented Feb 19, 2018 • edited Loading

MartinHjelmare commented Feb 19, 2018

OttoWinter commented Feb 19, 2018 • edited Loading

bieniu commented Feb 19, 2018

MartinHjelmare commented Feb 19, 2018

bieniu commented Feb 19, 2018 • edited Loading

OttoWinter commented Feb 19, 2018

OttoWinter commented Feb 19, 2018 • edited Loading

balloob commented Feb 20, 2018

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

OttoWinter Feb 21, 2018 • edited Loading

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

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

balloob commented Mar 2, 2018

OttoWinter commented Feb 7, 2018 •

edited

Loading

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

OttoWinter commented Feb 19, 2018 •

edited

Loading

OttoWinter commented Feb 19, 2018 •

edited

Loading

bieniu commented Feb 19, 2018 •

edited

Loading

OttoWinter commented Feb 19, 2018 •

edited

Loading

OttoWinter Feb 21, 2018 •

edited

Loading