Comments on Yet another LED flasher

Discussion in 'Programmer's Corner' started by nerdegutta, Oct 17, 2010.

  1. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    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.


    Code ( (Unknown Language)):
    1.  
    2. /*
    3.  
    4. Program:        main.c
    5. Description:    Blinking LED according to pushed buttons
    6. PIC:            16F628
    7. IDE:            MPLAB
    8. Compiler:        Hi - Tech C
    9. Date:            Oct 2010
    10. Author:            Jens Christoffersen
    11. web:            www.nerdegutta.org
    12.  
    13. */
    14.  
    15. #include <htc.h>        // Header file for PIC 16F6xx
    16. #include "..\delay.c"    // Needed for DelayMs() - function
    17.  
    18. /* Configuration */
    19.  
    20. __CONFIG    (WDTDIS &     // Disable watchdog
    21.             PWRTEN &     // Disable power up timer
    22.             MCLREN &    // Master Clear Reset enable
    23.             BOREN &        // Enabale Brown out reset
    24.             LVPDIS  &    // Disable low voltage programming    
    25.             DATUNPROT &    // Unprotect data code
    26.             UNPROTECT & // Unprotect the program code
    27.             INTIO);        // Use internal RC oscillator
    28.  
    29.  
    30. /* Functions start here */
    31. void blinkAllLEDS()
    32. {
    33.     PORTB = 0b11111111;    // Turning all LEDs on
    34.     DelayMs(5);            // Wait 5 ms
    35.     PORTB = 0b00000000;    // Turning all LEDs off
    36.     DelayMs(5);            // Wait 5 ms
    37. }                        // End blinkAllLEDS
    38.  
    39. void goLeft()            
    40. {
    41. int counterPortB = 1;                    // Declaring counterPortB as integer variable with the value of 1
    42.     while (counterPortB < 255)            // While counterPortB is less than 255 do...
    43.     {
    44.     PORTB = counterPortB;                // Sending the value of counterPortB to port b
    45.     DelayMs(10);                        // Wait 10 ms
    46.     PORTB = 0;                            // Turning all bits LOW
    47.     DelayMs(10);                        // Wait 5 ms
    48.     counterPortB = counterPortB * 2;    // For each run, counterPortB is multiplied by 2
    49.     }
    50. counterPortB = 1;                        // Setting counterPortB to 1
    51.  
    52. }                                        // End goLeft
    53.  
    54. void goRight()
    55. {
    56. int counterPortB = 256;                    // Declaring counterPortB as integer variable with the value of 256
    57.     while (counterPortB > 0)            // While counterPortB is bigger than 0 do...
    58.     {
    59.     PORTB = counterPortB;                // Sending the value of counterPortB to port b
    60.     DelayMs(10);                        // Wait 10 ms
    61.     PORTB = 0;                            // Turning all bits LOW
    62.     DelayMs(10);                        // Wait 10 ms
    63.     counterPortB = counterPortB / 2;    // For each run, counterPortB divided by 2
    64. }
    65. counterPortB = 256;                        // Sett counterPortB to the value of 256
    66. }                                        // End goLeft
    67.  
    68. void goLeftRight()
    69. {
    70.     goLeft();
    71.     goRight();
    72. }
    73.  
    74. /* Start with main program */
    75. void main()
    76. {
    77. TRISA = 0b11111111;        // Setting all bits on port a to input
    78. TRISB = 0b00000000;        // Setting all bits on port b to output
    79.  
    80. PORTA = 0b00000000;        // Setting all bits on port a to LOW
    81. PORTB = 0b00000000;        // Setting all bits on port b to LOW
    82.  
    83. CMCON = 0x07;            // Disabling the analogue comparators
    84.  
    85. while (1)                // Start endless lopp
    86. {
    87.     if (RA0 == 1)        // Checks if bit RA0 is 1 -> button pressed.
    88.     {                    // Start if-statement
    89.     while (1)            // Start while loop
    90.     {                    // Starting braces
    91.         blinkAllLEDS();    // Calling the blinkAllLEDS function
    92.     }                    // End while loop
    93.     }                    // End if-statement
    94.      
    95.     if (RA1 == 1)
    96.     {
    97.     while (1)
    98.     {
    99.         goLeft();
    100.     }
    101.     }
    102.      
    103.     if (RA2 == 1)
    104.     {
    105.     while (1)
    106.     {
    107.         goRight();
    108.     }
    109.     }
    110.      
    111.     if (RA3 == 1)
    112.     {
    113.     while (1)
    114.     {
    115.         goLeftRight();
    116.     }
    117.     }
    118.  
    119. }                        // End while-loop
    120.  
    121. }                        // End main function
    122.  
    123.  
    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... :confused:

    Would be great with some comments.
     
  2. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    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
     
  3. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    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?
     
  4. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    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
    Code ( (Unknown Language)):
    1.  if (RA1)
    instead of
    Code ( (Unknown Language)):
    1.  if (RA1 == 1)
    But I checked and the compiler makes the same code. So use the style you prefer
     
  5. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    I didn't know about the native _delay(). Thanks!

    I think I prefer:

    Code ( (Unknown Language)):
    1.  
    2. if (RA0 == 1)
    3.  
    for now. For me the readability is important.
     
  6. AlexR

    Well-Known Member

    Jan 16, 2008
    735
    54
    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
    Code ( (Unknown Language)):
    1. 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.
    Code ( (Unknown Language)):
    1. /*
    2.  
    3. Program:        main.c
    4. Description:    Blinking LED according to pushed buttons
    5. PIC:            16F628
    6. IDE:            MPLAB
    7. Compiler:        Hi - Tech C
    8. Date:            Oct 2010
    9. Author:            Jens Christoffersen
    10. web:            [URL="http://www.nerdegutta.org"]www.nerdegutta.org[/URL]
    11.  
    12. */
    13.  
    14. #include <htc.h>        // Header file for PIC 16F6xx
    15. #include "..\delay.c"    // Needed for DelayMs() - function
    16.  
    17. /* Configuration */
    18.  
    19. __CONFIG    (WDTDIS &     // Disable watchdog
    20.             PWRTEN &     // Disable power up timer
    21.             MCLREN &    // Master Clear Reset enable
    22.             BOREN &        // Enabale Brown out reset
    23.             LVPDIS  &    // Disable low voltage programming    
    24.             DATUNPROT &    // Unprotect data code
    25.             UNPROTECT & // Unprotect the program code
    26.             INTIO);        // Use internal RC oscillator
    27.  
    28.  
    29. /* Functions start here */
    30. void blinkAllLEDS()
    31. {
    32.     PORTB = 0b11111111;    // Turning all LEDs on
    33.     DelayMs(5);            // Wait 5 ms
    34.     PORTB = 0b00000000;    // Turning all LEDs off
    35.     DelayMs(5);            // Wait 5 ms
    36. }                        // End blinkAllLEDS
    37.  
    38. void goLeft()            
    39. {
    40.     int counterPortB = 1;                  // Declaring counterPortB as integer variable with the value of 1
    41.     while (counterPortB < 255)            // While counterPortB is less than 255 do...
    42.     {
    43.         PORTB = counterPortB;                // Sending the value of counterPortB to port b
    44.         DelayMs(10);                        // Wait 10 ms
    45.         PORTB = 0;                            // Turning all bits LOW
    46.         DelayMs(10);                        // Wait 5 ms
    47.         counterPortB = counterPortB * 2;    // For each run, counterPortB is multiplied by 2
    48.     }
    49.     counterPortB = 1;                        // Setting counterPortB to 1
    50.  
    51. }                                        // End goLeft
    52.  
    53. void goRight()
    54. {
    55.     int counterPortB = 256;                    // Declaring counterPortB as integer variable with the value of 256
    56.     while (counterPortB > 0)            // While counterPortB is bigger than 0 do...
    57.     {
    58.         PORTB = counterPortB;                // Sending the value of counterPortB to port b
    59.         DelayMs(10);                        // Wait 10 ms
    60.         PORTB = 0;                            // Turning all bits LOW
    61.         DelayMs(10);                        // Wait 10 ms
    62.         counterPortB = counterPortB / 2;    // For each run, counterPortB divided by 2
    63.     }
    64.     counterPortB = 256;                        // Sett counterPortB to the value of 256
    65. }                                        // End goLeft
    66.  
    67. void goLeftRight()
    68. {
    69.     goLeft();
    70.     goRight();
    71. }
    72.  
    73. /* Start with main program */
    74. void main()
    75. {
    76.     TRISA = 0b11111111;        // Setting all bits on port a to input
    77.     TRISB = 0b00000000;        // Setting all bits on port b to output
    78.      
    79.     PORTA = 0b00000000;        // Setting all bits on port a to LOW
    80.     PORTB = 0b00000000;        // Setting all bits on port b to LOW
    81.      
    82.     CMCON = 0x07;            // Disabling the analogue comparators
    83.      
    84.     while (1)                // Start endless lopp
    85.     {
    86.         if (RA0 == 1)        // Checks if bit RA0 is 1 -> button pressed.
    87.         {                    // Start if-statement
    88.             [COLOR=Red][B]while (1)[/B][/COLOR]            // Start while loop
    89.             {                    // Starting braces
    90.                 blinkAllLEDS();    // Calling the blinkAllLEDS function
    91.             }                    // End while loop
    92.         }                    // End if-statement
    93.      
    94.         if (RA1 == 1)
    95.         {
    96.             [B][COLOR=Red]while (1) [/COLOR][/B]
    97.             {
    98.                 goLeft();
    99.             }
    100.         }
    101.          
    102.         if (RA2 == 1)
    103.         {
    104.            [B][COLOR=Red]while (1)[/COLOR] [/B]
    105.             {
    106.                 goRight();
    107.             }
    108.         }
    109.          
    110.         if (RA3 == 1)
    111.         {
    112.             [COLOR=Red][B]while (1)[/B][/COLOR]
    113.             {
    114.                 goLeftRight();
    115.             }
    116.         }
    117.  
    118.     }                        // End while-loop
    119.  
    120. }                        // 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.
     
  7. Markd77

    Senior Member

    Sep 7, 2009
    2,803
    594
    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.
     
  8. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    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. :)
     
  9. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    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
     
  10. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    Shift operators... Hmm. Adding new words to my vocabulary. Thanks I will google it... Like this.
     
  11. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    Also then working on port levels try to use the unsigned char type. And as beginner you are just doing fine
     
  12. Markd77

    Senior Member

    Sep 7, 2009
    2,803
    594
    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
     
  13. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    unsigned char.... Learning a lot.

    - thank you.

    ... reading about shift operators ...
     
  14. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    I have fixed the Goleft function. Also look how I use the debug tools in MPLAB SIM.
     
  15. nerdegutta

    Thread Starter Moderator

    Dec 15, 2009
    2,515
    785
    Nice!

    Thanks a lot. :)

    I really really really appreciate it!
     
Loading...
You May Also Like:

Load More