Go Down

Topic: Any clever ideas for excluding aberrant values produced by an ISR? (Read 3034 times) previous topic - next topic

Robin2

I have a simple ISR to record the value of micros() everytime a QRE1113 reflective optical sensor detects the white blob on a disc on the shaft of a small motor.
Code: [Select]
void revDetectorISR() {
 isrMicros = micros();
 newIsrMicros = true;
}

The code in loop() figures out the time interval between pulses.

Most of the time this works fine but occasionally (perhaps 1 in 20) a very short interval is logged - usually less than half the correct interval. The correct intervals can range between about 100,000 µsecs and 4,000 µsecs depending on the motor speed.

I'm just wondering if anyone has a clever (i.e. simple and few CPU cycles) method to identify and ignore those incorrect intervals. A moving average would not be appropriate because that would give credence to the incorrect values.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

jremington

In loop(), are you reading or using the global variable isrMicros without turning off the interrupts? 

If so, it will occasionally be corrupted during that process by the ISR.
No PM's please.

Delta_G

If you know the expected value then it sounds like you just need an if statement to see if it is within some range.  If you currently have some running average going or something then you could just compare to the current average. 

Code: [Select]

if(abs(newValue - currentAverage) <= someThreshold){
  // record the value
}
else {
  // don't
}



In loop(), are you reading or using the global variable isrMicros without turning off the interrupts? 

If so, it will occasionally be corrupted during that process by the ISR.
Yes, @Robin should have posted all complete code.  Or at least a minimal example that compiles and exhibits the issue.  Of all people I would expect @Robin to know this. ;)
|| | ||| | || | ||  ~Woodstock

Please do not PM with technical questions or comments.  Keep Arduino stuff out on the boards where it belongs.

pylon

Calculating the time difference in the ISR should consume too much time, so it might be an option. Just ensure all variables used in the ISR and the loop() code are declared volatile.

Robin2

Yes, @Robin should have posted all complete code.  Or at least a minimal example that compiles and exhibits the issue.  Of all people I would expect @Robin to know this. ;)
I don't expect anyone to set up a similar experiment to see if they get the same problem. I believe I have stated the case with sufficient clarity. Maybe another way to present it is like this
Code: [Select]
8123
7990
7957
8011
3876
8001
7997


What is a clever way to detect and ignore the 3876?

Calculating the time difference in the ISR should consume too much time, so it might be an option. Just ensure all variables used in the ISR and the loop() code are declared volatile.
I have tried that. It does not make any difference. The problem seems to be that something spurious triggers the interrupt. And I have spent too much time trying to figure out the cause.

...R
Two or three hours spent thinking and reading documentation solves most programming problems.

jremington

Quote
I believe I have stated the case with sufficient clarity.
Nope.
Quote
And I have spent too much time trying to figure out the cause.
And so have we.
No PM's please.

Paul_KD7HB

Can you change to a different interrupt pin and see if the problem follows?

Paul

pylon

Quote
And I have spent too much time trying to figure out the cause.
I don't think so. It's definitely better to find that error than to simply throw away value you don't like.


If you want to go that way, how about throwing all values away that aren't inside a +/-20% range of the previous value? You have to decide if that fits your setup.

Delta_G

I don't expect anyone to set up a similar experiment to see if they get the same problem. I believe I have stated the case with sufficient clarity. Maybe another way to present it is like this

Come on man, you've answered enough questions to know how this goes.

So the question remains, are you reading the variables in a critical section?  If we can eliminate the bad readings, isn't that in many ways better than figuring out how to ignore them?

And the code in my reply still stands.  That's the only way I see you getting it done.  You have to compare the new reading to the old readings to see if it fits.
|| | ||| | || | ||  ~Woodstock

Please do not PM with technical questions or comments.  Keep Arduino stuff out on the boards where it belongs.

DKWatson

Similar to the manner in which PWM duty cycles are derived and assuming you have a spare timer, when servicing your ISR, you know roughly when the next interrupt will arrive, say X microseconds. You also have some idea as to the rate of change that may occur, call it dX. The next interrupt then, is unlikely to occur before X - dX.

After setting newIsrMicros, set the OCR value on the available timer to OCR + (X - dX) and then turn your ISR off. The only instruction in the timerX_COMP_vect will then be to turn your ISR back on.
Live as if you were to die tomorrow. Learn as if you were to live forever. - Mahatma Gandhi

Coding Badly

What is a clever way to detect and ignore the 3876?
The correct way is with statistics (calculate the likelihood the current value is a member of the set).

A simple way is a (small) window average ignoring the high and low values.


Are you averaging or are the individual values important?

Are those values "real"?  Or a simplified example for illustration purposes?  (This question is very important.  You may have an overrun problem.)

But, it would be comforting to see the rest of the code that touches those two variables.


Danois90

I'm wondering what the purpose of "newIsrMicros" is? If it is used to tell the "loop()" that there is new data to process, that data should be protected in the ISR by the same variable.

Code: [Select]
void loop()
{
  if (newIsrMicros)
  {

    //Do something with isrMicros

    newIsrMicros = false; //Done playing with isrMicros
  }
}

void revDetectorISR() {
 if (newIsrMicros) return; //loop() has not handled isrMicros yet
 isrMicros = micros();
 newIsrMicros = true;
}


But for this to work properly, you would probably need to add a counter which counts how many times "revDetectISR" has been invoked without updating "isrMicros" and this would make the ISR a bit more bulky.
Instead of mocking what's wrong, teach what's right! ;)
When you get help, remember to thank the helper and give some karma!
Please, do NOT send me any private messages!!

Coding Badly

If it is used to tell the "loop()" that there is new data to process, that data should be protected in the ISR by the same variable.
Both variables have to be protected by a critical section.  Please add that to your example.


Danois90

Both variables have to be protected by a critical section.  Please add that to your example.
What would be the purpose of that? As long as the same variable is not written to in two places "at once", there should be no problem.
Instead of mocking what's wrong, teach what's right! ;)
When you get help, remember to thank the helper and give some karma!
Please, do NOT send me any private messages!!

Coding Badly


Ugh.  I am just so tired of answering questions that are trivial to answer with Google.  @Danois90, do some research.  Nick Gammon's website is a great place to start.


Go Up