Pages: 1 [2] 3   Go Down
Author Topic: Interrupt still happening after detached. ( fix now available)  (Read 2845 times)
0 Members and 1 Guest are viewing this topic.
Global Moderator
Offline Offline
Brattain Member
*****
Karma: 474
Posts: 18696
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

I think the documentation for attachInterrupt should be amended to strongly suggest doing the lines above. I agree that you don't expect an interrupt to immediately register from some action that you might have taken an hour ago, just because you attach interrupts. You reasonably expect them to be effective "now" not in the past.

There is also a reasonably strong case for amending attachInterrupt to do it for you. After all, attachInterrupt is supposed to be shielding you from having to know all that low-level stuff.
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 474
Posts: 18696
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

In fact it should probably be more like this:

Code:
  uint8_t oldSREG = SREG;
  cli();
  EIFR |= (1 << INTF0);    // clear any outstanding interrupt 0
  SREG = oldSREG;

Just in case there is an interrupt between reading EIFR and or'ing in the new value.
Logged

Left Coast, USA
Offline Offline
Sr. Member
****
Karma: 5
Posts: 499
Sometimes I just can't help myself.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

...you should "or" it in (or you have the effect of clearing the other interrupt flag...
No you don't.  Here's how it works:

If you write a '0' to one of the interrupt flag bits it doesn't affect that bit.  If you write a '1' to one of the interrupt flags it makes sure that that bit is cleared to '0'.

There is absolutely no reason to "or" the bit into the interrupt flag register.  Just write the '1' to whatever bit position corresponds to the flag that you want to clear, and the other bits are not changed.  That's why the ATmega designers did it this way and that's why I suggested the code that I showed.

Quote from: Nick Gammon
In fact it should probably be more like this:

Code:
 uint8_t oldSREG = SREG;
  cli();
  EIFR |= (1 << INTF0);    // clear any outstanding interrupt 0
  SREG = oldSREG;

Just in case there is an interrupt between reading EIFR and or'ing in the new value.

Well, if you just write the bit instead of "oring" into EIFR you don't need to do the SREG thing.  Period.  Full stop.



Regards,

Dave
« Last Edit: April 23, 2011, 07:55:45 am by davekw7x » Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 474
Posts: 18696
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Good point. Well that simplifies things.
Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 85
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks for the replies.   
So then would this be the final code then? :

// put these 2 lines before Setup? or in Setup?
#define INTF0 0
#define INTF1 1

EIFR = (1 << INTF0);   //use before attachInterrupt(0,isr,xxxx) to clear interrupt 0 flag
EIFR = (1 << INTF1);   //use before attachInterrupt(1,isr,xxxx) to clear interrupt 1 flag

Logged

Left Coast, USA
Offline Offline
Sr. Member
****
Karma: 5
Posts: 499
Sometimes I just can't help myself.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


// put these 2 lines before Setup? or in Setup?
#define INTF0 0
#define INTF1 1
No.  They are already defined.  Do not re-define them!


Quote from: Loudhvx
EIFR = (1 << INTF0);   //use before attachInterrupt(0,isr,xxxx) to clear interrupt 0 flag
EIFR = (1 << INTF1);   //use before attachInterrupt(1,isr,xxxx) to clear interrupt 1 flag
That would work.

Just take the code you have been testing with and put the "EIFR = (1<< INTF0); " statement before the call to attachInterrupt().

Let us know if it works out.


Regards,

Dave
« Last Edit: April 23, 2011, 10:45:51 am by davekw7x » Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 85
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I ran 12 permutations of the 4 clearing methods against each interrupt and both interrupts simultaneously.  (Just to make sure nothing unexpected happened)  (Of course, there are probably more conditions I didn't think of.)

Both of these methods work, and work exclusively on the interrupt flag they are meant to clear.

EIFR = 1;   //use before attachInterrupt(0,isr,xxxx) to clear interrupt 0 flag
EIFR = 2;   //use before attachInterrupt(1,isr,xxxx) to clear interrupt 1 flag

or
 
EIFR = (1 << INTF0);   //use before attachInterrupt(0,isr,xxxx) to clear interrupt 0 flag
EIFR = (1 << INTF1);   //use before attachInterrupt(1,isr,xxxx) to clear interrupt 1 flag


Which method would be better to advise users in the Arduino's attachInterrupt() documentation?

In my opinion, even though the first method looks more straight forward, the latter keeps the 0 and 1 aligned with the interrupt numbers, so would be less prone to an accidental mismatch.  There are probably other advantages to the latter method as well?
« Last Edit: April 23, 2011, 11:51:40 am by Loudhvx » Logged

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 291
Posts: 25854
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Which method would be better to advise users in the Arduino's attachInterrupt() documentation?
Why not write an "ATTACH_INTERRUPT" macro that hides it?
Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Offline Offline
Jr. Member
**
Karma: 0
Posts: 85
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I would think, eventually, the fix should be included in the attachInterrupt() routine itself, unless there is some unforseen down-side.  (As Nick mentioned, as unlikely as it may seem, maybe some sketches out there are using the pre-triggered interrupts?)
« Last Edit: April 23, 2011, 12:01:14 pm by Loudhvx » Logged

Left Coast, USA
Offline Offline
Sr. Member
****
Karma: 5
Posts: 499
Sometimes I just can't help myself.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

...these methods work

The compiler evaluates constant expressions at compile time and just sticks the value into the code appropriately.  (No run-time operations are required to do the bit shifting in the statements we are talking about here.)

The code generated by the statement EIFR = 1; is exactly the same as that generated by the statement EIFR = (1 << INTF0);

Quote from: Loudhvx
Which method would be better to advise users in the Arduino's attachInterrupt() documentation?
Use symbolic identifiers, not "magic numbers."

In other words don't use EIFR = 1;

Another possibility (with the exact same results) is to use the avr gcc/libc macro _BV  (stands for Bit Value);

_BV(INTF0) is the same as (1 << INTF0)
_BV(INTF1) is the same as (1 << INTF1)

Etc.

Since the shift operation is standard C (and C++) and the _BV requires a vendor-supplied macro, many programmers will prefer the shift stuff.  (Believe it or not, some people actually program things other than Arduino boards and with compilers other than avr-gcc, and they like to keep things vendor-independent wherever it's practical.  Really.)



Regards,

Dave
« Last Edit: April 23, 2011, 12:03:11 pm by davekw7x » Logged

Offline Offline
Jr. Member
**
Karma: 0
Posts: 85
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

(Believe it or not, some people actually program things other than Arduino boards and with compilers other than avr-gcc, and they like to keep things vendor-independent wherever it's practical.  Really.)

Thanks for the clarification.   smiley-lol
Logged

0
Offline Offline
Full Member
***
Karma: 0
Posts: 173
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I have reproduced it on a Uno.

Extensive reading and testing seems to indicate what the problem is. Rather bizarrely the processor, when attachInterrupt is called, can "remember" whether the interrupt condition has already been met. Hence, if you ground the pin, and then call attachInterrupt, the interrupt fires.

This proves it:

Code:
volatile int flag = 0;

void setup() {
  Serial.begin (115200);
  digitalWrite(2, HIGH);      //pullup 2

  pinMode(13, OUTPUT);       //LED
  Serial.println ("go!");
  digitalWrite(13, HIGH);                 //time to go!
  delay (5000);
  digitalWrite(13, LOW);                 //time is up!
  
   // interrupt on
  attachInterrupt(0, isr0, FALLING);     //enable interrupt

  // did we get an interrupt?
  Serial.println (flag, DEC);
  }

void loop() {}

void isr0 () {
  flag = 1;
}

There is no detachInterrupt there. After the sketch starts and displays "go" you have 5 seconds to ground (and release) pin 2. If you do, and only if, then it displays 1, otherwise it displays 0. And this is despite the fact that you generate the "interrupt" before interrupts are enabled on that pin.

Now, add this line just before attaching the interrupt:

Code:
EIFR = 1;

That clears the interrupt flag (for interrupt 0). According to the datasheet:

Quote
The flag is cleared when the interrupt routine is executed. Alternatively, the flag can be cleared by writing a logical one to it.

So this is manually clearing the "I saw an interrupt" flag, before enabling interrupts.

Arguably, attachInterrupt should do this. And note that detachInterrupt has no effect on the flag at all.

I don't know whether there would be programs around that rely on interrupts being "pre-triggered" before you even attach the interrupt. Perhaps there are.

Note that if you are using interrupt 1, it would be:

Code:
EIFR = 2;


Thanks Nick.

I've been pulling my hair out thinking  I had some electrical issue on my project.
I had suspected it was something to do with attach/detach but couldn't nail it down.

I'm using an UNO- REV2 (does the rev make a difference for this problem)?
Which Rev did you use to duplicate this?

Should this be treated as a software bug and implemented in the compiler, so the user doesn't have to remember to clear the interrupt manually?
Logged

0
Offline Offline
Full Member
***
Karma: 0
Posts: 173
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I think the documentation for attachInterrupt should be amended to strongly suggest doing the lines above. I agree that you don't expect an interrupt to immediately register from some action that you might have taken an hour ago, just because you attach interrupts. You reasonably expect them to be effective "now" not in the past.

There is also a reasonably strong case for amending attachInterrupt to do it for you. After all, attachInterrupt is supposed to be shielding you from having to know all that low-level stuff.

Agreed. Has this issue been formally reported to the maintainers of the IDE / compiler?
Perhaps it's already been fixed in 1.0, but that release apparently has enough problems to avoid using it for a while.
Logged

Global Moderator
Offline Offline
Brattain Member
*****
Karma: 474
Posts: 18696
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Perhaps it's already been fixed in 1.0, but that release apparently has enough problems to avoid using it for a while.

My copy of version 1.0 RC2 does not appear to have any mention of EIFR in attachInterrupt.
Logged

0
Offline Offline
Full Member
***
Karma: 0
Posts: 173
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Perhaps it's already been fixed in 1.0, but that release apparently has enough problems to avoid using it for a while.

My copy of version 1.0 RC2 does not appear to have any mention of EIFR in attachInterrupt.

Thanks. Do we know if this issue has been officially reported to the maintainers?
Logged

Pages: 1 [2] 3   Go Up
Jump to: