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.
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; }
However I was using assembler(including LJMP command) and not c.
The LJMP command is not the issue. It's the limitations of the compiler (regarding program size and location) that you may run into at some point.
http://www.keil.com/demo/limits.asp
Back to your program:
1. You're writing delay routines in C. That is an absolute no-no. Don't. You will never get a specific delay this way. You may not get any delay at all, since the optimizer is free to completely remove these functions and any calls to them from the code, since they do not have side effects and do not return a value. For delays, use assembly or a timer.
2. "I did run the program in Keil simulator, but what happens is that all bits of P1 and P2 are constantly high when they should be flashing according to the four states I have defined."
You did read and internalize the specifics of the '51s ports ? Such as that there is a port state and a latch state, which may be different depending on the circuitry of the port (both internal and external) ?
3.
unsigned int x; for (x=0;x<65536;x++);
You do realize that the '51 is not a 32 bit system, and that unsigned int is a 16 bit value (0...65535) ?
None of your delay functions will ever return. They're endless loops !
4. Please please get some sort of textbook on elementary, basic C programming. Experienced programmers may refuse to further look at your code when they see the first unnecessary goto (as a rule of thumb: use goto only to exit deeply-nested loops and in certain cases of error handling). Read up on the various types of control structures that C offers (for, while, do/while, switch/case) and use them.
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". Preferably, the compiler should warn about a constant larger than 32767 if not suffixed with "u".
About goto: Yes, I have used it a number of times. But never for a state machine. That is very poor programming, and completely unmaintainable. The only way a goto is good to have, is as a multi-loop break statement, instead of inventing one or stupid exit variables.
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.