Troubleshooting AVR Interrupt Timing

My efforts towards making a temperature data logger ran into a bit of a wall during the week and it took me a while to figure out why.  The problem was related to the button used to start and stop data logging.  Most of the time it would work but occasionally it just wouldn't register the button press.  My error was a bit subtle but blindingly obvious once spotted.  The problem, to quote Doc Brown, was that I wasn't thinking fourth dimensionally.

  1: //Problem code fragment

  2: 

  3: if (BIT_GET(flags, START_STOP_LOG) && !BIT_GET(driveStatus, STA_NOINIT)) {

  4:         if (BIT_GET(flags, LOGGING)) {

  5:                 BIT_CLEAR(flags, LOGGING);

  6:         } else {

  7:                 BIT_SET(flags, LOGGING);

  8:         }

  9: }

 10: BIT_CLEAR(flags, START_STOP_LOG);

Shown above is the code I was having trouble with.  It's part of the main function that handles the button press event.  The software design uses a timer based interrupt service routine (ISR) that's responsible for updating the SD card status and de-bouncing the input buttons.  It updates the SD card status every 10 ms and scans the buttons every 100 ms.  Once it de-bounces the button it sets a START_STOP_LOG flag to indicate that a button press has occurred.  The main routine handles the flag and either sets or clears a LOGGING flag. I try to keep the code in an ISR to a minimum and just leave it to handle time critical tasks and setting flags, all of the other stuff is then done in the main routine.  I should point out that this is only skeleton code to allow me to get the program flow right.  It doesn't actually do anything yet.

The problem with the way I was going about it was that if the ISR runs and detects a button press just after the first if statement is evaluated, nothing will happen.  This is because once the ISR is finished, the flag is then cleared immediately on line 10 to say that the event has been handled, when in reality it hasn't.  It's kind of like programming something with multiple threads, where variables can seemingly change on their own.  The fix however is pretty straight forward and is shown below.

  1: //Corrected code fragment

  2: 

  3: if (BIT_GET(flags, START_STOP_LOG)) {

  4:         if (!BIT_GET(driveStatus, STA_NOINIT)) {

  5:                 if (BIT_GET(flags, LOGGING)) {

  6:                         BIT_CLEAR(flags, LOGGING);

  7:                 } else {

  8:                         BIT_SET(flags, LOGGING);

  9:                 }

 10:         }

 11:         BIT_CLEAR(flags, START_STOP_LOG);

 12: }

I've moved clearing of the flag into the if statement so that it can only happen if the flag is set.  However the if statement is also responsible for handling the button press.  So if the drive is initialized it will start or stop the logging then clear the flag, if it isn't initialized it will just clear the flag, as you can't log anything if the drive isn't ready.  By doing things this way the ISR can't jump in and set the flag before it's cleared because the only way to get to that point on line 11 is if the flag is already set and the event has already been handled.  It now works a treat, registering every button press.

BIT_SET, BIT_GET, BIT_CLEAR are preprocessor macros to increase code readability.  While START_STOP_LOG, STA_NOINIT, and LOGGING are preprocessor defines.