Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use callback for backlight fade #641

Merged
merged 3 commits into from
Feb 17, 2024

Conversation

presslab-us
Copy link
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
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
Copy link
Collaborator

fvanroie commented Feb 17, 2024

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
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
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
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
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