STM32F4 ADC race condition between adcConverts Topic is solved

[pseidel]

We discovered what appears to be a very timing specific race condition that can occur between adcConverts with different groups (might occur for same groups as well). Effectively, the second adcConvert with group B can return samples where the first sample is a resample of the group A's first channel. The rest of group B then ends up shifted right by 1 in the returned samples. Example failure for 2 groups each containing 2 channels.

• adcConvert (GroupA) = [ ChannelA0, ChannelA1 ]
• adcConvert (GroupB) = [ ChannelA0, ChannelB0 ] (Missing ChannelB1, B0 in B1's slot)

The issue occurs for any number of channels in either group, tested with 1v1, 2v1, 16v9, just with different timing required. I have a chSysPolledDelayX in between the converts. I can go from a 0.03% failure rate to 99.9% failure rate by changing the polled delay from 61 to a 62 at 168 MHz.


I am able to resolve this in 2 ways by modifying ADCv2. The first is removing ADC_CR2_CONT from CR2 in adc_lld_start_conversion (breaks the depth parameter when != 1). The second is clearing CR2_ADON in adc_lld_stop_conversion, basically undoing the #822 bug fix (adcp->adc->CR2 = 0; adcp->adc->CR2 = ADC_CR2_ADON;).

I will also note the #822 bug fix in adc_lld_stop_conversion seems incomplete as the first CR2 line preserves the injected bits, but the second CR2 line clears them anyway. So injected channels would still be disabled after a completed non-circular adcConvert. SWSTART is also cleared by hardware on start of conversion, so the first CR2 write doesn't really do anything.

The other thing I noticed is that, once fixed, the frequency of execution of the adcConverts slows down. Using the second fix, it goes from 33798 Hz to 30543 Hz. This and some logic analyzer palToggleLines in ISRs indicate that there is an early DMA RX ISR occurring. In the attached image, the red timing marks indicate a failure with 13.14 us from DMA int to DMA int, where the green one is the standard / correct 16.34 us.

We are running with the second fix for now as we don't use injected channels.


I verified this on an STM32F4 Discovery board, 168 MHz, GNU Arm Embedded Toolchain 10.3-2021.10, -O2, ChibiOS 21.11.2.

For the following code, PA0 is tied to ground for sampling and VREFINT is also sampled for 2 significantly different values. The tunable delay is nDelayAfterA. The comments have some values that worked for me. Interestingly, depth of 1 or 2 doesn't require changing the delay.

The once a second serial printout seems necessary for probably timing reasons. G_Line_Adc_Int and G_Line_Dma_Int are configured and ready to be injected with palToggleLine in the DMA and ADC interrupts. I can provide additional information (logic analyzer captures, etc) if you need.

Patrick

Code: Select all

#include "ch.h"
#include "hal.h"

#include "chprintf.h"

static BaseSequentialStream* const G_Debug = (BaseSequentialStream*) &SD2;

// Grounded externally.
static const ioline_t G_Line_InputPin = PAL_LINE (GPIOA, 0);

static const ioline_t G_Line_ConvertA = PAL_LINE (GPIOC, 1);
static const ioline_t G_Line_ConvertB = PAL_LINE (GPIOC, 2);

static const ioline_t G_Line_FailA = PAL_LINE (GPIOB, 0);
static const ioline_t G_Line_FailB = PAL_LINE (GPIOB, 1);

ioline_t G_Line_Adc_Int = PAL_LINE (GPIOE, 7);
ioline_t G_Line_Dma_Int = PAL_LINE (GPIOE, 8);

// ADC Configs
static const ADCConfig G_AdcConfig = {};

static const ADCConversionGroup G_GroupA = {
   .circular      = false,
   .num_channels   = 2,
   .end_cb         = NULL,
   .error_cb      = NULL,

   .cr1 = 0,
   .cr2 = ADC_CR2_SWSTART,

   .smpr1   = 0,
   .smpr2   = ADC_SMPR2_SMP_AN0 (ADC_SAMPLE_56),

   .htr = 0,
   .ltr = 0,

   .sqr1   = 0,
   .sqr2   = 0,
   .sqr3   = ADC_SQR3_SQ1_N (ADC_CHANNEL_IN0) | ADC_SQR3_SQ2_N (ADC_CHANNEL_IN0)
};

static const ADCConversionGroup G_GroupB = {
   .circular      = false,
   .num_channels   = 2,
   .end_cb         = NULL,
   .error_cb      = NULL,

   .cr1 = 0,
   .cr2 = ADC_CR2_SWSTART,

   .smpr1   = ADC_SMPR1_SMP_VREF (ADC_SAMPLE_56),
   .smpr2   = 0,

   .htr = 0,
   .ltr = 0,

   .sqr1   = 0,
   .sqr2   = 0,
   .sqr3   = ADC_SQR3_SQ1_N (ADC_CHANNEL_VREFINT) | ADC_SQR3_SQ2_N (ADC_CHANNEL_VREFINT)
};

static THD_WORKING_AREA (waThreadHeartbeat, 128);
static THD_FUNCTION (ThreadHeartbeat, arg)
{
   (void) arg;
   chRegSetThreadName ("blinker");
   while (true)
   {
      palSetLine (LINE_LED_BLUE);
      chThdSleepMilliseconds (500);
      palClearLine (LINE_LED_BLUE);
      chThdSleepMilliseconds (500);
   }
}

static void debugPrintf (const char* sFmt, ...)
{
   va_list ap;

   va_start(ap, sFmt);
   chvprintf(G_Debug, sFmt, ap);
   va_end(ap);
}

int main ()
{
   // Initialize ChibiOS.
   halInit ();
   chSysInit ();

   // Initialize the debug serial output. Print a startup message with the revision.
   sdStart (&SD2, NULL);
   debugPrintf ("\r\n\r\nLaunching %s\r\n", "AdcDiag");

   // Start the heartbeat thread.
   chThdCreateStatic (waThreadHeartbeat, sizeof (waThreadHeartbeat), NORMALPRIO, ThreadHeartbeat,
                  NULL);

   // Serial
   palSetPadMode (GPIOA, 2, PAL_MODE_ALTERNATE(7));

   // ADC Grounded externally.
   palSetLineMode (G_Line_InputPin, PAL_MODE_INPUT_ANALOG);

   // Logic analyzer outputs
   palSetLineMode (G_Line_ConvertA, PAL_MODE_OUTPUT_PUSHPULL | PAL_STM32_OSPEED_HIGHEST);
   palSetLineMode (G_Line_ConvertB, PAL_MODE_OUTPUT_PUSHPULL | PAL_STM32_OSPEED_HIGHEST);
   palSetLineMode (G_Line_FailA, PAL_MODE_OUTPUT_PUSHPULL | PAL_STM32_OSPEED_HIGHEST);
   palSetLineMode (G_Line_FailB, PAL_MODE_OUTPUT_PUSHPULL | PAL_STM32_OSPEED_HIGHEST);
   palSetLineMode (G_Line_Adc_Int, PAL_MODE_OUTPUT_PUSHPULL | PAL_STM32_OSPEED_HIGHEST);
   palSetLineMode (G_Line_Dma_Int, PAL_MODE_OUTPUT_PUSHPULL | PAL_STM32_OSPEED_HIGHEST);

   // ADC setup
   adcStart (&ADCD1, &G_AdcConfig);
   adcSTM32EnableTSVREFE ();

   rtcnt_t nTimeBeforeB = 0;
   systime_t nLast = chVTGetSystemTimeX ();
   int n = 0;
   int nAFail = 0;
   int nBFail = 0;

   adcsample_t a [4];
   adcsample_t b [4];

   // 0:  9 B failures / s
   // 44: No failures
   // 61: 10 B failures / s
   // 62: 33k B failures / s
   // 200: 3 A failures / s
   volatile rtcnt_t nDelayAfterA = 62;

   while (true)
   {
      rtcnt_t nLastTimeBeforeB = nTimeBeforeB;

      rtcnt_t nTimeBeforeA = chSysGetRealtimeCounterX ();
      palToggleLine (G_Line_ConvertA);
      adcConvert (&ADCD1, &G_GroupA, a, 1);

      chSysPolledDelayX (nDelayAfterA);

      nTimeBeforeB = chSysGetRealtimeCounterX ();
      palToggleLine (G_Line_ConvertB);
      adcConvert (&ADCD1, &G_GroupB, b, 1);

      // Threshold checks
      // A is grounded (PA0)
      bool bAFailed = a [0] > 100;
      // B is VREFINT, significantly non-zero.
      bool bBFailed = b [0] < 1400 || b [0] > 1800;

      if (bAFailed)
      {
         nAFail++;
         palToggleLine (G_Line_FailA);
         // debugPrintf ("A: %4d, %4d\r\n", a [0], a [1]);
      }

      if (bBFailed)
      {
         nBFail++;
         palToggleLine (G_Line_FailB);
         // debugPrintf ("B: %4d, %4d\r\n", b [0], b [1]);
      }

      // Print results once a second
      n++;

      systime_t nNow = chVTGetSystemTimeX ();
      if (nNow - nLast > TIME_MS2I(1000))
      {
         nLast = nNow;
         debugPrintf ("nDelayAfterA: %d, Hz: %d, A to B Delay = %d, B to A Delay = %d, A Fail = %d, B Fail = %d\r\n",
               nDelayAfterA, n, nTimeBeforeB - nTimeBeforeA,
               nTimeBeforeA - nLastTimeBeforeB, nAFail, nBFail);
         n = 0;
         nAFail = 0;
         nBFail = 0;
      }
   }
}

[Giovanni]

Hi,

Not sure how to fix this, il looks like an F4 silicon bug, clearing ADON has the problem that a "fSTAB" time is required after setting it again (it is 2..3 microseconds), see the datasheet.

In addition, I am not sure this problem is still present in later devices using ADCv2, any fix should not impact those too.

There is a device errata on the ADC, not sure it can explain this:

Code: Select all

ADC sequencer modification during conversion

Description

When a software start-of-conversion is used as an ADC trigger, and if the ADC_SQRx or ADC_JSQRx register is
modified during the conversion, the current conversion is reset and the ADC does not automatically restart the
new conversion sequence. The hardware start-of-conversion trigger is not impacted and the ADC automatically
restarts the new sequence when the next hardware trigger occurs.

Workaround

When a software start-of-conversion is used, apply the following sequence:
1. First set the SWSART bit in the ADC_CR2 register.
2. Then restart the new conversion sequence.

Giovanni

[Giovanni]

Reviewing the code I think that stopping the ADC by clearing SWSTART is wrong, it is cleared already in HW anyway. It looks like a conversion of the group A is stuck in the internal FIFO and is picked up on the next conversion group.

The problem is how to reliably stop a conversion, clearing ADON works but has the problem that the user has to add a delay between conversions.

Newer STM32 ADCs have explicit stop mechanisms.

Could you try something like this?

Code: Select all

void adc_lld_stop_conversion(ADCDriver *adcp) {

  dmaStreamDisable(adcp->dmastp);
  adcp->adc->CR1 = 0;
  if ((adcp->adc->CR2 & ADC_CR2_CONT) != 0U) {
    adcp->adc->CR2 = 0;
  }
  adcp->adc->CR2 = ADC_CR2_ADON;
}

It is possible that disabling ADON very briefly can clear the internal logic and then not require the full fSTAB time to restart the analog part. If confirmed to work I will commit this.

Giovanni

[pseidel]

Giovanni,

Thanks for taking a look. It does look like ADCv2 hardware lacks a proper way to stop any in progress conversions compared to the others. I've tried a few other things related to the DDS bit and such, but no luck other than the 2 fixes I provided.

The hope was that clearing and setting ADON quickly would bypass the presumed to be internal circuitry charge time. I haven't noticed a change in the internal VREF adc value with or without the ADON fix (always 1683 - 1685 counts on one unit).

I also noticed the errata, but I didn't understand what "restart the new conversion sequence" was. I assumed that would be SWSTART, but the previous line does that. Unless they wanted SWSTART, change sequence, then SWSTART again?

Your proposed fix works fine for us, tested with many delay values. Note that I am only testing with SWSTART and no hardware triggering.

Thanks,
Patrick

[Giovanni]

Hi,

Good, I will open a ticket and commit the above code as fix. Thanks for helping with this.

Giovanni

[Giovanni]

Hi,

Fixed as bug 1294.

Giovanni