Why are the Do While loops not stopping?

Question

Ive been staring at this for a while now and I am going cross-eyed. I am writing a simply program to change colors of a 3 color LED. It is working, but then I noticed the LED would sometimes flash. I did a variety of tests and eventually, using the serial monitor, found out the issue. The brightness should count down to a brightness level of 0 and up to a level of 255. However, sometimes it counts below 0 to -1 and sometimes it continues infinitely past 255. I cant figure out why. Below is the code and the serial monitor outputs. The only way I can stop is by manually setting the value outside the loop. You can see those lines commented out to allow for the error.

int randLeg;  // var to pick a random leg

// set all 3 legs to max brightness at start
int legThreeBrightness = 255;
int legFiveBrightness = 255;
int legSixBrightness = 255;
int delayTime = 15;
int increment = 1;

void setup() {

    Serial.begin(9600);     

    // set pins as output
    pinMode(3, OUTPUT);
    pinMode(5, OUTPUT);
    pinMode(6, OUTPUT);
    
    // turn all the LEDs on
    analogWrite(3, legThreeBrightness);
    analogWrite(5, legFiveBrightness);
    analogWrite(6, legSixBrightness);

    randomSeed(analogRead(0));  // seed rand num generator

}  

void loop() {  
  
    randLeg = random(4, 7);  // pick a random leg
    
    switch (randLeg) {
    case 4:  //-1 for pin 3
        if (legThreeBrightness == 255){
          do {
                Serial.println(legThreeBrightness);
                analogWrite(3, legThreeBrightness);
                delay(delayTime);
                legThreeBrightness = legThreeBrightness - increment;
             } while (legThreeBrightness >= 0);   
             //legThreeBrightness = 0;
        }
        else {
         do {
                Serial.println(legThreeBrightness);
                analogWrite(3, legThreeBrightness);
                delay(delayTime);
                legThreeBrightness = legThreeBrightness + increment;
             } while (legThreeBrightness <= 255);
             //legThreeBrightness = 255;
        }
      break;
      
    case 5:
        if (legFiveBrightness == 255){
          do {
                ////Serial.println(legFiveBrightness);
                analogWrite(5, legFiveBrightness);
                delay(delayTime);
                legFiveBrightness = legFiveBrightness - increment;
             } while (legFiveBrightness >= 0);  
             //legFiveBrightness = 0; 
        }
        else {
         do {
                ////Serial.println(legFiveBrightness);
                analogWrite(5, legFiveBrightness);
                delay(delayTime);
                legFiveBrightness = legFiveBrightness + increment;
             } while (legFiveBrightness <= 255);
             //legFiveBrightness = 255;
        }
      break;
    case 6:
        if (legSixBrightness == 255){
          do {
                ////Serial.println(legSixBrightness);
                analogWrite(6, legSixBrightness);
                delay(delayTime);
                legSixBrightness = legSixBrightness - increment;
             } while (legSixBrightness >= 0);   
             //legSixBrightness = 0;
        }
        else {
         do {
                ////Serial.println(legSixBrightness);
                analogWrite(6, legSixBrightness);
                delay(delayTime);
                legSixBrightness = legSixBrightness + increment;
             } while (legSixBrightness <= 255);
             //legSixBrightness = 255;
        }
      break;
   }
    
      
}

10 9 8 7 6 5 4 3 2 1 0 -1

and

250 251 252 253 254 255 256 257 258 259 260 261 262 263 264 265

Answer 1

change >= and <= to > and < respectively.

You want it to stop at 0, but your do-while will do another round, since 0>=0 is true. So your code only stops at -1 and 256.

When legThreeBrightness is 256 it should be counting down, but your if (legThreeBrightness == 255){ doesn't detect this, so the code will continue counting up. I'd probably change this line to if (legThreeBrightness >= 255){ to prevent this from ever happening (which it shouldn't, but still).

Answer 2

You have two issues related to bounds checking. For the sake of brevity, since all there LED branches of the switch statement are the same, I will refer to legNumBrightness instead of the individual legThreeBrightness, legFiveBrightness, legSixBrightness variables since this applies to all three (individually).

Because do while loops always run once before testing the condition, you have an issue with the value of legNumBrightness going out of bounds. i.e. when it is equal to 255, it decrements by one, until it is zero, and then because 0>=0 is still true (0 is equal to 0, after all), it decrements once more, so legNumBrightness is now -1. So on the next visit of that legNumBrightness = 255, the increments up by one branch is taken, until it reaches 255 <= 255, which is again true, and hence will go to 256. As the if bounds test used is only that of legNumBrightness == 255, it will only decrement if the value is exactly 255. Thus if it is not 255, it will keep incrementing until the maximum value of an int is reached (32767), overflow to -32768, and probably keep toggling between -32768 and 32767.

So to fix this, you need to change the if (legNumBrightness == 255) bound checking to if (legNumBrightness >= 255). Now, because of the do while loop, legNumBrightness will still sometimes go to -1 and 256, but can no longer overrun past 256. To fix the -1 and 256 overrun, remove the = from the do while tests so they become while (legNumBrightness > 0) and while (legNumBrightness < 255) and thus will not try to increment or decrement when the boundary has been reached.

I would also recommend using a while() loop in preference to a do while() loop, so the condition is checked before running the loop content, but it does not appear necessary from brief testing of this logic.

e.g. for legThreeBrightness

 case 4:  //-1 for pin 3
        if (legThreeBrightness >= 255){
          do {
                //Serial.println(legThreeBrightness);
                analogWrite(3, legThreeBrightness);
                delay(delayTime);
                legThreeBrightness = legThreeBrightness - increment;
             } while (legThreeBrightness > 0);   
             //legThreeBrightness = 0;
        }
        else {
         do {
                //Serial.println(legThreeBrightness);
                analogWrite(3, legThreeBrightness);
                delay(delayTime);
                legThreeBrightness = legThreeBrightness + increment;
             } while (legThreeBrightness < 255);
             //legThreeBrightness = 255;
        }
      break;

Answer 3

I agree with @Gerben that you need to change >= and <= to > and < respectively, for the do-while loop check.

Secondly, your if statement expects legThreeBrightness (et al) to be 255 in order to decrement the brightness. Because of your while statement is over-running 255 because of <= 255, the if statement will always execute the else part and because it is a do-while it will execute the loop once, incrementing legThreeBrightness.

To fix this, change the if statement to check for a value above 250:

case 4:  //-1 for pin 3
    if (legThreeBrightness > 250){  // <---- Notice the > 250
      do {
            //Serial.println(legThreeBrightness);
            analogWrite(3, legThreeBrightness);
            delay(delayTime);
            legThreeBrightness = legThreeBrightness - increment;
         } while (legThreeBrightness > 0);   
         //legThreeBrightness = 0;
    }
    else {
     do {
            //Serial.println(legThreeBrightness);
            analogWrite(3, legThreeBrightness);
            delay(delayTime);
            legThreeBrightness = legThreeBrightness + increment;
         } while (legThreeBrightness < 255);
         //legThreeBrightness = 255;
    }
  break;

This is easy to check and you should see the problem disappear straight away.