constrain() getting ignored for one loop

I have a problem with constrain() (and min() and max() for that matter) where I can still get out of bound values for one loop.

I'm trying to build a LED that fades in and out at different speeds depending on where the potentiometer is, but I have found that if the value of the potentiometer doesn't add up to exactly 0 or 255 it will ignore my constrain for one loop causing the LED to flash unevenly.

My code looks like this:

int led = 3;
int pot = A0;
int potval;
int dir = 1;
int brightness;

void setup() {
  Serial.begin(9600);
  pinMode(pot, INPUT);
  pinMode(led, OUTPUT);

}

void loop() {
  Serial.print("potval: ");
  Serial.print(potval);
  Serial.print("\t brightness: ");
  Serial.println(brightness);

  potval = map(analogRead(pot), 0, 1023, 0, 22);

  analogWrite(led, brightness);

  if (potval == 0) {
    analogWrite(led, 0);
  }
  else if (potval >= 21) {
    analogWrite(led, 255);
  }

  if (brightness <= 0 || brightness >= 255)
  {
    brightness = constrain(brightness, 0, 255);
    dir = -dir; // switch fading direction
  }

  brightness += potval * dir;

  delay(100);
}

How can I make sure the values are always in bounds?

You constrain first, then add something to brightness. Do the add, then do the constrain....

MarkT:
You constrain first, then add something to brightness. Do the add, then do the constrain....

Yes, thank you, that was it!

I may be reading this wrong, but going into loop() brightness is 0. I've added comments that reflect how I'm reading the code, which may be wrong:

  potval = map(analogRead(pot), 0, 1023, 0, 22);   // No idea what potval is

  analogWrite(led, brightness);               // brightness is 0, so the LED is dark?

  if (potval == 0) {                          // No idea
    analogWrite(led, 0);                      // LED off
  }
  else if (potval >= 21) {                    // Causes LED to full brightness
    analogWrite(led, 255);
  }

  if (brightness <= 0 || brightness >= 255)          // brightness still 0
  {
    brightness = constrain(brightness, 0, 255);      // Since it's 0, it remains 0
    dir = -dir; // switch fading direction           // dir = 1 on first pass, changes to -1
  }

  brightness += potval * dir;                        // brightness is now equal to -potval

Since brightness is now a negative number, the if test falls into constrain() with a negative value, which makes it 0 again. Is this what you expect? Put a bunch of Serial.println(brightness ) after each line and see what brightness is doing.

I read the program as.

 potval = map(analogRead(pot), 0, 1023, 0, 22);

pot connected to analog pin is read at full scale then converted to 0 to 22

 if (potval == 0) {    
    analogWrite(led, 0);    
  }
  else if (potval >= 21) {   
    analogWrite(led, 255);
  }

if the pot is turned all the way on or off ignore fading and turn the led on or off depending on the way the pot is set

 if (brightness <= 0 || brightness >= 255)  
  {
    brightness = constrain(brightness, 0, 255); 
    dir = -dir; // switch fading direction  
  }

  brightness += potval * dir;

this is a attempt at saying if the fade reaches the min or max then reverse the fade, fad will be more than 0 point per loop and less than 22 or the led will be over written by the If argument (not a good way to do this)

 brightness = constrain(brightness, 0, 255);

this is in the wrong place and depending on whether brightness is a byte or a int there's a whole bunch of things that could happen.

most people would write the program the same way you would read the code rather than having it loop
so this makes more sense when reading the code

 potval = map(analogRead(pot), 0, 1023, 0, 22);//convert analog range to 0-22

  if (potval == 0) { //pot is at min turn led off 
    analogWrite(led, 0);
  }
  else if (potval >= 21) {//pot is at max turn led on
    analogWrite(led, 255);
  }
  //should be another line that says skip next section this if potval is 0 or 22
  if (brightness <= 0 || brightness >= 255)//brightness is min or max for a analog write
  {
    dir = -dir; //reverse direction
  }

  brightness += potval * dir;//add or minus based on pot reading ranged 0-22  1-21 usable
  brightness = constrain(brightness, 0, 255); //write new number to brightness with in output range
  analogWrite(led, brightness); // show the new number

potval = map(analogRead(pot), 0, 1023, 0, 22);

As both ranges have a zero the map() function can be simpler.

potval = analogRead(pot) * 22 / 1023;

or even

potval = analogRead(pot) / 46;

Or simpler
potval = potval>>5; // /31 max vs 22, close enough?