Problems with kitchen timer sketch [solved]

EDIT: This has been solved already
Hi
I´m having trouble with this kitchen timer sketch of mine, any help is appreciated. (I´m using Arduino Uno r3, 1x8 lcd display in 8-bit mode and 3 pushbuttons connected from A0 / A1 / A2 to ground.)

 [i]// For 1x16 display, 8 bit mode

// include the library code:
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(12, 11, 9, 8, 7, 6, 5, 4, 3, 2);

unsigned long startTime = 0; 
int duration = 0; // how long a key has been kept pressed (ms)
int seconds = 0;
int minutes = 0;
byte reading = 0;
byte keys = 15;
byte flagCntr = 0;
boolean served = false; // indicates that a function has been served
                    // and we need to wait for the key to be released
volatile boolean flag = false; // indicates that 1/4 second has passed
boolean eggRunning = false; 

void setup() 
{
  pinMode(A0, INPUT_PULLUP); // A0,14
  pinMode(A1, INPUT_PULLUP); // A1,15
  pinMode(A2, INPUT_PULLUP); // A2,16
  pinMode(A3, INPUT_PULLUP); // A3,17
      // set up Timer1 
  noInterrupts();           // disable all interrupts
  TCCR1A = 0;
  TCCR1B = 0;
  TCNT1  = 0;

  OCR1A = 15625;            // compare match register 16MHz/256/15625=4Hz
  TCCR1B |= (1 << WGM12);   // CTC mode
  TCCR1B |= (1 << CS12);    // 256 prescaler 
  TIMSK1 |= (1 << OCIE1A);  // enable timer compare interrupt
  interrupts();             // enable all interrupts

  lcd.begin(16, 1);         // set up the LCD's number of columns and rows:
}

ISR(TIMER1_COMPA_vect)      // timer compare interrupt service routine
{
  flag = true;
}

void loop() 
{
  if(flag) // if 1/4 seconds has passed
  {
     flagCntr++;
     if(flagCntr == 4) // if a second has passed
     {
      flagCntr = 0;
      if(eggRunning) // and egg timer is running
      {
        subTime(1);
        if(seconds == 0 && minutes == 0) // update egg timer and check 
                                         // if it has finished
        {
          eggRunning = false;
          lcd.setCursor(0, 0);
          lcd.print("Done!");
          delay(2000);
        }
      }
    }
  }

  reading = (PINC & B1111); // check if keys are pressed
  delay(50);
  if((PINC & B1111) == reading) // if a valid reading
  {
    if(reading == 15) // if not pressed
    {
    keys = 15;
    served = false; 
    duration = 0;
    }
    else
    {
    if(reading != keys) // if a new key
     { 
     keys = reading;
     startTime = millis();
     }
    else {duration = millis() - startTime;}
    }
  }
  else
  {
    keys = 15; 
    duration = 0;
  }
  if(flag) // if 1/4 seconds has passed
  {
   if(!served && keys != 15)
   {
    switch(keys) // find and serve function
    {
      // case 7: Mode key (A3) (for future extension)
      case 9: // Reset key (+ and - keys pressed simultaneously)
       if(!eggRunning) // only serve reset, + and - keys if egg timer
                       // isnt running
       {
       seconds = 0;
       minutes = 0;
       served = true;
       }
       break;
      case 11: // + key (A2)
       if(!eggRunning)
       {
       if(duration > 7000)
       addTime(125);
       else if(duration > 5000)
       addTime(25);
       else if(duration > 3000)
       addTime(5);
       else if(duration > 1000 || duration < 250)
       addTime(1);
       }
       break;
      case 13: // - key (A1)
       if(!eggRunning)
       {
       if(duration > 7000)
       subTime(125);
       else if(duration > 5000)
       subTime(25);
       else if(duration > 3000)
       subTime(5);
       else if(duration > 1000 || duration < 250)
       subTime(1);
       }
       break;
      case 14:  // Enter key (A0)
       if(seconds != 0 || minutes != 0) // if egg timer is "00:00"
                                        // dont allow it to run
       {
       eggRunning = !eggRunning; // toggle egg timer start / stop
       served = true;
       }
       break;
      default:
       served = true; // odd key combination detected,
                      // wait for keys to be released
    }
   }
  
   lcd.setCursor(0, 0);  // set the cursor to column 0, line 0
   if(minutes < 10){lcd.print(" ");}
   lcd.print(minutes);      // print time
   lcd.print(":");
   if(seconds < 10){lcd.print(0);}       // add leading zero
   lcd.print(seconds);
   flag = false; 
  }
}

void addTime(int value)
{
 seconds = (seconds + value);
 if(seconds > 59) 
 {
 minutes = (minutes + (seconds / 60));
 seconds = (seconds % 60);
 }
 if(minutes > 99){minutes = (minutes % 100);}
}

void subTime(int value)
{
 seconds = (seconds - value);
 if(seconds < 0) 
 {
  minutes = (minutes + ((seconds - 59)/ 60));
  seconds = ((seconds % 60) + 60);
 }
 if(minutes < 0){minutes = ((minutes % 100) +100);}
}[/i]

Brief description of how it is supposed to work: First there is "0:00" on the display. By pressing + key (pushbutton connected to A2) or - key (connected to A1) you can increase or decrease the time on display. Pressing + and - keys simultaneously will reset the time back to "0:00". When + or - key is being kept pressed down the time on the display will increase (or decrease) on an increasing speed. Pressing Enter key (connected to A0) will start the timer counting down and pressing again will stop it. While the timer is counting down you can´t adjust the time because the + and - keys are ignored. When the timer reaches zero "Done!" is printed on the display.

What is wrong with it: It lets me to adjust the time on the display just fine, but when I press the Enter key the timer doesnt count down. It blocks the + and - keys tough, unless the time is on 0:00 and pressing the Enter key again unblocks the + and - keys, just like it´s supposed to, so the eggRunning flag seems to work at least.(Could be something obvious wrong with it, but I just can´t seem to spot it.)

Also a second question: What do you think of the code ? For example is it overly complicated ?

simplify your code with a basic state machine approach, and try the enum resource to make it more clear what's happening when:

Pseudo code:

enum TimerState {
  MENU_SELECT,
  COUNT_DOWN,
  PAUSE,
  COMPLETE
};

TimerState timerState = MENU_SELECT;
TimerState lastState = COMPLETE;


void setup() 
{
  // put your setup code here, to run once:

}

void loop() 
{
  if(timerState == MENU_SELECT)
  {
    if(upButtonPressed)
    {
      add to time
    }
    else if(downButtonPressed())
    {
      reduce from time
    }
    else if(enterPressed())
    {
      timerState = COUNT_DOWN;
    }
  }
  else if(timerState = COUNT_DOWN)
  {
    countDownDisplay();
    if(enterPressed())
    {
      timerState = PAUSE;
    }
  }
  else if(timerState == PAUSE)
  {
    displayPauseMessage();
    if(enterPressed())
    {
      timerState = COUNT_DOWN;
    }
    if(time <= 0)
    {
      timerState = COMPLETE;
    }
  }
  else //if(timerState == COMPLETE)
  {
    if(enterPressed())
    {
      timerState = MENU_SELECT;
    }
  }
}
int duration = 0; // how long a key has been kept pressed (ms)

I hope you don't plan to press the key for more than 32.767 seconds...

 if(flag) // if 1/4 seconds has passed

Wouldn't quarterSecondElapsed make more sense as the variable name?

     if(eggRunning) // and egg timer is running

I'd like to see a you-tube video of the egg running...

If the variable name was meaningful, the comment wouldn't be necessary. There is no reason to use short names when longer ones make more sense.

   if(reading == 15) // if not pressed

If what is not pressed?

   if(reading == 15) // if not pressed
    {
    keys = 15;
    served = false;
    duration = 0;
    }
    else
    {
    if(reading != keys) // if a new key

Your
indentation
leaves
a
lot
to
be
desired.

Learn about the Tools + Auto Format function, if you can't use the tab key properly.

     case 9: // Reset key (+ and - keys pressed simultaneously)

Mixing binary constants and decimal constants is not a good idea. Pick one base (2, 8, 10, or 16) and stick with it.

seconds = (seconds + value);

The parentheses are not needed.

Look at the code to deal with the eggRunning variable. You call subTime() once a second, but subTime() doesn't show the time (which it shouldn't) and neither does the code in loop. Why not?

After doing some tidying up as PaulS suggested, I tried doing some experimenting with this kitchen timer and was able to solve it myself.

Here´s what the problem was: (Here I refer to the non-working version) There is 2 places on the sketch where the "flag" variable is tested: on the very beginning of void loop() and then for the second time somewhere around halfway of the void loop(). At the very end of the void loop() the "flag" variable is cleared. The thing is that the end of the void loop() isn´t the end of the program as the program just "wraps over" and continues from the beginning of the void loop(). Because of this when the "flag" is tested for the first time its going to come out as false, since it has only just been cleared and the ISR haven´t had time yet to set it again.

Here´s the tidied up and working version

```
*// include the library code:
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(12, 11, 9, 8, 7, 6, 5, 4, 3, 2);

unsigned long startTime = 0;
unsigned long duration = 0; // how long a key has been kept pressed (ms)
int seconds = 0;
int minutes = 0;
byte reading = 0;
byte keys = 15;
byte quarterSecCntr = 0;
boolean served = false; // indicates that a function has been served
// and we need to wait for the key to be released
volatile boolean quarterSecPassed = false;
boolean eggRunning = false;

void setup()
{
  pinMode(A0, INPUT_PULLUP); // A0,14
  pinMode(A1, INPUT_PULLUP); // A1,15
  pinMode(A2, INPUT_PULLUP); // A2,16
  pinMode(A3, INPUT_PULLUP); // A3,17

//pinMode(13, OUTPUT);
  //digitalWrite(13, LOW);
 
  // set up Timer1
  noInterrupts();          // disable all interrupts
  TCCR1A = 0;
  TCCR1B = 0;
  TCNT1  = 0;

OCR1A = 15625;            // compare match register 16MHz/256/15625=4Hz
  TCCR1B |= (1 << WGM12);  // CTC mode
  TCCR1B |= (1 << CS12);    // 256 prescaler
  TIMSK1 |= (1 << OCIE1A);  // enable timer compare interrupt
  interrupts();            // enable all interrupts

lcd.begin(16, 1);        // set up the LCD's number of columns and rows:
}

ISR(TIMER1_COMPA_vect)      // timer compare interrupt service routine
{
  quarterSecPassed = true;
}

void loop()
{
  reading = (PINC & 15); // check if keys are pressed
  delay(50);
  if ((PINC & 15) == reading) // if a valid reading
  {
    if (reading == 15) // if none of the keys is pressed
    {
      keys = 15;
      served = false;
      duration = 0;
    }
    else
    {
      if (reading != keys) // if a new key
      {
        keys = reading;
        startTime = millis();
      }
      else {
        duration = millis() - startTime;
      }
    }
  }
  else
  {
    keys = 15;
    duration = 0;
  }
  if (quarterSecPassed)
  {
    quarterSecCntr++;
    if (quarterSecCntr == 4) // if a second has passed
    {
      quarterSecCntr = 0;
      if (eggRunning) // and egg timer is running
      {
        subTime(1);
        if (seconds == 0 && minutes == 0) // update egg timer and check if it has finished
        {
          eggRunning = false;
          lcd.setCursor(0, 0);
          lcd.print("Done!");
          delay(2000);
        }
      }
    }
    if (!served && keys != 15)
    {
      switch (keys) // find and serve function
      {
        // case 7: Mode key (A3) (for future extension)
        case 9: // Reset key (+ and - keys pressed simultaneously)
          if (!eggRunning) // only serve reset, + and - keys if egg timer isnt running
          {
            seconds = 0;
            minutes = 0;
            served = true;
          }
          break;
        case 11: // + key (A2)
          if (!eggRunning)
          {
            if (duration > 7000)
              addTime(125);
            else if (duration > 5000)
              addTime(25);
            else if (duration > 3000)
              addTime(5);
            else if (duration > 1000 || duration < 250)
              addTime(1);
          }
          break;
        case 13: // - key (A1)
          if (!eggRunning)
          {
            if (duration > 7000)
              subTime(125);
            else if (duration > 5000)
              subTime(25);
            else if (duration > 3000)
              subTime(5);
            else if (duration > 1000 || duration < 250)
              subTime(1);
          }
          break;
        case 14:  // Enter key (A0)
          if (seconds != 0 || minutes != 0) // if egg timer is "00:00" dont allow it to run
          {
            eggRunning = !eggRunning; // toggle egg timer start / stop
            served = true;
          }
          break;
        default:
          served = true; // odd key combination detected, wait for keys to be released
      }
    }

lcd.setCursor(0, 0);  // set the cursor to column 0, line 0
    if (minutes < 10) {
      lcd.print(" ");
    }
    lcd.print(minutes);  // print time
    lcd.print(":");
    if (seconds < 10) {
      lcd.print(0);      // add leading zero
    }
    lcd.print(seconds);
    quarterSecPassed = false;
  }
}

void addTime(int value)
{
  seconds = seconds + value;
  if (seconds > 59)
  {
    minutes = minutes + (seconds / 60);
    seconds = seconds % 60;
  }
  if (minutes > 99) {
    minutes = minutes % 100;
  }
}

void subTime(int value)
{
  seconds = seconds - value;
  if (seconds < 0)
  {
    minutes = minutes + ((seconds - 59) / 60);
    seconds = (seconds % 60) + 60;
  }
  if (minutes < 0) {
    minutes = (minutes % 100) + 100;
  }
}*
```

P.S: I used the led on the Arduino pin 13 to indicate if a loop was entered (led on for 100ms every time the loop was entered) or not (led off).
Thank you to everyone who tried to help.

Some further education: It is not "void loop()", it is the loop() function (that happens to return nothing). In programming, "nothing" is often known as "void".