Go Down

Topic: [SOLVED] Dealing with a noisy interrupt signal (Read 9075 times) previous topic - next topic

dlloyd

Apr 06, 2016, 05:34 am Last Edit: Apr 20, 2016, 04:28 pm by dlloyd
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:

Code: [Select]
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).

Coding Badly


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


dlloyd


PaulMurrayCbr

http://paulmurraycbr.github.io/ArduinoTheOOWay.html

Coding Badly


Why are you reading the pin state in loop?


GoForSmoke

#5
Apr 06, 2016, 08:38 am Last Edit: Apr 06, 2016, 09:13 am by GoForSmoke Reason: fixing spacing, adding a }
Use if-else logic to slice away one condition after the other.


Code: [Select]

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().

Code: [Select]

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.
1) http://gammon.com.au/blink  <-- tasking Arduino 1-2-3
2) http://gammon.com.au/serial <-- techniques howto
3) http://gammon.com.au/interrupts
Your sketch can sense ongoing process events in time.
Your sketch can make events to control it over time.

aarg

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.
  ... with a transistor and a large sum of money to spend ...
Please don't PM me with technical questions. Post them in the forum.

dlloyd

Why are you reading the pin state in loop?
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.

dlloyd

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.

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).


GoForSmoke

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?
1) http://gammon.com.au/blink  <-- tasking Arduino 1-2-3
2) http://gammon.com.au/serial <-- techniques howto
3) http://gammon.com.au/interrupts
Your sketch can sense ongoing process events in time.
Your sketch can make events to control it over time.

GoForSmoke

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.
1) http://gammon.com.au/blink  <-- tasking Arduino 1-2-3
2) http://gammon.com.au/serial <-- techniques howto
3) http://gammon.com.au/interrupts
Your sketch can sense ongoing process events in time.
Your sketch can make events to control it over time.

dlloyd

#11
Apr 06, 2016, 07:06 pm Last Edit: Apr 06, 2016, 09:57 pm by dlloyd
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.

Quote
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.

Code: [Select]
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);
  }
}

GoForSmoke

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().
 
1) http://gammon.com.au/blink  <-- tasking Arduino 1-2-3
2) http://gammon.com.au/serial <-- techniques howto
3) http://gammon.com.au/interrupts
Your sketch can sense ongoing process events in time.
Your sketch can make events to control it over time.

dlloyd

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.

Quote
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.

Quote
And still the pin variables are int.
Done.

Quote
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.

Robin2

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
Two or three hours spent thinking and reading documentation solves most programming problems.

Go Up