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

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

Open
depau opened this issue Feb 15, 2022 · 9 comments
Open

Comments

@depau
Copy link

depau commented Feb 15, 2022

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

static matchesItemType(item) {
return (
!this.requiredItemTypes.length ||
this.requiredItemTypes.includes((item.groupType || item.type || '').split(':')[0])
);
}

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

depau commented Mar 21, 2022

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
Copy link
Collaborator

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

@michikrug
Copy link
Collaborator

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
Copy link
Author

depau commented Oct 30, 2022

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
Copy link
Collaborator

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
Copy link
Author

depau commented Oct 31, 2022

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
Copy link
Collaborator

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 michikrug reopened this Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants