Skip to content

Use callback for backlight fade - #641

Merged
fvanroie merged 3 commits into
HASwitchPlate:masterfrom
presslab-us:backlight_fade_cb
Feb 17, 2024
Merged

fvanroie merged 3 commits into
HASwitchPlate:masterfrom
presslab-us:backlight_fade_cb

Conversation

@presslab-us

Copy link
Copy Markdown
Contributor

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

@presslab-us

Copy link
Copy Markdown
Contributor Author

This fixes the problems from the commit here: #626

@fvanroie
fvanroie merged commit 2fe7bcc into HASwitchPlate:master Feb 17, 2024
fvanroie pushed a commit that referenced this pull request Feb 17, 2024
@fvanroie

fvanroie commented Feb 17, 2024

Copy link
Copy Markdown
Collaborator

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.

fvanroie pushed a commit that referenced this pull request Feb 17, 2024
@presslab-us

Copy link
Copy Markdown
Contributor Author

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: presslab-us@584689c

@fvanroie

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants