Some quick background, I may be needing some basic PWM for a project where I would need several (8-12?) independently running PWM pins. In experimenting with some basic coding for a single LED, I've come up with an oddity that I just haven't been able to see the cause of, and am hoping someone here can recognize it. I'm genuinely hoping that the cause is something trivial and I'm overlooking it, but I am flummoxed by it none the less.
The routine is interrupt driven where each interrupt cycles the LED using the "current" duty cycle. The main routine does nothing more that alters the duty cycle of the pwm for an LED fade in or out and changes the fade direction when the extremes are reached . With the values in the test code, it's close to one second fade in followed by one second fade out. The base code works as intended. However, in the fade out section of SOME of the cycles there is a flicker. It's repetitive and predictable. After loading, reset, etc. in the 6th fade there is always one late in the fade and more pronounced. There is one as well in the 7th but earlier and less obvious. The eighth fade appears to be fine. I don't believe I've ever seen one a flicker in the ramp up, only the ramp DOWN direction. The flicker appears in the same cycles as far as I've been willing to watch and I think in the same point of the fade when it does. That's true early, I can't say for later on in the running.
This routine has been loaded onto an UNO R3 as well as a Boarduino with the same results.
Thanks for any insight, etc
Lyle
#include <TimerOne.h>
int led = 13;
void ramp_LED();
unsigned long current_time, cycle_start_time;
int duty_cycle = 0;
int pwm_cntr = 0;
int ramp = HIGH; //direction of fade, HIGH= up, LOW = down
void setup() {
Timer1.initialize(240); //set period for interrupts
Timer1.attachInterrupt(ramp_LED);
pinMode(led, OUTPUT);
digitalWrite(led, HIGH);
cycle_start_time = micros();
// Serial.begin(115200);
}
void loop()
{
current_time = millis();
if (current_time - cycle_start_time > 9) //interval for duty_cycle step change in mS
{
ramp == HIGH ? duty_cycle++:duty_cycle--; //change step in direction of fade change
if (duty_cycle > 99) //see if duty_cycle has reached maximum ON time
ramp = LOW; //reverse direction to DOWN
if (duty_cycle < 2) //see if duty_cycle has reached minimum ON time
ramp = HIGH; //reverse direction to UP
cycle_start_time = millis(); //record time of most recent step change
}
}
void ramp_LED()
{
pwm_cntr++;
if (pwm_cntr == duty_cycle) //test if cntr has reached turn_off threshold
{
digitalWrite(led, LOW);
}
if (pwm_cntr == 99) //test if cntr has reached full period & then reset
{
pwm_cntr = 0;
digitalWrite(led, HIGH);
}
}
I'm afraid that alteration didn't fix the problem. I first applied the volatile definition to PWM_CNTR only, as it's the only variable being altered by the ISR (and ONLY in the ISR). No change. Then just in case I tried the modification to DUTY_CYCLE which is being tested but not altered in the ISR. No change either. If this relates to the problem I can't see it yet. I may try an alternate approach which would be to call the ISR with the counter value rather than trying to declare it globally, but that seems like a bandage if it works.
I'll continue to dig, but I'll also be glad to have other sets of eyes on this. Thx
Lyle
Here's the code as I have it presently:
#include <TimerOne.h>
int led = 13;
unsigned long current_time, cycle_start_time;
volatile int duty_cycle = 0;
volatile int pwm_cntr = 0;
int ramp = HIGH; //direction of fade, HIGH= up, LOW = down
void ramp_LED();
void setup() {
Timer1.initialize(240); //set period for interrupts
Timer1.attachInterrupt(ramp_LED);
pinMode(led, OUTPUT);
digitalWrite(led, HIGH);
cycle_start_time = micros();
// Serial.begin(115200);
}
void loop()
{
current_time = millis();
if (current_time - cycle_start_time > 9) //interval for duty_cycle step change in mS
{
ramp == HIGH ? duty_cycle++:duty_cycle--; //change step in direction of fade change
if (duty_cycle > 99) //see if duty_cycle has reached maximum ON time
ramp = LOW; //reverse direction to DOWN
if (duty_cycle < 2) //see if duty_cycle has reached minimum ON time
ramp = HIGH; //reverse direction to UP
cycle_start_time = millis(); //record time of most recent step change
}
}
void ramp_LED()
{
pwm_cntr++;
if (pwm_cntr == duty_cycle) //test if cntr has reached turn_off threshold
{
digitalWrite(led, LOW);
}
if (pwm_cntr == 99) //test if cntr has reached full period & then reset
{
pwm_cntr = 0;
digitalWrite(led, HIGH);
}
}
DOH!! I fixed that, however it alters but does not fix the flicker. What's frustrating to me is that this should be a simple, straightforward routine. Which in my mind means the fix should be equally straightforward. I will continue to dig.
Lyle
#include <TimerOne.h>
int led = 13;
unsigned long current_time, cycle_start_time;
volatile int duty_cycle = 0;
volatile int pwm_cntr = 0;
int ramp = HIGH; //direction of fade, HIGH= up, LOW = down
void ramp_LED();
void setup() {
Timer1.initialize(240); //set period for interrupts
Timer1.attachInterrupt(ramp_LED);
pinMode(led, OUTPUT);
digitalWrite(led, HIGH);
cycle_start_time = micros();
// Serial.begin(115200);
}
void loop()
{
current_time = micros();
if (current_time - cycle_start_time > 9000) //interval for duty_cycle step change in uS
{
ramp == HIGH ? duty_cycle++:duty_cycle--; //change step in direction of fade change
if (duty_cycle > 99) //see if duty_cycle has reached maximum ON time
ramp = LOW; //reverse direction to DOWN
if (duty_cycle < 2) //see if duty_cycle has reached minimum ON time
ramp = HIGH; //reverse direction to UP
cycle_start_time = micros(); //record time of most recent step change
}
}
void ramp_LED()
{
pwm_cntr++;
if (pwm_cntr == duty_cycle) //test if cntr has reached turn_off threshold
{
digitalWrite(led, LOW);
}
if (pwm_cntr == 99) //test if cntr has reached full period & then reset
{
pwm_cntr = 0;
digitalWrite(led, HIGH);
}
}
I'm still struggling to find the cause of the flicker/flash in this routine. Simple as it appears to be, the cause is not yet apparent to me. I have tried a modification that took the digitalWrites out of the ISR and placed them in the main routine. This lets the ISR do nothing more than toggle a next state flag. While the flickers changed timing, they are still there. I'm still hoping someone can see what I'm not.
Lyle
#include <TimerOne.h>
int led = 13;
unsigned long current_time, cycle_start_time;
volatile int duty_cycle = 0;
volatile int pwm_cntr = 0;
int ramp = HIGH; //direction of fade, HIGH= up, LOW = down
volatile byte led_current_state = HIGH, led_next_state = HIGH;
void ramp_LED();
void setup() {
Timer1.initialize(240); //set period for interrupts
Timer1.attachInterrupt(ramp_LED);
pinMode(led, OUTPUT);
digitalWrite(led, HIGH);
cycle_start_time = micros();
// Serial.begin(115200);
}
void loop()
{
current_time = micros();
if (current_time - cycle_start_time > 9000) //interval for duty_cycle step change in uS
{
ramp == HIGH ? duty_cycle++ : duty_cycle--; //change step in direction of fade change
if (duty_cycle > 99) //see if duty_cycle has reached maximum ON time
ramp = LOW; //reverse direction to DOWN
if (duty_cycle < 2) //see if duty_cycle has reached minimum ON time
ramp = HIGH; //reverse direction to UP
cycle_start_time = micros(); //record time of most recent step change
}
if (led_next_state != led_current_state) //see if LED is to change state
{
if(led_next_state == HIGH) //LED is to be lit
{
digitalWrite(led, HIGH); //turn ON led
led_current_state = HIGH; //record present state of LED
}
else
{
digitalWrite(led, LOW); //turn OFF led
led_current_state = LOW; //record present state of LED
}
}
}
void ramp_LED()
{
pwm_cntr++;
if (pwm_cntr == duty_cycle) //test if cntr has reached turn_off threshold
led_next_state = LOW;
if (pwm_cntr == 99) //test if cntr has reached full period & then reset
{
pwm_cntr = 1;
led_next_state = HIGH;
}
}
After a bit of experimenting, this reworked version seems to be flicker free:
#include <TimerOne.h>
const int led = 13;
unsigned long current_time, cycle_start_time;
volatile int duty_cycle = 0;
volatile int pwm_cntr = 0;
int ramp = HIGH; //direction of fade, HIGH= up, LOW = down
volatile byte led_next_state = HIGH;
byte led_current_state = HIGH;
const int MAX_DUTY_CYCLE = 99;
const unsigned long CYCLE_INTERVAL = 9000; // microseconds
void ramp_LED();
void setup() {
Timer1.initialize(240); //set period for interrupts
Timer1.attachInterrupt(ramp_LED);
pinMode(led, OUTPUT);
digitalWrite(led, HIGH);
cycle_start_time = micros();
// Serial.begin(115200);
}
void loop()
{
noInterrupts ();
current_time = micros();
if (current_time - cycle_start_time > CYCLE_INTERVAL) //interval for duty_cycle step change in uS
{
if (ramp == HIGH) //change step in direction of fade change
duty_cycle++;
else
duty_cycle--;
if (duty_cycle >= MAX_DUTY_CYCLE) //see if duty_cycle has reached maximum ON time
ramp = LOW; //reverse direction to DOWN
if (duty_cycle <= 2) //see if duty_cycle has reached minimum ON time
ramp = HIGH; //reverse direction to UP
cycle_start_time = current_time; //record time of most recent step change
}
if (led_next_state != led_current_state) //see if LED is to change state
{
if(led_next_state == HIGH) //LED is to be lit
{
digitalWrite(led, HIGH); //turn ON led
led_current_state = HIGH; //record present state of LED
}
else
{
digitalWrite(led, LOW); //turn OFF led
led_current_state = LOW; //record present state of LED
}
}
interrupts ();
}
void ramp_LED()
{
pwm_cntr++;
if (pwm_cntr >= duty_cycle) //test if cntr has reached turn_off threshold
led_next_state = LOW;
if (pwm_cntr >= MAX_DUTY_CYCLE) //test if cntr has reached full period & then reset
{
pwm_cntr = 1;
led_next_state = HIGH;
}
}
I put in the noInterrupts() / interrupts() because of the accessing of 16-bit variables outside an ISR where they might be in the process of changing.
(See "critical sections").
However I think the important change was to change the equality tests in ramp_LED. Initially I thought they were OK, because you only add to pwm_cntr in the one place, but the critical part is that duty_cycle can change outside the ISR.
So imagine if pwm_cntr was about to reach duty_cycle in this line:
if (pwm_cntr == duty_cycle) //test if cntr has reached turn_off threshold
Now imagine that the main code decreases duty_cycle by one. Now the ISR overshoots, and misses that "if" condition.
Further testing indicates that taking out the noInterrupts() / interrupts() lines doesn't seem to affect things, however in principle you should guard those variable accesses. Maybe put noInterrupts() / interrupts() around the exact places where that shared access is happening.
Agree about guarding access. But (there's always a butt) with the value range (0-99) and the type of access (read-only in one context; write-only in the other) guarding is not necessary. Just don't tell anyone. Getting people to use the correct general purpose pattern has been extremely difficult. We really do not want to muddy the waters discussing when it is not necessary. (The relevant variables really should be changed to byte (uint8_t) to reflect (document) the limited range.)
Thanks for the insight and edit, it works! I know it's easy to say now, but it felt like it had to be something in the ISR, I just couldn't come up with a cause. . It didn't dawn on me, but it makes total sense that the duty_cycle variable might be altered at just the wrong time and pull the carpet out from under the operation. During the course of my edits I would introduce things that would cause small changes in timing and result in noticeable changes in the flicker rate and position. One of the things I had tried was a no_interrupt wrapper like yours, but I did it with Timer1.detachInterrupt. Wonder if that might have different results? I may test that, but I'm not driven to do so.
Detaching the interrupt is a bit different but would have the same end-effect (as noInterrupts). With or without interrupts enabled, the problem was that you changed the end point of the test which allowed it to overshoot.
It's a not uncommon problem with timers. For example, if you are counting up to 200, and the count is currently 150, and then you change the count to be up to 100, you are now 50 past that point. You will particularly notice it if you are comparing equal (as the hardware timers do). For a 16-bit timer, for example, it now counts up to 65535 and you get a very noticeable effect.