Inaccurate OCR1A interrupt compare

Hi all,

I'm working on a project of creating a 16 bit PWM output using only the Arduino board (Duemilanove 328).
And it's working great really! I can control all of the pins individually with a unique 16 bit duty cycle.

Only problem however is: my Timer1 interrupt sometimes "compares" too early with the OCR1A that I've set.
This problem only occurs in the interval OCR1AH/OCR1AL of opproximately 2/128 to 3/128.
So almost 256 steps of around 56000 total I get a miscompare. This is quite annoying and I have no idea why this is happening. The past few days I've been breaking my head over this with no avail.
I really hope somebody can help me out :slight_smile:

Below is my program, quite lengthy I know. But no need to inspect everything, only the ISR routine really. The other code is mainly to enable PWM on all pins rather than one alone.
Basically what my code does is: it checks for the pin with the lowest duty cycle (OCR1AH/OCR1AL) and sets that as the compare value. Next it checks the pin with the lowest but one duty cycle and sets that as OCR1AH/OCR1AL. This goes on until it has checked all the pins at which it sets the OCR1AH/OCR1AL to 255/255 and updates the appropriate values of all the pins.

What the program outputs is this: TCTN1H TCNT1L - OCR1AH OCR1AL
These are the timer values at which the interrupt is executed and at which it should have been executed. Usually there is an offset of about 70 low bytes, due to variable declaration, ..
However, wait a bit and you'll see a very strange behavior when it should execute around 2/128 at which it suddenly executes at 0/96.
This is NOT when it should have happened! I truly have no idea why this happens, and only around that value.
For those who want to visualize the problem, look at the onboard LED to see it become somewhat more dim at the "problem zone". Or you could attach an LED with a resistor to pin 8.

A very mysterious problem. Anybody who has any ideas or has experienced this in the past, please let me know.
Thanks in advance.

#include <avr/interrupt.h>  
#include <avr/io.h>

const int aantalPwmPins = 9; // edit this

volatile byte pwmPins[aantalPwmPins] = {0,1,2,8,4,10,11,12,13}; // edit this
volatile byte compValH[aantalPwmPins];
volatile byte compValL[aantalPwmPins];
volatile byte tempCompValH[aantalPwmPins];
volatile byte tempCompValL[aantalPwmPins];


volatile byte tempOCH;
volatile byte tempOCL;
volatile byte beginIndex = 0;

volatile byte tempOrder[aantalPwmPins];
volatile byte tempPwmOrder[aantalPwmPins];
volatile byte pwmOrder[aantalPwmPins];
volatile byte flagOrder[aantalPwmPins];

//TEMP
volatile byte hi;
volatile byte lo;

ISR(TIMER1_OVF_vect) {  
 
};  

ISR(TIMER1_COMPA_vect) {

tempOCH = OCR1AH;
tempOCL = OCR1AL;

lo = TCNT1L;
hi = TCNT1H;

  for(byte i = beginIndex; i<aantalPwmPins; i++)
  {
    if(tempOCL == compValL[pwmOrder[i]] && tempOCH == compValH[pwmOrder[i]])
    {
      writeOffToPin(pwmPins[pwmOrder[i]]);

      if(i != aantalPwmPins - 1)
      {
        OCR1AH = compValH[pwmOrder[i+1]];
        OCR1AL = compValL[pwmOrder[i+1]];
      }
      else
      {
        OCR1AH = 255;
        OCR1AL = 255;
      }
      
      TCNT1H = tempOCH;
      TCNT1L = tempOCL;
    }
  }
  
  if(tempOCL == 255 && tempOCH == 255)
  {    
    TIMSK1 &= 253;
    
    for(byte j = 0; j < aantalPwmPins; j++)
    {
      compValH[j] = tempCompValH[j];
      compValL[j] = tempCompValL[j];
      pwmOrder[j] = tempPwmOrder[j];
    }
    
    for(byte i = 0; i < aantalPwmPins; i++)
    {
      if(compValH[pwmOrder[i]] == 0)
      {
        if(compValL[pwmOrder[i]] == 0)
          continue;

        beginIndex = i;
        OCR1AH = compValH[pwmOrder[i]];
        OCR1AL = compValL[pwmOrder[i]];
        break;
      }
      else
      {
        beginIndex = i;
        OCR1AH = compValH[pwmOrder[i]];
        OCR1AL = compValL[pwmOrder[i]];
        break;
      }
    }    

    for(byte j = 0; j < aantalPwmPins; j++)
    {
      if(!(tempCompValH[pwmOrder[j]] == 0 && tempCompValL[pwmOrder[j]] == 0))
        writeOnToPin(pwmPins[pwmOrder[j]]);
    }

    TCNT1H = 0;
    TCNT1L = 0;
    
    TIMSK1 |= 2;
  }
};

void setup() {
  Serial.begin(115200);
  for(int i = 0; i < 14; i++)
    pinMode(i, OUTPUT);
  
  setupTimers();  
}

void loop()
{
  for(int h = 0; h < 50; h++)
  {
    for(int l = 0; l < 256; l++)
    {
      updatePwm(8, (byte) h, (byte) l);
      updatePwm(13, (byte) h, (byte) l);
      
      Serial.print(hi, DEC);
      Serial.print(" ");
      Serial.print(lo, DEC);
      
      Serial.print(" - ");
      Serial.print(h, DEC);
      Serial.print(" ");
      Serial.println(l, DEC);

      delay(10);
    }
    analogWrite(6, h);
  }
}

void setupTimers()
{
  TCCR1A = 0;
  TCCR1B = 0;

  TCCR1B = _BV(CS10);
  TIMSK1 = _BV(OCIE1A) | _BV(TOIE1);
  
  OCR1AH = 255;
  OCR1AL = 255;
  
  TIFR1 = 1;
}

void writeOnToPin(byte pin)
{
  if(pin < 8) //PORTD
  {
    PORTD |= 1<<pin;
  }
  else if(pin < 14) //PORTB
  {   
    pin -= 8;
    PORTB |= 1<<pin;
  }
}

void writeOffToPin(byte pin)
{
  if(pin < 8) //PORTD
  {
    PORTD &= ~(1<<pin);
  }
  else if(pin < 14) //PORTB
  {
    pin -= 8;
    PORTB &= ~(1<<pin);
  }
}


void printArray(volatile byte *a)
{
  for (int i = 0; i < aantalPwmPins; i++)
  {
    if(a[i] < 100) Serial.print(" ");
    if(a[i] < 10)  Serial.print(" ");
    Serial.print(a[i], DEC);
    Serial.print(' ');
  }
  //Serial.println();
}


void updatePwm(byte pin, byte high, byte low)
{
  byte c = pwmIndex(pin); 
  if(c != 255)
  {
    tempCompValH[c] = high;
    tempCompValL[c] = low;
    
    newQuickSort();
  }
}

byte pwmIndex(byte c)
{
   for(byte i = 0; i < aantalPwmPins; i++)
     if(pwmPins[i] == c) return i;
     
   return 255; //error
}     



void newQuickSort()
{
  for(byte i = 0; i < aantalPwmPins; i++)
    flagOrder[i] = false;
  
  byte lowestH = 255;
  byte lowestL = 255;
  byte lowestIndex;
  
  
  for(byte i = 0; i < aantalPwmPins; i++)
  {
    for(byte j = 0; j < aantalPwmPins; j++)
    {
      if(flagOrder[j] == false)
      {
        if(tempCompValH[j] < lowestH)
        {
          lowestIndex = j;
          lowestH = tempCompValH[j];
          lowestL = tempCompValL[j];
        }
        else if(tempCompValH[j] == lowestH)
        {
          if(tempCompValL[j] <= lowestL)
          {
            lowestIndex = j;
            lowestH = tempCompValH[j];
            lowestL = tempCompValL[j];
          }
        }
      }
    }
    
    flagOrder[lowestIndex] = true;
    tempOrder[i] = lowestIndex;
    lowestH = 255;
    lowestL = 255;
  }
  
  cli();
  for(byte i = 0; i < aantalPwmPins; i++)
    tempPwmOrder[i] = tempOrder[i];
  sei();
}

Before trying to diagnose any ISR problem, you need to first ensure you haven't made any obvious mistakes...

//TEMP
byte hi;
byte lo;

...are shared between the ISR and loop. They have to be declared volatile. Make that change, ensure there are not any other similar mistakes, and retest. If you still believe you have a problem, report back.

They're just for testing purposes, so no harm done there if some value was wrong. But I just changed all the variables to volatile and the problem still persists, it even lasts a little longer. Ranging from 2/170 to 3/323.
I've updated my source code above as well.

I appreciate the effort, though.

I have an idea... but since your code is quite complex and I am not a counter guru I might be wrong..

However, The code in the ISR is quite slow I think, as it searches for matches deeper in the arrays it will get slower and slower.

Until the time in the ISR exceeds the difference in the counts perhaps?

ISR(TIMER1_COMPA_vect) {

tempOCH = OCR1AH;
tempOCL = OCR1AL;

lo = TCNT1L;
hi = TCNT1H;

  for(byte i = beginIndex; i<aantalPwmPins; i++)
  {
    if(tempOCL == compValL[pwmOrder[i]] && tempOCH == compValH[pwmOrder[i]])
    {
      writeOffToPin(pwmPins[pwmOrder[i]]);

Could be faster done like this..

ISR(TIMER1_COMPA_vect) {

tempOCH = OCR1AH;
tempOCL = OCR1AL;

lo = TCNT1L;
hi = TCNT1H;

// use a static loop counter...
static byte i = 255 ;
if (i==255) i = beginIndex ;

  for(; i<aantalPwmPins; i++) // and start looking at the last match in the array
// most times should pass through the loop only once.

// the first time it will search on average half the table to find the point where it should be
// some times the first search will take so long that the next matching count entry is 
// missed.. and hence the search will look through two table entries to get the right entry
// Should be ok after that.
  {
     if (tempOCH > compValH[pwmOrder[i]]) // current table value is in the past
         continue ; // look at next table entry
     if (tempOCH < compValH[pwmOrder[i]])
         return ; // can not match yet
     // not < or > so ==
     if(tempOCL > compValL[pwmOrder[i]]) // current table value in past
         continue; // look at next table entry
     if(tempOCL < compValL[pwmOrder[i]])
         return ; // can not match yet.
    // not < or > so ==
      writeOffToPin(pwmPins[pwmOrder[i]]);
      // more work

      return ; // leaving i set to the last matched location.
    }
    // reset i to 255 to restart search, get to here when searched full range of i in for loop.
    i = 255 ;
}

Actually what is intended at the end of the loop is not clear to me.. so I have probably broken the code.

But the intent, to only search incrementally through the sorted array is clear I hope.

Also, I split the comparison of H and L parts into two parts. I have used this logic structure before, but am not sure of the definition of these two bytes, it is really a guess based on the names.

good luck.

You NEVER execute so much code inside an ISR, if you want to execute a lot of code inside an ISR you set a flag inside the ISR and read it in the main loop and if its set you execute the corresponding code, what you are suffering is interrupts that are interrutping interrups and you may end in a stack overflow problem.

Two more comments..
Code like the your setupTimers() requires me to read the datasheet to understand.
For some reason I can not be bothered.
Perhaps, because when I write code that contains obscure special values in combinations, I add comments about what the values are intended to achieve, and on what page of the datasheet I can confirm the correctness of my assumptions.
(ok - no I dont record the references to the datasheet page but I should)

So - I have no idea what the timer is set up to do...
I guess you do, but can not validate it. Neither can anyone else.

void setupTimers()
{
  TCCR1A = 0;
  TCCR1B = 0;

  TCCR1B = _BV(CS10);
  TIMSK1 = _BV(OCIE1A) | _BV(TOIE1);
  
  OCR1AH = 255;
  OCR1AL = 255;
  
  TIFR1 = 1;
}

And the second comment is, I missed the indirection by pwmOrder in all the code the ISR().

You could reorder the data outside the ISR() so that the access order is the native order.
The ISR() code would be about 2 x faster for that, especially as the array pwmOrder is declared volatile.

Actually, I think that the code has WAY too many 'volatile' variables for reasonable speed.

You only refer to the non temp arrays in the ISR I see.
So they do not need to be volatile.

Also, the temp arrays are read by the ISR and are built in main line with interrupts disabled.
Except that they are not.. the tempCompValL[pin] and tempCompValH[pin] are changed while interrupts are disabled.. so the ISR() could be copying them before the sort order is updated. This is a bug that should trigger relatively rarely and be hard to find when you are looking for it. It would NOT cause the errors you report.
In any case, them being volatile does not help.

Then beginIndex, tempOCH and tempOCL are only used in the ISR() - so should be local variables to the ISR and not volatile. (beginIndex should be a static local to the ISR()).

These changes are all simple multiples of the code execution time...
So the code will be perhaps 6 x quicker - 2x for removing the indirection by pwmOrder and 3x for removing the surplus volatiles.

The change to the search from the entire array to only incremental will make a difference based on the number of pins (9), and even more if you add more pins.

again - good luck.

Thanks for the help !

I've already made some changes, modified the code to work a bit smoother and it helped a bit.
However I found a very neat solution that seems to eliminate the sudden "dim" effect.

I moved the compare part (OCR1AH/OCR1AL == 255/255) which actually means the end of the current duty cycle and update the values for each pin to the overflow vector handler.
There I set the clock to stop ticking the moment it enters and to start again when it ends the overflow handler. This seems to solve the problem, whereas I used to set the enable compare flag on/off.

I'll post more tonight, after running some more tests!

You NEVER execute so much code inside an ISR, if you want to execute a lot of code inside an ISR you set a flag inside the ISR and read it in the main loop and if its set you execute the corresponding code,

Absolutely.

what you are suffering is interrupts that are interrutping interrups and you may end in a stack overflow problem.

Not exactly, interrupts are disabled while the ISR is running (unless you specifically reenable them within the ISR). Most likely he is missing some interrupts.

Don