Hi guys,

Say if I want to turn on a couple of leds, the following codes work fine:

GPIO |= (1<<LED1)+(1<<LED2);

or

    GPIO |= (1<<LED2);
    GPIO |= (1<<LED1);

but if I use the following codes, only the LED2 lights up, why?

    GPIO |= (1<<LED1);
    GPIO |= (1<<LED2);

I am using MPLAB X with XC8 complier, PIC12F683 under win7.
LED1 and LED2 are defined at the beginning of the code.

PIC12F683 datasheet

 

What have you done to debug this problem?

Have you run the debugger an looked at the values in the register?

Have you measured the output of the pins with a scope, logic probe, voltmeter, locic analyzer etc?

 

OK, so it works the first two ways, but not the third. This rules out things like TRISIO or ANSEL being set wrong.

Does it work just fine in the simulator, but not in the real hardware? That would indicate you're bumping up against the read-modify-write (R-M-W or just RMW) problem. As statements such as "GPIO |= (1<<LED2);" can and will compile down to a single assembly instruction even C is quite vulnerable to this.

If you can place another instruction like a series of nop()'s between the two instructions and that makes it work then you have a RMW issue.

Google the term, there is lots of info concerning it out there.

 

What have you done to debug this problem?

Have you run the debugger an looked at the values in the register?

Have you measured the output of the pins with a scope, logic probe, voltmeter, locic analyzer etc?

I have measured the voltage with a DMM and scope, but I have no debug header for this chip as it requires one.

 

Thanks ErnieM,

following your advice, I found the solution here, its a RMW problem:

http://www.mikroe.com/download/eng/documents/compilers/mikroc/pro/pic/help/rmw.htm

PS:
I have tried the following codes:

    GPIO |= (1<<LED1);
    __delay_ms(5000);
    GPIO |= (1<<LED2);

the interesting thing is, the LED1 turns on first, then LED2 turns on but at the same time, the LED1 turns off as well... the end result, only LED2 turn on.

OK, so it works the first two ways, but not the third. This rules out things like TRISIO or ANSEL being set wrong.

Does it work just fine in the simulator, but not in the real hardware? That would indicate you're bumping up against the read-modify-write (R-M-W or just RMW) problem. As statements such as "GPIO |= (1<<LED2);" can and will compile down to a single assembly instruction even C is quite vulnerable to this.

If you can place another instruction like a series of nop()'s between the two instructions and that makes it work then you have a RMW issue.

Google the term, there is lots of info concerning it out there.

 

Usually the best thing to do is to use a variable like GPIO_temp, do all your work on that and then copy that to GPIO when required. That way there will never be any problems. It's only a few extra instructions.

 

OK, so it works the first two ways, but not the third. This rules out things like TRISIO or ANSEL being set wrong.

Not necessarily. For example, you know you can set an analog pin to be an output, right? In this case, if I recall correctly, you can set the pin to output a '0' or a '1' but the pin will always read as a '0'. And so, if the OP has LED1 and LED2 connected as active high outputs with LED2 on the GP5 pin, then it's possible he/she has forgotten to turn off the analog functions.

 

What's the LED resistors value? Have you tried 2.2K or 4.7K? Or are you using a very low resistance value?

 

Not necessarily. For example, you know you can set an analog pin to be an output, right? In this case, if I recall correctly, you can set the pin to output a '0' or a '1' but the pin will always read as a '0'. And so, if the OP has LED1 and LED2 connected as active high outputs with LED2 on the GP5 pin, then it's possible he/she has forgotten to turn off the analog functions.

so after reading your replay, I added these codes to turn off the analog functions, still get the same result.

here are my codes to turn off the ADC

    /*
    ANS<3:0>: Analog Select bits
    Analog select between analog or digital function on pins AN<3:0>, respectively.
    1 = Analog input. Pin is assigned as analog input(1).
    0 = Digital I/O. Pin is assigned to port or special function.
     */
    ANSELbits.ANS=0;
    /*
     ADON: ADC Enable bit
    1 = ADC is enabled
    0 = ADC is disabled and consumes no operating current
     */
    ADCON0bits.ADON=0;

 

What's the LED resistors value? Have you tried 2.2K or 4.7K? Or are you using a very low resistance value?

I am using some cheap RED led with 1.8v voltage drop, about 10mA normal operation current.

the resistor value was 330R, then I replaced one of them to a 10K, the result is still the same.

Then remove the led and the resistor all together, and I probe the pin connected to the pin directly, still get the same result.

 

here are my codes to turn off the ADC

That's not how you turn off the analog pin functions. Please look at example 4.1 in section 4 of the datasheet...

If you're not going to post your program, can you at least tell us how you're defining LED1 and LED2?

<added>

Oops! Just found the "ANS" structure member in the 12F683 header... My apologies... What you're doing should disable the ADC analog functions...

 

I have tried the following codes:

    GPIO |= (1<<LED1);
    __delay_ms(5000);
    GPIO |= (1<<LED2);

the interesting thing is, the LED1 turns on first, then LED2 turns on but at the same time, the LED1 turns off as well... the end result, only LED2 turn on.

That is interesting, and it is similar to what the RMW problem looks like. However, after a full 5 second delay it has to be something else. Somehow when the port value is read the LED1 pin is read as a zero, and is thus reset when LED2 is set.

Your code to turn off the analog pin functions is very good, you must have read section 4 of the datasheet.

Could you post your code, at least a portion to demonstrate this problem? What are LED1 and LED2?

If I had the code I could run it in the MPLAB simulator and perhaps see something. This is an interesting problem.

 

my code is not intended to do anything, just teaching myself PIC and messing around. I have comment out all the stuff that are irrelevant(intended to isolate the faulty code), still get to the same result, but feel free to check my commented out code as well.

here is all my code:

PS:
PIC12F683, XC8(v1.10) complier, MPLAB X(v1.41), Win7 64bit, PICKit3(Firmware Suite Version.....01.28.07)

/*
 * File:   main.c
 * Author: JMS
 *
 * Created on 16 November 2012, 10:23 PM
 */
//#pragma config CPD = OFF, BOREN = OFF, IESO = OFF,
//#pragma config FOSC = INTOSCIO, FCMEN = OFF, MCLRE = OFF,
//#pragma config WDTE = OFF, CP = OFF, PWRTE = OFF
#pragma config FOSC = INTOSCIO, WDTE = OFF

#define _XTAL_FREQ 4000000

#define LED1 1 //LED1 connects to GP1
#define LED2 5 //LED2 connects to GP5

//#define PINA_DDR TRISIObits.TRISIO1
//#define PINB_DDR TRISIObits.TRISIO5
//#define PINC_DDR TRISIObits.TRISIO4
//#define PIND_DDR TRISIObits.TRISIO2

//#define PINA GPIObits.GP1
//#define PINB GPIObits.GP5
//#define PINC GPIObits.GP4
//#define PIND GPIObits.GP2

//#define DelayMS 100

//#include <stdint.h>
#include <xc.h>

void InitialIO(void);
//void Leds(void);


int main(void) {
    InitialIO();


    GPIO |= (1<<LED1);
    __delay_ms(2000);
    GPIO |= (1<<LED2);
    while(1)
    {
        //Leds();
    }

}

/*
void Leds(void)
{
        PINA=1;
        __delay_ms(DelayMS);
        PINA=0;
        __delay_ms(DelayMS);
        PINB=1;
        __delay_ms(DelayMS);
        PINB=0;
        __delay_ms(DelayMS);
        PINC=1;
        __delay_ms(DelayMS);
        PINC=0;
        __delay_ms(DelayMS);
        PIND=1;
        __delay_ms(DelayMS);
        PIND=0;
        __delay_ms(DelayMS);
}
*/

void InitialIO(void)
{
    /*
    ANS<3:0>: Analog Select bits
    Analog select between analog or digital function on pins AN<3:0>, respectively.
    1 = Analog input. Pin is assigned as analog input(1).
    0 = Digital I/O. Pin is assigned to port or special function.
     */
    ANSELbits.ANS=0;
    /*
     ADON: ADC Enable bit
    1 = ADC is enabled
    0 = ADC is disabled and consumes no operating current
     */
    ADCON0bits.ADON=0; 


    //PINA_DDR = 0;
    //PINB_DDR = 0;
    //PINC_DDR = 0;
    //PIND_DDR = 0;



    TRISIObits.TRISIO1=0;  //set GP1 output
    TRISIObits.TRISIO5=0;  //set GP5 output

    GPIObits.GP1 = 0;  //turn LED1 off
    GPIObits.GP5 = 0;  //turn LED5 off

    //PINA=0;
    //PINB=0;
    //PINC=0;
    //PINC=0;
}

 

You have to disable the analog comparator functions too (example 4.1 in the datasheet).

Good luck on your project.

Happy Holidays. Mike

 

MMcLaren got it. The comparators are by default turned on at power up, and also need to be disabled using:

CMCON0bits.CM = 0b111;

(See FIGURE 8-4: COMPARATOR I/O OPERATING MODES)

When I added this to your code the simulator (I ran your code plus some mods) was able to correctly set all possible I/O lines high, and also set LED1 and then LED2.

When the comparator is active the I/O pins associated with then are read as zero. They may be set correctly, but read incorrectly. This is why setting the LED2 and then the LED1 bit worked.

It is interesting that the simulator does not show this correctly: it shows the bits not being able to be set, which is different from the hardware. I've noted a few simulator discrepancies such as this, but they are limited to cases where the settings are incorrect anyway. Properly set config settings will simulate correctly, improper setting simulate differently then the actual hardware will act, but never in a way that hides the error.

I just noticed your line "GPIO |= (1<<LED1)+(1<<LED2);" also worked, as it compiles to a simple write statement so no read is required. However, you have mixed logical statements (1<<LED1) with arithmetic statement (the +) which is generally something to be avoided as a style issue, but may in some cases (such as signed variables) may lead to undesired results.

In case you have not found it, the simulator is built inside MPLAB, and can be accessed by selecting MPLAB SIM in the Debugger | Select Tool menu item. It is a very useful program!

 

I just noticed your line "GPIO |= (1<<LED1)+(1<<LED2);" also worked, as it compiles to a simple write statement so no read is required. However, you have mixed logical statements (1<<LED1) with arithmetic statement (the +) which is generally something to be avoided as a style issue, but may in some cases (such as signed variables) may lead to undesired results.

This depends on the compiler.

Some will produce the arithmetic value, because the + operator is arithmetic. If for instance && would be used, then it would interprete the values as boolean. But not neccessarily. You'd have to look up the ANSI specification what to do in such a case.

(1<<value) is not neccessarily a logical value. I am not even sure if it logical at all. Aren't shift operations arithmetic by default?

 

Thanks for helping me Mike, you have a good holiday too

You have to disable the analog comparator functions too (example 4.1 in the datasheet).

Good luck on your project.

Happy Holidays. Mike

 

MMcLaren got it. The comparators are by default turned on at power up, and also need to be disabled using:

CMCON0bits.CM = 0b111;

(See FIGURE 8-4: COMPARATOR I/O OPERATING MODES)

When I added this to your code the simulator (I ran your code plus some mods) was able to correctly set all possible I/O lines high, and also set LED1 and then LED2.....

Thank you so much for helping me on this and taking your time to explain it to me in details.

It looks like I will need to look at the datasheet more thoroughly, as a bad habit of mine, I usually jump the section that I will be using and don't pay too much attention to other parts of the datasheet.

And thanks for point out the DEBUG SIM in mplab, this is an area I will need to spend some more time on.

I just noticed your line "GPIO |= (1<<LED1)+(1<<LED2);" also worked, as it compiles to a simple write statement so no read is required. However, you have mixed logical statements (1<<LED1) with arithmetic statement (the +) which is generally something to be avoided as a style issue, but may in some cases (such as signed variables) may lead to undesired results.

as for this, I was intended to use something like this:

GPIO |= (1<<LED1)|(1<<LED2);

I am not sure why I typed "+" instead.

 

Well you are most welcome. I actually quite enjoy "puzzle problems" such as this, especially when the problem is as well defined as your.

Reading the data sheets actually became easier in the last decade. In the past some of the more subtly interactions were not flagged as well as they are now. Yes, you got caught this time but next time you either will not get caught or wiggle right out of the trap.

Using the | or a + to combine bits is mostly a style issue, but if you accidentally include the same bit twice you'll be very happy you or'ed bits instead of added them.

Good luck!

 

As a rule on PIC 10/12/16F you should NEVER write to a single bit on an output port.

It is good form to always use a shadow register, set or clr the bits in that shadow register and then write it once to the PORT as a whole register.

Not only does that fix RMW faults like the one you had, it also allows some benefits like ensuring all the PORT pins change at the same instant and in one single instruction. That can be useful in making manual PWM outputs and LED multiplexing etc.