Go Down

Topic: Bugs in analogRead-Command found (Read 8607 times) previous topic - next topic

Edgar_M

Dear Arduino Community!

I've recently found 2 bugs in analogRead (in file wiring_analog.c, version "Modified 28 September 2010 by Mark Sproul") and I tought, with this I can contribute to this great project and community.

Bug #1

Line 67:
Code: [Select]

ADMUX = (analog_reference << 6) | (pin & 0x07);


should be:
Code: [Select]

ADMUX = (analog_reference << 6) | (pin & 0x0F);


Because, as stated in the comment above, you want to mask the low 4 bits of the channel-number (0x07 = B00000111, 0x0F = B00001111)


Bug #2

Line 71:
Code: [Select]

//delay(1);


Currently commented out, but this is mandatory! If you've made an analogRead from a different channel in the last call, the ADC needs some settle time (a few clock cycles only) to switch the channel properly. Otherwise you get considerably wrong readings!

Hope, this helps  :)

Greets,
Edgar

retrolefty

But wouldn't adding a delay to analogRead cause it to hang if used inside a users interrupt routine (ISR) as delay() requires interrupts to be enabled and they are globally disabled while inside a user's ISR ?

Lefty

Edgar_M

Ok, that's a good point.

Then, maybe something like this would do the trick also:
Code: [Select]

   __asm__ __volatile__ (" nop \n nop \n nop \n nop \n nop \n nop \n nop \n"
   "nop \n nop \n nop \n nop \n nop \n nop \n nop \n nop \n nop \n nop \n":: );


(untested - one has to find out, if that many nop-commands are necessarry)

Edgar_M

Better solution:
Code: [Select]

   delayMicroseconds(1000);


As far as I know this command doesn't rely on interrupts.

Tested on an ATmega328P-Board - works

robtillaart

analogRead itself uses about 120uSec IIRC which is quite long. Therefor I would not use it in a ISR.

It is possible to split up the analogRead in some parts, this can help to make the part in the ISR shorter,

Code: [Select]

//
//    FILE: asyncAnalogRead.pde
//  AUTHOR: Rob Tillaart
//    DATE: 09-jun-2012
//
// PUPROSE: experiment
//
// SEE ALSO: http://www.avrfreaks.net/index.php?name=PNphpBB2&file=printview&t=56429&start=0

void setup()
{
  Serial.begin(115200);
  Serial.println("start: ");
  Serial.println(analogRead(0));
}

void loop()
{
  unsigned long start=0;
  unsigned long duration = 0;
  int count=0;

  AR_Start(0);
  start = micros();
  while(!AR_Ready())
  {
    //digitalWrite(13, count & 0x01);
    count++; // dummy behaviour
  }
  duration = micros() - start;
  digitalWrite(13, LOW);

  Serial.print(count);
  Serial.print("\t");
  Serial.print(duration);
  Serial.print("\t");
  Serial.print(AR_Value());
  Serial.println();
  delay(1000);
}

#include "wiring_private.h"
#include "pins_arduino.h"

// uint8_t analog_reference = DEFAULT;

void AR_Start(uint8_t pin)
{
#if defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
  if (pin >= 54) pin -= 54; // allow for channel or pin numbers
#else
  if (pin >= 14) pin -= 14; // allow for channel or pin numbers
#endif

#if defined(ADCSRB) && defined(MUX5)
  // the MUX5 bit of ADCSRB selects whether we're reading from channels
  // 0 to 7 (MUX5 low) or 8 to 15 (MUX5 high).
  ADCSRB = (ADCSRB & ~(1 << MUX5)) | (((pin >> 3) & 0x01) << MUX5);
#endif
  // set the analog reference (high two bits of ADMUX) and select the
  // channel (low 4 bits).  this also sets ADLAR (left-adjust result)
  // to 0 (the default).
#if defined(ADMUX)
  ADMUX = (DEFAULT << 6) | (pin & 0x07);
#endif

  // without a delay, we seem to read from the wrong channel
  //delay(1);
  sbi(ADCSRA, ADSC);
}

boolean AR_Ready()
{
  // ADSC is cleared when the conversion finishes
  return bit_is_set(ADCSRA, ADSC)==0;
}

int AR_Value()
{

  // we have to read ADCL first; doing so locks both ADCL
  // and ADCH until ADCH is read.  reading ADCL second would
  // cause the results of each conversion to be discarded,
  // as ADCL and ADCH would be locked when it completed.
  int low  = ADCL;
  int high = ADCH;
  // combine the two bytes
  return (high << 8) | low;
}

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Edgar_M

Anyway. I just wanted to notify about the bug. (I've found it when I was looking for the reason why I got fluctuating readings from my temp sensor)

I'm confident that the arduino code maintainers will find an appropriate solution for the delay.

Greets,
Edgar

Jantje

I raised your karma for the effort and feedback.
(And because I also noticed fluctuating results in analog read)
Best regards
Jantje
Do not PM me a question unless you are prepared to pay for consultancy.
Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -


Jantje

Do not PM me a question unless you are prepared to pay for consultancy.
Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -

jimLee

If your only using one analog pin over and over, would the delay issue not be a problem? It seems like it would be ok, but I'm not sure about the vocabulary here.

-jim lee
PNW Ardiuno & Maker club
1012 9Th Street, Anacortes, WA 98221 (Around the back of building)
GITHUB -> https://github.com/leftCoast

westfw

Have you submitted your findings at https://github.com/arduino/Arduino/issues ?
That's pretty much the official channels for bug reports, if you have enough data to be pretty sure it's a bug.
The forums here are more useful for getting consensus if you think there MIGHT be a bug...

Edgar_M

westfw: Ok. Thanks for the info. (By the way: I had a quick look on the github page after I've read your post, but couldn't find the entry)

jimLee: Yes, you're right - if you're always using only one input channel, the bug doesn't show up. Explanation: The last used analog input channel is kept in an internal register of the ADC hardware ("ADMUX"). The Arduino analogRead-command writes the pin-selection to this register every time right before each conversion. But if the pin hasn't changed, it gets only written with the same value, and the ADC hardware doesn't have to switch anything.

Remark: I'm not sure, if this here applies to every different ATmega MCU, since there exist quite a few different MCUs regarding the onboard hardware. I was using an ATmega328P, when I faced this problem


fat16lib

Code: [Select]

ADMUX = (analog_reference << 6) | (pin & 0x0F);

This will not work correctly with Mega.  There is not a bug here.


Code: [Select]

//delay(1);

This is a fix, not a bug. There should not be a delay.  Your problem is a high impedance input.

analogRead() works fine with low impedance inputs.   There should be no delay to slow readings.

All SAR ADCs have a sample and hold capacitor which must be charged by input to the ADC.  High impedance inputs can't quickly charge the sample and hold.

High impedance inputs can be read at high speed by placing an op-amp or instrumentation amplifier before the ADC.

You can read high impedance inputs  at slower speeds by switching the mux with a dummy call to analogRead().
Code: [Select]

  // call to switch mux
  analogRead(pin);

  // wait for S/H to charge
  delay(sampleHoldTime);

  // read value
  v = analogRead(pin);
 

pito

#13
Mar 15, 2013, 09:16 pm Last Edit: Mar 15, 2013, 09:35 pm by pito Reason: 1
As fat16lib wrote the main issue with adc readings is the understanding of the analog S/H part.
The C_S/H capacitor must be fully charged (or discharged) via the external impedance (resistance) in order to get correct readings. So if you are muxing between adc inputs, based on actual source voltage and its impedance, the C_S/H capacitor must get enough time to follow the voltage differences and be stable at the moment of switching off the S/H circuit from the input (see the picture below).
The time is approximately:
t = 5 * ( Rext + Rin )* C_S/H
where
Rext shall be < 10k
Rin = 1..100k (see the datasheet)
C_S/H = 14pF



Edgar_M

Hi, fat16lib!

Thanks for your post. To your first point: Can you please be a little bit more precise with your statement "This will not work correctly with Mega". Why? The ADMUX-register is organized as follows:
|REFS1 | REFS0 | ADLAR |   -   | MUX3 | MUX2 | MUX1 | MUX0 |  (MSB first, ATmega328)

But this is the less important point, anyway. MUX3 will rarely be used in practice.

Much more relevant is the second - bug or not bug ( ;-) ):

I think, you might be right with your theory about the high-impedance input. And that this is the true cause for fluctuating readings.

But if this is the case, then one should think in general about the concept of the Arduino analogRead command. Because what brings us here?

In my view, the purpose of the Arduino commands is to abstract the technically a bit more complex details of configuring the hardware of the various Atmel MCUs away from the user, to make it simply more user-friendly. But abstraction should not be done any further than to ensure proper function in any situation. If this cannot be guaranteed, then more parameters have to be passed through to the user. For instance in our case a second (optional) function parameter might be useful, where the user is able to specify a delay, if needed for high impedance conversions. Or an option, which adds a predefined delay, when present.

Your suggestion works for sure, but without looking behind the scenes, it'll be difficult to comprehend for the plain part-time Arduino programmer

What do you think?

Go Up