CCS C Software and Maintenance Offers
FAQFAQ   FAQForum Help   FAQOfficial CCS Support   SearchSearch  RegisterRegister 

ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 

CCS does not monitor this forum on a regular basis.

Please do not post bug reports on this forum. Send them to CCS Technical Support
Proofread Clock code

 
Post new topic   Reply to topic    CCS Forum Index -> General CCS C Discussion
View previous topic :: View next topic  
Author Message
seifpic



Joined: 11 Mar 2012
Posts: 45
Location: Egypt

View user's profile Send private message

Proofread Clock code
PostPosted: Wed Mar 14, 2012 10:30 pm     Reply with quote
Hello,

I am making a clock using a PIC16F628A, LEDs instead of 7-seg displays (3 LEDs for each segment) powered by 3 74164 8 bit shift registers, 3 buttons for setting the clock, and a DS1307 RTC for keeping time. My schematic is messy but here it is anyway (eagle Cad): http://www.mediafire.com/?le9ze19tyt6su47

I am using the DS1307 driver found here:
http://www.ccsinfo.com/forum/viewtopic.php?t=23255&highlight=ds1307+driver

Most importantly, here is my code which I want proofread:

Code:
 
#include <16f628a.h>
#use delay (crystal=32768)
#include <DS1307.c>
#define hr_clk pin_a0
#define hr_out pin_a1

#define mint_clk pin_b6
#define mint_out pin_b7

#define min_clk pin_b4
#define min_out pin_b5

#define set pin_a3
#define inc pin_a4
#define dec pin_a5

#define ampin pin_b1
#define pmpin pin_b0

   int8 zero = 0b11110110;
   int8 one = 0b00010100;   
   int8 two = 0b10111010;
   int8 three = 0b11101010;   
   int8 four = 0b11001100;
   int8 five = 0b01101110;
   int8 six = 0b01111110;
   int8 seven = 0b11000010;
   int8 eight = 0b11111110;
   int8 nine = 0b11101110;
   int8 ten = 0b11110111;
   int8 eleven = 0b00010101;
   int8 twelve = 0b10111011;
   
   char hr;
   char min;
   char unused;
   char curhr;
   char curmin;
   short am;

//Start Display Change Function

void Shift_out(int8 b, char out, char clk) {
    int8 i = 0;
    for(i=0; i<=8; ++i)             // loop thru 8 bit
    {
        if(bit_test(b,i))           // test if bit set or clear
        {
            output_high(out);// set output high
            output_high(clk);// clock
        }
        else
        {
            output_low(out);        // set output low
            output_high(clk);       // clock 
        }
    }
   
    output_low(clk);
}

void num(int rnum, char out, char clk) {
   
   switch(rnum) {
      case 0:
         Shift_out(zero, out, clk);
      break;
      case 1:
         Shift_out(one, out, clk);
      break;
      case 2:
         Shift_out(two, out, clk);
      break;
      case 3:
         Shift_out(three, out, clk);
      break;
      case 4:
         Shift_out(four, out, clk);
      break;
      case 5:
         Shift_out(five, out, clk);
      break;
      case 6:
         Shift_out(six, out, clk);
      break;
      case 7:
         Shift_out(seven, out, clk);
      break;
      case 8:
         Shift_out(eight, out, clk);
      break;
      case 9:
         Shift_out(nine, out, clk);
      break;
      case 10:
         Shift_out(ten, out, clk);
      break;
      case 11:
         Shift_out(eleven, out, clk);
      break;
      case 12:
         Shift_out(twelve, out, clk);
      break;     
   }
}

void UpdateMin(int mins) {
   if(mins < 10) {
      curmin = mins;
      num(curmin, min_out, min_clk);
      Shift_out(zero, mint_out, mint_clk);
   } else {
      curmin = mins;
      num((curmin % 10), min_out, min_clk);
      num(((curmin / 10) % 10), mint_out, mint_clk);
   }
}

void updateDisplay() {
   
   
   ds1307_get_time(hr,min,unused); //get the time from the RTC
   
   if(curhr == hr && curmin == min) {
      break;
   } else if(curhr != hr) {
     
      if(hr < 12) {
         am = 1;
         output_high(ampin);
         output_low(pmpin);
         
         curhr = hr;
      } else {
         am = 0;
         output_high(pmpin);
         output_low(ampin);
         
            switch(hr) {
               case 13: curhr = 1;
               break;
               case 14: curhr = 2;
               break;
               case 15: curhr = 3;
               break;
               case 16: curhr = 4;
               break;
               case 17: curhr = 5;
               break;
               case 18: curhr = 6;
               break;
               case 19: curhr = 7;
               break;
               case 20: curhr = 8;
               break;
               case 21: curhr = 9;
               break;
               case 22: curhr = 10;
               break;
               case 23: curhr = 11;
               break;
            }
     
        }
       
        num(curhr, hr_out, hr_clk);
        UpdateMin(min);
   } else if(curhr == hr && curmin != min) {
      UpdateMin(min);   
   }
}
//End Display Change Function

void TSMode() {
   int8 setting = 1;
   int1 hrset = 0;
   int1 mintset = 0;
   int1 minset = 0;
   Shift_out(zero, hr_out, hr_clk);
   Shift_out(zero, mint_out, mint_clk);
   Shift_out(zero, min_out, min_clk);
   while(setting == 1) { //setting hours loop
      if(input(inc)) { //increase button pressed?
         if(hrset < 12) {
            hrset++;
            num(hrset, hr_out, hr_clk);
         }
      }
      if(input(dec)) { //decrease button pressed?
         if(hrset > 0) {
            hrset--;
            num(hrset, hr_out, hr_clk);
         }
      }
      if(input(set)) { //set button pressed?
         setting = 2; //increase setting exists hours loop and goes to minutes tens loop
      }
   }
   
   while(setting == 2) { //setting hours loop
      if(input(inc)) { //increase button pressed?
         if(mintset < 9) {
            mintset++;
            num(mintset, mint_out, mint_clk);
         }
      }
      if(input(dec)) { //decrease button pressed?
         if(mintset > 0) {
            mintset--;
            num(mintset, mint_out, mint_clk);
         }
      }
      if(input(set)) { //set button pressed?
         setting = 3; //increase setting exists minutes ten loop and goes to minutes loop
      }
   }
   
   while(setting == 3) { //setting hours loop
      if(input(inc)) { //increase button pressed?
         if(minset < 9) {
            minset++;
            num(minset, min_out, min_clk);
         }
      }
      if(input(dec)) { //decrease button pressed?
         if(minset > 0) {
            minset--;
            num(minset, min_out, min_clk);
         }
      }
      if(input(set)) { //set button pressed?
         break; //exists loop
      }
   }
   ds1307_set_date_time(0,0,0,0,hrset,((mintset * 10) + minset),0);
}


void main() {
   
   ds1307_init();
   updatedisplay();
   
   while(true) {
      if(input(set)) {
         TSMode();
      }
      output_toggle(pin_a2);
      delay_ms(500);
   }
}


Note: I haven't bought the components to test this out yet.

Thanks in advance.
ckielstra



Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 2:00 am     Reply with quote
Code:
    for(i=0; i<=8; ++i)             // loop thru 8 bit
    {
        if(bit_test(b,i))           // test if bit set or clear
        {
            output_high(out);// set output high
            output_high(clk);// clock
        }
        else
        {
            output_low(out);        // set output low
            output_high(clk);       // clock
        }
    }
   
    output_low(clk);
Your loop is 9 bits, not 8 as intended.
Also you can optimise the code a little bit more by moving the two output_high commands outside the if-statement. Then replace it by one single output_high.

Code:
#use delay (crystal=32768)
Is this really the clock frequency for your PIC? Possible, but seems very low. The DS1307 has its own 32768Hz crystal. I can''t check your schematic as I don''t have Eagle installed. Posting a screenshot graphic would have been easier to everyone.


Code:
            switch(hr) {
               case 13: curhr = 1;
               break;
               case 14: curhr = 2;
               break;
               case 15: curhr = 3;
               break;
               case 16: curhr = 4;
               break;
               case 17: curhr = 5;
               break;
               case 18: curhr = 6;
               break;
               case 19: curhr = 7;
               break;
               case 20: curhr = 8;
               break;
               case 21: curhr = 9;
               break;
               case 22: curhr = 10;
               break;
               case 23: curhr = 11;
               break;
            }
Here you are doing a similar thing as in a previous post, writing out all the possible combinations. The code is good but dumb, difficult to maintain and wasting memory.
Whenever you are writing three times or more lines of similar code you have to say to yourself: 'How can I change this into a formula?' Then you write the formula into compact code.
I leave it as an exercise to you to optimize the above code.
Same is valid for your other long switch statement.

Code:
   if(curhr == hr && curmin == min) {
      break;
   } else if(curhr != hr) {
break ????
That line is doing nothing, so it can be removed and the if-statement above as well.

Code:
   int8 zero = 0b11110110;
   int8 one = 0b00010100;   
   int8 two = 0b10111010;
   int8 three = 0b11101010;   
   int8 four = 0b11001100;
...
These are constants. Declaring them as variables uses valuable RAM and makes less efficient code. Change to:
Code:
#define ZERO  0b11110110;
#define ONE   = 0b00010100;   
#define TWO   = 0b10111010;
#define THREE = 0b11101010;   
#define FOUR  = 0b11001100;
...


Updatedisplay() is not called from within the loop inside main. This means your displayed is only updated once at startup.

It is good coding convention to write constants with capital letters, this makes it easier to distinguish them from variables. For example MINT_OUT, MINT_CLK, PIN_A1, ZERO, ELEVEN, PIN_B1.

Code:
   int1 hrset = 0;
   int1 mintset = 0;
   int1 minset = 0;
You don't want these to be booleans!

Considering the number of problems I found in this short time it is to be expected there are more issues, but for now this should give you enough input to post an updated version.
seifpic



Joined: 11 Mar 2012
Posts: 45
Location: Egypt

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 9:31 am     Reply with quote
ckielstra wrote:
I can''t check your schematic as I don''t have Eagle installed. Posting a screenshot graphic would have been easier to everyone.


The schematic is messy but here it is anyway: http://i.imgur.com/tfjjc.png

This is my updated code:

Code:

#include <16f628a.h>
#use delay (crystal=32768)
#include <DS1307.c>
#define HR_CLK pin_a0
#define HR_OUT pin_a1

#define MINT_CLK pin_b6
#define MINT_OUT pin_b7

#define MIN_CLK pin_b4
#define MIN_OUT pin_b5

#define set pin_a3
#define inc pin_a4
#define dec pin_a5

#define ampin pin_b1
#define pmpin pin_b0

#define ZERO 0b11110110
#define ONE 0b00010100 
#define TWO 0b10111010
#define THREE 0b11101010 
#define FOUR 0b11001100
#define FIVE 0b01101110
#define SIX 0b01111110
#define SEVEN 0b11000010
#define EIGHT 0b11111110
#define NINE 0b11101110
#define TEN 0b11110111
#define ELEVEN 0b00010101
#define TWELVE 0b10111011
   
   char hr;
   char min;
   char unused;
   char curhr;
   char curmin;
   int8 finhr;
   short am;

//Start Display Change Function

void Shift_out(int8 b, char out, char clk) {
    int8 i = 1;
    for(i=1; i<=8; ++i)             // loop thru 8 bit
    {
        if(bit_test(b,i))           // test if bit set or clear
        {
            output_high(out);// set output high
        }
        else
        {
            output_low(out);        // set output low 
        }
    }
    output_high(clk);// clock
    output_low(clk);
}

void num(int rnum, char out, char clk) {
   
   switch(rnum) {
      case 0:
         Shift_out(ZERO, out, clk);
      break;
      case 1:
         Shift_out(ONE, out, clk);
      break;
      case 2:
         Shift_out(TWO, out, clk);
      break;
      case 3:
         Shift_out(THREE, out, clk);
      break;
      case 4:
         Shift_out(FOUR, out, clk);
      break;
      case 5:
         Shift_out(FIVE, out, clk);
      break;
      case 6:
         Shift_out(SIX, out, clk);
      break;
      case 7:
         Shift_out(SEVEN, out, clk);
      break;
      case 8:
         Shift_out(EIGHT, out, clk);
      break;
      case 9:
         Shift_out(NINE, out, clk);
      break;
      case 10:
         Shift_out(TEN, out, clk);
      break;
      case 11:
         Shift_out(ELEVEN, out, clk);
      break;
      case 12:
         Shift_out(TWELVE, out, clk);
      break;     
   }
}

void UpdateMin(int mins) {
   if(mins < 10) {
      curmin = mins;
      num(curmin, MIN_OUT, MIN_CLK);
      Shift_out(ZERO, MINT_OUT, MINT_CLK);
   } else {
      curmin = mins;
      num((curmin % 10), MIN_OUT, MIN_CLK);
      num(((curmin / 10) % 10), MINT_OUT, MINT_CLK);
   }
}

void updateDisplay() {
   
   
   ds1307_get_time(hr,min,unused); //get the time from the RTC
   
   if(curhr != hr) {
     
      if(hr < 12) {
         am = 1;
         output_high(ampin);
         output_low(pmpin);
         
         curhr = hr;
      } else {
         am = 0;
         output_high(pmpin);
         output_low(ampin);
         
            curhr = (hr - 12);
     
        }
        finhr = hr;
        num(curhr, HR_OUT, HR_CLK);
        UpdateMin(min);
   } else if(curhr == hr && curmin != min) {
      UpdateMin(min);   
   }
}
//End Display Change Function

void TSMode() {
   int8 setting = 1;
   int8 hrset = 0;
   int8 mintset = 0;
   int8 minset = 0;
   Shift_out(ZERO, HR_OUT, HR_CLK);
   Shift_out(ZERO, MINT_OUT, MINT_CLK);
   Shift_out(ZERO, MIN_OUT, MIN_CLK);
   while(setting == 1) { //setting hours loop
      if(input(inc)) { //increase button pressed?
         if(hrset < 12) {
            hrset++;
            num(hrset, HR_OUT, HR_CLK);
         }
      }
      if(input(dec)) { //decrease button pressed?
         if(hrset > 0) {
            hrset--;
            num(hrset, HR_OUT, HR_CLK);
         }
      }
      if(input(set)) { //set button pressed?
         setting = 2; //increase setting exists hours loop and goes to minutes TENs loop
      }
   }
   
   while(setting == 2) { //setting hours loop
      if(input(inc)) { //increase button pressed?
         if(mintset < 9) {
            mintset++;
            num(mintset, MINT_OUT, MINT_CLK);
         }
      }
      if(input(dec)) { //decrease button pressed?
         if(mintset > 0) {
            mintset--;
            num(mintset, MINT_OUT, MINT_CLK);
         }
      }
      if(input(set)) { //set button pressed?
         setting = 3; //increase setting exists minutes TEN loop and goes to minutes loop
      }
   }
   
   while(setting == 3) { //setting hours loop
      if(input(inc)) { //increase button pressed?
         if(minset < 9) {
            minset++;
            num(minset, MIN_OUT, MIN_CLK);
         }
      }
      if(input(dec)) { //decrease button pressed?
         if(minset > 0) {
            minset--;
            num(minset, MIN_OUT, MIN_CLK);
         }
      }
      if(input(set)) { //set button pressed?
         break; //exists loop
      }
   }
   ds1307_set_date_time(0,0,0,0,hrset,((mintset * 10) + minset),0);
}


void main() {
   
   ds1307_init();
   updatedisplay();
   
   while(true) {
      if(input(set)) {
         TSMode();
      }
      output_toggle(pin_a2);
      delay_ms(500);
   }
}
PCM programmer



Joined: 06 Sep 2003
Posts: 21708

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 9:42 am     Reply with quote
Quote:

int8 i = 1;
for(i=1; i<=8; ++i) // loop thru 8 bit
{
if(bit_test(b,i)) // test if bit set or clear
{
output_high(out);// set output high
}
else
{
output_low(out); // set output low
}
}

I can't believe you are still doing the loop like this. You know that bits
are numbered from 0 to 7. So that's what you need your for() loop
to generate.

This CCS manual says that bits are numbered starting at 0:
Quote:

bit_test( )

Syntax: value = bit_test (var, bit)

Parameters: var may be a 8,16 or 32 bit variable (any lvalue)
bit is a number 0- 31 representing a bit number, 0 is the least significant bit.


What I wanted you to do in the other thread was to use the little program
and modify and test it until you get an output of 0 to 7. You still need to
do this:
http://www.ccsinfo.com/forum/viewtopic.php?t=47720&start=10
seifpic



Joined: 11 Mar 2012
Posts: 45
Location: Egypt

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 9:50 am     Reply with quote
PCM programmer wrote:

What I wanted you to do in the other thread was to use the little program
and modify and test it until you get an output of 0 to 7. You still need to
do this:
http://www.ccsinfo.com/forum/viewtopic.php?t=47720&start=10


I am sorry I ignored your other post but I'm new to this program and I don't know how to debug the code. I don't have the components yet therefore I can't test the code that way. The help file isn't the best in describing how the debug works.
PCM programmer



Joined: 06 Sep 2003
Posts: 21708

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 9:57 am     Reply with quote
You don't need to run the hardware debugger. You can just run that
little program and look at the output on a terminal window. Edit the
for() loop until you get a displayed output of 0 to 7. Hint: Look at the
test you are using in the middle of the for() loop.


I was just searching for a tutorial for you and I found someone who
thinks the same way I do, in terms of test programs:
http://cprogramminglanguage.net/c-for-loop-statement.aspx
seifpic



Joined: 11 Mar 2012
Posts: 45
Location: Egypt

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 10:04 am     Reply with quote
PCM programmer wrote:
You don't need to run the hardware debugger. You can just run that
little program and look at the output on a terminal window.


Ok, I changed the code to normal C and ran it using Codepad. Here is the link to the result: http://codepad.org/EJjkfTbw

I then changed the 8 to 7 and I get 8 outputs: http://codepad.org/RWlRTp20
dezso



Joined: 04 Mar 2010
Posts: 102

View user's profile Send private message

PostPosted: Thu Mar 15, 2012 9:53 pm     Reply with quote
Sorry seifpic, it was my fault for providing you with the faulty example!
_________________
I'm could be wrong many time's, at least I know what I'm doing Smile
ckielstra



Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands

View user's profile Send private message

PostPosted: Fri Mar 16, 2012 2:19 am     Reply with quote
Congratulations on the code improvement. The code is looking much better now.

One major bug is that you are still not updating the clock from within the main loop. That means your clock will never 'tick'.

Code:
finhr = hr;
This variable is never used.

Code:
   if(curhr != hr) {
     
...         
            curhr = (hr - 12);
For 'hr > 12' you will now always do a display update.

Some minor issues. These are not bugs but suggestions to make your program easier to maintain:
1) You do have a function UpdateMin() but not an UpdateHr().
2) The function num() contains twelve similar code constructs, a strong indication for code optimization. Try reading up on the concept of 'arrays' and you can replace all code by 1 line.
3) The function Shift_out() always needs the parameters for output port and clock line combined, i.e. there is a hard wires combination between these parameters. By adding some extra code you can hide this dependency for the programmer. Only inside the shift function you need to be aware of this connection. The programmer of the 'main' function should only have to say he wants to update the hours, ten_minutes, or minutes and the shift function can figure out the required pins.
4) Different code constructs used for exiting a loop in the time set function. Two times you leave the loop by setting a variable and the third time with a break command. Why using two different techniques? Both will work but think for yourself which one you like better and then stick to that. Consistent coding is easier to read and you will see patterns for code optimization.
Display posts from previous:   
Post new topic   Reply to topic    CCS Forum Index -> General CCS C Discussion All times are GMT - 6 Hours
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum


Powered by phpBB © 2001, 2005 phpBB Group