Forums

How best to "detect a min pulse width" using C18 + 18F1330 pic

Started by Dennis September 25, 2010
I'm still learning C, and am having some problems with the code to detect a 
pulse over a specific width.


My method is:

The pulse is negative going, normal state is high:
_____       ________
     \_____/


Pulse duration time is measured by a register 'Break_Time' which is 
incremented in the ISR each time TIMER1 overflows.

I'm using interrupt INT0 to sense the start and finishing edges of the 
pulse.


1)Sense pulse using INT0

2)On interrupt:

        3) If INTEDG0 was set for falling edge start timer to measure pulse 
duration

        4) if INTEDG0 was set for rising edge then stop timer & test 
'Break_Time'
            to see if the pulse length was long enough.

        5) If pulse is too short then the 'gWatchDog' register is loaded 
with a value that             forces a reset in the main program loop.


 The code I am using is below. Apologies for the poor formatting, I'm still 
trying to get that right as well. Debugging seems to show that the reset { 
gWatchDog = 125; } line is executing after the end of the pulse is detected.

I've tried a few different approached to no avail. Any suggestions on what 
I'm doing wrong here or the tidiest way to do this task?

Thanks
D.


//______________________________________________________
//
//    I N T 0      I S R
//______________________________________________________


 if ( INTCONbits.INT0IF == 1 )  // is interrupt INT0?
    {
    INTCONbits.INT0IF = 0; // yes - so clear INT0 flag




        if ( INTCON2bits.INTEDG0 == 0 ) // Falling Edge / Start of Break 
Detected
        {
        gBreakLength = 0;
        PIE1bits.TMR1IE = 1;         // Enable TMR1 interrupt
        T1CONbits.TMR1ON = 1;        // Start it counting
        PIR1bits.TMR1IF = 0;
        INTCON2bits.INTEDG0 = 1;     // Set INT0 for rising edge, look for 
end of break
        gWatchDog = 0;               // kick dog
        INTCONbits.INT0IF = 0;       // clear INT0 flag
        } // end if





        if ( INTCON2bits.INTEDG0 == 1 ) // Rising Edge / End of Break 
Detected
            {

            if ( gBreakLength >= 10 )   // if pulse>10  open comms otherwise 
reset
            {
            INT0_DISABLED();            // disable interrupt INT0
            gCommsBufferIndex = 0;      // initialise buffer index
            UART_Init();                // Set up UART
            INTCONbits.INT0IE = 0;      // disable INT0 (now in serial mode)
            } //end if

//### PROBLEM HERE ###

        else                    // Force reset. Break <10 so force a reset
        { gWatchDog = 125; }    //load overflow value into watchdog register


    } //end of INT0




On Sat, 25 Sep 2010 20:44:57 +0800, "Dennis" <invalid@nowhere.com>
wrote:

> >I'm still learning C, and am having some problems with the code to detect a >pulse over a specific width. > > >My method is: > >The pulse is negative going, normal state is high: >_____ ________ > \_____/ > > >Pulse duration time is measured by a register 'Break_Time' which is >incremented in the ISR each time TIMER1 overflows. > >I'm using interrupt INT0 to sense the start and finishing edges of the >pulse. > > >1)Sense pulse using INT0 > >2)On interrupt: > > 3) If INTEDG0 was set for falling edge start timer to measure pulse >duration > > 4) if INTEDG0 was set for rising edge then stop timer & test >'Break_Time' > to see if the pulse length was long enough. > > 5) If pulse is too short then the 'gWatchDog' register is loaded >with a value that forces a reset in the main program loop. > > > The code I am using is below. Apologies for the poor formatting, I'm still >trying to get that right as well. Debugging seems to show that the reset { >gWatchDog = 125; } line is executing after the end of the pulse is detected. > >I've tried a few different approached to no avail. Any suggestions on what >I'm doing wrong here or the tidiest way to do this task?
I see where you initialize gBreakLength to zero on a falling edge but not where you calculate a new value for it before testing for >= 10. Once that's fixed, there is also a small window of vulnerability if the timer rolls over and the rising edge hits before it passes 10 again. It may be enough to just observe a roll-over and set a flag, such that you handle UART_Init() and the rest when there is a rising edge that's >= 10 *or* is after a roll-over event. Or, if the timer rolls over that may be considered "too long" and handled differently. One way or another, though, it should be addressed. Also, strongly recommend that you get in the habit of *never* using literal "magic numbers" (e.g., to test against gBreakLength or to set gWatchDog). Abstract those out to a header file and do something like #define MIN_BREAK_LENGTH 10 #define WDOG_EVENT 125 -- Rich Webb Norfolk, VA
"Rich Webb" <bbew.ar@mapson.nozirev.ten> wrote in message 
news:cr0s96pb275hapgd3ci2q2sf4lluhpe9sl@4ax.com...
> On Sat, 25 Sep 2010 20:44:57 +0800, "Dennis" <invalid@nowhere.com> > wrote: > >> >>I'm still learning C, and am having some problems with the code to detect >>a >>pulse over a specific width. >> >> >>My method is: >> >>The pulse is negative going, normal state is high: >>_____ ________ >> \_____/ >> >> >>Pulse duration time is measured by a register 'Break_Time' which is >>incremented in the ISR each time TIMER1 overflows. >> >>I'm using interrupt INT0 to sense the start and finishing edges of the >>pulse. >> >> >>1)Sense pulse using INT0 >> >>2)On interrupt: >> >> 3) If INTEDG0 was set for falling edge start timer to measure >> pulse >>duration >> >> 4) if INTEDG0 was set for rising edge then stop timer & test >>'Break_Time' >> to see if the pulse length was long enough. >> >> 5) If pulse is too short then the 'gWatchDog' register is loaded >>with a value that forces a reset in the main program loop. >> >> >> The code I am using is below. Apologies for the poor formatting, I'm >> still >>trying to get that right as well. Debugging seems to show that the reset { >>gWatchDog = 125; } line is executing after the end of the pulse is >>detected. >> >>I've tried a few different approached to no avail. Any suggestions on what >>I'm doing wrong here or the tidiest way to do this task? > > I see where you initialize gBreakLength to zero on a falling edge but > not where you calculate a new value for it before testing for >= 10. >
The gBreakLength is incremented in the TIMER1 ISR, when the timer rolls over. This bit works ok.
> Once that's fixed, there is also a small window of vulnerability if the > timer rolls over and the rising edge hits before it passes 10 again. It > may be enough to just observe a roll-over and set a flag, such that you > handle UART_Init() and the rest when there is a rising edge that's >= 10 > *or* is after a roll-over event. Or, if the timer rolls over that may be > considered "too long" and handled differently. One way or another, > though, it should be addressed. >
Ok, I'll look at those points.
> Also, strongly recommend that you get in the habit of *never* using > literal "magic numbers" (e.g., to test against gBreakLength or to set > gWatchDog). Abstract those out to a header file and do something like > #define MIN_BREAK_LENGTH 10 > #define WDOG_EVENT 125
I will change the code to reflect your suggestion, which looks like it would make maintenance ?& changes easier as well. Thanks for your advice Rich.
> > -- > Rich Webb Norfolk, VA
"Rich Webb" <bbew.ar@mapson.nozirev.ten> wrote in message 
news:cr0s96pb275hapgd3ci2q2sf4lluhpe9sl@4ax.com...
> On Sat, 25 Sep 2010 20:44:57 +0800, "Dennis" <invalid@nowhere.com> > wrote: > >> >>I'm still learning C, and am having some problems with the code to detect >>a >>pulse over a specific width. >> >> >>My method is: >> >>The pulse is negative going, normal state is high: >>_____ ________ >> \_____/ >> >> >>Pulse duration time is measured by a register 'Break_Time' which is >>incremented in the ISR each time TIMER1 overflows. >> >>I'm using interrupt INT0 to sense the start and finishing edges of the >>pulse. >> >> >>1)Sense pulse using INT0 >> >>2)On interrupt: >> >> 3) If INTEDG0 was set for falling edge start timer to measure >> pulse >>duration >> >> 4) if INTEDG0 was set for rising edge then stop timer & test >>'Break_Time' >> to see if the pulse length was long enough. >> >> 5) If pulse is too short then the 'gWatchDog' register is loaded >>with a value that forces a reset in the main program loop. >> >> >> The code I am using is below. Apologies for the poor formatting, I'm >> still >>trying to get that right as well. Debugging seems to show that the reset { >>gWatchDog = 125; } line is executing after the end of the pulse is >>detected. >> >>I've tried a few different approached to no avail. Any suggestions on what >>I'm doing wrong here or the tidiest way to do this task? > > I see where you initialize gBreakLength to zero on a falling edge but > not where you calculate a new value for it before testing for >= 10. > > Once that's fixed, there is also a small window of vulnerability if the > timer rolls over and the rising edge hits before it passes 10 again. It > may be enough to just observe a roll-over and set a flag, such that you > handle UART_Init() and the rest when there is a rising edge that's >= 10 > *or* is after a roll-over event. Or, if the timer rolls over that may be > considered "too long" and handled differently. One way or another, > though, it should be addressed. > > Also, strongly recommend that you get in the habit of *never* using > literal "magic numbers" (e.g., to test against gBreakLength or to set > gWatchDog). Abstract those out to a header file and do something like > #define MIN_BREAK_LENGTH 10 > #define WDOG_EVENT 125 > > -- > Rich Webb Norfolk, VA
Rich, Are you really sure that it is always best to put every single number into a header file ? (I often do but) I'm becoming more and more convinced that a magic number used only once is (often) better used directly and commented. And also that magic numbers which are not global should be defined in the source file in which they are used. If you don't do this then should you supoport each source file with (at least) TWO header files, one for global stuff and one for local stuff ? Michael Kellett
"Michael Kellett" <nospam@nospam.com> wrote in message 
news:JpidnVYgEdov1j3RnZ2dnUVZ8vadnZ2d@bt.com...
> > "Rich Webb" <bbew.ar@mapson.nozirev.ten> wrote in message > news:cr0s96pb275hapgd3ci2q2sf4lluhpe9sl@4ax.com... >> On Sat, 25 Sep 2010 20:44:57 +0800, "Dennis" <invalid@nowhere.com> >> wrote: >> >>> >>>I'm still learning C, and am having some problems with the code to detect >>>a >>>pulse over a specific width. >>> >>> >>>My method is: >>> >>>The pulse is negative going, normal state is high: >>>_____ ________ >>> \_____/ >>> >>> >>>Pulse duration time is measured by a register 'Break_Time' which is >>>incremented in the ISR each time TIMER1 overflows. >>> >>>I'm using interrupt INT0 to sense the start and finishing edges of the >>>pulse. >>> >>> >>>1)Sense pulse using INT0 >>> >>>2)On interrupt: >>> >>> 3) If INTEDG0 was set for falling edge start timer to measure >>> pulse >>>duration >>> >>> 4) if INTEDG0 was set for rising edge then stop timer & test >>>'Break_Time' >>> to see if the pulse length was long enough. >>> >>> 5) If pulse is too short then the 'gWatchDog' register is loaded >>>with a value that forces a reset in the main program loop. >>> >>> >>> The code I am using is below. Apologies for the poor formatting, I'm >>> still >>>trying to get that right as well. Debugging seems to show that the reset >>>{ >>>gWatchDog = 125; } line is executing after the end of the pulse is >>>detected. >>> >>>I've tried a few different approached to no avail. Any suggestions on >>>what >>>I'm doing wrong here or the tidiest way to do this task? >> >> I see where you initialize gBreakLength to zero on a falling edge but >> not where you calculate a new value for it before testing for >= 10. >> >> Once that's fixed, there is also a small window of vulnerability if the >> timer rolls over and the rising edge hits before it passes 10 again. It >> may be enough to just observe a roll-over and set a flag, such that you >> handle UART_Init() and the rest when there is a rising edge that's >= 10 >> *or* is after a roll-over event. Or, if the timer rolls over that may be >> considered "too long" and handled differently. One way or another, >> though, it should be addressed. >> >> Also, strongly recommend that you get in the habit of *never* using >> literal "magic numbers" (e.g., to test against gBreakLength or to set >> gWatchDog). Abstract those out to a header file and do something like >> #define MIN_BREAK_LENGTH 10 >> #define WDOG_EVENT 125 >> >> -- >> Rich Webb Norfolk, VA > > Rich, > > Are you really sure that it is always best to put every single number into > a header file ? > (I often do but) I'm becoming more and more convinced that a magic number > used only once is (often) better used directly and commented. > And also that magic numbers which are not global should be defined in the > source file in which they are used. > If you don't do this then should you supoport each source file with (at > least) TWO header files, one for global stuff and one for local stuff ? > > Michael Kellett > > >
Michael / Rich - a novice question - if I understand it correctly the advantage of defining constants in a header file is purely for clarity, it does not affect the code in any way (ie more compact etc). cheers D.
On Mon, 27 Sep 2010 08:45:52 +0100, "Michael Kellett"
<nospam@nospam.com> wrote:

> >"Rich Webb" <bbew.ar@mapson.nozirev.ten> wrote in message >news:cr0s96pb275hapgd3ci2q2sf4lluhpe9sl@4ax.com... >> On Sat, 25 Sep 2010 20:44:57 +0800, "Dennis" <invalid@nowhere.com> >> wrote: >> >> Also, strongly recommend that you get in the habit of *never* using >> literal "magic numbers" (e.g., to test against gBreakLength or to set >> gWatchDog). Abstract those out to a header file and do something like >> #define MIN_BREAK_LENGTH 10 >> #define WDOG_EVENT 125 >> >> -- >> Rich Webb Norfolk, VA > >Rich, > >Are you really sure that it is always best to put every single number into a >header file ? >(I often do but) I'm becoming more and more convinced that a magic number >used only once is (often) better used directly and commented. >And also that magic numbers which are not global should be defined in the >source file in which they are used. >If you don't do this then should you supoport each source file with (at >least) TWO header files, one for global stuff and one for local stuff ?
Some things, like register-dependent values, will typically be defined in the device header file or are fixed properties from the datasheet. Others, that are not likely to change, could be tagged either in a header file or in the source itself. E.g., UART0_LCR = 0x00000083; // divisor unlock UART0_DLM = 3; // 4800 w/pclk 14.7, prsc 1 UART0_DLL = 0; is clear enough that there's not a huge benefit to #define'ing it. But where the numbers may change during development or where they can't be derived from the datasheet, like the break-length test value, then I'd strongly recommend sticking them in a header file. WRT using two headers: yes, where it makes sense: one header that other parts of the program need to know about and one that contains only local values. If there are only a few of the latter, collecting them together at the top of the source file achieves the same results. -- Rich Webb Norfolk, VA
On Mon, 27 Sep 2010 20:02:53 +0800, "Dennis" <invalid@nowhere.com>
wrote:


>Michael / Rich - a novice question - if I understand it correctly the >advantage of defining constants in a header file is purely for clarity, it >does not affect the code in any way (ie more compact etc).
Well, it does great things for readability. Be kind to your maintenance programmers; make things easy to understand. If you pick it up again a year from now (where you're the maintenance programmer, too), and are trying to figure out what's going on, you'll thank your past self for planning ahead. WRT compactness, during source file translation (in phase 4), the preprocessing directives are carried out with appropriate substitutions made in the now partially translated source code file. So, writing #define MAX_FOO 666 if (BarBaz >= MAX_FOO) { // stuff } is seen identically to if (BarBaz >= 666) { // stuff } by the subsequent translation phases. No difference other than readability and a very slight increase in the time it takes to run the source through the compiler. -- Rich Webb Norfolk, VA
Michael Kellett wrote:

> Are you really sure that it is always best to put every single number into > a header file ? > (I often do but) I'm becoming more and more convinced that a magic number > used only once is (often) better used directly and commented.
It's very rare for a magic number to be used only once. Even if 0x34 is used once as the length of the FOO packet, there's every chance that it gets used again as a bit mask initializing the UART, or some such thing. Defining a name like FOO_PKT_LEN gives your maintenance programmer a search target that's just as plain as searching for a comment, and assures you that you and your program agree on the definition, which a comment does not.
> And also that magic numbers which are not global should be defined in the > source file in which they are used. > If you don't do this then should you supoport each source file with (at > least) TWO header files, one for global stuff and one for local stuff ?
This is true. I have no problem with definitions migrating out of the program preamble into the header file as the system grows. I got this in spades in firmware for a device I had to interface with. The code was full of set_bit (flags, 3); or clr_bit (flags2, 5); to set or clear individual bits in a couple of bytes of generic boolean flags. And, for added spice, if (flags & 0x04) to make decisions in the code, so that the connections with the sets and clrs was as obscure as possible. After several days figuring out how the program actually did what it did, we wrote a Python program to scan for these constructs and replace them with set_bit (flags, BUFFER_FULL) or if (tst_bit (flags2, INPUT_COMPLETE)) The third thing we did was to change the Python program to work from files of definitions, so we could run it over that programmer's other source programs. Not to demean the engineer who wrote those programs. His designs and the programs he wrote for them were accurate, and good or even brilliant in most ways. Just that by the time he understood bit-for-bit what to do, he wrote that down and moved on to make the next working product. It was after, when he'd moved on and the product requirements had changed, that we found we had a problem. Mel.
> > Michael Kellett
"Rich Webb" <bbew.ar@mapson.nozirev.ten> wrote in message 
news:t141a6phfols5e6amvfli55lllftj419l5@4ax.com...
> On Mon, 27 Sep 2010 20:02:53 +0800, "Dennis" <invalid@nowhere.com> > wrote: > > >>Michael / Rich - a novice question - if I understand it correctly the >>advantage of defining constants in a header file is purely for clarity, it >>does not affect the code in any way (ie more compact etc). > > Well, it does great things for readability. Be kind to your maintenance > programmers; make things easy to understand. If you pick it up again a > year from now (where you're the maintenance programmer, too), and are > trying to figure out what's going on, you'll thank your past self for > planning ahead. > > WRT compactness, during source file translation (in phase 4), the > preprocessing directives are carried out with appropriate substitutions > made in the now partially translated source code file. So, writing > > #define MAX_FOO 666 > > if (BarBaz >= MAX_FOO) { > // stuff > } > > is seen identically to > > if (BarBaz >= 666) { > // stuff > } > > by the subsequent translation phases. No difference other than > readability and a very slight increase in the time it takes to run the > source through the compiler. > > -- > Rich Webb Norfolk, VA
Thanks Rich - I'm learning a few things today. D.
On 27.09.2010 09:45, Michael Kellett wrote:

> Are you really sure that it is always best to put every single number into a > header file ?
"Always" is, of course, every bit as bad as "never" in that regard --- it's hardly ever strictly correct to use them.
> (I often do but) I'm becoming more and more convinced that a magic number > used only once is (often) better used directly and commented.
That's correct only if that magic number is a) 100% certain never to need to be changed, or b) is guaranteed to be found quickly by _everybody_ who ever needs to change it. Right now you're looking at this from the aspect of "I need to change this value --- how to do that"? But what about the maintainer who comes in much later, and who just needs to get an overview over _all_ magic numbers he might possibly have to change, anywhere in the module? He'll have a considerably easier job if all those constants are collected in one place.
> And also that magic numbers which are not global should be defined in the > source file in which they are used.
Here local coding guidelines come into play. If your shop's guidelines include a mandated section in every *.c file where such constants are defined to go (and where maintainers can thus be relied upon to look for them), by all means use that. Otherwise it's probably better to put them in a header.
> If you don't do this then should you supoport each source file with (at > least) TWO header files, one for global stuff and one for local stuff ?
Not necessarily. Headers for "local" stuff are for multi-C-file modules that need to distinguish between the external interface of the entire module and the interfaces holding the module together.