search

HASwitchPlate / openHASP

HomeAutomation Switchplate based on lvgl for ESP32
https://www.openhasp.com
MIT License
726 stars 185 forks source link

Use callback for backlight fade #641

Closed presslab-us closed 9 months ago

presslab-us commented 9 months ago

This adds a callback to the backlight fade which allows for setting the backlight again before the previous fade is complete.

presslab-us commented 9 months ago

This fixes the problems from the commit here: https://github.com/HASwitchPlate/openHASP/pull/626

fvanroie commented 9 months ago

This still doesn't work on the FreeTouchDeck. Not from HA or from the command line... I'm not sure why I would get different results from you, but this needs to work flawlessly on all devices.

presslab-us commented 9 months ago

If you could test the branch without pulling to master it would make things cleaner IMHO. Less back and forth with different commits and pull requests.

I don't have enough information to know why it doesn't work for you, but it looks like FreeTouchDeck uses an older ESP32. Maybe that is the problem, although I would expect some compile error if the function wasn't supported. Perhaps the best solution is to only enable for ESP32-S3 like so: https://github.com/presslab-us/openHASP/commit/584689cd6ae623c5f38c55afbc2c423348f53a0a

fvanroie commented 8 months ago

Well, imho, new features need to be fully tested before a PR is sent. I can do a code review but I cannot test new features on all 20+ devices before merging. That would just take a up too much time. Please test this on multiple devices first.

I thought there was a very low risk in adding this feature, but apparently it is much more complicated than initially thought. My default mode is to just revert the change, as it is more important to have a stable binary.

Another more elegant way is to make your new feature s electable by adding a define USE_HASP_BACKLIGHT_FADE and put the changes into a macro. This way, users can easily test the new feature and disable it quickly without touching any code.

presslab-us commented 8 months ago

I only have one device to test it on; I'm not going to buy 20+ other devices for testing, that's not practical for me either.

I guess my point was that you could have built from my branch and tested that on your device rather than merging it into master and then testing it that way, that's all I was saying. It's possible to do that through VSCode.

fvanroie commented 8 months ago

It's fine, you don't have to buy anything, just don't shift the testing stage onto the maintainer. You can gather other users on the GH discussion board or Discord channel. There are plenty of people that can help you test it.

If I had known this wouldn't work properly, then it wouldn't have been merged. Like I said, if there is a problem I will just roll back.