On Tue, Jul 22, 2014 at 01:52:33AM -0500, Richard R. Pope wrote:
> James,
>      I understand the confusion, so let's try this again. Here is all of=
=20
> the code except for the includes and configuration bits.
>      I was able to change PIR1bits.TMR1IF to TMR1F. The timer tool from=20
> MicroElektronika doesn't use all of the proper flag names. This name was=
=20
> suggested to me by another person and since it works I used it. The=20
> other name works so I will use it because this is more consistent.
>      You were correct about the void InitTimer1() not being called. I=20
> have added a call to this function. I also added a line showing the=20
> frequency of the crystal that I am going to use.

Thanks.  I shall add review comments in the code, below each line that
causes my brain to explode.  ;-)

> Thanks,
> rich!
>=20
> //  Cystral Freq =3D 20MHz

Typo in word Crystal.

>=20
> //Global variables
> int count =3D 0;

This variable should be marked volatile.

> void main(void)
> {
>=20
>      PORTA =3D 0;            //    Turn porta off
>      TRISA =3D 255;        //    Set Porta as all inputs

The decimal constant makes it harder to understand why all ports are
inputs; requires one to remember that 255 is 0xff.

Both lines are redundant, the power on reset values for TRISA are all
bits set.

>      PORTC =3D 2;            //    Set RC1 to RC5 high

Comment does not match the constant.  Only RC1 is high.

>      TRISC =3D 193;        //    Start with DP1 on, the rest of portc is =
inputs

193 is 0xC1.  Bits 7 and 6 are unimplemented, but you're writing them
high.  So only RC0 is configured as input.

Comment does not match the constant.  Only RC0 is input.

>      CMCON0 =3D 7;         //  Turn off Comparators
>      ANSEL =3D 16;         //  Set RC0/AN4 as analog
>      InitTimer1();        //    Set up the timer
>=20
> // Define commands
>      int    DP =3D 1;

This is RC0.

>      int Reset =3D 33 + DP;

This is 34 decimal, 0x22 hex, binary 100010.  So by using this you are
setting RC1 and RC5.  Very odd that you are adding 33 to 1 to get
this.  Ask yourself what you intended to do here.

>      int GateOn =3D 17 + DP;

This is 18 decimal, 0x12 hex, binary 10010.  So by using this you are
setting RC1 and RC4.  Again, a very odd way to set a constant.

>      int GateOff =3D DP;

This is RC0.

>=20
> while (1 =3D=3D 1)            //    Loop forever
>      {
>          PORTC =3D Reset;        //    Clear the Display
>          PORTC =3D GateOn;        //    Start counting
>          delay(1);            // Count the frequency for one second =3D H=
Z
>          PORTC =3D GateOff;
>          delay(15);            // Display the frequency for 15 seconds
>=20
>      }
> }    //  End NewControlCode.c
>=20
> void delay(int time)
> {
>      int limit =3D time * 10 + 1;
>=20
>      TMR1ON =3D 0;        // Turn the timer off
>      TMR1IE     =3D 0;    // Disable timer interrupts
>      resettimer();    // Reset the timer to starting values
>      count =3D 0;        // Reset the timer interrupt counter
>      TMR1IE     =3D 1;    // Enable the timer interrupts
>      TMR1ON =3D 1;        // Turn the timer on
>=20
>      while(count < limit)
>      {
>      }
>     return;
> }
>=20
> void resettimer(void)
> {
>      TMR1H =3D 0x0B;    // Reset the timer to roll over
>      TMR1L =3D 0xDC;   // at 100mS
> }

This doesn't really need to be in a separate function.

> //Timer1
> //Prescaler 1:8; TMR1 Preload =3D 3036; Actual Interrupt Time : 100 ms
>=20
> //Place/Copy this part in declaration section
>=20
> void InitTimer1(){
>    T1CON                 =3D 0x31;

The comment here should say you are setting the prescaler to 1:8 and
turning the timer on.

While I agree that the prescaler should be set, I don't see why you
should turn the timer on here, since it is turned on in delay() function.

>    PIR1bits.TMR1IF     =3D 0;

Another use of PIR1bits.TMR1IF that can be just TMR1IF.

>    TMR1H                 =3D 0x0B;
>    TMR1L                 =3D 0xDC;

There's no reason to set these values here given that they are set by
resettimer() which is called by the delay() function.

>    TMR1IE             =3D 1;

There's no need to set TMR1IE now, since it will be cleared and set in
the delay() function.  You must choose where things are set, and
unless you have a good reason, set them in one place only.

>    INTCON             =3D 0xC0;

When it boils down to it, InitTimer1() ends up being setting the
prescaler (T1CKPS0, T1CKPS1) and INTCON (GEI, PEIE).

> }
>=20
> void Interrupt(){
>    if (TMR1IF){
>      TMR1IF =3D 0;
>      TMR1H     =3D 0x0B;
>      TMR1L    =3D 0xDC;

There's no need to reset the timer in the Interrupt() function, as it
will be reset in the delay() function.  Clearing TMR1IF is enough.

>      count++;
>    }
> }

--=20
James Cameron
http://quozl.linux.org.au/
--=20
http://www.piclist.com/techref/piclist PIC/SX FAQ & list archive
View/change your membership options at
http://mailman.mit.edu/mailman/listinfo/piclist
.