Fix light flash #559

placidorevilla · 2019-03-22T19:01:05Z

Description:

Fixes light flash. Right now, requesting a flash with whatever duration, actually changes the light to the target color, but doesn't revert. Furthermore, the light component saves the flash configuration as the new one, so when rebooting, or turning off/on, the light changes to the flash color.

Checklist:

The code change is tested and works locally.
The code change follows the standards

OttoWinter · 2019-03-22T20:48:50Z

src/esphome/light/light_transformer.cpp

@@ -59,7 +59,7 @@ LightColorValues LightFlashTransformer::get_end_values() { return this->get_star
 LightFlashTransformer::LightFlashTransformer(uint32_t start_time, uint32_t length, const LightColorValues &start_values,
                                             const LightColorValues &target_values)
    : LightTransformer(start_time, length, start_values, target_values) {}
-bool LightFlashTransformer::is_continuous() { return false; }
+bool LightFlashTransformer::is_continuous() { return true; }


Flashes are not continuous transformers

is_continuous(): Whether the output needs to be written in every loop cycle.

If this is to fix some behavior with flash it should be done in the state.

From what I can see in the code, if is_continuous is false, the flash is applied in the first iteration of loop, but then the loop ends and it's never reverted back to the previous state. What is your suggestion of how this should be? (is_continuous is very confusing as a property, since you could argue that transitioning from brightness 50% to brightness 50% over 10 seconds is continuous, but then so is a flash, since it has a duration...).

but then the loop ends and it's never reverted back to the previous state

See line 63, at the end of a transformer the end values are applied (or they should be)

this->remote_values_ = this->values_ = this->transformer_->get_end_values();

but then so is a flash, since it has a duration...

Continuous here means that the transformer has a new value in each loop cycle, like it is for a transition. Flashes only need to override the state two times: At the beginning of the flash, and at the end of the flash.

The flash transformer never says is_finished, because it's not updated ever again, since it's not continuous :-)

OK, I think I found out what you intended to do here. If we call get_current_values() while there's a transform in place in each iteration of the loop if this transform is not continuous, the transform will eventually end and it will revert and be removed

Ah ok now I understand, yes that makes sense :)

On second thought, I think we can just remove is_continuous - while a transformer is active it should have complete control of the light. So remove is_continuous and change line 204 to

if (this->next_write_ || this->transformer_ != nullptr) {

OttoWinter · 2019-03-22T20:53:12Z

src/esphome/light/light_state.cpp

@@ -197,7 +202,7 @@ void LightState::current_values_as_cwww(float color_temperature_cw, float color_
  *warm_white = gamma_correct(*warm_white, this->gamma_correct_);
 }
 void LightState::loop() {
-  if (this->active_effect_ != nullptr) {
+  if (this->active_effect_ != nullptr && this->transformer_ == nullptr) {


Please revert this, effects should still be applied when a transformer is active (effects can call transformers)

Then it's impossible to flash while an effect is currently running.

It is, see random light effect as an example. Light effects do not always write to the state. As seen in the random light effect, they can use the .turn_on(); ... .perform(); api to call transitions etc.

Let me give you an example of what I mean. I have an rgb strip, that I can apply an effect to (let's say rainbow). I also have a user defined service like this:

20 services: 21 - service: flash 22 then: 23 - light.turn_on: 24 id: light 25 brightness: 100% 26 red: 100% 27 green: 100% 28 blue: 100% 29 flash_length: 100ms

While I'm running the effect. If I call that service asynchronously, the strip won't flash, it will briefly change brightness, but that's all.

I see, but I'd say that's the intended behavior. Flashes (like transitions) should only temporarily affect the light strip, after that the previous state should be exactly continued. So for example flashing red during a rainbow effect should:

Turn strip red

Wait for flash time

Go back to rainbow effect

If the user wants to cancel the effect they must a) turn off the light or b) call with effect: "None".

Removing is_continuous would solve that I think

src/esphome/light/light_state.cpp

placidorevilla · 2019-03-27T04:44:25Z

I see, but I'd say that's the intended behavior. Flashes (like transitions) should only temporarily affect the light strip, after that the previous state should be exactly continued. So for example flashing red during a rainbow effect should:

Turn strip red
Wait for flash time
Go back to rainbow effect
If the user wants to cancel the effect they must a) turn off the light or b) call with effect: "None".

The current code in this PR implements exactly this behavior. The stop/start of the effect is necessary, since for addressable lights, upon enabling an effect, the AddressableLight object takes full control of the light (via set_effect_active()), so the transform doesn't do anything, even if active_effect_->apply() is not called.

Removing is_continuous would solve that I think

On second thought, I think we can just remove is_continuous - while a transformer is active it should have complete control of the light

Removing is_continuous would also work, but the only place this method is called is in this loop anyway, so it would be reasonable to completely remove the method from the abstract class completely. As a drawback, for flashes, it would update the output on every loop, which is not necessary. The code in the PR keeps is_continuous and uses it to avoid updating the output, and it just checks if the transform is finished in case it's not continuous.

LMKWYT, I'd keep this PR as it is right now, since I believe it implements the intended behavior without any of the drawbacks, but if you prefer to remove is_continuous altogether and pay the penalty of updating the output on every loop while the transition is ongoing, I can do that too.

OttoWinter · 2019-03-27T21:01:26Z

the AddressableLight object takes full control of the light (via set_effect_active()), so the transform doesn't do anything, even if active_effect_->apply() is not called.

Addressable effects are a completely different thing. Yes they take full control of the strip (and in fact they're kind of a hack). But for non-addressable effects my statement still stands: transition/flash are temporary transformers. AND effects can call them internally. Calling a transition or flash should not kill the effect.

By that reasoning, we should also kill the effect when a transition begins - but some addressable effects for example take the current RGB value of the light as the base of their data (for example addressable flicker)

As a drawback, for flashes, it would update the output on every loop, which is not necessary.

Yes that was the original intent of that property. However, there are no outputs that I'm aware of for which this would be any problem. If that were the case, transitions would never work.

since I believe it implements the intended behavior without any of the drawbacks

As I said, this PR will change the behavior in that flashes cancel the effect. Flashes are temporary changes to a light's state and should not affect anything

placidorevilla · 2019-03-27T21:16:33Z

the AddressableLight object takes full control of the light (via set_effect_active()), so the transform doesn't do anything, even if active_effect_->apply() is not called.

Addressable effects are a completely different thing. Yes they take full control of the strip (and in fact they're kind of a hack). But for non-addressable effects my statement still stands: transition/flash are temporary transformers. AND effects can call them internally. Calling a transition or flash should not kill the effect.

The code I propose restarts the effect as soon as the flash is done. If this is not a valid approach, then a flash can't take over the effect. I was maybe thinking of doing something specific for addressable effects (like pausing the effect somehow, but the APIs are protected for that). Honestly, I don't know how to implement the behavior without stopping/restarting the effect.

By that reasoning, we should also kill the effect when a transition begins - but some addressable effects for example take the current RGB value of the light as the base of their data (for example addressable flicker)

Transitions don't work in this case anyway. If flicker is running and I request a color change, the color is immediately changed (still flickering, though).

As a drawback, for flashes, it would update the output on every loop, which is not necessary.

Yes that was the original intent of that property. However, there are no outputs that I'm aware of for which this would be any problem. If that were the case, transitions would never work.

I'm not saying it would be a problem, but it feels weird to update the output with the same color on every loop for the duration of the flash. If you think this is OK, I can completely remove the is_continuous methods.

since I believe it implements the intended behavior without any of the drawbacks

As I said, this PR will change the behavior in that flashes cancel the effect. Flashes are temporary changes to a light's state and should not affect anything

As I said, the effect restarts immediately after (in fact, it doesn't clear the active_effect_ member, so it can maintain the state, for example, for the scan effect, if flash is called, when it's done, the scan continues from exactly the same pixel).

OttoWinter · 2019-03-27T21:32:48Z

If this is not a valid approach, then a flash can't take over the effect.

That's the thing, the issue here are addressable effects - they do not work like other effects in that they allow transformers to work while they're present. With "normal" effects flashes do take over the effect.

Transitions don't work in this case anyway. If flicker is running and I request a color change, the color is immediately changed (still flickering, though).

Yes, they do not transition to the target color, but they do use it as an argument - one of the use cases here is that you can give an addressable effect a color as a parameter.

As I said, the effect restarts immediately after (in fact, it doesn't clear the active_effect_ member, so it can maintain the state, for example, for the scan effect, if flash is called, when it's done, the scan continues from exactly the same pixel).

Oh I did not see that. However, I do not like this approach at all as there could be tons of other edge cases, for example: what happens when a light effect is cancelled while a flash is in effect? stop will be called twice - it may work now but custom effect writers will not except that. I'm sure there are other edge cases too - especially for the light component I like things to be 100% clean with no workarounds because the logic is already very complex.

## Description: Extension of #559 (I want this fix in 1.12.2, but the fix in there is too hacky and will probably cause other issues in the future; addressable light effects need a new effects model to fix them) **Related issue (if applicable):** fixes <link to issue> **Pull request in [esphome](https://github.com/esphome/esphome) with python changes (if applicable):** esphome/esphome#<esphome PR number goes here> **Pull request in [esphome-docs](https://github.com/esphome/esphome-docs) with documentation (if applicable):** esphome/esphome-docs#<esphome-docs PR number goes here> ## Checklist: - [ ] The code change is tested and works locally. - [ ] The code change follows the [standards](https://esphome.io/guides/contributing.html#contributing-to-esphome-core) If user exposed functionality or configuration variables are added/changed: - [ ] Documentation added/updated in [esphome-docs](https://github.com/esphome/esphome-docs).

OttoWinter reviewed Mar 22, 2019

View reviewed changes

Fix light flash

790b8b8

OttoWinter mentioned this pull request Mar 31, 2019

Fix light flashes #578

Merged

3 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix light flash #559

Fix light flash #559

placidorevilla commented Mar 22, 2019

OttoWinter Mar 22, 2019

placidorevilla Mar 22, 2019

OttoWinter Mar 22, 2019 •

edited

placidorevilla Mar 22, 2019

placidorevilla Mar 22, 2019

OttoWinter Mar 23, 2019

OttoWinter Mar 22, 2019

placidorevilla Mar 22, 2019

OttoWinter Mar 22, 2019

placidorevilla Mar 22, 2019

OttoWinter Mar 23, 2019

placidorevilla commented Mar 27, 2019

OttoWinter commented Mar 27, 2019

placidorevilla commented Mar 27, 2019

OttoWinter commented Mar 27, 2019

Fix light flash #559

Fix light flash #559

Conversation

placidorevilla commented Mar 22, 2019

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OttoWinter Mar 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

placidorevilla commented Mar 27, 2019

OttoWinter commented Mar 27, 2019

placidorevilla commented Mar 27, 2019

OttoWinter commented Mar 27, 2019

OttoWinter Mar 22, 2019 •

edited