Timer2 does not work as it should

Question

I am currently playing with Arduino timers (on UNO currently) and I am building a library that has a function that must be called every millisecond.

There is plenty of code samples on Internet so I went with something that seemed perfect for my needs. I have then updated it to make it a reusable C++ class.

Here is my code (simplified to just isolate the issue):

#define BIT(x) (0x01 << (x))
static volatile uint32_t milliseconds = 0;
class Timer
{
 public:
 Timer()
 {
 // Disable interrupts
 uint8_t savedStatus = SREG;
 cli();
 // Use Timer2 with 1 ms interrupts
 // OC2A & OC2B disconnected, mode 2 (CTC, Clear Timer on Compare match)
 TCCR2A = BIT(WGM21);
 // Don't force output compare (FOCA & FOCB), mode 2, Clock Select clk/128 (CS = 5)
 TCCR2B = BIT(CS22) | BIT(CS20);
 // Set timer counter compare match (when value reached, 1ms has elapsed)
 OCR2A = 125 - 1;
 // Reset Timer2 counter
 TCNT2 = 0;
 // Set Timer2 interrupt mode (Set interrupt on OCR2A compare match)
 TIMSK2 = BIT(OCIE2A);
 // Restore interrupt enabling state
 SREG = savedStatus;
 }
};
static void callEveryMillisecond(uint32_t ms)
{
 // Do something here
}
// Attach interrupt routine to the Timer Compare Interrupt
ISR(TIMER2_COMPA_vect)
{
 milliseconds++;
 callEveryMillisecond(milliseconds);
}

After checking ATmega328 datasheet (section 17) several times, I came to the conclusion that what I do in Timer class constructor above is correct, and for Arduino UNO, the values used must lead to one call of the ISR every ms:

• CPU frequency = 16MHz
• Prescaler = 128
• -> timer frequency = 16MHz/128 = 125KHz
• -> max counter value = 125

I decided to use the CTC (Clear Timer on Compare match) mode, which means that when the timer TCNT2 (which is incremented at 125KHz frequency), reaches OCR2A value, then it will trigger the ISR, and then will be reset on next tick; the additional tick to reset the counter explains why we use 125 - 1 for OCR2A and not just 125.

Now comes the weird stuff. In order to check that my code worked, I decided to blink the LED on pin 13 every 10 seconds, hence I have added the following code to my program:

static void callEveryMillisecond(uint32_t ms)
{
 if (ms % 10000 == 0)
 digitalWrite(13, digitalRead(13) ^ 1);
}
static Timer timer;
void setup()
{
 pinMode(13, OUTPUT);
}
void loop()
{
}

Then I measured the time during which pin 13 LED is lit on (or off, both measures are the same). Measures are not very accurate (I just used my wristwatch) but good enough for my check.

I was surprised to see the measured time to be about 20 seconds instead of 10s.

Initially I thought about a mismatch in CPU frequency, but after performing a lot of small changes, here is what I found which worked perfectly:

//static Timer timer;
void setup()
{
 static Timer timer;
 pinMode(13, OUTPUT);
}

Just moving the Timer instance into setup() did the trick: my measures rightly indicated 10 seconds between 2 states of the pin 13 LED.

Now my questions are:

1. Why the second code sample works perfectly, whereas the first does not?

2. Is there, in standard Arduino included code, some code that would overwrite some of the settings I perform for Timer2 in Timer::Timer()?

3. Is there a way to make the first code sample work? Note that I don't like the second sample because it does not seem natural coding, also, I will need access to Timer timer instance in several locations of my future program, hence it can't be possible if it is declared in the scope of setup().

• alexan_e: Is there, in standard Arduino included code, some code that would overwrite some of the settings I perform for Timer2 in Timer::Timer()? You can easily check it, read the timer registers in the start of setup and send them through the serial port in the console to see if they match the values they should have.
• jfpoilpret: Good idea! Initially I did not want to add Serial to the picture but that could help. However that will not tell what changed these register. I'll check that tonight and report back on the results.
• alexan_e: We'll take it step by step, lets make sure first that something does change and then we can search further what changed the timer values.

Answer 1

I have located the source of the problem.

In file wiring.c (arduino-1.5.6-r2\hardware\arduino\avr\cores\arduino) which gets called before setup(), there is the following part that tries to changes the timer 2 prescaller to 64 by setting the CS22 bit.

// set timer 2 prescale factor to 64
#if defined(TCCR2) && defined(CS22)
 sbi(TCCR2, CS22);
#elif defined(TCCR2B) && defined(CS22)
 sbi(TCCR2B, CS22);
#else
 #warning Timer 2 not finished (may not be present on this CPU)
#endif

You already set CS20 and CS22 so this doesn't affect you. What does affect you is the following part which sets bit WGM20

 // configure timer 2 for phase correct pwm (8-bit)
#if defined(TCCR2) && defined(WGM20)
 sbi(TCCR2, WGM20);
#elif defined(TCCR2A) && defined(WGM20)
 sbi(TCCR2A, WGM20);
#else
 #warning Timer 2 not finished (may not be present on this CPU)
#endif

and you have already set WGM21 so WGM21 and WGM20 set changes the timer mode to fast PWM , top=0xFF which gives almost double delay that the mode you were trying to use (CTC top=124).

How to workaround this issue?

Timer2 is used when its associated PWM pins are used with an analogWrite() call, so the Arduino setup code sets all prescalers to well-defined values just in case the timers might get used later.

I would not try to fight that code; instead, I would just make sure that my setup code runs later, e.g. by moving it to a separate setup() method in your Timer class and calling that from the global setup() routine.

• jfpoilpret: Great! Do you have an idea why wiring.h does that? I mean, normally Arduino default libraries don't use Timer2 on UNO, so this behavior is strange.
• jfpoilpret: Do you think there is a way to "disable" this code from wiring.h?
• microtherion: Timer2 is used when its associated PWM pins are used with an analogWrite, so the Arduino setup code sets all prescalers to well defined values in case the timers might get used later. I would not try to fight that code; instead, I would just make sure that my setup code runs later, e.g. by moving it to a separate setup() method in your timer class and calling that from the global setup() routine.
• alexan_e: @jfpoilpret The wire.c gets called from init() and if timer2 registers are defined it applies the values shown above, why , I have no idea (one of the reasons I code in native C using AVRstudio).
• jfpoilpret: I have just checked wiring.c content and I have to say the init() function content seems to exceed its responsibilities somehow. Obviously there's no usable #if in the function to bypass this code block. Hence I'll have to find another solution to solve my problem. However I consider you have fully answered my question and therefore accept your answer. Maybe I'll edit the content to mention the fact we cannot really bypass this code block.

Answer 2

As a follow-up up to @alexan_e answer, here is what I finally did to ensure that my Timer2 settings are not overwritten by the init() method in Arduino wiring.c.

#define BIT(x) (0x01 << (x))
static volatile uint32_t milliseconds = 0;
class Timer
{
 public:
 // Don't use the constructor anymore
 void init()
 {
 // Disable interrupts
 uint8_t savedStatus = SREG;
 cli();
 // Use Timer2 with 1 ms interrupts
 // OC2A & OC2B disconnected, mode 2 (CTC, Clear Timer on Compare match)
 TCCR2A = BIT(WGM21);
 // Don't force output compare (FOCA & FOCB), mode 2, Clock Select clk/128 (CS = 5)
 TCCR2B = BIT(CS22) | BIT(CS20);
 // Set timer counter compare match (when value reached, 1ms has elapsed)
 OCR2A = 125 - 1;
 // Reset Timer2 counter
 TCNT2 = 0;
 // Set Timer2 interrupt mode (Set interrupt on OCR2A compare match)
 TIMSK2 = BIT(OCIE2A);
 // Restore interrupt enabling state
 SREG = savedStatus;
 }
};
static void callEveryMillisecond(uint32_t ms)
{
 if (ms % 10000 == 0)
 digitalWrite(13, digitalRead(13) ^ 1);
}
// Attach interrupt routine to the Timer Compare Interrupt
ISR(TIMER2_COMPA_vect)
{
 milliseconds++;
 callEveryMillisecond(milliseconds);
}
static Timer timer;
void setup()
{
 // Explicit call to Timer initialization; this occurs AFTER 
 // Arduino wiring.c init() has been called
 timer.init();
 pinMode(13, OUTPUT);
}
void loop()
{
}

Answer 3

In your setup function, you need to instantiate your timer, like this:

static Timer timer;
void setup()
{
 timer = new Timer(); <<<--- here
 pinMode(13, OUTPUT);
}
void loop()
{
}

• jfpoilpret: There is no need for a new as timer is not a pointer but an instance of Timer: it is in the static data section, not the heap. Its constructor is called (otherwise my ISR would never get called)
• kuroi neko: That will bloat the code with the malloc library and consume a bit of the precious RAM for no good reason. The only thing needed here is to get the init code called after the static object constructors. You don't need another instance to do that, just move the object creation inside setup() or put the init code in a separate method that you will call inside setup().

Answer 4

The way most of the libraries do this is to put an instance of (in your case) Timer in the Timer.cpp file, and reference it with an extern definition when needed - typically in your main cpp file. Also you are calling digitalWrite() and digitalRead() during an ISR. This can be bad since those functions are slow, and may not be guaranteed to behave when interrupts are disabled (as they are during an ISR). A safer approach is for your ISR to set a flag in your Timer class that can be read by an Timer.isExpired() or similar method in your loop() code.

• jfpoilpret: Putting an Timer instance in Timer.cpp won't change the issue: the problem is the well-known C++ initialization order chaos.
• jfpoilpret: I don't see anything indigitalRead() or digitalWrite() that should prevent it from being called from an ISR.