[Core] PWM Backlight for RP2040 #17706

KarlK90 · 2022-07-17T12:51:04Z

Description

Decouple PWM frequency from breathing mechanism by using a virtual timer with
256Hz interrupt frequency
Make PWM frequency configurable
~~Implement non-blocking backlight pulse~~ reverted

Types of Changes

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

sigprof · 2022-07-17T16:16:57Z

quantum/backlight/backlight_chibios.c

+
+/* PWM Low Level driver specific fields */
+#ifndef pwm_lld_driver_fields
+#    define pwm_lld_driver_fields , 0, 0,


Maybe we can decide to rely on the default zero initialization for static structs and just drop this platform-dependent part?

The current initializer does not match the STM32 version of PWMConfig anyway (there are 3 extra fields there now — cr2, bdtr, dier), so we already rely on dier being implicitly initialized to 0.

Yes, that is the best option in this case I guess. Done that.

sigprof · 2022-07-17T16:27:54Z

quantum/backlight/backlight_chibios.c

@@ -100,7 +109,7 @@ void backlight_set(uint8_t level) {
    } else {
        // Turn backlight on
        uint32_t duty = (uint32_t)(cie_lightness(rescale_limit_val(0xFFFF * (uint32_t)level / BACKLIGHT_LEVELS)));
-        pwmEnableChannel(&BACKLIGHT_PWM_DRIVER, BACKLIGHT_PWM_CHANNEL - 1, PWM_FRACTION_TO_WIDTH(&BACKLIGHT_PWM_DRIVER, 0xFFFF, duty));
+        pwmEnableChannel(&BACKLIGHT_PWM_DRIVER, BACKLIGHT_PWM_CHANNEL - 1, PWM_FRACTION_TO_WIDTH(&BACKLIGHT_PWM_DRIVER, BACKLIGHT_PWM_COUNTER_FREQUENCY, duty));


This 0xFFFF should not be changed — it is actually the maximum possible value of duty, which does not depend on the underlying PWM timer frequency.

PWM_FRACTION_TO_WIDTH(pwmp, denominator, numerator) actually calculates pwmp->period * numerator / denominator, which is the pulse width that would give the numerator/denominator duty fraction.

You are right, I didn't fully understand the code here.

sigprof · 2022-07-17T16:28:18Z

quantum/backlight/backlight_chibios.c

@@ -150,7 +159,7 @@ void breathing_callback(PWMDriver *pwmp) {
    uint32_t duty                     = cie_lightness(rescale_limit_val(scale_backlight(breathing_table[index] * 256)));

    chSysLockFromISR();
-    pwmEnableChannelI(pwmp, BACKLIGHT_PWM_CHANNEL - 1, PWM_FRACTION_TO_WIDTH(&BACKLIGHT_PWM_DRIVER, 0xFFFF, duty));
+    pwmEnableChannelI(pwmp, BACKLIGHT_PWM_CHANNEL - 1, PWM_FRACTION_TO_WIDTH(&BACKLIGHT_PWM_DRIVER, BACKLIGHT_PWM_COUNTER_FREQUENCY, duty));


This 0xFFFF should also remain as it is for the same reason as above.

sigprof · 2022-07-17T16:45:28Z

quantum/backlight/backlight_chibios.c

+#ifndef BACKLIGHT_PWM_COUNTER_FREQUENCY
+#    define BACKLIGHT_PWM_COUNTER_FREQUENCY 0xFFFF
+#endif
+
+#ifndef BACKLIGHT_PWM_PERIOD
+#    define BACKLIGHT_PWM_PERIOD 256
+#endif


Making this tunable is a great feature by itself (256 Hz PWM lighting looks like crap, even the shittiest old WS2812B use ≈400 Hz, and that looks bad too). The only concern is that exposing the tunable parameters this way may not be the most understandable one, but restrictions of the actual hardware are mostly on these values, so maybe such tunables would be acceptable. (The AVR driver currently has only the BACKLIGHT_CUSTOM_RESOLUTION tunable that controls the PWM output frequency indirectly.)

Nice! Previously the advance of backlight steps was done in the PWM channel wrap callback, which of course would either slow down or speed up depending on the chosen PWM period. This is now done using a virtual timer which runs at ~256Hz so in line with the old cone and decouples the advance from the PWM settings.

sigprof · 2022-07-17T16:52:36Z

platforms/chibios/chibios_config.h

@@ -28,6 +28,11 @@
 #    define USE_GPIOV1
 #    define PAL_OUTPUT_TYPE_OPENDRAIN _Static_assert(0, "RP2040 has no Open Drain GPIO configuration, setting this is not possible");

+#    define BACKLIGHT_PAL_MODE (PAL_MODE_ALTERNATE_PWM | PAL_RP_PAD_DRIVE12 | PAL_RP_GPIO_OE)
+#    define BACKLIGHT_PWM_COUNTER_FREQUENCY 490196


Maybe write this as 125000000/255 instead of using a strange number of unknown origin?

Although the datasheet says that dividing by 256 should be supported too, so this might be 125000000/256 = 488281 (with the 0.25 remainder truncated). Or maybe just use 1000000, which should be obtainable precisely even if someone decides to run the chip on another frequency than the maximum 125 MHz, and would give a slightly better brightness resolution (3906 steps at 256 Hz output frequency instead of 1914).

Good point, I changed it to use the suggested 1000000 value.

KarlK90 · 2022-07-17T17:51:58Z

@sigprof That is some great feedback, I have incorporated your suggestions and will answer tomorrow in greater detail.

zvecr · 2022-07-27T21:18:31Z

quantum/backlight/backlight_chibios.c

    chSysUnlockFromISR();
 }

-// TODO: integrate generic pulse solution
 void breathing_pulse(void) {


The code that was here was here was mostly intentional, avoiding previous race conditions. This was mostly on the list of things to remove so im not keen on its reintroduction.

You mean removing the backlight pulse feature as is? I can revert it if you want.

I reverted this to the blocking wait.

zvecr · 2022-07-27T21:20:34Z

platforms/chibios/vendors/RP/_pin_defs.h

+
+#define RP2040_PWM_CHANNEL_A 1
+#define RP2040_PWM_CHANNEL_B 2


Probably shouldnt exist in pin defs, as its not coupled to GPIO

Suggested change

#define RP2040_PWM_CHANNEL_A 1

#define RP2040_PWM_CHANNEL_B 2

The channels are coupled to the GPIOs every pin can only be one at a time, but yes it could probably live in chibios_config.h for the time being?

After revisiting this I still think that _pin_defs.h while not optimal is the better option over e.g. chibios_config.h

* Decouple PWM frequency from breathing mechanism by using a virtual timer with 256Hz interrupt frequency * Make PWM frequency configurable * Drive by correct PAL mode for STM32F4xx onekeys

KarlK90 · 2022-10-04T21:08:24Z

Tested this on RP2040 and STM32F401 Blackpill - also applied a fix to use the correct alternate function. Therefore I'm merging.

qmk/qmk_firmware#17706

github-actions bot added core keyboard labels Jul 17, 2022

KarlK90 force-pushed the feature/rp2040-pwm-driver branch from c38247f to cd4baea Compare July 17, 2022 12:52

sigprof requested changes Jul 17, 2022

View reviewed changes

KarlK90 force-pushed the feature/rp2040-pwm-driver branch 2 times, most recently from 77bf97f to 3f13520 Compare July 17, 2022 17:44

KarlK90 force-pushed the feature/rp2040-pwm-driver branch 2 times, most recently from 4ffd3b9 to 5355630 Compare July 18, 2022 15:44

KarlK90 mentioned this pull request Jul 19, 2022

[Core] Adjust PWM hardware audio driver for RP2040 #17723

Merged

14 tasks

KarlK90 force-pushed the feature/rp2040-pwm-driver branch 2 times, most recently from 05234bd to b43f0f3 Compare July 27, 2022 17:58

KarlK90 marked this pull request as ready for review July 27, 2022 17:58

KarlK90 requested review from sigprof and a team July 27, 2022 17:58

KarlK90 force-pushed the feature/rp2040-pwm-driver branch 2 times, most recently from ee15630 to 2723475 Compare July 27, 2022 21:31

zvecr requested changes Jul 27, 2022

View reviewed changes

KarlK90 force-pushed the feature/rp2040-pwm-driver branch from 2723475 to 5fb5d63 Compare July 27, 2022 21:33

github-actions bot added the documentation label Jul 27, 2022

tzarc requested a review from zvecr August 13, 2022 13:35

tzarc added the breaking_change_2022q3 label Aug 13, 2022

tzarc added breaking_change_2022q4 and removed breaking_change_2022q3 labels Aug 25, 2022

KarlK90 force-pushed the feature/rp2040-pwm-driver branch from 5fb5d63 to 9d5fb48 Compare September 30, 2022 10:37

tzarc approved these changes Oct 3, 2022

View reviewed changes

KarlK90 added the hacktoberfest-accepted All the good issues to tackle during Hacktoberfest! label Oct 3, 2022

Adjust Backlight PWM implementation for RP2040

20ca1e0

* Decouple PWM frequency from breathing mechanism by using a virtual timer with 256Hz interrupt frequency * Make PWM frequency configurable * Drive by correct PAL mode for STM32F4xx onekeys

KarlK90 force-pushed the feature/rp2040-pwm-driver branch from 9d5fb48 to 20ca1e0 Compare October 3, 2022 17:38

KarlK90 removed the hacktoberfest-accepted All the good issues to tackle during Hacktoberfest! label Oct 3, 2022

KarlK90 merged commit 996a900 into qmk:develop Oct 4, 2022

KarlK90 deleted the feature/rp2040-pwm-driver branch October 4, 2022 21:10

ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022

[Core] PWM Backlight for RP2040 (qmk#17706)

7f7bb3d

eerraa added a commit to eerraa/vial-qmk that referenced this pull request Jan 20, 2023

PWM Backlight for RP2040

e02bd08

qmk/qmk_firmware#17706

eerraa mentioned this pull request Jan 20, 2023

PWM Backlight for ERA65(RP2040) vial-kb/vial-qmk#343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] PWM Backlight for RP2040 #17706

[Core] PWM Backlight for RP2040 #17706

KarlK90 commented Jul 17, 2022 •

edited

sigprof Jul 17, 2022

KarlK90 Jul 18, 2022

sigprof Jul 17, 2022

KarlK90 Jul 18, 2022

sigprof Jul 17, 2022

KarlK90 Jul 18, 2022

sigprof Jul 17, 2022

KarlK90 Jul 18, 2022

sigprof Jul 17, 2022

KarlK90 Jul 18, 2022

KarlK90 commented Jul 17, 2022

zvecr Jul 27, 2022

KarlK90 Jul 27, 2022 •

edited

KarlK90 Sep 30, 2022

zvecr Jul 27, 2022

KarlK90 Jul 27, 2022

KarlK90 Sep 30, 2022

KarlK90 commented Oct 4, 2022


		#define RP2040_PWM_CHANNEL_A 1
		#define RP2040_PWM_CHANNEL_B 2

[Core] PWM Backlight for RP2040 #17706

[Core] PWM Backlight for RP2040 #17706

Conversation

KarlK90 commented Jul 17, 2022 • edited

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlK90 commented Jul 17, 2022

Choose a reason for hiding this comment

KarlK90 Jul 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarlK90 commented Oct 4, 2022

KarlK90 commented Jul 17, 2022 •

edited

KarlK90 Jul 27, 2022 •

edited