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– st20002016年06月30日 04:21:21 +00:00Commented Jun 30, 2016 at 4:21
2 Answers 2
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.
- Put the empty constructor in the .h file, this avoids it being compiled into a real empty function.
- Qualify
frequency
,clock_pulse
andopening_time
asconst
, and makeperiod_ms
local toTimer1::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. - Your
CRITICAL_SECTION_*
macros are redundant withATOMIC_BLOCK(ATOMIC_RESTORESTATE)
from avr-libc. But then, do you really need to block the interrupts the whole time? - I don't find the other macros in Timer1.h really useful either: just put comments next to each access to a hardware register.
- I would set
TCCR1B
last: the timer starts counting the moment you set this register. - You should subtract 1 from the values you set in
OCR1A
andOCR1B
: the period and HIGH time areOCR1A+1
andOCR1B+1
respectively. - 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
andopening_time
. - Your variables
ocr1a
andocr1b
serve no useful purpose, they just needlessly eat RAM. - You casting
opening_time
tofloat
, then back tounsigned int
, serves no useful purpose either. - 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;
}
-
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– Sebastian Freeman2016年07月01日 14:17:57 +00:00Commented Jul 1, 2016 at 14:17
-
I mean to say that the user will change the values while the Arduino is running via Serial.Sebastian Freeman– Sebastian Freeman2016年07月01日 14:44:22 +00:00Commented Jul 1, 2016 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– Sebastian Freeman2016年07月02日 02:59:49 +00:00Commented Jul 2, 2016 at 2:59
-
@SebastianFreeman: It's the prescaler. When the three
CS1x
bits are set to zero (which happens when you clearTCCR1B
), the timer stops counting. It starts again when you change any of these three bits.Edgar Bonet– Edgar Bonet2016年07月02日 07:13:20 +00:00Commented Jul 2, 2016 at 7:13
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.