Digital clock program using AT892051

what's wrong with it? Can you please say what it is cos i need this project to be submitted asap

what's wrong with it? Can you please say what it is cos i need this project to be submitted asap

I advised you of something that is wrong and I told you where to look.

There are almost certainly other faults, but I/We don't have the time to decipher your scribbles.

I understand the value of your time and I'm very thankful to you'll for looking into my bug. I though regret that my code is scribbled at your end cause over here I have written it in proper language without any srcibbling

The code is hard to read. Both because it wasn't correctly posted, and because you have made it very complicated.

If you program in C - why do you then think you should have an assembler instruction somewhere in the code? Have you ever seen any Keil C program with a RETI for an interrupt handler? Please in that case show a link.

You have off-by one errors - how will you get your clock to display the 7:59:59? When secnds reaches 59, you reset to zero - should wait to 60. Same for the other variables.

You seem to have multiple blocks:

if (8>i) {i++; ...}

How many times can i be incremented in a single visit to your ISR?

What is the full range you expect Dispcount to run to until restarted? You have Dispcount declared as unsigned char but are trying to store values way outside the numeric range. And should Dispcount really be overflowing? I would have thought that the display output should happen once/second, meaning your state machine should restart every 1000ms.

And it isn't easy to read the code when you have many if() statements processing Dispcount in varying order - sometimes a lower value gets tested after a higher value.

When you test an integer for a large number of different values, there is a specific language construct - switch statement - available. It was added for a purpose. Can you guess what purpose?

There are other errors too. You really have not spent much time looking at your code and tried to single-step it in your head. You post to a forum to ask for help unless you have already made sure that you have caught all the easy errors? Because you think that a prfessional developer who have this as occupation would send out all his code on forums to find someone to catch all the errors? But who would the legendary/magicall beings be who reads forum posts and catches trivial errors if the developers themselves don't spend time catching them?

Another thing - if this clock is battery-operated, it would consume 100% CPU power and hence a lot of batteries by the main loop that is constantly converting the individual time components into a string. Is it meaningful to try to build this string hundreds or thousands of times every second?

So you simply need to learn how to post code correctly.

Did you not read Andy Neil's response on your duplicate thread?

www.danlhenry.com/.../keil_code.png

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?

Thanks Mr.Westermark for your thoughts on my program. The thing is that I'm new to Keil C, so finding it difficult in the beginning to cope up with it.
As you said, the shouldn't be used in this program?
And is it that my variable minutes, seconds must have 60 counts even if i use (Minutes>=59)?
I don't know how many times i can be incremented in the ISR. It must be incremented only once? if yes then where in particular in my program.
and my last question, what should be Dispcount declared as?

I hope this isn't a scribbled one.

#include <AT892051.H>
#include <stdio.h>
#include <intrins.h>

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*/

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 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;

Yes I want to learn about how to post a source code and where

 is used cause I did not get a clear picture of the example given

So, this is a school assignment, then?

"The thing is that I'm new to Keil C"

But your school knows that - and they expect that, at this point in your course, you should have learned enough 'C' (most of it is not Keil-specific) to enable you to complete this task.

Perhaps it's time to review your course notes, textbook, etc...?

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.