Calculating a running average

Question

The running average calculated from circular array produced constant < 0 average when average should always be between 0 and 1.

I am writing firmware for an MSP430 device that uses LEDs and photodiodes to detect specific types on ink. The device scans at about 155us and the samples under the scanner range from velocities of .1m/s to 3.3m/s. The goal of the device is to test for ink and measure the ink (pass) to test (not pass) ratio and turn on a green LED when the ratio is between the corresponding value and turn on a red LED when it is not. I am using static integer arrays to store the values of consecutive passes and test values to the same index number of each array. After the last index of the array, the index is set back to zero and the old values are written over.

GREEN_LED_ON; and similar definitions are port definitions for my MCU and are verified to be correct.

event is the test result. If ink is detected, event=DETECTED and vice versa

test will be the average set by a GUI, but for now it is nothing because I don't have this part of my function working

Normally I will have the variable average set by an accompanying GUI, but for testing purposes I set average<0 just to figure out why the LEDs were coming on at the wrong time and I found that I am getting an average<0. Average should always be 0=

Notes:

• I've tried checking if various sections of the code are working by using the LED outputs. I commented out the average section that controls the LEDs and verified that the event== part works by turning on and off the LEDs. Then I tried putting that code in the flag== section and the LEDs also corresponded with that section as expected.
• I found a problem with the array value. Once n > size it only tests once instead of waiting for the next flag change. I can't have n>size because further down in my code, n++ will equal 7, which is out of bounds

I added a section of code to make sure rollover doesn't occur. The values 8,000 and 30,000 were chosen to match the slowest possible running speed.

I also changed where the array index increments and made sure it says within bound of the array.

Here is the updated function:

void display(char event, char test) {

static int size=5;
static int array[6]={0};  //array with number of passes for each n
static int n=0;
static float sum=0;//total number of passes
static float average=0;//average pass rate over n
static int consecpass=0; //consecutive passes
static int consecfail=0; //consecutive fails
static int totalnumberoftests[6]={0}; //total number of tests conducted.  Counts the number of passing or failing tests for the nth value
static float counter=1; //used to count the total number of tests
static int flag=0;


    if (event == DETECTED)
    {
        if (flag==0)
        {

            sum=sum-array[n];
            counter=counter-totalnumberoftests[n];
            array[n]=0;
            totalnumberoftests[n]=consecfail;
            sum=sum+array[n];
            counter=counter+totalnumberoftests[n];

            flag=1;
            consecpass++;

            n++;
            if(n>=size)n=0;
            //GREEN_LED_ON;
            //RED_LED_OFF;
        }else{

        consecfail=0;
        consecpass++;
        //GREEN_LED_ON;
        //RED_LED_OFF;

        }

    } if (event==NOT_DETECTED){

        if(flag==1)
        {

            sum=sum-array[n];
            counter=counter-totalnumberoftests[n];
            array[n]=consecpass;
            totalnumberoftests[n]=consecpass;
            sum=sum+array[n];
            counter=counter+totalnumberoftests[n];

            flag=0;
            consecfail++;

            n++;
            if(n>=size)n=0;
            //RED_LED_ON;
            //GREEN_LED_OFF;
        }else{


        consecpass=0;
        consecfail++;
        //RED_LED_ON;
        //GREEN_LED_OFF;


        }
    }

    if (consecpass>8000)
    {
        sum=sum-array[n];
        counter=counter-totalnumberoftests[n];
        array[n]=consecpass;
        totalnumberoftests[n]=consecpass;
        sum=sum+array[n];
        counter=counter+totalnumberoftests[n];
        consecpass=0;
        n++;
        if(n>=size)n=0;
    }

    if(consecfail>30000)
    {
        sum=sum-array[n];
        counter=counter-totalnumberoftests[n];
        array[n]=0;
        totalnumberoftests[n]=consecfail;
        sum=sum+array[n];
        counter=counter+totalnumberoftests[n];
        consecfail=0;
        n++;
        if(n>=size)n=0;
    }

    average=sum/counter;

    if(average<.6 && average > .1)
    {
        GREEN_LED_ON;
        RED_LED_OFF;
    }else{
        GREEN_LED_OFF;
        RED_LED_ON;
    }


}

if (n >= size) statement to AFTER the flag statements to avoid having the final values of my arrays be 1. Here is the change (it is on both if(flag==) statements:

if (flag == 1) {
    sum = sum - array[n];
    counter = counter - totalnumberoftests[n];
    array[n] = consecpass;
    totalnumberoftests[n] = consecpass;
    sum = sum + array[n];
    counter = counter + totalnumberoftests[n];

    flag = 0;
    consecfail++;

    n++;
    if (n >= size)
        n = 0;

Here is the original code:

void display(char event, char test) {

    static int size = 6;
    static int array[6] = { 0 };  //array with number of passes for each n
    static int n = 0;
    static float sum = 0;//total number of passes
    static float average = 0;//average pass rate over n
    static int consecpass = 0; //consecutive passes
    static int consecfail = 0; //consecutive fails
    static int totalnumberoftests[6] = { 0 }; //total number of tests conducted.  Counts the number of passing or failing tests for the nth value
    static float counter = 1; //used to count the total number of tests
    static int flag = 0;    

    if (n >= size) {    
        n = 0;    
    }

    if (event == DETECTED) {
        if (flag == 0) {
            n++;
            sum = sum - array[n];
            counter = counter - totalnumberoftests[n];
            array[n] = 0;
            totalnumberoftests[n] = consecfail;
            sum = sum + array[n];
            counter = counter + totalnumberoftests[n];

            flag = 1;
            consecpass++;               
        } else {    
            consecfail = 0;
            consecpass++;               
        }    
    } 

    if (event == NOT_DETECTED) {    
        if (flag == 1) {
            n++;
            sum = sum - array[n];
            counter = counter - totalnumberoftests[n];
            array[n] = consecpass;
            totalnumberoftests[n] = consecpass;
            sum = sum + array[n];
            counter = counter + totalnumberoftests[n];

            flag = 0;
            consecfail++;               
        } else {        
            consecpass = 0;
            consecfail++;               
        }
    }

    average = sum / counter;

    if (average < 0) {
        GREEN_LED_ON;
        RED_LED_OFF;
    } else {
        GREEN_LED_OFF;
        RED_LED_ON;
    }        
}

Answer 1

You have UB.

At the top you do if (n>=size) n = 0;. This still allows n to be size-1

But, further down you do n++ before accessing the arrays. This would allow you to access array[size], which is UB.

I believe you want to move the n++ to the bottom of your if sections, after doing the calculations. And, you might want to combine both with if (++n >= size) n = 0;

---

UPDATE:

I set size=5 and I still get negative numbers. The arrays are both within the boundary now correct?

Possibly. But, a few things.

Only average needs [or should be] a float.

You have "parallel" arrays. I'd use a struct instead.

I've been over your logic a few times and I'm still not sure what you're trying to achieve.

So, I refactored to try to remove any more possible trivial errors.

I'm a little suspicious of the subtract-and-then-add-back for the counters, particularly at the switch over point. This is only place that you could get negative numbers.

Maybe inserting the function into a unit test program that simulates various lengths of ink/no ink stream data would help. You can then breakpoint things. This can test/verify your logic without the pain of having to use the live system (i.e. the unit test program is an ordinary app that you can run on your PC). You can conditionally compile in/out the LED code vs. the "abort on negative value" as mentioned below.

Anyway, here's my take on a simplification that may help clarify [please pardon the gratuitous style cleanup]:

// NOTE: in the long term, an int could wrap to a negative -- adjust this
// as needed
typedef long long ctr;

struct event {
    ctr testcount;
    ctr passcount;
};

// NOTE: moved this outside function to guarantee all values are zero at start
#define EVENTMAX    6
struct event events[EVENTMAX];

void
display(char event, char test)
{
    static int n = 0;
    static ctr sum = 0;             // total number of passes
    static double average;          // average pass rate over n
    static ctr consecpass = 0;      // consecutive passes
    static ctr consecfail = 0;      // consecutive fails
    static ctr testcount = 1;       // used to count the total number of tests
    static int flag = 0;
    struct event *cur;

    cur = &events[n];

    if (event == DETECTED) {
        if (flag == 0) {
            sum -= cur->passcount;
            testcount -= cur->testcount;

            cur->passcount = 0;
            cur->testcount = consecfail;

            sum += cur->passcount;
            testcount += cur->testcount;

            flag = 1;

            if (++n >= EVENTMAX)
                n = 0;
        }
        else {
            consecfail = 0;
        }

        consecpass++;
    }

    if (event == NOT_DETECTED) {
        if (flag == 1) {
            sum -= cur->passcount;
            testcount -= cur->testcount;

            cur->passcount = consecpass;
            cur->testcount = consecpass;

            sum += cur->passcount;
            testcount += cur->testcount;

            flag = 0;

            if (++n >= EVENTMAX)
                n = 0;
        }
        else {
            consecpass = 0;
        }

        consecfail++;
    }

    // add code to abort program if _either_ sum _or_ testcount is negative
    // here ...

    average = sum;
    average /= testcount;

    if (average < 0) {
        GREEN_LED_ON;
        RED_LED_OFF;
    }
    else {
        GREEN_LED_OFF;
        RED_LED_ON;
    }
}

---

UPDATE #2:

I found that one of the problems was roll over. I fixed this with an if statement that stores values if they get higher than 8,000 or 30,000. When I'm in debug mode, the values of the arrays look as I expect them to, but I occasionally get negative numbers in sum and counter. How could the wrong indexes be getting subtracted?

You could be subtracting from a "stale" entry even if the index is valid. I realize you're trying to an RLE impl.

A guess ...

You change the "current" entry, but after the ++n, the "other" side might get the "next" entry when it needs the previous entry.

That is, you subtract array[0], apply stuff, set array[0] and then increment n.

The other side is now subtracting array[1], when it needs to subtract array[0] and set array[1].

So, you may have an "off by one" error, logically speaking.

Dunno.

With the unit test app, you could add debug printf for everything. When neg val happens, stop. Then analyze the log. unit test could/should test for all edge cases.

If you feed a sequence of (e.g.) 1x10, 0x3, 1x5, 0x20 into the function, these should show up in the arrays/struct. The stored entries should match the input sequence.

So, dump the array(s) to verify that the counts are as expected.

What values hit 8000/30000?

Answer 2

In case anyone come back to this, this is the code I am using and my running average is working perfectly

 void display(char event, char test) {

   static int size=9; //size of the array
   static int array[10]={0, 0, 0, 0, 0, 0, 0, 0, 0, 0};  //array with number of passes for each n
   static int n=0; //number for the index in both arrays
   static long int sum=0;//total of passes
   static double average=0;//rate of passes per tests.  The sum of the values in array[n] divided by total tests in totalnumberoftest[n] attay
   static int consecpass=0; //consecutive passes
   static int consecfail=0; //consecutive fails
   static int totalnumberoftests[10]={0, 0, 0, 0, 0, 0, 0, 0, 0, 0}; //total number of tests conducted.  Counts the number of passing or failing tests for the nth value
   static long int counter=1; //used to count the total number of tests
   static int flag=0;//flag used to indicate when the input goes from event==DETECTED to event==NOT_DETECTED
   static int lowlimitb=0;// integer value from low limit b in desert for average setting.  Value entered should be the positive tests/total tests percentage


        sum=sum-(float)array[n]; //subtract the nth value of array from sum
        counter=(float)counter-totalnumberoftests[n];
        array[n]=0;//set the nth value to zero, because the previous state of event was NOT_DETECTED, meaning there were no positive tests
        totalnumberoftests[n]=consecfail; //add the number of negative tests to total tests
        sum=sum+(float)array[n]; //add array index n to sum (should be zero)
        counter=counter+(float)totalnumberoftests[n]; //counter is the total number of tests.  Add totalnumberoftests with the last index n adds the last consecfail to counter

        flag=1; //set flag==1 to indicate the last event was DETECTED
        consecpass++; //count a pass
        consecfail=0;

        n++; //set the next index for the arrays
        if(n>size)n=0;  //if array index is greater than the size of the array, set the index back to zero. This will overwrite the data in array index zero
        //GREEN_LED_ON;
        //RED_LED_OFF;
    }else{ //the last event=DETECT, no need to change array indices


    consecpass++;
    //GREEN_LED_ON;
    //RED_LED_OFF;

    }

} if (event==NOT_DETECTED){

    if(flag==1) //flag gets set to 1 in event==DETECTED
    {

        sum=sum-(float)array[n];    //subtract the nth value of array from current sum of passes
        counter=counter-(float)totalnumberoftests[n];
        array[n]=consecpass; //set array[n] equal to the number of consecutive passes
        totalnumberoftests[n]=consecpass; //set the number of tests for index n = number of passes
        sum=sum+(float)array[n]; //add the last number of consecutive passes (stored in array[n]) to the current sum of passes
        counter=counter+(float)totalnumberoftests[n];

        flag=0; //set the flag==0 so the array indices do not change until event changes
        consecfail++;

        n++; //set the next index for the arrays
        if(n>size)n=0;//if array index is greater than the size of the array, set the index back to zero. This will overwrite the data in array index zero
        //RED_LED_ON;
        //GREEN_LED_OFF;
    }else{

    consecpass=0;
    consecfail++;
    //RED_LED_ON;
    //GREEN_LED_OFF;


    }
}

if (consecpass>8000) //used to prevent rollover and to indicate failure if device is standing still.  8000 was selected because the absolute minimum speed will be 10cm/s
{
    sum=sum-(float)array[n];
    counter=counter-(float)totalnumberoftests[n];
    array[n]=consecpass;
    totalnumberoftests[n]=consecpass;
    sum=sum+(float)array[n];
    counter=counter+(float)totalnumberoftests[n];
    consecpass=0;
    n++;
    if(n>size)n=0;
}

if(consecfail>30000) //used to prevent rollover and to indicate failure if device is standing still.  30000 was selected because the absolute minimum speed will be 10cm/s
{
    sum=sum-(float)array[n];
    counter=counter-(float)totalnumberoftests[n];
    array[n]=0;
    totalnumberoftests[n]=consecfail;
    sum=sum+(float)array[n];
    counter=counter+(float)totalnumberoftests[n];
    consecfail=0;
    n++;
    if(n>size)n=0;
}

average=(double)sum/(double)counter; //average is a float.  The total number of positive tests/total number of tests


if(average<.35 && average > .25 //acceptable passing range
{
    GREEN_LED_ON;
    RED_LED_OFF;


} else {

    GREEN_LED_OFF;
    RED_LED_ON;
    }



  }
   }