I am trying to program the 8051 for a "Pelican" type pedestrian road crossing. It should control the red, amber and green lights for the traffic, the red and green pedestrian lights, and the bleeper. I have written some code but unfortunately it does not work. Bits 0 to 5 of P1 are supposed to control the lights and the bleeper and bit 0 of P2 is supposed to control the pedestrian button. Any ideas where the problem is? Code size limit is 2K. Thanks! Marko
#include <reg51.h> sbit red_traf=P1^0; sbit amber_traf=P1^1; sbit green_traf=P1^2; sbit red_ped=P1^3; sbit green_ped=P1^4; sbit bleeper=P1^5; sbit button=P2^0; unsigned char count; void delay_vshort() { unsigned int x; for (x=0;x<65536;x++); } void delay_short() { unsigned int x; for (x=0;x<131072;x++); } void delay_long() { unsigned int x; for (x=0;x<1048576;x++); } main() { P2 = 0xff; s0: green_traf=1; red_ped=1; if (button==1) goto s1; else goto s0; s1: amber_traf=1; red_ped=1; delay_short(); goto s2; s2: red_traf=1; green_ped=1; bleeper=1; delay_long(); goto s3; s3: for (count=0;count<5;count++) { amber_traf = 1; green_ped = 1; delay_vshort(); amber_traf = 0; green_ped = 0; delay_vshort(); } goto s4; s4: amber_traf=1; red_ped=1; delay_short(); goto s0; }
Hi.
You have Evalution version with restriction for compile program for single 51 !!!
Please read >
# The startup code generated includes LJMPs. Code generated cannot be used in single-chip devices that support 2K Bytes or less of program space. # Programs start at offset 0x0800. Programs generated with the evaluation software may not be programmed into single-chip devices with less than 2K Bytes of on-chip ROM.
Hi! I am using the Phillips P89C664 device and I have done some very basic stuff on it so far and everything worked in simulator and on the actual board. However I was using assembler(including LJMP command) and not c.
I would expect a decent compiler to generate a warning about too large constant, if it sees the value 65536 without the suffixed "l" or "ul". <p>
I would even expect that a compiler warns about meaningless comparisons, such as (unsigned value < 0) or (16 bit unsigned value > 65536).
But, then again, some people tend to ignore compiler warnings, since they don't abort the compilation process.
I'd like to know where this code is to be used, I'd be scared to cross the street when the sign control is coded by a programmer that inexperiencecd.
Marko, get the experience in an area where life and limb is not affected by 'mistakes' in the code.
Erik
I'd like to know where this code is to be used,
I guess (and hope) that the code is merely being written for self-educational purposes.
"4. Please please get some sort of textbook on elementary, basic C programming."
Don't even mention "BASIC"...! ;-)
Reading list: http://www.keil.com/books/8051books.asp www.8052.com/books.phtml
The ACCU (Association of C & C++ Users) also has a whole load of book reviews - including a section on embedded:
brian.accu.org/.../
Quite a number of schools have small boards with traffic lights, since they represent an interesting concept and the end goal is easy to grasp.
It is easy to start with a fully timer-based solution, and then extend it with faked sensors, that the logic should respond to.
Thanks for all suggestions so far. I do ralize that the code is very poorly written, however it is written for self educational purposes only. I am not going to use it for a real pedestrian crossing - I am not insane. Again all I wish is to make the program work and learn from it. I did rewrite the code because there were some obvious mistakes in there regarding the logic behind it. Here is the code:
#include <reg664.h> //Register map for 8051. sbit red_traf=P1^0; //Red traffic light is connected to bit 0 of P1. sbit amber_traf=P1^1; //Amber traffic light is connected to bit 1 of P1. sbit green_traf=P1^2; //Green traffic light is connected to bit 2 of P1. sbit red_ped=P1^3; //Red pedestrian light is connected to bit 3 of P1. sbit green_ped=P1^4; //Green pedestrian light is connected to bit 4 of P1. sbit bleeper=P1^5; //Bleeper is connected to bit 5 of P1. sbit button=P2^0; //Pedestrian button is connected to bit 0 of P2. unsigned char count; void delay_vshort() //This defines a very short delay. { unsigned int x; for (x=0;x<65536;x++); } void delay_short() { //This defines a short delay. unsigned int x; for (x=0;x<131072;x++); } void delay_long() //This defines a long delay. { unsigned int x; for (x=0;x<1048576;x++); } main() { P1 = 0x00; //P1 defined as an output port. P2 = 0xff; //P2 defined as an input port. s0: green_traf=1; //Green traffic light is on. red_ped=1; //Red pedstrian light is on. if (button==1) goto s1; //Check if button was pressed. else goto s0; s1: green_traf=0; //Switch off green traffic light. amber_traf=1; //Switch on amber traffic light. delay_short(); //Leave amber traffic light on for a short time. s2: green_traf=0; //Switch off green traffic light. red_traf=1; //Swich on red traffic light. red_ped=0; //Switch off red pedestrian light. green_ped=1; //Switch on green pedestrian light. bleeper=1; //Switch on the bleeper. delay_long(); //Remain in the same state for a long time. s3: red_traf=0; //Switch off red traffic light. for (count=0;count<5;count++) //This will make amber traffic and green pedestrian light //flash five times. { amber_traf = 1; //Switch amber traffic light on. green_ped = 1; //Switch green pedestrian light on. delay_vshort(); //Wait for a very short time. amber_traf = 0; //Switch both previous lights off. green_ped = 0; delay_vshort(); } s4: bleeper=0; //Turn off the bleeper. green_ped=0; //Turn off green pedestrian light. amber_traf=1; //Turn on amber traffic light. red_ped=1; //Turn on red pedestrian light. delay_short(); //Wait for a short time. goto s0; //Return to the beginning. }
The whole idea is to use the pin 0 of P2 as an input(=pedestrian button). But when I run the program the traffic amber and red pedestrian light stay on indefinetly. Does anybody know if I defined P1 and P2 as input and output ports correctly? I thank you again for your precious time. Regards, Marko
I had to make your code readable to comment on it, so I include it for the benefit of otheres
include <reg664.h> //Register map for 8051. sbit red_traf=P1^0; //Red traffic light is connected to bit 0 of P1. sbit amber_traf=P1^1; //Amber traffic light is connected to bit 1 of P1. sbit green_traf=P1^2; //Green traffic light is connected to bit 2 of P1. sbit red_ped=P1^3; //Red pedestrian light is connected to bit 3 of P1. sbit green_ped=P1^4; //Green pedestrian light is connected to bit 4 of P1. sbit bleeper=P1^5; //Bleeper is connected to bit 5 of P1. sbit button=P2^0; //Pedestrian button is connected to bit 0 of P2. unsigned char count; void delay_vshort() //This defines a very short delay. { unsigned int x; for (x=0;x<65536;x++); } void delay_short() //This defines a short delay. { unsigned int x; for (x=0;x<131072;x++); } void delay_long() //This defines a long delay. { unsigned int x; for (x=0;x<1048576;x++); } main() { P1 = 0x00; //P1 defined as an output port. P2 = 0xff; //P2 defined as an input port. s0: green_traf=1; //Green traffic light is on. red_ped=1; //Red pedstrian light is on. if (button==1) goto s1; //Check if button was pressed. else goto s0; s1: green_traf=0; //Switch off green traffic light. amber_traf=1; //Switch on amber traffic light. delay_short(); //Leave amber traffic light on for a short time. s2: green_traf=0; //Switch off green traffic light. red_traf=1; //Swich on red traffic light. red_ped=0; //Switch off red pedestrian light. green_ped=1; //Switch on green pedestrian light. bleeper=1; //Switch on the bleeper. delay_long(); //Remain in the same state for a long time. s3: red_traf=0; //Switch off red traffic light. for (count=0;count<5;count++) //This will make amber traffic and green pedestrian light flash five times. { amber_traf = 1; //Switch amber traffic light on. green_ped = 1; //Switch green pedestrian light on. delay_vshort(); //Wait for a very short time. amber_traf = 0; //Switch both previous lights off. green_ped = 0; delay_vshort(); } s4: bleeper=0; //Turn off the bleeper. green_ped=0; //Turn off green pedestrian light. amber_traf=1; //Turn on amber traffic light. red_ped=1; //Turn on red pedestrian light. delay_short(); //Wait for a short time. goto s0; //Return to the beginning. }
go through your code and follow it under the assumption that the button is held only for a millisecond
fix what you see by that and then come back
Thanks for all suggestions so far.
Did you read my comments about the size of an integer in '51 C ?
All of your delay loops are still endless loops. They will never return. You program will enter an endless loop before getting to s2. Does the compiler emit any warnings ?
I am afraid I do not understand.
step through your code 'on paper' follow the process when the button is only detected pressed once.
THAT is known as debugging
1.I did fix that but the problem is that now the delays are way too small. How do I program three different delays each 0.5s, 4s and 10s long respectively? 2.The compiler did not emit any warnings apart from these: TRAFFICLIGHT.C(46): warning C280: 's2': unreferenced label TRAFFICLIGHT.C(54): warning C280: 's3': unreferenced label TRAFFICLIGHT.C(66): warning C280: 's4': unreferenced label To get rid of these warnings I was using unnecessary goto statements at the end of each state. Regards, Marko
A permanent fix for timing loops - use the hardware. You might use a timer in the chip to count x clock cycles.
The quickie: Nest several loops. Create a function that takes about 1 ms.
Call that function 500 times to get a 0.5 second loop.
Addendum: Just note that a delay created using a loop can (and most often will) greatly change in speed depending on used compiler, and used optimization mode, memory model, version of the compiler, ...
Don't rely on it for production code.
Just note that a delay written in C created using a loop can (and most often will) greatly change in speed depending on used compiler, and used optimization mode, memory model, version of the compiler
a delay loop written in assembler, however, only is affected by the execution of interrupts.
I do ralize that the code is very poorly written, however it is written for self educational purposes only.
You have that dangerously backwards. Knowingly writing poor code may sometimes be justified by pressing circumstances (e.g. momentary lack of time to do it right), but never in a setting of self-education.
If you can't do it properly with practically infinite amounts of time on your hands, how will you ever do an acceptable job under stress?
I do not work in a programming business and most probably never will. As far as my area of expertise is concerned I do think that I do more than an acceptable job under stress.
Thanks for your help guys. I have learned a great deal about philosophical issues in programming. Unfortunately I did not learn that much about actual coding itself. Consider the matter ad acta. Thanks again! Best regards, Marko
Unfortunately I did not learn that much about actual coding itself.
But you did learn (or at least should have learned) quite a lot from this thread.
Delays generated using a compiler (might be a C, Pascal or whatever compiler) does give the possibility of unexpected changes to the delay time.
Optimization of loops may sometime result in the loop being completely removed - a compiler that doesn't notice any side effect are free to ignore the meaningless code.
It is possible to generate longer delays, by generating delays of known length and call that delay from a loop.
Using numeric constants larger than supported can give results that may look non-obvious.
The recommended method for creating delays are using timers (unless the delay should be extremely short, in which case you normally just add one or more NOP statements in assembler).
Goto isn't forbidden, but shuld normally not be used for implementing state machines or loops. Their best - and possibly only - suggested use is to break out of multiple loops.
Using tabs to indent source, results in slightly smaller source files, but the amount of indentation can't be guaranteed.
Mixing spaces and tabs when indenting generates code that may look really funny (or not) when displayed with a different tab size.
Setting breakpoints - or single-stepping - the code in the simulator would have shown problems with the delay functions.
Some compilers generates warnings that are important to look at. Some compilers do not generate so many warnings, so you can't always rely on warnings/errors to catch all possible errirs in the source.
You got some references to literature.
I problably missed some things in the thread, but philosophy aside: There was definitely a lot to learn about programming in this thread.
That makes me want to sit down and cry.
Why did you not learn anything about coding ?
The first problem your code runs into was described in great detail.
Ways to tackle the next problems you're going to run into were described in good detail.
Good coding practices were described (I'm sorry if you mistook them for philosophical issues. Once you've been trying to unravel a sparsely commented, goto-infested hunk of spaghetti code, you are going to realize that good coding practices are essential in everyday programming and not an abstract matter of philosophy.).
Ah, here's another one: Comments ! Uncommented source code is worthless. Everyone (including yourself) will have a hard time understanding what the code is supposed to do without comments. If you don't have a clear perception of what the code is supposed to do, it becomes very hard (close to impossible) to find out where and why the code is not doing what it is supposed to do.
Don't play the macho tough guy programmer who can memorize every tiny bit of information about his programs and keep that memory for months or years. Chances are, you can't (like most others programmers). Even if you can, you're going to make the life of any other programmer you deal with (that includes those you ask to look at your source code and give you hints) miserable.
Good commenting practices are even more important than having ultra-elite coding skills. Once you've got the comments down (yes, write them first), generating the appropriate code is a fairly simple matter (where most problems can be solved by just consulting a textbook).
Uncommented source code
there is no such thing, if there are no comments, it is not code, it is scribbles
One way to increase your delay is to add NOPs (the asm code for No Operation Keil gives you a macro for it
void delay(unsigned int msec) { unsigned int x; while (msec > 0) { // YOU Need to figure out the number of nops // and how big x needs to be // since x is an unsigned int it must be less than 65535 for (x=0;x<9999;x++) { _nop_; _nop_; _nop_; _nop_; _nop_; } msec--; } } the time a nop uses is different for different cpus The standard is 1/12 the crystal frequency. This will get you started. You would move on to using the timer interupt later. the interupt also give you the benifit that your cpu is not doing nothing during the delays. It can do something else during the delay time.
1."Goto" statement has been replaced with while. 2.Delays are generated by using 8051's internal timer. 3.The code is now hopefully properly commented.
What I would like to do now is to extend the whole thing a bit by adding a sort of a lock-out function for the pedestrian button so that green pedestrian light cannot go green too quickly since the last time the button was pressed because that would hinder the flow of traffic. I do have a general idea of how to do that using a counter/timer, but implementation is a bit tricky for me because of my lack of skills. The code should probably look something like this: 1.when a button is pressed a counter should be started 2.if a button is pressed again the counter should be checked to see if it has reached a certain level 3.if the level has been reached the program should continue 4.if the level has not been reached the counter should continue counting till the level is reached 6.when the level is reached stop the counter and continue with the program
Would that be a good approach? Any comments regarding the code?
Regards, Marko
P.S. The code so far:
#include <reg664.h> //register map for 8051 #define ON 1 #define OFF 0 sbit red_traf = P1^0; //red traffic light is connected to bit 0 of P1 sbit amber_traf = P1^1; //amber traffic light is connected to bit 1 of P1 sbit green_traf = P1^2; //green traffic light is connected to bit 2 of P1 sbit red_ped = P1^3; //red pedestrian light is connected to bit 3 of P1 sbit green_ped = P1^4; //green pedestrian light is connected to bit 4 of P1 sbit bleeper = P1^5; //bleeper is connected to bit 5 of P1 sbit button = P2^0; //pedestrian button is connected to bit 0 of P2 unsigned char count; void delay(int del) //delay procedure { int c=0; TMOD=0x01; //CT1 not used, CT0 as a timer, in mode 01 TR0=0; TF0=0; do { TH0=0; //Load CT0 with zero to get TL0=0; //the longest possible time. TR0=1; //start timer while(!TF0); //while Timer Flag is low, do nothing i.e.wait till it goes high TR0=0; //stop timer TF0=0; //make timer flag low again c++; } while (c<del); } main() { while (1) //infinite loop { P1 = 0x00; //reset all bits of P1 P2 = 0xff; //Make all bits 1, so that port2 works as //an input port. /* Go traffic */ // traffic: G = ON, A = OFF, R = OFF // pedestrians: G = OFF, R = ON green_traf = ON; red_ped = ON; while (button) {} // wait for button // traffic: G = OFF, A = ON, R = OFF // pedestrians: G = OFF, R = ON - no change green_traf = OFF; amber_traf = ON; delay (30); // short delay /* Go pedestrians */ // traffic: G = OFF, A = OFF, R = ON // pedestrians: G = ON, R = OFF amber_traf = OFF; red_traf = ON; red_ped = OFF; green_ped = ON; bleeper = ON; // turn bleeper on delay (100); // long delay // traffic: G = OFF, A = ON, R = OFF // pedestrians: G = ON, R = OFF bleeper = OFF; //Turn off the bleeper red_traf=OFF; //Switch off red traffic light for (count=0;count<7;count++) //This will make amber traffic and green pedestrian light flash five times { amber_traf=ON; //Switch amber traffic light on green_ped=ON; //Switch green pedestrian light on delay(5); //Wait for a very short time amber_traf=OFF; //Switch both previous lights off green_ped=OFF; delay(5); } } //infinite loop } //main
Neil Kurzman thanks for your comments. They were very useful.
Think a bit more about your comments.
Make sure that your functions and variables have names so that you never have to make comments about what you do.
You seldom want to write comments that say _what_ you do - unless you have a very hairy algorithm. You normally want to write comments about _why_ you do things.
Why use comment "turn off bleeper" for a line like:
bleeper = OFF;
Why comment delay(100) as "long delay"? How long is it? Why do you have a delay? If the delay function makes delays in ms - rename it as delay_ms(delay) and let the reader decide if it is a long delay or not.
Now you are programming. You need a plan. One way to do this is to have a periodic "Event" either in an interupt if it is short. Else, the interupt sets a flag and the event is handled from a function call in main.
Lets assume you set a flag every 100ms in an interupt.
when this flag is seen in the main loop you can decrement all your timers by 1 tick.
Example if(tickFlag) { tickFlag = 0; // clear tick flag if(Counter1 > 0)Counter1--;// dec counter down to 0 }
Then assuming Counter1 was you timer lock out. You would ignore the button if the counter is not 0;
This is one posible way. There are may ways to do this task.
View all questions in Keil forum