-
-
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 hue gateway as thing to activate predefined scenes #6043 #7540
Conversation
Travis tests have failedHey @leluna, |
Please add WIP, we need to add the dynamic option provider for scene ids before this feature is merged. Note that https://developers.meethue.com/develop/hue-api/ needs a developer account. This is not a solution for a final user. The final user should not have to determine the scene id. |
Replaces #6044 |
Travis tests have failedHey @leluna, |
Take care, it does not compile because you forgot @author in your 2 new classes and the header on one of the two new classes.
and
|
Quick test after updating few files to build a jar: The dynamic options are built on the bridge channel. The problem I see is that I have no group/room indication. So I have several scenes with the same name (Veilleuse for example) and I don't know for which room the scene applies. I tested one scene from the UI and it worked as expected. Good. Remains to implement the additional channel on a group thing type. |
Here are the proposed changes to get the room included in the option label:
Note that we have to delay a little the start of the scene job to give a chance to get the groups loaded first by the light job. |
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.
Few changes required.
....openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/HueBindingConstants.java
Outdated
Show resolved
Hide resolved
* @param id the scene to be activated | ||
* @throws IOException if the bridge cannot be reached | ||
*/ | ||
public CompletableFuture<Result> activateScene(String id) { |
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.
Suggestion: runScene
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.
How about recallScene? That would be consistent to the hue API
...enhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/HueThingHandlerFactory.java
Outdated
Show resolved
Hide resolved
...enhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/HueThingHandlerFactory.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/Scene.java
Show resolved
Hide resolved
* | ||
* @param id the ID of the scene to activate | ||
*/ | ||
void activateScene(String id); |
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.
Suggestion: runScene
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.
-> recallScene
@@ -24,6 +24,7 @@ | |||
import org.eclipse.smarthome.core.library.types.IncreaseDecreaseType; |
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 all the changes on this class in a different PR because they have no link with the current feature added.
If you want to push these changes, I would suggest that you consider the hue light handler too because the code is very similar.
It makes no real sense to make it different in one place only.
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 scene channel is added to groups here. When navigting through the class, I think these are quite minor changes that improved the readability of the code greatly for me. Is it of importance that the two classes are kept similar (except for the case that someone want to refactor both 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.
This will not help the maintenance if this is coded differently in the different classes.
...rc/main/java/org/openhab/binding/hue/internal/handler/HueStateDescriptionOptionProvider.java
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/hue/internal/handler/HueStateDescriptionOptionProvider.java
Show resolved
Hide resolved
<channel-type id="scene"> | ||
<item-type>String</item-type> | ||
<label>Scene</label> | ||
<description>The scene channel allows activating a scene to all lights that belong to the scene.</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.
Suggestion: running
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.
-> recalling
And for implementing the new channel on the group thing type, you will need a new interface I am just asking myself if we should avoid rebuilding the options when there is no change in scenes. |
As I can see in the code, there is still something missing to build the options for the channel on each group thing. And please consider setting the group id for scenes when it is not returned by the API. My proposal only sets it when all lights are in the same room. Without that included, the scenes are not usable with the bridge v1 because several scenes have the same name. |
@leluna : on the group thing, of course you can define a pattern for the option label without the group name. |
Regarding the scene ids, one alternative could be to not display it in the option label but give a way to the user to retrieve it in another way. It could be done through a console command like for example: |
@leluna : do you have some time to finish this new feature ? |
@lolodomo Sorry for the long pause, finding time is more difficult than I thought... Also, sthe group API is more complex than I thought... I will try to finish the group state option as soon as possible. I would like to avoid computing the group ad hoc. Computing the group ad hoc seems like added complexity for only legacy versions to me. As the backend API is more stable than the frontend app in general, I think we should stick to the API whenever possible for future maintenance. Concerning the global scene label: I would propose to only display the group name if it is defined for a group. The reason is that the group that it belongs to is not well defined in general. There are three types of group: light group, room and zone. I fail to see why we should display only the room that it belongs to, but not zones and light groups.. A zone be defined by the lights of exactly one room. Also, it is possible to define zones with exactly the same lights.... Sure we could imply that undefined group ID only exists for legacy versions, but that is not true. Anyone or any product that uses the API directly can create scenes using the API without group ID, with LightScene being even the default. Concerning the group scene label: I would propose to include a scene if the group includes AT LEAST ALL lights of the scene. If we want to be consistent to the hue API, the scene would be applicable to the group already if the group includes AT LEAST ONE light of the scene. However, that would mean that a room in a zone would also display all scenes of the zone, which is strange to me.. Only including the scene if the lights are an EXACT match to the group seem rather far from the API tho me. Thus, this solution would offer only a subset of the hue API functionalities, but a superset of the current hue app functionalities (I think?!). Concerning the console command: I think that would be great! That would make the label a lot shorter. Also, I can only copy paste the ID by using developer tools in the browser right now =.= I think it is much quicker if you implement the console command. The most agile solution would be probably delivering the scene feature without ID (still usable via paper UI), and you add the console command as a new feature? Or should I leave it in the label, and you remove it with the console command feature? Sorry for the HUGE amount of text... I thought it would be better for all me considerations to be documented somewhere... And thanks for the help again and again! |
Ok, no problem.
Why not considering the API version ? I guess the API version is different for a bridge v1 ? In this case, we compute the group. If we have the scenes available on each group thing, the feature would be usable. If not, that full feature is simply useless for owners of the bridge v1 while we could make it usable.
Make sense to me.
Ok, I will provide a separate PR. So please don't put the id in the label but please add a TRACE log somewhere in the code so that the scene ids could be retrieved at least from the logs ... until the console command is added. |
I prepared the addition of the console command for scenes with PR #7664 |
c207cda
to
62bb6af
Compare
Travis tests have failedHey @leluna, |
Signed-off-by: leluna <[email protected]>
Travis tests have failedHey @leluna, |
Other problem: The integration test is failing in travis build, and I can't really understand the problem based on the log... Any hint? |
I got a NPE:
It is due to that part |
If I remove this sort, then it looks like the list of scenes is correctly built for each group thing and for the bridge thing. |
On the bridge channel, of course I got only the scene name without any group and so several options with the same label. That is certainly here where my "patch" is required. |
Apparently, we don't have to take care about failing travis incremental builds because there is apparently a bug somewhere in the build process when integration tests are present for a binding. What is important here is that the full build of your PR is succeeded. |
Please note that you have to take care that the binding will still be ok if used with the hue emulation binding. |
@leluna : no time to finish? |
I take a look to the hue emulation binding. If I correctly understand, the hue emulation binding is defining a hue scene for each openHAB rule (NGRE ?) having a particular tag... The recallScene feature from this PR will lead to a failure with the hue emulation binding because it does not yet support "scene" as group state update. That is not really a problem but we just have to check that the returned error will be correctly handled by the hue binding. Then the hue emulation binding could be enhanced to support this new feature. Ideally, this should be tested by someone using the hue emulation binding but I am confident that this enhancement will not break the use of the binding with the hue emulation binding. |
@leluna : as you are not available, I propose to create my own PR with your code and of course I will credit you. I will rebase the code changes, fixes the NPE and add the missing console command. |
Thanks! Really can't find the time to finish it atm. Also, have no clue of the hue simulation binding... The NPE should be fixed already, if I recall correctly. Changed the sort. |
@lolodomo Hey were you already able to commit or merge something? |
Not yet. |
Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
@Tomibeck : in case you are interested in this new feature, please move to the new PR, I inserted a jar file for testing. |
* [hue] Add support for hue scene activation Closes #6043 This is the continuation of the PR #7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
Closed by #8098 |
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]> Signed-off-by: CSchlipp <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]> Signed-off-by: MPH80 <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]> Signed-off-by: Daan Meijer <[email protected]>
* [hue] Add support for hue scene activation Closes openhab#6043 This is the continuation of the PR openhab#7540 95% of credits go to leluna Signed-off-by: Laurent Garnier <[email protected]> Also-by: leluna <[email protected]>
Closes #6043
Signed-off-by: leluna [email protected]