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:
ADMUX = (analog_reference << 6) | (pin & 0x07);
should be:
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:
//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!
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 ?
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,
//
// 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;
}
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.
Have you submitted your findings at Issues · arduino/Arduino · GitHub ?
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...
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
This will not work correctly with Mega. There is not a bug here.
//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().
// call to switch mux
analogRead(pin);
// wait for S/H to charge
delay(sampleHoldTime);
// read value
v = analogRead(pin);
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
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
fat16lib:
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.
I couldn't help but notice this generalization. An SAR ADC does not have to have a track and hold amplifier in order to be an SAR ADC. Early chip ADC's had separate SHA's and in fact such IC's are still in use and production. I'm not trying to start an argument, just pointing out a fact that has maybe fallen through the crack due to the rapid rise of technology.
):