Go Down

Topic: Button triggered Macro, problems with interrupt and delay (Read 1 time) previous topic - next topic

twistedsymphony

Dec 28, 2012, 04:45 pm Last Edit: Dec 28, 2012, 05:14 pm by twistedsymphony Reason: 1
So I'm building a macro game controller, my intent is that I push a button and the Arduino will run through a macro sequence, ideally I'd like to be able to push the button a 2nd time to immediately stop the macro even if it hasn't completed. pushing it a 3rd time would start the macro sequence back at the beginning again.

I've got the hardware side complete and confirmed working.
I wrote a small program that turns an LED on and off via an interrupt and then I wrote another one that runs through my sequence as part of the main loop, both of those worked great (and confirmed that my hardware is setup properly) but I'm having problems now that I'm trying to combine them.

Initially when I tried running my macro sequence via an interrupt it would just skip over the calls to "delay" so I swapped that out for a manual calculation of delay using millis() but I can't seem to get that working and I don't know why. I would appreciate any help or advice.

Here is my sketch:

Code: [Select]
/*
SNES Controller Test
presses each of the buttons on the controller when the go button is pushed
should also stop the sequence if the go button is pushed a second time
*/

//set names for the output pins
int A = 4;
int B = 0;
int X = 1;
int Y = 2;

int R = 12;
int L = 7;

int Select = 13;

int Up = 11;
int Down = 10;
int Left = 9;
int Right = 8;

int Go = 1; //interrupt 1 pin #3

volatile int state = HIGH; // The go button state toggle
unsigned long lastInterruptTime = 0; //set the interrupt counter

// the setup routine runs once when you press reset:
void setup() {
 // initialize the digital pins as an output.
 pinMode(A, OUTPUT);
 pinMode(B, OUTPUT);
 pinMode(X, OUTPUT);
 pinMode(Y, OUTPUT);
 pinMode(L, OUTPUT);
 pinMode(R, OUTPUT);
 pinMode(Select, OUTPUT);
 pinMode(Up, OUTPUT);
 pinMode(Down, OUTPUT);
 pinMode(Left, OUTPUT);
 pinMode(Right, OUTPUT);
 
 //setup interrupt    
 attachInterrupt(Go, buttonPushed, LOW);
 
 //make sure everything is in the starting state that we want
 allClear();
}

//sets all outputs to off (high)
void allClear(){
 digitalWrite(Up, HIGH);
 digitalWrite(Down, HIGH);
 digitalWrite(Left, HIGH);
 digitalWrite(Right, HIGH);
 digitalWrite(A, HIGH);
 digitalWrite(B, HIGH);
 digitalWrite(X, HIGH);
 digitalWrite(Y, HIGH);
 digitalWrite(L, HIGH);
 digitalWrite(R, HIGH);
 digitalWrite(Select, HIGH);
}

// the loop routine runs over and over again forever:
void loop() {
wait();
}

// the interrupt routine run with the go button is pushed
void buttonPushed(){
 unsigned long interruptTime = millis(); //check the interrupt time
 
 // If interrupts come faster than 50ms, assume it's a bounce and ignore
 if (interruptTime - lastInterruptTime > 50){
   state = !state; //change state
   if (state){
     doStuff(); //run the sequence
   } else {
     allClear(); //clear the outputs
   }
   lastInterruptTime = interruptTime; //set the last interrupt time
 }
}

// my non "delay" delay routine
void myDelay(int interval){
 unsigned long previousMillis = millis(); //get the start time
 unsigned long currentMillis = millis(); //get the current time
 while (previousMillis + interval < currentMillis){ //loop until the start time plus the desired delay is reached
   currentMillis = millis(); //get the current time
 }
}

void wait(){
 myDelay(1000); // wait for 1 second
}

// the output sequence routine
void doStuff(){
 digitalWrite(Up, LOW);
 myDelay(1000);
 digitalWrite(Up, HIGH);
 digitalWrite(Down, LOW);
 myDelay(1000);
 digitalWrite(Down, HIGH);
 digitalWrite(Left, LOW);
 myDelay(1000);
 digitalWrite(Left, HIGH);
 digitalWrite(Right, LOW);
 myDelay(1000);
 digitalWrite(Right, HIGH);
 digitalWrite(A, LOW);
 myDelay(1000);
 digitalWrite(A, HIGH);
 digitalWrite(B, LOW);
 myDelay(1000);
 digitalWrite(B, HIGH);
 digitalWrite(X, LOW);
 myDelay(1000);
 digitalWrite(X, HIGH);
 digitalWrite(Y, LOW);
 myDelay(1000);
 digitalWrite(Y, HIGH);
 digitalWrite(L, LOW);
 myDelay(1000);
 digitalWrite(L, HIGH);
 digitalWrite(R, LOW);
 myDelay(1000);
 digitalWrite(R, HIGH);
 digitalWrite(Select, LOW);
 myDelay(1000);
 digitalWrite(Select, HIGH);
}

PeterH

You can't use delay() in an interrupt, and you should not be trying to. Interrupts are designed for things that happen very quickly, and your replay sequence does not happen very quickly.

I suggest you restructure your code into two parts:

A function to poll the switch input state, do any debouncing necessary and detect button transitions (up to down). Use this to toggle the state of your sketch between 'replay running' and 'replay not running'. The function should not call delay() or do anything that would prevent it returning promptly. Call this function once in loop().

A function which is called repeatedly while the replay is running. It should use the techniques demonstrated in 'blink without delay' (and also used in your current interrupt handler) to decide whether it is time to perform the next output action, and perform it if so. This function should be called once in loop() if the state is 'replay running'.
I only provide help via the forum - please do not contact me for private consultancy.

twistedsymphony

#2
Dec 28, 2012, 05:39 pm Last Edit: Dec 28, 2012, 05:42 pm by twistedsymphony Reason: 1
thanks! I'll give the restructuring a shot.

Though the "myDelay" function in my code above is modeled after the "blink without delay" program.

it doesn't seem to actually work though, I'm not sure why.

PeterH


thanks! I'll give the restructuring a shot.

Though the "myDelay" function in my code above is modeled after the "blink without delay" program.

it doesn't seem to actually work though, I'm not sure why.


Yeah, but it's not designed to be used inside an interrupt.
I only provide help via the forum - please do not contact me for private consultancy.

twistedsymphony

If I comment out the interrupt code and put my dostuff function in the main loop  it still doesn't work.

PaulS

Quote
Though the "myDelay" function in my code above is modeled after the "blink without delay" program

It may be modeled after the blink without delay example, it is still a blocking function. It does nothing different from what the regular delay() function does.

Nick Gammon

You are better of replacing this:

Code: [Select]

// the loop routine runs over and over again forever:
void loop() {
 wait();
}


with some kind of test for the button push, and then react accordingly.

eg.

Code: [Select]

void loop ()
 {
  static byte oldButtonState = HIGH;
 byte newButtonState = digitalRead (theButton);

 if (oldButtonState != newButtonState)
   {
    if (newButtonState == LOW) 
       buttonPushed ();
   oldButtonState = newButtonState;
   }  // end of button down
 }  // end of loop
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

twistedsymphony

#7
Dec 29, 2012, 05:24 am Last Edit: Dec 29, 2012, 05:29 am by twistedsymphony Reason: 1
ok so I've re-written my sketch per everyone's suggestions... it works!

My only questions now are:
Is there a more elegant way to do what I'm doing between each step in the doStuff function?

I'm interested in loading in the sequence from a separate file and ideally programming the state of all 12 outputs plus the delay time in a single line for ease of reading... I have some ideas but I don't like any of them any suggestions on the best way to go about this?



Code: [Select]
/*
SNES Controller Test
presses each of the buttons on the controller when the go button is pushed
should also stop the sequence if the go button is pushed a second time
*/

//set names for the output pins
int A = 4;
int B = 0;
int X = 1;
int Y = 2;

int R = 12;
int L = 7;

int Select = 13;

int Up = 11;
int Down = 10;
int Left = 9;
int Right = 8;

int Go = 3;

byte state = HIGH; // The macro state 0=running 1=stopped
byte lastButtonState = HIGH; // The go button state toggle 0=depressed
unsigned long lastPushTime = 0; //the milliseconds since the last time the button was pushed
unsigned long currentMillis = 0; //get the delay start time in milliseconds
unsigned long previousMillis = 0; //get the current time in milliseconds

// the setup routine runs once when you press reset:
void setup() {
 // initialize the digital pins as an output.
 pinMode(A, OUTPUT);
 pinMode(B, OUTPUT);
 pinMode(X, OUTPUT);
 pinMode(Y, OUTPUT);
 pinMode(L, OUTPUT);
 pinMode(R, OUTPUT);
 pinMode(Select, OUTPUT);
 pinMode(Up, OUTPUT);
 pinMode(Down, OUTPUT);
 pinMode(Left, OUTPUT);
 pinMode(Right, OUTPUT);
 
 // initialize the digital pins as an input.
 pinMode(Go, INPUT);
 
 //make sure everything is in the starting state that we want
 allClear();
}

//sets all outputs to off (high)
void allClear(){
 digitalWrite(Up, HIGH);
 digitalWrite(Down, HIGH);
 digitalWrite(Left, HIGH);
 digitalWrite(Right, HIGH);
 digitalWrite(A, HIGH);
 digitalWrite(B, HIGH);
 digitalWrite(X, HIGH);
 digitalWrite(Y, HIGH);
 digitalWrite(L, HIGH);
 digitalWrite(R, HIGH);
 digitalWrite(Select, HIGH);
}

// the loop routine runs over and over again forever:
void loop() {
 checkPushed(); //check if the button was pushed
 if (!state){ //if the state is set to run
   doStuff(); //run the sequence
   state = HIGH; //reset the state so we don't run again
 }
 allClear(); //set all the outputs to the starting position
}

// the interrput routine run with the go button is pushed
void checkPushed(){
 byte buttonState = digitalRead(Go);
 if (lastButtonState && !buttonState){
   unsigned long pushTime = millis(); //check the time
 
   // If pushed again within 60ms, assume it's a bounce and ignore
   if (pushTime - lastPushTime > 60){
     state = !state; //change state
     lastPushTime = pushTime; //set the last check time
   }
 }
 lastButtonState = buttonState;
}

// check for the button push
void myDelay(unsigned long interval){
 currentMillis = millis(); //get the current time
 previousMillis = currentMillis; //get the start time
 while (currentMillis - previousMillis < interval){ //loop until the desired delay is reached
   currentMillis = millis(); //get the current time
   checkPushed(); //check if the button has been pushed
   if (state){ //if the button push triggered a state change
     return; //exit the delay function
   }
 }
}


// the output sequence routine
void doStuff(){
 digitalWrite(Up, LOW); //start of first step in the sequence
 myDelay(1000); //continue running step while checking for a state change
 if (state){ //if there was a state change
   return; //quit the sequence
 }
digitalWrite(Up, HIGH); //end of first step in the sequence

 digitalWrite(Down, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(Down, HIGH);
 
 digitalWrite(Left, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(Left, HIGH);
 
 digitalWrite(Right, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(Right, HIGH);
 
 digitalWrite(A, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(A, HIGH);
 
 digitalWrite(B, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(B, HIGH);
 
 digitalWrite(X, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(X, HIGH);
 
 digitalWrite(Y, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(Y, HIGH);
 
 digitalWrite(L, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(L, HIGH);
 
 digitalWrite(R, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(R, HIGH);
 
 digitalWrite(Select, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(Select, HIGH);
}

PaulS

Quote
Is there a more elegant way to do what I'm doing between each step in the doStuff function?

I don't know about more elegant, but state is not a boolean. It should be, it you are going to use it as such. It should be assigned the values true or false. It should also have a better name, like stuffToDo. It should also be assigned the correct values at the correct times.

Code: [Select]
if(!stuffToDo)
   doStuff();

looks pretty stupid, doesn't it?

No to mention which the doStuff() function needs a better name. Something meaningful, like A348fGhw().

twistedsymphony

#9
Dec 29, 2012, 09:40 pm Last Edit: Dec 29, 2012, 09:42 pm by twistedsymphony Reason: 1
so I found another project that used an external file to separate the logic from the macro and it seems to work very well but now I'm having a weird bug....

So when I run my old "doStuff" function everything works great, I get a low signal on each output one at a time each for 1 second.

when I run my new function instead, which loops through an array and should be doing the exact same thing as "doStuff" the pin for pin_B, and pin_X don't work... I also sometimes get pin_X flashing low along at the same time I loop to the next step (just a quick low back to high, rather than staying low for the full 1 second).

I'm sure it's not the hardware since this doesn't happen on doStuff, Also I'm outputting the raw data that I'm processing to serial and everything seem to be correct.... I'm at a loss as to why this is happening

Code: [Select]
/*
runs through an automated sequence when the button is pushed
*/

#include <avr/pgmspace.h> //this is so we can use flash memory instead of SRAM

//set the binary button settings 4 bytes
#define A     0x80000000 //B10000000000000000000000000000000
#define B     0x40000000 //B01000000000000000000000000000000
#define X     0x20000000 //B00100000000000000000000000000000
#define Y     0x10000000 //B00010000000000000000000000000000
#define LB    0x08000000 //B00001000000000000000000000000000
#define RB    0x04000000 //B00000100000000000000000000000000
#define Select  0x02000000 //B00000010000000000000000000000000
#define Up    0x01000000 //B00000001000000000000000000000000
#define Down  0x00800000 //B00000000100000000000000000000000
#define Left  0x00400000 //B00000000010000000000000000000000
#define Right 0x00200000 //B00000000001000000000000000000000

PROGMEM prog_uint32_t steps[] = {
A+1000,
B+1000,
X+1000,
Y+1000,
LB+1000,
RB+1000,
Back+1000,
Up+1000,
Down+1000,
Left+1000,
Right+1000,
};

long sizeOfSeq = (sizeof(steps) / sizeof(steps[0])); //determine the length of the sequence

//set names for the output pins
int pin_A = 4;
int pin_B = 0;
int pin_X = 1;
int pin_Y = 2;

int pin_RB = 12;
int pin_LB = 7;

int pin_Select = 13;

int pin_Up = 11;
int pin_Down = 10;
int pin_Left = 9;
int pin_Right = 8;

int pin_Go = 3;

byte state = HIGH; // The macro state 0=running 1=stopped
byte lastButtonState = HIGH; // The go button state toggle 0=depressed
unsigned long lastPushTime = 0; //the millis since the last time the button was pushed
unsigned long currentMillis = 0; //get the start time
unsigned long previousMillis = 0; //get the current time

// the setup routine runs once when you press reset:
void setup() {
 // initialize the digital pins as an output.
 pinMode(pin_A, OUTPUT);
 pinMode(pin_B, OUTPUT);
 pinMode(pin_X, OUTPUT);
 pinMode(pin_Y, OUTPUT);
 pinMode(pin_LB, OUTPUT);
 pinMode(pin_RB, OUTPUT);
 pinMode(pin_Select, OUTPUT);
 pinMode(pin_Up, OUTPUT);
 pinMode(pin_Down, OUTPUT);
 pinMode(pin_Left, OUTPUT);
 pinMode(pin_Right, OUTPUT);
 
 //setup interrupt    
 pinMode(pin_Go, INPUT);

 //make sure everything is in the starting state that we want
 digitalWrite(pin_Select, LOW); //try to get it set off
 allClear();
}

//sets all outputs to off (high)
void allClear(){
 digitalWrite(pin_Up, HIGH);
 digitalWrite(pin_Down, HIGH);
 digitalWrite(pin_Left, HIGH);
 digitalWrite(pin_Right, HIGH);
 digitalWrite(pin_A, HIGH);
 digitalWrite(pin_B, HIGH);
 digitalWrite(pin_X, HIGH);
 digitalWrite(pin_Y, HIGH);
 digitalWrite(pin_LB, HIGH);
 digitalWrite(pin_RB, HIGH);
 digitalWrite(pin_Select, HIGH);
}

// the loop routine runs over and over again forever:
void loop() {
 checkPushed(); //check if the button was pushed
 if (state == LOW){
   //doStuff();
   runSeq();
   allClear();
   state = HIGH;
 }
}

// the interrput routine run with the go button is pushed
void checkPushed(){
 byte buttonState = digitalRead(pin_Go);
 if (lastButtonState == HIGH && buttonState == LOW){
   unsigned long pushTime = millis(); //check the time
 
   // If pushed again within 60ms, assume it's a bounce and ignore
   if (pushTime - lastPushTime > 60){
     state = !state; //change state
     lastPushTime = pushTime; //set the last check time
   }
 }
 lastButtonState = buttonState;
}

// check for the button push
void myDelay(unsigned long interval){
 currentMillis = millis(); //get the current time
 previousMillis = currentMillis; //get the start time
 while (currentMillis - previousMillis < interval){ //loop until the start time plus the desired delay is reached
   currentMillis = millis(); //get the current time
   checkPushed();
   if (state){
     return;
   }
 }
}

void runSeq(){
 unsigned long seq = 0;
 Serial.begin(9600);
 for(int i=0; i<sizeOfSeq; i++){ //loop through each step in the sequence
   seq = pgm_read_dword(&(steps[i])); //reads the steps from the included file

   Serial.print("seq ");
   Serial.print(i, DEC);
   Serial.print(": ");
   Serial.println(seq, BIN);

   if ((seq&A    ) == A    ) {digitalWrite(pin_A    , LOW);} else {digitalWrite(pin_A    , HIGH);}
   if ((seq&B    ) == B    ) {digitalWrite(pin_B    , LOW);} else {digitalWrite(pin_B    , HIGH);}
   if ((seq&X    ) == X    ) {digitalWrite(pin_X    , LOW);} else {digitalWrite(pin_X    , HIGH);}
   if ((seq&Y    ) == Y    ) {digitalWrite(pin_Y    , LOW);} else {digitalWrite(pin_Y    , HIGH);}
   if ((seq&LB   ) == LB   ) {digitalWrite(pin_LB   , LOW);} else {digitalWrite(pin_LB   , HIGH);}
   if ((seq&RB   ) == RB   ) {digitalWrite(pin_RB   , LOW);} else {digitalWrite(pin_RB   , HIGH);}
   if ((seq&Select) == Select) {digitalWrite(pin_Select , LOW);} else {digitalWrite(pin_Select , HIGH);}
   if ((seq&Up   ) == Up   ) {digitalWrite(pin_Up   , LOW);} else {digitalWrite(pin_Up   , HIGH);}
   if ((seq&Down ) == Down ) {digitalWrite(pin_Down , LOW);} else {digitalWrite(pin_Down , HIGH);}
   if ((seq&Left ) == Left ) {digitalWrite(pin_Left , LOW);} else {digitalWrite(pin_Left , HIGH);}
   if ((seq&Right) == Right) {digitalWrite(pin_Right, LOW);} else {digitalWrite(pin_Right, HIGH);}
   
   myDelay(seq&0x000fffff);
   if (state){
     return;
   }
 }
}

// the output sequence routine
void doStuff(){
 digitalWrite(pin_A, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_A, HIGH);
 
 digitalWrite(pin_B, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_B, HIGH);
 
 digitalWrite(pin_X, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_X, HIGH);
 
 digitalWrite(pin_Y, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_Y, HIGH);
 
 digitalWrite(pin_LB, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_LB, HIGH);
 
 digitalWrite(pin_RB, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_RB, HIGH);
 
 digitalWrite(pin_Select, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_Select, HIGH);
 
 digitalWrite(pin_Up, LOW);
 myDelay(1000);
 if (state){
   return;
 }
digitalWrite(pin_Up, HIGH);

 digitalWrite(pin_Down, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_Down, HIGH);
 
 digitalWrite(pin_Left, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_Left, HIGH);
 
 digitalWrite(pin_Right, LOW);
 myDelay(1000);
 if (state){
   return;
 }
 digitalWrite(pin_Right, HIGH);
}

Nick Gammon

Code: [Select]

void runSeq(){
  unsigned long seq = 0;
  Serial.begin(9600);


Do Serial.begin in setup, not every time through loop.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

twistedsymphony

moved... though that doesn't seem to change the results.

Nick Gammon

Is this right? ...

Code: [Select]

    seq = pgm_read_dword(&(steps[i])); //reads the steps from the included file


Anyway, I wouldn't bother putting such a small array into PROGMEM.
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

twistedsymphony

the only reason it's small is because I'm testing it out, the final version will have hundreds of elements in the array.

Nick Gammon

What do your debugging displays tell you? (perhaps paste them here).
Please post technical questions on the forum, not by personal message. Thanks!

More info:
http://www.gammon.com.au/electronics

Go Up