1
\$\begingroup\$

I am currently getting into AVR programming and I want to read in two values from two potentiometers on the analog inputs PB3/ADC3 PB4/ADC2. I want to (just for now) light an two LEDs (PB1 and PB2) when the analog input value is above (arbitrary upper limit) 900.

Here is the code with which I try to do it.

  #include <avr/io.h>
  #include <stdint.h>
  #include <util/delay.h>
//1MHz clock
#define F_CPU 1000000

uint8_t channelPB4 = 0x2;
uint8_t channelPB3 = 0x3;

int main(void){
    uint16_t resultAdc1 = 0, resultAdc2 = 0;

    //ADC enable
    ADCSRA |= (1<<ADEN);
    //set prescaler to 128 in order to be in recommended range
    ADCSRA |= (1<<ADPS1) | (1<<ADPS0) | (1<<ADPS2);
    //read value empty
    ADCSRA |= (1<<ADSC);
    while (ADCSRA & (1<<ADSC) ) {}
    //dummy readout
    (void) ((ADCH <<8 ) | ADCL);


    DDRB |= (1<<PB1) | (1<<PB2);


    while(1){
        ADMUX = (ADMUX & 0xf8) | channelPB3;
        //start conversion
        ADCSRA |= (1<<ADSC);
        //wait for completion of conversion
        while(ADCSRA & (1<<ADSC)){}
        resultAdc1 = (ADCH<<8) | ADCL;

        ADMUX = (ADMUX & 0xf8) | channelPB4;
        //start conversion
        ADCSRA |= (1<<ADSC);
        //wait for completion of conversion
        while(ADCSRA & (1<<ADSC)){}
        resultAdc2 = (ADCH<<8) | ADCL;

        if (resultAdc1 > 900){
            PORTB |= (1<<PORTB1);
        }else if(resultAdc1 < 900){
            PORTB &= ~(1<<PORTB1);
        }
        if(resultAdc2 > 900){
            PORTB |= (1<<PORTB2);
        }else if(resultAdc2 < 900){
           PORTB &= ~(1<<PORTB2);
        }
        _delay_ms(500);
    }

    return 0;
}

But what I get is, that my LEDs are constantly on. Even when I remove the potentiometer from the analog input and wire the analog input to ground. My reference is VCC, so ground should be 0 on the range from 0 to 1023.

Can you spot my mistake?

UPDATE:Schematic (sorry it is not very tidy)

schematic

simulate this circuit – Schematic created using CircuitLab

\$\endgroup\$
7
  • \$\begingroup\$ Why did you switch from PB1/2 to PORTB1/2? \$\endgroup\$ Oct 15 '16 at 20:52
  • \$\begingroup\$ I thought it was easier to read in the long run, because when I use PORTB1/2 it is together with the PORTB register. \$\endgroup\$
    – elhe
    Oct 15 '16 at 20:54
  • \$\begingroup\$ I forgot if this is actually relevant, but anyway: maybe the ADC pins are actually in output mode, try explicitly configuring them as inputs (simply replace DDRB |= by DDRB =. Also, a schematic might help; it provides a bit more context, and maybe the problem is actually in your wiring. \$\endgroup\$
    – marcelm
    Oct 15 '16 at 22:08
  • \$\begingroup\$ Added the schematic. The data sheet states, that the DDRB register has default values of 0, so not setting the DDRB on the analog input pins (as I did) should have them configured as inputs. \$\endgroup\$
    – elhe
    Oct 16 '16 at 10:11
  • \$\begingroup\$ (ADCH<<8) | ADCL; is wrong. For 16bit registers on 8bit AVRs, you need to read the low register first in order to move the value of the high register into a temporary access register. You should do something like: uint8_t low = ADCL; uint8_t high = ADCH; resultAdc2 = (high<<8) | low;. Reading the high register first discards the conversion result. \$\endgroup\$ Oct 16 '16 at 10:26
2
\$\begingroup\$

Your issue is down to the way in which you are accessing the ADC register. 8bit AVRs only have an 8bit data path, which means accesses to 16bit registers must be performed in two stages, which by design require a specific sequence.

Basically you need to read the low register first which will at the same time either move the value of the high register into a temporary access register (for timers usually), or lock the value of the high register to allow subsequent access. This process is to prevent corrupting of the value if it changes between reading the low and high bytes.

Your access is done using the line resultAdc2 = (ADCH<<8) | ADCL; which is incorrect as the compiler will convert that into first reading the high register and then reading the low register. Instead you should do something like:

uint8_t low = ADCL;
uint8_t high = ADCH; 
resultAdc2 = (high<<8) | low;. 

Furthermore your problem is made worse due to the way the ADC result register access works. From Page 137 of the datasheet, there is the following quote:

"When ADCL is read, the ADC Data Register is not updated until ADCH is read."

What this means is that if you read ADCH first, you discard the value of ADCL. This is not necessarily an issue, but it will corrupt the result unless you have ADLAR set to 1.

However, what it also means is that if you then proceed to read ADCL, it will lock the data register until you read ADCH. The reading of ADCH is not done until the next loop which means that in the mean time the next ADC conversion result cannot be written to the ADC register, meaning that in your code the value of the register never updates.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for this elaborate explanation. This solved my problem. ->Answer accepted \$\endgroup\$
    – elhe
    Oct 16 '16 at 11:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.