"If" wrong interpreted when ADC=1023?

Question

My code on ATtiny13A:

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

#include "dbg_putchar.h"

void ADC_init()
{
    // Set the ADC input to PB4/ADC2
    ADMUX |= (1 << MUX1);
    //ADMUX |= (1 << ADLAR);
    ADMUX |= (1 << REFS0);

    // Set the prescaler to clock/128 & enable ADC
    // At 9.6 MHz this is 75 kHz.
    // See ATtiny13 datasheet, Table 14.4.
    ADCSRA |= (1 << ADPS1) | (1 << ADPS0) | (1 << ADEN);
}


int adc_read (void)
{
    // Start the conversion
    ADCSRA |= (1 << ADSC);

    // Wait for it to finish
    while (ADCSRA & (1 << ADSC));

    uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH
    uint8_t high = ADCH; // unlocks both

    int ADC_val = (high<<8) | low;

    return ADC_val;
}

void wyslij_wynik_pomiaru(int wynik)
{
    char str[4];
    itoa(wynik, str, 10);
    dbg_putchar(str[0]);
    if (wynik > 9) dbg_putchar(str[1]);
    if (wynik > 99) dbg_putchar(str[2]);
    if (wynik > 999) dbg_putchar(str[3]);
    dbg_putchar('a');
}


int main(void)
{
    DDRB = _BV(1);
    dbg_tx_init();
    ADC_init();
    int wynik_poczatkowy = adc_read();
    int wynik_nowy;
    while(1)
    {
        wynik_nowy = adc_read();
        if (abs(wynik_poczatkowy-wynik_nowy) > 150) {
            wynik_poczatkowy = wynik_nowy;
            PORTB |= _BV(1); //turn on ESP8266
            _delay_ms(5000);
            wyslij_wynik_pomiaru(wynik_nowy); //send data
            _delay_ms(5000);
            PORTB  &= ~_BV(1); //turn off ESP8266
            _delay_ms(14900);
        }
        _delay_ms(100);
    }
}

When you read the voltage level of the ADC and it is less than 1023, but steady, then everything works correctly: IF loop executes only once, however when the voltage level read on the ADC is equal to 1023, then the IF is executed continuously (condition is still met, but it should not).

Why is this happening and how to fix it?

Answer 1

If ADMUX:ADLAR is set your code will crash & burn when you left shift high << 8, because you are shifting data into the sign bit of "high-promoted-to-int" = undefined behavior. Are you sure ADLAR is not set? You have commented it out. If ADLAR is not set, your code will probably work, by luck.

You need to read up on implicit integer promotion. In this shift operation, the uint8_t variable high will get implicitly promoted to an int, which is a signed int16 (assuming int is 16 bit), which is not what you want or need.

Also, in order to write deterministic, portable programs, get rid of the int type, it should never be used in embedded systems programming. Use uint16_t instead.

Also, (if ADLAR is not set) what makes you think bits 7 to 2 in ADCH contain jolly-good-data? The manual doesn't mention what reading those bits will give you.

To turn your code into production quality, change it in the following ways:

uint16_t adc_read (void)
{
    // Start the conversion
    ADCSRA |= 1 << ADSC;

    // Wait for it to finish
    while (ADCSRA & (1 << ADSC))
        ; // semicolon on separate line to show that this was intentional


    uint8_t low  = ADCL; // must read ADCL first - it then locks ADCH
    uint8_t high = ADCH; // unlocks both

    const uint8_t ADCH_DATA_MASK = 0x03; // the two MSB of 10 bit ADC value
    high = high & ADCH_DATA_MASK; // mask out data, discard junk

    return (uint16_t)high << 8 | (uint16_t)low;
}

Your main() code needs to be adapted to support the above changes. In main(), you may have to convert the uint16_t to int16_t for your algorithm to work.

Answer 2

This is just a hunch, but have you tried using unsigned ints for your ADC values and doing the comparison in a different way? Since 1023 is a power of two minus one, I suspect there's an arithmetic issue. Try something like this:

if (wynik_poczatkowy > wynik_nowy + 150 || wynik_nowy > wynik_poczatkowy + 150)
{
    ...
}