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/stm32wl: Replace ztimer with busy_wait and fix initialization sequence #21238

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

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Feb 21, 2025

Contribution description

This PR fixes two issues with the ADC of the STM32WL55.

  1. The ADC initialization routine used ztimer for an uncritical delay, which caused issues (a crash) when it was called from the early reset vector to check the battery status. I explained it here in greater detail: boards/nucleo-wl55jc: add ADC configuration #21235 (comment)
    Using ztimer for an uncritical delay is not really necessary, especially since the intended delay is 20µs and if only ZTIMER_MSEC was available instead of ZTIMER_USEC, the delay was extended to 1ms.
    I replaced it with busy_wait and added a factor of 2, because busy_wait is not really accurate. We can wait longer, but we shouldn't push our luck by waiting shorter.

  2. Similar to the L0, L1 and F0/G0/C0 ADC implementations, the initialization order was incorrect. The resolution setting will be ignored when the ADC is already enabled, as described in the Errata [1].
    I adapted the _adc_enable and _adc_disable functions from PR cpu/stm32{f0,g0,c0}: fix ADC initialization sequence #21230. At this moment it is a bit pointless to have _disable_adc return something, but in the future someone (maybe me) wants to add a timeout to the somewhat dangerous infinite while loops and then it makes sense to have the return in place already.
    Also I changed the is the ADC already initialized?-test from checked the ADEN bit to checking if the sampling time has already been set.
    This should work without issues since the reset value for this register is 0.

  3. (No fix but an improvement) Similar to the F0/G0/C0 PR from cpu/stm32{f0,g0,c0}: fix ADC initialization sequence #21230, I added the RS-register guard to avoid setting or resetting a flag that shouldn't be touched.
    The issue here is that sometimes a flag is set by writing a 1 to it (typical behavior) and sometimes it is reset by writing a 1 to it. A typical RMW-cycle could potentially reset a flag, which is why the original STM32-HAL uses these guards.
    The older documentation does not say that this is required, but it can't hurt. The initialization is not really time critical so we don't have to save cycles.

Testing procedure

The testing procedure is a bit of a hen-egg-problem, because for some reason the Nucleo-WL55 did not have the ADC lines configured, so PR #21235 has to be applied as well as this one. I recommend checking out PR #21235 and manually copying cpu/stm32/periph/adc_wl.c from this PR.

The first part of this PR can be easily tested with tests/periph/vbat with BOARD=nucleo-wl55jc make -C tests/periph/vbat flash term.

With current master:

0x8001741 => *** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

With this PR:

main(): This is RIOT! (Version: 2025.04-devel-142-g04340-nucleo-wl55jc-ADC)

RIOT backup battery monitoring test

This test will sample the backup battery once a second


VBAT: 386[mV]
VBAT: 1598[mV]
VBAT: 1658[mV]
VBAT: 1670[mV]
VBAT: 1668[mV]
VBAT: 1668[mV]
VBAT: 1670[mV]
VBAT: 1665[mV]
...

The second part of this PR can be tested with tests/periph/adc with with BOARD=nucleo-wl55jc make -C tests/periph/adc flash term.

With master:

main(): This is RIOT! (Version: 2025.04-devel-142-g04340-nucleo-wl55jc-ADC)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
6 to 16-bit resolution and print the sampled results to STDOUT.
Not all MCUs support all resolutions, unsupported resolutions
are printed as -1.

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)
Successfully initialized ADC_LINE(6)
ADC_LINE(0): 4095 4095 4095 4094    -1    -1
ADC_LINE(1): 1675 1572 1554 1550    -1    -1
ADC_LINE(2): 1335 1503 1542 1561    -1    -1
ADC_LINE(3): 2086 1902 1750 1703    -1    -1
ADC_LINE(4): 1483 1533 1546 1558    -1    -1
ADC_LINE(5): 1473 1526 1538 1550    -1    -1
ADC_LINE(6): 440 156   39    0    -1    -1

ADC_LINE(0): 4095 4095 4095 4094    -1    -1
ADC_LINE(1): 1834 1609 1560 1551    -1    -1
ADC_LINE(2): 1457 1535 1551 1563    -1    -1
ADC_LINE(3): 1663 1674 1679 1680    -1    -1
ADC_LINE(4): 1543 1557 1560 1563    -1    -1
ADC_LINE(5): 1523 1539 1551 1553    -1    -1
ADC_LINE(6): 423 143   34    0    -1    -1

With this PR:

main(): This is RIOT! (Version: 2025.04-devel-142-g04340-nucleo-wl55jc-ADC)

RIOT ADC peripheral driver test

This test will sample all available ADC lines once every 100ms with
6 to 16-bit resolution and print the sampled results to STDOUT.
Not all MCUs support all resolutions, unsupported resolutions
are printed as -1.

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)
Successfully initialized ADC_LINE(6)
ADC_LINE(0): 19  85  348 1399    -1    -1
ADC_LINE(1): 19  84  340 1362    -1    -1
ADC_LINE(2): 20  86  347 1393    -1    -1
ADC_LINE(3): 32 104  394 1550    -1    -1
ADC_LINE(4): 21  86  347 1390    -1    -1
ADC_LINE(5): 63 255 1023 4095    -1    -1
ADC_LINE(6): 18  70  274 1075    -1    -1

ADC_LINE(0): 20  86  349 1400    -1    -1
ADC_LINE(1): 20  84  340 1364    -1    -1
ADC_LINE(2): 20  86  347 1393    -1    -1
ADC_LINE(3): 23  95  384 1538    -1    -1
ADC_LINE(4): 21  86  346 1390    -1    -1
ADC_LINE(5): 63 255 1023 4095    -1    -1
ADC_LINE(6): 18  70  275 1074    -1    -1

Issues/PRs references

This has to be merged before #20971 can be merged (and tested...).

See also

[1] https://www.st.com/resource/en/errata_sheet/es0500-stm32wl55xx-stm32wl54xx-device-errata-stmicroelectronics.pdf p.13 section 2.3.2

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Feb 21, 2025
@crasbe
Copy link
Contributor Author

crasbe commented Feb 21, 2025

(Totally unrelated: I still don't love git.)

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 Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant