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.

Sign up for GitHub

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

Jump to bottom

[WIP] fix backlight_avr.c breathing feature doesn't handle correctly #16628

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[WIP] fix backlight_avr.c breathing feature doesn't handle correctly #16628

wants to merge 2 commits into from

Conversation

monksoffunk
Copy link
Contributor

@monksoffunk monksoffunk commented Mar 13, 2022 •
edited
Loading

Description

The current backlight breathing feature for AVR doesn't work.
This PR aims to solve the following 2 problems.
I want to start as Draft PR and change the stage after discussion.

Problem 1

Breathing does not appear to occur, at least visually.

This is caused by argument overflow for scale_backlight

set_pwm(cie_lightness(rescale_limit_val(scale_backlight((uint16_t)pgm_read_byte(&breathing_table[index]) * ICRx / 255))));

this must be

set_pwm(cie_lightness(rescale_limit_val(scale_backlight((uint16_t)pgm_read_byte(&breathing_table[index]) * (ICRx / 255)))));

or

set_pwm(cie_lightness(rescale_limit_val(scale_backlight((uint16_t)((uint32_t)pgm_read_byte(&breathing_table[index]) * ICRx / 255)))));

Problem 2

Even after correcting 1, the behavior of breathing is still strange.
Specifically, after breathing slightly shorter than BREATHING_PERIOD, another short breathing occurs and the next breathing starts in the middle.

Cause:
Because BREATHING_STEPS is 128, the decimal point of uint16_t interval is truncated in most BREATHING_PERIOD settings including default 6.
By BREATHING_PERIOD 3 setting, interval is (uint16_t)3* 120 / 128 = (uint16_t)2.8125 = 2, so the problem becomes more pronounced.

Solution 1:
Change BREATHING_STEPS to 120

Solution 2:
Change interval from int to float

Solution 1 is preferable from the point of view of AVR load.
Since BREATHING_ISR_FREQUENCY is 120, BREATHING_ISR_FREQUENCY / BREATHING_STEPS always be 1.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Mar 13, 2022
@drashna drashna requested a review from a team March 13, 2022 07:30
@monksoffunk
Copy link
Contributor Author

monksoffunk commented Mar 13, 2022 •
edited
Loading

Monitoring variables(when BREATHING_PERIOD 3, BREATHING_STEPS 128):
https://github.com/monksoffunk/qmk_firmware/blob/481b1a356341ab0c51b75dcab6ba623d77f3b483/quantum/backlight/backlight_avr.c#L427-L430

breathing_counter=  0  index=  0  val(OCRxx)=0
breathing_counter=  1  index=  0  val(OCRxx)=0
breathing_counter=  2  index=  1  val(OCRxx)=0
breathing_counter=  3  index=  1  val(OCRxx)=0
breathing_counter=  4  index=  2  val(OCRxx)=0
breathing_counter=  5  index=  2  val(OCRxx)=0
breathing_counter=  6  index=  3  val(OCRxx)=0
breathing_counter=  7  index=  3  val(OCRxx)=0
breathing_counter=  8  index=  4  val(OCRxx)=0
breathing_counter=  9  index=  4  val(OCRxx)=0
breathing_counter= 10  index=  5  val(OCRxx)=0
breathing_counter= 11  index=  5  val(OCRxx)=0
breathing_counter= 12  index=  6  val(OCRxx)=0
breathing_counter= 13  index=  6  val(OCRxx)=0
breathing_counter= 14  index=  7  val(OCRxx)=0
breathing_counter= 15  index=  7  val(OCRxx)=0
breathing_counter= 16  index=  8  val(OCRxx)=0
breathing_counter= 17  index=  8  val(OCRxx)=0
breathing_counter= 18  index=  9  val(OCRxx)=0
breathing_counter= 19  index=  9  val(OCRxx)=0
breathing_counter= 20  index= 10  val(OCRxx)=0
breathing_counter= 21  index= 10  val(OCRxx)=0
breathing_counter= 22  index= 11  val(OCRxx)=17
breathing_counter= 23  index= 11  val(OCRxx)=17
breathing_counter= 24  index= 12  val(OCRxx)=17
breathing_counter= 25  index= 12  val(OCRxx)=17
breathing_counter= 26  index= 13  val(OCRxx)=35
breathing_counter= 27  index= 13  val(OCRxx)=35
breathing_counter= 28  index= 14  val(OCRxx)=53
breathing_counter= 29  index= 14  val(OCRxx)=53
breathing_counter= 30  index= 15  val(OCRxx)=71
breathing_counter= 31  index= 15  val(OCRxx)=71
breathing_counter= 32  index= 16  val(OCRxx)=88
breathing_counter= 33  index= 16  val(OCRxx)=88
breathing_counter= 34  index= 17  val(OCRxx)=106
breathing_counter= 35  index= 17  val(OCRxx)=106
breathing_counter= 36  index= 18  val(OCRxx)=142
breathing_counter= 37  index= 18  val(OCRxx)=142
breathing_counter= 38  index= 19  val(OCRxx)=178
breathing_counter= 39  index= 19  val(OCRxx)=178
breathing_counter= 40  index= 20  val(OCRxx)=213
...
breathing_counter=221  index=110  val(OCRxx)=142
breathing_counter=222  index=111  val(OCRxx)=106
breathing_counter=223  index=111  val(OCRxx)=106
breathing_counter=224  index=112  val(OCRxx)=88
breathing_counter=225  index=112  val(OCRxx)=88
breathing_counter=226  index=113  val(OCRxx)=71
breathing_counter=227  index=113  val(OCRxx)=71
breathing_counter=228  index=114  val(OCRxx)=53
breathing_counter=229  index=114  val(OCRxx)=53
breathing_counter=230  index=115  val(OCRxx)=35
breathing_counter=231  index=115  val(OCRxx)=35
breathing_counter=232  index=116  val(OCRxx)=17
breathing_counter=233  index=116  val(OCRxx)=17
breathing_counter=234  index=117  val(OCRxx)=17
breathing_counter=235  index=117  val(OCRxx)=17
breathing_counter=236  index=118  val(OCRxx)=0
breathing_counter=237  index=118  val(OCRxx)=0
breathing_counter=238  index=119  val(OCRxx)=0
breathing_counter=239  index=119  val(OCRxx)=0
breathing_counter=240  index=120  val(OCRxx)=0
breathing_counter=241  index=120  val(OCRxx)=0
breathing_counter=242  index=121  val(OCRxx)=0
breathing_counter=243  index=121  val(OCRxx)=0
breathing_counter=244  index=122  val(OCRxx)=0
breathing_counter=245  index=122  val(OCRxx)=0
breathing_counter=246  index=123  val(OCRxx)=0
breathing_counter=247  index=123  val(OCRxx)=0
breathing_counter=248  index=124  val(OCRxx)=0
breathing_counter=249  index=124  val(OCRxx)=0
breathing_counter=250  index=125  val(OCRxx)=0
breathing_counter=251  index=125  val(OCRxx)=0
breathing_counter=252  index=126  val(OCRxx)=0
breathing_counter=253  index=126  val(OCRxx)=0
breathing_counter=254  index=127  val(OCRxx)=0
breathing_counter=255  index=127  val(OCRxx)=0
breathing_counter=256  index=  0  val(OCRxx)=0
breathing_counter=257  index=  0  val(OCRxx)=0
breathing_counter=258  index=  1  val(OCRxx)=0
breathing_counter=259  index=  1  val(OCRxx)=0
breathing_counter=260  index=  2  val(OCRxx)=0
breathing_counter=261  index=  2  val(OCRxx)=0
breathing_counter=262  index=  3  val(OCRxx)=0
breathing_counter=263  index=  3  val(OCRxx)=0
breathing_counter=264  index=  4  val(OCRxx)=0
breathing_counter=265  index=  4  val(OCRxx)=0
breathing_counter=266  index=  5  val(OCRxx)=0
breathing_counter=267  index=  5  val(OCRxx)=0
breathing_counter=268  index=  6  val(OCRxx)=0
breathing_counter=269  index=  6  val(OCRxx)=0
breathing_counter=270  index=  7  val(OCRxx)=0
breathing_counter=271  index=  7  val(OCRxx)=0
breathing_counter=272  index=  8  val(OCRxx)=0
breathing_counter=273  index=  8  val(OCRxx)=0
breathing_counter=274  index=  9  val(OCRxx)=0
breathing_counter=275  index=  9  val(OCRxx)=0
breathing_counter=276  index= 10  val(OCRxx)=0
breathing_counter=277  index= 10  val(OCRxx)=0
breathing_counter=278  index= 11  val(OCRxx)=17
breathing_counter=279  index= 11  val(OCRxx)=17
breathing_counter=280  index= 12  val(OCRxx)=17
breathing_counter=281  index= 12  val(OCRxx)=17
breathing_counter=282  index= 13  val(OCRxx)=35
breathing_counter=283  index= 13  val(OCRxx)=35
breathing_counter=284  index= 14  val(OCRxx)=53
breathing_counter=285  index= 14  val(OCRxx)=53
breathing_counter=286  index= 15  val(OCRxx)=71
breathing_counter=287  index= 15  val(OCRxx)=71
breathing_counter=288  index= 16  val(OCRxx)=88
breathing_counter=289  index= 16  val(OCRxx)=88
breathing_counter=290  index= 17  val(OCRxx)=106
breathing_counter=291  index= 17  val(OCRxx)=106
breathing_counter=292  index= 18  val(OCRxx)=142
breathing_counter=293  index= 18  val(OCRxx)=142
breathing_counter=294  index= 19  val(OCRxx)=178
breathing_counter=295  index= 19  val(OCRxx)=178
breathing_counter=296  index= 20  val(OCRxx)=213
breathing_counter=297  index= 20  val(OCRxx)=213
breathing_counter=298  index= 21  val(OCRxx)=267
breathing_counter=299  index= 21  val(OCRxx)=267
breathing_counter=300  index= 22  val(OCRxx)=303
breathing_counter=301  index= 22  val(OCRxx)=303
breathing_counter=302  index= 23  val(OCRxx)=356
breathing_counter=303  index= 23  val(OCRxx)=356
breathing_counter=304  index= 24  val(OCRxx)=428
breathing_counter=305  index= 24  val(OCRxx)=428
breathing_counter=306  index= 25  val(OCRxx)=499
breathing_counter=307  index= 25  val(OCRxx)=499
breathing_counter=308  index= 26  val(OCRxx)=571
breathing_counter=309  index= 26  val(OCRxx)=571
breathing_counter=310  index= 27  val(OCRxx)=431
breathing_counter=311  index= 27  val(OCRxx)=431
breathing_counter=312  index= 28  val(OCRxx)=685
breathing_counter=313  index= 28  val(OCRxx)=685
breathing_counter=314  index= 29  val(OCRxx)=685
breathing_counter=315  index= 29  val(OCRxx)=685
breathing_counter=316  index= 30  val(OCRxx)=685
breathing_counter=317  index= 30  val(OCRxx)=685
breathing_counter=318  index= 31  val(OCRxx)=1023
breathing_counter=319  index= 31  val(OCRxx)=1023
breathing_counter=320  index= 32  val(OCRxx)=1023
breathing_counter=321  index= 32  val(OCRxx)=1023
breathing_counter=322  index= 33  val(OCRxx)=1457
breathing_counter=323  index= 33  val(OCRxx)=1457
breathing_counter=324  index= 34  val(OCRxx)=1457
breathing_counter=325  index= 34  val(OCRxx)=1457
breathing_counter=326  index= 35  val(OCRxx)=1999
breathing_counter=327  index= 35  val(OCRxx)=1999
breathing_counter=328  index= 36  val(OCRxx)=1999
breathing_counter=329  index= 36  val(OCRxx)=1999
breathing_counter=330  index= 37  val(OCRxx)=2661
breathing_counter=331  index= 37  val(OCRxx)=2661
breathing_counter=332  index= 38  val(OCRxx)=2661
breathing_counter=333  index= 38  val(OCRxx)=2661
breathing_counter=334  index= 39  val(OCRxx)=3455
breathing_counter=335  index= 39  val(OCRxx)=3455
breathing_counter=336  index= 40  val(OCRxx)=3455
breathing_counter=337  index= 40  val(OCRxx)=3455
breathing_counter=338  index= 41  val(OCRxx)=4393
breathing_counter=339  index= 41  val(OCRxx)=4393
breathing_counter=340  index= 42  val(OCRxx)=4393
breathing_counter=341  index= 42  val(OCRxx)=4393
breathing_counter=342  index= 43  val(OCRxx)=5487
breathing_counter=343  index= 43  val(OCRxx)=5487
breathing_counter=344  index= 44  val(OCRxx)=5487
breathing_counter=345  index= 44  val(OCRxx)=5487
breathing_counter=346  index= 45  val(OCRxx)=6749
breathing_counter=347  index= 45  val(OCRxx)=6749
breathing_counter=348  index= 46  val(OCRxx)=6749
breathing_counter=349  index= 46  val(OCRxx)=6749
breathing_counter=350  index= 47  val(OCRxx)=8191
breathing_counter=351  index= 47  val(OCRxx)=8191
breathing_counter=352  index= 48  val(OCRxx)=9825
breathing_counter=353  index= 48  val(OCRxx)=9825
breathing_counter=354  index= 49  val(OCRxx)=9825
breathing_counter=355  index= 49  val(OCRxx)=9825
breathing_counter=356  index= 50  val(OCRxx)=11663
breathing_counter=357  index= 50  val(OCRxx)=11663
breathing_counter=358  index= 51  val(OCRxx)=11663
breathing_counter=359  index= 51  val(OCRxx)=11663
breathing_counter=  0  index=  0  val(OCRxx)=0
breathing_counter=  1  index=  0  val(OCRxx)=0
breathing_counter=  2  index=  1  val(OCRxx)=0
breathing_counter=  3  index=  1  val(OCRxx)=0
breathing_counter=  4  index=  2  val(OCRxx)=0
breathing_counter=  5  index=  2  val(OCRxx)=0
breathing_counter=  6  index=  3  val(OCRxx)=0
breathing_counter=  7  index=  3  val(OCRxx)=0
breathing_counter=  8  index=  4  val(OCRxx)=0
breathing_counter=  9  index=  4  val(OCRxx)=0

@monksoffunk
Copy link
Contributor Author

Well, I've got time, so let's move on.

Oh, another PR #16770 has already been merged!
It's slightly different from mine, but the LED behavior is much the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@monksoffunk