user31524: I'm trying to solve a nagging issue with external interrupts - this is for a slot car track lap count controller. It uses the Mega's 6 external interrupt lines connected to photo diodes. The HW itself works fine - yes, trust me, it's fantastic.
I have observed the following. The code in the loop() is surrounded with:
detachAllInterrupts();
delay(10);
...
... process any interrupts and/or button presses
...
delay(10);
attachAllInterrupts();

If I omit or reduce the delay amount the random firing gets worse.
The ISR is lean and mean:
void lapDetected1() {
  lane1.lapDetected();
}

calling the method of class Lane:
    void lapDetected() { // called by ISR, short and sweet
      now = millis();
      if ((now - finish) < laneProtectionTime) {
        return;
      }
      start = finish;
      finish = now;
      count++;
      reported = false;
    }

all the variables used are declared volatile.
I don't like to use delay and I'm seeking advice on how to correctly implement ISRs and their handling.
here's the link to the entire code on GitHub: https://github.com/gabe2001/PCLapCounterWithArduino/
Thanks for any advice I can get here!
-gabe

user34085: Now, you write the following: 
detachAllInterrupts();
delay(10);
...
... process any interrupts and/or button presses
...
delay(10);
attachAllInterrupts();

I understand that you put this stuff into the loop, such that the loop in effect looks like this:
void loop() {
    detachAllInterrupts();
    delay(10);
    ...
    ... process any interrupts and/or button presses
    ...
    delay(10);
    attachAllInterrupts();
}

Assuming that the above is correct, then you will be enabling and disabling your interrupts in one long stream. You would often NOT disable your interrupts, because you don't know when the are available or not, then.
You should typically ENABLE the interrupts in the startup code, and then in the loop() you can have any Other processing you want, but not any processing referring to the interrupt processing. 
The interrupt processing would take care of the button pressed etc. So - in the lane.lapdetected1() algorithm, you would take care of reading buttons pressed etc., and, you would have to attach the interrupt itself directly to your algorithm. I cannot see that you have told that when an interrupt has been happening, then the code execution goes to the ISR lane.lapdetected1() algorithm?
What you would want to do, is to setup an interrupt in the setup(), which is run once upon start of the program in the arduino, and then, providing that you want the interrupt on pin 2 (external pin), and the algorithm to run while you receive falling signals on the pin 2 is lapdetected1, then you could use the following:
void setup() {
  attachInterrupt(digitalPinToInterrupt(2), lapdetected1, FALLING);
}

Thereby you would remove the detachAllInterrupts(), and all the way down to attachAllInterrupts() from the loop construction and thereby you would be able now to process interrupts within the lapdetected1-algorithm and run what ever you would want to run, normally, from within the loop-construction.

31524: <p>I'm trying to solve a nagging issue with external interrupts - this is for a slot car track lap count controller. It uses the Mega's 6 external interrupt lines connected to photo diodes. The HW itself works fine - yes, trust me, it's fantastic.</p>

<p>I have observed the following. The code in the loop() is surrounded with:</p>

<pre><code>detachAllInterrupts();
delay(10);
...
... process any interrupts and/or button presses
...
delay(10);
attachAllInterrupts();
</code></pre>

<p>If I omit or reduce the delay amount the random firing gets worse.</p>

<p>The ISR is lean and mean:</p>

<pre><code>void lapDetected1() {
  lane1.lapDetected();
}
</code></pre>

<p>calling the method of class Lane:</p>

<pre><code>    void lapDetected() { // called by ISR, short and sweet
      now = millis();
      if ((now - finish) &lt; laneProtectionTime) {
        return;
      }
      start = finish;
      finish = now;
      count++;
      reported = false;
    }
</code></pre>

<p>all the variables used are declared <em>volatile</em>.</p>

<p>I don't <em>like</em> to use delay and I'm seeking advice on how to correctly implement ISRs and their handling.</p>

<p>here's the link to the entire code on GitHub: <a href="https://github.com/gabe2001/PCLapCounterWithArduino/" rel="nofollow noreferrer">https://github.com/gabe2001/PCLapCounterWithArduino/</a></p>

<p>Thanks for any advice I can get here!
-gabe</p>
 34085: <p>Now, you write the following: </p>

<pre><code>detachAllInterrupts();
delay(10);
...
... process any interrupts and/or button presses
...
delay(10);
attachAllInterrupts();
</code></pre>

<p>I understand that you put this stuff into the loop, such that the loop in effect looks like this:</p>

<pre><code>void loop() {
    detachAllInterrupts();
    delay(10);
    ...
    ... process any interrupts and/or button presses
    ...
    delay(10);
    attachAllInterrupts();
}
</code></pre>

<p>Assuming that the above is correct, then you will be enabling and disabling your interrupts in one long stream. You would often NOT disable your interrupts, because you don't know when the are available or not, then.</p>

<p>You should typically ENABLE the interrupts in the startup code, and then in the <code>loop()</code> you can have any Other processing you want, but not any processing referring to the interrupt processing. </p>

<p>The interrupt processing would take care of the button pressed etc. So - in the <code>lane.lapdetected1()</code> algorithm, you would take care of reading buttons pressed etc., and, you would have to attach the interrupt itself directly to your algorithm. I cannot see that you have told that when an interrupt has been happening, then the code execution goes to the ISR <code>lane.lapdetected1()</code> algorithm?</p>

<p>What you would want to do, is to setup an interrupt in the <code>setup()</code>, which is run once upon start of the program in the arduino, and then, providing that you want the interrupt on pin 2 (external pin), and the algorithm to run while you receive falling signals on the pin 2 is lapdetected1, then you could use the following:</p>

<pre><code>void setup() {
  attachInterrupt(digitalPinToInterrupt(2), lapdetected1, FALLING);
}
</code></pre>

<p>Thereby you would remove the <code>detachAllInterrupts()</code>, and all the way down to <code>attachAllInterrupts()</code> from the loop construction and thereby you would be able now to process interrupts within the lapdetected1-algorithm and run what ever you would want to run, normally, from within the loop-construction.</p>
