Strange Interrupt behaviour - any ideas why?

I am using a QRE1113 reflective optical detector to count revolutions and measure the speed for a small DC motor (one pulse per revolution). My ISR records the value of micros() and the code in loop() calculates the interval between interrupts - i.e. the time for a single revolution.

Using attachInterrupt(0, revMicrosISR, RISING); and this ISR code

void revMicrosISR() {
		isrRevMicros = micros();
		isrRevCount ++;
		newRevMicros = true;
}

I get the correct interval between pulses most of the time (about 15000 µsecs) but maybe 10% of the time I get spurious values as low as 48 and anything in between.

However I have discovered that if I use a CHANGE interrupt and this code

void revMicrosISR() {
	isrPinVal = digitalRead(2);
	if (isrPinVal == HIGH && prevIsrPinVal == LOW) {
		isrRevMicros = micros();
		isrRevCount ++;
		newRevMicros = true;
	}
	prevIsrPinVal = isrPinVal;
}

I get correct values all the time.

According to my oscilloscope the timing is very regular.

Just out of curiosity I wonder if anyone has any idea why the two approaches should give different results.

...R

Are the variables in the ISR's defined as volatile?

but maybe 10% of the time I get spurious values as low as 48 and anything in between.

I would guess because of bouncing.

Just out of curiosity I wonder if anyone has any idea why the two approaches should give different results.

The second code calls digitalRead(), which is not a very fast function. Perhaps it is just slowing things down enough so that the next (spurious) interrupt doesn't happen.

If you use direct port manipulation to read the pin, does that affect the results?

I suspect its random noise on the opposite edge of the signal. Very difficult to debounce with RISING or FALLING mode (especially if the signal is a squarewave), very easy to debounce with CHANGE mode.

...check my latest update that works with all modes.

EDIT: If you decide to give it a try, just use stableWidth = 200000 / the max frequency that you're using the sensor. If your using it at up to 10Hz, use stableWidth = 20000 µs (debounce interval).

Robin, we're going to have to ask you for a schematic. 8)

I've been doing some work with that same sensor this week. It's not really a digital device. It needs a comparator or some kind of adjustable threshold to decide between 'detected' and 'not detected'. As the motor arm approaches the sensor, the response curve will rather directly measure the distance to the arm.

For my usage, I just ran it to an analog input and I apply the threshold in software. If you must use an interrupt, then I expect some external circuitry is necessary.

For extra complexity, have a look at how Sparkfun does the "digital" version of this sensor. It's not digital at all. It just uses a digital pin to charge and discharge a capacitor, giving a numerical output to the distance-sensed.

I might try what @PaulS has suggested in Reply #2. In a version that I did not post I thought I had tried to introduce a short delay but I realized after I posted the question that I had forgotten to clear the interrupt flag before I re-enabled the interrupt.

The circuit I am using is the same as the Sparkfun analog circuit except that I am using a 56k resistor instead of the 10k resistor - seems to work better. I have the wire marked OUT connected to Pin 2.

QRE1113 analog circuit.png

The added complexity of @dlloyd’s de-bounce code does not seem to be necessary.

All the relevant variables are volatile.

…R

Robin2:
and the code in loop() calculates the interval between interrupts - i.e. the time for a single revolution.

Another possibility is a race condition. Post the relevant code from loop.

In the CHANGE version I tried replacing digitalRead(2); with (PIND & 0b00000100) >> 2; and it made no difference - both versions work properly.

I can't think why there might be a race because each revolution takes about 15000 µsec.

This is the code that calculates the interval between interrupts - it is called directly from loop()

void calcRevMicros() {
	if (newRevMicros == true) {
		prevRevMicros = latestRevMicros;
		noInterrupts();
			latestRevMicros = isrRevMicros;
			totalRevs = isrRevCount;
			newRevMicros = false;
		interrupts();
		revMicros = latestRevMicros - prevRevMicros;
}

...R

Robin2:
I can't think why there might be a race because each revolution takes about 15000 µsec.

Agree. I can't see anything suspicious.

This is the code that calculates the interval between interrupts - it is called directly from loop()

I can see one very minor improvement but it won't have any affect on this problem.

PaulS:
I would guess because of bouncing.

That's the horse I'm backing.

In the absence of other ideas I agree that bouncing seems likely to be the culprit but I don't immediately see how the CHANGE version of the interrupt eliminates or avoids it.

As I have a simple working solution I don't think I will spend more time exploring this.

@Coding Badly, do you want to share your minor improvement?

...R

Being a RISC processor data has to first be loaded into a register. Registers are used for local variables. Given those two facts, this should produce the same code except that some of the instructions will be moved out of the critical section reducing the amount of time interrupts are disabled...

void calcRevMicros() 
{
  uint32_t tempRevMicros;
  uint16_t tempRevCount;

  if (newRevMicros == true) 
  {
    noInterrupts();
    tempRevMicros = isrRevMicros;
    tempRevCount = isrRevCount;
    newRevMicros = false;
    interrupts();

    prevRevMicros = latestRevMicros;
    latestRevMicros = tempRevMicros;
    totalRevs = tempRevCount;

    revMicros = latestRevMicros - prevRevMicros;
  }

You do have a race condition…

void calcRevMicros() 
{
	if (newRevMicros == true) 
	{
		prevRevMicros = latestRevMicros;

// <<<<< An interrupt here corrupts the state >>>>>

		noInterrupts();

I believe the code in #10 resolves the problem.

However, if I’m correct, a RISING interrupt occurs before you process the previous interrupt. Which may or may not indicate a bigger issue (like a bounce).

@Coding Badly, thanks. Two interesting points.

Am I correct in thinking that your "race" problem is because an interrupt at the place you mention would have the result of prevRevMicros holding an out of date value?

When I designed the program I did not anticipate that because the interval between interrupts "should" be long.

However I remain puzzled by why the other version using a CHANGE interrupt does not exhibit the same problem.

I may do some more thinking.

...R

I modified the code a bit so I could see the results for 50 consecutive interrupts and it appears that glitches also appear when using the CHANGE interrupt - but not as often as with the RISING interrupt. If the correct value is about 12000 I occasionally get a pair of shorter values - say 8000 and 4000 - which clearly indicate an extra interrupt has happened within the period of the regular pulse.

I think the simplest thing may be to reject readings that are significantly different from the norm. Taking a simple average is not the answer.

Incidently, using the concept in Reply #10 slightly increased the number of glitches.

...R

I don't think the picture in Reply #14 illustrates the difference I appear to be experiencing.

If you imagine taking the lowest trace and checking for a change of State from LOW to HIGH I think it would give the same result as the middle trace - but that is not happening with my system, or at least not as often.

I wonder if my problem may be partial changes of state in the signal - i.e. occasionally going just far enough to be detected by the Arduino, but most of the time not doing so. The signal is not nice and square like the diagrams in Reply #14.

I have not done any more with this since my last posting but I think I might use the RISING interrupt and disable it for (say) 75% of the duration of the previous pulse - on the grounds that the speed is not going to change much from one revolution to the next.

...R

Sounds like unnecessarily complex debouncing. Just decide on a threshold which represents rotation faster than it can physically rotate. If you experience an interrupt inside that debounce threshold then don't update the variable.

Robin2:
Using attachInterrupt(0, revMicrosISR, RISING); and this ISR code

void revMicrosISR() {

isrRevMicros = micros();
isrRevCount ++;
newRevMicros = true;
}

Is micros() safe in an ISR? It doesn’t seem like it is. All it’s doing is taking TCNT0, combining it with a count of how many times TCNT0 overflowed, and multiplying by 4us/tick:

unsigned long micros() {
 unsigned long m;
 uint8_t oldSREG = SREG, t;
 
 cli();
 m = timer0_overflow_count;
#if defined(TCNT0)
 t = TCNT0;
#elif defined(TCNT0L)
 t = TCNT0L;
#else
 #error TIMER 0 not defined
#endif

#ifdef TIFR0
 if ((TIFR0 & _BV(TOV0)) && (t < 255))
 m++;
#else
 if ((TIFR & _BV(TOV0)) && (t < 255))
 m++;
#endif

 SREG = oldSREG;
 
 return ((m << 8) + t) * (64 / clockCyclesPerMicrosecond());
}

If Timer 0 overflows in between your ISR firing and micros() reading TCNT0, you are going to get a reading thats ~1000us too short.

That said, if this was the root cause of your problem it should have gotten worse by making your ISR longer…not better.

Edited to Add: It seems this has been thought of before. Because micros() looks at the Timer 0 interrupt flag, it’d need to rollover twice to get an error → http://forum.arduino.cc/index.php?topic=271092.msg1911822#msg1911822

Robin2:
I have not done any more with this since my last posting but I think I might use the RISING interrupt and disable it for (say) 75% of the duration of the previous pulse - on the grounds that the speed is not going to change much from one revolution to the next.
...R

That may work (unless an undesired interrupt occurs in the remaining 25%).

Have you tried using FALLING interrupt mode? This mode will be more immune to noise on the ground rail. This seems to be the most commonly used interrupt mode for use in noisy systems. EDIT: For falling mode, use a strong external pullup resistor ... the internal pullup is weak and makes the signal more susceptible to noise.

I did try FALLING - but with the same 56k resistor as I am using for RISING and CHANGE. It is neatly soldered in now and I am reluctant to change it. I did not see any difference between FALLING and RISING.

I have made a first try with a version using Timer1 to "wait" before re-enabling the external interrupt and it seems to solve the problem. I need to spend a bit more time figuring out the ideal prescale value and count range. One of the complications is that I am testing at 16MHz and 5v but it will have to work at 8MHz and 3.6v.

BigBobby:
If Timer 0 overflows in between your ISR firing and micros() reading TCNT0,

I don't see how it could because my ISR would have disabled interrupts.

...R