Interrupts and timers

Hi Guys,

I am trying to make myself familiar with timers and interrupts. For this purpose I created this piece of code which doesn`t behave as expected sometimes. The ISR function blink() turns the timer on and off. The ISR of the timer callback() turns a LED ON and OFF. So in the end I can control if the LED blinks by a push button. In most of the cases this works fine. But sometimes the LED stays solid ON without blinking, which is as far as I understand this code not possible. Maybe some of you guys can explain this strange behaviour to me?

Greetings from Austria
leo

#include "TimerOne.h"

volatile bool isBlinking = false;

void setup()
{
  //***LED***
  pinMode(13, OUTPUT);

  //***Timer***
  Timer1.initialize(100000);
  Timer1.stop();
  Timer1.attachInterrupt(callback);

  //***Interrupt***
  pinMode(2, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(2), blink, FALLING);
}

void callback()
{
  digitalWrite(13, !digitalRead(13));
}

void blink()
{
  cli();
  if (isBlinking == false) {
    Timer1.start();
    isBlinking = true;
  }
  else { //true
    isBlinking = false;
    Timer1.stop();
    digitalWrite(13, LOW);
  }
  sei();
}

void loop()
{
}

I'm not sure (you have to dive into TimerOne library) but I'm getting the feeling .stop() might not remove the timer interrupt flag. So if the interrupt fires before interrupts are disabled it is queued and executed after interrupts resume. Or even (but not 100% sure), if a specific interrupt is off but global interrupts on, it only means the ISR isn't called, not that the flag isn't set. Aka, the the flag is still set when an interrupt fires only no code is run. But once you turn on the interrupt and the flag is still set it will run the code.

But there are more then one library called TimerOne so which do you use?

Also, quick note, when entering a ISR, interrupts are disabled by default. No need to do it yourself :wink:

To see if it fixes it, change to

Timer1.start();
TIFR1 = 0; //clear all Timer 1 Interrupt Flags

Thanks for the fast answer. Your explanation sounds understandable to me. The callback() function fires one more time and turns on the LED forever.

I tried your fix with the flag register but it didn`t change the situation at all. I also included your statement after Timer.stop() but nothing changed.

#include "TimerOne.h"

volatile bool isBlinking = false;

void setup()
{
  //***LED***
  pinMode(13, OUTPUT);

  //***Timer***
  Timer1.initialize(100000);
  Timer1.stop();
  Timer1.attachInterrupt(callback);

  //***Interrupt***
  pinMode(2, INPUT_PULLUP);
  attachInterrupt(digitalPinToInterrupt(2), blink, FALLING);
}

void callback()
{
  digitalWrite(13, !digitalRead(13));
}

void blink()
{
  cli();
  if (isBlinking == false) {
    Timer1.start();
    TIFR1 = 0; //clear all Timer 1 Interrupt Flags
    isBlinking = true;
  }
  else { //true
    isBlinking = false;
    Timer1.stop();
    TIFR1 = 0; //clear all Timer 1 Interrupt Flags
    digitalWrite(13, LOW);
  }
  sei();
}

void loop()
{
}

To answer your first question: I use this lib

Just changed the code as below:

Timer1.stop();
TIFR1 |= (1 << TOV1); //clear all Timer 1 Interrupt Flags

Got this from the data sheet:

• Bit 0 – TOV1: Timer/Counter1, Overflow Flag
The setting of this flag is dependent of the WGM13:0 bits setting. In Normal and CTC modes, the TOV1 Flag is
set when the timer overflows. Refer to Table 16-4 on page 132 for the TOV1 Flag behavior when using another
WGM13:0 bit setting.
TOV1 is automatically cleared when the Timer/Counter1 Overflow Interrupt Vector is executed. Alternatively,
TOV1 can be cleared by writing a logic one to its bit location.

It seems to me this solved the problem.

Ahh, yeah. Missed the "write 1 for clear" part :smiley: Not messing with it daily.

No problem, thanks for putting me into the right direction anyway.

As far as I am reading many people suggest to use a timer an poll the button state within its ISR.
Unfortunately I can`t find some arduino related examples for this method. Maybe someone who already used this method can post a code snipet of it here please?

A IRS for a button usually is overkill. Unless your program is very busy or time critical, polling is much easier.

Same actually for simply blinking a led, timer is a bi overkill. Using millis is much simpler.

Okay, I got it but if my loop lasts for a very long time there is no alternative to external interrupts, or polling inside the ISR of a timer?

but if my loop lasts for a very long time

There is no reason, generally, that that should be the case.

If you're loop is slow you probably have a bigger problem. :smiley: Aka, don't make the loop (to) slow.

Finally found a solution that polls in the ISR of Timer2, wchich fires in a ~20ms rate. Seems to work fine and the loop stays free for other stuff.

volatile int pin_LED = 13;
volatile int pin_button = 2;

volatile boolean oldSwitchState = HIGH;
volatile boolean newSwitchState1 = HIGH;
volatile boolean newSwitchState2 = HIGH;
volatile boolean newSwitchState3 = HIGH;

volatile boolean LEDstatus = LOW;

void setup()
{
  //***Timer2***
  pinMode(13, OUTPUT);
  TCCR2A = 0;
  TCCR2B = 0;
  TCCR2A |= (1<<WGM21);                         //CTC Mode                                          (Timer zählt bis auf OCR2A und wird dann automatisch 0 gesetzt)
  TIMSK2 |= (1<<OCIE2A);                        //enable OCR1A Interrupt
  TCCR2B |= (1<<CS22) | (1<<CS21) | (1<<CS20);  //prescaler 1024                                    (Alle 1024 CPU Takte zählt der Timer einen Tick weiter)
  OCR2A = 200;                                  // OCR1A = f_clock / (prescaler * f_target) - 1     (Wert bis zu dem gezählt wird)
  
  //***LED***
  pinMode(pin_LED, OUTPUT);
  digitalWrite(pin_LED, LOW);

  //***Button***
  pinMode(pin_button, INPUT_PULLUP);
}

ISR(TIMER2_COMPA_vect)
{
  newSwitchState1 = digitalRead(pin_button);
  delay(1);
  newSwitchState2 = digitalRead(pin_button);
  delay(1);
  newSwitchState3 = digitalRead(pin_button);

  if (newSwitchState1 == newSwitchState2 && newSwitchState1 == newSwitchState3) {
    if ( newSwitchState1 != oldSwitchState ) { //hatt sich der zustand des schalter geändert?
      if ( newSwitchState1 == LOW ) {          //ist der schalter gedrückt (soll nur beim drücken nicht beim auslassen reagieren
        if ( LEDstatus == LOW ) {
          digitalWrite(pin_LED, HIGH);
          LEDstatus = HIGH;
        }
        else                    {
          digitalWrite(pin_LED, LOW);
          LEDstatus = LOW;
        }
      }
      oldSwitchState = newSwitchState1;
    }
  }
}

void loop()
{
}
TIFR1 |= (1 << TOV1); //clear all Timer 1 Interrupt Flags

does not write a 1 to the TOV1 only, it will clear any other set flags in that register that happen to be set, I would use:

TIFR1 = (1 << TOV1);

Are you sure? With my method I write 1 to the TOV1 Bit and leave the others untouched. With yours I overwrite all other Bits with zeros.

Or am I missing something?

zopffa:
Or am I missing something?

Yes, the properties of a "write one to clear" register.

Do you expect the other bits in the register behave differently than the one you are writing?

By getting the current content of the flags register and writing it back you will clear all those bits
that happen to be set.

Don't you think that you should only reset the bit you use?

Now I get it!

When I use

TIFR1 |= (1 << TOV1)

and let’s say another Bit in TIFR1 is 1, so for example TIFR1 is 0b00000011 (TOV1 is Bit0). Then I create (1 << TOV1) which is the same as 0b00000001.

Now I use the or operator on both Bytes:

0b00000011 TIFR1
|0b00000001 (1 << TOV1)
=0b00000011

so in fact I write Bit0 and Bit1 value = 1 to the register.

Is that what you mean?

zopffa:
so in fact I write Bit0 and Bit1 value = 1 to the register.

Is that what you mean?

Yes.

It may not hurt in this case, because all the bits belong to the same timer,
but it is inefficient (one read and the or are superfluous) and incorrect.

There are some other similar behaving bits, for example the PINx registers,
where writing a 1 to a bit toggles the output state, where it is more critical to get it right.

Using a timer for 20ms timing is borderline. I really hope your loop is faster than 20ms :wink: