ATmega328p analog to digital conversion not responding

Question

The following code does not update initial adcValue. LEDs are working properly with different program, and they also work properly given the initial adcValue. They dont respond to potentiometer adjusting. The delay is there just to make it slower, it also doesnt work without it.

#define F_CPU   1000000UL

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

double adcValue = 700;

void startConversion()
{
    ADCSRA |= (1 << ADSC);
}        
void setupADC()
{
    // AVcc with external capacitor as reference, ADC5 as input
    ADMUX = (1 << REFS0) | (1 << MUX0) | (1 << MUX2);
    // Enabling ADC, acticating interrupts, setting prescaler to 8
    ADCSRA = (1 << ADEN) | (1 << ADIE) | (1 << ADPS0) | (1 << ADPS1) ;
    //disabling digital input buffer
    DIDR0 = (1 << ADC5D);       
    startConversion();
}            
ISR(ADC_vect)
{       
    adcValue = ADC;     
}    
int main(void)
{
    DDRB = 0b00111111;          
    setupADC();     
    sei();          
    while(1)
    {
        _delay_ms(500);
        if( adcValue < 400)
        {
            PORTB = 0b00000000;
        } 
        else if ( adcValue < 800)
        {
            PORTB = 0b00000011;
        } 
        else 
        {
            PORTB = 0b00000111;
        }

    startConversion();
}

}

SOLUTION:

now it works

volatile double adcValue = 700;

adcValue needs to be volatile. It's being updated in an ISR. — yhyrcanus
It also needs to be an integer since AVR doesn't have a FPU. It's not a PC. — Lundin
ADC has type uint16_t making adcValue a double is unnecessary in this case and ill-advised in any event on this platform. — Clifford
If you wish to answer your own question, do so by adding an answer rather then editing the question. A good answer however would explain why the solution works. Bug-fixing is not the role of SO. embedded.com/introduction-to-the-volatile-keyword — Clifford

yhyrcanus yhyrcanus · Accepted Answer · 2021-02-01T16:00:56

adcValue is getting optimized away because the compiler doesn't "see" the variable change in the ISR (it can't tell if the ISR will ever be called). Therefore, it'll just be replaced with a constant. The way to fix this is to mark it volatile, which tells the compiler not to assume anything about the variable and prevents the compiler from optimizing it away.

Now, on top of this, adcValue is a double type. In general, you want to avoid using floating point (and especially double) variables in devices without an FPU. Specifically, the conversion between integer and floating point types takes a LOT of cycles. Like, on the order of hundreds of cycles. You probably don't want this in your ISR.

If you really need the variable to be a float, I suggest doing the conversion outside of the ISR. You'd have two variables, the static volatile uint16_t wAdcValue, and local to where you need a float, you'd assign float fAdcValue = (float) wAdcValue; Note that any floating point manipulation will require more processing and flash usage to handle it.

ATmega328p analog to digital conversion not responding

1 Answers