LimitlessLED: Configurable fade-out behavior

[SmilyOrg]

Description:

Adds a per-group fade option with values of out (default) or none.

By default, the lights are faded out when turned off, but this can cause usability issues. For example, when you switch off a light bulb via HA, it first fades to minimum brightness, then turns off. This breaks if you then manually switch it off and on via wall switch, since it turns back on at minimum brightness, which can be frustrating and annoying.

This PR adds a fade per-group option that allows you to turn off the fade out behavior. This is not as nice on v5 bulbs as there is then a harsh cut off, but it preserves manual wall switch behavior. However it's less of an issue for v6 bulbs, since they fade off/on automatically. In fact, maybe the default behavior for v6 bulbs should be none?

Related forum topic: LimitlessLED MiLight starts dimmed if turned off with Hass and then at wall

If this is an acceptable addition, I'll also look into updating the documentation as mentioned below.

🌞 💡 👍

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

light:
  platform: limitlessled
  bridges:

    - host: !secret limitless_v6_ip
      port: 5987
      version: 6
      groups:

      - number: 1
        type: rgbww
        name: Safari Glow
        fade: none

      - number: 2
        type: rgbww
        name: Other Bulb
        fade: out

      # `out` by default
      - number: 3
        type: rgbww
        name: The Third

Checklist:

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass

[mention-bot]

@SmilyOrg, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @happyleavesaoc and @Azelphur to be potential reviewers.

[homeassistant]

Hi @SmilyOrg,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[balloob]

I think that the real solution here is to remove the pipeline. The pipeline is adding a default transition to each request and thus messing up the "on" state that is stored when a device is turned off.

Adding more hacks on top of it does not seem like the correct solution to me.

[SmilyOrg]

I believe the pipeline is used for nicer looking transitions between states, so that lights don't jarringly switch between different colors or brightnesses, so it's not without value. As far as I know, the only time it can break the "on" state is when you add a fade transition before turning it off, which is what this PR addresses.

The off pipeline method doesn't seem to have any inherent default transition to it, it probably just waits for the previous commands to finish before turning it off through the specific turn-off command. I'm not well versed in the underlying library, but that's what I gathered from a brief overview and from its real world behavior.

I think that there are use cases for having both fade options.

1. If you control the light through HA 100% of the time, having a fade out before turning it off looks nicer (at least for v5 bulbs).

2. If you sometimes turn it off directly at a wall switch, not having it fade out is valuable so that it doesn't break the "on" state, but at the expense of not looking as nice (again, v6 bulbs do fading themselves and should probably default to not having a fade out).

Let me know if I'm missing something though!

Thanks for all the hard work on Home Assistant, it's awesome and exactly what I've wished for before I found it 😄 👍

[amelchio]

I think @balloob only suggested removing the pipeline.transition call in turn_off, not the pipeline altogether. I agree with that.

Maybe a compromise is to add the transition only if transition_time is not zero? Then users can still get the broken way if they prefer, but things will work properly by default.

[SmilyOrg]

Ah, I wouldn't have anything against that as that's how I'm currently running it anyway. I was just concerned about use case 1.

I'm not sure where the transition_time comes from though. It seems like the default value is 0, but then I should see it turn off immediately even with pipeline.transition?

[amelchio]

I am afraid that I cannot be of much help. I do not have this hardware so I cannot test.

It sounds like maybe this bulb has a default transition time? Does it change immediately when you set a new brightness in the Home Assistant UI?

[balloob]

Sorry, @amelchio is right. Let's remove it for the turn off command. It only impacts v5 users and it helps people not run into that issue.

[happyleavesaoc]

Transitions on and off were initially added because if you turned off a 100% brightness bulb, you could not turn it on starting at 0%. It would flash 100% brightness (the last state), switch to 0%, and then fade to 100%. This is on v5 bridge.

[SmilyOrg]

@happyleavesaoc that is a good point, I wonder if a better workaround is possible. I'll have to play around with the official app a bit.

[corneyl]

Transitions on and off were initially added because if you turned off a 100% brightness bulb, you could not turn it on starting at 0%.

Is this also the case when one sends the transition command before the on command? Maybe the fade already starts with the bulb off, and continues when it's turned on. The first part of the transition will be invisible, but it will probably look smoother compared to when it's turned on at minimum brightness before the fade starts.

[happyleavesaoc]

@corneyl Pretty sure that anything sent before the on command is ignored by the bulb.

[balloob]

It sucks one way or the other. I think that not being able to turn a light off and on to a normal value is worse than a flash of very bright light ?

[Spartan-II-117]

Seems to me that making it configurable gives us the best of both worlds.

[tbrasser]

@Spartan-II-117 I'm putting all kinds of (groups of) milights behind zwave relays, so having this configurable would be great.

[balloob]

I agree with @Spartan-II-117. Let's make it configurable.

[javydekoning]

Just wanted to pitch in that making this configurable would indeed be very helpful!

[SmilyOrg]

It's annoying that both solutions have irritating cons to them. I played around with the app a bit and I can confirm that it does seem to ignore commands if turned off.

If it is to be configurable, does my proposed fade property make sense and/or does anyone suggest a better solution? Also, should the default stay as is (broken wall switch) or change to the different behavior (flash of light, no turn off transition)? I'm leaning towards the latter, at least for v6 bulbs, as the cons are somewhat mitigated by the integrated fade transition.

I wonder if the behavior of the bulbs could be hacked further using https://github.com/sidoh/esp8266_milight_hub 🤔

[amelchio]

I'll reply because nobody else did, but it seems that the case is made and you are free to pick the solution that you find best.

In my opinion, the fade should probably be skipped by default since it is sort of an extra that could be unexpected if it is not known from other controller apps. The forum topic seems to agree with that.

I wonder about the naming of the option, perhaps it should be a fadeout boolean? Or maybe you anticipate values other than none and out? I have no opinion, just something for you to think about 😃

Again, I do not have this hardware so my comments could be way off.

[javydekoning]

I think it should be just 'fade' as the lights also fade-in when turned on. A boolean should be sufficient, unless if you'd like to control the fade speed.

By default the lights should not fade as this can create unexpected behavior when they are controlled outside of hass.

[SmilyOrg]

My thinking behind fade having none and out values was that as far as I know, this component doesn't exactly fade in currently as much as it turns on and sets the brightness (with a transition?). Though that might make it act like a fade if the light was set to 0% before turning it off (which is what fade out does).

I was thinking that if there was an explicit fade in command, the value set could be expanded to e.g. in and inOut.

However I'm not against having fade just as a boolean for now and changing it if/when it is needed.

[javydekoning]

So, what's needed here to get this merged in?

[amelchio]

I got the impression that @SmilyOrg wanted to change the default around and also update the documentation.

[jodur]

Yeah what is status of this merge proposal. The fading is very annoying when you also use the manual switches or Phone apps.
On and Off commands, should never influence color our brightness settings.

[SmilyOrg]

I'll have to take some time to fix this up and finish it. In the meanwhile if anyone has any thoughts on boolean vs. enum please let me know :)

[jodur]

I think a boolean should be sufficient. Disable or Enable (current behaviour) Fade by HA. The fade should be disabled by default. For compatibillity old users could stil enable the old behaviour.

[happyleavesaoc]

I think that's a reasonable approach. Go with a boolean for now just to get a solution in.

[SmilyOrg]

Ah ok, thanks. I'll fix this up as soon as I get the chance.

[SmilyOrg]

Ok, I fixed it as per discussion and added docs in home-assistant/home-assistant.io#2865. I wasn't able to run tox locally, but I'm not sure if that's a requirement as travis seems to be happy.

[balloob]

Nice 🐬

[balloob]

Marking this as a breaking change because transitions are now opt-in instead of always on.

[SmilyOrg]

Thanks! Sorry that it took me so long 😄