5
\$\begingroup\$
#include <avr/io.h>
#include <util/delay.h>

void init(){
    // LED output.
    DDRB |= 1<<4;
    // Turn on ADC on third pin, continuous mode, prescaler 128.
    ADMUX |= (1<<MUX0) | (1<<MUX1);
    ADCSRA |= (1<<ADEN) | (1<<ADSC) | (1<<ADATE) | (1<<ADPS2) | (1<<ADPS1) | (1<<ADPS0);
}

int main(){
    init();

    uint16_t adc = 0;
    while(1){
        adc = (ADCH << 8) | ADCL;
        // adc = (adc + 1) & 1023;
        // _delay_ms(1);

        if(adc > 512){
            PORTB |= 1<<4;
        }
        else{
            // PORTB = 0;
            PORTB &= ~(1<<4);
        }
    }
}

This is the code I have running on my ATtiny13A. There is an LED connected to pin B4, and potentiometer to B3. The code is supposed to turn on the LED if the analog value is greater than 512 (half of maximum 1024). It doesn't work though, and LED seems to be disabled all the time.

Normally I would assume there is something wrong in configuration of peripherals, but there are some very strange things happening. For example, if I change PORTB &= ~(1<<4); to PORTB = 0 (which in this case should do exactly the same thing), the code magically starts working just as expected. Another thing I tried is changing LED status using simple counter by uncommenting the two lines near _delay_ms - and again, LED starts blinking as expected.

The only time when this code does not work is when it's in exactly this form - so there must be something wrong with combination of ADC reading and output port bit writing.

Why does this happen?

\$\endgroup\$
  • \$\begingroup\$ One more thing - even with the code as shown, it seems as though the LED could be turned on ONE time - so if the potentiometer started low (with LED off), I could turn LED on, then off, but the next time I tried turning potentiometer, it fails. This is so weird... \$\endgroup\$ – akrasuski1 Nov 27 '17 at 1:57
  • \$\begingroup\$ OK, I have a good lead - if I use PORTB = 0 (the line that worked), but then add a single nop afterwards, it gets broken again. It appears to be a timing issue for some reason - adding 4 nops fixes it again... \$\endgroup\$ – akrasuski1 Nov 27 '17 at 2:27
11
\$\begingroup\$
  1. Incorrect 16 bit register access pattern

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

In C, the evaluation order of the | operator is undefined, so this code is could be reading ADCH first, and then reading ADCL. Once you read ADCL then you have locked the data register until the next pass though the loop, which is certainly not what you want.

Try changing this line to...

adc = ADC;

(the C compiler should generate the correct access order coding).

  1. Reading ADC while it is being updated.

This diagram strongly suggests that the ADC register is updating up until ADIF goes high...

enter image description here

I'd expect that new bits are shifted into place with each successive approximation, and the extra delay of the nop could be enough so that the ADC is ready by the time you hit it just by chance.

Try replacing...

adc = (ADCH << 8) | ADCL;

...with...

while (! ADCSRA & _BV(ADIF) );
adc = ADC;
ADCSRA |= _BV( ADIF );

...and see if that resolves the timing issue.

Report back if this fixes the problem. If not, we will move on to next try!

\$\endgroup\$
  • \$\begingroup\$ Aha, that solved the problem. Good catch! The generated assembly indeed read ADCH first (operation order is generally not guaranteed by C though). \$\endgroup\$ – akrasuski1 Nov 27 '17 at 2:46
  • 6
    \$\begingroup\$ "In C, [...] the | operator is evaluated left to right" - this is not correct. The evaluation order is, in fact, unspecified (unlike, say, for ||, where it is indeed left-to-right to permit short-circuit evaluation). \$\endgroup\$ – NPE Nov 27 '17 at 9:21
  • \$\begingroup\$ I didn't check the datasheet for now, but does checking ADIF work when the ADC is in free-running mode? It starts a new conversion right after the previous one, so either the data register has to be fully buffered, or it should be unreadable at all times... \$\endgroup\$ – ilkkachu Nov 27 '17 at 13:08

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.