State Machines and Interrupts

I am trying to learn state machines and the interrupt function and implement them into a bigger project. The following code is for blinking an led on and off and the frequency changes depending on a state variable that is either SLOW or FAST.

// Arduino Due R3

volatile static int freq1 = 2000;
volatile static int freq2 = 4000;
long static unsigned int time1 = millis();
enum states{FAST, SLOW};
states state = SLOW;
void freq();
boolean start = true;

void setup() {
  pinMode(13, OUTPUT);
  attachInterrupt(2, freq, FALLING);
}

void loop() {
  switch(state){
    case SLOW:
    if(millis() - time1 < 2000){
      digitalWrite(13, HIGH);
    }
    if(millis() - time1 < 4000){
      digitalWrite(13, LOW);
      time1 = millis();
    }
    if(millis() - time1 > 4000){
      time1 = millis();
    }
    case FAST:
    if(millis() - time1 < 1000){
      digitalWrite(13, HIGH);
    }
    if(millis() - time1 < 2000){
      digitalWrite(13, LOW);
      time1 = millis();
    }
    if(millis() - time1 > 2000){
      time1 = millis();
    }
  }
}

void freq(){
  if(state == SLOW){
    state = FAST;
  }else{
    state = SLOW;
  }
}

When I upload this to my Due the led connected to pin 13 just stays on and isn’t blinking. On my hardware I have a button whose signal is going through an inverting schmitt trigger and is then fed into digital pin 2 for the interrupt. I tried to follow along with the blink without delay program in this program but this program seems to have some syntax problem. Anybody know what could be wrong?

switch / case needs break.

state needs volatile (and a critical section).

Thanks for your help! What do you mean by a critical section?

It might be OK if state is one byte.

Stuff about critical sections and interrupts:

http://www.gammon.com.au/forum/?id=11488&reply=7#reply7

In most cases, data that is shared between setup / loop and an interrupt service routine has to be protected from the interrupt service routine when it is accessed in setup / loop. One case where data must be protected is when the data is stored in multiple bytes. enum is two bytes in the AVR / Arduino world. state is an enum so state has to be protected.

A "critical section" is a section of code that runs to completion without being interrupted. A critical section is how you protect state when it is accessed from setup / loop. In the AVR / Arduino world, a critical section is composed of: save the global interrupt flag, disable interrupts, access the data, restore the global interrupt flag.

There are copious details available from Nick Gammon... http://gammon.com.au/interrupts

pYro_65 has a nice wrapper... http://arduino.cc/forum/index.php/topic,125253.0.html

Personally, I prefer the Libc atomic macros... http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html

    if(millis() - time1 < 2000)
    {
      digitalWrite(13, HIGH);
    }
    if(millis() - time1 < 4000)
    {
      digitalWrite(13, LOW);
      time1 = millis();
    }

If millis() - time is less than 2000 it will also be less that 4000 so both tests will be true and the second one will always be acted upon. Same problem in the FAST case.

Good point.

long static unsigned int time1 = millis();

Just:

unsigned long time1 = millis ();

... will do. Although a that point millis() will return zero so there isn't much point calling it.

UKHeliBob is right.

This condition will immediately be true:

    if(millis() - time1 < 2000)

So will this:

    if(millis() - time1 < 4000)