-
Notifications
You must be signed in to change notification settings - Fork 2k
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
cpu/stm32: Make ADC Resolution Definition uniform #20971
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crasbe
I tested your PR using tests/periph/adc
program using 8, 10 and 12 bit resolution.
The code works fine for these boards:
nucleo-f207zg
,nucleo-f334r8
,nucleo-f439zi
,nucleo-l476rg
.
As described in #20780 (comment), this PR requires some additional work, at least for the L0 series. The resolution setting is ignored due to a incorrect configuration sequence of the ADC registers. Perhaps I'll create another PR to fix that before this one can be properly tested. |
Are you planning to merge the sprawling STM32 ADC driver implementations into one? 😃 |
I don't think that would be a good idea, the ADCs are actually pretty different between the microcontrollers and the current implementations reflect all the different flavors. While you could merge all the files into one, but it would be a big |
Similarly to the discussion in #21011, I changed the definitions used in the resolution check from the ones with |
Contribution description
This PR is based on #20780.
The different implementations for the STM32 ADCs had wildly different styles of defining (in
cpu/stm32/include/periph/xx/periph_cpu.h
) and checking the resolution flags (incpu/stm32/periph/adc_xxx.c
).The c0 and g0 did not define the
adc_res_t
structure at all, so the generic fallback fromdrivers/include/periph/adc.h
was used. However it appears that this did not set the right flags, because for example the value for 6-bit resolution from the generic structure is00
whereas it should be(0x3 << 3)
. Even worse, for higher resolutions, the DMA bits would be set instead of the resolution bits.I did not check it yet, but it seems like the c0 and g0 STM32s were always stuck in 12-bit mode with the current code.
This PR makes all
adc_res_t
structure definitions uniformly, using the official register macros instead of magic numbers. Furthermore, the resolution check uses the official register mask for evaluating the resolution.(Note: The F1 series only supports 12-bit mode, so nothing was changed here. There are no registers to be set, so the generic structure works for it.)
Testing procedure
I did not test this PR yet, because I don't have any Nucleos on hand until next week, but the
tests/periph/adc
test should be working on all affected Nucleos now.@krzysztof-cabaj if you have time, you could look into testing this PR with some Nucleos that I don't have?
Issues/PRs references
This closes #20780.
STM32C0x1 Reference Manual: https://www.st.com/resource/en/reference_manual/rm0490-stm32c0x1-advanced-armbased-32bit-mcus-stmicroelectronics.pdf p.215
STM32F205, 207, 215, 217 Reference Manual: https://www.st.com/resource/en/reference_manual/rm0033-stm32f205xx-stm32f207xx-stm32f215xx-and-stm32f217xx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf p.240
STM32G0x0 Reference Manual: https://www.st.com/resource/en/reference_manual/rm0454-stm32g0x0-advanced-armbased-32bit-mcus-stmicroelectronics.pdf p.320
STM32G0x1 Reference Manual: https://www.st.com/resource/en/reference_manual/dm00371828-stm32g0x1-advanced-arm-based-32-bit-mcus-stmicroelectronics.pdf The resolution bits are defined on p.389