Help with clocking code

[younder]

Hi Folks,

I wrote the following code in order to get custom clock for general use (LCD display update, etc.) I usually use both (pulse & rise edge pulse).

My questios is: is that the best way in terms of CPU scan? Any other better way to get the same result?

Sorry, I'm not expert in the C language, so any help would be appreciated!

PS. I'm using CCS C 5.042

Hugo

Code:

#include <18f2550.h>
#device ICD = TRUE
#device ADC=10                
#use delay (clock=48000000)
#fuses HSPLL,NOWDT,NOPROTECT,NOBROWNOUT,NOLVP,USBDIV,PLL5,CPUDIV1,DEBUG

void Initialization() { 
//Disable all interrupts
   disable_interrupts(GLOBAL); 
//Configure Timer1 - Configura Timer1 
   setup_timer_1 (T1_INTERNAL | T1_DIV_BY_8); //Configures timer1 
   enable_interrupts(INT_TIMER1); //Enable Timer1 interrupt 
//Set AD for Internal Clock
   setup_adc(ADC_CLOCK_INTERNAL);
//Set GIE&PEIE bits and Enable all interrupts
   enable_interrupts(GLOBAL); 
 } 
 
// =============================================================
// "Timer1", interrupt every 10ms
// =============================================================
int1 Pulse_50ms=0;      
int1 Pulse_100ms=0;     
int1 Pulse_200ms=0;    
int1 Pulse_500ms=0;     
int1 Pulse_1000ms=0;  
#INT_TIMER1          
void timer1()         
  {
  static int8 Counter50=3;
  static int8 Counter100=0;
  static int8 Counter500=1;
  
      set_timer1(50536 + get_timer1()); //10ms @ 48 MHz
         
//50ms Clock generator
    Counter50++;
       if (Counter50==5) Pulse_50ms=!Pulse_50ms, Counter50=0;     
//100ms&200ms Clock generator
    Counter100++;
       if (Counter100==10) Pulse_100ms=1;
       if (Counter100==20) 
       {
       Pulse_200ms=!Pulse_200ms;
       Pulse_100ms=0;
       Counter100=0;
       }
//500ms&1s Clock generator
    Counter500++;
       if (Counter500==50) Pulse_500ms=1;
       if (Counter500==100)
       {
       Pulse_1000ms=!Pulse_1000ms;
       Pulse_500ms=0; 
       Counter500=0;
       }
 }
 
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 50ms 
// =============================================================
int1 Edge_50ms;  
void Function_Edge_50ms() //Function declaration
{
static int1 aux50;     //Auxiliar Variable 
   if (Pulse_50ms) //If pulse is true 
      {  
         if (aux50==0) Edge_50ms=1; else Edge_50ms=0; //and aux50 is false then Edge_50ms=TRUE for only one scan 
         aux50=1; //Next scan Edge_50ms will be false
      }
      else aux50=0; //Clear aux50 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 100ms
// =============================================================
int1 Edge_100ms;  
void Function_Edge_100ms() //Function declaration
{
static int1 aux100; //Auxiliar Variable
   if (Pulse_100ms) //If pulse is true
      {  
         if (aux100==0) Edge_100ms=1; else Edge_100ms=0; //and aux100 is false then Edge_100ms=TRUE for only one scan
         aux100=1; //Next scan Edge_100ms will be false
      }
      else aux100=0; //Clear aux100 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 200ms
// =============================================================
int1 Edge_200ms; 
void Function_Edge_200ms() //Function declaration
{
static int1 aux200; //Auxiliar Variable
   if (Pulse_200ms) //If pulse is true
      {  
         if (aux200==0) Edge_200ms=1; else Edge_200ms=0; //and aux200 is false then Edge_200ms=TRUE for only one scan
         aux200=1; //Next scan Edge_200ms will be false
      }
      else aux200=0; //Clear aux200 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 500ms
// =============================================================
int1 Edge_500ms;  
void Function_Edge_500ms() //Function declaration
{
static int1 aux500; //Auxiliar Variable
   if (Pulse_500ms) //If pulse is true
      {  
         if (aux500==0) Edge_500ms=1; else Edge_500ms=0; //and aux500 is false then Edge_500ms=TRUE for only one scan
         aux500=1; //Next scan Edge_500ms will be false 
      }
      else aux500=0; //Clear aux500 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 1000ms
// =============================================================
int1 Edge_1000ms; 
void Function_Edge_1000ms() //Function declaration
{
static int1 aux1000; //Auxiliar Variable
   if (Pulse_1000ms) //If pulse is true
      {  
         if (aux1000==0) Edge_1000ms=1; else Edge_1000ms=0; //and aux1000 is false then Edge_1000ms=TRUE for only one scan
         aux1000=1; //Next scan Edge_1000ms will be false 
      }
      else aux1000=0; //Clear aux500 till next rising edge detection
}
// *************************************************************

void Main() { 

Unsigned int16 Temp0=0; 
int1 Temp1=0;

  Initialization();
   while(1){ 

  Function_Edge_50ms();  
  Function_Edge_100ms(); 
  Function_Edge_200ms(); 
  Function_Edge_500ms();
  Function_Edge_1000ms(); 
  
  if (Edge_500ms) Temp0++; // increment temp0 by 1 every 1s
  if (Pulse_500ms) Temp1=1; else Temp1=0; // 500ms blinking bool
  
  //if (Edge_100ms) printf(LCD_PUTC,"%4ld %1lu",Temp0,Temp1); //display every 100ms
         } 
} 
//------------------ EOF --------------------- 

[younder]

Any expert to help me with my poor code?

[hate]

From a quick glance at your code, it seems you use the 'add a predetermined value to current timer value to overflow the timer with a certain period' technique.

Code:

set_timer1(50536 + get_timer1()); //Pulso de clock de 100ms @ 48 MHz

That technique is also my favorite for very accurate timings. But I don't think your usage of the flags are appropriate for that accuracy as you may loose some of it while the ISR returns control to the interrupted function. If you don't need that precision, that's ok but if you need the precision, my advise would be to perform whatever task you need to perform in the ISR itself. And please use English comments next time if you seek for advice again.

Btw I'm not sure with the units you used in your comments. 100ms pulse with a 16-bit timer at 48MHz? Are you sure about that? Or is it a 32-bit timer?

[younder]

I've just prepared an example code... and removed the portuguese comments 

I also replaced the initial code (original post) by the one below so that we can avoid misunderstandings...

Code:

#include <18f2550.h>
#device ICD = TRUE
#device ADC=10                
#use delay (clock=48000000)
#fuses HSPLL,NOWDT,NOPROTECT,NOBROWNOUT,NOLVP,USBDIV,PLL5,CPUDIV1,DEBUG

void Initialization() { 
//Disable all interrupts
   disable_interrupts(GLOBAL); 
//Configure Timer1 - Configura Timer1 
   setup_timer_1 (T1_INTERNAL | T1_DIV_BY_8); //Configures timer1 
   enable_interrupts(INT_TIMER1); //Enable Timer1 interrupt 
//Set AD for Internal Clock
   setup_adc(ADC_CLOCK_INTERNAL);
//Set GIE&PEIE bits and Enable all interrupts
   enable_interrupts(GLOBAL); 
 } 
 
// =============================================================
// "Timer1", interrupt every 10ms
// =============================================================
int1 Pulse_50ms=0;      
int1 Pulse_100ms=0;     
int1 Pulse_200ms=0;    
int1 Pulse_500ms=0;     
int1 Pulse_1000ms=0;  
#INT_TIMER1          
void timer1()         
  {
  static int8 Counter50=3;
  static int8 Counter100=0;
  static int8 Counter500=1;
  
      set_timer1(50536 + get_timer1()); //10ms @ 48 MHz
         
//50ms Clock generator
    Counter50++;
       if (Counter50==5) Pulse_50ms=!Pulse_50ms, Counter50=0;     
//100ms&200ms Clock generator
    Counter100++;
       if (Counter100==10) Pulse_100ms=1;
       if (Counter100==20) 
       {
       Pulse_200ms=!Pulse_200ms;
       Pulse_100ms=0;
       Counter100=0;
       }
//500ms&1s Clock generator
    Counter500++;
       if (Counter500==50) Pulse_500ms=1;
       if (Counter500==100)
       {
       Pulse_1000ms=!Pulse_1000ms;
       Pulse_500ms=0; 
       Counter500=0;
       }
 }
 
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 50ms 
// =============================================================
int1 Edge_50ms;  
void Function_Edge_50ms() //Function declaration
{
static int1 aux50;     //Auxiliar Variable 
   if (Pulse_50ms) //If pulse is true 
      {  
         if (aux50==0) Edge_50ms=1; else Edge_50ms=0; //and aux50 is false then Edge_50ms=TRUE for only one scan 
         aux50=1; //Next scan Edge_50ms will be false
      }
      else aux50=0; //Clear aux50 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 100ms
// =============================================================
int1 Edge_100ms;  
void Function_Edge_100ms() //Function declaration
{
static int1 aux100; //Auxiliar Variable
   if (Pulse_100ms) //If pulse is true
      {  
         if (aux100==0) Edge_100ms=1; else Edge_100ms=0; //and aux100 is false then Edge_100ms=TRUE for only one scan
         aux100=1; //Next scan Edge_100ms will be false
      }
      else aux100=0; //Clear aux100 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 200ms
// =============================================================
int1 Edge_200ms; 
void Function_Edge_200ms() //Function declaration
{
static int1 aux200; //Auxiliar Variable
   if (Pulse_200ms) //If pulse is true
      {  
         if (aux200==0) Edge_200ms=1; else Edge_200ms=0; //and aux200 is false then Edge_200ms=TRUE for only one scan
         aux200=1; //Next scan Edge_200ms will be false
      }
      else aux200=0; //Clear aux200 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 500ms
// =============================================================
int1 Edge_500ms;  
void Function_Edge_500ms() //Function declaration
{
static int1 aux500; //Auxiliar Variable
   if (Pulse_500ms) //If pulse is true
      {  
         if (aux500==0) Edge_500ms=1; else Edge_500ms=0; //and aux500 is false then Edge_500ms=TRUE for only one scan
         aux500=1; //Next scan Edge_500ms will be false 
      }
      else aux500=0; //Clear aux500 till next rising edge detection
}
// *************************************************************
// =============================================================
// "One Shot Rising, OSR", Generates rising edge pulse every 1000ms
// =============================================================
int1 Edge_1000ms; 
void Function_Edge_1000ms() //Function declaration
{
static int1 aux1000; //Auxiliar Variable
   if (Pulse_1000ms) //If pulse is true
      {  
         if (aux1000==0) Edge_1000ms=1; else Edge_1000ms=0; //and aux1000 is false then Edge_1000ms=TRUE for only one scan
         aux1000=1; //Next scan Edge_1000ms will be false 
      }
      else aux1000=0; //Clear aux500 till next rising edge detection
}
// *************************************************************

void Main() { 

Unsigned int16 Temp0=0; 
int1 Temp1=0;

  Initialization();
   while(1){ 

  Function_Edge_50ms();  
  Function_Edge_100ms(); 
  Function_Edge_200ms(); 
  Function_Edge_500ms();
  Function_Edge_1000ms(); 
  
  if (Edge_500ms) Temp0++; // increment temp0 by 1 every 1s
  if (Pulse_500ms) Temp1=1; else Temp1=0; // 500ms blinking bool
  
  //if (Edge_100ms) printf(LCD_PUTC,"%4ld %1lu",Temp0,Temp1); //display every 100ms
         } 
} 
//------------------ EOF --------------------- 

[FTL]

Btw I'm not sure with the units you used in your comments. 100ms pulse with a 16-bit timer at 48MHz? Are you sure about that? Or is it a 32-bit timer?

I think the basic timing parameters are correct:
48,000,000 as the oscillator (Tosc) rate.
Divide by 4 to get the timer increment rate (Tclk), so 12,000,000 Hz.
Divide by 8 since the divide by 8 pre-scaler was set, so 1,500,000 counts / sec on the timer.
The timer is set to 65,536-15,000 (50,536), so it will overflow after 15,000 counts. 1,500,000 / 15,000 = 100, so the timer will overflow 100 times a second, or every 10ms.

The timer reset adds in the existing value of the timer since it will have counted up a bit from when it overflowed until that statement in the interrupt function is executed, so that is good, but . . .

This will probably still lose a few counts with every interrupt cycle, since it takes several instructions to read the current timer value and reset the timer. You need to look at the generated assembler code and see how many instructions that takes, then divide by 8, and subtract that from the timer as well. Look at the timer reset assembler code and review the datasheet carefully since you are resetting a 2-byte timer so the timer is probably stopped, then the two bytes are loaded and the timer restarted again. That may lose a few counts.

Your code for detecting the various pulses and edges looks like it will work, but it uses a lot of overhead since it is polling the pulse variables in the main while loop, and as Hate said, that polling may cause some delays. I would set the edge variables in the ISR since you know when the pulses are changing value already.

Lastly, please look at re-formatting your code to make it much more readable with consistent indentation. Especially with the if's having the then and else clauses on one line sometimes and the subtle use of the comma operator in one place to execute two statements in a then clause instead of using { and } to create a block. Also indenting your comments to match the code indent level would make it much easier to read.

[hate]

I think the basic timing parameters are correct:
48,000,000 as the oscillator (Tosc) rate.
Divide by 4 to get the timer increment rate (Tclk), so 12,000,000 Hz.
Divide by 8 since the divide by 8 pre-scaler was set, so 1,500,000 counts / sec on the timer.
The timer is set to 65,536-15,000 (50,536), so it will overflow after 15,000 counts. 1,500,000 / 15,000 = 100, so the timer will overflow 100 times a second, or every 10ms.

Well, I haven't checked his settings nor done the calculations myself but just wondering, where do you exactly see a 10ms on the code OP originally posted:

Code:

      set_timer1(50536 + get_timer1()); //Pulso de clock de 100ms @ 48 MHz

Or on my reply:

Btw I'm not sure with the units you used in your comments. 100ms pulse with a 16-bit timer at 48MHz? Are you sure about that? Or is it a 32-bit timer?

Am I missing something or need glasses?

For the accuracy problem, it doesn't matter how many instructions it takes to read the timer, because as far as I recall the assembler 'ADD' instruction adds the value to the Timer's final value after the instruction is performed i.e. assuming the instruction takes 3 cycles (which sure isn't), the timers current value is 15 and a value to be added is 7, timer's current value becomes 18 after the execution of the instruction and 7 is added to make it 25 immediately, no time gaps lost. And I'm pretty sure the compiler is smart enough to add the LOW byte first.

Reading the value in the prescaler was quite complicated back in the days. Unless newer PICs provide a way to read how many clock cycles the prescaler has advanced, I can't think of another way than to provide an external clock to increment the timer to the next integer and time the operation. It's not worth the effort imo.

[FTL]

I never noticed the reference to 100ms in the original code. You are correct in questioning that. Sorry, I did not quite understand your comment. Certainly some blindness on my part. There are some references to 10ms though:

// "Timer1", interrupt every 10ms

and I think some of them were fixed in the updated code (I never really looked at the original code much). I guess I saw the comment about the interrupt being entered every 10ms, and then saw the 100ms pulse being changed every five 10ms interrupts, so I saw it as 10ms and never noticed the comment about 100ms. The interrupt code certainly does not execute every 100ms as indicated in some of the comments.

I have not looked at the code generated by the following statement, but was suggesting that the original poster should examine it. My experience in examining the code produced by the CCS compiler for PIC16's and PIC18's has often surprised me with how many assembler instructions it takes to accomplish what looks like a simple C statement.

set_timer1(50536 + get_timer1());

This is a Pic18, so it is an 8-bit processor. The timer (timer1) is a 16 bit timer. So it takes more than one instruction to extract the current value of the timer (get_timer1() call) from the two SFRs it is stored in and put it into two adjacent bytes to treat as a 16 bit integer. To do the extraction properly, you need to extract the high byte, then the low byte, then extract the high-byte again and compare it to the value you got before to be sure it hasn't changed because the low order byte rolled over while you were doing all of that. If the high order byte changed, you then need to re-read the low order byte. The addition then takes a few more instruction cycles, since the high and low order bytes must be added separately, and it needs to check for overflow on the low-order addition (requiring a test and possible branch). Depending on how well the code is optimized there may be function call overheads or the two function calls in this statement may be compiled in-line. Then to re-load the timer, it cannot be done in one operation, so it really needs to be done by stopping the timer, setting the two single byte values in the SFRs, then restarting it. You can't change it while it is running in case it overflowed from the low order byte to the high order byte between your two store instructions. I didn't bother to look up the datasheet for that device, but suggested that the OP did so to see what is involved in resetting the timer, since it may not be that simple.

If it takes more than 8 instruction cycles to extract the current timer, add it to the constant 50536 and then reset the timer, one or more counts of the timer could easily be lost in this process, so I was suggesting that the statement might actually need to be something like:

set_timer1(50536 + get_timer1()+2);

where the "2" needs to be verified by examining the assembler code to see what is happening. Since the code is using a 8x pre-scaler, it is less likely to be an issue, but if it were not using a pre-scaler, it would definitely lose counts with each interrupt and cause the interrupt to occur less than every 10ms (not by much, but a little less).

On all of the 8-bit PICs, all instructions take 1 instruction cycle (except branches which take 2). Entering an interrupt is less deterministic. The PIC datasheets usually have an exact timing chart for that. The whole purpose of the get_timer() call it to compensate for interrupt entry time, so my comments are directed more towards trying to be as accurate as possible.

If I were writing this in assembler, I could take advantage of the fact that I know that the 16 bit timer has a low value in it (high order byte is zero and the low order byte is less than 10) since it was just reset and we are executing early in the timer interrupt routine. The timer change could be done quite quickly by stopping the timer, setting the high order byte to a constant (197), then adding 104 to the value in the low byte, then restarting the timer. The compiler cannot make those assumptions since it cannot really take any guesses as to the value in the timer and must generate much more generalized 16-bit arithmetic code. (Note: 50536 = 197*256 + 104).

Edit:

I just looked up the PIC18f2550 datasheet. Timer1 has a 16-bit timer read/write mode where the high order byte of the timer is buffered, so the timer reads and writes can be done much more quickly than on some other PIC's. Assuming the compiler uses that feature, that reduces the overheads I talked about above (which do apply to PIC16 timers), so the generated code should be simpler and the delays are less, but the principle remains valid and I think the generated assembler code should be examined to get the most accurate timing.

[hate]

This is a Pic18, so it is an 8-bit processor. The timer (timer1) is a 16 bit timer. So it takes more than one instruction to extract the current value of the timer (get_timer1() call) from the two SFRs it is stored in and put it into two adjacent bytes to treat as a 16 bit integer. To do the extraction properly, you need to extract the high byte, then the low byte, then extract the high-byte again and compare it to the value you got before to be sure it hasn't changed because the low order byte rolled over while you were doing all of that. If the high order byte changed, you then need to re-read the low order byte. The addition then takes a few more instruction cycles, since the high and low order bytes must be added separately, and it needs to check for overflow on the low-order addition (requiring a test and possible branch). Depending on how well the code is optimized there may be function call overheads or the two function calls in this statement may be compiled in-line.

I get your point here and you're absolutely correct. My point is, the statement:

Code:

set_timer1(50536 + get_timer1());

is basically a:

Code:

a = a + b;

statement which can also be restated as:

Code:

a += b;

Now a clever optimizing compiler should just add the constant (b) to the timer1's current value (a) without reading timer1 first, performing the addition and writing the result back. My reasoning for this optimization depends on the fact that if 'set_timer' is used inline (I doubt it isn't for a statement like this), the only argument it gets is the timer's current value itself plus a constant. I accept that totally depends on how good the compiler is. It might be good or not.

Then to re-load the timer, it cannot be done in one operation, so it really needs to be done by stopping the timer, setting the two single byte values in the SFRs, then restarting it. You can't change it while it is running in case it overflowed from the low order byte to the high order byte between your two store instructions. I didn't bother to look up the datasheet for that device, but suggested that the OP did so to see what is involved in resetting the timer, since it may not be that simple.

Correct, except for the 16-bit read/write mode. But there is another factor here. When the timer overflows, it's high and low bytes are both zero if it's up-counting. The problem with the PICs interrupts is that it doesn't have an interrupt vector table unlike many other MCUs to allow different interrupts to be distinguished by hardware itself. Instead the compiler has to generate its own pseudo-vector table to distinguish between the interrupts and that takes a couple of clock cycles. But considering the code uses a single ISR as far as I can see, I don't think the low byte will overflow before adjusting the timer to its new value. So it will be basically:
1- Buffer the high byte
2- Perform an addition to the low byte
3- High byte will be automatically updated
I couldn't find any clue on the datasheet if the write operation cannot be performed like that (though I didn't search much) but this is how things are done in AVRs and I don't think PICs will have the disadvantage.

If it takes more than 8 instruction cycles to extract the current timer, add it to the constant 50536 and then reset the timer, one or more counts of the timer could easily be lost in this process, so I was suggesting that the statement might actually need to be something like:

set_timer1(50536 + get_timer1()+2);

where the "2" needs to be verified by examining the assembler code to see what is happening. Since the code is using a 8x pre-scaler, it is less likely to be an issue, but if it were not using a pre-scaler, it would definitely lose counts with each interrupt and cause the interrupt to occur less than every 10ms (not by much, but a little less).

In case the compiler isn't that smart, I agree with you here but the 2 hugely depends on the prescaler setting. I'm not sure I understand why would the timer lose count without a prescaler though? It's even the opposite, without a prescaler the timer will continue incrementing on each clock until the user code in the ISR is encountered and will have the number of clock cycles passed during the execution of the interrupt overhead. And if a prescaler is used, the timing would be a little over 10ms because of the lost cycles in the prescaler if they are not compensated.

On all of the 8-bit PICs, all instructions take 1 instruction cycle (except branches which take 2). Entering an interrupt is less deterministic. The PIC datasheets usually have an exact timing chart for that. The whole purpose of the get_timer() call it to compensate for interrupt entry time, so my comments are directed more towards trying to be as accurate as possible.

Agreed but again it doesn't matter how much it takes to enter the interrupt with that technique unless cpu-heavy operations are performed inside ISRs. The beauty lies in the ADDITION of a predetermined value to the current timer value and not simply INSERTING it.

If I were writing this in assembler, I could take advantage of the fact that I know that the 16 bit timer has a low value in it (high order byte is zero and the low order byte is less than 10) since it was just reset and we are executing early in the timer interrupt routine. The timer change could be done quite quickly by stopping the timer, setting the high order byte to a constant (197), then adding 104 to the value in the low byte, then restarting the timer. The compiler cannot make those assumptions since it cannot really take any guesses as to the value in the timer and must generate much more generalized 16-bit arithmetic code. (Note: 50536 = 197*256 + 104).

If you stop the timer, you'll definitely lose some clock cycles. My approach to this problem would be to discard the 'set_timer' function completely if I were in doubt with optimization and directly manipulate the high and low timer registers by hand to make sure they get updated correctly.

Btw most of the timings above would be inaccurate if the prescaler value is not compensated. If I were to code this, I wouldn't use any prescaler at all. Using a lower overflow value with a higher comparison value for a static counter variable in the ISR would do much better imo.

[FTL]

This is an interesting discussion. Firstly, I recognize that most of this is purely academic n this case as from what I gather from the OP, the "pulses" he generates do not really need any accuracy. He mentions LCD display updates for instance, where 1sec +/- 100ms is probably more than good enough, and we are discussing less than +/- 1us, which admittedly is certainly less than the accuracy of an internal oscillator and is probably less than the accuracy of even a good crystal running the chip.

My points all revolve around two facts:
- in general, it takes several instructions to do 16 bit arithmetic on an 8-bit PIC18 and one or more instruction cycles will pass between the initial read of the value and when the completed addition is saved in the timer counter again.
- the timer is updating asynchronously while you are trying to read and update it, so between any two instructions the low order byte could roll over and increment the high-order byte.

Now a clever optimizing compiler should just add the constant (b) to the timer1's current value (a) without reading timer1 first, performing the addition and writing the result back. My reasoning for this optimization depends on the fact that if 'set_timer' is used inline (I doubt it isn't for a statement like this), the only argument it gets is the timer's current value itself plus a constant. I accept that totally depends on how good the compiler is. It might be good or not.

On a PIC18 you cannot just add to the low-order byte and have the carry automatically add to the high order byte. On a PIC18, you need to add the low order byte of X (Xlow) to the low-order byte of the counter (Clow). The ADDWF instruction can do that in place, although an instruction is needed to first load Xlow into the W register. When the low-order addition is completed, the carry bit may be set. You then need to add the carry bit, Xhigh and Chigh and store it in Chigh. you must first load Xhigh into W, then ADDWFC will conveniently do the add and store in one instruction. That is theory only takes four instructions for a general 16-bit addition, with three instructions passing between the first read of the value and the final set with the completed addition.

The next problem is that you can't do that directly to the counter registers as they may change between the two addition instructions as Clow may have had a value of 255 and you could have been on the last cycle of the pre-scaler. If Clow overflowed between the ADDWF and ADDWFC instructions, Chigh would have been automatically incremented and the counter would now be 256 values too low as the Chigh increment would have been lost.

I agree that most of the CCS built-in functions would normally be expanded inline, but I think it is a stretch to assume the compiler would recognize that the get_timer1() and set_timer1() calls in this statement are effectively a "A+=50336" construct.

The timer is effectively stopped as soon as you read its value since you are going to replace its value with the value you just read. Any timer increments between the read and the write are lost.

The 16-bit read/write mode of the timer is some help, but not as much as it might appear, since it is the high-order byte that is buffered and the actual read/write is initiated by the operation on the low-order byte. That means the ADDWF and ADDWFC must be done on temporary variables that must then be moved into the high and low order bytes (in that order) of the timer.

Correct, except for the 16-bit read/write mode. But there is another factor here. When the timer overflows, it's high and low bytes are both zero if it's up-counting. . . .

Yes, but the compiler can't really assume that. That is what I was referring to when I gave the example of how I would do it is I were writing it in assembler. The compiler does not know if interrupts are disabled for an extended period elsewhere in the program for instance, so it is possible from the compiler's point of view that the low order byte of the timer is about to overflow, so a simplified algorithm cannot be used.

Another interesting thing with updating the timer with a pre-scaler is the datasheet says that the pre-scaler is cleared whenever the timer value is changed by the program, so on average half of the pre-scaler count would be lost in this scenario. Even that is not totally true since we are in a situation here where both the timer and the pre-scaler have just been reset by the timer overflow that caused the interrupt and we are a relatively fixed number of instructions after that event.

In case the compiler isn't that smart, I agree with you here but the 2 hugely depends on the prescaler setting.

I agree. The 2 is just an example. The compiler would certainly optimize the addition out by adding it to the constant at compile time, but I like to code that way to document what is happening. In fact, I would not use 50536 at all. I would code: "65536 - 15000 + x" (where x may actually be 0-3 inclusive) so it is clear I am setting the timer to overflow in 15000 counts less a very small amount.

I would guess that the best value for the additional time is probably between zero and three (inclusive), but my point is that it should be considered even if after consideration that a value of zero is determined to be the best value. I am agreeing with you that zero may be the correct value here, but you don't know for sure unless you examine the compiler generated code.

If I were to code this, I wouldn't use any prescaler at all.

If extreme accuracy were the goal, I would do that as well. I would also use a couple of lines of inline assembler to update the timer value. On the other hand, it would not be possible to get a 10ms delay with no prescaler. With a 48Mhz oscillator, the 16 bit timer overflows every 5.5ms or so. It could be set for a 5ms timer pop and then double all the various counters in the code that generate the longer term pulses. That would double the number of interrupts currently processed.

Instead the compiler has to generate its own pseudo-vector table to distinguish between the interrupts and that takes a couple of clock cycles.

Actually the PIC18's have dual priority interrupts, and any event can be assigned to any priority. With up to two active interrupts, there is no requirement to test all the flags. But you do have to remember that the higher-priority interrupts can interrupt the lower-priority ones. There is no hardware status saving in the PIC18 interrupt handler, so the interrupt prologue code must save some of the status registers. As well, the last time I looked at CCS generated interrupt code, it saves a few temporary variables that it expects to keep their values. The interrupt prologue code was actually quite long.

3- High byte will be automatically updated

The PIC18 has no hardware support to automatically add the carry bit to a high order byte. It must be done with several instructions.

Anyway, thanks for the comments and the discussion. It gave me a reason to carefully think about a few PIC18 related things and has reminded me about a few features of the PIC18's that I had forgotten about.

[hate]

Well, I still don't agree with some of your points but I won't go into the trouble of making another post as it's been way too long since I used a PIC after I switched to the AVRs. Maybe I'm wrong with the compiler, maybe I don't recall the PIC hardware correctly or maybe my PIC assembler is rusty. So a lot of 'maybe's by my side which I have no time to make sure of. The only thing I'm sure though is that I had used that technique back then when I'm using PICs and now with AVRs to generate very precious timings in case needed. The way that I implemented it didn't skip a single clock cycle no matter how many clock cycles are lost in the interrupt overhead or user code on PICs too (btw I'm aware of priority interrupts of PIC18s but I don't see this as an interrupt vector but just as a 1 level deep interrupt priority feature ). Anyway I think we both made our points and the rest is left to the OP to use and test what he/she gathered from this discussion and make a post of his/her findings on his/her will. Thank you too for the discussion.

[FTL]

Agreed. Experimentation and examining the actual compiler output is required to go further. 'till next time.

[Ichan]

Mentioned already on both replies but maybe hidden on those many words - generate the pulse inside the ISR.

Use a variable as a flag something like enable_50ms, then inside the ISR there will be if enable_50ms then....

That flag can be set / reset from anywhere outside the ISR.

-ichan.

[younder]

Guys, nice discussion...however I use the mentioned pulses in most of my codes...and the main concern I have is regarding the particular scan in where for example, the rise edge 50ms is being used for call many functions or even do some math...display at LCD etc. I Also was wondering if the way I'm generating the flags inside the ISR is most appropriate way in terms of processing. Thinking in how to improve the code...I get to another possibility which would be a for loop + table inside the ISR instead many if's... I also realized that... why interrupt this fast? The slowest 'tick' I'm using is 50mSec, and with the chosen divider, it's possible to interrupt tick at this rate, by making it tick at 25mSec easily, and this then reduces the CPU time in the interrupt by an enormous amount....

Finnaly, switch to 37500 counts from the current 15000, and change the count values to 2,4,8,20,40. This would take the CPU time down to under 0.5* straight away.....

Below the for loop ideia:

Code:

int8 counts[] = {5,10,20,50,100}; 
union { 
   int8 mask; 
   struct { 
      int1 MS_50; 
      int1 MS_100; 
      int1 MS_200; 
      int1 MS_500; 
      int1 MA_1000; 
   } pulse; 
} 
#INT_TIMER1             
void timer1(void)           
{ 
  static int8 counters[5] = {3,1,1,1,1}; 
  int8 ctr; 
  int8 mask=1; 
  set_timer1(50536 + get_timer1()); //15000 counts 
  for (ctr=0;ctr<5;ctr++) 
  { 
     --counters[ctr]; 
     if (counters[ctr]==0) 
     { 
        counters[ctr]=counts[ctr]; 
        pulse.mask ^= mask; 
     } 
     mask*=2; 
  } 
} 

[Ichan]

Interesting code, i do not use CCS to verify it. Some comments / questions:
- why counters[] array initialized with {3,1,1,1,1} not all 1?
- does the pulse var really need to be in structure ?
- pulse var not as volatile ?
- how is the use the "pulse" generated, scanning it on the main loop?

-ichan

[Signal]

1) Intended use of this "tool" is too inconcrete (undefined) for me. And at the same time too restrictive for client: only set of predefined periods.
2) I assume Pulse variables are intended to be polled by clients.
3) set_timer1(50536 + get_timer1())
I'd make it on lower level, possibly start with searching for supporting library. Otherwise it is strange to have timer and CCP modules built in chip but to emulate periodic interrupts manually.
4) edge moments (and associated cpu burden) are not spreaded in time - bad for precision.
5) int1 Pulse_50ms=0;
actually it is bool. In terms of uC - one bit.
6) I do not see why predefined set (50 100 200 500 1000) of ms is better than (50 100 200 400 800) or (43.7 87 175 350 699) ms.

So forgetting for the moment that I do not get the point of discussed task, I'd do the following (improvisation):

Code:

unsigned int pulse;

#define MASK_BASE 12 //TODO 1: define value according to used timer and requirements for periods
enum PERIODS{
  P0_043 = 1u<<MASK_BASE,
  P0_087 = 1u<<(MASK_BASE+1),
  P0_175 = 1u<<(MASK_BASE+2),
  P0_350 = 1u<<(MASK_BASE+3),
  P0_699 = 1u<<(MASK_BASE+4)
};
enum SPREAD{
  S0_043 = 0u<<(MASK_BASE-3),
  S0_087 = 1u<<(MASK_BASE-3),
  S0_175 = 2u<<(MASK_BASE-3),
  S0_350 = 3u<<(MASK_BASE-3),
  S0_699 = 4u<<(MASK_BASE-3)
};
//OR:
#define PULSE_43ms  ((1u<<(MASK_BASE+0))&(pulse+(0u<<(MASK_BASE-3))))
#define PULSE_87ms  ((1u<<(MASK_BASE+1))&(pulse+(1u<<(MASK_BASE-3))))
#define PULSE_175ms ((1u<<(MASK_BASE+2))&(pulse+(2u<<(MASK_BASE-3))))
#define PULSE_350ms ((1u<<(MASK_BASE+3))&(pulse+(3u<<(MASK_BASE-3))))
#define PULSE_700ms ((1u<<(MASK_BASE+4))&(pulse+(4u<<(MASK_BASE-3))))


//TODO: 2. Set up any periodic ISR even with self wrap-around timer over power of 2 modulo
//3. ISR
void SomePeriodISR(void)
{
    pulse++;
}

//4. Polling for required flag
unsigned int edge;
client(){
  if(P0_175&(pulse+S0_175)){
    //5. pulse consumer
  }
  if(PULSE_350ms){
    //another variant
  }
  if(P0_043&(pulse^edge)){
    edge = pulse;
    //6. edge consumer
  }
}

[flo0319]

Maybe the easy way for your custom clock is to build a SystemTick ( a simple uint16) incremented in ISR (how Signal indicate) where also need to be update the Timer1 with a preload value to generate 1ms period (or another resolution which you need for it). This tick can be used with a TimeOut() procedure to make a call or whatever you want.

If you thing that are too many interrupts per second, I can tell you that are only 1000 in a second at a period of 1ms doing 1 word addition  and 1 word assign (maybe 20-30 instruction cycles ~= 2us with all calls and save context ). This will represent 0.2% load and is an uniform way to trigger any custom clock in your application.

[kreutz]

You might want to take a look at the following page: http://www.leapsecond.com/pic/picdiv.htm   at the end of the page they have download links for the assembly language source code.

[younder]

Interesting code, i do not use CCS to verify it. Some comments / questions:
- why counters[] array initialized with {3,1,1,1,1} not all 1?
- does the pulse var really need to be in structure ?
- pulse var not as volatile ?

-ichan

The initialization is basically to get the final edges in different scans... and not overload the particular scan in where you are using the edge pulse to call a function or do anything else...the way the modified code is wouldn't be very different on performance in relation to the initial one. If's are efficient, but it'd be much easier to change times, and keep track of what is meant to happen 'when', with the table.

- how is the use the "pulse" generated, scanning it on the main loop?

-ichan

Yes...scanning it on main loop to perform different tasks which can be scanning at this rate as example below... See complete code on Reply #3...

Code:

void Main() { 

Unsigned int16 Temp0=0; 
int1 Temp1=0;

  Initialization();
   while(1){ 

  Function_Edge_50ms();  //Generate edge pulses
  Function_Edge_100ms(); 
  Function_Edge_200ms(); 
  Function_Edge_500ms();
  Function_Edge_1000ms(); 

  if (Edge_50ms) read_AD(0); //Read AD0 every 50ms
  
  if (Edge_500ms) Temp0++; // increment temp0 by 1 every 1s ....
  if (Pulse_500ms) Temp1=1; else Temp1=0; // 500ms blinking bool .... Temp1 could also be an output pin
  
  if (Edge_100ms) printf(LCD_PUTC,"%4ld %1lu",Temp0,Temp1); //display every 100ms thru 100ms edge flag
         } 
} 

//------------------ EOF --------------------- 

Posted on: February 24, 2015, 01:12:04 01:12 - Automerged

Maybe the easy way for your custom clock is to build a SystemTick ( a simple uint16) incremented in ISR (how Signal indicate) where also need to be update the Timer1 with a preload value to generate 1ms period (or another resolution which you need for it). This tick can be used with a TimeOut() procedure to make a call or whatever you want.

If you thing that are too many interrupts per second, I can tell you that are only 1000 in a second at a period of 1ms doing 1 word addition  and 1 word assign (maybe 20-30 instruction cycles ~= 2us with all calls and save context ). This will represent 0.2% load and is an uniform way to trigger any custom clock in your application.

I guess what you are saying is what the code on post#3 is already doing, isn't? the difference is in the mentioned period (1ms)... there you will see a 10ms period used to generate the SystemTick...

Posted on: February 24, 2015, 01:17:30 01:17 - Automerged

1) Intended use of this "tool" is too inconcrete (undefined) for me. And at the same time too restrictive for client: only set of predefined periods.

see if with the example above you get what I intend to do with the pulses and edges....

Posted on: February 24, 2015, 01:20:47 01:20 - Automerged

6) I do not see why predefined set (50 100 200 500 1000) of ms is better than (50 100 200 400 800) or (43.7 87 175 350 699) ms.

The statment below in the post #12:

"Finnaly, switch to 37500 counts from the current 15000, and change the count values to 2,4,8,20,40. This would take the CPU time down to under 0.5* straight away...." means:

48,000,000 as the oscillator (Tosc) rate.
Divide by 4 to get the timer increment rate (Tclk), so 12,000,000 Hz.
Divide by 8 since the divide by 8 pre-scaler was set, so 1,500,000 counts / sec on the timer.
The timer is set to 65,536-37,500 (28,035), so it will overflow after 37,500 counts.
(1/1,500,000)*37,500=0,025s or 25ms.
by changing from set_timer1(50536 + get_timer1()) to set_timer1(28,035 + get_timer1()) it will overflow every 25ms...

Count values 2,4,8,20,40 will give us 2*25ms=50ms, 4*25ms=100ms... etc

Again, this would take the CPU time down to under 0.5* straight away in comparison to the initial posted code (reply #3)

[Ichan]

For non critical timing application i usually code it like below.

Code:

#define TMR1_PERIOD 0x018F    // 100us system tick

static volatile struct clockType {
   unsigned int ticks;
   unsigned int ms001;
   unsigned int ms010;
   unsigned int ms100;
   unsigned int sec01;
} sysclk;

void __attribute__((interrupt, shadow, auto_psv)) _T1Interrupt() {
   IFS0bits.T1IF = 0;
   if(++sysclk.ticks >= 10) {
      sysclk.ticks = 0;
      if(++sysclk.ms001 >= 10) {
         if(++sysclk.ms010 >= 10) {
            if(++sysclk.ms100 >= 10) {
               ++sysclk.sec01;
            }
         }
      }
   }
}

int main (void) {
   for (;;) {
      if(sysclk.ms001 >= 10 ){
         sysclk.ms001 = 0;
         // every 10ms
      }
      if(sysclk.ms010 >=5 ){
         sysclk.ms010 = 0;
         // every 50ms 
      }
      if(sysclk.ms100 >= 5) {
         sysclk.ms100 = 0;
         // every 500ms
      }
      if(sysclk.sec01 >= 3) {
         sysclk.sec01 = 0;
         // every 3s
      }
   }
}

Non critical timing means it is allowed to be affected a little by other background process - the code above actually copy pasted from here.

-ichan