Hi.

I'm in the beginning of my journey to learn Hi-Tech C, MPLAB, and PIC MCU programming. I've spent some time trying to make "Yet another LED flasher".

I would be very thankful if someone could provide me with some comments on my program.

I've put a lot of comments in, so that I might remember them over time.

/* 
 
Program:        main.c 
Description:    Blinking LED according to pushed buttons 
PIC:            16F628 
IDE:            MPLAB 
Compiler:        Hi - Tech C 
Date:            Oct 2010 
Author:            Jens Christoffersen 
web:            www.nerdegutta.org 
 
*/ 
 
#include <htc.h>        // Header file for PIC 16F6xx 
#include "..\delay.c"    // Needed for DelayMs() - function 
 
/* Configuration */ 
 
__CONFIG    (WDTDIS &     // Disable watchdog 
            PWRTEN &     // Disable power up timer 
            MCLREN &    // Master Clear Reset enable 
            BOREN &        // Enabale Brown out reset 
            LVPDIS  &    // Disable low voltage programming     
            DATUNPROT &    // Unprotect data code 
            UNPROTECT & // Unprotect the program code 
            INTIO);        // Use internal RC oscillator 
 
 
/* Functions start here */ 
void blinkAllLEDS() 
{ 
    PORTB = 0b11111111;    // Turning all LEDs on 
    DelayMs(5);            // Wait 5 ms 
    PORTB = 0b00000000;    // Turning all LEDs off 
    DelayMs(5);            // Wait 5 ms 
}                        // End blinkAllLEDS 
 
void goLeft()             
{ 
int counterPortB = 1;                    // Declaring counterPortB as integer variable with the value of 1 
    while (counterPortB < 255)            // While counterPortB is less than 255 do... 
    { 
    PORTB = counterPortB;                // Sending the value of counterPortB to port b 
    DelayMs(10);                        // Wait 10 ms 
    PORTB = 0;                            // Turning all bits LOW 
    DelayMs(10);                        // Wait 5 ms 
    counterPortB = counterPortB * 2;    // For each run, counterPortB is multiplied by 2 
    } 
counterPortB = 1;                        // Setting counterPortB to 1 
 
}                                        // End goLeft 
 
void goRight() 
{ 
int counterPortB = 256;                    // Declaring counterPortB as integer variable with the value of 256 
    while (counterPortB > 0)            // While counterPortB is bigger than 0 do... 
    { 
    PORTB = counterPortB;                // Sending the value of counterPortB to port b 
    DelayMs(10);                        // Wait 10 ms 
    PORTB = 0;                            // Turning all bits LOW 
    DelayMs(10);                        // Wait 10 ms 
    counterPortB = counterPortB / 2;    // For each run, counterPortB divided by 2 
} 
counterPortB = 256;                        // Sett counterPortB to the value of 256 
}                                        // End goLeft 
 
void goLeftRight() 
{ 
    goLeft(); 
    goRight(); 
} 
 
/* Start with main program */ 
void main() 
{ 
TRISA = 0b11111111;        // Setting all bits on port a to input 
TRISB = 0b00000000;        // Setting all bits on port b to output 
 
PORTA = 0b00000000;        // Setting all bits on port a to LOW 
PORTB = 0b00000000;        // Setting all bits on port b to LOW 
 
CMCON = 0x07;            // Disabling the analogue comparators 
 
while (1)                // Start endless lopp 
{ 
    if (RA0 == 1)        // Checks if bit RA0 is 1 -> button pressed. 
    {                    // Start if-statement 
    while (1)            // Start while loop 
    {                    // Starting braces 
        blinkAllLEDS();    // Calling the blinkAllLEDS function 
    }                    // End while loop 
    }                    // End if-statement 
     
    if (RA1 == 1) 
    { 
    while (1) 
    { 
        goLeft(); 
    } 
    } 
     
    if (RA2 == 1) 
    { 
    while (1) 
    { 
        goRight(); 
    } 
    } 
     
    if (RA3 == 1) 
    { 
    while (1) 
    { 
        goLeftRight(); 
    } 
    } 
 
}                        // End while-loop 
 
}                        // End main function

I know all the IF-statements in main() could be done with switch - case statement, but I'm still learning.

A small drawback is that the PIC needs to be reset before another bit can be checked for HIGH.

When it come to the TRISA, TRISB, PORTA, PORTB and CMCON I'm a little concerned if I've got it right...

Would be great with some comments.

 

Are you using a PIC16628 or a 16f628A? This is important. The 16f628 do not have internal OSC, the 16f628A do have internal OSC. To me it look like you have mixed functions from the 16f628 and 16f628a. I recommend all beginners to use internal OSC if the chip has that function. On a 16f628A then selecting internal OSC it will default to 4MHz
Also hitech C have native delay functions __delay_ms, __delay_us, and _delay(). Using __delay_ms and __delay_us requires a
#define _XTAL_FREQ 4000000 //tell the compiler that a 4MHz clock source is used

 

Thanks for reply.

It says: "PIC 16F628-20I/P" on the chip.

I thought it had an internal osc:

http://www.microchip.com/wwwproducts/ProductCompare.aspx?product1=PIC16F628&product2=PIC16F628A

Copied from datasheet:
"The PIC16F62X has eight oscillator configurations.
The single pin ER oscillator provides a low cost solu-
tion. The LP oscillator minimizes power consumption,
XT is a standard crystal, INTRC is a self-contained
internal oscillator. The HS is for High Speed crystals.
The EC mode is for an external clock source."


When I'm searching for the datasheet, I always get the PIC 16F62x - which led me to think that my PIC had an internal 4MHz RC oscillator.

Am I wrong? Or just plain confused?

 

Thanks for reply.

It says: "PIC 16F628-20I/P" on the chip.

I thought it had an internal osc:

http://www.microchip.com/wwwproducts/ProductCompare.aspx?product1=PIC16F628&product2=PIC16F628A

When I'm searching for the datasheet, I always get the PIC 16F62x - which led me to think that my PIC had an internal 4MHz RC oscillator.

Am I wrong? Or just plain confused?

In this case it was me. Both wrong and plain confused. I misread the 16f628 data sheet. The 16f628 do have an internal 4MHz osc. The 16f628 is older so they used another wording style that I am used to.
Your setting of CMON and the TRISB and and TRISA registers are correct for your application. You can also mix input and output on a port. Like setting half of PORTB to input and the other half to be input. Or any other mix. However you may not be able to use all pins as both input and output pins. This will be explained in the datasheet.
You may also write

 if (RA1)

instead of

 if (RA1 == 1)

But I checked and the compiler makes the same code. So use the style you prefer

 

I didn't know about the native _delay(). Thanks!

I think I prefer:

if (RA0 == 1)

for now. For me the readability is important.

 

The PIC16F628 does indeed have a 4MHz internal clock just like the PIC16F628A. What it lacks is the optional 48KHz internal clock but unless you are into low power applications that is not going to worry you all that much.

As to your program, I see no real problems or oversights but a couple of small points.
First off comments. I realise that you are just starting out but comments such as

int counterPortB = 1;   // Declaring counterPortB as integer variable with the value of 1

are pretty much useless. C code tends to be self-documenting, the command statement says what it does. The comment should give the reader some idea of why you are doing it. By the way, why are you doing it and why does counterPortB need to be an int?
Since it is never going to be greater than 255 why not declare it a char and save yourself a byte of RAM. PICs are never over-endowed with RAM so try to use the smallest class of variable that will do the job. Also using unsigned variables will produce smaller, faster code than using signed variables.

You did a reasonable job of formatting your code but did loose your way towards the end (same thing happens to me all the time). However properly formatted code makes it easier to see where the blocks start and end and which closing braces go with which opening braces so I took the liberty of fixing up your formatting.

/* 
 
Program:        main.c 
Description:    Blinking LED according to pushed buttons 
PIC:            16F628 
IDE:            MPLAB 
Compiler:        Hi - Tech C 
Date:            Oct 2010 
Author:            Jens Christoffersen 
web:            www.nerdegutta.org 
 
*/ 
 
#include <htc.h>        // Header file for PIC 16F6xx 
#include "..\delay.c"    // Needed for DelayMs() - function 
 
/* Configuration */ 
 
__CONFIG    (WDTDIS &     // Disable watchdog 
            PWRTEN &     // Disable power up timer 
            MCLREN &    // Master Clear Reset enable 
            BOREN &        // Enabale Brown out reset 
            LVPDIS  &    // Disable low voltage programming     
            DATUNPROT &    // Unprotect data code 
            UNPROTECT & // Unprotect the program code 
            INTIO);        // Use internal RC oscillator 
 
 
/* Functions start here */ 
void blinkAllLEDS() 
{ 
    PORTB = 0b11111111;    // Turning all LEDs on 
    DelayMs(5);            // Wait 5 ms 
    PORTB = 0b00000000;    // Turning all LEDs off 
    DelayMs(5);            // Wait 5 ms 
}                        // End blinkAllLEDS 
 
void goLeft()             
{ 
    int counterPortB = 1;                  // Declaring counterPortB as integer variable with the value of 1 
    while (counterPortB < 255)            // While counterPortB is less than 255 do... 
    { 
        PORTB = counterPortB;                // Sending the value of counterPortB to port b 
        DelayMs(10);                        // Wait 10 ms 
        PORTB = 0;                            // Turning all bits LOW 
        DelayMs(10);                        // Wait 5 ms 
        counterPortB = counterPortB * 2;    // For each run, counterPortB is multiplied by 2 
    } 
    counterPortB = 1;                        // Setting counterPortB to 1 
 
}                                        // End goLeft 
 
void goRight() 
{ 
    int counterPortB = 256;                    // Declaring counterPortB as integer variable with the value of 256 
    while (counterPortB > 0)            // While counterPortB is bigger than 0 do... 
    { 
        PORTB = counterPortB;                // Sending the value of counterPortB to port b 
        DelayMs(10);                        // Wait 10 ms 
        PORTB = 0;                            // Turning all bits LOW 
        DelayMs(10);                        // Wait 10 ms 
        counterPortB = counterPortB / 2;    // For each run, counterPortB divided by 2 
    } 
    counterPortB = 256;                        // Sett counterPortB to the value of 256 
}                                        // End goLeft 
 
void goLeftRight() 
{ 
    goLeft(); 
    goRight(); 
} 
 
/* Start with main program */ 
void main() 
{ 
    TRISA = 0b11111111;        // Setting all bits on port a to input 
    TRISB = 0b00000000;        // Setting all bits on port b to output 
     
    PORTA = 0b00000000;        // Setting all bits on port a to LOW 
    PORTB = 0b00000000;        // Setting all bits on port b to LOW 
     
    CMCON = 0x07;            // Disabling the analogue comparators 
     
    while (1)                // Start endless lopp 
    { 
        if (RA0 == 1)        // Checks if bit RA0 is 1 -> button pressed. 
        {                    // Start if-statement 
            while (1)            // Start while loop 
            {                    // Starting braces 
                blinkAllLEDS();    // Calling the blinkAllLEDS function 
            }                    // End while loop 
        }                    // End if-statement 
     
        if (RA1 == 1) 
        { 
            while (1) 
            { 
                goLeft(); 
            } 
        } 
         
        if (RA2 == 1) 
        { 
           while (1) 
            { 
                goRight(); 
            } 
        } 
         
        if (RA3 == 1) 
        { 
            while (1) 
            { 
                goLeftRight(); 
            } 
        } 
 
    }                        // End while-loop 
 
}                        // End main function

One last point, are you aware that if your code hits one of the while(1) statements (highlighted in red near the end of the file) your program is going to get stuck there for ever (until reset) so for example if RA1 goes high you will never get out of doing goRight(). But maybe this is what you intended. Without comments saying why you are doing it we don't know.

 

I don't know much about C......
There are a couple of times when you have set variables to 256. While not a problem in C, it means that you are using 2 bytes instead of 1 and the code generated will be slower as a result. If you can use 255 (the limit for an 8 bit number) it might be more efficient.
Of course it's possible that int means 8 bit, in which case it probably converts the 256 to 0 when it complies.

 

Thanks guys.

AlexR:
I see your point regarding my commenting.

Why counterPortB is an int... Well, just because I thought it had to be. I thought char was just letters a-z. Now I learn. Nice!

Regarding the while(1)-loop. I'm aware that when it it enters this loop, it needs to be reset/ power on/off...


Markd77:
I tried to use 255, but then the LEDs did not behave like I wanted.
I think its simple math:
I need to send some values to the bits on port b. To light bit 0, I'm sending a 1 to that bit. So to light all the bits I send 255 to PORTB. I do it in a loop that multiplies 1 with 2, until it reaches 255.

But to reverse the sequence I need to divide it by 2. 255 / 2 = 127.5 which is a number the PIC do not understand. That is why I send the 256.

256 / 2 = 128
128 / 2 = 64
64 / 2 = 32

You get the idea...

But thanks for pointing that out for me.

 

Use shift operators. If you shift left you multiply with 2, right shift will divide by two. Check the manual for details. I can not look it up for you as I am sitting on wrong computer

 

Shift operators... Hmm. Adding new words to my vocabulary. Thanks I will google it... Like this.

 

Also then working on port levels try to use the unsigned char type. And as beginner you are just doing fine

 

I think I see the aim:
10000000
01000000
00100000
etc.
I think you just need to load the value 128 in at the start. It's quite possible that C compiles the shift operator if it sees *2 or /2

 

unsigned char.... Learning a lot.

And as beginner you are just doing fine

- thank you.

... reading about shift operators ...

 

I have fixed the Goleft function. Also look how I use the debug tools in MPLAB SIM.

 

Nice!

Thanks a lot.

I really really really appreciate it!