Weird behaviour in Arduino loop

Question

I'm creating a distance measurement system in Arduino (Mega2560 board, Arduino 1.8.9, JSN-SR04T-2.0 ultrasonic sensor). As the sensor is not very stable, I want to average 15 measurements or the measured values every 500 milliseconds, whichever happens first. The values are stored in a 15 integer long array and there is a counter which tells how many measurements are made. The problem is that when 15 measurements are done and I want to average (not included in the code, only the if(...) part), the counter jumps from 14 to a very high value with no explanation and continue counting from there. Also, if I set a very high cycle time (e.g 1800 ms), sometimes it goes back to 0 without entering the if structure (where the cntr=0 line is).

I stripped down the code to a minimum to find what could cause the problem. It seems that when I use the value stored in "valueArrayUS", it doesn't work. If I comment it out, it works. I tried 2 different ways to use that value, both messed it up.

The minimum runnable code is this:

int trigPin = 8;    // Trigger
int echoPin = 9;    // Echo
int cntr=0; //how many measurements
long startTime=0; //start time of cycle
int valueArrayUS[15]; //max 15 values per cycle
int valueArrayUScp[15]; //copy of valueArrayUS
int sumUS=0;
long duration, cm;
int i=0, j=0;

void setup() {
  Serial.begin (9600);
  pinMode(trigPin, OUTPUT);
  pinMode(echoPin, INPUT);

  for (i=0; i<15; i++) {
    valueArrayUS[i]=0; //default state
    valueArrayUScp[i]=valueArrayUS[i];
  }
  startTime=millis(); //time counter start
}

void loop() {

  //ultrasonic measurement
  digitalWrite(trigPin, LOW);
  delayMicroseconds(50);
  digitalWrite(trigPin, HIGH);
  delayMicroseconds(20);
  digitalWrite(trigPin, LOW);

  duration = pulseIn(echoPin, HIGH, 38000); //if no object is detected -> 38 ms long pulse from sensor -> 38 ms timeout
  cm = (duration/2) / 29.1;     // Divide by 29.1 or multiply by 0.0343
  valueArrayUS[cntr]=cm;

  Serial.print("cntr:  ");
  Serial.print(cntr);
  Serial.println();

  if ((millis()-startTime>500) || (cntr==15)) { //cycle is ended or buffer full

    Serial.print("enters the cycle, cycle time: ");
    Serial.println();
    Serial.print(millis()-startTime);
    Serial.print(" ms ");
    Serial.println();
    Serial.print("cntr at cycle start: ");
    Serial.print(cntr);
    Serial.println();

    //averaging would be here
    for (j=0; j<15; j++) { 
      valueArrayUScp[j]=valueArrayUS[j];  //THIS IS WHERE IT GOES WRONG
      //sumUS=sumUS+valueArrayUS[j]; //THIS IS WHERE IT GOES WRONG
    }

   for (i=0; i<15; i++) {
    valueArrayUS[i]=0; //default state
    }
    cntr=0; 
    startTime=millis();
    }
  else {
    cntr=cntr+1;
    Serial.print("cntr++    ");
    Serial.println();
  }
}

You can see the commented line and line above it, both wrecks the code. If both are commented out, it works as expected. If any of them is present, the result is like this:

15:51:33.848 -> cntr:  11
15:51:33.894 -> cntr++    
15:51:33.894 -> cntr:  12
15:51:33.894 -> cntr++    
15:51:33.894 -> cntr:  13
15:51:33.942 -> cntr++  
15:51:33.942 -> cntr:  14
15:51:33.942 -> cntr++    
15:51:33.942 -> cntr:  117
15:51:33.989 -> cntr++   
15:51:33.989 -> cntr:  118
15:51:33.989 -> cntr++    
15:51:34.036 -> cntr:  119
15:51:34.036 -> cntr++    
15:51:34.036 -> cntr:  120
15:51:34.036 -> cntr++    
15:51:34.083 -> cntr:  121
15:51:34.083 -> cntr++    
15:51:34.083 -> cntr:  122
15:51:34.083 -> cntr++    
15:51:34.129 -> cntr:  123
15:51:34.129 -> cntr++    
15:51:34.129 -> cntr:  124
15:51:34.129 -> cntr++    
15:51:34.176 -> cntr:  125
15:51:34.176 -> cntr++    
15:51:34.176 -> cntr:  126
15:51:34.223 -> enters the cycle, cycle time: 
15:51:34.223 -> 557 ms 
15:51:34.270 -> cntr at cycle start: 126
15:51:34.270 -> cntr:  0
15:51:34.270 -> cntr++    
15:51:34.318 -> cntr:  1

It should be:

15:57:33.132 -> cntr:  14
15:57:33.132 -> cntr++    
15:57:33.132 -> cntr:  15
15:57:33.132 -> enters the cycle, cycle time: 
15:57:33.179 -> 392 ms 
15:57:33.179 -> cntr at cycle start: 15
15:57:33.226 -> cntr:  0
15:57:33.226 -> cntr++    
15:57:33.226 -> cntr:  1

What could cause that?

Answer 1

Your loop does this:

{
  valueArrayUS[cntr]=cm;
    //...
  if ((millis()-startTime>500) || (cntr==15)) { //cycle is ended or buffer full
    //...
  else {
    cntr=cntr+1;
  }
}

With the rest of the code deleted, it's easier to see that you're:

1. using cntr
2. testing whether cntr is 15 (i.e. past the end of the array)
3. updating cntr

That's not a problem the first 15 times through loop(), but after that 15th time cntr becomes 15 at the end of the function, and on the next pass through the function you end up doing this:

valueArrayUS[15]=cm;

That means you're writing past the end of the array, and it looks like the next thing in memory after valueArrayUS must be cntr, because you're apparently writing over that value.

That's the heart of the problem, but once that has happened then you're in for a world of hurt, because now cntr is likely to have a value > 15, so your cntr==15 condition isn't true, and now loop is going to write sensor values way past the end of valueArrayUS. That will effectively stomp all over your memory, and exactly what memory gets overwritten on a given run will depend on what the sensor happens to be reading.

To fix, you need to rearrange your steps:

1. check whether cntr is in bounds
2. if it's not, then do your calculation and reset cntr
3. if cntr is in bounds, then you can read the sensor, store the value, and update cntr

Also, code defensively:

• change your condition from cntr == 15 to cntr > 14 or cntr >= 15
• declare your other loop variables, i and j, locally, like: for (int i...