Why my INTERRUPT doesn't work

Hi.
I’m using an INTERRUPT for the 1st time and I can’t get it working.
Any idea ?

//LCArduiDef.h LearnCbot  definitions Arduino
#include < avr/interrupt.h >
#include <Arduino.h>
#define  Led13  13  
#define   Led13On  digitalWrite (Led13,1)
#define   Led13Off digitalWrite (Led13,0)
#define   Led13Toggle
#define HP  1
#define   HPOn   digitalWrite (HP,1) 
#define   HPOff  digitalWrite (HP,0)
#define   HPToggle  digitalWrite (HP,!digitalRead(HP))
#define L1  4  
#define L2  5
#define L3  6  
#define L4  7
#define   Led1On   digitalWrite (L1,1) 
#define   Led1Off  digitalWrite (L1,0)
#define   Led1Toggle  digitalWrite (L1,!digitalRead(L1))
#define   Led2On   digitalWrite (L2,1) 
#define   Led2Off  digitalWrite (L2,0)
#define   Led2Toggle  digitalWrite (L2,!digitalRead(L2))
#define   Led3On   digitalWrite (L3,1) 
#define   Led3Off  digitalWrite (L3,0)
#define   Led3Toggle  digitalWrite (L3,!digitalRead(L3))
#define   Led4On   digitalWrite (L4,1) 
#define   Led4Off  digitalWrite (L4,0)
#define   Led4Toggle  digitalWrite (L4,!digitalRead(L4))
#define P1  2  
#define P2  3
#define   Pous1On  (!digitalRead(P1)) 
#define   Pous2On  (!digitalRead(P2))

void LcSetup () {
  pinMode (1,1); 
  pinMode (2,0); 
  pinMode (3,0); 
  digitalWrite (2, 1); 
  digitalWrite (3, 1);
  pinMode (4,1); 
  pinMode (5,1); 
  pinMode (6,1); 
  pinMode (7,1);
}
void avance () {
  Led2Off;
  Led4Off;
  Led1On;
  Led3On;
}
void ISR1 () {
  Led1Off;
  Led3Off;
  Led2On;
  Led4On;
  delay(500);
  Led2Off;
  Led1On;
  delay(1000);

}





void setup() {
  // put your setup code here, to run once:
  LcSetup ();
  attachInterrupt(P1, ISR1, FALLING);
  attachInterrupt(P2, ISR1, FALLING);
  //attachInterrupt(P2, ISR2, FALLING)
}

void loop() {
  // put your main code here, to run repeatedly: 
  while (!Pous1On && !Pous2On){
    avance();
  }


}

You can't use delay() inside an ISR, because it requires interrupts to be enabled. Interrupts are disabled during interrupt handling.

Here is one problem...

  attachInterrupt(P1, ISR1, FALLING);
  attachInterrupt(P2, ISR1, FALLING);

The attachInterrupt function takes an interrupt number, not a pin number. On the UNO they are 0 and 1.

DavidOConnor:
Here is one problem...

  attachInterrupt(P1, ISR1, FALLING);

attachInterrupt(P2, ISR1, FALLING);




The attachInterrupt function takes an interrupt number, not a pin number. On the UNO they are 0 and 1.

P2 IS a pin number (as a #define).

P2 IS a pin number (as a #define).

Yes it is, and it should be an interrupt number instead.

Hiya, I'm a newbie too, but had trouble using external interrupts too. I may be quite wrong but I think you have defined the ISR wrongly....

void ISR1 () {
Led1Off;
Led3Off;
Led2On;
Led4On;
delay(500);
Led2Off;
Led1On;
delay(1000);

I think it should read:

ISR(serviceRoutineName) {
Led1Off;
Led3Off;
Led2On;
Led4On;
delay(500);
Led2Off;
Led1On;
delay(1000);

Where "serviceRoutineName" is the name you gave your interrupt routine ...currently "ISR1"

In my view, putting a delay in your ISR is fine so long as you understand the implications. In my case I had no indication that the routine was wrongly defined by the compiler, and it took some research to figure it out.

I think that the Reference section about attaching the ISR on the Arduino needs updating to say words about how to create the ISR in the first place....the example should not work too.

Anyhow hope this helps

happymacer:
Hiya, I'm a newbie too, but had trouble using external interrupts too. I may be quite wrong but I think you have defined the ISR wrongly....

Using attachInterrupt() you can use any name for your interupt service routine.

RTFM

...R

I suggest writing a much more simple sketch just to test the interrupt.

And don't attempt to do anything in the interrupt routine, except to set some kind of flag variable ( which must be volatile ), and then check the status of that variable, in loop( )

  Led1Off;
  Led3Off;
  Led2On;
  Led4On;

What are these supposed to be doing? That is not how to call a function, if that is what you were trying to do.

Also, you've said nothing about what is supposed to trigger this interrupt, but whatever it is, you are going about this all wrong. An interrupt is like the alarm going off. It need to be handled now, and it needs to be handled quickly. It is not like a doorbell ringing, where the person ringing the doorbell could wait a few seconds.

happymacer:
In my view, putting a delay in your ISR is fine so long as you understand the implications.

The implications are that it won't work.

Interrupts are almost never the most sensible way to respond to button presses. I doubt that it is the best way here.

I strongly dislike the way you have implemented code as preprocessor macros. Putting code in macros is poor practice at the best of times, but especially when it doesn't maintain correct language syntax at the point the macros are used.

In my view, putting a delay in your ISR is fine so long as you understand the implications.

Since delay() relies on interrupts, which don't happen while an ISR is running, the "implications" are that the code will freeze. As long as your are fine with the Arduino entering a block of code that it can never exit from, feel free to do stupid things like call delay() in an ISR.