Thread: I need a few little tweaks with my code, help much appreciated!

I was given a project a couple of years ago before I got into coding which stated: You are required to design a traffic light sequencing application that makes use of two traffic lights that could be used to control the flow of traffic at a 'T' junction. This application does not change the light sequences with the passage of time, but by user interaction with a push button. Each button push will advance the light sequence by one step. Using a second button, get the light sequence to reverse one step per button push
I have a code below which is a traffic light system, however i used a time delay, how would i go about changing it to the button?
im using a pic16f877 microcontroller and C language to code
here's the code :

Code:

 #include <pic.h>

#include <delay.c>


int i;


int start()

{

    for(;;)

    {

        PORTB = 0 ;

        RB2 = 1;

        RB4 = 1;

        for(i=1;i<20;i++)

        {

            DelayMs(250);

            {

                PORTB = 0 ;      

                RB1 = 1;

                RB4 = 1;

                RB5 = 1;

            }

        }

        for(i=1;i<20;i++)

        {

            DelayMs(250);

            {

                PORTB = 0 ;

                RB0 = 1;

                RB6 = 1;

            }

        }

        for(i=1;i<20;i++)

        DelayMs(250);

            {

                PORTB = 100000 ;

                RB0 = 1;

                RB1 = 1;

                RB5 = 1;

            }

        for(i=1;i<20;i++)

        {

            DelayMs(250)

            {

            }

        }

    }

}

start();


}


　

   

   

main()

{

  TRISB = 0; 

  PORTB = 0;

  start(); 

}

this was a team based project between three people and not just myself.

judging from this as well as your previous post, i suggest you do some reading on Event driven programming as well as C. This is a fairly easy task and will appear so once you've read up a little.

Let's break down what you need to do. The program starts, and certain lights are lit. Those specific lights are to remain lit until a button is pressed. When that button is pressed, other lights become lit. Those specific lights remain on until the button is pressed again.

You should be able to derive suitable pseudo-code just from that one paragraph alone. Before I go much further, see if you can come up with the steps required (in English, not code) to achieve your goal. Post what you come up with, and we'll go from there.

---

A few other things for your consideration:

- You shouldn't #include C files - if you're using an IDE, just add it to the project with its matching header file #include'd
- Your braces dont' line up - you have an extra closing brace after the "start()" function ... and an erroneous call to that function right before that brace.
- Use custom names for the port pins! We don't know how the circuit is wired, so we don't know which light each RB pin is driving. Something like this, for example:

Code:

#define LIGHT_GREEN_1  RB0
#define LIGHT_GREEN_2  RB1
#define LIGHT_YELLOW_1 RB2
#define LIGHT_YELLOW_2 RB3
#define LIGHT_RED_1    RB4
#define LIGHT_RED_2    RB5

// ...

if( /* something */ )
{
    LIGHT_GREEN_1 = 1;
    LIGHT_YELLOW_1 = 0;
    LIGHT_RED_1 = 0;
}

// ...

Also, you can organize your code much better by using a state machine. I'd suggest this approach as you redesign your code.

basically this attachment however only two traffic lights are needed and obviously a button or two as one is needed for a reverse sequence..
i'll use the first two traffic light components on the diagram as its easier to refer to, i'll attach a button to rc0 too.

here's my attempt (please bare with me on this one):
[code]
for (; //a superloop would be a good idea wouldn't it?
{
while a button is pressed
rbo will be lit;
rb7 will be lit too;

when the button is pressed again
rb1 will be lit;
rb6 will be lit;

when the button is pressed again
rb2 will be lit;
rb5 will be lit;

because light go ina sequence of : re & green, amber and amber, green and red?

Figuring out the exact light sequence is up to you and your assignment. I would probably have the following sequence myself:

Code:

Light 1   Light 2
-------   -------
Green     Red
Yellow    Red
Red       Red
Red       Green
Red       Yellow
Red       Red

Pretty good start on the psuedo-code. I'm going to nitpick one of your comments, as the distinction is important. You said, "while a button is pressed" when I think you meant "When a button is pressed". The former implies that things occur only while the button is being held down. The latter implies that things occur after a button has been pressed.

So you start out with the lights in a certain configuration. Since nothing else is changing unless a button is pressed, you'll probably be safe having two things in your main loop: (1) Output the LED control lines, and (2) Check if the button is pressed. If the button is not pressed, nothing changes and the loop continues. If the button is pressed, then you have to update the LED control lines.

Two important things about the switch inputs:

1. If you only want to advance the lights one state for each push of the button, you will probably want a flag so that your loop, when detecting a button press, will only react to it the first time. The flag would have to be cleared when the button is no longer being pressed. I'll leave the exact logic behind this up to you, if you choose to go this route.

2. You should call a brief delay after reading the button press (~25ms should suffice) for debounce.

Is this enough to get you going? Give an attempt at coding (or further pseudo-coding) and we'll continue development.

it wont let me post the code on here for some reason, no matte rhow many times I try to include the code tags it still wont work, I've reached the first sequence of the lights, here I am stuck as to how to continue the sequence with the next button press, I mean without the lights staying on
[code] bit button_up = 0;
bit toggle = 0;
void main(void)
{
TRISB = 0X00; //trisb input
TRISC = 0X00;//trisc output
TRISD = 0XFF;//trisd output
for (; //superloop
{
if (RD0 == 1) //if its not pressed then
button_up == 1;//button up is assigned 1
else
{
if (button_up == 1)
{
button_up = 0;
toggle = !toggle;
RB0 = toggle;//GREEN ON LIGHT 1
RC2 = toggle;//RED ON LIGHT 2
}
[\code]

That's strange why you can't properly post code. I don't know why that is. Allow me:

Code:

bit button_up = 0;
bit toggle = 0;

void main(void)
{
    TRISB = 0X00; //trisb input
    TRISC = 0X00;//trisc output
    TRISD = 0XFF;//trisd output

    for (;;)
    {
        if (RD0 == 1) //if its not pressed then
            button_up == 1;//button up is assigned 1
        else
        {
            if (button_up == 1)
            {
                button_up = 0;
                toggle = !toggle;
                RB0 = toggle;//GREEN ON LIGHT 1
                RC2 = toggle;//RED ON LIGHT 2
            }
        }
    }
}

You implemented the switch/flag concept well - good job on that!

Now you just have to flesh out the light changing routine. As of now, it only toggles between two states. The "toggle" method you currently have probably is not be best way to go about it, since there are several (more than two) distinct states.

There are many possible ways for you to approach this. I would again recommend the FSM (Finite State Machine, not Flying Spaghetti Monster) I linked in post #3. You would have several possible "states", each with its own distinct value. Then a variable will contain the current "state" value, and you can determine the light configuration based on the value of the "state" variable.

A very generalized example might be as follows:

Code:

#define STATE_L1_G_L2_R  0    /* Light 1 GREEN,  Light 2 RED */
#define STATE_L1_Y_L2_R  1    /* Light 1 YELLOW, Light 2 RED */
#define STATE_L1_R_L2_R  2    /* Light 1 RED,    Light 2 RED */
#define STATE_L1_R_L2_G  3    /* Light 1 RED,    Light 2 GREEN */
#define STATE_L1_R_L2_Y  4    /* Light 1 RED,    Light 2 YELLOW */
#define STATE_L1_R_L2_R  5    /* Light 1 RED,    Light 2 RED */
#define MAX_STATES       6

// ...

int state = STATE_L1_G_L2_R;

    // if button press...

    state++;

    if(state == MAX_STATES)
        state = STATE_L1_G_L2_R;

    switch(state)
    {
        case STATE_L1_G_L2_R:
            // set PORTB and PORTC values as needed
            break;
        case STATE_L1_Y_L2_R:
            // set PORTB and PORTC values as needed
            break;
        // etc...

I gave a bit more code than I normally would, because I feel that in this instance, seeing a bit of the code would be a lot easier than trying to explain it in words. Don't rush off and try to copy/implement what I have without first making sure you fully understand the concept I've tried to illustrate.

You'd also have to take the second button into account - you'd use similar code as with the first button, but go the other way.

If this seems too complicated for you, let us know. There are easier, though more messy, solutions that exist - I'm just offering one I think is better overall.

If you need any clarifications on any of these points, please do not hesitate to ask.

thank you, it's amazing how things which I've never come across before actually make more sense to me than some of the stuff I've been taught in the past, one question though in regards to setting the port values, I understand the rest, being the different states and assigning the lights to each state it's just how to pronounce which ports they attach to? as i'm only familiar with the toggle method, which like you said would only work properly with two values, either being a 1 or 0.
Would it be :[code] case STATE_L1_G_L2_r:
RB0=1[\code]?
I know that may be hard to understand the way I've done it, i'd just like to thank you for your time by the way, I really appreciate it!

To close a code tag, it needs to go the other way: [/code].

I'm not sure what you mean by "pronounce". But you might mean something like:

Code:

case STATE_L1_G_L2_R:
    LIGHT_1_GREEN = 0; //turn the light off
    LIGHT_1_YELLOW = 1;  //turn the light on
    DelayMs(250); //or something to wait for the lights to actually change
    state = STATE_L1_Y_L2_R;  //we're in the next state in the flow
    break;

You're very welcome.

Would it be...

Yes, it would be something like that. To function correctly, each state case should have all outputs accounted for. If a light is supposed to be on, you need to explicitly turn it on (set it to 1**). If a light is supposed to be off, you need to explicitly turn it off (set it to 0**).

There are really two ways you can go about this.

One would be to change the port pins individually. This would require a few more lines of code, but has the bonus that you can make the code self-documenting (assuming you give the port pins meaningful aliases, as I showed in post #3).

Code:

case STATE_L1_G_L2_R:
    
    LIGHT_GREEN_1 = 1;   /* explicitly set all light values for Light 1 */
    LIGHT_YELLOW_1 = 0;
    LIGHT_RED_1 = 0;

    LIGHT_GREEN_2 = 0;   /* explicitly set all light values for Light 2 */
    LIGHT_YELLOW_2 = 0;
    LIGHT_RED_2 = 1;

    break;

Another way would be to update the entire port in one go:

Code:

PORTB = 0x0A;

While slightly fewer lines of code are required for this version, it might be easier to get tripped up if you're a beginner. I'd recommend the first way I mentioned (controlling individual bits, like you have been doing).

---

** The schematic you showed does not indicate if you're sourcing or sinking current (positive and negative logic, respectively) - but your previous code snippets imply you're sourcing current; that is, driving the anode of the LED (I'm assuming LEDs, since lamps would require buffering from the MCU). Basically, I'm assuming positive logic (logic high == 1 == light on / logic low == 0 == light off).