Led and button not working

  1. Hi everyone, I am building a traffic light system. So when the button is not pressed, led 1 and 4 will turn on for 4 seconds, then led 2 and 3 will turn on for 4 seconds. If the button is pressed, the led for pedestrian (led 5) will wait and turn on only when the led 2 is on (it means that when we press the button, if led 1 and 4 are on, then led 5 will wait until led 1 and 4 are off). Only led 5 and 2 will turn on at this time, others will be off. After 2 seconds, led 5 will be off, led 3 will be on for 2 seconds. Led 2 will be on for the entire time (4 seconds). Then we return to normal. But the problem is when I run the program, all 4 leds are on. Can anyone tell me where my code goes wrong?
    I use Arduino IDE but I write the code in C.
#include <avr/io.h>
#include <avr/interrupt.h>

volatile int second = 1;

volatile boolean fourSecond = false;
volatile boolean twoSecond = false;
volatile boolean buttonPressed = false;

enum light_state {SN, EW, PES, START_UP};
enum light_state state;

int main(void) {

  DDRB = 0xFF;

  DDRD &= ~(1 << DDD2); //Set button as input

  EIMSK |= (1 << INT0); //enable PORTB2 as external interrupt
  EICRA |= (1 << ISC01); //detect falling edge
  EIFR |= (1 << INTF0); //clear flag

  TCCR1B |= (1 << WGM12) | (1 << CS12) | (1 << CS10); //turn on CTC mode and set up Timer 1 with the prescaler of 1024
  TIMSK1 = (1 << OCIE1A); //set CTC compare value to 2 Hz at 16 MHz AVR clock , with a prescaler of 1024
  OCR1A = 7812; //enable output Compare A Match Interrupt
  sei();
  while (1) {
    if (!buttonPressed) { //when the button is not pressed
      switch (state) {
        case SN:
          PORTB |= (1 << PORTB1) | (1 << PORTB4);
          PORTB &= ~((1 << PORTB2) | (1 << PORTB3));
          state = EW;
          break;
        case EW:
          PORTB &= ~((1 << PORTB1) | (1 << PORTB4));
          PORTB |= (1 << PORTB2) | (1 << PORTB3);
          state = SN;
          break;
        case START_UP :
          PORTB &= ~((1 << PORTB1) | (1 << PORTB2) | (1 << PORTB3) | (1 << PORTB4));
          state = SN;
          break;
        default:
          state = START_UP;
          break;
      }
    } else if (state == SN && buttonPressed == true) { //when button is pressed and the state is SN
      switch (state) {
        case PES:
          PORTB |= (1 << PORTB2) | (1 << PORTB5);
          PORTB &= ~((1 << PORTB1) | (1 << PORTB3) | (1 << PORTB4));
          state = SN;
        case SN:
          PORTB |= (1 << PORTB2) | (1 << PORTB3);
          PORTB &= ~((1 << PORTB1) | (1 << PORTB4) | (1 << PORTB5));
          state = PES;
          break;
        case START_UP :
          PORTB &= ~((1 << PORTB1) | (1 << PORTB2) | (1 << PORTB3) | (1 << PORTB4) | (1 << PORTB5));
          state = PES;
          break;
        default:
          state = START_UP;
          break;
      }
      buttonPressed = false;
      twoSecond = false;
    }
  }
}

ISR(INT0_vect) {
  buttonPressed = true;
}

ISR(TIMER1_COMPA_vect) {
  if (buttonPressed == false) {
    if (second == 4) {
      second = 1;
    } else {
      second++;
    }
  } else {
    if (second == 2) {
      second = 1;
    } else {
      second++;
    }
  }
}

Why are you using an input interrupt (and a timer interrupt!) for a simple push button switch?

i was required to do it

Hello
Why you don´t use the functions digitalRead() and digitalWrite() ?

Then, at least, debounce the switch.

I'm always suspicious when I see switch-case statements nested in 'if' statements. Normally, a state machine should have "one big" switch case for the states, any if-else logic should be contained in the state code for each case.

Also, and I refuse to even read this code without it, there should be some documentation about which ports are connected to which LEDs. Especially here, where there is no high level function like digitalWrite() to make it explicit in the code.

i write it in C so no digitalWrite()

I know. What I am saying, because of that you need to document your program with comments. There are no comments about the timer configuration either, just some "what" comments but no "why" comments. No indication of the timer period...

Also, not to slice things too finely, but digitalWrite() is C.

If I were grading your work, I would give you some points for some technical insight, but ding you hard for the lack of internal documentation.

thanks for the feedback, i tried to change that

Did you ever write any previous code, to simply turn the LEDs on and off?

yep but i am not allowed to use digitalWrite() or something like that

Did it work?

yes sir. It worked

If you do, don't edit your original post. Post it as a new message.

Then why didn't you make functions out of them? It would make your job 10x easier.

while (1) {
    _delay_ms(500);
      switch (state) {
        case SN:
          PORTB |= (1 << PORTB1) | (1 << PORTB4);
          PORTB &= ~((1 << PORTB2) | (1 << PORTB3));
          state = EW;
          break;
        case EW:
          PORTB &= ~((1 << PORTB1) | (1 << PORTB4));
          PORTB |= (1 << PORTB2) | (1 << PORTB3);
          state = SN;
          break;
        case START_UP :
          PORTB &= ~((1 << PORTB1) | (1 << PORTB2) | (1 << PORTB3) | (1 << PORTB4));
          state = SN;
          break;
        default:
          state = START_UP;
          break;
      }
}

what I'm trying to do is that I want to replace the delay_ms for timer. i want to user timer 1 to generate the 4 seconds

You should develop separate functions for everything, even if they are written in low level code. Or should I say, especially if they are. Then tie them together in a high level main program.

I know this may seem unhelpful, but otherwise you're just asking people to shovel your dirt pile (they have to sit and figure out the whole Mensa puzzle).

I don't see how your professor could fault you for doing so.

thanks for your advice

Do I sense some "but..." with that?

nah, i try to divide my code first and put them together

If I were marking it, I would give you extra points for that... even though it makes it easier (once you get used to it). Also please do bring the results back here if it doesn't help...