analogRead problem with IDE 1.5.5 libraries

I have a DUE test board where I use most of the analog inputs. For that test setup, I have written an automated test that reads the analog inputs and compares it with expected values.
When compiling with IDE 1.5.4, all runs fine. Compiling the same sketch with 1.5.5, I get mostly wrong analogRead results. I suspect a problem with the ADC initialization change that was introduced with 1.5.5. While appreciating a higher speed for A/D conversion, for sure no such problem must result from that change.

Please find below a few lines of code that I use.
I do not know if that could be important, but I apply the test voltages via 10kOhm series resistors (which should not be a problem since the analog inputs have a very high impedance). And another detail: I use an external 2.5V reference.

For now, I will revert to IDE 1.5.4, hopefully a solution will be found for 1.5.6.
Can anyone meanwhile confirm the bug, and is there a patch possible in the libraries of IDE 1.5.5?

#define A_U_IN 11

static const unsigned char AinPins[] = {
0,1,2,3,4,5,6,7
};

unsigned short Ain[sizeof(AinPins)];
unsigned short Apower;
boolean AinOk[sizeof(AinPins)];
boolean ApowerOk;

// Note: external reference voltage is 2.5V
static const unsigned short AinTest[] = {
1870, 1900, //=DAC0, set to value 1800, at A0
2130, 2170, //=DAC1, set to value 2200, at A1
795, 843, //=0.50V at Ain2
1203, 1255, //=0.75V at Ain3
1610, 1667, //=1.00V at Ain4
2018, 2078, //=1.25V at Ain5
2425, 2490, //=1.50V at Ain6
2833, 2902 //=1.75V at Ain7
};

// Following code is in a loop:

// Read the analog inputs and compare with pre-defined values
for (i = 0; i < MIN(sizeof(AinPins), 8); i++) {
Ain = analogRead(AinPins*);*
AinOk = (Ain >= AinTest[i << 1] && Ain <= AinTest[(i << 1) + 1]);
* }*
* Apower = analogRead(A_U_IN);
_ ApowerOk = (Apower >= 2701 && Apower <= 3243);
// ... other I/O and text output via USB*_

Faster ADC settings require lower impedance source, that's just how it is, the sample/hold
capacitor must be charged in the time available.... With 10k and the input capacitance
of 8pF quoted in the datasheet 12 bit settling time is 665ns, which if you are configuring
the ADC for 1MHz operation is impossible, you have to charge the sample cap in a fraction
of the conversion time.

It sounds like what would be good is a conversion speed parameter, possibly three values,
slow/medium/fast, with documented source impedance requirements.

I am not sure if the source impedance is the only problem. I also partially see very similar converted values (e.g. 2898) for different analog inputs that are read in a fast sequence, while the input voltages differ as listed in the table in my source code.
If the source impedance was the only problem, we would just talk about decreased accuracy.
Is it possible that there needs to be an additional delay if the analog read input changes?

It appears 1.5.5 lost the ability to use analog input numbers and only accepts analog input names. Take a look at the short sketch below. I'm going back to 1.5.4 until the bug is fixed.

void setup() {
  Serial.begin(115200);
  analogReadResolution(12);

  //These read properly
  Serial.println(analogRead(A0));
  Serial.println(analogRead(A1));
  Serial.println(analogRead(A2));
  Serial.println(analogRead(A3));
  Serial.println(analogRead(A4));
  Serial.println(analogRead(A5));
  Serial.println(analogRead(A6));
  Serial.println(analogRead(A7));
  Serial.println(analogRead(A8));
  Serial.println(analogRead(A9));
  Serial.println(analogRead(A10));
  Serial.println(analogRead(A11));
  Serial.println();

  //These read incorrectly
  for(byte n = 0; n <= 11; n++)  {
    Serial.println(analogRead(n));
  }
  Serial.println();

  while(1);  {}  //halt here

}

void loop() {
}

Jon

No, I don't think that's the issue, the code for analogRead converts pin numbers the same
in 1.5.4 and 1.5.5, from analogRead():

  if (ulPin < A0)
    ulPin += A0;

One strange thing I've noted is that the ADC conversion time seems to change depending
on apparently unrelated code (calling Serial.println in setup(), for instance, after
analogReadResolution (12) ;). Perhaps the delay causes the ADC to go into some sort
of over-run condition that affects the speed of subsequent conversions. I'm seeing
a benchmark vary between about 1.7us and 2.0us dependent on this.

I haven't noticed any obvious difference in behaviour between using 0 and A0 in
analogRead.

Hi guys,

The bug was introduced after the optimization of the ADC reads (shame on me :()

May I ask you to try with the following patch and check if that fixes the problem?

replace the file Arduino/hardware/arduino/sam/cores/arduino/wiring_analog.c
with the file here: https://raw.github.com/arduino/Arduino/a1c48091051bfc7f3366cac6fc23212c4e4fb4a3/hardware/arduino/sam/cores/arduino/wiring_analog.c

Thanks

That fixes the read values, although conversion times are back to the same as 1.5.4.

Evaluating Arduino and Due ADCs has some information about speeding up ADC read times. Using the line

REG_ADC_MR = (REG_ADC_MR & 0xFFF0FFFF) | 0x00020000;

reduces twenty reads in my sketch from 840uS to 240uS.

Jon

Hi stratosfear,

stratosfear:
That fixes the read values, although conversion times are back to the same as 1.5.4.

Just checked and you're right, If you multiplex over many pins the ADC Startup time is applied at every ADC conversion and things slows up again. The speed increases only if you read from a single pin.

stratosfear:
Evaluating Arduino and Due ADCs has some information about speeding up ADC read times. Using the line

REG_ADC_MR = (REG_ADC_MR & 0xFFF0FFFF) | 0x00020000;

reduces twenty reads in my sketch from 840uS to 240uS.

The problem with this line is that it sets incorrect Tracking and Conversion timings for the ADC, see the discussion here for more details: ADC initialization mistake in ADC.h · Issue #1418 · arduino/Arduino · GitHub

Now, the sad thing is that I've already fixed that but I've lost the fix with a clumsy move on git.
And I perfectly remember that I did the same steps we are taking here, i.e.:

  1. I applied a patch similar to the one in my previous message
    [sam] Fixed regression in analogRead() (fails to read multiple channels) · arduino/Arduino@a1c4809 · GitHub
  2. I've tested ADC again, and I've seen the slowdown like you did
  3. I've applied another change that I can't remember now, but at the time was quite obvious.

So I know for sure that there is a solution and is simple, I just don't remember how :frowning:

Current issue seems to be that when you switch between different pins to analogRead()
all the ones you've already used remain enabled, so that the readings are completely
spurious (an adc_start() triggers multiple conversions).

I found out that simply disabling the current channel before enabling the new one
causes the ADC to shutdown and thus you incur a STARTUP delay.

So you have to disable the old channel after enabling the new one.

Change this code in analogRead()

      if (ulChannel != latestSelectedChannel) {
        adc_enable_channel( ADC, ulChannel );
        latestSelectedChannel = ulChannel;
      }

to

      if (ulChannel != latestSelectedChannel) {
        adc_enable_channel( ADC, ulChannel );
       	if (latestSelectedChannel != -1)
	  adc_disable_channel (ADC, latestSelectedChannel) ;
        latestSelectedChannel = ulChannel;
      }

(In the section conditional on

#if defined __SAM3X8E__ || defined __SAM3X8H__

of course)

[ edit: I think what is happening is that the conversion sequencer powers down the ADC between conversions if sleep mode is active, but also powers down if no channels are
currently enabled. The datasheet has to be read between the lines here! ]

You find it! Thanks MarkT!

It's the second good fix in a row that you propose, I'm really impressed :slight_smile:

If someone wants to test it the updated wiring_analog.c is here:

https://raw.github.com/arduino/Arduino/b530742603439c87a06d2da9ddd60dd9f82f5082/hardware/arduino/sam/cores/arduino/wiring_analog.c

I think what is happening is that the conversion sequencer powers down the ADC between conversions if sleep mode is active, but also powers down if no channels are currently enabled. The datasheet has to be read between the lines here!

As I said, at the time it seemed almost obvious, but I agree, the datasheet should be a little bit clear on that.

C

Thanks! It took a little guesswork to finesse this issue - currently working
on interrupt-driven sampling and digital filtering audio with the Due as it seems
powerful enough to handle quite a lot of interesting stuff (although 12 bits isn't
enough)

I would like to have a go at interfacing a pair of I2S chips (ADC and DAC) using the
synchronous serial controller, but that's down the line a bit (thinking of an I2S
shield with 12.288MHz crystal oscillator, some Wolfson Micro 24 bit ADCs and DACs,
SD slot...)

I tried out both versions of wiring_analog.c with 1.5.5 and still have the same problem with analogRead not working properly. I reverted to 1.5.4 and it works fine with my code.

I'm new to arduino, but have some embedded programming experience. I'm happy to help debug the code if you can point me to the parts of the code that have changed between 1.5.5 and 1.5.4.

Erm: diff - Wikipedia

@donesolnas

the change applied are the following:

first (high speed, but breaks reading on multiple analog channels), **EDIT:**this one is in 1.5.5:

second (reading are correct again but slow if you read on multiple channels), this one is in 1.5.5:

third (reading are correct and at high speed as expected):

Another way could be to download the nightly build that should have the patch now:

I today tried the latest patch from the Nightly Build and it basically works in my application. However, accuracy is significantly reduced when compared to the analogRead from the 1.5.4 IDE.
I found a solution for that. The wiring_analog.c code has to be supplemented in analogRead:

delayMicroseconds(2); // this line new
// Start the ADC
adc_start( ADC );

I tried several delay times and 2microseconds seem to be sufficient to have comparable accuracy than before, and that time should not hurt too much on overall conversion time.
I suggest to take that patch over for the Nightly Builds and then into IDE 1.5.6.

I presume that delay only matters if switching channels? It would be a shame to slow down
the common case by a factor or 2 or 3 needlessly?

I can confirm that the delay is not necessary if the channel is not switched. Hence the delay can be made conditional.

However, after thinking over night about the matter, another aspect came into my mind. What is the exact effect to the analog section of the SAM when more than one channel is enabled, even if for short time only? Does this mean that the related analog input pins are then electrically connected with low impedance? If that was the case, this way of managing the channels would not be recommendable at all, since this could then destroy the related switches in the SAM if the source impedance at those pins is sufficiently low. A usual low source impedance would be e.g. some filter capacitors at the analog input pins, which could discharge quickly over connected analog input pins.

Embed:
What is the exact effect to the analog section of the SAM when more than one channel is enabled, even if for short time only? Does this mean that the related analog input pins are then electrically connected with low impedance?

Looking at the logic diagram on the datasheet it seems that there is a multiplexer before the ADC, so theoretically only one pin at a time can be connected to the ADC.

@Embed
may you post some more information on your test setup? May the inaccuracy you're experiencing be due to high impedance on your analog source?

The inaccuracy for my test setup, noticed with the latest patch, is probably due to the high impedance.
I generate the test voltages with 100k trimmers, followed by 10k resistors in series to the analog inputs.
I guess I understand now this effect. With older libraries, the SAM introduced a startup time that was long enough to charge the input capacity of the AD converter. With the latest patch, this startup time is omitted. So there should be a note in the Arduino documentation that specifies the max. input impedance without recognizable effects on accuracy.

One question still remains even after your explanation:

Looking at the logic diagram on the datasheet it seems that there is a multiplexer before the ADC, so theoretically only one pin at a time can be connected to the ADC.

If only one pin can be connected to the ADC at a time, why is it then necessary at all to disconnect the old channel, since it should be disconnected at the time when the new one is connected? Disconnection would then only be necessary if you want to have the ADC going to sleep as in the libraries until IDE 1.5.4. This part of the code could then be removed from the analogRead function. However, should it be necessary to disconnect the old channel in order to get correct results, then it would not be the case that only one input is connected.

The whole ADC architecture is designed to be able to sample any subset of the analog
channels, in any order, at any sample rate (from one of many clock sources) and write
the results direct to memory buffer via DMA. You don't have to enable all of this
stuff, but do need to say exactly which subset of channels are active (since a sample
event causes them all to be sampled in the defined order and various status bits updated
and/or interrupts triggered and/or DMA writes performed).

If you set the enabled set to empty the ADC automatically drops into power-save
mode. So the best way to drive it for analogRead() is enable new channel, disable old channel (if different), trigger a reading.