using millis() for a timer

Question

Basically the user presses a button and cycles thru 3 different modes. Mode 1 the LED is on, mode 2 the LED is off and mode 3 the LED is kept on for a certain amount of time. I'm using the millis() to keep track of the time. the LED in the third mode just stays on and never switches to off. I have the Serial.Writes in there to help me debug. I have the delay after the button read since if I didnt put it in it would register a lot of button presses.

int ledPin = 5;
int buttonApin = 9;
int ledMode = 0;

boolean lastButton = HIGH;
boolean currentAButton = HIGH;

// debounce button
boolean debounceFUNC(boolean last)
{
    boolean current = digitalRead(buttonApin);
    if (last != current)
    {
        delay(5);
        current = digitalRead(buttonApin);
    }
    return current;
}

void setMode(int ledMode)
{
    // turns on led
    if (ledMode == 1)
    {
        digitalWrite(ledPin, HIGH);
    }
    // turns off led
    if (ledMode == 2)
    {
    digitalWrite(ledPin, LOW);
    }
    // timer - turns off after 5000
    if (ledMode == 3)
    {
        long interval = 5000;
        unsigned long cMillis = millis();
        static unsigned long pMillis;

        if (cMillis - pMillis > interval)
        {   
        pMillis = cMillis;
        digitalWrite(ledPin, HIGH);
        Serial.println("On");
        Serial.println(interval - cMillis);
        }
        else
        {
        digitalWrite(ledPin, LOW);
        Serial.println("Off");
        }

        }
    }

void setup()
{
    Serial.begin(9600);
    pinMode(ledPin, OUTPUT);
    pinMode(buttonApin, INPUT_PULLUP);
}

void loop()
{
    // sets led mode
    currentAButton = debounceFUNC(lastButton);
    delay(500);
    if (currentAButton == LOW)
    {
        ledMode++;
        Serial.println(ledMode);
    }
    else if (ledMode > 3)
    {
        ledMode = 0;
    }
    // ledMode function call
    setMode(ledMode);
}

screenshot of the serial monitor.

Answer 1

The problem is in the logic, basically your statement:

    if (cMillis - pMillis > interval)
    {   
       pMillis = cMillis;
       digitalWrite(ledPin, HIGH);
        Serial.println("On");
        Serial.println(interval - cMillis);
    }
    else
    {
       digitalWrite(ledPin, LOW);
       Serial.println("Off");
    }

will always be true, this is as the difference in time, cMillis - pMillis is always greater than interval.

This means that it never enters the else statement to turn off the LED, and thus means the else statement is usually the first thing that rune at ledMode = 3.

What you need to do as the simplest fix is to change your comparison, change > to <, in the is statement:

if (cMillis - pMillis > interval)

to

if (cMillis - pMillis < interval)

this will ensure the LED is always on during the 5000 mS and then as soon as it is greater it will definitely go to the else

Answer 2

There is a problem with your debounce logic. You want to compare the current button state with the previous state, which is supposed to be stored in lastButton. But that variable is initialized to true and then never updated. Your program may work better if you issue lastButton = currentAButton somewhere.

You can find lots of tutorials on button debouncing, and there are several methods. The one you are using, which is based on delay(), is probably the worst: it should work in this simple case, but delay() is toxic as soon as your Arduino has more than one thing to do. A very common (and my preferred) method is to just ignore the button for some “debounce time” after you detect a state change. A debouncer function can be based on something like:

if (now - last_state_change < DEBOUNCE_TIME)
    return last_state;

Then you have to detect the button presses, which is not the same as the button being down: you don't want to interpret the down state as a continuous stream of very fast button presses. Instead, a button press is the button being down now whereas it was up on the previous read. A button_press() function could call the debounce() function and then have something like

bool press = last_state && !state;

This is effectively detecting falling edges of the debounced button. You can combine the debouncer and the falling-edge detector into a single function as follows:

bool button_press()
{
    static bool last_state;
    static uint32_t last_state_change;
    uint32_t now = millis();

    // Ignore the button during the debounce time.
    if (now - last_state_change < DEBOUNCE_TIME)
        return false;

    bool state = digitalRead(BUTTON_PIN);
    if (state != last_state) last_state_change = now;
    bool press = last_state && !state;
    last_state = state;
    return press;
}

Next there is the management of the LED states. What you are trying to implement is called a finite state machine, sometimes abbreviated “FSM”. If you search the Web for that term, you will find a few examples on how to implement it. It canonically revolves around a switch statement that enumerates all the possible states and tells what to do in each of those states. Something like

switch (state) {
    case STATE_FOO:
        if (some_condition()) {
            do_some_action();
            state = STATE_BAR;  // switch to the next state
        }
        break;
    case STATE_BAR:
        ...
}

Below is a FSM implementation of your LED logic. Note that you did not completely specify what should happen when the “timed” state times out: you could go to a forth “timed out” state, or you could just go back to the “off” state. In the code below I've taken that last option:

void loop()
{
    static enum { OFF, TIMED, ON } state;
    static uint32_t timed_started;
    uint32_t now = millis();

    // Switch state on button press.
    if (button_press()) switch(state) {
        case OFF:    // -> TIMED
            digitalWrite(LED_PIN, HIGH);
            state = TIMED;
            timed_started = now;
            break;
        case TIMED:  // -> ON
            // LED is lready on.
            state = ON;
            break;
        case ON:     // -> OFF
            digitalWrite(LED_PIN, LOW);
            state = OFF;
            break;
    }

    // Turn off after LED_ON_TIME.
    if (state == TIMED && now - timed_started >= LED_ON_TIME) {
        digitalWrite(LED_PIN, LOW);
        state = OFF;
    }
}

Note that it would have been more “canonical” to have the tests if (button_press()) inside the switch rather than the other way around. Then the test for the timeout would also be inside the same big switch. It's just that it seemed easier to me to write it this way.

For the sake of testability, here are the pieces that would be missing for having a complete working program:

const int BUTTON_PIN = 9;
const int LED_PIN = LED_BUILTIN;
const uint32_t DEBOUNCE_TIME = 50;
const uint32_t LED_ON_TIME = 5000;

void setup()
{
    pinMode(BUTTON_PIN, INPUT_PULLUP);
    pinMode(LED_PIN, OUTPUT);
}

// button_press() and loop() as above.