[SOLVED] Simple sketch with interrupts fails randomly - why?

Hi,
I was trying to sample a tachometer using interrupts but got strange results. Now I’ve stripped some code to get to the core but still can’t figure out what’s going on.
Consider the code below

const byte PIN_TACH          = 2;    // Tachometer pin
const unsigned long T_SAMPLE = 200;  // Sample period
volatile byte counter        = 0;    // Counter to be increased in interrupt

void setup() {
  pinMode(PIN_TACH, INPUT);
  attachInterrupt(0, tacometer0, CHANGE);
  Serial.begin(115200);
  Serial.println("Starting");
}

void loop() {
  byte _counter;
  unsigned long time;
  unsigned long now;

  counter = 0; 
  time = millis() + T_SAMPLE;
  while (1) {
    now = millis();
    if ( time>now ) continue;

    noInterrupts();        
    _counter = counter;  // Copy counter and
    counter  = 0;        // ...reset both as soon as possible.
    interrupts();

    time = now + T_SAMPLE;   // Set next sample to future.

    //if ( _counter>6 || _counter<5 ) { // Still glitches if uncommented but more seldom.
      Serial.print(now);
      Serial.print(',');
      Serial.println(_counter);
    //}
  }
}

// Interrupt function
void tacometer0() {
  counter++;
}

Sample period is 200 milliseconds. 15Hz input sine wave should give 6 interupts per period (200ms) because 15 * 0,2 * 2 = 6 when the interrupt triggers on changing input. I use a signal generator to output a 14.5Hz sin wave 2,5 +/- 2,5V on pin 2. With 14.5Hz I expect counter to be 6 or sometimes 5. However, every now and then it gets totally out of order. Some samples are e.g. 10 or 23 or so. See attachment.
I thought there was a conflict with the Serial but even though using the statement “if ( _counter>6 || _counter<5 ) {}” I still get some similar result, however much more seldom, maybe a few times per hour.

So help :fearful:! What could be wrong?

I’m also wondering if I really need to use noInterrupts() when reading the counter. I assume reading a byte is atomic so then it wouldn’t be necessary, right? (Anyhow, I tried with and without and couldn’t notice any difference.)

sine_wave.png

I notice that a higher value is for ??800 to ??000. Is that always at that point ?

You do this:

time = millis() + T_SAMPLE;

But you read millis() at that point, that could be at a different moment as the moment you make the counter zero. To get a consistance pace, this is better:

time += T_SAMPLE;

Reading a byte can't be split into two by an interrupt. So that is atomic. But disabling interrupts won't hurt, you might make it a 16-bit later.

It's the hardware that could be the problem. The best way is to filter the higher frequencies(noise), and use a transistor to get a nice 0..5V signal to the interrupt.

Hi thanks for your answer. The code time = millis() + T_SAMPLE; is outside the loop so shouldn’t really be relevant. However, you’re right that setting counter to zero isn’t happen at the exact same moment as updating time. Still, the deviation shouldn’t cause count to jump from expected 5 or 6 to e.g. 45. Also, since I’m (re)setting time before resetting counter I would expect a lower count rather than a higher.

Not sure about the filter either. I will try. However if that’s the problem, it doesn’t explain why I get those odd values much more often when doing Serial.print in each cycle compared to making Serial.print only when value are out of order using the satement

if ( _counter>6 || _counter<5 ){...}

I assumed Serial.print wasn’t interfering with interrupts or millis(), but perhaps that is not so? Do you know?

You properly protect count in one place but not in the other. Why?

I have been playing with the FreqCount and FreqCounter libraries with some success. I have also seen similar issues to what you are talking about. The FreqCount and FreqCounter libraries use hardware interrupts as well, but my frequency is much higher than yours. I think you may want to look at the http://www.pjrc.com/teensy/td_libs_FreqCount.html#compare library which seems like it would work for your tach project. I had to add a delay(20) after my last Serial.print(). It seems to be necessary to allow everything to finish before beginning a new frequency count.

I am chasing a similar glitch, so if I solve anything, I'll be sure to share it.

Right now, my problem is that sometimes it appends the previous value to the current value. So, in your case it would be 55 or 56 or 65 instead of 5 or 6. No rhyme or reason to it.

Good luck.

@CodingBadly - Not sure what you mean. Where is count not protected? [quote author=Coding Badly link=topic=117408.msg883516#msg883516 date=1344219915] You properly protect count in one place but not in the other. Why? [/quote]

@81Pantah - Thanks I'll check it out! About values, in my case the count seem to be more or less random but never less than expected.

I will have to do some more test to figure out I guess. Thought I should test interrupt1 also. Lets see...

snoozerman: @CodingBadly - Not sure what you mean. Where is count not protected?

Where you set it zero at the top of loop.

Using a Serial.print() takes time, and the data is send with an interrupt while the rest of the code is running. But with such low frequencies I can't imagine that it would influence your wave count. Perhaps Serial.print() causes some electric noise which is added to your signal. A small 1nF capacitor at the input should fix that.

Do you use an Arduino board, or a (badly decoupled) own circuit ? Is your +5V actually +5.0V or do you use USB power ? If so, please use an adapter. Perhaps you could try "Serial.begin(9600);", does that change anything ?

With such weird unpredictable results, I still think that it is hardware or a noisy input signal.

I think it's unwise to design your loop() to run forever. I don't think the hardware Serial needs to be polled from main() any more, but I don't think it's wise to make assumptions about that. Better to have loop() be called repeatedly in the conventional way.

The timing code isn't quite right. You need to use subtraction rather than addition, in order for timer overflow to work properly. I don't think timer overflow is causing the problems you're describing, but you should still handle that correctly.

now = millis();
if (lastTime - now > T_SAMPLE)
{
    ... take sample
    lastTime += T_SAMPLE;
}

Have you confirmed that the interrupt is being triggered reliably at the frequency you expect? If not, you may need to test that explicitly, for example by using a bigger counter (unsigned long should cover every eventuality) and letting it run for a longer interval, then stopping the counter and reporting the interval and the count. (You can use a simple global flag to stop the interrupt doing anything, so you can start/stop the counting within loop() without any complex interaction/synchronisation with the interrupt handler.)

Wouldn't this be more elegant?

now = millis();
if (now - lastTime > T_SAMPLE)
{
    lastTime = now;

    ... take sample

}

New results! :D @Krodal - I think you were completely right about the hardware!! And yes, my setup is... hmm... artistic ?! :roll_eyes: That's just a web of wires on my desktop. I tried some different capacitors and ended up with a ~0,7 nF. Not a single miscount! Also noticed I could use Serial.Print() to print every sample! So I guess you were right about Serial causing noise on the interrupt input also. Below is my end result I think. I rewrote the loop slightly trying to 1) Minimize time between reading micros() (or millis()) and global counter. 2) Minimizing the likelyhood of incrementing counter after millis() (or micros()) is read but before counter is. 2) Eleminating the likelyhood of incrementing counter after it is read but before it's reset to zero. 3) For the sake of a few bytes shorter code; removed noInterrupts() since I will for sure only use byte type for counter and operation on a byte is atomic anyhow.

With this done I did some tests - all successful this far. If I used micros() instead of millis() and set sample period to 2000 microseconds I managed to sample 51,25 kHz sine (205 counts per loop) when using Serial.print() - without a any a single weird sample! Test duration reported by the Arduino was 19min 57,8 seconds as total time while timestamps from the PC reported 19min 58seconds (1 second resolution). Not too bad :) And more than enough for me!

Only post the loop() here - the rest is the same as before.

void loop() {
  byte _counter;           // Copy global (interrupt)counter value into it.
  byte deltaCount;         // Difference between two successive counter samples.
  unsigned long nextTime;  // Time when to trigger next sample.
  unsigned long now;       // Time for current loop.

  _counter = counter;     
  nextTime = micros() + T_SAMPLE;
  while (1) {
    now = micros();                  // Only one call to millis()
    deltaCount = counter - _counter; // Only one read of counter
    if ( nextTime>now ) continue;
    
    // do stuff... 

    // ...e.g. use Serial.print() to test and verify 
    Serial.print(now);
    Serial.print(',');
    Serial.println(deltaCount);

    // Last but not least, update _counter and nextTime 
    _counter += deltaCount;
    nextTime  = now + T_SAMPLE;
  }

}

Thank you all! Really great job!

Thank you everyone!

Ps. The reason for the loop in the loop; while (1) {...} is that is more relevant for the complete part of my project and this is just a snippet.

...and using Arduino Duemilanove on usb power Ds.