Add analog support for RP2040 #19453

sigprof · 2022-12-31T22:29:53Z

Description

Add analog support for RP2040.

The low-level driver is already present in ChibiOS-Contrib, but the fix from ChibiOS/ChibiOS-Contrib#355 is required to make it actually work; without that fix the code will compile, but adc_read() will work only once (on the second call it will lockup).

TODO:

Add RP_IRQ_ADC1_PRIORITY to the default mcuconf.h files.
Update documentation for the supported pins and driver configuration options.

Types of Changes

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

KarlK90 · 2023-01-13T10:24:50Z

With #17915 being merged to develop the ADC fixes are now available in QMK 🎉.

The ADCv2 bodge defined `ADC_CFGR1_RES_10BIT` and other similar constants, so that the same code as on STM32 could be used.

sigprof · 2023-01-15T22:34:49Z

The joystick code also builds and works with the fix from #19602.

The temperature sensor could be read with adc_read(TO_MUX(4, 0)), but the results are not really precise (the datasheet says that the precision greatly depends on the reference voltage, which in most cases is generated by some LDO not intended to be used as an analog reference). However, the sensor needs to be enabled by adcRPEnableTS(&ADCD1);, and that call works only after the ADC initialization (e.g., after a dummy adc_read()). Because the sensor has some non-zero power consumption, and probably nobody would actually want to use it, that initialization is left to the user.

keyboards/handwired/onekey/rp2040/mcuconf.h

Also change the priority from 2 to 3 to match the ChibiOS tests.

sigprof · 2023-01-16T11:12:55Z

I changed the priority to 3 to match the value used by ChibiOS tests:

lib/chibios-contrib/testhal/RP/RP2040/RT-RP2040-PICO-ADC/cfg/mcuconf.h:#define RP_IRQ_ADC1_PRIORITY                3
lib/chibios-contrib/testhal/RP/RP2040/RT-RP2040-PICO-I2C-24AA01/cfg/mcuconf.h:#define RP_IRQ_ADC1_PRIORITY                3
platforms/chibios/boards/GENERIC_PROMICRO_RP2040/configs/mcuconf.h:#define RP_IRQ_ADC1_PRIORITY                3
platforms/chibios/boards/GENERIC_RP_RP2040/configs/mcuconf.h:#define RP_IRQ_ADC1_PRIORITY                3
platforms/chibios/boards/QMK_PM2040/configs/mcuconf.h:#define RP_IRQ_ADC1_PRIORITY                3

This should still work, but I can't actually test this at the moment.

docs/adc_driver.md

Co-authored-by: Ryan <[email protected]>

tzarc · 2023-01-16T17:39:28Z

This should still work, but I can't actually test this at the moment.

Works fine.

KarlK90

Could you update the rp2040 platform dev line about the ADC driver? https://github.com/qmk/qmk_firmware/blob/master/docs/platformdev_rp2040.md#L7 The other changes LGTM.

sigprof · 2023-01-17T05:57:21Z

Also I suppose that the QMK_PM2040 board should now have RP_ADC_USE_ADC1 and HAL_USE_ADC enabled for the benefit of converters?

What about the GENERIC_PROMICRO_RP2040 board (which is apparently the default one selected for "processor": "RP2040")? Apparently that board now has RP_SIO_USE_UART0, RP_SPI_USE_SPI0, RP_I2C_USE_I2C1 enabled, but the corresponding HAL options are not enabled anywhere; it probably should have RP_ADC_USE_ADC1 enabled too?

sigprof · 2023-01-17T06:41:37Z

Looks like it is possible to get something like qmk compile -kb handwired/onekey/promicro -km adc -e CONVERT_TO=elite_pi to build, but it requires an additional change to analog.c, because the GPxx defines are not available when a converter is used:

diff --git a/platforms/chibios/drivers/analog.c b/platforms/chibios/drivers/analog.c
index 84ca99be3c..a8ce21bb6d 100644
--- a/platforms/chibios/drivers/analog.c
+++ b/platforms/chibios/drivers/analog.c
@@ -241,10 +241,10 @@ __attribute__((weak)) adc_mux pinToMux(pin_t pin) {
         // STM32F103x[C-G] in 144-pin packages also have analog inputs on F6...F10, but they are on ADC3, and the
         // ChibiOS ADC driver for STM32F1xx currently supports only ADC1, therefore these pins are not usable.
 #elif defined(RP2040)
-        case GP26: return TO_MUX(0, 0);
-        case GP27: return TO_MUX(1, 0);
-        case GP28: return TO_MUX(2, 0);
-        case GP29: return TO_MUX(3, 0);
+        case 26U: return TO_MUX(0, 0);
+        case 27U: return TO_MUX(1, 0);
+        case 28U: return TO_MUX(2, 0);
+        case 29U: return TO_MUX(3, 0);
 #endif
     }

Would this be acceptable? (If it would, then the STM32 cases above should probably be changed to use something like PAL_LINE(GPIOA, 0) too.)

sigprof · 2023-01-17T07:50:21Z

The converter support overrides _pin_defs.h with the converter version that defines only the pin names for the source MCU, so the pin names for the conversion target MCU are not available (and in general they may even conflict with the pin names for the source MCU, so there is no way to get them both). So apparently if the driver code needs to refer to some specific target MCU pins (as is the case for pinToMux() in the ADC driver), the only option to make that kind of code work with converters is to use something else instead of the QMK pin names.

Most other drivers don't have this problem, because the pin names are passed externally in defines (which, BTW, would contain the source pin names in case a converter is used), and the only places where explicit pin names are used in the driver code are some defaults which the converters override anyway.

According to docs/platformdev_rp2040.md, the `GENERIC_PROMICRO_RP2040` board should enable the peripherals that match the Sparkfun Pro Micro RP2040 pinout in `mcuconf.h`, but leave the corresponding options in `halconf.h` disabled (so the options that enable the peripherals don't actually do anything in the default state, but just enabling the options in `halconf.h` is enough to make the Pro Micro compatible peripherals work). Make the ADC feature behave the same way: enable the `RP_ADC_USE_ADC1` option in the board config, so that just enabling `HAL_USE_ADC` would be enough to use the ADC. After this change the `handwired/onekey/rp2040` keyboard no longer needs the ADC configuration in `mcuconf.h`, so it is removed again.

With this change keyboards using Pro Micro or Elite-C that were also using analog input on the F4…F7 pins could work with RP2040-based converters that map the GP26…GP29 pins to the same locations. Unfortulately, some keyboards could be using analog input on other pins (the Pro Micro pinout has 9 pins which support analog input on ATmega32U4, and the Elite-C pinout adds 2 more such pins, but RP2040 has only 4 analog pins), and those keyboards won't work with RP2040-based controllers, but that problem won't be detected at compile time (ADC reads for unsupported pins would just return 0 at runtime). The `analog.c` code needs to be changed to use raw pin numbers instead of the `GPxx` defines, because those defines are not available when using a converter.

KarlK90 · 2023-01-17T20:40:53Z

Also I suppose that the QMK_PM2040 board should now have RP_ADC_USE_ADC1 and HAL_USE_ADC enabled for the benefit of converters?

To be inline with the expectation that a converter "just works" this should be done.

What about the GENERIC_PROMICRO_RP2040 board (which is apparently the default one selected for "processor": "RP2040")? Apparently that board now has RP_SIO_USE_UART0, RP_SPI_USE_SPI0, RP_I2C_USE_I2C1 enabled, but the corresponding HAL options are not enabled anywhere; it probably should have RP_ADC_USE_ADC1 enabled too?

Yes, I think the ADC peripheral should be enabled as well without activating the HAL driver.

KarlK90 · 2023-01-17T20:42:45Z

The whole analog.c/adc handling could probably see a overhaul, but I think the way it is handled in the scope of RP2040 support is fine for now.

tzarc · 2023-01-17T20:48:38Z

The whole analog.c/adc handling could probably see a overhaul, but I think the way it is handled in the scope of RP2040 support is fine for now.

It's been ripe for replacing SRC += analog.c with ANALOG_ENABLE = yes for quite some time. Adding the defines for enabling board-specific ADCs at that point would be relatively trivial.

zvecr · 2023-01-18T00:30:35Z

From my perspective, I dont care too much if converters fail. Long term it would be expected to work, but it shouldnt be a blocker for getting ADC support in.

sigprof · 2023-01-18T06:14:17Z

Actually the converters should work with the latest code here. The only issue is that some keyboards may use analog inputs on Pro Micro/Elite-C pins outside of the F4…F7 range, and compiling that code with CONVERT_TO=some_rp2040_controller would silently give a broken firmware, because the unsupported pin usage could be detected only at runtime. But I don't see any good way around that (even if we add some analogReadPin() wrapper using __builtin_constant_p() to have a compile time check if the argument is constant, that won't help in cases when the pin is passed indirectly, such as when using the joystick feature). Maybe that's a mostly theoretical issue though (apparently all existing boards that use the joystick feature use pins from the F4…F7 range, and boards that use other pins might actually be using a custom PCB, not a replaceable controller).

KarlK90 · 2023-01-18T19:56:13Z

So this sounds like the PR is ready to go in?! The refactoring should be done in a separate PR imho.

tzarc · 2023-01-18T23:31:10Z

Can iterate on later PRs if required.

Co-authored-by: Ryan <[email protected]>

github-actions bot added core keyboard labels Dec 31, 2022

JohnAZoidberg mentioned this pull request Jan 4, 2023

Framework Laptop 16 Keyboards FrameworkComputer/qmk_firmware#1

Draft

49 tasks

sigprof added 3 commits January 16, 2023 00:07

Add analog support for RP2040

a035220

Use the ADCv2 bodge for RP2040

629094e

The ADCv2 bodge defined `ADC_CFGR1_RES_10BIT` and other similar constants, so that the same code as on STM32 could be used.

Update the ADC driver docs for RP2040

09ef8b6

sigprof force-pushed the rp2040-adc-support branch from 3f478f9 to 09ef8b6 Compare January 15, 2023 22:12

github-actions bot added the documentation label Jan 15, 2023

sigprof marked this pull request as ready for review January 15, 2023 22:15

tzarc approved these changes Jan 15, 2023

View reviewed changes

drashna requested review from KarlK90 and a team January 16, 2023 09:33

tzarc reviewed Jan 16, 2023

View reviewed changes

keyboards/handwired/onekey/rp2040/mcuconf.h Outdated Show resolved Hide resolved

Move RP_IRQ_ADC1_PRIORITY into the board configs

6094b47

Also change the priority from 2 to 3 to match the ChibiOS tests.

fauxpark requested changes Jan 16, 2023

View reviewed changes

docs/adc_driver.md Outdated Show resolved Hide resolved

docs/adc_driver.md Outdated Show resolved Hide resolved

Apply suggestions from code review

ac29805

Co-authored-by: Ryan <[email protected]>

KarlK90 approved these changes Jan 16, 2023

View reviewed changes

Mark the RP2040 ADC driver as implemented in docs

78f8ce7

sigprof added 2 commits January 17, 2023 11:23

tzarc merged commit 272281f into qmk:develop Jan 18, 2023

RephlexZero mentioned this pull request Jan 19, 2023

Update platformdev_rp2040.md #19638

Closed

omikronik pushed a commit to omikronik/qmk_firmware that referenced this pull request Jan 22, 2023

Add analog support for RP2040 (qmk#19453)

1e98364

Co-authored-by: Ryan <[email protected]>

JohnAZoidberg mentioned this pull request Feb 8, 2023

Upstream to QMK FrameworkComputer/qmk_firmware#13

Closed

9 tasks

sigprof mentioned this pull request Jun 12, 2023

Fix the analog driver for STM32-like MCUs when using a converter #21223

Draft

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analog support for RP2040 #19453

Add analog support for RP2040 #19453

sigprof commented Dec 31, 2022 •

edited

Loading

KarlK90 commented Jan 13, 2023

sigprof commented Jan 15, 2023

sigprof commented Jan 16, 2023

tzarc commented Jan 16, 2023

KarlK90 left a comment

sigprof commented Jan 17, 2023 •

edited

Loading

sigprof commented Jan 17, 2023

sigprof commented Jan 17, 2023

KarlK90 commented Jan 17, 2023

KarlK90 commented Jan 17, 2023

tzarc commented Jan 17, 2023

zvecr commented Jan 18, 2023

sigprof commented Jan 18, 2023

KarlK90 commented Jan 18, 2023

tzarc commented Jan 18, 2023

Add analog support for RP2040 #19453

Add analog support for RP2040 #19453

Conversation

sigprof commented Dec 31, 2022 • edited Loading

Description

Types of Changes

Checklist

KarlK90 commented Jan 13, 2023

sigprof commented Jan 15, 2023

sigprof commented Jan 16, 2023

tzarc commented Jan 16, 2023

KarlK90 left a comment

Choose a reason for hiding this comment

sigprof commented Jan 17, 2023 • edited Loading

sigprof commented Jan 17, 2023

sigprof commented Jan 17, 2023

KarlK90 commented Jan 17, 2023

KarlK90 commented Jan 17, 2023

tzarc commented Jan 17, 2023

zvecr commented Jan 18, 2023

sigprof commented Jan 18, 2023

KarlK90 commented Jan 18, 2023

tzarc commented Jan 18, 2023

sigprof commented Dec 31, 2022 •

edited

Loading

sigprof commented Jan 17, 2023 •

edited

Loading