Skip to content

Commit

Permalink
Discover color temperature range for Home Assistant groups. #8032
Browse files Browse the repository at this point in the history
  • Loading branch information
@Koenkk
Koenkk committed Jul 14, 2021
1 parent a2bf9c5 commit 515599a
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 4 deletions.
11 changes: 7 additions & 4 deletions lib/extension/homeassistant.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,13 @@ class HomeAssistant extends Extension {
discoveryEntry.discovery_payload.supported_color_modes = colorModes;
}

if (entityType === 'device' && hasColorTemp) {
const colorTemp = firstExpose.features.find((e) => e.name === 'color_temp');
discoveryEntry.discovery_payload.max_mireds = colorTemp.value_max;
discoveryEntry.discovery_payload.min_mireds = colorTemp.value_min;
if (hasColorTemp) {
const colorTemps = exposes.map((expose) => expose.features.find((e) => e.name === 'color_temp'))
.filter((e) => e);
const max = Math.max(...colorTemps.map((e) => e.value_max));
const min = Math.min(...colorTemps.map((e) => e.value_min));

This comment has been minimized.

Sign in to view

Copy link
@sjorge

sjorge Jul 15, 2021

Contributor

@Koenkk should this not be the max for min and the min for max? ?

If you have a group with 3 bulbs ranging
153-500
250-450
153-370

The only range supported by all of them would be: 250-370, unless I understand this code the range would be 153-500 which has values out of range for 2 of the 3 bulbs

This comment has been minimized.

Sign in to view

Copy link
@Koenkk

Koenkk Jul 15, 2021

Author Owner

That is somewhat a grey area:

  1. Either you will be unable to set some bulbs to its max/min color temp
  2. The color temp can be set higher then the range of the bulb, so the used color temp (if out of range) will be less/more than the requested one, however in this case the bulb will automatically fallback to it's max/min color temp.

Both aren't perfect but I think 2 is better.

This comment has been minimized.

Sign in to view

Copy link
@sjorge

sjorge Jul 15, 2021

Contributor

I'm no HA user so I'm not effect by this, but I'd lean towards 1 because if you grouped them, one would assume you want the bulbs to be in the same state. Like turn on all bulbs above the kitchen table and set them to color_temp.

In my example above, they might end up different color_temps when e.g. sliding to 500.

This comment has been minimized.

Sign in to view

Copy link
@Koenkk

Koenkk Jul 15, 2021

Author Owner

Asked for some input here: #8032 (comment)

This comment has been minimized.

Sign in to view

Copy link
@jwoodard80

jwoodard80 Jul 15, 2021 •
edited
Loading

To me, the entire purpose behind a "group" is to change multiple items in a like fashion. (IE: 4 lights in a ceiling fan) Therefore, in my opinion, the "group" options need to take on ONLY the overlapping abilities of the lights.

I open my app and move the slider all the way over..... with your first example I am left with X number in one state and Y number in another. I just don't see this being a desired outcome.

Given the random example of:
1 RGB and Dimmable Bulb
1 Dimmable Fixed Temp Bulb
1 Dimmable / Color Temp Bulb

The shared feature set is dimmable so that should be the only tunable attribute for the group. One would not assume that they would be able to turn all the bulbs red so the option shouldn't be there.

This would extend to Mireds as well. Given 3 bulbs as mentioned earlier in the thread.

Bulb A 153-500
Bulb B 250-450
Bulb C 153-370

The "group" range should be 250-370. This would mean that whatever was asked of the group could be accomplished with a uniform result. If someone wanted something of A and C as they are lower values, they could either create another group or adjust them individually.

I hope this makes sense.

This comment has been minimized.

Sign in to view

Copy link
@Koenkk

Koenkk Jul 16, 2021

Author Owner

Thanks, implemented solution B!

This comment has been minimized.

Sign in to view

Copy link
@SHxKM

SHxKM Dec 9, 2021

@Koenkk @jwoodard80 - is this implemented on colors vs. hue-saturation? I can easily set an RGB color on a Zigbee group composed of 4 RGB lights + 3 Color-Temp lights.

This is particularly problematic when exposing the group as an entity to HA, and then from HA to HomeKit, as HomeKit will display an RGB picker by default, and ANY choice made there (even if it looks like it's only changing the temprature) will only apply to the RGB light (probably because Homekit is sending an xy style state update).
discoveryEntry.discovery_payload.max_mireds = max;
discoveryEntry.discovery_payload.min_mireds = min;
}

const effect = definition && definition.exposes.find((e) => e.type === 'enum' && e.name === 'effect');
Expand Down
4 changes: 4 additions & 0 deletions test/homeassistant.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ describe('HomeAssistant extension', () => {
"name":"ha_discovery_group",
"sw_version":this.version,
},
"max_mireds": 500,
"min_mireds": 150,
"json_attributes_topic":"zigbee2mqtt/ha_discovery_group",
"name":"ha_discovery_group",
"schema":"json",
Expand Down Expand Up @@ -1713,6 +1715,8 @@ describe('HomeAssistant extension', () => {
"sw_version":this.version,
},
"json_attributes_topic":"zigbee2mqtt/ha_discovery_group",
"max_mireds": 500,
"min_mireds": 150,
"name":"ha_discovery_group",
"schema":"json",
"state_topic":"zigbee2mqtt/ha_discovery_group",
Expand Down

0 comments on commit 515599a

Please sign in to comment.