How can the average of the ADC Readings be calculated?

Question

The aim is to store the newest 10 ADC readings in an array and then calculate the average of them to be used elsewhere. Removing the oldest each time it is updated.

Regarding the LED timing, it must switch timing from 1s to 0.25s if ADC reading is within boundaries as written below, how can this be implemented correctly? I know my method works but can be done better. As for the LED's they must change patterns if a switch is pressed as you can see, which they do, but yet again I'm sure it can be done another simpler way!

Below is my code, Also I'm sure there are many an error and plenty of room for optimization, I will gladly accept it all!

#include <avr/io.h>
#define F_CPU 16000000UL
#include <util/delay.h>

#include <avr/io.h>
#include <avr/interrupt.h>
unsigned int timecount0;


unsigned int adc_reading;

volatile uint32_t timing = 1;
volatile uint32_t accumulator = 0;
volatile uint16_t average = 0;
volatile uint16_t samples = 0;


#define LED_RED PORTB = ((PORTB & ~0b00001110)|(0b00000010 & 0b00001110))
#define LED_GREEN PORTB = ((PORTB & ~0b00001110)|(0b00001000 & 0b00001110))
#define LED_BLUE PORTB = ((PORTB & ~0b00001110)|(0b00000100 & 0b00001110))
#define LED_RGB PORTB = ((PORTB & ~0b00001110)|(0b00001000 & 0b00001110))

#define DELAY_COUNT 6

volatile uint8_t portdhistory = 0xFF;


void Timer0_init(void)
{
    timecount0 = 0; // Initialize the overflow count. Note its scope
    TCCR0B = (5<<CS00); // Set T0 Source = Clock (16MHz)/1024 and put Timer in Normal mode

    TCCR0A = 0;         // Not strictly necessary as these are the reset states but it's good
    // practice to show what you're doing
    TCNT0 = 61;         // Recall: 256-61 = 195 & 195*64us = 12.48ms, approx 12.5ms
    TIMSK0 = (1<<TOIE0);    // Enable Timer 0 interrupt


    PCICR |= (1<<PCIE0);
    PCMSK0 |= (1<<PCINT0);
    sei();              // Global interrupt enable (I=1)

}


void ADC_init(void)
{
    ADMUX = ((1<<REFS0) | (0<<ADLAR) | (0<<MUX0));  /* AVCC selected for VREF,ADLAR set to 0, ADC0 as ADC input (A0)  */
    ADCSRA = ((1<<ADEN)|(1<<ADSC)|(1<<ADATE)|(1<<ADIE)|(7<<ADPS0));
                                        /* Enable ADC, Start Conversion, Auto Trigger enabled, 
                                           Interrupt enabled, Prescale = 32  */
    ADCSRB = (0<<ADTS0); /* Select AutoTrigger Source to Free Running Mode 
                            Strictly speaking - this is already 0, so we could omit the write to
                            ADCSRB, but included here so the intent is clear */
    sei(); //global interrupt enable
}


int main(void)
{
    ADC_init();
    Timer0_init();


    DDRD = 0b00100000;  /* set PORTD bit 5 to output  */
    DDRB = 0b00111110;  /* set PORTB bit 1,2,3,4,5 to output  */


    sei();              // Global interrupt enable (I=1)


    while(1)
    {
        if(!(PIND & (1<<PIND2)))
        {
            PORTD = PORTD |= (1<<PORTD5);
            PORTB = PORTB |= (1<<PORTB4);
            if(average>512)
            {
                PORTB = PORTB |= (1<<PORTB5);
            }

        }
        else
        {

            PORTD = PORTD &= ~(1<<PORTD5);
            PORTB = PORTB &= ~(1<<PORTB4);
        }




    }

}

ISR(TIMER0_OVF_vect)
{

        TCNT0 = 61;     //TCNT0 needs to be set to the start point each time
        ++timecount0;   // count the number of times the interrupt has been reached


        if(!(PIND & (1<<PIND3)))
        {           

        if (timecount0 >= 0)    // 40 * 12.5ms = 500ms
        {
            PORTB = ((PORTB & ~0b00001110)|(0b00000000 & 0b00001110));
        }

        if (timecount0 >= 8*timing) 
        {
            LED_RED;
        }

        if (timecount0 >= 16*timing)    
        {
            LED_GREEN;
        }

        if (timecount0 >= 24*timing)    
        {
            PORTB = ((PORTB & ~0b00001110)|(0b00000110 & 0b00001110));


        }
        if (timecount0 >= 32*timing)    
        {
            PORTB = ((PORTB & ~0b00001110)|(0b00001000 & 0b00001110));


        }
        if (timecount0 >= 40*timing)    
        {
            PORTB = ((PORTB & ~0b00001110)|(0b00001010 & 0b00001110));          

        }

        if (timecount0 >= 48*timing)    
        {
            PORTB = ((PORTB & ~0b00001110)|(0b00001100 & 0b00001110));      



        }

        if (timecount0 >= 56*timing)    
        {
            PORTB = ((PORTB & ~0b00001110)|(0b00001110 & 0b00001110));  


        }

        if (timecount0 >= 64*timing)
        {

            timecount0 = 0;

        }

        }
        else
        {
            if (timecount0 >= 0)
            {

                PORTB = ((PORTB & ~0b00001110)|(0b00000000 & 0b00001110)); //ALL OFF
            }

            if (timecount0 >= 8*timing) 
            {
                LED_RED;
                //PORTB = ((PORTB & ~0b00001110)|(0b00000010 & 0b00001110)); //RED
            }

            if (timecount0 >= 16*timing)    
            {
                LED_GREEN;


            }

            if (timecount0 >= 24*timing)    
            {
                LED_BLUE;



            }
            if (timecount0 >= 32*timing)
            {

                timecount0 = 0;

            }
        }           

}

ISR (ADC_vect)  //handles ADC interrupts

{

    adc_reading = ADC;   //ADC is in Free Running Mode
    accumulator+= adc_reading;


    if ((adc_reading > 768) & (adc_reading <= 1024))
    {
        timing = 10;

    }

    if ((adc_reading >= 0) & (adc_reading<= 768) )
    {
        timing = 2.5;

    }


    samples++;

    if(samples == 10)
    {
        average = accumulator/10;
        accumulator = 0;
        samples = 0;
    }


}

Get all that slow math out of the ISR. Save the value and return to the main loop. — TomServo, Mar 27 '20 at 22:00

score 5 · Accepted Answer · answered Mar 27 '20 at 04:47

Depending on your processors, you may way to keep ISR() speedy and avoid expensive /,%.

The LED stuff, I'd handle in a timer interrupt.

#define N 10
volatile unsigned sample[N];
volatile unsigned count = 0;
volatile unsigned index = 0;
volatile unsigned sum = 0;

ISR (ADC_vect)  {
  if (count >= N) {
    sum -= sample[index];
  } else {
    count++;
  }
  sample[index] = ADC;
  sum += sample[index];
  index++;
  if (index >= N) {
    index = 0;
  }
}

unsigned ADC_GetAvg(void) {
  block_interrupts();
  unsigned s = sum;
  unsigned n = count;
  restore_interrupts();
  if (n == 0) {
    return 0; //ADC ISR never called
  }
  return (s + n/2)/n;  // return rounded average
}

I'd recommend an integer version of a low pass filter than the average of the last N.

score 3 · Answer 2 · answered Mar 27 '20 at 16:06

In terms of the moving averaging w/ N = 10, chux - Reinstate Monica has provided the solution. Chux - Reinstate Monica also recommends looking at an integer version of a low pass filter. I personally like the Exponentially Weighted Moving Average (EWMA) because it's fairly simple to code and only requires a few values to do the averaging. This is compared to having to hold 10 in array in your case. I would recommend Elliot Williams's Make: AVR Programming Chapter 12 for this. In case you don't have access to this readily, the EWMA, as explained in Make AVR, starts with

y_current = (1/16)*x_current + (15/16)*y_previous

where in our case, y_current is the updated EWMA value, x_current is the newest sample from your ADC, and y_previous is the last EWMA value. The choice of 16 can also be changed along with the weights, 1 and 15. Keeping it a power of 2 is important though, as you will see. As shown in Elliot Williams book, you multiply by 16 and compensate for rounding problems and get the following,

16*y_current = x_current + 16*y_previous - (16*y_previous - 8)/16.

Now, I know this looks ugly but what we have is scaled by 16 average value that's an integer and only relies on integer addition (the 16*y_previous is stored as one value so you don't do the multiplication) and a bit shift; that's the reason why a power of 2 was chosen in the EWMA, dividing by 16 is the same as a right bit shift of 4. Ok, so what does this average look like in code:

// Snippet from Make: AVR Programming
uint16_t x_current; // ADC value.
uint16_t y_current; // Average ADC value.
// Get the EWMA.
y_current = x_current + y_current - ((y_current - 8) >> 4);
// Send the value over USART (assuming it's wired up). Remember that
// y_current is scaled by 16.
printf("%d\n",(y_current>>4));

The above is just the EWMA that you can use in your code and an example of sending it, which is just a reminder that the value if scaled. Remember, this is just the averaged ADC value. Likely you will be wanting to use the ADC value as an input to a function to get the value of some measured quantity. Rather than actually using a function and calculating values, you can create a look-up table where the index is the ADC value and the array entry at that index is the precalculated value.

In terms of your other code, the things that could be corrected/streamlined are in your ISRs. In ISR(TIMER0_OVF_vect) you have some bit operations that are constant and can be precalculated so that your not doing it everytime the ISR(TIMER0_OVF_vect) fires.

PORTB = ((PORTB & ~0b00001110)|(0b00000000 & 0b00001110));

becomes

PORTB = ((PORTB & 0b11110001)|(0b00000000)); // Saves a bit inversion and '&'

which shows that your ORing, |, doesn't affect the result, because you're ORing against all zeros.

Finally, in your ISR (ADC_vect) you are using the bitwise, &, and not the logical and, &&. You get the same result but it's like using a wrench to hammer in a nail. I know this is a lot but I hope it helps and let me know if you need clarification.

How can the average of the ADC Readings be calculated?

2 Answers2