openhab-google-assistant icon indicating copy to clipboard operation
openhab-google-assistant copied to clipboard

SpecialColorLight may not be used on group lights if groupType === Switch

Open depau opened this issue 3 years ago • 9 comments

The default matchesItemType() method used for simplelight accepts items with a matching group type, not just the type:

https://github.com/openhab/openhab-google-assistant/blob/16b6fc498a11903a60ca9cfc9d7e0a719329c8dc/functions/devices/default.js#L34-L39

This "randomly" (depending on the device types list order) prevents SpecialColorLight from being used on group lights when the group type is set to "Switch", since the simplelight sometimes is checked first.

In order for it to work I need to explicitly set the group type to none or something else.

depau avatar Feb 15 '22 14:02 depau

Thanks for raising that.

Just for my understanding: how would a group with type switch work as specialcolorlight, as the members would be of different types?

michikrug avatar Feb 16 '22 07:02 michikrug

I think now I got your issue. This is a bit tricky...

I could imagine solving this by introducing a separate metadata label for the SpecialColorLight to not only rely on Light and the group type.

michikrug avatar Mar 13 '22 21:03 michikrug

Sorry, I thought I had replied to this.

Just for my understanding: how would a group with type switch work as specialcolorlight, as the members would be of different types?

Well it turns out your questioning is correct since sending on/off to a group with multiple items doesn't result in always the correct behavior. For instance, the power is turned off but the brightness is also set to 0 which is annoying.

Still, I think that the GA integration should not behave as it does right now since it's confusing, the group type should be used as last resort in my opinion.

I could imagine solving this by introducing a separate metadata label for the SpecialColorLight to not only rely on Light and the group type.

That works and it's probably a lot less confusing for users.

Consider the fact that in order to understand what was going on for this issue I literally had to go through the code and fully understand it, with no access to the logs (since I'm using openhab cloud) and no way to inspect the execution since it's not running on my machine. So anything that removes this type of complexity is good imo.

depau avatar Mar 21 '22 13:03 depau

Thanks for the response. I will put this on my todo list.

michikrug avatar Mar 22 '22 16:03 michikrug

You now can use the SpecialColorLight metadata label

Changes were released in https://github.com/openhab/openhab-google-assistant/releases/tag/v3.5.0

michikrug avatar Oct 28 '22 21:10 michikrug

Sweet, thanks! How do I know when this is deployed to myOpenHAB? I updated a few lights to try it out but that caused them to disappear from Google Home.

depau avatar Oct 30 '22 15:10 depau

It is already. But I just recognized that your initial issue is not resolved by my changes as the specialcolorlight still needs the group type none.

michikrug avatar Oct 30 '22 18:10 michikrug

I see, and I do indeed confirm that after changing it to none it shows up in Google Home, but yeah that's an issue for me.

In my opinion, wherever possible, the group type should always be considered first when matching items, since that allows for "duck typing" with items.

For instance I have a light fixture with multiple lights, it would be useful to be able to create a specialcolorlight group containing groups of power switches, brightness dimmers and color selectors, each set with the proper type and tagged with the proper GA tag.

I'm probably missing something since I'm not very experienced with the Google Assistant/Home API so take this with a pinch of salt, but taking into consideration the way openHAB works and the way GA's device model is structured, my humble general design suggestion is to support group metadata tags that specify the device type (for instance gaDeviceTelevision, gaDeviceVacuum) and then allow implementing traits with special tags applied to items inside the group (such as OH Switch + gaTraitOnOff, OH Color/Dimmer + gaTraitColorSetting, OH Switch + gaTraitDock), giving priority to group types when matching item types. Then additionally some way to implement arbitrary traits with string items that have to provide valid JSON payloads, or something along these lines.

The reason why I use openHAB and not, i.e., Home Assistant, is because of openHAB's object-oriented approach to home automation as opposed to a prescriptive, "we only support these device types and nothing more" approach. The Google Assistant add-on design feels very HomeAssistant-y to me and I'm not a big fan of this design.

With this said I really do appreciate the work that has been put into this project, please take this as an attempt to provide constructive criticism based on my experience and not as a rant :)

depau avatar Oct 31 '22 13:10 depau

For instance I have a light fixture with multiple lights, it would be useful to be able to create a specialcolorlight group containing groups of power switches, brightness dimmers and color selectors, each set with the proper type and tagged with the proper GA tag.

Without spending too many thoughts into that, I would say this should already work. At least if the sub groups have the proper groupType and metadata property set. Happy to hear if this is not working with specifics on the item setup.

I'm probably missing something since I'm not very experienced with the Google Assistant/Home API so take this with a pinch of salt, but taking into consideration the way openHAB works and the way GA's device model is structured, my humble general design suggestion is to support group metadata tags that specify the device type (for instance gaDeviceTelevision, gaDeviceVacuum) and then allow implementing traits with special tags applied to items inside the group (such as OH Switch + gaTraitOnOff, OH Color/Dimmer + gaTraitColorSetting, OH Switch + gaTraitDock), giving priority to group types when matching item types. Then additionally some way to implement arbitrary traits with string items that have to provide valid JSON payloads, or something along these lines.

I do get the idea. And afaik this is very similar to what the Amazon Echo integration is doing. But I am also completely unaware of how the Echo API is working.

In the end, the current way it is working is not that much different, isn't it? You do not specify traits explicitly but with the metadata property on the group members you kinda assign the same functionality. With the trait approach, we may could get rid of some device specific properties with the same meaning, like fanPower, tvPower, lightPower ... if just called traitOnOff.

With this said I really do appreciate the work that has been put into this project, please take this as an attempt to provide constructive criticism based on my experience and not as a rant :)

All good 👍🏻

michikrug avatar Oct 31 '22 18:10 michikrug