Author Topic: PIC18 TMR2 Random Long Pulse? (Screencaps)  (Read 1862 times)

0 Members and 1 Guest are viewing this topic.

Offline klr5205Topic starter

  • Regular Contributor
  • *
  • Posts: 114
  • Country: us
PIC18 TMR2 Random Long Pulse? (Screencaps)
« on: May 15, 2018, 01:46:25 pm »
Does anybody have any traps for young players that would be likely to cause an intermittent long pulse from TMR2 and/or the non-blocking delay function written below? 

My program is purely interrupt driven except for a hearbeat signal I thought I would add as an afterthought.  I set up TMR2 to generate approximately 1ms system interrupts, and increment an unsigned long 'currentSystemTime' at each tick.  I have a generic delay function:

Code: [Select]
void delayMilliseconds(unsigned long ms) {            // ms: duration
    unsigned long start = currentSystemTime;          // start: timestamp
    for (;;) {
        unsigned long now = currentSystemTime;        // now: timestamp
        unsigned long elapsed = now - start;          // elapsed: duration
        if (elapsed >= ms)                            // comparing durations: OK
            return;
    }
}

And I use it in a heartbeat function.  My ISR decides if it's been 1000 counts since the last heartbeat. If so, it sets a flag which triggers the following code to run:

Code: [Select]
if (statusFlag){
        for(int i=0; i<2; i++){
                STATUS_LED_SetLow();
                delayMilliseconds(100);
                //__delay_ms(100);
                STATUS_LED_SetHigh();
                delayMilliseconds(100);
                //__delay_ms(100);
                }
            statusFlag = 0;
            lastTime = currentSystemTime;

I get several pulses that look perfectly fine, but I get random intervals where a particularly long pulse is followed by a short pulse... the sum of which is still less than what a "good" period should be.  Take a look at the screenshots below to see what I mean:

"Good" Pulse


"Bad" Pulse


Processor is a PIC18F47K40
TMR2 not used for anything else besides 1ms tick.
TMR0 is used in oneshot mode for firing a triac.  (not during screencaps)
EUSART1 is used to received 9600 baud data.    (no data being received during screencaps)

This is pretty vexing behavior to me, although I'm a mostly self-taught, fairly unseasoned mcu programmer.  Any ideas where to look?


 

Offline agehall

  • Frequent Contributor
  • **
  • Posts: 381
  • Country: se
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #1 on: May 15, 2018, 01:55:57 pm »
First of all, I would recommend using specified integer sizes. unsigned long is always dodgy for things like this as one is never certain how big they are.

The most apparent problem I see in your code, is what happens when currentSystemTime wraps around. That could wreck havoc on your system.
 

Offline klr5205Topic starter

  • Regular Contributor
  • *
  • Posts: 114
  • Country: us
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #2 on: May 15, 2018, 02:12:30 pm »
I thought using an unsigned variable here was the more or less accepted way of dealing with the rollover of currentSystemTime  (or whatever variable). 

Copied from a stackexchange post:
Quote
An unsigned long is 0 to 4,294,967,295 (2^32 - 1).
So lets say previousMillis is 4,294,967,290 (5 ms before rollover), and currentMillis is 10 (10ms after rollover). Then currentMillis - previousMillis is actual 16 (not -4,294,967,280) since the result will be calculated as an unsigned long (which can't be negative, so itself will roll around). You can check this simply by:
Serial.println( ( unsigned long ) ( 10 - 4294967290 ) ); // 16
So the above code will work perfectly fine. The trick is to always calculate the time difference, and not compare the two time values

 

Offline NorthGuy

  • Super Contributor
  • ***
  • Posts: 3137
  • Country: ca
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #3 on: May 15, 2018, 02:47:27 pm »
If your "interrupt driven code" uses all of the processor time, the main loop doesn't get any chance to execute. I guess that's what you're observing. Disable all interrupts except TMR2 and see if you get a good pulse.
 

Offline klr5205Topic starter

  • Regular Contributor
  • *
  • Posts: 114
  • Country: us
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #4 on: May 15, 2018, 03:02:41 pm »
First of all, I would recommend using specified integer sizes. unsigned long is always dodgy for things like this as one is never certain how big they are.

The most apparent problem I see in your code, is what happens when currentSystemTime wraps around. That could wreck havoc on your system.

Based on your comment I changed my delay function and all related variables to unsigned int and the problem seems to have gone away... although I have my scope monitoring for a short pulse

For my purposes I think this is completely acceptable... but what if I truly needed an unsigned long?
 

Online Ian.M

  • Super Contributor
  • ***
  • Posts: 12807
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #5 on: May 15, 2018, 03:08:00 pm »
XC8 can be configured to use either 24 bit or 32 bit longs.  For the sake of your sanity use ANSI C99 standard fixed width types from stdint.h.  See: https://en.wikipedia.org/wiki/C_data_types#Fixed-width_integer_types

See XC8's  stdint.h for which C99 types are supported.
« Last Edit: May 15, 2018, 03:13:20 pm by Ian.M »
 

Online mikerj

  • Super Contributor
  • ***
  • Posts: 3233
  • Country: gb
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #6 on: May 15, 2018, 03:25:13 pm »
The basic problem here is that reading and writing a 24/32 bit value on an 8 bit micro is not an atomic operation.  This means that a simple expression such as:

Code: [Select]
unsigned long now = currentSystemTime;

can end up with a very incorrect time value in 'now', since in the process of copying the 3/4 bytes of 'currentSystemTime' the timer interrupt could fire and change the value of currentSystemTime.  The simple fix for this is to disable the relevant interrupt whilst reading or writing multi-byte words in your main loop, which are also accessed by interrupts.  You should also declare the currentSystemTime variable as volatile.

Code: [Select]
PIE4 &= ~TMR2IE; /* Disable timer 2 interrupt whilst accessing 'currentSystemTime' */
unsigned long now = currentSystemTime;
PIE4 |= TMR2IE; /* Re-enable timer 2 interrupt */

The register/bit names for timer 2 interrupt enable in the above code may not be correct for your compiler, it's just an example of what needs to be done.

Note that using a 16 bit int has not fixed your problem, it has reduced the opportunity for it to fail but it will undoubtedly fail at some point.
« Last Edit: May 15, 2018, 03:26:45 pm by mikerj »
 
The following users thanked this post: Ian.M, JPortici

Offline klr5205Topic starter

  • Regular Contributor
  • *
  • Posts: 114
  • Country: us
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #7 on: May 15, 2018, 07:28:53 pm »
I left my scope armed to catch a short pulse all afternoon while I did something else and it didn't catch any "bad" pulses which if it is truly doomed to fail again as it is coded  is a huge improvement vs having a bad pulses every several seconds like before.

However, I do understand and take your point about disabling timer 2 interrupts when I read currentSystemTime and I will do that.

Thanks!
 

Offline cv007

  • Frequent Contributor
  • **
  • Posts: 822
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #8 on: May 15, 2018, 10:29:24 pm »
since you are blocking on the delay, simplify-
Code: [Select]
void delayMilliseconds(uint16_t ms){ // 0 - 65535 ms
  for( ; ms; __delay_ms(1), ms-- );
}
unless there are other reasons for 'currentSystemTime', it could be eliminated and the tmr2 simply has to keep track of when 1 second has elapsed and set a flag (as is currently done), then no atomic access issues

if 'currentSystemTime' is also needed for timestamp reasons, or non-blocking delays, etc.,  I think I would still go the simple route for a blocking ms delay function


also, since you are only reading 'currentSystemTime', an alternate way is to check if the read was atomic
Code: [Select]
uint32_t t;
while( t = currentSystemTime, currentSystemTime != t );
although just clearing/setting the tmr2 irq is easy enough, and probably better
 

Offline agehall

  • Frequent Contributor
  • **
  • Posts: 381
  • Country: se
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #9 on: May 16, 2018, 05:17:22 am »
For my purposes I think this is completely acceptable... but what if I truly needed an unsigned long?

I always try to use uint32_t or whatever type I need. That way I know how big my datatype is by just reading the code which I find much easier than any alternatives. There are cases where you cannot use these types or where it becomes cumbersome and in those cases I try to be very careful about what I do with the data - there is always a chance you overflow the integer by accident if you don't know what you are dealing with...
 
The following users thanked this post: TomS_

Offline klr5205Topic starter

  • Regular Contributor
  • *
  • Posts: 114
  • Country: us
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #10 on: May 16, 2018, 01:24:43 pm »
unless there are other reasons for 'currentSystemTime', it could be eliminated and the tmr2 simply has to keep track of when 1 second has elapsed and set a flag (as is currently done), then no atomic access issues

This is a good point and I'm a little disappointed that I didn't recognize what feels obvious at this point lol.  I don't need (or even use) a timestamp whatsoever so why try so hard to keep track of system time? 

I can just create a milliFlag that gets toggled at each 1ms interrupt and then do this:

Code: [Select]
void delayMilliseconds(int delay){
    int numMillis = 0;
    bool toggle = 0;

    while(numMillis < delay){

      if(toggle != milliFlag){
        numMillis++;
        toggle = milliFlag;
      }
   }
}

This seems simpler and less error prone than messing around large integers that can apparently cause issues due to the compiler (or my lack of understanding of it :-DD)
 

Offline cv007

  • Frequent Contributor
  • **
  • Posts: 822
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #11 on: May 16, 2018, 03:29:16 pm »
Quote
My program is purely interrupt driven
Quote
I don't need (or even use) a timestamp whatsoever
Then you don't even need timer2 to do anything, and you can simply do this in your main-
Code: [Select]
for(;;){
    STATUS_LED_SetLow();
    __delay_ms(800);
    STATUS_LED_SetHigh();
    __delay_ms(200);
}
those delays are simply inlined time killers, and as long as you have _XTAL_FREQ defined (which you most likely already do), they work just as well as a timer without having to use a timer

if you need delays that can change at runtime through variables, then a generic delay_ms function can be created as I showed previously
 

Offline NorthGuy

  • Super Contributor
  • ***
  • Posts: 3137
  • Country: ca
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #12 on: May 16, 2018, 03:37:49 pm »
those delays are simply inlined time killers, and as long as you have _XTAL_FREQ defined (which you most likely already do), they work just as well as a timer without having to use a timer

Not exactly as well. All the time spent in interrupts while you're throttling is added to the delay, so if there's a heavy interrupt load your delay may be much longer than intended. This is not a problem with the timer, because the timer keeps going while the CPU processes the ISRs.
 

Offline cv007

  • Frequent Contributor
  • **
  • Posts: 822
Re: PIC18 TMR2 Random Long Pulse? (Screencaps)
« Reply #13 on: May 16, 2018, 04:06:54 pm »
Quote
This is not a problem with the timer, because the timer keeps going while the CPU processes the ISRs
True, but with the latest code shown the delay function would would never know anyway as its looking for a flag (the timer could be toggling the flag multiple times- the delay function would never know). I would assume since this is a 'heartbeat' indicator, the actual delay time precision is not important, and the added benefit of not using a precise timer method is you could see the effects of an isr that takes too much time for some reason (although I would have a hard time believing the mcu could be that busy given the info provided- tmr0 doing a one-shot, uart doing 9600 baud).

There may be better ways than just blinking the led at a 'constant' interval to indicate activity, which only indicates that a main loop is running- maybe it would be nicer to show tmr0 and/or uart activity. Just a thought.
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf