1

Good evening everyone,

I have bene playing around with making my own library, specifically for the controlling Timer1 on the Arduino Mega 2560. I'm sort of new to OOP but I am trying my hand at it here.

I wrote some code to generate a PWM wave with a 100 ms period and a 25% duty cycle, and I have Siglent SDS 1052DL, 50MHz oscilloscope that I'm using that verifies the code is successful. I am using clear-timer-on-compare-match mode and timer interrupts to generate the PWM wave. My code is below:

#include "Timer1.h"

Timer1 timer;

void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);
  pinMode(13, OUTPUT);
  timer.begin();
}

void loop(){
  // put your main code here, to run repeatedly:
}

I made a Timer1.cpp file and a Timer1.h to go along with this file. They are below.

Timer1.cpp

#include "Arduino.h"
#include "Timer1.h"

float frequency = 10.0;
float period_ms;
float clock_pulse = 16.0;

unsigned long opening_time = 25000;

unsigned int ocr1a;
unsigned int ocr1b;

Timer1::Timer1() { // Object used to control 16-bit timer1

}

void Timer1::begin() {
  CRITICAL_SECTION_START; // Clear I bit
  CLEAR_TCCR1A; // Reset TCCR1A
  CLEAR_TCCR1B; // Reset TCCR1B
  SET_CTC_MODE; // Set to CTC mode
  DISABLE_TIMER1_INTERRUPTS; // Disable OCIE1A/B
  TCCR1B |= (1 << CS12); // 256
  period_ms = (1000.0f / frequency);
  ocr1a = 1000UL * (period_ms / clock_pulse);
  OCR1A = ocr1a;
  TIMSK1 |= (1 << OCIE1A);

  ocr1b = (unsigned int)float(opening_time) / clock_pulse;
  OCR1B = ocr1b;
  TIMSK1 |= (1 << OCIE1B);
  TCNT1 = 0; // Reset counter

  CRITICAL_SECTION_END; // Set I bit
}

ISR(TIMER1_COMPA_vect) {
  digitalWrite(13, HIGH);
}

ISR(TIMER1_COMPB_vect) {
  digitalWrite(13, LOW);
}

Timer1.h

#ifndef TIMER1_H
#define TIMER1_H

// ========== HANDY MACROS ========== //
#define CLEAR_TCCR1A                  TCCR1A = 0
#define CLEAR_TCCR1B                  TCCR1B = 0
#define SET_CTC_MODE                  TCCR1B |= (1 << WGM12)
#define DISABLE_TIMER1_INTERRUPTS     TIMSK1 &= ~((1 << OCIE1B) | (1 << OCIE1A))
#define CLEAR_CLOCK_SELECT_BITS       TCCR1B &= ~((1 << CS12) | (1 << CS11) | (1 << CS10))

#define CRITICAL_SECTION_START unsigned char _sreg = SREG; cli()
#define CRITICAL_SECTION_END   SREG = _sreg

class Timer1{
  public:
  // Constructor:
  Timer1();
  // Begin:
  void begin();

};

#endif // TIMER1_H

This code works exactly as I would expect. I originally had placed the whole block of code found in the Timer1::begin() method in the Timer1::Timer1() constructor method. When I did this, I was getting a completely different signal (2.04 ms period with about an 80% duty cycle). Is there a simple explanation for what is occurring?

  • You are coding in C++ but have variables global to the file Timer1.cpp. It would be better if you defined these variables with scope of the class Timer1. Then they should be defined for member functions which should include the constructor. What I'm wondering ... how did the code in the constructor compile in the Timer1.h file when the defined variables only had scope over the Timer1.cpp file? – st2000 Jun 30 '16 at 4:21
4

In your test code, you declare Timer1 in global scope. This means its constructor will be called right when the program starts, even before main() is called. The first thing the Arduino main() does is to call init(), which configures all the timers for PWM operation, as expected by analogWrite(). Then, any initialization of Timer1 done by the constructor will be undone by init(). That's why you want to defer that initialization until setup().

Edit: I took the liberty to do a code review on your post. Below are a few suggestions. I hope some of them might be useful.

  1. Put the empty constructor in the .h file, this avoids it being compiled into a real empty function.
  2. Qualify frequency, clock_pulse and opening_time as const, and make period_ms local to Timer1::begin(). This way all the floating point operations are performed at compile time and you don't get all the floating point support compiled into your program.
  3. Your CRITICAL_SECTION_* macros are redundant with ATOMIC_BLOCK(ATOMIC_RESTORESTATE) from avr-libc. But then, do you really need to block the interrupts the whole time?
  4. I don't find the other macros in Timer1.h really useful either: just put comments next to each access to a hardware register.
  5. I would set TCCR1B last: the timer starts counting the moment you set this register.
  6. You should subtract 1 from the values you set in OCR1A and OCR1B: the period and HIGH time are OCR1A+1 and OCR1B+1 respectively.
  7. You may want to round the period and HIGH time to the nearest integer. It may make no difference here but you users may want to change frequency and opening_time.
  8. Your variables ocr1a and ocr1b serve no useful purpose, they just needlessly eat RAM.
  9. You casting opening_time to float, then back to unsigned int, serves no useful purpose either.
  10. You may want to replace digitalWrite() with direct port access, as this function is really slow and you are calling it from an ISR.

Below is my throw at Timer1.cpp. Note that port and bit could be made private data of the class, and the pin number could be a parameter to the begin() method.

#include "Arduino.h"
#include "Timer1.h"

/* The three constants below can be tuned to taste:  */

const float frequency    = 10;  // hertz
const float opening_time = 25;  // milliseconds
const int   pin          = 13;

/* Nothing should need to change below this line. */

const uint16_t prescaler = 256;
const float f_timer = float(F_CPU) / prescaler;
static volatile uint8_t *port;
static uint8_t bit;

void Timer1::begin() {
    // Prepare the output.
    port = portOutputRegister(digitalPinToPort(pin));
    bit = digitalPinToBitMask(pin);
    pinMode(pin, OUTPUT);

    // Configure Timer 1.
    TCCR1A = 0;  // undo the configuration done by...
    TCCR1B = 0;  // ...the Arduino core library
    TCNT1  = 0;  // reset counter
    OCR1A  = round(f_timer / frequency) - 1;
    OCR1B  = round(f_timer * opening_time * 1e-3) - 1;
    TIMSK1 = _BV(OCIE1A)   // enable TIMER1_COMPA interrupt
           | _BV(OCIE1B);  // enable TIMER1_COMPB interrupt
    TCCR1B = _BV(WGM12)    // CTC mode
           | _BV(CS12);    // prescaler = 256
}

ISR(TIMER1_COMPA_vect) {
    *port |= bit;
}

ISR(TIMER1_COMPB_vect) {
    *port &= ~bit;
}
| improve this answer | |
  • Thanks for all the helpful comments and suggestions! I will definitely use some of your tips. Some of the other things you pointed out, were as a result of trying to reduce a code down enough to show the problem and exclude the portions that I felt didn't relate to the question. In the end, I want the user to be able to change the frequency/period, the reason for which the Arduino will need to calculate the values. – Sebastian Freeman Jul 1 '16 at 14:17
  • I mean to say that the user will change the values while the Arduino is running via Serial. – Sebastian Freeman Jul 1 '16 at 14:44
  • What about setting the TCCR1B starts the counting? Is it the enabling of the CTC mode, or the setting of the prescaler? – Sebastian Freeman Jul 2 '16 at 2:59
  • @SebastianFreeman: It's the prescaler. When the three CS1x bits are set to zero (which happens when you clear TCCR1B), the timer stops counting. It starts again when you change any of these three bits. – Edgar Bonet Jul 2 '16 at 7:13
1

I believe you should be able to put your code back into the constructor if you move all the variable you defined with file scope in Timer1.cpp:

float frequency = 10.0;
float period_ms;
float clock_pulse = 16.0;

unsigned long opening_time = 25000;

unsigned int ocr1a;
unsigned int ocr1b;

...to the Timer1 class definition (either in the public or private section). This way all the variables will have the scope of the class and the code should work the same regardless where in the class it is.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.