arduino-pico icon indicating copy to clipboard operation
arduino-pico copied to clipboard

Samples obtained from ADCInput are incorrectly assigned when using 3 inputs.

Open Geekachuqt opened this issue 5 months ago • 4 comments

Hi, I've encountered as issue where the ADCInput library appears to return samples from the wrong buffer when using 3 inputs.

I'm reading the values from 3 potentiometers at 48KHz. When using ADCInput adc(A0, A1, A2) and including 3 adc.read() calls in my loop, the read values shuffle around between the variables I am assigning them to. However, if I use ADCInput adc(A0,A1,A2,A3) and include 4 adc.read() calls in my loop, and just not assigning the fourth call to any variable, the read values are stable and appear at the expected positions.

This is a bit tough to provide a MVTC for, as it's hardware dependant. That said, I would expect any test code that works correctly with 4 ADCs enabled will not function correctly with 3 ADCs selected.

I will say that I'm not very certain about what is going on here, but this is my best guess as to what is going on.

Geekachuqt avatar Jun 12 '25 12:06 Geekachuqt

I can't reproduce any issue with 3 in inputs in a very basic sketch I just tried. I tied A0 and A2 to GND, and A1 to 3V3 and run a 3-input ADCInput sampler. I get consistent results, no shifting at all when I run it or move the GND/3V3 connection around.

#include <ADCInput.h>

ADCInput test(A0, A1, A2);

void setup() {
  test.begin(1);
}

void loop() {
  Serial.printf("%4d %4d %4d\n", test.read(), test.read(), test.read());
}

So, you'll need to double check your sketch and develop an MCVE because it can't be reproduced here with the straightforward case.

earlephilhower avatar Jun 12 '25 16:06 earlephilhower

And be sure you're running the latest release 4.5.4 since there was an ADCInput fix documented.

earlephilhower avatar Jun 12 '25 16:06 earlephilhower

Okay, so something else is causing issues then. I'll try to work on a MCVE and get back to you. As a sidenote: what kind of buffer sizes and amounts would you recommend for working at audio rates?

Geekachuqt avatar Jun 14 '25 08:06 Geekachuqt

Ah, your question leads me to think you're overflowing the buffers (i.e. not processing them fast enough always) and having to drop a complete buffer of ADC samples. When that happens in the 1 or 2 input case (always), or 4 input case (as long as you have an even number of 32b samples which is something any sane person already does) things align properly and, while you lose data you don't lose tracking.

For 3 inputs to guarantee even with overflowed samples you don't lose tracking, make sure your buffer length is a multiple of 3. So if you have a buffer length of 3000 (32b words)then every buffer will start with sample 0. If you have a buffer length of 3001 (32b words) the 1st sample in buffer 0 will be 0, but the 2nd buffer will have sample 2 at index 0 and the 3rd will have sample 1 and you'll go on like that. In normal mode this is fine, but if you have to drop a buffer because you didn't process it fast enough you'll have issues.

So, make it a multiple of 6 words (so you can evenly divide in 1, 2, 3, or 4 samples) and then even with overflow it should still line up if you drop buffers.

Choosing buffer sizes and counts isn't a 1-size-fits-all thing. The buffer length sets the minimum latency. You don't get notified of samples until a complete buffer length is filled (and at that point you get the whole buffer's worth). Too small and the IRQ overhead will be high. Too large and you'll be processing data from 500ms before.

The bufferCnt needs to be more than 3 and can help with occasional stalls in your app. Think of it like a temporary storage bucket for your data. If you never stop processing and always complete a buffer before the next one is ready then 3 is fine. If occasionally you need to do other work, make sure you add enough buffers so that there's space the DMA can fill before throwing away things. The number can be calculated using the maximum backup time divided by the time it takes to fill one buffer, but in practice it's simpler to just try 6-10 bufferCnt and if there's issues then increase it. If you find that you can't avoid overflow even with extra buffers than your per-buffer processing needs to speed up, of course.

There's no harm in supplying as many buffers as memory allows. They won't be used unless your app stalls processing and don't introduce any intrinsic delay.

For 8000hz speech I'd probably use a buffer size of 1/60th of a second samples (8000/60 * ADCChannels * 0.5 [because each ADC sample is 1/2 a 32b word]) and maybe 10 bufferCnt (i.e. 1/6th of a second max hiccup). And then factor in the "must be a multiple of 3 and 2". So in the 3 ADC case it would be 200 but then I'd move up to 204 (6 * 34) to meet that factor constraint.

earlephilhower avatar Jun 14 '25 18:06 earlephilhower

So it's not that there was a bug, but rather a configuration requirement. On a related note, it was indeed an underrun issue that was caused by overloading core1. I have an application that runs a DSP model on core 1, and my complete sampling routine on core 0. I had originally assumed that the cores ran fully independently, but it turns out that's not the case - overloading core1 will mess somehow with code that runs on core0, or possibly with something under the hood.

If I comment out the model code, and instead add a function that simply copies the input buffer to the output buffer, everything works as it should.

However, thanks to your advice I was able to reduce the latency of my processing by 33% by choosing better suited buffer sizes, so thanks a lot! I understand the underlying code and requirements of the sample buffers a lot better now.

For what it's worth, I'm sticking to keeping 4 ADCs active and discarding the fourth sample using an "empty" adc.read() call. It may not be the most efficient way, but at least this way everything behaves as I expect, and it makes sense.

Geekachuqt avatar Jun 16 '25 08:06 Geekachuqt

Great, looks like we understand what's going on here and now have a check in the core that it can't happen. Thx for the update! Closing...

earlephilhower avatar Jun 16 '25 19:06 earlephilhower