Go Down

Topic: pwm testing quirk (Read 2671 times) previous topic - next topic

l_dowell

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

Code: [Select]

#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);
  } 
}

Grumpy_Mike

Any variable used in an interrupt service routine must be declaired volatile.

larryd

As mike says.

See: trap 24 here:
http://www.gammon.com.au/forum/?id=12153&page=999#TRAP24
No technical PMs.
If you are asked a question, please respond with an answer.
If you are asked for more information, please supply it.
If you need clarification, ask for help.

l_dowell

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:
Code: [Select]

#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);
  } 
}

larryd

Code: [Select]
  cycle_start_time = micros();
//  Serial.begin(115200);
}

void loop()
{
  current_time = millis();
  if (current_time - cycle_start_time > 9)   


cycle_start_time = micros(); // <<<< uses micros()
current_time = millis();          // <<<< uses millis()

No technical PMs.
If you are asked a question, please respond with an answer.
If you are asked for more information, please supply it.
If you need clarification, ask for help.

l_dowell



cycle_start_time = micros(); // <<<< uses micros()
current_time = millis();          // <<<< uses millis()


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

Code: [Select]
#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);
  } 
}

l_dowell

#6
Dec 05, 2013, 05:51 pm Last Edit: Dec 05, 2013, 05:53 pm by l_dowell Reason: 1
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

Code: [Select]
#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;
  } 
}

Coding Badly

Code: [Select]
  if (pwm_cntr == duty_cycle)                   //test if cntr has reached turn_off threshold

Equals?


nickgammon

#9
Dec 05, 2013, 09:11 pm Last Edit: Dec 05, 2013, 09:23 pm by Nick Gammon Reason: 1
After a bit of experimenting, this reworked version seems to be flicker free:

Code: [Select]

#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.

http://www.gammon.com.au/interrupts

(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:

Code: [Select]

 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.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

nickgammon

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.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

Coding Badly

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.


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.)

nickgammon

Fair enough, but I looked at the "int" type and thought maybe one day he might make it go higher than 255.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

l_dowell

#13
Dec 06, 2013, 03:59 am Last Edit: Dec 06, 2013, 04:02 am by l_dowell Reason: 1
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.

Much appreciated
Lyle

nickgammon

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.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

Go Up