STM32 ADCv4 Driver in Dual Mode

[jawe]

During the use of STM32 ADCv4 driver I encountered a few bugs

Codebase used: recent Github version (git-svn-id: svn://svn.code.sf.net/p/chibios/svn/trunk@16557 27425a3e-05d8-49a3-a47f-9c15f0e5edd8)
Compiler: gcc version 11.3.1 20220712 (Arm GNU Toolchain 11.3.Rel1)
Platform: STM32H745, custom board

Below is a list of bugs I found, in the attachment are patches numbered the same way with the fix for each bug.

Bug 1:
When DUAL_MODE is enabled driver enforces 32 bit sample size. Implementation does only exist for 16 / 8 bit size. Possibly mistake because the DMA in fact does 32 bit transfers in this mode, but that transfer does contain 2 samples of 16 bit each (or 2x 8 for 16 bit total).
Fix: Enforce NOT using 32 bit sample size.
Using 32 bit samples is possible in dual mode but would require allot more implementation.

Bug 2:
When using DUAL_MODE and ADC3 and DMA (i.e. not BDMA) DMA transfer size is wrong.
Fix: set transfer size to sample size, not 2x, that is only needed for ADC12, ADC3 does not have a slave
Note: BDMA setting was already correct.

Bug 3:
When DUAL_MODE and ADC12 and ADC3 is enabled the code block intended to configure ADC1 or ADC3 in non-dual mode runs all the time. So the configuration done in the extra code block added for ADC1 and dual mode, gets invalidated immediately after.
Fix: group single mode code into block and add an else so it does only run for ADC3 if everything is enabled.

Bug 4:
Driver enforces 3 settings in the CCR register CKMODE (Clock mode), DAMDF (Dual ADC mode data), DUAL (dual mode). The last one is not masked from the user settings allowing configurations that are not compatible with the driver.
Fix: mask DUAL bits

Bug 5:
Driver initializes the CCR register in the init() and stop() functions with default values that are enforced by the driver. This gets invalidated the moment start() gets called because it calls rccResetADCx() resetting all registers.
start_conversion() does not set the enforced values instead expects these values to be setup.
Apart from DMA data format setting being wrong this also causes ADC2 to never run since the DUAL setting is never set, effectively running only in single mode.
Fix: Setup CCR register in start_conversion() like it is done for other registers with forced settings.

Cleanup (patch6): This untangles interleaved preprocessor and c-code if blocks in adc_ld_stop(). And fixes indentation in start_conversion()


For my target platform I also had to set ADC_CFGR_OVRMOD for ADC1 in the ADCConversionGroup config. This is a workaround for an errata (ES0445 - Rev 5 p. 22 2.10.6 slave data may be shifted in dual regular simultaneous mode). If this is not done the samples from the slave ADC (ADC2) may be shifted by 1 position in the buffer. In my observation this happened usually within 10-20 sec. So the master and slave sample sequence gets out of sync. But the shift only happened once. With the workaround from the errata this behavior is fixed. I don't know if other platforms that use the same driver are effected by this as well. If that is the case it might be worth it to enforce this directly by the driver.

[Giovanni]

Hi,

Thanks for patches, the problem with Bug 2 is that the adcsample_t type is fixed so it cannot be right for both ADC12 and ADC3. This cannot be fixed without changing the whole ADC driver concept.

Giovanni

[jawe]

Regarding bug 2, I don't see the problem.

In fact if patch 1 is applied but patch2 is not, now ADC3 + Dual mode will now create buffer overflows in the ADC3 sample buffer.

The size of adcsample_t is in fact fixed, but that should not matter really. This only needs to hold one sample of an individual ADC, this does not have to be bigger just because the Dual mode just happens to use a DMA transfer size larger than sizeof(adcsample_t). Dual mode does not require a larger sample size.

The DMA transfers for ADC12 in dual mode and the transfer for ADC3 (regardless of dual) work differently.
ADC12: Copies 2 samples from the ADC12 Common Data Register (CDR) in one transfer. So the transfer size has to be 2x sizeof(adcsample_t).
ADC3: Copies 1 sample from the ADC3 Data register in one transfer. So the transfer size has to be 1x sizeof(adcsample_t).

For ADC12 this is correctly setup already for the cases that STM32_ADC_SAMPLES_SIZE==8 and STM32_ADC_SAMPLES_SIZE==16. There is no implementaion for STM32_ADC_SAMPLES_SIZE==32, thats the reason for patch 1. This is possible but requires some changes in DMA and CCR settings.
For ADC3 this is NOT correctly setup. Here the transfer size currently is the same as for ADC12 settings even though the DMA transfers work differently. For the BDMA case it is correct though. ADC3 should work the same way regardless of dual mode setting since it does not have a slave.


The transfer size of the ADC12 DMA can be bigger than the adcsample_t since the driver in dual mode enforces that there will be equal amounts of samples between the two ADCs (1&2). And the DMA stream will get exactly half as many transfer requests as there are samples, while copying twice as much data.