Made AVR backlight pwm resolution configurable

[Duckle29]

Description

This PR aims to make hardware PWM resolution configurable, allowing higher PWM frequencies at lower resolutions.

Currently, 16bit timers use the full 16bit resolution, resulting in a ~244Hz frequency for the backlight PWM. This would be noticeable in the peripheral vision if moving quickly / turning your head quickly. Additionally, with most keyboards having very few brightness levels, this resolution is wasted.

For now, making the top of the counter optionally configurable using #define BACKLIGHT_CUSTOM_RESOLUTION = <custom resolution> should offer configurability for users that find 244Hz too slow.

In order to achieve this, some things have been changed slightly.

The cie_lightness function was hard-coded to assume the max value was 65535.
In order to avoid floating point arithmetic, I replaced the hard-coded 8% and 16% values of that with division with the nearest integer to the reciprocal of the respective percentages. x/12 for ~8% (8.333...%) and x/6 for ~16% (16.666...%)

No change default behavior should be as it was, except for the minor differences in percentages.

Types of Changes

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

Checklist

• My code follows the code style of this project.
• 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).

[Duckle29]

Currently, I can't test these changes myself, so I need help with that. There's a good chance I messed up some of the math when changing from the hard-coded values.

[dotleon]

With BACKLIGHT_CUSTOM_RESOLUTION set to 65535, the maximum brightness is right, but breathing doesn't work as intended, it keeps the 6 second default time but the leds are off for one second then switch to like 5% of brightness for 5 seconds.

With the resolution set to 4096, the pwm frequency measurably increases, but the maximum brightness is very low, maybe around 10%.
Breathing changes into this fast (<1s) repeating strange flickering pattern.

[Duckle29]

@dotleon Hey, and thanks for testing it. Would you be up for testing the newest push?
You'd have to add #define BACKLIGHT_CUSTOM_RESOLUTION 0xFFF to your config.h :)

[dotleon]

Maximum brightness is good, scaling is working properly, and pwm frequency increases as intended.
Breathing is unchanged https://gfycat.com/jealoussociablehawaiianmonkseal

[yanfali]

Maximum brightness is good, scaling is working properly, and pwm frequency increases as intended.
Breathing is unchanged https://gfycat.com/jealoussociablehawaiianmonkseal

that's epilepsy inducing.

[fauxpark]

The breathing ISR still seems to be assuming a 244Hz frequency:

qmk_firmware/quantum/backlight/backlight_avr.c

Line 382 in e4dd9e1

breathing_counter = breathing_period * 244 / 2; \

qmk_firmware/quantum/backlight/backlight_avr.c

Lines 448 to 450 in e4dd9e1

	uint16_t interval = (uint16_t)breathing_period * 244 / BREATHING_STEPS;
	// resetting after one period to prevent ugly reset at overflow.
	breathing_counter = (breathing_counter + 1) % (breathing_period * 244);

[Duckle29]

Yeah that seems to be what it was :)

@dotleon This should let breathing work too. Was the shimmering of the LEDs a camera artifact, or was that also observed by eye? Ignoring the way too fast breathing, the LEDs seemed to be shimmering a bit in the video you posted

[dotleon]

@Duckle29 the shimmering was not a camera artifact, I think something overflows, I've experimented a little yesterday and here are my findings:

• even when the resolution is set to 0xFFFF (so in theory what it was before the changes) the breathing doesn't work correctly, it seems to be smooth to 50% of brightness then above that it starts to "shimmer"

• If I understand correctly, the breathing ISR runs every time the timer resets, so 16MHz/ICRx, won't it has to run too many times below a certain resolution? Maybe you should set a lower cap of 0xFF for resolution, but maybe that's still too high of a pwm.
  There's no need to run breathing that fast so the best solution would be to run it at a fixed rate

• breathing_counter overflows pretty fast at lower resolutions like 16Mhz/0xFF*breathing_period, this wouldn't be an issue if breathing would run at a lower pwm.
  Something overflows at 0xFFF too, I couldn't figure out what, but the way it behaves it clearly looks like it does. I've casted a bunch of stuff in the ISR to uint32_t and that fixed it.

• Lowest breathing period is 1 sec, and breathing has 128 steps, so I guess there is no point in running breathing task more than 128 times a second.

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[drashna]

Some merge conflicts here.

[Duckle29]

While the conflict has been resolved, I don't feel comfortable merging this for a few reasons.

Lack of hardware to test on and as such a lack of confidence in the change.
I'd rather wait for #8041 to merge, and for me to get some test hardware (working on an all in one test board. I think) and then re-do this.

[tzarc]

Retargeted to develop, put it "on hold" for now.