arduino-libraries / Arduino_AdvancedAnalog

Advanced Analog Library
GNU Lesser General Public License v2.1
7 stars 5 forks source link

Sampling with very high frequencies did not work as expected. #51

Closed divebubble closed 7 months ago

divebubble commented 7 months ago

First of all, thank you for this great library.

Sampling with very high frequencies did not work as expected.

According to the documentation for the STM32H7, sampling rates of > 2 MHz should be possible. I have read that you are working with a slow timer.

In my case I need at least 4 ADC channels with a sampling rate of at least 8bit / 600'000Hz each. The problem is that I should work with ultrasonic receivers that receive at a frequency of 200KHz. In order to analyze the received data correctly, the triple sampling rate is the lower limit.

So far I have managed to sample at 70KHz per channel, distributed over two ADCs with 2 channels each. As soon as I go higher (80KHz) the program no longer starts.

Is this due to this "slow timer" or are my requirements too high? It's been about 30 years since I wrote anything in C++ or assembler, but with a few hints I might be able to adapt the code myself.

leonardocavagnis commented 7 months ago

Hi @divebubble, Please, could you share your sketch code?

divebubble commented 7 months ago

Hi @leonardocavagnis

Arduino Giga R1 WiFi, Arduino_AdvancedAnalog 1.3.0 The test sketch runs sometimes up to a sampling rate of 974'800Hz per channel. Sometimes it stops already at 450'000 or already at 70'000

#include <Arduino_AdvancedAnalog.h>

/*
  need to add a signal generator to A0 to check
  if any usefull data in buffer
*/

AdvancedADC adc_1(A0, A1);
AdvancedADC adc_2(A5, A6);

const int sampleRate = 10000; 
const int sampleIncrease = 960;
int increaseCnt = 0;

void setup() {
    Serial.begin(9600);

}

void InitADCandRead(int SamplingRate) {
    int tryRead = 0;

    Serial.print("Trying sampling rate of "+String(SamplingRate));

    // Initialize ADC with: resolution, sample rate, number of samples per channel, queue depth
    if (!adc_1.begin(AN_RESOLUTION_8, SamplingRate, 32, 64)) {
        Serial.println("Failed to start ADC_1!");
    }
    if (!adc_2.begin(AN_RESOLUTION_8, SamplingRate, 32, 64)) {
        Serial.println("Failed to start ADC_2!");
    }

    //trying 5 times
    while (tryRead < 5)
    {
      tryRead ++;

      if (adc_1.available()) {

        SampleBuffer buf1 = adc_1.read();
        SampleBuffer buf2 = adc_2.read();

        // simple frequency counter on channel 1
        // ----------------------------------------
        int lastBuf = 0;
        int frequency = 0;
        for (int i = 0; i < buf1.size(); i++)
        {
          if ((i % 2) == 0)
          {
            if (lastBuf < 127 && buf1[i] >= 127) frequency ++;
            lastBuf = buf1[i];
          }
        }
        buf1.release();
        buf2.release();

        Serial.print(",");
        Serial.print(frequency * (SamplingRate / 32));
        Serial.println();

        break;
      } 
      else
      {
        delay(1);
      }
    }

    adc_1.stop();
    adc_2.stop();

    if (tryRead > 4)
    {
      Serial.println("adc_1.available() 5 times false");
    }
}

void loop() {

  InitADCandRead(sampleRate + (increaseCnt * sampleIncrease));
  increaseCnt ++;

  delay(100);
}
iabdalkader commented 7 months ago

Is this due to this "slow timer" or are my requirements too high?

I'm not sure which slow timer you're referring to, the ADC trigger timer frequency is not fixed as it gets set to the sample_rate argument you pass to begin(). The ADC is clocked at 64 MHz, that's fixed, but with this clock the total conversion time is (Sample Time + Resolution/2 + 0.5) which is 17 cycles (or 0.265625 uS) so it should max out at ~3.6 Msps in theory, if everything else can keep up, in reality that's not likely.

Thanks for posting the sketch, I can test it later.

iabdalkader commented 7 months ago

Arduino Uno R1, Arduino_AdvancedAnalog 1.3.0

@divebubble This library only works on H7 micro-controllers, I think maybe you meant Giga or Portenta ?

iabdalkader commented 7 months ago

@divebubble

I tested your sketch, and noticed this:

      if (adc_1.available()) {

        SampleBuffer buf1 = adc_1.read();
        SampleBuffer buf2 = adc_2.read();

read() is blocking, you only test if ADC1 has buffers, but you don't test ADC2, so it may block forever. You should do something like:

if (adc_1.available() && adc_2.available()) {

        SampleBuffer buf1 = adc_1.read();
        SampleBuffer buf2 = adc_2.read();

This is just a hint, there might be an issue with stopping/restarting the ADCs.

iabdalkader commented 7 months ago

@divebubble This should fix the issue, I've been running your sketch for a while without any lockups. Are you able to test this PR ?

https://github.com/arduino-libraries/Arduino_AdvancedAnalog/pull/52

divebubble commented 7 months ago

@iabdalkader regarding the slow timer... some time ago there was a note about Known issues. But this has been removed in the meantime ADC is running at the slowest possible clock (PCLK), should probably set up a PLL and change the clock source.

adc_2.available() I have just one signal generator, so i can check only one.

I check PR #52 and let you know.

iabdalkader commented 7 months ago

ADC is running at the slowest possible clock (PCLK), should probably set up a PLL and change the clock source.

This is still true, but even at the slowest clock, it should be able to handle a very high sample rate, I think 600KHz is is doable, but let me know if it doesn't work.

I have just one signal generator, so i can check only one.

If you use more than one ADC (like in your example sketch) you must poll each instance with available() before calling read() on that instance, otherwise read() can block. If that's okay with you, to block on read(), then don't have to worry about polling.

divebubble commented 7 months ago

Thanks for the fix, now the test did work as expected > 600'000Hz

I have one more request... is it possible in HAL_ADC_ConvCpltCallback

// Timestamp the buffer. TODO: Should move to timer IRQ.
descr->dmabuf[ct]->timestamp(HAL_GetTick());

set the timestamp to the timer IRQ. As I understand it, this should then be much finer.

... and, since i probably need the code for a commercial project, can i donate an amount somewhere?

iabdalkader commented 7 months ago

I can try use the microseconds timer, if it works, to set the timestamp, but moving it to another IRQ handler might be more tricky. The only difference will be the timestamp will be set before the transfer is done vs after.

... and, since i probably need the code for a commercial project, can i donate an amount somewhere?

Thank you, you can always donate to Arduino's website I guess.

divebubble commented 7 months ago

Donation done... Thanks for you work. Whether the timestamp is set before or after the transfer is not important. But microseconds or the cycle count of the processor would be better for synchronizing the different streams. If not, I can also try to calculate the time based on the audio waves.

iabdalkader commented 7 months ago

I switched to us ticks for the timestamp in the same PR.