-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[hue] Add support for groups/rooms #7476
Conversation
|
||
private @NonNullByDefault({}) String groupId; | ||
|
||
private final Logger logger = LoggerFactory.getLogger(HueGroupHandler.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move final fields above non-final fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lolodomo,
thanks for introducing Groups to hue binding. Can you please my question, whether hue groups support only channel on off switch, or although for example brightness or hsb color
<label>Hue Group</label> | ||
<description>A group of lights or a room that could be switched on and off.</description> | ||
|
||
<channels> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but is on/off switch really the only channel supported by groups. I think they support brightness and HSB as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see that until today but yes the hue app allows setting the brightness of a group. And the brightness of a group seems to be the average brightness of its on lights . I will add the brightness control.
For the color control, I don't know yet if this is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment on the issue: #7476
Based on the unofficial documentation you can send any parameter to a group as you would do with a single light:
http://www.burgestrand.se/hue-api/api/groups/
http://www.burgestrand.se/hue-api/api/lights/#changing-light-color-and-turning-them-onoff
Maybe give it a try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now added brightness control in addition to switch control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think the group can really handle all of the light commands as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now added control control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now added color temperature control.
Travis tests have failedHey @lolodomo, |
1 similar comment
Travis tests have failedHey @lolodomo, |
Ok just tested the binding. After some fiddling arround, this works great! Its so much cleaner to just turn on/turn off a room instead of turning 13 lights in serial on/off, this is so much more responsible! I will attach my configuration if someone is also curios about that binding:
Items:
Here are my findings:
|
I just had a look at the zigbee light link user guide. It seems like groups are clusters of zigbee lights. So based on the documentation i would propose to use 0004 as group thing type. Have a look at page 23 of that pdf. What do you think about that? |
What is a Hue Entertainment Area ? How can I identify it ? Does it contain lights ? How do you create such an area with the hue app ? Regarding the group 0, I agree it has to be renamed, I propose "All lights". No problem to use 0004 as thing type. Color temperature control will be added. |
After a Google search, I think you are talking about the usage with a Hue Play HDMI Sync Box. |
A Hue Entertainment Area is used for the Hue Entertainment Sync. You can create an Entertainment Area in the Hue App in Settings "Entertainment Zones". It is used for light synchronization with the TV through the HDMI Sync Box or a Windows App. Have a look at it: There are also a lot of youtube videos showing this functionality. On how to identify theses groups - i don't know. |
Yes! |
Looking at the result of the groups requests, it looks like it exists a "type" field. It can take the value ""LightGroup" or "Room". Maybe this type is different for a Hue Entertainment Areas. I have a bridge v1 and the app does not to create such area. I will log the group type so that you can tell me what value you get for Hue Entertainment Areas. |
Here is my output from api/.../groups:
So i can see basically three different group types: Entertainment, Zone and Room. I am not sure what my group 8 is :D. |
Thinking a bit longer about Group 8:
I think this is a hidden Group created by the hue bridge to control a single lamp with the Hue Dimmer Switch, as Light Number 25 is the Heater in my Bathroom, so we should also need to hide the type Lightgroup. |
Can anyone explain me why we are using always the zigbee id for thing-types. I think this is really a pain to use not speaking names for things but cryptic numbers for both developers of binding but above all for users of the binding. I would prefer it to have speaking things name like: dimmable, colorlight, presencesensor, group etc. I know this would be a breaking change for the binding, but in my opinion, would make life for all of us easier in the long run. So maybe we can leave group instead of 0004 and make a new pull request for changing other thing names? |
I totally agree with you. I just made the proposal to use 0004 in order to use the same standard. Maybe this is a thing which can be discussed with @kaikreuzer ? I would personally also prefer more memorable names. |
Here is a new version with the following changes:
|
I don't think it's a good thing to hide Lightgroup, this is the basic group type for lights or plugs. Many diy hue bridges for example deconz use only groups of this type |
I don"r hide the groups of type "LightGroup"; I only hide the ones of type "Entertainment". |
Ok, here is how my inbox now looks like: I would prefer the group type in the ?detailed description?, this would be also suitable for the release version. I made a mock up for this: As you can see Group 0 is not renamed to "All lights", maybe this is an issue with my cache or temporary folders? I just removed the old jar waited a few minutes and copied the new jar in the addon folder. I like @DerOetzi idea about renaming the thing types for that binding. I am curios if we could do something like a downwards compatible version. To accept both the Zigbee Id and from now on the new thing types. |
There was PR 6044 for that, no ? |
Yes, thats why I asked you :). Maybe this could also be added to rooms as scenes are always related to groups (rooms/zones). |
I will first wait for the merge of this PR. |
Group 0 is added manually because with old bridges, it was not returned by the "groups" request. Probably with your more recent bridge or firmware, this "bug" was fixed by Philips. So I have a little change to do to not request this group if already provided by the "groups" request and for the renaming in the discovery, I have to consider its id and not its name. |
Not only about the speaking name argument above, but I think with using 0004 ClusterID for groups, we start to mix up two different layers of ZigBee device layer and cluster layer. |
New version for renaming group 0.
This was your idea ! I can revert. |
This is not possible. Paper UI displays the thing type label at this place. |
You are absolutely right, that is my idea. I just wanted to stay to the current notation. However, I don't like the notation myself. If we can decide that i would prefer for all thing more memorable, self descriptive names. |
So let me know if the last version is OK. |
Totally agree: That shouldn't be part of this PR because it should be discussed thoroughly for being a breaking change. Although I basically support the idea of descriptive thing names |
I had to clean my cache and my tmp directory, after that everything looks fine and works as expected. Also the Group 0 is now named All Lights. |
Travis tests have failedHey @lolodomo, TravisBuddy Request Identifier: 512bf4f0-8883-11ea-88d0-3540a036c4cf |
Color temperature control is now implemented. |
Travis tests have failedHey @lolodomo, |
1 similar comment
Travis tests have failedHey @lolodomo, |
I have a doubt if this is necessary to have 4 channels on a group thing: switch, brigthness, color and color_temperature. |
@cpmeister : could you please have a final look ? |
"Could not notify groupStatusListeners for unknown event type " + type); | ||
} | ||
} catch (Exception e) { | ||
logger.error("An exception occurred while calling the Group Listeners", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error logging level should be reserved for issues catastrophic enough to threaten the operation of openHab itself. An error in a binding shouldn't qualify for this. Please read the openHab coding guidelines for expected use of logging in bindings.
My general rule for logging levels is:
- trace - used for normal expected execution paths
- debug - used for unexepected execution paths, but well within normal operation
- info - used for notable points in an execution path, like a milestone. (in openhab we try reserve this logging level for the core, so bindings should rarely ever call this.)
- warn - used for notable unexpected execution paths that a regular user (not just a developer) should be notified of. Warnings should be used to indicate that something not-normal occurred and user intervention is required to resolve. Warnings do not indicate a failure to operate merely an abnormal condition of operation that can still be handled by the binding. Failures in binding operation should be indicated by changing the thing status to offline.
- error - used to indicate catastrophic program failure. This should be used to indicate a catastrophic failure in openhab's ability to operate. A failure in a binding would never cause openhab as a whole to fail so a failure in a bindings should never log an error. Instead that failure should be indicated by changing the thing status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, updated.
...nhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/HueGroupHandler.java
Show resolved
Hide resolved
<description>The group identifier identifies one certain hue group or room.</description> | ||
<required>true</required> | ||
</parameter> | ||
<parameter name="fadetime" type="integer" min="0" step="100"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<parameter name="fadetime" type="integer" min="0" step="100"> | |
<parameter name="fadetime" type="integer" min="0" step="100" unit="ms"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
Another PR would be welcome to fix all other thing types.
</parameter> | ||
<parameter name="fadetime" type="integer" min="0" step="100"> | ||
<label>Fade Time</label> | ||
<description>Fade time in ms for changing values</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<description>Fade time in ms for changing values</description> | |
<description>Fade time for changing values</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I revert this one becauuse Paper UI is not displaying th eunit, so without this information in the description, the user will not know what is the unit of this configuration setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your information: eclipse-archived/smarthome#3974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that some UIs (e.g. Paper UI) - which will be removed in OH3 - do not show this "unit" but we prefer to not have it inside the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the unit in the description, the user will not know how to adjust the setting.
Hopefully, the unit is inside the description in every existing bindings.
I would suggest to change that when OH3 is released, not before. And this will require to change more or less every bindings.
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Ok, I have now rebased the PR to solve the conflict. |
Travis tests have failedHey @lolodomo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Manually checked sign-off |
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: CSchlipp <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
Fix openhab#7419 Signed-off-by: Laurent Garnier <[email protected]>
Fix #7419
Signed-off-by: Laurent Garnier [email protected]