[SOLVED] Dealing with a noisy interrupt signal

Using Arduino Programming Questions
1

EDIT: Latest Results and Code at Reply #47

I wrote this code to test a nasty interrupt signal and create a clean output signal. It can be used to test your own interrupt routines to see how it would respond. I used an Uno ... all that's needed is one jumper wire from pin 13 to pin 2. View the results on the Serial Plotter.


Blue trace: Noisy signal with unwanted interrupts (2 on rising and 2 on falling). Signal alternates between normally high and normally low.
Orange trace: Debounced output signal with instantaneous response after clearing timeout
Red trace: Timeout signal

I've tried using the method "previousTime += minElapsed" which would have no time slippage, but there was no way I could get perfectly clean results ... looking at the signal, I thought this could work.

Not really important though, because I prefer resetting the interval after each interrupt which causes the overall elapsed time to expand. This means the interval will represent a stable period of time before the next valid interrupt is detected.

Here's the code:

int ledPin = 13;
int inputPin = 2;
byte inputState;

//----- pattern generator -----
unsigned long pattern;
unsigned long timeShiftPrev;
byte shiftCount = 0, signalSelect;

//----- ISR variables ---------
volatile unsigned long minElapsed = 100;
volatile unsigned long maxElapsed = 2500;
volatile unsigned long elapsedTime;
volatile unsigned long previousTime;
volatile byte cleanOutput;
volatile byte timeout;

void setup() {
  pinMode(ledPin, OUTPUT);
  Serial.begin(115200);
  attachInterrupt(0, onPulse, CHANGE);
}

void loop() {
  patternGenerator();
  inputState = digitalRead(inputPin);
}

void onPulse()
{
  elapsedTime = millis() - previousTime;
  if (elapsedTime < minElapsed )  //false interrupt
  {
    return;
  }
  if (elapsedTime >= minElapsed && elapsedTime <= maxElapsed )  //in range
  {
    previousTime = millis();
    timeout = 0;
    if (inputState)               //falling
    {
      cleanOutput = 0;
    }
    else                          //rising
    {
      cleanOutput = 1;
    }
    return;
  }
  if (elapsedTime > maxElapsed )  //timeout
  {
    previousTime = millis();
    timeout = 1;
  }
}

void patternGenerator()
{
  const unsigned long normallyLowSignal = 0x000AF500;
  const unsigned long normallyHighSignal = 0xFFF50AFF;
  const unsigned long shiftInterval = 100;

  unsigned long timeShiftNow = millis();
  if (timeShiftNow - timeShiftPrev > shiftInterval)
  {
    timeShiftPrev += shiftInterval;
    pattern = pattern >> 1;
    shiftCount++;
    if (shiftCount >= 32)
    {
      shiftCount = 0;
      signalSelect++;
      (signalSelect & 2) ? pattern = normallyHighSignal : pattern = normallyLowSignal;
    }
    digitalWrite(ledPin, (pattern & 1));
    Serial.print((pattern & 1) + 1);
    Serial.print(" ");
    Serial.print(cleanOutput);
    Serial.print(" ");
    Serial.println(timeout - 1);
  }
}

I guess it's my inexperience with coding, but if anyone knows why "previousTime += minElapsed" wouldn't work or has any suggestions for improving the ISR ... actually all comments are welcome!

EDIT: Code updated (as per reply#1).

2

What is supposed to happen in the ISR when elapsedTime == minElapsed?

3

Heh, thanks for catching that.

4

My debouncer at github.

5

Why are you reading the pin state in loop?

6

Use if-else logic to slice away one condition after the other.

void onPulse()
{
  elapsedTime = millis() - previousTime;

  if (elapsedTime < minElapsed )  //   ALL too soon cases are handled here 
  {
    return;                //
  }
  else if (elapsedTime <= maxElapsed )  //   ALL cases left within max time are  handled here 
  {
    previousTime = millis();
    timeout = 0;
    if (inputState)               //falling
    {
      cleanOutput = 0;
    }
    else                          //rising
    {
      cleanOutput = 1;
    }
    return;
  }
  else                                          // ALL cases left (timeout) are handled here
  {
    previousTime = millis();
    timeout = 1;
  }
}

BTW, the ISR should only save millis (if it has to) and set a flag. The rest should inside of loop().

void onPulse() // the IRQ
{
  interruptMillis = millis();  // interruptMillis must be a volatile variable
  interruptFlag = 1; // the flag should be a byte -- bytes run faster on 8-bit machines
}

............. snipped code

void loop()
{
............. snipped code

  if ( interruptFlag ) // this is a task inside of loop(), it always runs at least this check
  {
    interruptFlag = 0;    
    elapsedTime = interruptMillis - previousTime;

    if (elapsedTime > maxElapsed )  //   ALL cases over max time
    {
      previousTime = millis();
      timeout = 1;
    }
    else if (elapsedTime >= minElapsed )  //   ALL cases left >= min time
    {
      previousTime = millis();
      timeout = 0;
      if (inputState)               //falling
      {
        cleanOutput = 0;
      }
      else                          //rising
      {
        cleanOutput = 1;
      }
    }
    // no empty else is needed, this is ALL cases left, the < min time case
  }  // end of task

............. snipped code

}

I wouldn't save millis in the interrupt but then I wouldn't use an interrupt to time events on the millis scale.

7

PaulMurrayCbr:
My debouncer at github.

How would you implement your digital filter in an interrupt situation? Does it not depend on a constant sample rate? The interrupts are called randomly.

8

[quote author=Coding Badly date=1459921912 link=msg=2698072]
Why are you reading the pin state in loop?[/quote]
I did this to get the pin state just previous to the interrupt to determine rising or falling state change and to keep the ISR as quick as possible.

I suppose this is OK if the loop iteration speed is faster than the input rate. I was a hesitant to try checking the pin state inside the ISR as I was initially thinking that it could be during a noisy period and get an invalid reading.

Ah, OK ... when servicing an ISR, interrupts are disabled. I was initially not sure if registers are held constant (saved), but after re-reading Nick Gammons article, I see they are. (I have to stop thinking how register's work in FPGAs where everything is affected every clock cycle).

Thanks again ... I'll work on an updated version.

9

GoForSmoke:
Use if-else logic to slice away one condition after the other.

Yes, I do like the strategy and did try something similar (didn't have success ... my final "else" for timeout was prevented the "in range" condition from triggering), but I'll give it another go.

GoForSmoke:
I wouldn't save millis in the interrupt but then I wouldn't use an interrupt to time events on the millis scale.

Yes, micros() is definitely preferred ... just used millis while debugging the logic. I'll change this in my next update.

Thank you for your comments and strategy. I'll also be trying to get the output to respond on the same interrupt as when timeout is detected (on the graph, the output won't respond until the next interrupt ... the new conditional logic should resolve this).

10

Strategy, technique, whatever.

The output to graph takes far longer than you think. You do a lot of 32-bit work, AVR is 8-bit machine.

A little more technique for speed, time can be type byte, unsigned int or unsigned long. They all roll over, they all work as time between = now - start, one difference is how long an interval you can time and the other is how fast they run. The bytes run Much Faster.

byte is good to 255 units (millis or micros) but best to keep <= 200 or 250 and it is up to your code to not let it go farther. I use bytes with millis to debounce, usually 2 to 5 millis is enough.

unsigned int is good to 60000 or 65000, one full minute in millis.

unsigned long is good to 49.7-some days.

Morris Dovey left a time library for Arduino that uses unsigned long long good to billions of years.

byte quickMillis;
.....
quickMillis = ( millis() & 0xFF );

Fingers that learn code on computers often type 'int' when making a counting variable. Break the habit. When did you have an AVR pin number > 255? The byte runs quicker by at least 2x.

I would be surprised if all of that will have the interrupt and graph match but it's possible since both use serial print. At 115200 baud, serial chars send and arrive 1388 cycles apart, almost 87 micros. That is per character, your messages have a few.

Serial reporting can screw up fast timing, I haven't used the graph thing but I see no reason why it should be different. Strategy this time: make short runs that save data as you can and then output to serial.

Welcome to serial processing. We can make it look parallel but only down to a few micros without resorting to special near-machine-code-or-machine-code measures and then only with tight algorithms. That's a trade-off with these cheap yet versatile chips.

You are an exception to what I mostly see, people who code like the chip runs on a 1KHz clock.

Keep in mind that the function you call may run many cycles, the bit length of the variables you use may add many more (a 16-bit integer multiply takes more than a few) and floats are about 100x slower than longs.

If you keep the interrupt line normally HIGH and pull it LOW to signal, would that lessen the noise?

11

dlloyd:
Yes, I do like the strategy and did try something similar (didn't have success ... my final "else" for timeout was prevented the "in range" condition from triggering), but I'll give it another go.
Yes, micros() is definitely preferred ... just used millis while debugging the logic. I'll change this in my next update.

Look at the difference between the second example and the first. The order you have in the first runs marginally quicker in cases where minimum is not reached which may be desirable in tight code.

This is more like that without the returns.

if ( less than min ) // has no code, needs no {}, branches to the end
else if ( up to max )
{
// code
}
else
{
// code
}

How you order the conditional branches (aka if's) prioritizes execution in serial execution land.

12

Yeah, the plotter is a speed hog and so is generating the signal which is why I kept the frequency low for starts. So far so good with the plotter for crude troubleshooting ... it lacks a way to set the timebase though.

I learned recently about atomic readings ... I'll test using a byte counter for timekeeping once I get things cleaned up.

That's bothered me too about using 'int' for pin numbers. I've read somewhere that it ends up just using a byte when compiled. Wherever I look (even Arduino's reference) int seems to be the standard. I've never had use (yet) for using a signed integer for a counter.

If you keep the interrupt line normally HIGH and pull it LOW to signal, would that lessen the noise?

It seems to be more of a standard to do this. Even with I2C, SPI, RX/TX the lines default HIGH or require pullups.

In this case, I want all the noise I can get to see what can be achieved in software. Once I get things cleaned up, I'll test using a timer output (non-blocking) connected to an RC circuit driving a transistor and relay. The relay's contacts should produce some serious bounce ... I'll have it switch a 5V signal and connect it back to the interrupt. I have a new Saleae logic analyzer I'll try out and get some measurements and traces.

Here's the code cleaned up a bit (a little more to do yet):
Note: had to swap the rising and falling condition so as not to produce an inverted output. The plot now looks the same as posted earlier.

byte ledPin = 13;
byte inputPin = 2;
byte inputState;

//----- pattern generator -----
unsigned long pattern;
unsigned long timeShiftPrev;
byte shiftCount = 0, signalSelect;

const unsigned long minElapsed = 120000;
const unsigned long maxElapsed = 2500000;
unsigned long elapsedTime, previousTime;
byte cleanOutput, timeout;

//----- ISR variables ---------
volatile unsigned long isrTime;
volatile byte interruptFlag;

void setup() {
  pinMode(ledPin, OUTPUT);
  Serial.begin(115200);
  attachInterrupt(0, onPulse, CHANGE);
}

void loop() {
  patternGenerator();
  processSignal();
}

void onPulse()
{
  isrTime = micros();
  interruptFlag = 1;
}

void processSignal()
{
  if ( interruptFlag )
  {
    inputState = digitalRead(inputPin);
    interruptFlag = 0;
    noInterrupts ();
    unsigned long interruptTime = isrTime;
    interrupts ();
    elapsedTime = interruptTime - previousTime;

    if (elapsedTime > maxElapsed )        // ALL cases over max time
    {
      previousTime = interruptTime;
      timeout = 1;
    }
    else if (elapsedTime >= minElapsed )  // ALL cases left >= min time
    {
      previousTime = interruptTime;
      timeout = 0;
      if (inputState)                     // Rising
      {
        cleanOutput = 1;
      }
      else                                // Falling
      {
        cleanOutput = 0;
      }
    }
      // no empty else is needed, this is ALL cases left, the < min time case
  }   // end of task
}

void patternGenerator()
{
  const unsigned long normallyLowSignal = 0x000AF500;
  const unsigned long normallyHighSignal = 0xFFF50AFF;
  const unsigned long shiftInterval = 100000;

  unsigned long timeShiftNow = micros();
  if (timeShiftNow - timeShiftPrev > shiftInterval)
  {
    timeShiftPrev += shiftInterval;
    pattern = pattern >> 1;
    shiftCount++;
    if (shiftCount >= 32)
    {
      shiftCount = 0;
      signalSelect++;
      (signalSelect & 2) ? pattern = normallyHighSignal : pattern = normallyLowSignal;
    }
    digitalWrite(ledPin, (pattern & 1));
    Serial.print((pattern & 1) + 1);
    Serial.print(" ");
    Serial.print(cleanOutput);
    Serial.print(" ");
    Serial.println(timeout - 1);
  }
}
13

A HIGH wire has the pressure of voltage keeping it stable, yes?

You haven't tried using less than 32-bit time variables yet. Fear?
And still the pin variables are int.

One thing about global variables is that they only get allocated once.

Those local constants in the pattern generator, maybe the optimizer catches them otherwise they get initialized on the stack every time the function runs.

Also timeShiftNow is only used once after it gets set to micros(). Maybe the optimizer catches that otherwise you don't need the variable added to the stack, just use micros() in the time check if().

14

I guess I should clarify that I'd like to end up with an optimized, efficient and universal function that can clean up most any noisy signal. A function that works with ISR's and external interrupt inputs and keeps time measurements as accurate as possible.

I'd like it to work with touching a bare wire to ground (or vcc), relay contact and switch bounce up to 30ms, noisy ignition pulses, reed switches, interference over long cable runs, encoders, optic switching, electricity meter pulses, flow meters, rpm signals, pulse measurement (duty cycle and period), counting, frequency measurement.

I'll still have to deal with deglitching .. later I'll be attempting to clean up a single glitch occurring during a high and low stable period.

Goal: Frequency range of the function 0.01Hz - 100kHz (fully debounced and deglitched). The user would configure the function to suit the specifications of device connected.

You haven't tried using less than 32-bit time variables yet. Fear?

At this point ... yes. I'm starting with slower signals. An 8-bit timer will overflow about 117 times during 30ms of invalid interrupts (bounce). But I'll give it a go when I get further along.

And still the pin variables are int.

Done.

A HIGH wire has the pressure of voltage keeping it stable, yes?

Yes.

The pattern generator is just temporary, I'll be using it along with a timer output and relay driver on a separate Arduino sending pulses to pin2 on the UNO. This will avoid any synchronization with the UNO's clock and allow it to run full out.

15

I have read this Thread looking for inspiration.

I am trying to make a small (physically) rev counter (i.e. count the revolutions) for a small 130 size DC motor. In the space available the only solution I have thought of is to use a UV led and a photo transistor organized so that a small piece of rotating metal (white one side, black the other) reflects the light onto the sensor.

Initial tests suggest that it is bouncing and returning too many counts. I think the max RPM will be 12000 or about 200 per sec but I was getting counts near 1000.

...R

16

dlloyd:
Here's the code cleaned up a bit (a little more to do yet):

You have to put the digitalRead inside the interrupt service routine. Separating the interrupt handling from the digitalRead creates a race condition that will leave you pulling your hair out. (Technically, the race condition is always present. Moving the digitalRead to the top of the interrupt service routine dramatically reduces the chances of trouble.)

void processSignal()
{
  if ( interruptFlag )
  {
    inputState = digitalRead(inputPin);
    interruptFlag = 0;

I commend you for trying to handle the interrupt the "right way". However, in this situation, doing that opens up a second race condition. (There is actually a third but the third one is trivial to fix.) I strongly suggest, for this application, your processSignal code belongs in the interrupt service routine as you had it originally.

17

GoForSmoke:
One thing about global variables is that they only get allocated once.
Those local constants in the pattern generator, maybe the optimizer catches them otherwise they get initialized on the stack every time the function runs.

Didn't know that ... thank you.

GoForSmoke:
Also timeShiftNow is only used once after it gets set to micros(). Maybe the optimizer catches that otherwise you don't need the variable added to the stack, just use micros() in the time check if().

Done ... thanks again.

I seriously appreciate this (I've already lost enough hair). Later, I may end up doing a direct register read as a speedup measure.

Done. I've found a workaround to set the plot's timebase. Now we can see the transitions more clearly. The first set of traces were with the signal processing in the loop, the second set is with the processing in the ISR. There is a difference.

I'll be adding a "glitch" signal circled in red ... this one is going to be difficult to eliminate without affecting timing, but I have some ideas (my next challenge).

Some notes on the Serial Plotter: It seems to work quite well for me at this stage. I'm able to stretch the timebase by x2 or higher by repeating the print action.

Latest code:

byte ledPin = 13;
byte inputPin = 2;
byte inputState;

//-------pattern generator---------------
unsigned long pattern;
unsigned long timeShiftPrev;
byte shiftCount, signalSelect;

//-------ISR constants and variables-----
const unsigned long minMicros = 120000;
const unsigned long maxMicros = 2500000;
volatile unsigned long nowMicros, prevMicros, elapsedMicros;
volatile byte cleanOutput, timeout;

void setup() {
  pinMode(ledPin, OUTPUT);
  Serial.begin(115200);
  attachInterrupt(0, onPulse, CHANGE);
}

void loop() {
  patternGenerator(100000, 4);  // shiftInterval (µs), plotTimebase (x4)
}

void onPulse()
{
  nowMicros = micros();
  inputState = digitalRead(inputPin);
  elapsedMicros = nowMicros - prevMicros;
  if (elapsedMicros >= minMicros)  // ALL cases >= min time
  {
    prevMicros = nowMicros;
    (elapsedMicros >= maxMicros) ? timeout = 1 : timeout = 0;  // timeout flag
    if (inputState)      // Rising
    {
      cleanOutput = 1;
    }
    else                 // Falling
    {
      cleanOutput = 0;
    }
  }
}

void patternGenerator(unsigned long shiftInterval, byte plotTimebase)
{
  if (micros() - timeShiftPrev > shiftInterval)
  {
    timeShiftPrev += shiftInterval;
    pattern = pattern >> 1;
    shiftCount++;
    if (shiftCount >= 32)
    {
      shiftCount = 0;
      signalSelect++;
      //                             Normally High Signal   Normally Low Signal
      // (signalSelect & 2) ? pattern = 0xFBF50AFF : pattern = 0x040AF500; // bounce with glitch
      (signalSelect & 2) ? pattern = 0xFFF50AFF : pattern = 0x000AF501; // bounce
    }
    digitalWrite(ledPin, (pattern & 1));

    for (int i = 0; i < plotTimebase; i++) {
      Serial.print((pattern & 1) + 1);
      Serial.print(" ");
      Serial.print(cleanOutput);
      Serial.print(" ");
      Serial.println(timeout - 1);
    }
  }
}

Results (debouncing completed):

18

Yes, high speed pulse debouncing is a completely different animal than low speed switch debouncing. The premise, in it's simplest form, is that the bounce impulses are higher in frequency than the signal. I think it is hard to refute this. So the idea of low pass filtering and applying a threshold makes good sense.

However, in order to properly implement the low pass function, the digital filter must be sampling at precise, periodic intervals. This seems incompatible with the mechanism of a pin state interrupt. Furthermore, it will be difficult to run the filter at extremely high frequencies without incurring significant demand on the processor.

There isn't much doubt that it can be made to work well. A good question is, what is a useful upper limit on the signal frequency?

A corresponding question would be, can a simple hardware RC outperform it by a large margin?

19

aarg:
There isn't much doubt that it can be made to work well. A good question is, what is a useful upper limit on the signal frequency?

Yes, good question. Well, I'm hoping for at least 10kHz for a noisy signal, but there'll be a point where jumping into the ISR with needless false interrupts will consume too many cycles. I'm trying just the timing approach for now ... at least its very fast with the least demand on the processor.

20

@Robin2

I think something like this could work for your sensor because (at this stage) it should debounce both edges of the pulses, so it won't matter which side of the pulse the false transitions occur. (I assume you'll be using an interupt for this). Just use CHANGE mode for your interupt. Then, for example, you could use minMicros = 4000 (4ms, 250Hz) and maxMicros = 4000000 (4sec, 0.25Hz) to set the input timing range.