The only problem is that if one of the buttons is held down, the colors will cycle automatically.
What you need to do is to detect the transition, from released to pressed or from pressed to released, and only increment/decrement the value at the correct transition.
This requires keeping track of the previous state of the switches.
int prevUp = HIGH;
int prevDn = HIGH;
void loop()
{
int currUp = digitalRead(buttonPin1);
if(currUp != prevUp) // A transition occurred
{
if(currUp == LOW)
color++;
}
prevUp = currUp;
// handle other switch
// Set LED pins
}
Might I suggest that more meaningful names are useful. buttonPin1 doesn't mean a lot. upPin does, to me, at least. Ditto for buttonPin2 ==> dnPin.
Also, pressing the up and down buttons does not directly change the color. It changes the mode that will be used to set the pins to produce the color. So, a name like colorMode says that the variable controls the mode, not the color.
Otherwise, the code looks good. I knew you could get there.