ATtiny85 analog input constantly on max

Question

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)

^{simulate this circuit – Schematic created using CircuitLab}

Answer 1

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.