openhab / openhab-google-assistant

openHAB Google Assistant: Actions on Google for openHAB
Eclipse Public License 2.0
173 stars 85 forks source link

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

Open depau opened 2 years ago

depau commented 2 years ago

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.

michikrug commented 2 years ago

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 commented 2 years ago

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.

depau commented 2 years ago

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.

michikrug commented 2 years ago

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

michikrug commented 1 year ago

You now can use the SpecialColorLight metadata label

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

depau commented 1 year ago

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.

michikrug commented 1 year ago

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.

depau commented 1 year ago

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

michikrug commented 1 year ago

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 👍🏻