Skip to content
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/stm32l0,l1: Fix ADC initialization order #21011

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Nov 19, 2024

Contribution description

Most of this PR is already described in #20780.

The STM32L0 was enabled before the resolution bits were set, but any access to configuration registers is prohibited (and seemingly ignored by the microcontroller). Therefore, the resolution set in adc_sample was not actually applied and the resolution was always 12-bit.

The same issue was present in the STM32L1, however this ADC hardware implementation does not seem to ignore configuration register calls (even though they are prohibited when the ADC is on). However, the resolution bits were not masked in the configuration register before setting the new resolution, leading to the resolution being stuck at 6-bit (0b11). The resolution was only OR-ed to the register.

Testing procedure

For convenience you can change the test application (tests/periph/adc) to print all resolution values at once, as described in #20780 (comment).

Due to the issue described in #21010, I recommend flashing the NUCLEO-L152RE board with the Mass Storage Driver of the ST-Link or doing a powercycle (unplug and plug the USB cable back in) after flashing. This issue is not related to these changes.

You can connect one of the A0 to A5 pins of the Nucleo to +3.3V and observe the output.
tl;dr: The output should look something like this, when the channel is maxed out on all implemented bit sizes. The two -1 at the end show that the 14-bit and 16-bit resolution is not implemented.

expected:
ADC_LINE(5): 63 255 1023 4095 -1 -1

actual:
ADC_LINE(5): 4095 4095 4095 4095 -1 -1 (NUCLEO-L073RZ)
ADC_LINE(5): 63 63 63 63 -1 -1 (NUCLEO-L152RE)

Nucleo-L073RZ Master:

main(): This is RIOT! (Version: 2025.01-devel-29-g76b9b)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
a 10-bit resolution and print the sampled results to STDIO


Successfully initialized ADC_LINE(0)
Successfully initialized ADC_LINE(1)
Successfully initialized ADC_LINE(2)
Successfully initialized ADC_LINE(3)
Successfully initialized ADC_LINE(4)
Successfully initialized ADC_LINE(5)
ADC_LINE(0): 73 32 14 6 -1 -1
ADC_LINE(1): 515 526 530 530 -1 -1
ADC_LINE(2): 325 152 74 39 -1 -1
ADC_LINE(3): 476 518 524 531 -1 -1
ADC_LINE(4): 482 514 537 536 -1 -1
ADC_LINE(5): 4095 4095 4095 4095 -1 -1

Nucleo-L073RZ this PR:

main(): This is RIOT! (Version: 2025.01-devel-31-g5de25c-pr/stm32l0_adc_fix)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
a 10-bit resolution and print the sampled results to STDIO


Successfully initialized ADC_LINE(0)
Successfully initialized ADC_LINE(1)
Successfully initialized ADC_LINE(2)
Successfully initialized ADC_LINE(3)
Successfully initialized ADC_LINE(4)
Successfully initialized ADC_LINE(5)
ADC_LINE(0): 1 2 4 8 -1 -1
ADC_LINE(1): 7 32 133 522 -1 -1
ADC_LINE(2): 5 10 19 52 -1 -1
ADC_LINE(3): 7 32 129 528 -1 -1
ADC_LINE(4): 7 32 132 537 -1 -1
ADC_LINE(5): 63 255 1023 4095 -1 -1

Nucleo-L152RE Master:

main(): This is RIOT! (Version: 2025.01-devel-29-g76b9b)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
a 10-bit resolution and print the sampled results to STDIO


Successfully initialized ADC_LINE(0)
Successfully initialized ADC_LINE(1)
Successfully initialized ADC_LINE(2)
Successfully initialized ADC_LINE(3)
Successfully initialized ADC_LINE(4)
Successfully initialized ADC_LINE(5)
ADC_LINE(0): 24 27 29 30 -1 -1
ADC_LINE(1): 25 29 30 31 -1 -1
ADC_LINE(2): 21 24 26 28 -1 -1
ADC_LINE(3): 23 26 28 29 -1 -1
ADC_LINE(4): 24 28 30 31 -1 -1
ADC_LINE(5): 63 63 63 63 -1 -1

Nucleo-L152RE with the fixes applied:

main(): This is RIOT! (Version: 2025.01-devel-31-g5de25c-pr/stm32l0_adc_fix)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
a 10-bit resolution and print the sampled results to STDIO


Successfully initialized ADC_LINE(0)
Successfully initialized ADC_LINE(1)
Successfully initialized ADC_LINE(2)
Successfully initialized ADC_LINE(3)
Successfully initialized ADC_LINE(4)
Successfully initialized ADC_LINE(5)
ADC_LINE(0): 23 110 473 1967 -1 -1
ADC_LINE(1): 28 123 503 2039 -1 -1
ADC_LINE(2): 21 98 423 1813 -1 -1
ADC_LINE(3): 22 103 447 1879 -1 -1
ADC_LINE(4): 22 111 481 1999 -1 -1
ADC_LINE(5): 63 255 1023 4095 -1 -1

Issues/PRs references

This PR is a requirement for #20971, otherwise the changes in that PR can't be tested.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Nov 19, 2024
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Nov 19, 2024
@benpicco benpicco requested a review from maribu November 27, 2024 12:21
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 27, 2024
Copy link
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.

Looks goood to me!

cpu/stm32/periph/adc_l1.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

Please squash directly - I trust your testing.

@crasbe crasbe force-pushed the pr/stm32l0_adc_fix branch from c70b776 to 0485824 Compare November 27, 2024 14:52
@riot-ci
Copy link

riot-ci commented Nov 27, 2024

Murdock results

FAILED

17ee40d cpu/stm32l1: fix ADC initialization & resolution setting

Artifacts

cpu/stm32/periph/adc_l1.c Outdated Show resolved Hide resolved
@kfessel
Copy link
Contributor

kfessel commented Dec 10, 2024

@crasbe: if you provide test instruction it is nice if you tell what test you are taking about ( added that info in your message)

I usually just include the command-line e.g.:

<RIOT>/tests/periph/adc/ > BOARD=nucleo-l073rz make flash term 

other people use the make -D <dir>

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I was a bit confused by ADC_<something> vs ADC_<something>_Msk seem like one is alias for the other e.g.:

cmsis/l1/Include/stm32l152xd.h
1000:#define ADC_CR1_RES_Msk                      (0x3UL << ADC_CR1_RES_Pos)         /*!< 0x03000000 */
1001:#define ADC_CR1_RES                          ADC_CR1_RES_Msk  

using only one ( I would prefer _Msk, i think these files before the pr prefer the one without ) would improve readability

cpu/stm32/periph/adc_l1.c Outdated Show resolved Hide resolved
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

the two marked snipetes use msk an no msk in opposite roles

testing and (un)setting

seems like ARM CMSIS also prefers _Pos and _Msk (in their periph example(s) is no define without)

https://arm-software.github.io/CMSIS_6/latest/Core/group__peripheral__gr.html
https://arm-software.github.io/CMSIS_5/Core_A/html/group__peripheral__gr.html#ga139b6e261c981f014f386927ca4a8444

cpu/stm32/periph/adc_l1.c Outdated Show resolved Hide resolved
cpu/stm32/periph/adc_l1.c Outdated Show resolved Hide resolved
@crasbe
Copy link
Contributor Author

crasbe commented Dec 11, 2024

I just looked at the other code in the file and it does not use _Msk at all, only the code I introduced.
For consistency it would probably be best to not use the _Msk definitions at all then.

Likewise for the additions in my other PR #20971. Only the code introduced by me uses the _Msk definition.

@kfessel
Copy link
Contributor

kfessel commented Dec 11, 2024

I just looked at the other code in the file and it does not use _Msk at all, only the code I introduced. For consistency it would probably be best to not use the _Msk definitions at all then.

Likewise for the additions in my other PR #20971. Only the code introduced by me uses the _Msk definition.

I think either is ok (I would just prefer that there is one), diverting from cmsis isn't a problem as long a the manufacturer keeps doing that (microchip just removed their extra (as in: I could not find them in the cmsis manual) bitfield structs)

to me the _Msk variant is a little more telling what it actually is, but the current file make little to no use of _Msk.

i might have missed something in cmsis that defines the non _Msk values but i did not see it

@crasbe
Copy link
Contributor Author

crasbe commented Dec 13, 2024

@kfessel Can I resolve the conversations and squash the fixups?

@kfessel
Copy link
Contributor

kfessel commented Dec 13, 2024

@kfessel Can I resolve the conversations and squash the fixups?

yes

@crasbe crasbe force-pushed the pr/stm32l0_adc_fix branch from a8acff0 to 17ee40d Compare December 13, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants