Don't consider turning off/on as state change for preset resetting

[R1DEN]

should also revert to the behavior observed prior to 0.14.0

[blazoncek]

Unfortunately this doesn't get my approval. For the reasons posted in #3467

[R1DEN]

@blazoncek sorry for being a bit stubborn, but what would be the core issue with reverting things the way they were? As I've described here

[blazoncek]

Removing stateChanged = true; will prohibit interface update (including sending WS notifications, Alexa, etc).

[R1DEN]

@blazoncek sorry, my bad, mixed up briOld and briLast... made a separate bool that is changing only when turning the LEDs on/off which will not trigger a preset reset

[R1DEN]

@softhack007 hey, since you added the discussion label, maybe you have any ideas? Given your knowledge or wled internal workings and this comment

[danlex-corp]

@blazoncek I would like to add about saving the active preset when turned off. After upgrading to version 0.14, I observe the following behavior: When you turn on the WLED through a button in the interface or API, the active preset is not restored as a parameter (ps), it remains “-1”, although the behavior of the tape itself is restored and as if the preset is active, but this is only visible visually. It was very convenient to use the current active preset in Home Assistant scenarios, but when you turn it on/off, the preset is logically reset.

[blazoncek]

@danlex-corp since preset also stores WLED on state when you turn WLED on or off using UI power button you invalidate the currently selected preset. Unfortunately there is no way of knowing if other parameters changed as well without introducing additional variables to store entire state and previous state. This is unacceptable on ESP8266.

[danlex-corp]

@blazoncek I removed "on" state from the presets. This doesn't help either. As I understand it, now the active preset is not saved at all when turned off? How did this work before on version 0.13?

[R1DEN]

Isn't it possible to just not invalidate the preset when turning it off/on? And invalidate it on all other changes? I don't see the need for additional variables (apart from the toggledOnOff in this PR) if the invalidation check can be done on the fly. Can confirm this did work in 0.13

[Aircoookie]

I prefer the pre-0.14 behavior too, on/off should stay in preset just as changing the master brightness does.

However, the implementation with toggledOnOff is incorrect, as when other state is updated simultaneously with on/off, preset should be reset, but is incorrectly kept. Just removing stateChanged = true; in toggleOnOff() as originally implemented in https://github.com/Aircoookie/WLED/pull/3479/commits/9fbbad50f2ebbf8631054d1383480b3151dbcbc8 should be fine though.

@blazoncek

Removing stateChanged = true; will prohibit interface update (including sending WS notifications, Alexa, etc).

It shouldn't, I think? bri != briOld in led.cpp L103 also covers the case of on/off.

[blazoncek]

led.cpp is a mess and an burdensome legacy (and you know it). 😁 And all the logic behind it is a relic from the past (handleTransition() & stateUpdated()). It took me a year to figure out that convoluted logic it harbours. 😂

BTW toggleOnOff() will not initiate stateUpdated() so if you want to start a transition and do change brightness, you either need stateChanged = true; or call stateUpdated() from it.

Regardless I still think that when you toggle WLED on/off using UI power button you are invalidating a preset. Why? Have a preset with win&T=0 (or {"on":false}) and use it instead of power button. What happens? Now change it to win&T=2. What happens then?

FYI In 0_15 you can override UI power button to actually call a preset instead of performing plain on/off.

[blazoncek]

BTW when talking about global brightness (and on/off) I keep getting reminded of #2984 and the pain global brightness brings with it.

[github-actions[bot]]

Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. Thank you for contributing to WLED! ❤️

[R1DEN]

Any chance this could be implemented?

[blazoncek]

Not in this form.

[R1DEN]

Sure, that's not an issue. Maybe you could provide me some more context, info, links to "hidden dev/architecture rules" that are not in the contribution guide or something so that I could remake this PR?

[softhack007]

links to "hidden dev/architecture rules" that are not in the contribution guide

Hi @R1DEN, actually we don't have these documents. however to keep the overall design and architecture understandable and maintainable, it's our job as maintainers to discuss your proposed PR with you, and make sure it fits to the overall architecture.

In some industries this role is called "design authority". We don't have enough people to maintain an up-to-date architecture document (plus 100 pages of detailed design and coding rules), so our way is to engage in design discussions when a PR is presented for review.

I hope you understand. It's not about avoiding mistakes, but about keeping the wled design consistent.

[R1DEN]

@softhack007 perfectly understandable. So is the information already in the PR comment sufficient for me to try come up with a different solution? Or would you (or anybody else) add something?

[softhack007]

@softhack007 perfectly understandable. So is the information already in the PR comment sufficient for me to try come up with a different solution? Or would you (or anybody else) add something?

As led.cpp is not really "my area", I would pass this question on to @blazoncek and @Aircoookie.

I do agree with comments from @blazoncek that the code in led.cpp is quite tricky - any change may have unexpected side-effects - so maybe a different solution would be preferable from maintainability perspective.

[blazoncek]

I veto this in any case. For the reasons stated above. But since WLED is not my project @Aircoookie may decide otherwise.