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] Add support for TransitionTime #4660

Merged
merged 1 commit into from
Jan 27, 2019
Merged

Conversation

xLAva
Copy link

@xLAva xLAva commented Jan 23, 2019

Implemented fade time config for light things.
Implemented fade time channel for dynamically changing the fade times of light things.

Bug: #1722
Signed-off-by: Jochen [email protected]

@wborn
Copy link
Member

wborn commented Jan 23, 2019

It's fixing eclipse-archived/smarthome#1722.

@wborn wborn changed the title [Hue] Add support for TransitionTime #1722 [hue] Add support for TransitionTime Jan 23, 2019
@xLAva
Copy link
Author

xLAva commented Jan 24, 2019

The PR-openHAB2-Addons Jenkins build failed.
I checked the log and it looks like it was not related to my pull request.

@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 24, 2019
@kaikreuzer
Copy link
Member

Thanks for this PR! (I have re-run the PR build, now it is green)

It lgtm, my only concern is the new fade time channel: I do not think that we should add channels for adjusting configurations. This looks like a very ugly workaround and imho we should not create a precedence.

I'd therefore prefer to remove it and rather discuss how we could make it possible through rules to change channel configurations, which would be the proper solution to the problem.

@xLAva
Copy link
Author

xLAva commented Jan 25, 2019

I can understand your concerns.
I can split it up and only PR the configuration part.

But I really would like a dynamic way of controlling the fade time.
In my private use cases, I change the fade time for a single light nearly on a command basis.

The channel implementation was of course a workaround. My favorite solution would be to have control over the fade time with each light command send to the light.

I’m just not familiar enough with the code-base to know a better solution to accomplish this. The usage of a channel seemed like an easy solution.

Maybe your suggestion about the rules could be a way. Can you tell me more about that approach?
Or do you have another idea how to pack an additional value into the existing commands?
A wrapper command maybe?

@t-8ch
Copy link
Member

t-8ch commented Jan 25, 2019

This discussion would also be relevant for the ZigBee binding (cc @cdjackson) .
See also eclipse-archived/smarthome#6755

@xLAva
Copy link
Author

xLAva commented Jan 26, 2019

I tried a new approach with ThingActions instead of the Channel solution.
Is this the right direction to go?

@kaikreuzer
Copy link
Member

No, I don't think so. The new potential feature of changing channel configurations through rules is not specific to any binding, but would be applicable to all - so this would be a framework feature and nothing to be added to each and every binding.

@xLAva
Copy link
Author

xLAva commented Jan 26, 2019

But I actually don't want to change the configuration. I want to send a Light command with a specific fadetime and just ignore the configuration for that one command.

@kaikreuzer
Copy link
Member

Ok, yes, for parametrized commands, the ThingActions are the way to go.

@xLAva
Copy link
Author

xLAva commented Jan 26, 2019

Ok cool.
Just let me know if you have some more comments about my code, once you had time to look at it of course.

@kaikreuzer
Copy link
Member

Thanks, looks good to me!

Just one last request: Could you please also add the new features to the README.md?

@xLAva
Copy link
Author

xLAva commented Jan 26, 2019

Sure thing. Will do it tomorrow.

@xLAva
Copy link
Author

xLAva commented Jan 27, 2019

The README.md is updated in the new PR.

@xLAva
Copy link
Author

xLAva commented Jan 27, 2019

The PR-openHAB2-Addons Jenkins build failed again. Doesn't look like an issue with my changes.

@kaikreuzer
Copy link
Member

Thanks! The only thing you forgot is to also add the transitionTime configuration parameter to the README :-)

Implemented fade time config for light things.
Implemented ThingAction fadingLightCommand for Hue lights
Updated README.md

Bug: openhab#1722
Signed-off-by: Jochen [email protected]
@xLAva
Copy link
Author

xLAva commented Jan 27, 2019

You are right. Sorry about that. Added the configuration parameter documentation with the newest PR.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Perfect, now it looks good & complete 👍
Many thanks!

@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature for an existing add-on label Jan 27, 2019
@kaikreuzer kaikreuzer merged commit 92d71a9 into openhab:master Jan 27, 2019
@xLAva
Copy link
Author

xLAva commented Jan 27, 2019

Thank you for the fast feedback comments. :-)

@wborn wborn added this to the 2.5 milestone Feb 28, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Jun 19, 2019
Implemented fade time config for light things.
Implemented ThingAction fadingLightCommand for Hue lights
Updated README.md

Bug: openhab#1722
Signed-off-by: Jochen [email protected]
Signed-off-by: Pshatsillo <[email protected]>
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
Implemented fade time config for light things.
Implemented ThingAction fadingLightCommand for Hue lights
Updated README.md

Bug: openhab#1722
Signed-off-by: Jochen [email protected]
Signed-off-by: Maximilian Hess <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants