Skip to content

cpu/stm32: Make ADC Resolution Definition uniform#20971

Merged
crasbe merged 1 commit intoRIOT-OS:masterfrom
crasbe:pr/stm32_adc
Apr 3, 2025
Merged

cpu/stm32: Make ADC Resolution Definition uniform#20971
crasbe merged 1 commit intoRIOT-OS:masterfrom
crasbe:pr/stm32_adc

Conversation

@crasbe
Copy link
Copy Markdown
Contributor

@crasbe crasbe commented Nov 9, 2024 •
edited
Loading

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 (in cpu/stm32/periph/adc_xxx.c ).

The c0 and g0 did not define the adc_res_t structure at all, so the generic fallback from drivers/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 is 00 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?

  • C0 Series tested
  • F0 Series tested
  • F2 Series tested
  • F3 Series tested
  • F4 Series tested
  • F7 Series tested
  • G0 Series tested
  • L0 Series tested
  • L1 Series tested
  • L4 Series tested
  • WB Series tested
  • WL Series tested

Issues/PRs references

This closes #20780.

Depends on #21011 - merged.
Depends on #21230 - merged.
Depends on #21238 - merged.
Depends on #21346.

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

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Nov 9, 2024
krzysztof-cabaj
krzysztof-cabaj reviewed Nov 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@krzysztof-cabaj krzysztof-cabaj left a 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.

@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Nov 18, 2024

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.

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Nov 18, 2024

Are you planning to merge the sprawling STM32 ADC driver implementations into one? 😃

@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Nov 18, 2024

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.
Even worse, the STM32WB0x line introduces yet another ADC flavor, so another implementation will be added.

While you could merge all the files into one, but it would be a big #ifdef party that nobody can handle anymore 😅

This was referenced Nov 18, 2024
Closed
Merged
@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Dec 11, 2024

Similarly to the discussion in #21011, I changed the definitions used in the resolution check from the ones with _Msk suffix to the "normal" definition that is used everywhere else in the code.

@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Jan 16, 2025 •
edited
Loading

Now that PR #21011 is merged, I can confirm that this PR does work as it should with the L0 and L1 series now.
The boards used for testing were the NUCLEO-L073RZ and NUCLEO-L152RE.

Edit: I hope that I can get more Nucleos for testing soon.

@crasbe crasbe mentioned this pull request Feb 18, 2025
Closed
@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Feb 20, 2025

I can confirm that it works with:

Furthermore I have the following Nucleos which should be supported, but don't have the feature defined nor the pins mapped:

  • nucleo-f722zg
  • nucleo-wl55jc (it's a Nucleo-WL55JC2, but JC1 and JC2 only differ in the radio frequency)

@krzysztof-cabaj krzysztof-cabaj mentioned this pull request Feb 20, 2025
Merged
@krzysztof-cabaj
Copy link
Copy Markdown
Contributor

krzysztof-cabaj commented Feb 20, 2025

@crasbe

I add support for nucleo-f722ze - but only looking at documentation. Could you test it as you have access to this boards?

@krzysztof-cabaj krzysztof-cabaj mentioned this pull request Feb 21, 2025
Merged
This was referenced Feb 21, 2025
Merged
Merged
@crasbe crasbe mentioned this pull request Apr 2, 2025
Merged
@crasbe crasbe added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 2, 2025
@crasbe crasbe force-pushed the pr/stm32_adc branch 2 times, most recently from 474a553 to 805983d Compare April 2, 2025 19:54
@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Apr 2, 2025

Now that the last PR is in, this PR got rebased and I retested everything with everything I have here at home:

  • nucleo-c031c6 (has no vbat) (with PROGRAMMER=cpy2remed, because OpenOCD v0.11.0 does not support the C0 series yet)

  • nucleo-f030r8 (has no vbat)

  • nucleo-f207zg with periph_vbat (with cpu/stm32f2: Fix Sampling Time for VBat #21346)

  • nucleo-f302r8 with periph_vbat

  • nucleo-f722ze with periph_vbat

  • nucleo-g071rb with periph_vbat

  • nucleo-wb55jc with periph_vbat

  • p-nucleo-wb55 with periph_vbat

@riot-ci
Copy link
Copy Markdown

riot-ci commented Apr 2, 2025 •
edited
Loading

Murdock results

✔️ PASSED

1d3939d cpu/stm32: make ADC resolution uniform

Success Failures Total Runtime
10279 0 10280 08m:38s

Artifacts

benpicco
benpicco approved these changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One step towards a unified ADC driver for STM32!

@crasbe crasbe enabled auto-merge April 2, 2025 21:08
@crasbe crasbe added this pull request to the merge queue Apr 2, 2025
Merged via the queue into RIOT-OS:master with commit 8a76cee Apr 3, 2025
25 checks passed
@crasbe crasbe deleted the pr/stm32_adc branch April 3, 2025 08:22
@crasbe
Copy link
Copy Markdown
Contributor Author

crasbe commented Apr 3, 2025

Thanks everyone for helping me on this journey :)

@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

+1 more reviewer

@krzysztof-cabaj krzysztof-cabaj krzysztof-cabaj left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2025.04

Development

Successfully merging this pull request may close these issues.

stm32/adc: make resolution flag checks consistent for all STM32 families

5 participants

@crasbe @benpicco @krzysztof-cabaj @riot-ci @mguetschow