PicForum

Hello, I'm Dan, the new guy here..
I have a little project with a strange problem.

Here is my plan:
When GPIO4=1, GPIO0 goes high, while GPIO1 & GPIO2 alternate.
After 30 seconds (or if GPIO4 goes low), they ALL shut off until GPIO4 is triggered again.
I've tried doing this a few different ways, but am having a heck of a time nailing it down.

Any help would be greatly appreciated!

Here is my code:

Code: Select all

#include<pic12f629.h>
#include<htc.h>
#include <stdio.h>
#include <stdlib.h>
#include"xc.h"

#pragma config MCLRE=OFF , CP=OFF & CPD=OFF , BOREN=OFF , WDTE=OFF , PWRTE=OFF , FOSC=INTRCIO

#ifndef _XTAL_FREQ
 // Unless already defined assume 4MHz system frequency
 // This definition is required to calibrate __delay_us() and __delay_ms()
#define __delay_ms(x) _delay((unsigned long)((x)*(_XTAL_FREQ/4000.0)))
#define _XTAL_FREQ 4000000
#endif

//#define BUTTON GPIO3
#define PERIOD 1000000
#define XTAL 4000000
#define IPERIOD (4 * 1000000 / XTAL) // Period of instruction clock in uSeconds

volatile int i;

void sys_init(void);
void sys_init(void)
{
 GPIE=0;
 CMCON =0b00000111;
 TRISIO =0; TRISIO0=0; TRISIO1=0; TRISIO2=0; TRISIO4=1; TRISIO5=0; TRISIO3=1; GPIO =0;
        T0IE = 0; // Disable timer interrupt
 GIE = 0;
        OPTION_REG=0x00;
        WPU=0x30;
        STATUS=(1<<RP0);
}


int main(int argc, char** argv) {
    sys_init();   
    while(1)
    {
        if (GPIO4==0)
        {
            for (int i=0;i<30;i++)
            {
            GPIO0=1;
            GPIO1=1;GPIO2=0; __delay_ms(110);
            GPIO1=0;GPIO2=1; __delay_ms(110);
            }
        }
        if (GPIO4==1)
        {
            GPIO0=0; GPIO1=0; GPIO2=0;
        }
    }
}

As already mentioned over on the Microchip board, your problem will be "read-modify-write".
This tends to trip up new users when they try to rapidly update output pins in the same port, on old PICs which don't have LAT registers.
See some explanations of the effect here: http://www.mikroe.com/download/eng/docu ... lp/rmw.htm
and some solutions here: http://www.microchip.com/forums/m478014.aspx

Now, a few comments about your code.
Change:

Code: Select all

#include<pic12f629.h>
#include<htc.h>
#include <stdio.h>
#include <stdlib.h>
#include"xc.h"

to just:

Code: Select all

#include"xc.h"

(you don't need any of those other includes)

and change:

Code: Select all

#ifndef _XTAL_FREQ
 // Unless already defined assume 4MHz system frequency
 // This definition is required to calibrate __delay_us() and __delay_ms()
#define __delay_ms(x) _delay((unsigned long)((x)*(_XTAL_FREQ/4000.0)))
#define _XTAL_FREQ 4000000
#endif

to just:

Code: Select all

#define _XTAL_FREQ 4000000

(i.e. just define the clock frequency. You don't need to define __wait_ms(), that is already done in xc.h)

and change:

Code: Select all

#define XTAL 4000000
#define IPERIOD (4 * 1000000 / XTAL) // Period of instruction clock in uSeconds

to:

Code: Select all

#define IPERIOD (4 * 1000000 / XTAL_FREQ) // Period of instruction clock in uSeconds

(i.e. just use the existing clock frequency definition, don't create a second one. Also, you never use IPERIOD, so it's probably all redundant.)

Change:

Code: Select all

TRISIO =0; TRISIO0=0; TRISIO1=0; TRISIO2=0; TRISIO4=1; TRISIO5=0; TRISIO3=1; GPIO =0;

to

Code: Select all

    TRISIO = 0b00011000;
    GPIO = 0;

(Why use seven instructions to do what you can do in one?)

Get rid of:

Code: Select all

        STATUS=(1<<RP0);

Don't try to play with the status register from C code!

Change:

Code: Select all

int main(int argc, char** argv) {

to

Code: Select all

void main(void) {

(this is embedded C, there is no operating system passing command line parameters to you.)

change

Code: Select all

            for (int i=0;i<30;i++)
            {
            GPIO0=1;
            GPIO1=1;GPIO2=0; __delay_ms(110);
            GPIO1=0;GPIO2=1; __delay_ms(110);
            }

to:

Code: Select all

            for (int i=0;i<30;i++)
            {
            GPIO = (GPIO & 0b11111010) | 0b00000010;    //drop GP2,GP0, raise GP1
             __delay_ms(110);
            GPIO = (GPIO & 0b11111101) | 0b00000100;    //drop GP1, raise GP2
             __delay_ms(110);
            }

(RMW fix)

and change:

Code: Select all

        if (GPIO4==1)
        {
            GPIO0=0; GPIO1=0; GPIO2=0;
        }

to:

Code: Select all

        if (GPIO4==1)
        {
            GPIO &= 0b11111000;     //drop GPIO0, GPIO1, and GPIO2
        }

(RMW fix)

Small correction:

Code: Select all

#define IPERIOD (4 * 1000000 / _XTAL_FREQ) // Period of instruction clock in uSeconds

_XTAL_FREQ has a leading underscore.

Thank you both VERY much!

ric: I had posted on the Microchip board but was held up by a moderator (my first post) for what seemed like forever!! I lost hope and found this board. They both were posted about the same time .. figures.

Ian.M wrote:Small correction:

Code: Select all

#define IPERIOD (4 * 1000000 / _XTAL_FREQ) // Period of instruction clock in uSeconds

_XTAL_FREQ has a leading underscore.

Thanks Ian.
As it was never used, it wouldn't have caused a warning until something used it.

Ok, so i'm failing tweak after tweak.
This code didn't work, I'm pretty sure it's because of the quantity of bits.

Code: Select all

GPIO = (GPIO & 0b11111010) | 0b00000010;    //drop GP2,GP0, raise GP1
__delay_ms(110);
GPIO = (GPIO & 0b11111101) | 0b00000100;    //drop GP1, raise GP2
__delay_ms(110);

I changed it to (which worked):

Code: Select all

GPIO = 0b000011;
__delay_ms(110);
GPIO = 0b000101;
__delay_ms(110);

I took Bilbo's advice and added an IF statement within the loop. This works and does interrupt the loops, but only for a second, then the loop starts over again.

Code: Select all

if (i == 10) {
GPIO0 = 0; __delay_ms(1000); GPIO0 = 1;
break;
}

RESULTS:
Bringing GPIO3 low, turns off the lights and ends the sequence.
Bringing GPIO3 high, does nothing besides continue the sequence.
Leaving GPIO3 with nothing connected does nothing besides continue the sequence.

Here is my current code:

Code: Select all

#include"xc.h"

#pragma config MCLRE=OFF , CP=OFF & CPD=OFF , BOREN=OFF , WDTE=OFF , PWRTE=OFF , FOSC=INTRCIO

#define _XTAL_FREQ 4000000
#define __delay_ms(x) _delay((unsigned long)((x)*(_XTAL_FREQ/4000.0)))
#define IPERIOD (4 * 1000000 / _XTAL_FREQ) // Period of instruction clock in uSeconds

volatile int i;

void sys_init(void);

void sys_init(void) {
    GPIE = 0;
    //CMCON =0x07;
    TRISIO = 0b011000;
    GPIO = 0;
    T0IE = 0; // Disable timer interrupt
    GIE = 0;
    OPTION_REG = 0x00;
    WPU = 0x30;
}

void main(void) {
    sys_init();
    GPIO = 0b000000;
    while (1) {
        if (GPIO3 == 1) {
            for (int i = 0; i < 30; i++) {
                GPIO = 0b000011; //(GPIO & 0b000011) | 0b000001; //drop GP2,GP0, raise GP1
                __delay_ms(110);
                GPIO = 0b000101; //(GPIO & 0b000101) | 0b000001; //drop GP1, raise GP2
                __delay_ms(110);

                if (i == 30) {
                    GPIO = 0b000000;
                    break;
                }
            }
        }
        if (GPIO3 == 0) {
            GPIO = 0b000000; //drop GPIO0, GPIO1, and GPIO2
        }
    }
}



Any thoughts? Thank you..

I suspect your config lines aren't working, so pulling GPIO3 low is actually resetting the PIC.
Change:

Code: Select all

#pragma config MCLRE=OFF , CP=OFF & CPD=OFF , BOREN=OFF , WDTE=OFF , PWRTE=OFF , FOSC=INTRCIO

to

Code: Select all

#pragma config MCLRE=OFF , CP=OFF, CPD=OFF, BOREN=OFF, WDTE=OFF, PWRTE=OFF, FOSC=INTRCIO

Is there a pullup on RC3? There is no internal WPU on that pin.

Also note:

Code: Select all

volatile int i;

You don't need volatile here, and this variable is not used at all because you declare "i" again inside your loop.

Code: Select all

for (int i = 0; i < 30; i++) {

XC8 follows the C89 standard, which does NOT permit declaring a local variable inside the "for()" statement.
Yes, it does appear to work, but the compiler authors have stated you should not rely upon it.

Thank you ric.

I'm still batting 1000 (failing, that is). I've only been playing with this stuff for about three days now, so I'm in a over my head. Even though I did make some headway for never touching anything like this before.

I deleted the IF statements and left only the FOR loop. I figured if I could just get the loop to stop after 10 iterations (for trial purposes), then shut down, I could eliminate one issue. No luck. I commented out a few things.

Code: Select all

#include"xc.h"

#pragma config MCLRE=OFF , CP=OFF, CPD=OFF, BOREN=OFF, WDTE=OFF, PWRTE=OFF, FOSC=INTRCIO

#define _XTAL_FREQ 4000000
#define __delay_ms(x) _delay((unsigned long)((x)*(_XTAL_FREQ/4000.0)))
#define IPERIOD (4 * 1000000 / _XTAL_FREQ) // Period of instruction clock in uSeconds

//volatile int i;

void sys_init(void);

void sys_init(void) {
    GPIE = 0;
    //CMCON =0x07;
    TRISIO = 0b011000;
    GPIO = 0;
    T0IE = 0; // Disable timer interrupt
    GIE = 0;
    OPTION_REG = 0x00;
    WPU = 0x30;
}

void main(void) {
    sys_init();
    GPIO = 0b010000;
    while (1) {
        //if (GPIO4 == 1) {
            int i = 0;

            for (i = 0; i < 10;) {

                i = i + 1;

                if (i<10){

                GPIO = 0b010011; //(GPIO & 0b000011) | 0b000001; //drop GP2,GP0, raise GP1
                __delay_ms(150);
                GPIO = 0b010101; //(GPIO & 0b000101) | 0b000001; //drop GP1, raise GP2
                __delay_ms(150);
                //break;
                }

                if (i >= 10) {
                    GPIO = 0b010000;__delay_ms(1000);
                    break;
                }
                //break;
            }
            GPIO=0b000000;
        //}
//        if (GPIO4 == 0) {
//            GPIO = 0b010000; //drop GPIO0, GPIO1, and GPIO2
//        }
    }
}

Thank you

What exactly is this final version of the code doing?
I would expect it to blink GP1 and GP2 rapidly ten times,
then pause for a second with both low,
then restart the whole process.

You set GPIO.4 during the one second pause, but that won't do anything because it is set as input in TRISIO.

That's what it's doing .. but i want it to stop after ten times. Then go to sleep, only to be woken up by GPIO4 going LOW then HIGH again.

I put that one second dip in there so I could see whats happening, just for testing.