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

[hue] Reintroduce LK Wiser dimmer work-around for API v2 #15316

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Jul 27, 2023

Fixes #15315

@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Jul 27, 2023
@andrewfg
Copy link
Contributor

see how I can best reduce runtime impact by caching the need for the work-around rather than string-comparing the modelId property for each command

@jlaur you would simply set a boolean flag in the updateProperties() method.

@jlaur
Copy link
Contributor Author

jlaur commented Jul 28, 2023

see how I can best reduce runtime impact by caching the need for the work-around rather than string-comparing the modelId property for each command

@jlaur you would simply set a boolean flag in the updateProperties() method.

Yes, that was the old implementation. I just want to keep the code clean, and if there would be 10 booleans over time, it would start to get messy. But you are right, let's not over-engineer, we can always adapt if this would become a problem.

@andrewfg
Copy link
Contributor

if there would be 10 booleans over time, it would start to get messy

I suppose you could have a List of references to different product specific patch methods that apply model specific overrides for different device models, and then call List.forEach(call respective patch method). But honestly I think that would be even more messy.

@jlaur jlaur force-pushed the 15315-hue-apiv2-lk-wiser-dimmer branch from dcd53d3 to 07c156e Compare July 28, 2023 10:39
@andrewfg
Copy link
Contributor

@jlaur apropos the Osram patch: The original discussion link http://www.everyhue.com/vanilla/discussion/1756/solved-lightify-turning-off (*) has disappeared. And from googling and ebay I am pretty sure that the old Osram GU10 lamp that had this bug, is not longer available to buy. So probably the issue would only apply to older migrated bulbs.

(*) and also the new forum link https://developers.meethue.com/ seems currently to be returning a "502 Bad Gateway" error..

@jlaur
Copy link
Contributor Author

jlaur commented Jul 28, 2023

@jlaur apropos the Osram patch: The original discussion link http://www.everyhue.com/vanilla/discussion/1756/solved-lightify-turning-off (*) has disappeared. And from googling and ebay I am pretty sure that the old Osram GU10 lamp that had this bug, is not longer available to buy. So probably the issue would only apply to older migrated bulbs.

(*) and also the new forum link https://developers.meethue.com/ seems currently to be returning a "502 Bad Gateway" error..

Thanks, I also found that link no longer works. Since I don't have the details about the specifics here, I think I will scope the PR to only fix the LK Wiser issue. If anyone with an old bulb would experience issues, we could reimplement it then, and would have a chance to test it. There might be differences in the approach. I just re-verified that any transition time works with the LK Wiser Dimmer. I don't know if Osram needed a transition time of zero.

Copy link
Contributor

@andrewfg andrewfg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the above comment LGTM

Fixes openhab#15315

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur force-pushed the 15315-hue-apiv2-lk-wiser-dimmer branch from 07c156e to 5b5b6ed Compare July 28, 2023 10:55
@andrewfg
Copy link
Contributor

andrewfg commented Jul 28, 2023

I just re-verified that any transition time works with the LK Wiser Dimmer.

So probably the work around should check if there is already a transition time set in the DTO, and only if it is not set, then the patch should set a value of zero. This would eventually allow users to apply slow transitions.


EDIT: OK I see that you did already do the above in the latest commit.

@jlaur jlaur marked this pull request as ready for review July 28, 2023 11:05
@jlaur jlaur requested a review from cweitkamp as a code owner July 28, 2023 11:05
Copy link
Contributor

@andrewfg andrewfg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewfg
Copy link
Contributor

@cweitkamp / @jlaur I suggest to hold merging this PR until #15323 discussion is resolved.

@andrewfg andrewfg added the work in progress A PR that is not yet ready to be merged label Jul 28, 2023
@jlaur
Copy link
Contributor Author

jlaur commented Jul 28, 2023

@cweitkamp / @jlaur I suggest to hold merging this PR until #15323 discussion is resolved.

I consider this a regression in 4.0 and would prefer to have it merged and cherry-picked into 4.0.x to be included in 4.0.2.

Depending on the outcome of #15323 it will either be refactored (most likely) or removed, but I don't see this as an obstacle. This is probably to be considered an enhancement and thus would be destined for 4.1.

@jlaur jlaur removed the work in progress A PR that is not yet ready to be merged label Jul 28, 2023
@jlaur jlaur requested a review from a team July 28, 2023 18:57
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@lolodomo lolodomo merged commit 92238bc into openhab:main Jul 31, 2023
@lolodomo lolodomo added this to the 4.1 milestone Jul 31, 2023
@jlaur
Copy link
Contributor Author

jlaur commented Jul 31, 2023

@lolodomo - would you agree to cherry-pick this into the 4.0.x branch? This work-around already existed for API v1, so it's a regression.

@lolodomo
Copy link
Contributor

Ok, fine.

jlaur added a commit that referenced this pull request Jul 31, 2023
Fixes #15315

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur added the patch A PR that has been cherry-picked to a patch release branch label Jul 31, 2023
@jlaur jlaur deleted the 15315-hue-apiv2-lk-wiser-dimmer branch July 31, 2023 14:51
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
Fixes openhab#15315

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] LK Dimmer cannot be turned off with API v2
3 participants