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){

//set prescaler to 128 in order to be in recommended range

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

while(1){
//start conversion
//wait for completion of conversion

//start conversion
//wait for completion of conversion

PORTB |= (1<<PORTB1);
PORTB &= ~(1<<PORTB1);
}
PORTB |= (1<<PORTB2);
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

• Why did you switch from PB1/2 to PORTB1/2? Oct 15 '16 at 20:52
• I thought it was easier to read in the long run, because when I use PORTB1/2 it is together with the PORTB register.
– elhe
Oct 15 '16 at 20:54
• 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. Oct 15 '16 at 22:08
• 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.
– elhe
Oct 16 '16 at 10:11
• (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. Oct 16 '16 at 10:26

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;