My ADC settings appear to correspond to wrong pins

I’m trying to use the ADC on an Arduino UNO by setting registers instead of using analogRead. (If you care about why, I’ll post that in a few minutes). The odd thing is that setting the MUX bits on the ADMUX register should be the simplest part, but I’m getting redings from the wrong pin.

Bottom line: it looks as if:

  • A5 is doing nothing
  • A4 is setting channel 3
  • A3 is setting both channel 4 and 5

I am trying to read from three of the pins, A3, A4, and A5. I’m connecting each to a temperature sensor (TMP36) in turn and reading the Serial output. I’m expecting the connected pin to register 144 or 145 because that’s what I get with analogRead(), but these are my results (each column represents the readings of the three Analog pins when the sensor is connected to the pin indicated in the first row):

n/a A3 A4 A5
A3 [332-337] [222-223] [143-144] [255-256]
A4 [332-337] [143-144] [235-237] [288-289]
A5 [332-337] [143-144] [246-250] [229-230]

In the first column, I have no pins plugged into the sensor (for a control reading). In the second column, A3 is connected to the sensor; in the third column, A4 is connected to… etc.

I’m sure the problem is really my code. Can anyone help spot the error? (Just 39 lines.)

#include <stdint.h>
#include <avr/io.h>
#include <util/delay.h>

#define CONTROL_ADMUX (0b00000101)
#define ICE_ADMUX     (0b00000011)
#define FIRE_ADMUX    (0b00000100)

uint16_t readTemp(uint8_t admux)
{
  uint16_t reading;
  ADMUX  &= 0b11110000; // clear the input channel selection 
  ADMUX  |= admux;      // input channel selection
  ADCSRA |= (1<<ADIF);  // write 1 to clear flag
  ADCSRA |= (1<<ADSC);  // start conversion on ADC
  while(! ADCSRA & (1<<ADIF) ) {} // wait until flag is set
  reading = ADCL;         // read low byte from ADC
  reading += (ADCH << 8); // read high byte from ADC
  return reading;
}

void setup() 
{
  // configure ADC
  ADMUX  = (1<<REFS0);  // AVCC with external capacitor at AREF pin
  ADCSRA = 0b10010111; // full prescaler, enable ADC, set ADIF flag to clear it
  // Serial setup
  Serial.begin(9600);
}
void loop()
{
    uint16_t c, i, f;
    c = readTemp(CONTROL_ADMUX);
    i = readTemp(ICE_ADMUX);
    f = readTemp(FIRE_ADMUX);
    Serial.println(c);
    Serial.println(i);
    Serial.println(f);
    Serial.println("=====");
    delay(1000);
}

So the reason that I am trying to use the ADC directly is that this code is eventually actually supposed to go on attiny85 (sans the Serial and sans Arduino code entirely).

Yes, I realize that the register settings will not be exactly the same on a Tiny85, but it looks like the only thing that needs to change is the specification of the voltage reference. On the Tiny, I think it should be 0b00000000.

Vaselinessa: So the reason that I am trying to use the ADC directly is that this code is eventually actually supposed to go on attiny85...

Get the code to work on your Uno using the Arduino API and then use this core for the ATtiny85... http://code.google.com/p/arduino-tiny/

After much rebuilding of code, I think I found my problem: parentheses! And I replaced the empty curly braces ({}) with an empty semicolon (:wink: to be safe. (How do I disable smilies in my posts?)

uint16_t readTemp(uint8_t admux)
{
  uint16_t reading;
  ADMUX  &= 0b11110000; // clear the input channel selection 
  ADMUX  |= admux;      // input channel selection
  ADCSRA |= (1<<ADIF);  // write 1 to clear flag
  ADCSRA |= (1<<ADSC);  // start conversion on ADC
  while(! (ADCSRA & (1<<ADIF)) ) ; // wait until flag is set
  reading = ADCL;         // read low byte from ADC
  reading += (ADCH << 8); // read high byte from ADC
  return reading;
}

[quote author=Coding Badly link=topic=203421.msg1498455#msg1498455 date=1386478613] Get the code to work on your Uno using the Arduino API and then use this core for the ATtiny85... http://code.google.com/p/arduino-tiny/ [/quote]

Not acceptable to me, I afraid. The Arduino core takes up too much space for a number of my projects, so it's important to me to figure out these issues. (I have in some cases made use of the tiny arduino core, however.)

Vaselinessa:
After much rebuilding of code, I think I found my problem: parentheses! And I replaced the empty curly braces ({}) with an empty semicolon (:wink: to be safe. (How do I disable smilies in my posts?)

Click on “Additional Options”. That’s one of them.

Not (!) is higher precedence than bitwise and (&), that was your problem.

Not acceptable to me, I afraid. The Arduino core takes up too much space for a number of my projects…

Same here. (Which is why my attention is on version 2.)

This…

  reading = ADCL;         // read low byte from ADC
  reading += (ADCH << 8); // read high byte from ADC

…is a very bad choice. Use ADC instead.

Why? The datasheet says:

Otherwise, ADCL must be read first, then ADCH, to ensure that the content of the Data Registers belongs to the same conversion.

Because the compiler is free to put the reads in either order when it optimizes. For example, the compiler may prefer this (pseudo-code)…

{temporary register pair 1} = ADCH
{temporary register pair 1} = {temporary register pair 1} << 8
{temporary register pair 2} = ADCL;
{temporary register pair 2} = {temporary register pair 1} + {temporary register pair 2}

ADC is guaranteed by the compiler folks to perform the read correctly.

Where is that documented?

I'll need some time to find it (if I still can). I believe I found it in pieces (I think the guarantee was on nongnu.org and the potential optimization problem was described on avrfreaks.net).

The Atmega328 datasheet says:

Note that when using “C”, the compiler handles the 16-bit access.

But that’s just an assertion.

Let’s put it this way, accessing the bytes individually you rely on the compiler not re-ordering the accesses. Accessing the 16-bit value you rely on the compiler generating the accesses in the “correct” order. Which it seems to do, I admit:

000000be <loop>:
  be:	60 91 78 00 	lds	r22, 0x0078  // ACDL
  c2:	70 91 79 00 	lds	r23, 0x0079  // ACDH

But without something in writing, you could argue that it just happens to get it right, this particular time.

I would have thought that, with volatile variables (like ACDL, ACDH) the compiler would be required to access them in the order presented in the code. Otherwise all sorts of timing-dependent code would fail. Code that requires that you write to registers in a particular order, for example:

  // turn off brown-out enable in software
  MCUCR = bit (BODS) | bit (BODSE);  // turn on brown-out enable select
  MCUCR = bit (BODS);        // this must be done within 4 clock cycles of above

The compiler cannot (successfully) re-order or optimize such code. For example it might think “oh, I don’t need to do two instructions here as the the second one is basically changing what is written by the first one”. So I think that any attempt to “optimize” volatile registers (usually hardware registers) would be fraught with peril.