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

• jsotola: you can remove 2/3 of the code if you use arrays
• Keltari: @jsotola that is probably true, but doesnt change the error I am having,
• jsotola: first do loop counts to -1 ... the second do loop prints the -1 and counts to 256 ... second do loop keeps counting from 256 because do-while always runs at least once ... use if (legThreeBrightness > 255){
• Acccumulation: Seems to be the question title should changed slightly, such as "Why are the do while loops stopping later than I expect?"

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).

• Keltari: Honestly, I think I actually tried this last night, but I will try it again. It also doesnt explain why in only happens sometimes. I will report back.
• Peter Feerick: It's definitely this - you can see in the output it go out of bounds, and just keeps going. e.g. from one part of a run: 252 253 254 255 256 256 257 256 258 257 257 259 258 259 258 260 259 260 260 261 262 263 261 262 263 264 265 261 264 265 266 267 262 263 Another way to prevent the value going out of bounds is to not use a do while loop, which runs the loop BEFORE it tests the value. But you will still need to use < or > for the comparison as you don't want the value decremented/incremented past when you have reached the boundary of your range.
• Keltari: this fixed the issue

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.