-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Remove floating point calculation from ac_dimmer ISR #3770
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hey there @glmnet, mind taking a look at this pull request as it has been labeled with an integration ( |
@josephdouce can you please review this? |
Will do
Kind Regards
Joseph Douce BEng (Hons) MIET
…On Mon, 5 Sept 2022, 14:48 Guillermo Ruffino, ***@***.***> wrote:
@josephdouce <https://github.com/josephdouce> can you please review this?
—
Reply to this email directly, view it on GitHub
<#3770 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN74Y3ASXJNX3LWCK4UXILV4X22RANCNFSM6AAAAAAQEVGDWM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@josephdouce Have you had a chance to look at this yet?
-Azimath
…On Mon, Sep 5, 2022, 11:52 AM joseph douce ***@***.***> wrote:
Will do
Kind Regards
Joseph Douce BEng (Hons) MIET
On Mon, 5 Sept 2022, 14:48 Guillermo Ruffino, ***@***.***>
wrote:
> @josephdouce <https://github.com/josephdouce> can you please review
this?
>
> —
> Reply to this email directly, view it on GitHub
> <#3770 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAN74Y3ASXJNX3LWCK4UXILV4X22RANCNFSM6AAAAAAQEVGDWM
>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#3770 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYPHF2NDX2NVW3HNVRNSK3V4YJLZANCNFSM6AAAAAAQEVGDWM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Sorry for the delay, can't see any problem here, I was looking to do
something similar myself but didn't get chance. Good improvement moving the
calculation to cover all edge types too.
Kind Regards
Joseph Douce BEng (Hons) MIET
…On Tue, 13 Sept 2022, 19:06 Azimath, ***@***.***> wrote:
@josephdouce Have you had a chance to look at this yet?
-Azimath
On Mon, Sep 5, 2022, 11:52 AM joseph douce ***@***.***> wrote:
> Will do
>
> Kind Regards
>
> Joseph Douce BEng (Hons) MIET
>
> On Mon, 5 Sept 2022, 14:48 Guillermo Ruffino, ***@***.***>
> wrote:
>
> > @josephdouce <https://github.com/josephdouce> can you please review
> this?
> >
> > —
> > Reply to this email directly, view it on GitHub
> > <#3770 (comment)
>,
> > or unsubscribe
> > <
>
https://github.com/notifications/unsubscribe-auth/AAN74Y3ASXJNX3LWCK4UXILV4X22RANCNFSM6AAAAAAQEVGDWM
> >
> > .
> > You are receiving this because you were mentioned.Message ID:
> > ***@***.***>
> >
>
> —
> Reply to this email directly, view it on GitHub
> <#3770 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAYPHF2NDX2NVW3HNVRNSK3V4YJLZANCNFSM6AAAAAAQEVGDWM
>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#3770 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN74YYPPRUDWXN6W265BOLV6C7CBANCNFSM6AAAAAAQEVGDWM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
glmnet
approved these changes
Sep 14, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this implement/fix?
PR#3494 added code to linearize the RMS power output of the ac_dimmer relative to the output state. Doing so introduced an acos into the ISR, which most of the time works, but some race condition exists that causes this call to throw an exception when doing stuff like OTA, setting a new value in home assistant, or sometimes it just keeps working (depending on what version of ESPHome you're running and your luck).
The RMS power linearization only needs to be computed when a new state is written, as it doesn't vary cycle to cycle. This PR moves the acos function outside the ISR into the write_state function where it shouldn't cause problems.
The original PR was considered breaking because it might change how gamma is perceived on some lights. The linearization previously applied only to leading dimmers, while now it applies to both, so it might change how gamma is perceived on trailing dimmers.
Types of changes
Related issue or feature (if applicable): fixes 3509
Test Environment
Example entry for
config.yaml
:Checklist:
tests/
folder).If user exposed functionality or configuration variables are added/changed: