Add options flow with option to restore isy light brightness after off#46
Add options flow with option to restore isy light brightness after off#46shbatm merged 2 commits intoshbatm:masterfrom
Conversation
|
I'm struggling with this one. Principally, this does not match ISY behaviors... That's what On Levels are for, although I get why it's desired in Home Assistant and understand that other integrations do it this way. @OverloadUT... Punting this to you. As the current defacto code owner, what do you think? Keep the ISY standard (on levels) or adopt the trending HA method? |
|
@shbatm My first thought was to set the on_level via the api (which is how I found the PyISY issue) but since that would affect the switch locally that seemed like it wasn't a good fit for ISY. Not so great an experience if you dim the lights to 20% at night from home assistant and then the next day you (or someone else) turns the light on at the physical switch and get 20%. |
|
Oh man, this is indeed a tough one. (Although I definitely agree with NOT setting the on levels for the reason you explained.) Let's lay out some pros and cons of making this change: Pros
Cons
I'm really leaning towards no here, but it's far from cut and dry. I think what I really would like to see here is a Home Assistant ADR on this functionality. Then we could just conform to the official standard rather than try to choose based on a trend in implementation. |
|
@OverloadUT How about making this configurable in an options flow so the user can turn it off if they don't like the HomeAssistant defacto behavior? |
|
I think that would be a perfect solution. We try to keep away from having a bunch of configuration options, but this seems like a case where one is truly needed. |
|
I'll work on something this weekend |
I agree, I'd like to see this standardized in the Architecture. Reading some more back on the original comments linked above--the originating issue was that the old PyISY integration assumed full brightness when receiving an empty ON command, the new updates actually use the On Level like they're supposed to. @OverloadUT I agree with the Pros/Cons you listed. I can see why it should be an option in some cases--it is in line with the new Home Assistant Scenes philosophy of "saving" and "restoring" the light states, which is great if you control everything from HA, but it won't match the physical switch behavior, or the behavior in the ISY Programs (if you're using both HA and ISY like me). To me I think clicking on in HA should behave the same as clicking on the physical switch. If I want to restore the last on states, I should be using HA scenes with the restore capability. @bdraco I like the idea of adding a configuration option to enable it. Until an ADR or agreement in Architecture is made to decide one way or another, I think the default should be using the ISY's On Levels. In addition to the config option, something else that would be useful would be to add the "last on state" to the device state attributes (on level is there already), then the user can have case-by-case control for each device--they can assign a Lovelace button tap action to use either one as the brightness. |
|
@shbatm I'll set the default to the existing behavior of going to on-level when turning on and making the restore to previous home assistant level something the user needs to select when I build this. I'm going to link a couple z-wave bulbs to the isy to see how they behave in order to see if this is an insteon only behavior or an isy behavior. |
|
I couldn't test with z-wave as the brightness mapping isn't implemented (I guess there aren't any z-wave light users with an isy994). z-wave brightness on isy994 is apparently mapped 0..100 instead 0.255 which is curious since z-wave is usually 0..99. |
|
I started looking into adding better support for #21, but haven't had time to implement anything other than extending a lot of the Zwave properties. I started looking through the HA Z-Wave component and it seems like it's going to be messy to fully implement for ISY... As for the 0-255 vs 0-99 I think I've seen them both used on the ISY for different devices (based on the SDK)... Like I said, messy. |
bdraco
changed the title
Move sensor_string and sensor_string into the options flow as the config flow is only supposed to collect the minimal amount of information to get the integration up and running Add a Restore Light Brightness option to the config flow Add a last_brightness attribute to lights
|
@shbatm I fixed the conflict from the translation rename. This should be ready for review if you have a moment. I've been running it for 2 weeks without any issues. |
3 participants
Upstream change in Home Assistant
home-assistant/core#34320
Originated from
home-assistant/core#34299 (comment)