We are running a survey to help us improve the experience for all of our members. If you see the survey appear, please take the time to tell us about your experience if you can.
Hi, I'm writing a code for digital clock in Keil C, but I'm not getting the expected results. Can anybody please help me with the program? The code is as follows..
#include <AT892051.H> #include <stdio.h> #include <intrins.h>
sbit PIN_P10=P1^0;/* Character to be transfered through this bit in parallel*/ sbit PIN_P11=P1^1;/* Clock*/ sbit PIN_P12=P1^2;/* Latch*/ sbit CS4=P1^3; /* 1st 7-segment Display*/ sbit CS3=P1^4; /* 2nd 7-segment Display*/ sbit CS2=P1^5; /* 3rd 7-segment Display*/ sbit CS1=P1^6; /* 4th 7-segment Display*/
unsigned char DataWrite; unsigned char _crol_(unsigned char Temp,unsigned char DataWrite); char Display_Buffer[6]; unsigned char Dispcount; unsigned char mscount;
int Hours=1; int Minutes=0; int Seconds=0; int i=0; char Temp;
code unsigned char Lookup[11]= { 0xEE,0x28,0xCD,0x6C,0x2B,0x67,0xE7,0x2C,0xEF,0x2F };
void main() { TMOD=0x01; TH0=0xF8; /* Initializing timer0*/ TL0=0x30; /*for 1ms*/ IE=0x82; TR0=1; do { sprintf(Display_Buffer,"%02d%02d",Hours,Minutes); /* converting int into char*/ }while(1); } void Timer0(void) interrupt 1 { TF0=0; TR0=0; TH0=0xF8; TL0=0x30; TR0=1; { mscount++; Dispcount++; if(mscount>=1000) { mscount=0; Seconds++; } if(Seconds>=59) { Seconds=0; Minutes++; } if(Minutes>=59) { Minutes=0; Hours++; } if(Hours>=12) { Hours=1; } } { if(Dispcount==29) /* This code is for refresh rate*/ { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==30) { DataWrite=Lookup[Display_Buffer[0]-0x30]; } if(8>i) { i++; if (Dispcount==34) { Temp=DataWrite; } if (Dispcount==32) { PIN_P11=1; } if (Dispcount==36) { PIN_P10=Temp^7; } if (Dispcount==38) { PIN_P11=0; } if (Dispcount==40) { DataWrite=_crol_(Temp,1); } } PIN_P12=0; if(Dispcount==48) { CS1=1; } } { if(Dispcount==49) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==50) { DataWrite=Lookup[Display_Buffer[1]-0x30]; } { if(8>i) { i++; if (Dispcount==54) { Temp=DataWrite; } if (Dispcount==52) { PIN_P11=1; } if (Dispcount==56) { PIN_P10=Temp^7; } if (Dispcount==58) { PIN_P11=0; } if (Dispcount==60) { DataWrite=_crol_(Temp,1); } } PIN_P12=0; } if(Dispcount==68) { CS2=1; } } { if(Dispcount==69) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==70) { DataWrite=Lookup[Display_Buffer[2]-0x30]; } { if(8>i) { i++; if (Dispcount==74) { Temp=DataWrite; } if (Dispcount==72) { PIN_P11=1; } if (Dispcount==76) { PIN_P10=Temp^7; } if (Dispcount==78) { PIN_P11=0; } if (Dispcount==80) { DataWrite=_crol_(Temp,1); } } PIN_P12=0; } if(Dispcount==88) { CS3=1; } } { if(Dispcount==89) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==90) { DataWrite=Lookup[Display_Buffer[3]-0x30]; } { if(8>i) { i++; if (Dispcount==94) { Temp=DataWrite; } if (Dispcount==92) { PIN_P11=1; } if (Dispcount==96) { PIN_P10=Temp^7; } if (Dispcount==98) { PIN_P11=0; } if (Dispcount==100) { DataWrite=_crol_(Temp,1); } } PIN_P12=0; } if(Dispcount==108) { CS4=1; } } if(Dispcount==821) { CS1,CS2,CS3,CS4=0; } } RETI;
Talking about scribbling.
Can you - without looking at the datasheets or datadeclarations of your program - tell what the following code does?
if (Dispcount == 89) { CS1 = 0; CS2 = 0; CS3 = 0; CS4 = 0; }
Will you also be able to do it six months from now?
Is there a reason why you don't believe comments belong in source code? Maybe "The code was hard to write, so it should also be hard to read"?
How much do you think it "speeds up" your debugging, to have source code without comments?
I hope this isn't a scribbled one.
sbit PIN_P10=P1^0; /* Character to be transfered through thisbit in parallel*/ sbit PIN_P11=P1^1; /* Clock*/ sbit PIN_P12=P1^2; /* Latch*/ sbit CS4=P1^3; /* 1st 7-segment Display*/ sbit CS3=P1^4; /* 2nd 7-segment Display*/ sbit CS2=P1^5; /* 3rd 7-segment Display*/ sbit CS1=P1^6; /* 4th 7-segment Display*/
void main() {
TMOD=0x01; TH0=0xF8; /* Initializing timer0 for 1ms*/ TL0=0x30; IE=0x82; TR0=1;
do {
sprintf(Display_Buffer,"%02d%02d",Hours,Minutes); /* converting integer into character*/
}while(1);
}
void Timer0(void) interrupt 1 /* ISR */ { TF0=0; TR0=0; TH0=0xF8; TL0=0x30; TR0=1;
{
Dispcount++; /* Dispcount is used to refresh the Hours and Minutes on the LED*/ if(++mscount>=1000) { mscount=0; Seconds++;
} if(++Seconds>=59) { Seconds=0; Minutes++;
} if(++Minutes>=59) { Minutes=0; Hours++;
} if(++Hours>=12) { Hours=1; } }
{ if(Dispcount==29) /* This code is for refresh rate of LED1*/ { CS1=0; /* CS1, CS2, CS3, and CS4 are Rspective LED's Of HH:MM*/ CS2=0; CS3=0; CS4=0; } if(Dispcount==30) {
DataWrite=Lookup[Display_Buffer[0]-0x30];
if(8>i) /* For a byte to be read by the port pin*/ { i++; if (Dispcount==32) { Temp=DataWrite; } if (Dispcount==34) { PIN_P11=1; /* clock is set high*/ }
if (Dispcount==36) { PIN_P10=Temp^7; /* Taking MSB one at a time, the loop continous till all 8-bits are sent to the portpin*/ } if (Dispcount==38) { PIN_P11=0; /*clock is made low*/ } if (Dispcount==40) { DataWrite=_crol_(Temp,1); /* bits are rotated left*/ } PIN_P12=0; }
if(Dispcount==48) { CS1=1; /* Chip 1 is selected i.e; for tens unit of Hour*/ } }
{ if(Dispcount==49) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==50) {
DataWrite=Lookup[Display_Buffer[1]-0x30];
if(8>i)
{ i++; if (Dispcount==52) { Temp=DataWrite; } if (Dispcount==54) { PIN_P11=1; }
if (Dispcount==56) { PIN_P10=Temp^7; } if (Dispcount==58) { PIN_P11=0; } if (Dispcount==60) { DataWrite=_crol_(Temp,1); } PIN_P12=0; }
if(Dispcount==68) { CS2=1; } }
{ if(Dispcount==69) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==70) {
DataWrite=Lookup[Display_Buffer[2]-0x30];
if(8>i) { i++; if (Dispcount==72) { Temp=DataWrite; } if (Dispcount==74) { PIN_P11=1; }
if (Dispcount==76) { PIN_P10=Temp^7; } if (Dispcount==78) { PIN_P11=0; } if (Dispcount==80) { DataWrite=_crol_(Temp,1); } PIN_P12=0; }
if(Dispcount==88) { CS3=1; } }
{ if(Dispcount==89) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==90) {
DataWrite=Lookup[Display_Buffer[3]-0x30];
if(8>i) { i++; if (Dispcount==92) { Temp=DataWrite; } if (Dispcount==94) { PIN_P11=1; }
if (Dispcount==96) { PIN_P10=Temp^7; } if (Dispcount==98) { PIN_P11=0; } if (Dispcount==100) { DataWrite=_crol_(Temp,1); } PIN_P12=0; }
if(Dispcount==108) { CS4=1; } }
if(Dispcount==121) { CS1,CS2,CS3,CS4=0;
RETI;
There is no need to "hope" - the 'Preview' button will show you!
Other than "hoping" what specific steps did you take to ensure that it's not "scribbles"?
* did you, as instructed, follow the directions on how to post source code?
* did you, as instructed, take time to carefully comment your code?
* did you, as instructed, take time to consider carefully the data types required by your variables?
* did you, as suggested, take time to consider what 'C' language construct is appropriate to testing an integer varialbe for one of a number of different values?
etc, etc,...
No. No.No.
So those are the things that you need to start doing, then!
Note that you really have 2 separate problems here:
1. How to count time in hours, minutes, and seconds (for this part, the display is irrelevant);
2. How to display stuff on a (multiplexed?) 7-Segment display (for this part, the fact that it's time is largely irrelevant).
In general, it is preferably to subdivide your software along such lines...
Very much scribbles, since you didn't surround your code with the required tags - notice that it isn't until after your code that the post switches to source-code mode. That obviously indicates that you did something wrong - and the preview should have shown you this too.
You still have off-by-one for your time counters.
And why do you still consider your clock digits as "chips" and enable them with "CS1", "CS2" etc? Don't you think it would be better to rename these variables to something meaningful?
DriveMinutes=1; DriveTenMinutes=0; DriveHours=0; DriveTenHours=0;
And you have something called "Lookup". Would it have been too easy to read the code if the array had been called written:
SevenSegmentData = DecimalToSevenSegment[Display_Buffer[2]-'0'];
Still no information if "Dispcount" is expected to turn around after full numeric range. And is "i" expected to turn around or be reset back to zero? You say "the loop continous till all 8-bits are sent" - but what loop?
And why do you still have a RETI? Don't you think the _compiler_ will manage to insert a RETI instruction when you declare a function with the "interrupt" keyword?
You have code like:
void Timer0(void) interrupt 1 /* ISR */ { TF0=0; TR0=0; TH0=0xF8; TL0=0x30; TR0=1; { Dispcount++; /* Dispcount is used to refresh the Hours and Minutes on the LED*/ if(++mscount>=1000) { mscount=0; Seconds++; } if(++Seconds>=59) { <== should update every ms? Seconds=0; Minutes++; } if(++Minutes>=59) { <== should update every ms? Minutes=0; Hours++; } if(++Hours>=12) { <== should update every ms? Hours=1; } } // The following state machine intended to // start every ms even if you don't have had // a toggled second? On the other hand, you // tick your "Second" every ms so... { if(Dispcount==29) { /* This code is for refresh rate of LED1*/ CS1=0; /* CS1, CS2, CS3, and CS4 are Rspective LED's Of HH:MM*/ CS2=0; CS3=0; CS4=0; } if(Dispcount==30) { DataWrite=Lookup[Display_Buffer[0]-0x30]; } if(8>i) { /* For a byte to be read by the port pin*/ How do you synchronize your "Dispcount" statemachine with your "i" statemachine? i++; if (Dispcount==32) { Temp=DataWrite; } if (Dispcount==34) { PIN_P11=1; /* clock is set high*/ } if (Dispcount==36) { PIN_P10=Temp^7; /* Taking MSB one at a time, the loop continous till all 8-bits are sent to the portpin*/ } if (Dispcount==38) { PIN_P11=0; /*clock is made low*/ } if (Dispcount==40) { DataWrite=_crol_(Temp,1); /* bits are rotated left*/ } PIN_P12=0; } if(Dispcount==48) { CS1=1; /* Chip 1 is selected i.e; for tens unit of Hour*/ } } // Do you have any special reason for your extra // statement blocks introduced here and there? // If you have - why not a software comment? { if(Dispcount==49) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==50) { DataWrite=Lookup[Display_Buffer[1]-0x30]; } // Do you have any special reason for your extra // statement blocks introduced here and there? // If you have - why not a software comment? { if(8>i) { i++; if (Dispcount==52) { Temp=DataWrite; } if (Dispcount==54) { PIN_P11=1; } if (Dispcount==56) { PIN_P10=Temp^7; } if (Dispcount==58) { PIN_P11=0; } if (Dispcount==60) { DataWrite=_crol_(Temp,1); } PIN_P12=0; } } if(Dispcount==68) { CS2=1; } } // Do you have any special reason for your extra // statement blocks introduced here and there? // If you have - why not a software comment? { if(Dispcount==69) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==70) { DataWrite=Lookup[Display_Buffer[2]-0x30]; } // Do you have any special reason for your extra // statement blocks introduced here and there? // If you have - why not a software comment? { if(8>i) { i++; if (Dispcount==72) { Temp=DataWrite; } if (Dispcount==74) { PIN_P11=1; } if (Dispcount==76) { PIN_P10=Temp^7; } if (Dispcount==78) { PIN_P11=0; } if (Dispcount==80) { DataWrite=_crol_(Temp,1); } PIN_P12=0; } } if(Dispcount==88) { CS3=1; } } // Do you have any special reason for your extra // statement blocks introduced here and there? // If you have - why not a software comment? { if(Dispcount==89) { CS1=0; CS2=0; CS3=0; CS4=0; } if(Dispcount==90) { DataWrite=Lookup[Display_Buffer[3]-0x30]; } // Do you have any special reason for your extra // statement blocks introduced here and there? // If you have - why not a software comment? { if(8>i) { i++; if (Dispcount==92) { Temp=DataWrite; } if (Dispcount==94) { PIN_P11=1; } if (Dispcount==96) { PIN_P10=Temp^7; } if (Dispcount==98) { PIN_P11=0; } if (Dispcount==100) { DataWrite=_crol_(Temp,1); } PIN_P12=0; } } if(Dispcount==108) { CS4=1; } } if(Dispcount==121) { CS1,CS2,CS3,CS4=0; } }
Do you really have such scribbles in your own editor that you are unable to keep track of nesting of statements? Wait - don't answer that one...
And why do you repost your code with the same errors you have already been informed about?
I'm developing the clock using At89C2051, 74LS595 shift register and a ULN2803 driver. I'm taking a byte from a port pin of 2051 and transferring it on the shift register, so whenever a chip is selected the data in the shift register is passed to the ULN driver which inturn is displayed on the LED.
Have you considered how fast a shift register is compared to the instruction sequencing of your processor? Have you introduced huge capacitances on the signals, or why do you assume that you need a huge state machine for slowing down the output of the bits to the shift register?
What is the fastest frequency the shift register can shift? What is the fastest speed your processor can tggle a processor pin? What is your conclusion from these times?
sbit PIN_P10=P1^0; /* Character to be transfered through thisbit in parallel*/ sbit PIN_P11=P1^1; /* Clock*/ sbit PIN_P12=P1^2; /* Latch*/ sbit DriveMinutes=P1^3; /* 1st 7-segment Display*/ sbit DriveTenMinutes=P1^4; /* 2nd 7-segment Display*/ sbit DriveHours=P1^5; /* 3rd 7-segment Display*/ sbit DriveTenHours=P1^6; /* 4th 7-segment Display*/
unsigned char SevenSegmentData; unsigned char _crol_(unsigned char Temp,unsigned char SevenSegmentData); char Display_Buffer[6]; unsigned char Dispcount; unsigned char mscount;
code unsigned char DecimalToSevenSegment[11]= { 0xEE,0x28,0xCD,0x6C,0x2B,0x67,0xE7,0x2C,0xEF,0x2F };
void main() { TMOD=0x01; TH0=0xF8; /* Initializing timer0 for 1ms*/ TL0=0x30; IE=0x82; TR0=1; do { sprintf(Display_Buffer,"%02d%02d",Hours,Minutes); /* converting integer into character*/ }while(1); } void Timer0(void) interrupt 1 /* ISR begins*/ { TF0=0; TR0=0; TH0=0xF8; TL0=0x30; TR0=1; { i++; Dispcount++; /* Dispcount is used to refresh the Hours and Minutes on the LED*/ mscount++; if(++mscount>=1000) { mscount=0; Seconds++; } if(++Seconds>=59) { Seconds=0; Minutes++; Seconds++; } if(++Minutes>=59) { Minutes=0; Hours++; Seconds++; Minutes++; } if(++Hours>=12) { Hours=1; Seconds++; Minutes++; Hours++; } } // For DriveHours LED { if(Dispcount==29) /* This code is for refresh rate of LED1*/ { DriveMinutes=0; DriveTenMinutes=0; DriveHours=0; DriveTenHours=0; } if(Dispcount==30) { SevenSegmentData=DecimalToSevenSegment[Display_Buffer[0]-0x30]; } if(8>++i) /* For a byte to be read by the port pin*/ { if (Dispcount==32) { Temp=SevenSegmentData; } if (Dispcount==34) { PIN_P11=1; /* clock is set high*/ } if (Dispcount==36) { PIN_P10=Temp^7; /* Taking MSB one at a time, the loop continous till all 8-bits are sent to the portpin*/ } if (Dispcount==38) { PIN_P11=0; /*clock is made low*/ } if (Dispcount==40) { SevenSegmentData=_crol_(Temp,1); /* bits are rotated left*/ } PIN_P12=0; } if(Dispcount==48) { DriveTenHours=1; } }
I've posted the new code of only one LED now. But I stil don't know whtat to declare for Dispcount, since it keeps on incrementin for 1ms. Dispcount is used for refresh rate
That is an absolutely basic, first step in 'C' programming - nothing specifially to do with Keil. (in fact, it's not even specific to 'C' - the same would apply in any other programming language with "typed" variables)
You really need to go back and review the notes from your first few 'C' classes. (and/or the early chapters of your 'C' textbook)
If you're still stuck, you really need to discuss this with your tutors - since the assignment clearly assumes that you had nailed this issue a long time ago!
Are you reading anything we post here? Anything at all?
Still off-by-one error for your time variables. How will the clock ever show 59 for seconds if you reset it at 59?
Still stepping seconds and minutes and hours every ms.
Still random indents - your "if (8 > 8)" has same logic indent level as "if (Dispcount == 29)". Doesn't look like that in your code - are you mixing tabs and spaces?
Still no logic that takes care of "i" when it reaches 8. And "i" may be an acceptable temp name for a locally used loop variable - you have "i" as a global variable that is processed in by a state machine. So why isn't it named BitPosition or something?
And you tick "i" outside the state machine that needs multiple steps to process a single bit - how do you think the state machine can work if you reverse inside and outside of the loop?
By the way - you still claim "the loop continous till all 8-bits are sent to the portpin". Maybe you should describe exactly what you think your look like. It does not look like you think it does, unless it was your intention to have a broken "loop".
Why do you think you need both a SevenSegmentData and a Temp? What is Temp storing that can't be stored in SevenSegmentData?
PIN_P11=1; /* clock is set high*/
Why do you have a variable name that requires you to write a comment to tell that the clock is set high? Wouldn't it have been better to define the variable so the name had told the function? Why not:
DigitShiftregisterClockPin = 1;
This would also allow you to redesign the hw and move the signal to another processor pin without renaming a variable all over the code.
You still haven't come back with an explanation why you think you need 2ms between each pin toggle - does that really correspond with the hardware requirements? Isn't the shift register capable of running at many MHz, having all times specified in ns and not in ms? Do you need a huge state machine for emitting the 8 bits of a digit?
And you haven't given any feedback why you think you should update the display every 256ms (which is the speed Dispcount will turn around if an 8-bit variable. Remember that you are only displaying hours and minutes. You don't even seem to have a colon flashing every second. Wouldn't it be enough if the output state machine is only run the first time, and every time the clock has been changed, and every time the minute counter has been changed? If the minute counter hasn't changed - what change do you want to emit to the 7-segment digits?
Why use the magic constant 0x30 if you want to subtract the offset of the ASCII character '0'? Didn't you think it was better to write '0'? Did you have to look at an ASCII table to figure out the numeric value of '0'?
C has an operator for bit shift. Not an operator for bit rotate. Any reason why you need to rotate the data?
Look at the following code:
SevenSegmentData=DecimalToSevenSegment[Display_Buffer[0]-0x30];
Why can't you use a state machine that steps through the digit positions 0..3 and then reuse the same code for emitting the data? How much code does it take to figure out the bitmap combination for the four select lines based on a DigitPosition variable that steps 0..3?
How about you spending a full day walking through all the information you have received in this thread and rewrite the code based on the suggestions? Just reposting the code with microscopic changes between each post will not take you to a working solution this year. If you fix _at most_ 1 bug every post and have 100 bugs, it will take a huge number of posts to get to the finish line...
Another thing - why don't you think the variable declarations should be posted as code too?
Yes I'm reading your post in every minute detail. I did not understand the seconds, minutes, hours changing every ms. I'm using the mscount variable to increment it ecery 1ms which after counting 1000 will give me 1sec.
if(++mscount>=1000)
After i reaches 8, the DigitShiftregisterLatchPin will latch the byte data in the shift register. I don't know what a state machine is, can u please help me to know what a state machine is or how does it work? SevenSegmentData is a byte and I need to transfer it to the shift register using only a portpin, so I'm using the Temp variable to send it bit by bit. Doesn't C has rotate function? So what do I use instead of rotate. What command can be used for shifting a byte? The below is the modified code but only for Hours..
sbit DigitShiftregisterSerialPin=P1^0; /* Character to be transfered through thisbit in parallel*/ sbit DigitShiftregisterClockPin=P1^1; /* Clock*/ sbit DigitShiftregisterLatchPin=P1^2; /* Latch*/ /*sbit DriveMinutes=P1^3; /* 1st 7-segment Display*/ /*sbit DriveTensMinutes=P1^4; /* 2nd 7-segment Display*/ /*sbit DriveHours=P1^5; /* 3rd 7-segment Display*/ sbit DriveTensHours=P1^6; /* 4th 7-segment Display*/ unsigned char SevenSegmentData; unsigned char _crol_(unsigned char Temp,unsigned char SevenSegmentData); char Display_Buffer[1]; int Dispcount; int mscount; int Hours=1; int Minutes=0; int Seconds=0; char Temp; code unsigned char DecimalToSevenSegment[11]= { 0xEE,0x28,0xCD,0x6C,0x2B,0x67,0xE7,0x2C,0xEF,0x2F }; void main() { TMOD=0x01; TH0=0xF8; /* Initializing timer0 for 1ms*/ TL0=0x30; IE=0x82; TR0=1; do { sprintf(Display_Buffer,"%02d%02d",Hours); /* converting integer into character*/ }while(1); } void Timer0(void) interrupt 1 /* ISR begins*/ { TF0=0; TR0=0; TH0=0xF8; TL0=0x30; TR0=1; { /* To transfer bits into byte from port pin onto the shift register*/ Dispcount++; /* Dispcount is used to refresh the Hours and Minutes on the LED*/ mscount++; /* Generate 1ms Delay*/ if(++mscount>=1000) { mscount=0; Seconds++; } if(++Seconds>=60) { Seconds=0; Minutes++; } if(++Minutes>=60) { Minutes=0; Hours++; } if(++Hours>=13) { Hours=1; } } // For DriveTensHours LED { int i,j; if(Dispcount==29) /* This code is for refresh rate of DriveTensHours*/ { /* DriveMinutes=0; DriveTensMinutes=0; DriveHours=0; */ DriveTensHours=0; } if(Dispcount==30) { if(j<=1) { SevenSegmentData=DecimalToSevenSegment[Display_Buffer[j]-0x30]; if(8>i) /* For a byte to be read by the port pin*/ { if (Dispcount==32) { Temp=SevenSegmentData; } if (Dispcount==34) { DigitShiftregisterClockPin=1; /* clock is set high*/ /*}* /*if (Dispcount==36)*/ /*{*/ DigitShiftregisterSerialPin=Temp^7; /* Taking MSB one at a time, the loop continous till all 8-bits are sent to the portpin*/ /* } if (Dispcount==38) {*/ DigitShiftregisterClockPin=0; /*clock is made low*/ } if (Dispcount==40) { SevenSegmentData=_crol_(Temp,1); /* bits are rotated left*/ } DigitShiftregisterLatchPin=0; /* Data is Latched*/ } if(Display_Buffer[0]) { DriveTensHours=1; } } } } }
If that is true, then why are you not acting on what you read?
Did you actually check how you code looks in the Preview?
Do you think that is really clear and easy to read?
Have you thought about that 'C' language construct for choosing one of many discrete values?
Are you saying that you can't see that the variables "mscount" and "Seconds" are updated in two independant code blocks? Both blocks will be run on every call to the interrupts, so both variables will - independantly - be updated every millisecond.
if (++mscount >= 1000) { mscount = 0; Seconds++; } if (++Seconds >= 60) { Seconds = 0; Minutes++; }
It really doesn't matter at what value you restart mscount, since the next if statement neither looks at mscount or is nested inside the previous if statement.
Compare with:
if (++mscount >= 1000) { mscount = 0; Seconds++; if (++Seconds >= 60) { Seconds = 0; Minutes++; } }
Notice any difference to the logic?
But the question here - are you saying that your debugging of your code have not shon you this strange behaviour that mscount, seconds, minutes and hours are all getting updated like crazy? How could you have missed it?
Another thing - do not write code with nested comments - don't assume that a compuler supports it.
/* inside a comment /* inside nested comment */ back to first comment. */
Many compilers don't handle nested comments, so "back to first comment" may no longer be inside any comment at all. Your teacher has not told you that it is ok to use nested comments!
And your state machine is still broken. If you only enter a code block when the state is 30, and the state isn't changed inside the code block, then you will never manage to run the statements that tests for state = 32.
// For DriveTensHours LED /* ***** How can your comment claim "DriveTenHours LED"? * Is the LED named DriveTenHours? Strange name for a digit. * ***** Next thing - you have introduced a variable to index * which digit to pick up data for - why, if you claim that * the block is only for a single digit? If your comments * don't match the code, then your comments are lying. */ { int i,j; if (Dispcount==29) { /* This code is for refresh rate of DriveTensHours*/ /* ***** * NO IT ISN'T - it is not for "refresh rate". The code isn't there * for the variable "DriveTenHours" but for handling the external * hardware associated with a specific digit of the clock. */ /* DriveMinutes=0; DriveTensMinutes=0; DriveHours=0; */ DriveTensHours=0; } if( Dispcount==30) { /* ***** How can you use the variable "j" when "j" has never been * receiving a value? Is your code intended to be some form of * random generator??? And exactly what is the meaning of: * "random uninitialized variable less than one"? You added the * test just for fun, or did you have any plan with it? Is this * random variable "j" ever expected to get any assign or change * in your code? */ if (j<=1) { SevenSegmentData=DecimalToSevenSegment[Display_Buffer[j]-0x30]; /* ***** Why do you so constantly think 0x30 is a good choice * instead of '0' in above statement? You haven't motivated why * you want to introduce "magic" constants in your code. * ***** So "j" should now magically be the digit - but how * can you have a state machine that updates the data during * many calls of this interrupt, but keeping the state in a * variable that gets thrown away between each interrupt tick? * And who is stepping the "j" variable? */ if (8>i) { /* For a byte to be read by the port pin*/ /* ***** Yet another variable that hasn't received any * value (was global before, so then zero-initialized). * But when is this variable intended to change? And how * is that change inter-related to the updating of Dispcount * every millisecond? */ if (Dispcount==32) { /* ***** If you have already established a couple of * lines above that you should only enter this section * when Dispcount == 30 - how do you now suddenly think * that Dispcount should have managed to change to 32? * Your code is like: "If I have exactly 30 pounds in * my picket, I'll take these 32 pounds and buy some * thing for. */ Temp=SevenSegmentData; } if (Dispcount==34) { /* ***** Suddenly your 30 pounds in the pocket have * managed to become 34 - how? */ DigitShiftregisterClockPin=1; /* clock is set high*/ /*}* /*if (Dispcount==36)*/ /*{*/ DigitShiftregisterSerialPin=Temp^7; /* Taking MSB one at a time, the loop continous till all 8-bits are sent to the portpin*/ /* } if (Dispcount==38) {*/ DigitShiftregisterClockPin=0; /*clock is made low*/ } if (Dispcount==40) { /* ***** And how could the 30 pounds in your pocket * suddenly have become 40, without any action by * you? */ SevenSegmentData=_crol_(Temp,1); /* bits are rotated left*/ } DigitShiftregisterLatchPin=0; /* Data is Latched*/ } if(Display_Buffer[0]) { /* **** When is the above statement expected to not be true? * What did you expect to accomplish? Will you? If sprintf() * hasn't been run before the first timer interrupt: should * you then even let the timer tick shift out any data? * Think twice about your code to perform a lookup - what * entry will you try to lookup? */ DriveTensHours=1; } } } }