Troubles enabling TPM output compare - MC13213

[DSweet]

Hi all,

I've just started using an MC13213 mcu and am struggling setting up interrupts. I am trying to set up timer 0 channel 0 as an output compare to generate an interrupt to be used for a simple delay. I feel like I've set up all of the control registers properly, but once the program is running in the debugger (hiwave w/ P&E micro USB), the free running counter does not appear to be counting which in turn means an interrupt is never generated. Here is my initialization code:

    TPM1SC_CPWMS = 0; //set pwm for output compare
    TPM1SC_CLKSA = 0; //A:B -> 0:1 == System clock
    TPM1SC_CLKSB = 1;
    TPM1C0SC_MS0A  = 1;   //MS0A:MS0B -> 1:0 == outputcompare
    TPM1C0SC_MS0B  = 0;
    TPM1C0SC_ELS0A = 0;     //ELS0A:ELS0B -> 0:0 == software compare
    TPM1C0SC_ELS0B = 0;
   
    TPM1C0SC_CH0IE = 1;
    TPM1C0SC_CH0F = 0;
   
    TPM1C0V=TPM1CNT+E_PER_MS;   //set value register for compare

E_PER_MS is equal to the number of clock cycles per ms, so this interrupts once per ms.
Here is my interrupt code:

interrupt 5 void OC0Isr(){
    TPM1C0SC_CH0F;                //Read Flag
    TPM1C0SC_CH0F = 0;            //Clear Flag
    TPM1C0V=TPM1CNT+E_PER_MS;
    OCmSCnt++;                         /* Increment counter    */
}

My Delay function (delays a passed number of ms):

void OCDelay(INT16U ms){
    INT16U start_cnt;
    start_cnt = OCmSCnt;
    while((OCmSCnt - start_cnt) < ms){}  /* wait for ms to pass    */
}

My main function (just to blink an led every .5 seconds:

void main(void) {

 PTCDD = 0x10; // initialize PTC4 as output
 PTCD = 0;  // initialize PTC to 0
 OCDlyInit();
 ENABLE_INT();

for(; {
    OCDelay(500);
    PTCD_PTCD4 = ~PTCD_PTCD4;  // invert the output
  } /* loop forever */

  /* please make sure that you never leave main */
}

Any ideas on what is going on here? I am using the default start08.c startup code, except I disabled the COP timer. Thanks for any suggestions,

Dan

Added p/n to subject.

Message Edited by NLFSJ on 2008-03-18 09:30 AM

[bigmac]

Hello Dan,

Here are some additional coding tips -

When initialising the TPM, it is a good idea to simultaneously write all bits to the TPM1SC and TPM1C0SC registers, rather than individual bits.  In addition to being more efficient, it may avoid some potential side effects, and ensures that all bits are explicitly initialised.  While not applicable to the TPM, some registers do contain write once bits.  Here, it is mandatory to write all bits simultaneously.

When clearing TPM flags, the following is quite sufficient, since it corresponds to a read-modify-write sequence, as required by the TPM module.  Also continue to use this within the TPM initialisation, to clear any spurious interrupt.

TPM1C0SC_CH0F = 0;

Since you are polling the variable OCmSCnt to determine when timeout has occurred, it may result in simpler code if the counter is set when the delay is commenced, and decremented during the ISR.  It may not be important in this instance, but the affect of accumulated interrupt latencies can be reduced by calculating the next OC value from the previous OC value, rather than from the current channel count.

Whilst waiting for timeout to complete, is is a good idea to execute the __RESET_WATCHDOG() macro.  Then it does not matter whether the watchdog timer is enabled, or not.

Some of these ideas are demonstrated in the following code -

/* Global variable */

INT16U TPMBuf;

interrupt 5 void OC0Isr(void)

{
   TPM1C0SC_CH0F = 0;    // Clear Flag

   TPMBuf += E_PER_MS;
   TPM1C0V = TPMBuf;     // Next OC setting

   if (OCmSCnt)          // Test whether already timeoout

      OCmSCnt--;         // Decrement counter
}

void OCDelay(INT16U ms)

{
   OCmSCnt = ms;

   while (OCmSCnt)        // wait for ms to pass

      __RESET_WATCHDOG();
}

Regards,

Mac

[Denn]

Hi Dan,

Some suggestions:

As pointed by Mac, that whenever initializing a register it is much better to initialise, as a whole register (whenever possible) rather than individual bits.It creates efficient code both interms of code size and execution speed. Ultimately initialising individual bits will be replaced by assembly codes equalent to bit set/bit clear instructions on that particular register(around 5 bus cycles)  and it takes roughly the same amount of bus clk cycles as initialising the entire register LDA IMM(2 cycles) and STA Direct Ad,Extended Ad (3/4 cycles). And hence one can efficiently use program memory and reduce execution time of the code considerably.

And its a better, safe practice to read Interrupt event flag CHOF when it is set and then clear the flag in the ISR as menitioned in the HCS08 datasheet. But it also works without the initial dummy read of this flag/corresponding register!!!

Regards,

Denn

[bigmac]

Hello,

---

Denn*** wrote:

And its a better, safe practice to read Interrupt event flag CHOF when it is set and then clear the flag in the ISR as menitioned in the HCS08 datasheet. But it also works without the initial dummy read of this flag/corresponding register!!!

---

I think this point needs some clarification -

To clear a flag bit such as CH0F requires that the register be first read, and then a zero written to that bit.  If the flag is set, it will then be cleared.  Obvioulsy, nothing will happen if the bit is already clear.

Now consider the action of the statement to clear a single bit -

TPM1C0SC_CH0F = 0;

When this is executed, the value within TPM1C0 will first be read.  Then the bit corresponding with CH0F will be set to zero, and the whole byte then written back to the TPM1C0 register.  This what is meant by a "read-modify-write" sequence, and is precisely the action required to clear the flag bit.

In practice, the compiler may utilize a single BCLR instruction, which obeys the sequence described above.

The dummy read is therfore never necessary (but should do no harm).

Regards,

Mac

Message Edited by bigmac on 2008-03-18 10:45 PM

[Denn]

Hello Mac,

bigmac wrote:

Now consider the action of the statement to clear a single bit -

TPM1C0SC_CH0F = 0;

When this is executed, the value within TPM1C0 will first be read.  Then the bit corresponding with CH0F will be set to zero, and the whole byte then written back to the TPM1C0 register.  This what is meant by a "read-modify-write" sequence, and is precisely the action required to clear the flag bit.

In practice, the compiler may utilize a single BCLR instruction, which obeys the sequence described above.

The dummy read is therfore never necessary (but should do no harm).

I think the "read-modify-write" sequence what you are referring to is the one that is( I'm not sure about this ) happening within the controller.And if BCLR instruction obeys the sequence described above why should Freescale HCS08 datasheet menition about the initial dummy read of "event flag register" TPMxCnSC  and then the bit clear of event flag CHnF.

Its clearly mentioned in datasheet "Clear CHnF by reading TPMxCnSC while CHnF is set and then writing a 0 to CHnF".

When its written that way , why take a risk and give a chance for a bug ?

Regards,

Denn

[allawtterb]

---

Denn*** wrote:

I think the "read-modify-write" sequence what you are referring to is the one that is( I'm not sure about this ) happening within the controller.And if BCLR instruction obeys the sequence described above why should Freescale HCS08 datasheet menition about the initial dummy read of "event flag register" TPMxCnSC  and then the bit clear of event flag CHnF.

They mention that it must be read first because not every write to the register will create a read first.  Writing a whole byte to the register will not read the register first and if the bit is cleared this way a dummy read is needed before this. 

[Denn]

Hello allwtterb,

Sorry, I don't know to pronounce your alias name.Fine!!!

I think you really didn't get my question to Mac.

Mac told "In practice, the compiler may utilize a single BCLR instruction, which obeys the sequence described above."

The sequence which he explained is the internal read within control logic of MCU(not visible for the user. HARDWIRED) of the status-control reg TPMxCnSC ,modify that particular bit and store that value in particular location.And he says BCLR internally follows this sequence and hence there is no necessity of a dummy read of TPMxCnSC.

In short he feels only TPM1C0SC_CH0F = 0;
is sufficient and the dummy read before bit clearing is not really required.

My question was "If that was so why did FSL state for a dummy read before BCLR (TPM1C0SC_CH0F = 0"

Aren't we giving a chance for a bug ? Even I've observed just bitclearing is sufficient to clear event flag.But sometime later who knows event flag may not be cleared by just BCLR.How can one always ensure it always works in our favor.

Safe practice is to follow what data sheet says rather than to experiment to save maybe 1 byte (possibly just LDA ) in program memory.

Hope you understood my intention.

Regards,

Denn

[JimDon]

Denn,

Often those who have not had experience working on large projects with a long life do not understand the concept of writing code that will last for many year over generations of technology, both hardware and software. Nor do they understand that at some time in the future the code will be changed and that tweaking the code for utmost efficiency (when it is not required) is many times at odds with clarity and robustness.

For example writing:
   TPM1SC_CPWMS = 0; //set pwm for output compare
    TPM1SC_CLKSA = 0; //A:B -> 0:1 == System clock
    TPM1SC_CLKSB = 1;
    TPM1C0SC_MS0A  = 1;   //MS0A:MS0B -> 1:0 == outputcompare
    TPM1C0SC_MS0B  = 0;
    TPM1C0SC_ELS0A = 0;     //ELS0A:ELS0B -> 0:0 == software compare
    TPM1C0SC_ELS0B = 0;
   
    TPM1C0SC_CH0IE = 1;
    TPM1C0SC_CH0F = 0;

Instead of:

  TMP1SC = 0xXX  // what ever XX works out to be

The first method is much easier for someone (including yourself) to understand and modify at a later time.
In most cases, there is no shortage of  flash, the code only executes once, and the clarity is much more valuable than the few bytes of flash it may save.

The same is true of your point - while that code may be OK for now, it may not always be so. It is better to err on the side of not having a bug later on that would be difficult to find.

This is programming in the small vs programming in the large. This what "Software Engineering" is vs "Programming".

While there are times when clarity must be sacrificed for speed and size, an Engineer clearly documents this and make it clear the optimizations taken and why.

It also has to do with being a one trick pony vs having to deal with many different machines and contexts over a span of time. I know I will not remember the details later on, and I personally value clarity and ease of maintenance.

Now, having said this, BOTH points of view have validity and relevance, and going to extremes either way is not good either, striking a balance is best. It is important that we all understand the two sides, and not preclude the requirements of either sides. There is always a pro and con to every method, no one way is 100% correct.




Message Edited by JimDon on 2008-03-18 11:15 AM

[Denn]

Hello Jim,

First of all let me tell you I like the way you speak.

Instead of writing the code for clarity like this

   TPM1SC_CPWMS = 0; //set pwm for output compare
    TPM1SC_CLKSA = 0; //A:B -> 0:1 == System clock
    TPM1SC_CLKSB = 1;
    TPM1C0SC_MS0A  = 1;   //MS0A:MS0B -> 1:0 == outputcompare
    TPM1C0SC_MS0B  = 0;
    TPM1C0SC_ELS0A = 0;     //ELS0A:ELS0B -> 0:0 == software compare
    TPM1C0SC_ELS0B = 0;
   
    TPM1C0SC_CH0IE = 1;
    TPM1C0SC_CH0F = 0;

one can go to the type we follow(just an example nothing to do with above code),which generates less code & executes in less time.Also more clear than the above one.

Code:

      TPM2SC = 0x0B;//  0b00001011              //                 ||||||||                                                     //                 |||||||*---  PS0  : Timer Prescaler 0             //                 ||||||*----  PS1  : Timer Prescaler 1                  //                 |||||*-----  PS2  : Timer Prescaler 2                 //                 ||||*------  CLKS3 :Clock Source Select               //                 |||*-------  CLKS4 : Clock Source Select      //                 ||*--------  CPWMS: Center-Aligned PWM Select                     //                 |*---------  TOIE : Timer  Overflow Interrupt Enable          //                 *----------  TOF  : Timer  Overflow Flag    

 
Again in mass production kind of projects (productions in millions.I really mean it) say automotive or telcom, customers normally prefer cheaper (in price)  MCUs. Each cent they save will make a huge difference at the end .Here cost reduction will be the mantra.They want to eliminate all those things that are unnecessary.As you know the cheaper MCUs come with slight pinch like less featureslike say 32k flash MCUs will always be less costlier than say 60k variant of the same MCU.In a such case one has to code efficiently as much as possible. I would say better code is the one which executes in less time takes less memory.Also there could be some contraints from customers saying there should be atleast 10% free space in flash say for future bug fix before release into the market from the time of delivery of code.

In these scenarios one cannot lavishly spend program memory. Try to reduce code,Speedy execution will be the goal.

 "Software Engineering principles" like documentation/related quality process will always be followed in large scale production.These days one can even find in the ordinary medium scale productions unless product is produced in lesser quntities.

Anyway my concern had nothing to do with the above sample code.I understand Dan had written a sample pgm to test interrupt.It was with the clearing process of event flagCHnF inside the ISR.Was dummy read really reqd to clear event flag(also in this case ISR)???

Regards,

Denn

[JimDon]

Denn,

 I understand you point, and as I said the context of the project is very important to consider.

Automotive is interesting, as although the volume is in millions, the cost of errors is extremely high, not just in recalls and rework but also in market share and liability. Also, time to market is very important. You can't pull off a 6 month slip on your 2008 model.

They were of the opinion that rather than changing tested lib code to cut some bytes, better to buy the bigger part. Also, reuse of tested code is highly valued, which means robustness an maintainability. Whats more it turned out that buying 4 million 8k parts, instead of 1 million 1k, 1 million 2K etc. cost less in the long run, and made stock logistics and manufacturing easier.

On the other hand, a bedside alarm clock that fails to go off on February 29th, while not desirable, is no big deal.  Chances are the user may not even notice. And if you are going to make 50 million of them, it makes sense to spend a considerable amount of time squeezing the code down to the cheapest part. If it saves 5 cents per unit to not go off on on February 29th, that's 500K of savings. I'm not saying that would 100% be the right choice, but it might be.

So, my point is you can't just make sweeping rules that cover all situations, and quite often "advise" is biased one way or the other.

As for reading the register, I think that was resolved a few posts ago :--)...

[Denn]

Hi Jim,

---

JimDon wrote:

On the other hand, a bedside alarm clock that fails to go off on February 29th, while not desirable, is no big deal.  Chances are the user may not even notice. And if you are going to make 50 million of them, it makes sense to spend a considerable amount of time squeezing the code down to the cheapest part. If it saves 5 cents per unit to not go off on on February 29th, that's 500K of savings. I'm not saying that would 100% be the right choice, but it might be.

---

I really don't understand how byte initialization instead of bitwise initialization creates a bug like what you have mentioned. I would like to have an answer from you!!!  If there was a bug in the product it was due to an wrong testing strategy.You can't put the blame on single byte initialization.

Comparing my example code of byte initialization with the bitwise initialization.Which offers more readability ? And about "maintability and robustness".

General Defn of Robustness

An algorithm in computer science is robust if it continues to operate despite abnormalities in input, calculations, etc.

 If you think robustness of the SW gets affected by Byte initilization method then you are wrong. Atleast it scores over the bitwise initialization in terms speed of execution.

Maintainability:

Maybe bitwise method will give some freedom for future "tweakers" to modify the bits to accomodate to the future modifications.But it can easily make the programmer to commit mistakes just like what Dan has done in her code.She initialised the bits in reverse order. Trust me this mistake wouldn't have happened if she had done in Bytewise initialization. I feel "BITWISE"initialization would be suitable for lazy programmers who are scared of reading datasheets

JimDon wrote:

So, my point is you can't just make sweeping rules that cover all situations, and quite often "advise" is biased one way or the other.

---

I still don't think my advice was biased and can only be applied to a project with less memory.

Its good practice that's what I say.Its not a RULE, its a suggestion.One can take it or leave it !!!

For Young programmers:

An efficient code is the one which takes less memory and less time to execute.It takes character to code so.Maintainability and readability of code is done by proper comments and has got nothing to do with BITWISE INITIALIZATION. It only creates confusion and clutter and hence affects readability.

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

And my question still remains unanswered.

Which ISR is right.

Dan's ISR

interrupt 5 void OC0Isr(){
    TPM1C0SC_CH0F;                //Read Flag   ----- DUMMY READ
    TPM1C0SC_CH0F = 0;            //Clear Flag------ BCLR
// Do something

}
 or Mac's ISR

interrupt 5 void OC0Isr(void)

{
   TPM1C0SC_CH0F = 0;    // Clear Flag------ just BCLR

// Do something
}

Why would FSL ask for a dummy read if BCLR was sufficient as justified by Mac and Allawtterb. (Also Refer section Clearing Timer Interrupt Flags)

There they mention about a  2 step procedure. 1)==> Read then 2)==>write 0 to flagbit(BCLR used by compiler)

If BCLR was sufficient, then why would they ask for 2 step procedure in clearing the flag. Is it for the reliable operation !!??

Thanks & Regards,

Denn.

[allawtterb]

Denn*** wrote:

There they mention about a  2 step procedure. 1)==> Read then 2)==>write 0 to flagbit(BCLR used by compiler)

The following lines are from the HCS08 Reference Manual(Page 100):

"Some status bits are cleared by a sequence involving a read of the status bit followed by a write to another register in the peripheral module. Some users are surprised to find that a BSET or BCLR instruction has satisfied the requirement to read the status register."

The BSET command is explained on page 236 and BCLR on page 209, both mention that they read the 8-bit value before writing it back.  Also, if you look at the access details for each of these instructions you will see the details for the 5 cycles it uses.  They are rfwpp.  Indicating  a read, a free cycle, a write and then 2 program fetches. 

---

Denn*** wrote:

Why would FSL ask for a dummy read if BCLR was sufficient as justified by Mac and Allawtterb. (Also Refer section Clearing Timer Interrupt Flags)

Because there are cases where a dummy read is necessary:

Code:

interrupt 5 void OC0Isr(){    TPM1C0SC = 0x48;    // Clears the CH0F flag, but doesn't read first!      // Do something}

 
This will not properly clear the flag, even though the CH0F bit has a 0 written to it the register must first be read and a dummy read here is required.   

 - Brett

Edit - Fixed font sizes

Message Edited by allawtterb on 2008-03-19 09:00 AM

[Denn]

Hello Bret,Mac,Jim,

I am now fully convinced that dummy read before BCLR is not at all required as it is redundant.BCLR itself satisfy the required condition for clearing the event flag.

Thanks for pointing out that on the HCS08 Family Reference Manual, Rev. 2 . Somehow I missed going through the family manual.

Regards,

Denn.

[JimDon]

Denn,
There was absolutely nothing wrong with your code.
It was fine. I did not mean to imply it in anyway created a bug.
It is still not how I would do it, but non the less it does not make it wrong. It is just a matter of personnel style.

I was pointing out cost vs functionality trade offs, not commenting on your code.

As far as your other comments, concerning the dummy read, they are pointing out that BCLR does a read
and there fore satisfies the specification. A "real" read is as good as a "dummy" read.

Both methods will work fine, but the dummy read is not required because the BCLR does a read (and a write, 2 steps)
in the process of clearing the bit. According to your own "rules"

"An efficient code is the one which takes less memory and less time to execute."

Macs is the most correct..

[Denn]

Hello Friends,

I wish everyone a HAPPY EASTER in advance.

I'll be out of state and will be back only after few days.Until then your replies to my posts will not be accessible to me.I'll surely miss them.

Thanks for the support.

Regards,

Denn.

[allawtterb]

---

Denn*** wrote:

Aren't we giving a chance for a bug ? Even I've observed just bitclearing is sufficient to clear event flag.But sometime later who knows event flag may not be cleared by just BCLR.How can one always ensure it always works in our favor.

Safe practice is to follow what data sheet says rather than to experiment to save maybe 1 byte (possibly just LDA ) in program memory.

I understand the argument for explicitly reading the register.  It does provide the benefit of being explict that the register must be read before the bit can be written to.  It could be a problem for someone else who edits the code and doesn't know the opeation of changine a single bit, unless this is provided in a comment.

I don't think the chances for a bug are very high, since this is a documented "feature" of the BCLR instruction I can't imagine they would intentionally change it's operation.  I think it can be considered safe to assume that the operation of this instruction will not change. 

[Denn]

Hello,

So you think dummy read is not necessary.

 If then why would FSL datasheet mention about dummy read as the first step in clearing event flag.

And I never mentioned that chances of bug are high.I just said it could be a reason for a BUG!!! and why take a chance.

Regards,

Denn

[allawtterb]

---

Denn*** wrote:

So you think dummy read is not necessary.

No I did not say this, I don't have a problem with someone doing it either way.  When I have a flag that reqiures a reading before writing to clear it I have always explicitly read it for the reasons I stated above, being clear about the operations needed to clear the flag.

---

Denn*** wrote:

 If then why would FSL datasheet mention about dummy read as the first step in clearing event flag.

---

This is what my first response was about, writing to a single bit isn't the only way to clear the flag.  You can write to the whole byte.  If you do it this way a dummy read is always needed to clear the flag properly.

My point in my last post is that I don't think the BSET and BCLR instructions will be changed so that they will not read a RAM location first before writing. 

[DSweet]

Thanks!

I can't believe I overlooked this. I read the documentation and knew I wanted the bus clock selected, and thought I had put this in my code, but obviously I made a mistake. Btw, I was verifying that the free running counter was not working by looking at the memory window (address 0x30) and by opening the registers definition window and looking at the TPM 1 ch0 register counter value. Previously both of these just sat at zero, and now they count as they should. Thanks for your help.

Dan

[allawtterb]

---

DSweet wrote:
the free running counter does not appear to be counting

---

How have you verified this? Have you checked the RAM location that the counter value resides at?  As Denn has mentioned, I think you would be better off using the bus clock.  I don't know what clock modes you are running in but the XCLK you are trying to use (fixed system clock) for your source clock should not be used in either FEI or SCM.

[Denn]

Hi Dan,

I think  if you are using  bus clk as a source for TPM module then there should be an minor change in the code .TPM1SC_CLKSA = 1;   TPM1SC_CLKSB = 0;

Simple way to see whether you are entering ISR or not is to blink an LED( You may not notice it blinking because its hapening at a faster rate!!!! it seems like its ON always)/ Increment an variable from ISR and single step the code.You can see incrementing variable in TRUE TIME SIMULATOR .

Regards,

Denn

Message Edited by Denn*** on 2008-03-17 01:01 PM