Return to Revisions

One problem in your original code is that you are only storing the current state of the buttons if they are pressed. The next problem is that you are looping through all your brightness values when a button is pressed giving you your "fades all the way up in one go" problem.

This would be my replacement for your loop() code:

loop()
{
    int currentButtonStateA = digitalRead(buttonApin);
    if (currentButtonStateA != lastButtonStateA && currentButtonStateA == LOW)
    {
        currentBrightness += BRIGHTNESS_INCREMENT;  // BRIGHTNESS_INCREMENT is defined at the top of the file with #define BRIGHTNESS_INCREMENT 25
        if (currentBrightness > MAX_BRIGHTNESS)     // MAX_BRIGHTNESS is defined at the top of the file with #define MAX_BRIGHTNESS 255
        {
            currentBrightness = MAX_BRIGHTNESS;
        }
    }
    lastButtonStateA = currentButtonStateA;

    int currentButtonStateB = digitalRead(buttonBpin);
    if (currentButtonStateB != lastButtonStateB && currentButtonStateB == LOW)
    {
        currentBrightness -= BRIGHTNESS_INCREMENT;
        if (currentBrightness < MIN_BRIGHTNESS)     // MIN_BRIGHTNESS is defined at the top of the file with #define MAX_BRIGHTNESS 0
        {
            currentBrightness = MIN_BRIGHTNESS;
        }
    }
    lastButtonStateB = currentButtonStateB;

    analogWrite(ledPin, currentBrightness);
}

Hopefully this is pretty self explanatory. The current button A state is read and stored in a local variable. If it is different to the previous state and it is low, the current brightness is incremented and capped at a maximum value. Next the current button state is stored as the previous state ready for the next run through the loop.

Similar actions are performed for the B button, but with a decrement rather than an increment. Finally the LED brightness is set regardless of whether any buttons have been pressed.

You do not need to store your current button states (buttonStateA) or your counts for the number of button presses (buttonPushCounterA - which you never referenced anyway) or your ms value so could get rid of those global variables. You need a new global variable at the top of your file for currentBrightness (int currentBrightness = 0;).

You have used variables to store various non-changing numeric values, such as "maxBright = 255;" which is good practise, but is better (particularly with Arduino development) to use #define macros instead, such as "#define MAX_BRIGHTNESS 255".

The reason for this is that variables take up memory when the code is running - something that an Arduino Uno doesn't have a lot of, whereas a #define takes up no memory - the compiler uses it as a "search and replace" before compiling your code.