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

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

Open
crasbe opened this issue Jul 8, 2024 · 6 comments · May be fixed by #20971
Open

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

crasbe opened this issue Jul 8, 2024 · 6 comments · May be fixed by #20971

Comments

@crasbe
Copy link
Contributor

crasbe commented Jul 8, 2024

Description

As discovered in #20773 (comment), some variants of the STM32 ADC peripheral use magic numbers in an undocumented way to check if the ADC resolution has a valid value or or not:
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f0_g0_c0.c#L103-L106
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f2.c#L112-L114
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f3.c#L196-L199
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f4_f7.c#L131-L134
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_l4_wb.c#L220-L223
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_wl.c#L121-L124

Some other implementations explicitly check if the resolution value is one of the valid values, so IMO this is the better approach:
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_f1.c#L146-L149
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_l0.c#L120-L126
https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/periph/adc_l1.c#L149-L155

I have the following STM32 boards available:
NUCLEO-F413ZH
NUCLEO-G474RE (not supported yet, seems to have the same ADC as the H7 series and quite similar to L4, just a little more options)
NUCLEO-L073RZ (irrelevant, won't be changed)
NUCLEO-L452RE
P-NUCLEO-WB55 (duplicate for L4)

So we're still missing F0/F0/C0, F2, F3 and WL boards for testing. Maybe I can get some of them from Mouser in the future, but maybe someone else has some of these boards for testing.

Useful links

STM32G474 Reference Manual: https://www.st.com/resource/en/datasheet/stm32g474cb.pdf p.591

@dylad dylad changed the title stm32/adc: make resultion flag checks consistent for all STM32 families stm32/adc: make resolution flag checks consistent for all STM32 families Jul 9, 2024
@krzysztof-cabaj
Copy link
Contributor

I have access to F207 and F334. I will try to arrange access to C031.

@crasbe crasbe linked a pull request Nov 9, 2024 that will close this issue
12 tasks
@crasbe
Copy link
Contributor Author

crasbe commented Nov 18, 2024

It looks like this is turning into a bit of a rabbit hole again 🫠
To test the different resolutions, I modified the tests/periph/adc test to sample once with every resolution:

...
while (1) {
        for (unsigned i = 0; i < ADC_NUMOF; i++) {
            int sample6 = adc_sample(ADC_LINE(i), ADC_RES_6BIT);
            int sample8 = adc_sample(ADC_LINE(i), ADC_RES_8BIT);
            int sample10 = adc_sample(ADC_LINE(i), ADC_RES_10BIT);
            int sample12 = adc_sample(ADC_LINE(i), ADC_RES_12BIT);
            int sample14 = adc_sample(ADC_LINE(i), ADC_RES_14BIT);
            int sample16 = adc_sample(ADC_LINE(i), ADC_RES_16BIT);

            printf("ADC_LINE(%u): %i %i %i %i %i %i\n", i, sample6, sample8, sample10, sample12, sample14, sample16);
        }
        ztimer_sleep(ZTIMER_MSEC, DELAY_MS);
    }
...

The variable for the sample has to be commented out, so it does not throw a compiler error.

- int sample = 0;
+//int sample = 0;

When I connect one of the ADC inputs to 3.3V, this is what the output looks like:

2024-11-18 11:26:53,002 # ADC_LINE(0): 4095 4095 4095 4095 -1 -1
2024-11-18 11:26:53,006 # ADC_LINE(1): 923 672 589 544 -1 -1
2024-11-18 11:26:53,010 # ADC_LINE(2): 301 150 86 48 -1 -1
2024-11-18 11:26:53,013 # ADC_LINE(3): 480 510 530 528 -1 -1
2024-11-18 11:26:53,017 # ADC_LINE(4): 496 520 542 549 -1 -1
2024-11-18 11:26:53,021 # ADC_LINE(5): 489 515 518 521 -1 -1

The two -1 at the end are expected, because the L073 does not support 14-bit and 16-bit conversions, but the other values shouldn't be all 4096 = 0b00001111.11111111, that is 12-bit resolution.

I'm not 100% sure yet what the reason is for this behavior, but it looks like the ADC configuration in cpu/stm32/periph/adc_l0.c (and potentially others as well) is incorrect. The reference manual[1] states the following on page 311, section 14.3.7:

The software must write the ADCAL and ADEN bits in the ADC_CR register and configure
the ADC_CFGR1 and ADC_CFGR2 registers only when the ADC is disabled (ADEN must
be cleared).

This is not the case in the current code, the resolution (among other things) is configured with the ADC enabled, which might lead to the ADC ignoring the configuration.


[1] https://www.st.com/resource/en/reference_manual/rm0367-ultralowpower-stm32l0x3-advanced-armbased-32bit-mcus-stmicroelectronics.pdf

@crasbe
Copy link
Contributor Author

crasbe commented Nov 18, 2024

Interestingly, for the STM32L4, the behavior is different. I have a NUCLEO-L452RE here, and this is what the output looks like:

2024-11-18 11:52:59,127 # ADC_LINE(0): 63 255 1023 4095 -1 -1
2024-11-18 11:52:59,130 # ADC_LINE(1): 24 90 364 1455 -1 -1
2024-11-18 11:52:59,133 # ADC_LINE(2): 22 89 369 1482 -1 -1
2024-11-18 11:52:59,136 # ADC_LINE(3): 22 89 368 1474 -1 -1
2024-11-18 11:52:59,139 # ADC_LINE(4): 21 89 366 1474 -1 -1
2024-11-18 11:52:59,142 # ADC_LINE(5): 21 88 363 1459 -1 -1
2024-11-18 11:52:59,145 # ADC_LINE(6): 13 46 199 792 -1 -1

As expected, the resolution was applied correctly.

The reference manual of the STM32L4 [2] page 631, section 21.4.10 explains that you're only allowed to write to the configuration when the ADC is enabled, so exactly the opposite of the L0.

For all the other control bits of the ADC_CFGR, ADC_SMPRx, ADC_SQRy, ADC_JDRy,
ADC_OFRy, ADC_OFCHRy and ADC_IER registers:
   • For control bits related to configuration of regular conversions, the software is allowed
     to write them only if the ADC is enabled (ADEN=1) and if there is no regular conversion
     ongoing (ADSTART must be equal to 0).
   • For control bits related to configuration of injected conversions, the software is allowed
     to write them only if the ADC is enabled (ADEN=1) and if there is no injected
     conversion ongoing (JADSTART must be equal to 0).

Now I only have to find out how the ADCs of the other devices behave (or should behave).


[2] https://www.st.com/resource/en/reference_manual/rm0432-stm32l4-series-advanced-armbased-32bit-mcus-stmicroelectronics.pdf

@crasbe
Copy link
Contributor Author

crasbe commented Nov 18, 2024

The test on the NUCLEO-L152RE does not run successfully at all, it runs into a hard fault after the first loop iteration:

2024-11-18 15:04:39,478 # main(): This is RIOT! (Version: 2025.01-devel-30-ge6bf3-pr/stm32_adc)
2024-11-18 15:04:39,478 # 
2024-11-18 15:04:39,481 # RIOT ADC peripheral driver test
2024-11-18 15:04:39,481 # 
2024-11-18 15:04:39,487 # This test will sample all available ADC lines once every 100ms with
2024-11-18 15:04:39,492 # a 10-bit resolution and print the sampled results to STDIO
2024-11-18 15:04:39,492 # 
2024-11-18 15:04:39,493 # 
2024-11-18 15:04:39,496 # Successfully initialized ADC_LINE(0)
2024-11-18 15:04:39,499 # Successfully initialized ADC_LINE(1)
2024-11-18 15:04:39,502 # Successfully initialized ADC_LINE(2)
2024-11-18 15:04:39,506 # Successfully initialized ADC_LINE(3)
2024-11-18 15:04:39,509 # Successfully initialized ADC_LINE(4)
2024-11-18 15:04:39,512 # Successfully initialized ADC_LINE(5)
2024-11-18 15:04:39,515 # ADC_LINE(0): 19 25 28 30 -1 -1
2024-11-18 15:04:39,518 # ADC_LINE(1): 63 63 63 63 -1 -1
2024-11-18 15:04:39,521 # ADC_LINE(2): 27 28 29 30 -1 -1
2024-11-18 15:04:39,524 # ADC_LINE(3): 24 27 28 29 -1 -1
2024-11-18 15:04:39,527 # ADC_LINE(4): 22 27 30 31 -1 -1
2024-11-18 15:04:39,530 # ADC_LINE(5): 25 29 30 31 -1 -1
2024-11-18 15:04:39,630 # 
2024-11-18 15:04:39,633 # Context before hardfault:

(Nothing is printed after the "Context before hardfault:" message.)

Furthermore the ADC seems to be stuck in 6 bit mode, I connected Pin A1 to 3.3V.

The behavior is the same on the master branch (both hard fault and 6 bit).

I'm not sure yet what the cause of the issue is, but it might have to do with the watchdog not being correctly configured. But first I'd have to check where exactly the test program crashes.


[3] https://www.st.com/resource/en/reference_manual/cd00240193-stm32l100xx-stm32l151xx-stm32l152xx-and-stm32l162xx-advanced-arm-based-32-bit-mcus-stmicroelectronics.pdf

@crasbe
Copy link
Contributor Author

crasbe commented Nov 18, 2024

Update: The L0 fix was rather easy, just move the _enable_adc() function from line 129 to 144 (after the last access to one of the config registers) in cpu/stm32/periph/adc_l0.c.


The L1 issue is not caused by the ADC, but by ztimer. Commenting out the ztimer delay gets rid of the Hard Fault.
Furthermore, the reference manual [3] has a small note about the resolution flags on page 291:
This bit must be written only when ADON=0.

However interestingly, this was not the reason why the ADC was stuck in 6-bit mode. The reason was that the resolution bits of the ADC->CR1 register were never cleared. With every operation, the resolution was only OR-ed onto the register and 6-bit has the bits 11. Therefore, after setting the resolution to 6-bit once, it was stuck there.
Likewise, a very simple fix in cpu/stm32/periph/adc_l1.c

    /* set resolution, conversion channel and single read */
+    ADC1->CR1 &= ~ADC_CR1_RES_Msk;
    ADC1->CR1 |= res & ADC_CR1_RES;
    ADC1->SQR1 &= ~ADC_SQR1_L;
    ADC1->SQR5 = adc_config[line].chan;

None of the other ADC implementations suffer from the same bug as the L1, so I'll create a PR to fix the L0 and L1 ADCs, so we can actually test #20971 🙃
The ztimer stuff is a different beast again, I'll have to look at it with a debugger tomorrow.

@crasbe
Copy link
Contributor Author

crasbe commented Nov 19, 2024

The L1 issue is not caused by the ADC, but by ztimer. Commenting out the ztimer delay gets rid of the Hard Fault.

This error is the weirdest thing. The error depends on the way the program is flashed. If you flash it with OpenOCD make flash, the microcontroller will run into a Hard Fault and will Hard Fault when you press the reset button as well.
HOWEVER if you power cycle the device, it'll run as it should.
On the other hand, when you program it using the Mass Storage programmer of the built-in ST-Link, the program runs as well.
Programming the chip with the STM32CubeProgrammer will yield the same results as OpenOCD, so the internal MSD flasher does something different than the other programming mechanisms.

So my hypothesis is, that the ztimer code for the STM32L152RE does not fully configure something which makes the program crash (and crash again) and the reset of the ST-Link is somehow "harder" than the OpenOCD reset and the reset caused by the reset button.

I'll open a separate issue about it, because that doesn't really has anything to do and I already put in way too much time...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants