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

[lutron] Added a feature to override dimmer Thing configuration fade in/out times #8133

Closed

Conversation

aguiswite
Copy link

When feature is enabled, the specified Item fade time will be used as light level changes. When disabled, dimmer fading will be set according to the preconfigured default. Useful for changing lighting behavior in rules or on sitemap via UI.

Signed-off-by: Austin Guiswite [email protected]

…en enabled, the specified Item fade time will be used as light level changes. When disabled, dimmer fading will be set according to the preconfigured default. Useful for changing lighting behavior in rules or on sitemap via UI.

Signed-off-by: Austin Guiswite <[email protected]>
@aguiswite aguiswite requested a review from bobadair as a code owner July 15, 2020 20:53
@TravisBuddy
Copy link

Travis tests have failed

Hey @aguiswite,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@lolodomo lolodomo changed the title Added a feature to override dimmer Thing configuration fade in/out times [lutron] Added a feature to override dimmer Thing configuration fade in/out times Jul 15, 2020
@bobadair
Copy link
Member

Hi @aguiswite! Yes, support for fade and delay times in the dimmer handler is pretty bad at the moment. For fade time, it has used the following scheme since before I started working on it:

ON commands use the time specified in the fadeInTime parameter.
OFF commands use the time specified in the fadeOutTime parameter.
Percent commands (including 0 and 100) use a fixed 0.25 second fade value. Why? I don't know.
Delay times aren't supported at all yet.

I tried modifying the dimmer thing to add fade and delay channels, similarly to what you've done here, but it wasn't an especially satisfactory solution for some of the reasons discussed here:

openhab/openhab-core#1298

Unfortunately the proposal discussed in that issue to add attributes (such as fade and delay) to commands didn't go anywhere.

With Lutron, it's all further complicated by the fact that the LIP protocol supports fade and delay times in different formats, each with different time ranges. You can use seconds/fractions of seconds (ss.ss), seconds (ss), minutes/seconds (mm:ss) or hours/minutes/seconds (hh:mm:ss). Then, to make things even a bit more complicated, I'm in the process of adding support for the LEAP protocol, and that imposes it's own requirements on fade and delay times.

It seems like the approach being taken to support dimmer fade and delay times in other bindings is to use things actions. That's what I'd been planning to do with the Lutron binding, along with changing the fixed 0.25s delay for a number/percent command in the dimmer handler to use the fadeInTime parameter value instead.

I have a dev branch with a commit that adds dimmer thing actions that accept fade & delay here, if you want to take a look at it:

https://github.com/bobadair/openhab2-addons/tree/lutronactions

Would that work for your use case? It's not quite done yet, because it doesn't support all of the different lutron time formats. It's sort of been on the back burner for a while since I was working on other stuff, and frankly nobody else showed much interest in it.

@aguiswite
Copy link
Author

Thanks @bobadair! This is my first time hearing about Thing Actions. I looked at your updates and also did a little extra digging into other posts regarding this approach and it's clear to me that it is a far better solution. I unfortunately don't know enough about the other types of Lutron devices in order to help out, but I'm going to try compiling your updates to see how exactly they work. Can you confirm the below sample rule code is correct?

rule "Turn on Hallway Light (Night)" when Item HallwayPIR received command OPEN then val actions = getActions("lutron","lutron:dimmer:hallway") actions.setLightLevel(ON, 3, 0) end

@bobadair
Copy link
Member

Yes, thing actions are relatively new. A lot of binding developers started adding them in the 2.5 development cycle, but we're a little behind the curve here in Lutron binding land. :-)

Take a look at that branch and see if you can get it to work for you. I haven't really looked at it in a while, but I did a rebase/push on it just a few days ago, so it is at least based on a current version. There is a fix that needs to be incorporated in it, but I'll try to do that tomorrow. Your example rule looks roughly right to me, but I couldn't really confirm that it's correct without running it myself.

@bobadair
Copy link
Member

Hi @aguiswite. I pushed a couple of commits to that lutronactions branch that should have the dimmer thing action working now. I haven't had a chance to test it yet, though.

@aguiswite
Copy link
Author

aguiswite commented Jul 17, 2020

Thanks for making those changes. Last night I tested the setLightLevel action and got it working with my rules. There was one minor bug fix that needed to be made for a null pointer exception in the dimmer handler. I committed a fix to my forked copy of your branch if you want to check it out.

https://github.com/aguiswite/openhab-addons/commit/f2533fe1b85a1d4446123d136d8d01c1c135bbd7

@bobadair bobadair added the work in progress A PR that is not yet ready to be merged label Jul 17, 2020
@bobadair
Copy link
Member

Cool. Thanks. I pulled that commit in to my dev branch. That's what happens when you try to rush a code change and then don't have time to test it! Anyway, does the dimmer thing action pretty much accomplish what you want?

BTW - I added a WIP label to this PR while we discuss it.

@aguiswite
Copy link
Author

The dimmer thing action is exactly what I needed. I expect to only need this functionality within rules so it's a perfect solution. Should I continue using this version of the binding from your branch or should I wait until it's merged into master?

@bobadair
Copy link
Member

Should I continue using this version of the binding from your branch or should I wait until it's merged into master?

Sure, if that version works for you, use it! I want to make a few more changes and do some testing before I submit it, and then it will need to be reviewed, so it may be a week or two before it gets merged.

@bobadair
Copy link
Member

I have created PR #8153 to add the dimmer thing action discussed here.

@aguiswite - The action name and parameters have changed from the previous version, so you'll need to update your rules again when it is merged.

@aguiswite aguiswite closed this Jul 28, 2020
@aguiswite aguiswite deleted the lutron-dimmer-fade-feature branch July 28, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants