My First C program.

[Zero999]

The one thing I've not done before is written a proper C program. I've programmed in BASIC of different types on a desktop and MCUs in assembly, but never C. I've recently had a lot of time on my hands and decided to give it a go.

The idea is simple. A timer with two independent negative edge triggered channels. When pressing a switch, the LED turns on for 10 seconds. Nothing complicated.

It uses the old PIC12F509 because it's what I have to hand and also seems to be more widely available, than some of the more modern chips. It's compiled with plain old XC8 in free mode.

Is there anything glaringly obvious I've missed?

Code: [Select]

/*
 * File:   main.c
 *
 * Created on 14 April 2022, 18:47
 */

//  Turns on an LED for 10 seconds after being triggered by a pushbutton.
//  GP0 = Pushbutton switch 1
//  GP1 = LED1
//  GP3 = Pushbutton switch 2
//  GP4 = LED2
//
// PIC12F509 Configuration Bit Settings

#pragma config OSC = IntRC      // Oscillator Selection bits (internal RC oscillator)
#pragma config WDT = OFF        // Watchdog Timer Enable bit (WDT disabled)
#pragma config CP = OFF         // Code Protection bit (Code protection off)
#pragma config MCLRE = OFF      // GP3/MCLR Pin Function Select bit (GP3/MCLR pin function is digital input, MCLR internally tied to VDD)

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.
#define _XTAL_FREQ 4000000      // oscillator frequency for _delay()

// pin definitions
#define trigger1 GPIObits.GP0
#define LED1     GPIObits.GP1
#define trigger2 GPIObits.GP3
#define LED2     GPIObits.GP4
#define timeout1 10/0.016       // 10/16ms
#define timeout2 10/0.016

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

         __bit trigger1_s;      // shadow trigger 1
         __bit trigger2_s;      // shadow trigger 2
unsigned short counter1 = 0;    // timer counter 1
unsigned short counter2 = 0;    // timer counter 2
     
void main(void) {
   
    OPTION = 0b10010101;        // configure Timer0:
                                //-0------ enable weak pull-ups on GP0, 1 & 3
                                //--0----- timer mode (T0CS = 0)
                                //----0--- pre scaler assigned to Timer0 (PSA = 0)
                                //-----101 pre scaler = 64 (pre scaler = 101)
                                // -> increment every 64us     
    TRIS = 0b101101;            // configure GP1 & GP4 as an outputs
    GPIO = 0;                   // set all outputs low
    TMR0 = 0;                   // reset timer
   
    for (;;)
    {
        if (TMR0 >= 250)        // timer exceeds 250 * 64us = 16ms
        {
            TMR0 = 0;           // reset timer

            if (counter1)       // decrement counter if not zero
                counter1--;
            else LED1 = 0;

            if (trigger1_s & !trigger1 & !counter1) // trigger changed from high to low & counter is zero
            {
                counter1 = timeout1;
                LED1 = 1;
            }   
            trigger1_s = trigger1;

            if (counter2)       // decrement counter if not zero
                counter2--;
            else LED2 = 0;
           
            if (trigger2_s & !trigger2 & !counter2) // trigger changed from high to low & counter is zero
            {
                counter2 = timeout2;
                LED2 = 1;
            }   
            trigger2_s = trigger2;
        }   
    }
return;
}


[ledtester]

A little style point... when defining macros that represent arithmetic expressions you should enclose them in parens, e.g.:

Code: [Select]

#define timeout1 (10/0.016)       // 10/16ms
#define timeout2 (10/0.016)

In this case it doesn't matter but it's something to be aware of.

Also, I think the code can cause a little glitch -- i.e. it is possible for both LED1=0 and LED1=1 to be executed in the same loop iteration. You won't see the LED flash but it will create a glitch on the output line.

Perhaps you want to use logic like this:

Code: [Select]

    if (counter1) {
        counter--;
    } else {
        if (edge detected) {
            LED1 = 1;
            ...
        } else {
            LED1 = 0;
        }
    }


[magic]

& is the bitwise AND operator and && is logical AND which you should generally be using in conditionals.

For single bit variables they ought to be equivalent, and the ! operator produces 0 or 1 even though its output type is int, so your code will likely work.

The point about macros is right, and not just a matter of style. Macros with expressions in them are often misinterpreted when pasted into a larger expression due to operator precedence rules.

[Zero999]

A little style point... when defining macros that represent arithmetic expressions you should enclose them in parens, e.g.:

Code: [Select]

#define timeout1 (10/0.016)       // 10/16ms
#define timeout2 (10/0.016)

In this case it doesn't matter but it's something to be aware of.

Also, I think the code can cause a little glitch -- i.e. it is possible for both LED1=0 and LED1=1 to be executed in the same loop iteration. You won't see the LED flash but it will create a glitch on the output line.

Perhaps you want to use logic like this:

Code: [Select]

    if (counter1) {
        counter--;
    } else {
        if (edge detected) {
            LED1 = 1;
            ...
        } else {
            LED1 = 0;
        }
    }


It complied with the macro like that, but it did warn me about doing a floating point to unsigned integer conversion, which I thought was to be expected.

Good on you for noticing that logic error I failed to spot. It's also made the program a little smaller as it's eliminated the & !counter term from two expressions. Unfortunately I don't have an oscilloscope handy at the moment.

& is the bitwise AND operator and && is logical AND which you should generally be using in conditionals.

For single bit variables they ought to be equivalent, and the ! operator produces 0 or 1 even though its output type is int, so your code will likely work.

The point about macros is right, and not just a matter of style. Macros with expressions in them are often misinterpreted when pasted into a larger expression due to operator precedence rules.

Originally I was using if (trigger2_s > 0 && trigger2 = 0) but I thought it would be more compact to use &. which did work. I've changed it to && if you think it's better practice.

Here's the revised code.

Code: [Select]

/*
 * File:   main.c
 *
 * Created on 14 April 2022, 18:47
 */

//  Turns on an LED for 10 seconds after being triggered by a pushbutton.
//  GP0 = Pushbutton switch 1
//  GP1 = LED1
//  GP3 = Pushbutton switch 2
//  GP4 = LED2
//
// PIC12F509 Configuration Bit Settings

#pragma config OSC = IntRC      // Oscillator Selection bits (internal RC oscillator)
#pragma config WDT = OFF        // Watchdog Timer Enable bit (WDT disabled)
#pragma config CP = OFF         // Code Protection bit (Code protection off)
#pragma config MCLRE = OFF      // GP3/MCLR Pin Function Select bit (GP3/MCLR pin function is digital input, MCLR internally tied to VDD)

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.
#define _XTAL_FREQ 4000000      // oscillator frequency for _delay()

// pin definitions
#define trigger1 GPIObits.GP0
#define LED1     GPIObits.GP1
#define trigger2 GPIObits.GP3
#define LED2     GPIObits.GP4
#define timeout1 (10/0.016)       // 10/16ms
#define timeout2 (10/0.016)

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

         __bit trigger1_s;      // shadow trigger 1
         __bit trigger2_s;      // shadow trigger 2
unsigned short counter1 = 0;    // timer counter 1
unsigned short counter2 = 0;    // timer counter 2
     
void main(void) {
   
    OPTION = 0b10010101;        // configure Timer0:
                                //-0------ enable weak pull-ups on GP0, 1 & 3
                                //--0----- timer mode (T0CS = 0)
                                //----0--- pre scaler assigned to Timer0 (PSA = 0)
                                //-----101 pre scaler = 64 (pre scaler = 101)
                                // -> increment every 64us     
    TRIS = 0b101101;            // configure GP1 & GP4 as an outputs
    GPIO = 0;                   // set all outputs low
    TMR0 = 0;                   // reset timer
   
    for (;;)
    {
        if (TMR0 >= 250)        // timer exceeds 250 * 64us = 16ms
        {
            TMR0 = 0;           // reset timer

            if (counter1)       // decrement counter if not zero
                counter1--;
            else
            {
                if (trigger1_s && !trigger1) // trigger changed from high to low & counter is zero
                {
                    counter1 = timeout1;
                    LED1 = 1;
                }
                else LED1 = 0;
            }           
            trigger1_s = trigger1;
           
            if (counter2)       // decrement counter if not zero
                counter2--;
            else
            {
                if (trigger2_s && !trigger2) // trigger changed from high to low & counter is zero
                {
                    counter2 = timeout2;
                    LED2 = 1;
                }
                else LED2 = 0;
            }           
            trigger2_s = trigger2;
        }   
    }
return;
}

[Ian.M]

Use an explicit cast to unsigned short in your timeout macros and the warning should go away.

However there's another issue due to the baseline (and midrange) PIC's port architecture - changing one output pin on a port by using GPIObits can cause other output bits on the same port to 'flip'. This issue is generally known in the PIC community  as the 'Read Modify Write effect' ('RMW effect').  See http://picforum.ric323.com/viewtopic.php?f=38&t=12 for details, especially the links to  Stefan Uhlemayr's and John Temples's notes on the subject for reliable work-arounds.

[indeterminatus]

A few styling remarks, which are highly subjective. Your program is simple enough that it will not matter much.

The final "return;" statement adds nothing. For one, control will never reach this statement, and even if it would, it's the end of the function anyway. Unless I'm missing something that your particular compiler would enforce it.

Comments like "// decrement counter if not zero" are superfluous. They just repeat the code (on the same level of abstraction). It does not tell the programmer *why* something happens. As of now, comment and code match, but there's a non-zero chance that the code will evolve. It's easy to neglect reflecting those changes in comments, and once that happens the comment is not merely not adding value, it is actually mis-leading (and thus malicious).
In my opinion, it's good practice to be greedy with comments. Express your intent clearly in code, so you don't need them.

From an analytical point of view, what's the difference between the block that deals with counter1, LED1, trigger1_s, trigger1 and the block that deals with counter2, LED2, trigger2_s, trigger2? Is the logic the same, or is that completely separate? If it's the same (and always should be the same), then you might want to factor that out (and create a function with a descriptive name).

[Ian.M]

From an analytical point of view, what's the difference between the block that deals with counter1, LED1, trigger1_s, trigger1 and the block that deals with counter2, LED2, trigger2_s, trigger2? Is the logic the same, or is that completely separate? If it's the same (and always should be the same), then you might want to factor that out (and create a function with a descriptive name).

OTOH on baseline PICs with only a two level hardware stack and extremely limited RAM, abstraction can be very costly.   Also its impossible to make a pointer to a bit or pin variable, so you would need to use masks to test and manipulate the port bits in question.

Personally I'd probably write a function-like preprocessor macro to generate inline code to perform the duplicated task, but that's probably not advisable for a novice C programmer.  XC8's inline function support historically was disabled in free mode and I'm not sure if that has changed, and a C inline function implementation would still have to deal with pin masks due to the lack of indirect bit access.

[magic]

Originally I was using if (trigger2_s > 0 && trigger2 = 0) but I thought it would be more compact to use &. which did work. I've changed it to && if you think it's better practice.

To put it simply:
4 & 2 = 0, which becomes "false" in conditionals.
4 && 2 = 1, which is "true" because any nonzero integer is "true".

They should be equivalent for variables that can only have a value of 0 or 1, but it's a difference to keep in mind

Other differences are operator precedence, which could affect the meaning of some expressions, and the fact that && doesn't evaluate its right subexpression if the left is false ("short-circuit evaluation"). This is safe (and common) in C:

Code: [Select]

if (ptr != NULL && *ptr > 3) {...}Using & here would crash when ptr is NULL.

[Zero999]

Use an explicit cast to unsigned short in your timeout macros and the warning should go away.

However there's another issue due to the baseline (and midrange) PIC's port architecture - changing one output pin on a port by using GPIObits can cause other output bits on the same port to 'flip'. This issue is generally known in the PIC community  as the 'Read Modify Write effect' ('RMW effect').  See http://picforum.ric323.com/viewtopic.php?f=38&t=12 for details, especially the links to  Stefan Uhlemayr's and John Temples's notes on the subject for reliable work-arounds.

I am aware of that issue, but didn't think it affected my application, since one output should have plenty of time to settle, before the other one changes. I suppose it's possible if both buttons are pressed simultaneously, but it's highly unlikely. Am I mistaken?

If so, I know I could use a shadow register for GPIO, but how do I make one so I can flip individual bits like that? I'm already using shadow bits for the trigger signals, which will be internally assigned to one byte, so it shouldn't make much difference to the memory using, right?

[Ian.M]

Delays between port bit writes can mitigate the problem but not eliminate it.  In a high EMI environment its possible for an output pin to transiently read back incorrectly even though its been in a steady state for a long time.    Close proximity to a mobile phone can be sufficient EMI . . .

Code: [Select]

#define sGPIObits (*(GPIObits_t * volatile)&sGPIO)

volatile unsigned char sGPIO;
declares a shadow variable for the GPIO that you can use exactly as you would individual GPIO pins e.g:

Code: [Select]

sGPIObits.GP2=1;
To update the port from the shadow variable (i.e. when you want an output pin change to take effect) do:

Code: [Select]

GPIO=sGPIO;

[Zero999]

Delays between port bit writes can mitigate the problem but not eliminate it.  In a high EMI environment its possible for an output pin to transiently read back incorrectly even though its been in a steady state for a long time.    Close proximity to a mobile phone can be sufficient EMI . . .

Code: [Select]

#define sGPIObits (*(GPIObits_t * volatile)&sGPIO)

volatile unsigned char sGPIO;
declares a shadow variable for the GPIO that you can use exactly as you would individual GPIO pins e.g:

Code: [Select]

sGPIObits.GP2=1;
To update the port from the shadow variable (i.e. when you want an output pin change to take effect) do:

Code: [Select]

GPIO=sGPIO;

I think it'll be fine in this case, as long as there's good supply filtering and decoupling, but I still want to learn about it so will implement your suggestion.

Can you please explain how the macro sGPIObits works?

[Ian.M]

Can you please explain how the macro sGPIObits works?

Sure, I already did over on the Microchip forums: https://www.microchip.com/forums/FindPost/775994

[Zero999]

Here's the program with your suggestions implemented.

Code: [Select]

/*
 * File:   main.c
 *
 * Created on 14 April 2022, 18:47
 */

//  Turns on an LED for 10 seconds after being triggered by a pushbutton.
//  GP0 = Pushbutton switch 1
//  GP1 = LED1
//  GP3 = Pushbutton switch 2
//  GP4 = LED2
//
// PIC12F509 Configuration Bit Settings

#pragma config OSC = IntRC      // Oscillator Selection bits (internal RC oscillator)
#pragma config WDT = OFF        // Watchdog Timer Enable bit (WDT disabled)
#pragma config CP = OFF         // Code Protection bit (Code protection off)
#pragma config MCLRE = OFF      // GP3/MCLR Pin Function Select bit (GP3/MCLR pin function is digital input, MCLR internally tied to VDD)

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.
#define _XTAL_FREQ 4000000      // oscillator frequency for _delay()

#define sGPIObits (*(GPIObits_t * volatile)&sGPIO)  // shadow port structure

// pin definitions
#define trigger1    GPIObits.GP0
#define trigger1_s  sGPIObits.GP0   // shadow trigger 1
#define LED1        sGPIObits.GP1
#define trigger2    GPIObits.GP3
#define trigger2_s  sGPIObits.GP3   // shadow trigger 2
#define LED2        sGPIObits.GP4

#define timeout1 (int)(10/0.016)         // 10/16ms
#define timeout2 (int)(10/0.016)

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

volatile unsigned char sGPIO = 0;   // shadow register
unsigned short counter1 = 0;        // timer counter 1
unsigned short counter2 = 0;        // timer counter 2
     
void main(void) {
   
    OPTION = 0b10010101;        // configure Timer0:
                                //-0------ enable weak pull-ups on GP0, 1 & 3
                                //--0----- timer mode (T0CS = 0)
                                //----0--- pre scaler assigned to Timer0 (PSA = 0)
                                //-----101 pre scaler = 64 (pre scaler = 101)
                                // -> increment every 64us     
    TRIS = 0b101101;            // configure GP1 & GP4 as an outputs
    GPIO = 0;                   // set all outputs low
    TMR0 = 0;                   // reset timer
   
    for (;;)
    {
        if (TMR0 >= 250)        // timer exceeds 250 * 64us = 16ms
        {
            TMR0 = 0;           // reset timer

            if (counter1)       // decrement counter if not zero
                counter1--;
            else
            {
                if (trigger1_s && !trigger1) // trigger changed from high to low & counter is zero
                {
                    counter1 = timeout1;
                    LED1 = 1;
                }
                else LED1 = 0;
            }
                       
            if (counter2)       // decrement counter if not zero
                counter2--;
            else
            {
                if (trigger2_s && !trigger2) // trigger changed from high to low & counter is zero
                {
                    counter2 = timeout2;
                    LED2 = 1;
                }
                else LED2 = 0;
            }
            GPIO = sGPIO;
            sGPIO = GPIO;
        }
    }
return;
}


[Ian.M]

Ah!
*DON'T* do:

Code: [Select]

            sGPIO = GPIO;as that 'latches' any read-back glitches of the output pins at that instant, which is under a single clock cycle (Tcy/4) since the previous instruction updated them.

Instead, update the shadow input bits individually as you were doing previously, or use masking to avoid disturbing the output bits in sGPIO, e.g. by using my MASKIN() macro (definition and description in reply #5 here: https://www.eevblog.com/forum/microcontrollers/can-anybody-explain-me-why-this-simple-arduino-sketch-doesnt-work-as-expected/msg3991730/#msg3991730 which was originally developed for PICs)
so the input bits update would become:

Code: [Select]

            MASKIN(sGPIO,0b101101,GPIO);leaving the output shadow bits 1 and 4 unchanged.

Note the mask is exactly what you set TRIS to during port initialization, but on baseline PICs TRIS is write only as its an instruction that loads the port's TRIS latches, not a memory mapped register, so I would recommend #defining a name for that binary value then using the name in both the initialization and the above macro's mask.

[Zero999]

Oh I see, the golden rule is never read from an output port. I just defeated the point of using sGPIO.

[Ian.M]

<PEDANT>

Reading from an output port is unavoidable as its impossible to read individual bits! (The BTFSS and BTFSC instructions appear to read a single bit, but actually 'under the hood' read a whole 8 bit register onto the data bus then pick out a single bit using the ALU.)

However the key concept to avoiding soft RMW problems (as opposed to hard RMW problems where the read-modify-write is inherent in the single instruction cycle) is to discard the value of any output pins read back from a PORTx or GPIO register rather than using it to determine future output pin state.

</PEDANT>

OTOH *sometimes* you may actually need to use the read-back value.  Its generally poor engineering to do so, but sometimes in cost constrained products one tries to patch up in software what should have been fixed in hardware.  e.g. if you want to protect (to some degree) a pin against gross overload, and have determined the maximum normal capacitance it will drive, activate the output pin with the desired level, and if after a certain time it doesn't read back the correct level, well its "Houston, we have a problem"  time, and it may be that if you shut the pin off quickly enough you can prevent damage.  Depending on the application, it may then be a case of locking out till power cycled or otherwise reset, or it may be possible to retry after a significant delay to see if the fault clears while keeping the overload duty cycle well under 1%.  Note that depending on temperature and Vdd voltage, such soft overload detection may not 'kick in' before the abs. max. I/O pin current is exceeded, so the risk of damage is reduced, not eliminated, and also that its vulnerable to false triggering due to EMI.

[Zero999]

Here's the updated code.

Code: [Select]

/*
 * File:   main.c
 *
 * Created on 14 April 2022, 18:47
 */

//  Turns on an LED for 10 seconds after being triggered by a pushbutton.
//  GP0 = Pushbutton switch 1
//  GP1 = LED1
//  GP3 = Pushbutton switch 2
//  GP4 = LED2
//
// PIC12F509 Configuration Bit Settings

#pragma config OSC = IntRC      // Oscillator Selection bits (internal RC oscillator)
#pragma config WDT = OFF        // Watchdog Timer Enable bit (WDT disabled)
#pragma config CP = OFF         // Code Protection bit (Code protection off)
#pragma config MCLRE = OFF      // GP3/MCLR Pin Function Select bit (GP3/MCLR pin function is digital input, MCLR internally tied to VDD)

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.
#define _XTAL_FREQ 4000000      // oscillator frequency for _delay()

#define sGPIObits (*(GPIObits_t * volatile)&sGPIO)          // shadow port structure
#define MASKIN(reg,mask,x) (reg = (mask & x)|(~mask & reg)) // mask macro to select which bits to keep

// pin definitions
#define trigger1    GPIObits.GP0
#define trigger1_s  sGPIObits.GP0   // shadow trigger 1
#define LED1        sGPIObits.GP1
#define trigger2    GPIObits.GP3
#define trigger2_s  sGPIObits.GP3   // shadow trigger 2
#define LED2        sGPIObits.GP4
#define TRIS_set    0b101101        // configure GP1 & GP4 as an outputs

#define timeout1 (int)(10/0.016)         // 10/16ms
#define timeout2 (int)(10/0.016)

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

volatile unsigned char sGPIO = 0;   // shadow register
unsigned short counter1 = 0;        // timer counter 1
unsigned short counter2 = 0;        // timer counter 2
     
void main(void) {
   
    OPTION = 0b10010101;        // configure Timer0:
                                //-0------ enable weak pull-ups on GP0, 1 & 3
                                //--0----- timer mode (T0CS = 0)
                                //----0--- pre scaler assigned to Timer0 (PSA = 0)
                                //-----101 pre scaler = 64 (pre scaler = 101)
                                // -> increment every 64us     
    TRIS = TRIS_set;            // configure TRIS
    GPIO = 0;                   // set all outputs low
    TMR0 = 0;                   // reset timer
   
    for (;;)
    {
        if (TMR0 >= 250)        // timer exceeds 250 * 64us = 16ms
        {
            TMR0 = 0;           // reset timer

            if (counter1)       // decrement counter if not zero
                counter1--;
            else
            {
                LED1 = 0;
                if (trigger1_s && !trigger1) // trigger changed from high to low & counter is zero
                {
                    counter1 = timeout1;
                    LED1 = 1;
                }
            }
                       
            if (counter2)       // decrement counter if not zero
                counter2--;
            else
            {
                LED2 = 0;
                if (trigger2_s && !trigger2) // trigger changed from high to low & counter is zero
                {
                    counter2 = timeout2;
                    LED2 = 1;
                }
            }
            GPIO = sGPIO;
            MASKIN (sGPIO,TRIS_set,GPIO);
        }
    }
return;
}
I made a couple of optimisations which save a tiny bit of code space.

I moved the LED = 0; statements to before the if blocks. This shouldn't cause problems with the LEDs flickering off because they now refer to shadow registers, which are updated later.

I changed:

Code: [Select]

#define MASKIN(reg,mask,x) (reg^=(reg^(x))&(mask))
To:

Code: [Select]

#define MASKIN(reg,mask,x) (reg = (mask & x)|(~mask & reg))
Simply because it was more intuitive to me and discovered it saved a word of program memory. I've no idea why this should be. I thought it would take up more space because it's a longer expression, but it must be good luck it generates more compact code.

C seems to be very nitpicky about  spaces. I tried adding spaces in the macro and it refused to compile. Is there a rule about where spaces can and cannot go?

[free_electron]

<PEDANT>

Reading from an output port is unavoidable as its impossible to read individual bits!

ALWAYS shadow your output. Keep a copy of the port in ram , twiddle bits in ram , then send the byte to the port. it prevents glitches and issues with hardware. It also solves misery with devices where the input and output are physically different registers.

[Ian.M]

Your rewrite of MASKIN is no longer thread-safe.  If an ISR uses it to change or more bits in a variable or SFR and the ISR hits in the middle of MASKIN's execution changing different bits in the main program, the bits being preserved by (~mask & reg) get stored in a temporary 'scratchpad' RAM location and  can overwrite the bits changed by the ISR!    The whole point of XOR masking was to preserve the other bits without them going through 'scratchpad' RAM by updating the result using =^ which on 8 bit PICs for an 8 bit variable or SFR compiles as a single XORWF instruction so cant get split by an intterupt.

They will work identically in this program, but if you reuse the macro in the future on  midrange or PIC18 parts (that have interrupts), beware of the non-XOR version.

[Zero999]

Your rewrite of MASKIN is no longer thread-safe.  If an ISR uses it to change or more bits in a variable or SFR and the ISR hits in the middle of MASKIN's execution changing different bits in the main program, the bits being preserved by (~mask & reg) get stored in a temporary 'scratchpad' RAM location and  can overwrite the bits changed by the ISR!    The whole point of XOR masking was to preserve the other bits without them going through 'scratchpad' RAM by updating the result using =^ which on 8 bit PICs for an 8 bit variable or SFR compiles as a single XORWF instruction so cant get split by an intterupt.

They will work identically in this program, but if you reuse the macro in the future on  midrange or PIC18 parts (that have interrupts), beware of the non-XOR version.

Sorry I don't understand much of what you've said. I thought there's only one thread, so it should be fine. Please expand those initialisms.

How did you come up with that MASKIN macro? I did it the old way with a pen and paper. I even stuck it into a spreadsheet to check.

[Ian.M]

Sorry I don't understand much of what you've said. I thought there's only one thread, so it should be fine. Please expand those initialisms.

ISR = Interrupt Service Routine
SFR = Special Function Register - a 'Microchip'ism for memory mapped registers that control hardware, as opposed to GPR (General Purpose Register) which the rest of the world calls RAM!

Yes, there's only one thread as the PIC12F509 uses the 'classic' 12 bit Baseline core, which doesn't have interrupts. In the future if you use a Midrange ('classic' 14 bit core) or Enhanced Midrange ('new' 14 bit core) PIC12/16 or a PIC18 which have interrupts, your main program's execution may be paused at any point between instructions to let the ISR run.  A PIC18 (apart from some new ones with vectored interrupts) can have two levels of interrupt priority so your low priority ISR can itself get paused while the high priority ISR runs.

How did you come up with that MASKIN macro? I did it the old way with a pen and paper. I even stuck it into a spreadsheet to check.

I stood on the shoulders of Giants!  https://graphics.stanford.edu/~seander/bithacks.html#MaskedMerge

[Zero999]

Sorry I don't understand much of what you've said. I thought there's only one thread, so it should be fine. Please expand those initialisms.

ISR = Interrupt Service Routine
SFR = Special Function Register - a 'Microchip'ism for memory mapped registers that control hardware, as opposed to GPR (General Purpose Register) which the rest of the world calls RAM!

Yes, there's only one thread as the PIC12F509 uses the 'classic' 12 bit Baseline core, which doesn't have interrupts. In the future if you use a Midrange ('classic' 14 bit core) or Enhanced Midrange ('new' 14 bit core) PIC12/16 or a PIC18 which have interrupts, your main program's execution may be paused at any point between instructions to let the ISR run.  A PIC18 (apart from some new ones with vectored interrupts) can have two levels of interrupt priority so your low priority ISR can itself get paused while the high priority ISR runs.

That makes sense. In other words, if the program is interrupted in the middle of the masking part, bits can be changed in the accumulator, which interfere with the result.

How did you come up with that MASKIN macro? I did it the old way with a pen and paper. I even stuck it into a spreadsheet to check.

I stood on the shoulders of Giants!  https://graphics.stanford.edu/~seander/bithacks.html#MaskedMerge

There's some very clever hacks there.

Merge bits from two values according to a mask

Code: [Select]

unsigned int a;    // value to merge in non-masked bits
unsigned int b;    // value to merge in masked bits
unsigned int mask; // 1 where bits from b should be selected; 0 where from a.
unsigned int r;    // result of (a & ~mask) | (b & mask) goes here

r = a ^ ((a ^ b) & mask);
This shaves one operation from the obvious way of combining two sets of bits according to a bit mask. If the mask is a constant, then there may be no advantage.

The question is, it says it's quicker, but how can they be sure? In order to know that, they need to be sure of the architecture and the compiler. I find it odd the obvious solution I came up was is slightly smaller.

Another question is what's the best way of implementing the watchdog timer? I wouldn't want it to lock up with both outputs on. Is there a way to know it's reset due to the watchdog timer tripping, do it can go into a safe mode i.e. setting all IO ports to inputs?


I've made it so the program resets the watchdog timer every 8ms. The PIC12F509 has to have the WDT reset every 9ms, so I changed the pre scaler on the timer counter to 32, rather than 64, so it now decrements the counter every 32µs*250 = 8ms, which is also when the WDT is reset.

Code: [Select]

/*
 * File:   main.c
 *
 * Created on 14 April 2022, 18:47
 */

//  Turns on an LED for 10 seconds after being triggered by a pushbutton.
//  GP0 = Pushbutton switch 1
//  GP1 = LED1
//  GP3 = Pushbutton switch 2
//  GP4 = LED2
//
// PIC12F509 Configuration Bit Settings

#pragma config OSC = IntRC      // Oscillator Selection bits (internal RC oscillator)
#pragma config WDT = ON         // Watchdog Timer Enable bit (WDT disabled)
#pragma config CP = OFF         // Code Protection bit (Code protection off)
#pragma config MCLRE = OFF      // GP3/MCLR Pin Function Select bit (GP3/MCLR pin function is digital input, MCLR internally tied to VDD)

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.
#define _XTAL_FREQ 4000000      // oscillator frequency for _delay()

#define sGPIObits (*(GPIObits_t * volatile)&sGPIO)          // shadow port structure
#define MASKIN(reg,mask,x) (reg = (mask & x)|(~mask & reg)) // mask macro to select which bits to keep. Not thread safe but smaller.
//#define MASKIN(reg,mask,x) (reg^=(reg^(x))&(mask))        // mask macro. Slightly larger binary code, but thread safe.

// pin definitions
#define trigger1    GPIObits.GP0
#define trigger1_s  sGPIObits.GP0   // shadow trigger 1
#define LED1        sGPIObits.GP1
#define trigger2    GPIObits.GP3
#define trigger2_s  sGPIObits.GP3   // shadow trigger 2
#define LED2        sGPIObits.GP4
#define TRIS_set    0b101101        // configure GP1 & GP4 as outputs

#define timeout1 (int)(10/0.008)         // 10/8ms
#define timeout2 (int)(10/0.008)

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

volatile unsigned char sGPIO = 0;   // shadow register
unsigned short counter1 = 0;        // timer counter 1
unsigned short counter2 = 0;        // timer counter 2
     
void main(void) {
   
    OPTION = 0b10010100;        // configure Timer0:
                                //-0------ enable weak pull-ups on GP0, 1 & 3
                                //--0----- timer mode (T0CS = 0)
                                //----0--- pre scaler assigned to Timer0 (PSA = 0)
                                //-----100 pre scaler = 32 (pre scaler = 100)
                                // -> increment every 32us     
    TRIS = TRIS_set;            // configure TRIS
    GPIO = 0;                   // set all outputs low
    TMR0 = 0;                   // reset timer
   
    for (;;)
    {
        if (TMR0 >= 250)        // timer exceeds 250 * 32us = 8ms
        {
            CLRWDT();           // clear watchdog timer
            TMR0 = 0;           // reset timer

            if (counter1)       // decrement counter if not zero
                counter1--;
            else
            {
                LED1 = 0;
                if (trigger1_s && !trigger1) // trigger changed from high to low & counter is zero
                {
                    counter1 = timeout1;
                    LED1 = 1;
                }
            }
                       
            if (counter2)       // decrement counter if not zero
                counter2--;
            else
            {
                LED2 = 0;
                if (trigger2_s && !trigger2) // trigger changed from high to low & counter is zero
                {
                    counter2 = timeout2;
                    LED2 = 1;
                }
            }
            GPIO = sGPIO;
            MASKIN (sGPIO,TRIS_set,GPIO);
        }
    }
return;
}

[Ian.M]

They *think* its quicker when working with variables because (a & ~mask) | (b & mask) takes four Boolean operations whereas a ^ ((a ^ b) & mask) only takes three.  However as they note, if mask is a constant, and the compiler can determine its value, it will probably precompute ~mask reducing it to only three Boolean operations at runtime.

*ALL* resets, including WDT timeouts, set all available I/O pins to inputs.  If you want to do something special after a watchdog reset, you must examine the STATUS register bit /TO, which if zero indicates the last reset was due to the WDT, *BEFORE* any other code clobbers the STATUS register.  (See PIC12F509 datasheet sections '4.4 STATUS Register' and '7.6 Watchdog Timer (WDT)')

Unfortunately the XC8 startup code clobbers the STATUS register, so there is a special feature to preserve its value on reset for use in your code.  See: XC8 C Compiler User’s Guide for PIC, section '5.9.2.4 Status Register Preservation', and if you want to preserve any state across WDT resets, see section '5.3.9.5 Persistent Type Qualifier'. Note that __persistent variables must be explicitly assigned a 'sane' value in your code after powerup so you need to determine the type of the last reset from the preserved STATUS register and if it wasn't WDT (or SLEEP), initialize (load) all __persistent variables.

[Zero999]

I've figured out how to use the watchdog timer, but the program is still not doing what I expect it to.

The idea is to pulse GPIO2 and GPIO5 high for 200ms, low for 800ms, continuously to indicate an error has occured, if the WDT trips. What's weird is only GPIO5 pulses. I've checked the IO pin still works with another program, so it's not that. It doesn't matter for this application, but I want to understand why it's not pulsing GPIO2. I'm probably doing something stupid.

Here's the code.

Code: [Select]

/*
 * File:   main.c
 *
 * Created on 14 April 2022, 18:47
 */

//  Turns on an LED for 10 seconds after being triggered by a pushbutton.
//  GP0 = Pushbutton switch 1
//  GP1 = LED1
//  GP2 = Error
//  GP3 = Pushbutton switch 2
//  GP4 = LED2
//  GP5 = Error
//
// PIC12F509 Configuration Bit Settings

#pragma config OSC = IntRC      // Oscillator Selection bits (internal RC oscillator)
#pragma config WDT = ON         // Watchdog Timer Enable bit (WDT disabled)
#pragma config CP = OFF         // Code Protection bit (Code protection off)
#pragma config MCLRE = OFF      // GP3/MCLR Pin Function Select bit (GP3/MCLR pin function is digital input, MCLR internally tied to VDD)

// #pragma config statements should precede project file includes.
// Use project enums instead of #define for ON and OFF.
#define _XTAL_FREQ 4000000      // oscillator frequency for _delay()

#define sGPIObits (*(GPIObits_t * volatile)&sGPIO)          // shadow port structure
#define MASKIN(reg,mask,x) (reg = (mask & x)|(~mask & reg)) // mask macro to select which bits to keep. Not thread safe but smaller.
//#define MASKIN(reg,mask,x) (reg^=(reg^(x))&(mask))        // mask macro. Slightly larger binary code, but thread safe.

// pin definitions
#define trigger1    GPIObits.GP0
#define trigger1_s  sGPIObits.GP0   // shadow trigger 1
#define LED1        sGPIObits.GP1
#define trigger2    GPIObits.GP3
#define trigger2_s  sGPIObits.GP3   // shadow trigger 2
#define LED2        sGPIObits.GP4
#define TRIS_set    0b101101        // configure GP1 & GP4 as outputs
#define TRIS_error  0b011011        // if WDT GP2 & GP5 are error outputs, which pulse 200ms at 1Hz
#define timeout1 (int)(10/0.008)    // 10/8ms
#define timeout2 (int)(10/0.008)

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

volatile unsigned char sGPIO = 0;   // shadow register
unsigned short counter1 = 0;        // timer counter 1
unsigned short counter2 = 0;        // timer counter 2
     
void main(void) { 
    if (!__timeout)             // If the watchdog timer healthy flag is not set
    {

        TRIS = TRIS_error;
        for (;;)                // pulse error outputs
        {       
            GPIO = 0b111111;        // turn on error outputs
            __delay_ms(200);        // stay on for 200 ms
            GPIO = 0;               // turn off error outputs
            __delay_ms(800);         // stay off for 800 ms
        }       
    }
   
    OPTION = 0b10010100;        // configure Timer0:
                                //-0------ enable weak pull-ups on GP0, 1 & 3
                                //--0----- timer mode (T0CS = 0)
                                //----0--- pre scaler assigned to Timer0 (PSA = 0)
                                //-----100 pre scaler = 32 (pre scaler = 100)
                                // -> increment every 32us 
    TRIS = TRIS_set;            // configure TRIS
    GPIO = 0;                   // set all outputs low
    TMR0 = 0;                   // reset timer
             
    for (;;)
    {
        if (TMR0 >= 250)        // timer exceeds 250 * 32us = 8ms
        {
//            CLRWDT();           // clear watchdog timer
            TMR0 = 0;           // reset timer

            if (counter1)       // decrement counter if not zero
                counter1--;
            else
            {
                LED1 = 0;
                if (trigger1_s && !trigger1) // trigger changed from high to low & counter is zero
                {
                    counter1 = timeout1;
                    LED1 = 1;
                }
            }
                       
            if (counter2)       // decrement counter if not zero
                counter2--;
            else
            {
                LED2 = 0;
                if (trigger2_s && !trigger2) // trigger changed from high to low & counter is zero
                {
                    counter2 = timeout2;
                    LED2 = 1;
                }
            }
            GPIO = sGPIO;
            MASKIN (sGPIO,TRIS_set,GPIO);
        }
    }
return;
}

[Ian.M]

Baseline PICs have some quirks - TMR0 has grabbed the GP2/T0CKI pin!  See datasheet section '4.5 OPTION Register', specifically the note for bit 5 T0CS.  You also aren't resetting the watchdog in the error loop so I bet the pulse rate isn't what it should be!

Try this:

Code: [Select]

    if (!__timeout)             // If the watchdog timer healthy flag is not set
    {

        OPTION=0b11011111; //disable T0CKI, other bits POR default
        TRIS = TRIS_error;
        for (;;)                // pulse error outputs
        {       
            GPIO = 0b111111;        // turn on error outputs
            __delaywdt_ms(200);        // stay on for 200 ms, kicking the dog
            GPIO = 0;               // turn off error outputs
            __delaywdt_ms(800);         // stay off for 800 ms, kicking the dog
        }       
    }