Trying to use millis instead of delay for a switch state loop

Question

I am trying to write a code for an Arduino, which is a model of a redlight. The idea is that pressing the button will switch to the first state (turning LEDs on and off). Then the second and finally the third state will revert it back to the first state waiting for the button to be pressed again

There is the need for a time gap between the 2 states....it works perfectly well using the delay() function

But I am not allowed to use it, so instead, I am trying to use millis(), the idea is that the time should start once the button is pressed and stop once it's reverts back to the first state and restart over once the button is pressed again. And it's sadly not working, whenever I press the button nothing happens.

Does anybody know how I can fix it? here is the code:

const long delay2 = 4000;

int but = 2;
int leda = 3;
int ledb = 4;
int ledc = 5;
int ledd = 6;
int lede = 7;
volatile byte state=LOW;


void setup() {
    Serial.begin(9600);
    for(int p=3; p <= 8; p++) pinMode(p, OUTPUT);
    pinMode(but, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(but), changestate, FALLING);
}

void car(){
    digitalWrite(leda, HIGH);
    digitalWrite(ledb, LOW);
    digitalWrite(ledc, LOW);
    digitalWrite(ledd, LOW);
    digitalWrite(lede, HIGH);
}
void wait(){
    digitalWrite(leda, LOW);
    digitalWrite(ledb, HIGH);
    digitalWrite(ledc, LOW);
    digitalWrite(ledd, LOW);
    digitalWrite(lede, HIGH);
}
void ped(){
    digitalWrite(leda, LOW);
    digitalWrite(ledb, LOW);
    digitalWrite(ledc, HIGH);
    digitalWrite(ledd, HIGH);
    digitalWrite(lede, LOW);
}
void changestate() {
    state=state+1; 
    if (state > 3){
        state=0; 
    }
}

void loop(){
    switch(state){
        case 0:
            car();
            unsigned long start_time = millis();
            break;
        case 1:
            wait();
            while (millis() > start_time + delay1);  
              state=2;
            break;
        case 2:
           ped();
           while (millis() > start_time + delay2);
            state=0;
            break;
        default:
            break;
    }
} 


EDIT: The issue has been solved! Here's the code for further reference: 

#define delay1 500
#define delay2 4000


int but = 2;
int leda = 3;
int ledb = 4;
int ledc = 5;
int ledd = 6;
int lede = 7;
volatile byte state=LOW;


void setup() {
    Serial.begin(9600);
    for(int p=3; p <= 8; p++) pinMode(p, OUTPUT);
    pinMode(but, INPUT_PULLUP);
    attachInterrupt(digitalPinToInterrupt(but), changestate, FALLING);
}

void car(){
    digitalWrite(leda, HIGH);
    digitalWrite(ledb, LOW);
    digitalWrite(ledc, LOW);
    digitalWrite(ledd, LOW);
    digitalWrite(lede, HIGH);
}
void wait(){
    digitalWrite(leda, LOW);
    digitalWrite(ledb, HIGH);
    digitalWrite(ledc, LOW);
    digitalWrite(ledd, LOW);
    digitalWrite(lede, HIGH);
}
void ped(){
    digitalWrite(leda, LOW);
    digitalWrite(ledb, LOW);
    digitalWrite(ledc, HIGH);
    digitalWrite(ledd, HIGH);
    digitalWrite(lede, LOW);
}
void changestate() {
    state=state+1; 
    if (state > 3){
        state=0; 
    }
}

unsigned long start_time = millis();
void loop(){
    switch(state){
        case 0:
            car();
            break;
        case 1:
            start_time = millis();
            while(millis() < start_time + delay1){
              wait();
              state=2;
            }
            break;
        case 2:
           while(millis() < start_time + delay2){
             ped();
           }
            state=0;
            break;
        default:
            break;
    }
} 

Answer 1

Problematic things in code:

• unsigned long start_time = millis(); start_time can't be a local variable. After break command, loop function will end. Local variable on the stack will be destroed. After new calling loop will created again and content is unpredictable. Make it global.
• while (millis() > start_time + delay1); if this become true it will hangup for 49 days. You probably want do while (millis() < start_time + delay1); But this is same as use delay()
• millis() < start_time + delay1; don't use this form. It produce falsely result shortly before internal counter overflow (every 49.x days). Use form (millis() - start_time > delay1 This calculate valid elapsed time also if result of millis() is lower then start_time

Also, do you have debouced button? It's seem as school exercise. I will don't write code for you. Only standart hint - Use Serial.println() to log where your code is running and what is in yours variables.

Answer 2

Here is an untested suggestion. It only allows state changes if a sufficient amount of time has passed. You may want to have different times in an array if (millis() - start_time > mydelays[state]) {

volatile bool change_allowed = true;
volatile bool statechanged = true;
volatile unsigned long start_time = 0;

void changestate() {        
    if (millis() - start_time > DELAY) {
        change_allowed = true;
    }
    if (change_allowed) {
        start_time = millis();
        change_allowed = false;
        state=state+1;
        statechanged = true;
        if (state > 2) {
            state=0; 
        }
    }
}

void loop() {
    if (statechanged) {
        statechanged = false;
        switch(state){
            case 0:
...
}