Circular Array Stuck in if statements

Question

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 not have GREEN_LED_ON; etc in the if(event) loops, but I put them there to visual where my code is going wrong. The code seems to get stuck in which ever loop even starts with. For example, if I start with the device over no ink, the LED stays red, and when the device is over ink, the device stays green no matter what. Does anyone have any idea what I am doing wrong and how to fix it?

Notes:
*I also tried changing the while(event)s to if statements and I get the same result

*When I comment the arrays inside the if statements, the code works as expected

*Top version is the current portion of the code and the bottom is what I started with

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)
        {
            sum=sum-array[n];
            counter=counter-totalnumberoftests[n];
            array[n]=0;
            totalnumberoftests[n]=consecfail;
            sum=sum+array[n];
            counter=counter+totalnumberoftests[n];
            n++;
        }

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

    } 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];
            n++;
        }

        //array[n]=consecpass;
        //totalnumberoftests[n]=consecpass;
        consecpass=0;
        consecfail++;
        flag=0;
        //GREEN_LED_OFF;
        //RED_LED_ON;
    }

    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];
        n++;
    }

    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];
        n++;
    }

    average=sum/counter;

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


}

This was what I originally started with:

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 int sum=0;//total number of passes
static double 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=0; //used to count the total number of tests



 while(n<=size)
    {
        sum=sum-array[n]; //subtacts the nth value from the total sum of passing tests
        counter=counter-totalnumberoftests[n];  //subtracts the nth value of the total number of tests run

        if(event == DETECTED)
        {
            array[n]=0;
            totalnumberoftests[n]=consecfail;
            consecfail=0;
            consecpass++;
            GREEN_LED_ON;
            RED_LED_OFF;

        } if(event==NOT_DETECTED){

            array[n]=consecpass;
            totalnumberoftests[n]=consecpass;
            consecpass=0;
            consecfail++;
            GREEN_LED_OFF;
            RED_LED_ON;
        }
        sum=sum+array[n];
        counter=counter+totalnumberoftests[n];

        average=sum/counter;

        /*if(average<1)
        {
            GREEN_LED_ON;
            RED_LED_OFF;
        }else{
            GREEN_LED_OFF;
            RED_LED_ON;
        }*/
        n++;
    }
    if(n>size) n=0;


    }

Both while sections will "stuck" because event does not change value inside. I guess you meant "if" and not "while" there. — Anty, Feb 08 '17 at 23:42
@Anty I actually tried replacing the whiles with ifs and got the same exact result — Kyle G, Feb 08 '17 at 23:44
You still miss the point - "event" will not change value during display execution. How is display being called and how event is being read? — Anty, Feb 08 '17 at 23:54
It seems to me that you want to receive many calls to this function, each with an `event` value, and compute the average, etc., from those calls. (This is what @Anty is pointing out.) Because you want to receive many calls, you shouldn't have a while loop in your function - the loop is outside your function somewhere, generating all these calls. Your code should be doing something like "What's the event? Okay, I'll update the count and return" — aghast, Feb 08 '17 at 23:56
@AustinHastings I think I understand now. The while loop that controls the array indices is not being updated. Instead I should use flags maybe? — Kyle G, Feb 09 '17 at 00:02

score 1 · Answer 1 · answered Feb 09 '17 at 00:13

*When I comment the arrays inside the if statements, the code works as expected

static int size=6;
static int array[6]={0};  //array with number of passes for each n
static int totalnumberoftests[6]={0};

and this

 while(n<=size)

When n=6 you pass the array boundary - max index is 5 not 6 for those (min index = 0).

    array[n]=0;
    totalnumberoftests[n]=consecfail;

That is UB and this may produce invalid behavior.

Change condition in while to n < size.

Anyway this code seems "weird" to me.

Would the while loop necessary? Another commenter described that the event calling basically works as the while loop and having another one can cause problems with what I want to happen — Kyle G, Feb 09 '17 at 16:52

score 0 · Answer 2 · answered Feb 09 '17 at 06:17

To elaborate on my comment, if you are in an event-driven system, I expect there is some code (usually called an "event loop") somewhere that looks like this:

event_loop()
{
    while (TRUE) 
    {
        event = get_event_from_someplace(...);

        display(...);
    }
}

It may be that instead of calling display directly, there was some process where you register the event handler. But the upshot is that there is probably an endless loop in some library code that calls your function over and over and over again. So you don't need the while() in your code.

Your code should be a state machine that keeps track of internal state (using static variables, like you are) and then performs whatever updates are required for each call.

Something like this:

void display(char event, ...)
{
    static int consecutive_passes = 0;
    static int consecutive_fails = 0;


    if (event == DETECTED) {
        ++consecutive_passes;
    }
    else if (event == NOT_DETECTED) {
        ++consecutive_fails;
    }
    else {
        // What else is there?
    }
}

The idea is that this code gets called every time there is an event, and it just updates whatever set of things need updating. But there is no while loop, because the calls are coming from the event loop, and that's all the while loop you need.

I see. Thank you for the clarification! The point of the while loop is for the arrays to rewrite data after n>size(of the arrays) and I was using a while loop to set the index of the array. Can this be done with if statements and flags instead? I made an edit showing what I did with the code but it's still not working exactly how I want it to. — Kyle G, Feb 09 '17 at 16:45
Yes! Your update, with `if (n == size) n = 0;` is perfect. That manages the "circular" part of the array. I have to admit, however, that the rest of your code is baffling to me. Can you add an update that explains what you are testing, and what kind of statistics you are trying to collect? — aghast, Feb 09 '17 at 22:03

Circular Array Stuck in if statements

2 Answers2