Code Not Functioning Like I Wanted It To

Projects Programming
1

Hey everyone,

I put together this code but it's not performing the way I want it to. I'm not sure if it's a hardware or a software issue but I understand that some hardware issues can be overcome with software fixes; maybe this is one of those cases.

Desired outcome: When one of the buttons is pressed ("LeftSwitch" or "RightSwitch"), the corresponding four lights are supposed to run their sequence, then check to see if that button is still pressed. If the button is still pressed, all four lights stay on. If the button is not still pressed, all four lights go off. Within 1/2 second of the button being released, the lights must be ready to run their sequence again. Once a sequence is started, it should run to completion.

Outcome with this code: When a button is pressed, the sequence runs and then stays on or turns off like it should. If the button is released and then pressed again after the sequence runs and turns off, all four lights come on and do not sequence. They will all come on at once and all turn off at once when the button is pressed and released. If some time is allowed to pass with no input (5-10 seconds) and then the button is pressed again, the sequence runs like it should. The sequence does not run to completion if the button is released before the sequence has finished.

Note: I am using the proper pull down resistors to clean up the input. I can post a video as well if the descriptions above aren't useful.

Thanks for looking and thanks for the input!!

unsigned long previousMillisLED6LEFT = 0;
unsigned long previousMillisLED6 = 0;
unsigned long previousMillisX = 0;

unsigned long previousMillisXLEFT = 0;

const int RightSwitch = 40;
const int LeftSwitch = 42;

int intervalLED6 = 300;
int intervalLED5 = 600;
int intervalLED4 = 900;
int intervalLED3 = 1200;
int intervalX = 25;
int intervalLED6LEFT = 300;
int intervalLED5LEFT = 600;
int intervalLED4LEFT = 900;
int intervalLED3LEFT = 1200;
int intervalXLEFT = 25;
// each LED gets a state varaible
boolean LED7state = false;     // the LED will turn ON in the first iteration of loop()
boolean LED6state = false;     // need to seed the light to be OFF
boolean LED5state = false;     // the LED will turn ON in the first iteration of loop()
boolean LED4state = false;     // need to seed the light to be OFF
boolean LED3state = false;     // the LED will turn ON in the first iteration of loop()
boolean LED2state = false;     // need to seed the light to be OFF
boolean LED1state = false;     // need to seed the light to be OFF
void setup() {
  Serial.begin(9600);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  pinMode(6, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT);
}
void loop() {
  // get current time stamp
  // only need one for both if-statements
  unsigned long currentMillis = millis();
  unsigned long currentMillisLEFT = millis();
  /////////////////////////////RIGHT SIDE CODE
  if (digitalRead(RightSwitch) == HIGH) {
    if ((unsigned long)(currentMillis - previousMillisX) >= intervalLED6) {
      digitalWrite(5, HIGH);
      previousMillisLED6 = currentMillis;
      if ((unsigned long)(currentMillis - previousMillisX) >= intervalLED5) {
        digitalWrite(4, HIGH);
        if ((unsigned long)(currentMillis - previousMillisX)  >= intervalLED4) {
          digitalWrite(3, HIGH);
          if ((unsigned long)(currentMillis - previousMillisX)  >= intervalLED3) {
            digitalWrite(2, HIGH);
            if ((unsigned long)(currentMillis - previousMillisX) >= intervalX) {
              previousMillisX = currentMillis;
            }
          }
        }
      }
    }
    if (digitalRead(RightSwitch) == LOW)
    {
      digitalWrite(5, LOW);
      digitalWrite(4, LOW);
      digitalWrite(3, LOW);
      digitalWrite(2, LOW);
    }
  }

  //////////////////////////////////////LEFT SIDE CODE
  if (digitalRead(LeftSwitch) == HIGH) {
    if ((unsigned long)(currentMillisLEFT - previousMillisXLEFT) >= intervalLED6LEFT) {
      digitalWrite(9, HIGH);
      previousMillisLED6LEFT = currentMillisLEFT;
      if ((unsigned long)(currentMillisLEFT - previousMillisXLEFT) >= intervalLED5LEFT) {
        digitalWrite(8, HIGH);
        // previousMillisLED5 = currentMillis;
        if ((unsigned long)(currentMillisLEFT - previousMillisXLEFT)  >= intervalLED4LEFT) {
          digitalWrite(7, HIGH);
          if ((unsigned long)(currentMillisLEFT - previousMillisXLEFT)  >= intervalLED3LEFT) {
            digitalWrite(6, HIGH);
            if ((unsigned long)(currentMillisLEFT - previousMillisXLEFT) >= intervalXLEFT) {
              previousMillisXLEFT = currentMillisLEFT;
            }
          }
        }
      }
    }
    if (digitalRead(LeftSwitch) == LOW)
    {
      digitalWrite(9, LOW);
      digitalWrite(8, LOW);
      digitalWrite(7, LOW);
      digitalWrite(6, LOW);
    }
  }
}
2

It is difficult to read the code, because you have so much unneeded junk. Please clean it up a bit.

To start with, get rid of all the "(unsigned long)" casts in the if statements. The variables are already unsigned long.

All the "intervals" should be declared unsigned long:

int intervalX = 25;
...
int intervalLED3LEFT = 1200;

Why do you redeclare these variables every pass through loop()? You can declare them "static", once.

  unsigned long currentMillis = millis();
  unsigned long currentMillisLEFT = millis();

What are all the "state" variables for? You don't use them. Hint: a state machine is a better idea than this attempt.

Finally, do you really intend all the ifs to be nested as they are? Compounded with timing, it is nearly impossible to follow such logic, and it would hurt my brain to try.

3 
 // only need one for both if-statements

unsigned long currentMillis = millis();
 unsigned long currentMillisLEFT = millis();

Your comment speaks the truth. You don't need two of these.

There's this rarely-used function provided by the Arduino framework called delay(). It's a baby function and totally unsuited to larger projects but maybe it is the right one to use here. The clue is this statement...

lights are supposed to run their sequence, then check to see if that button is still pressed

The "problem" with delay() is that the Arduino puts its metaphorical fingers in its ears and doesn't respond to any inputs. That - unusually - seems to be what you're asking for.

But you are so close to completion with the method you have, I wouldn't change it too much. Maybe you could try this...

unsigned long previousMillisRight = 0;
unsigned long previousMillisLeft = 0;

const int RightSwitch = 40;
const int LeftSwitch = 42;

int intervalLED6 = 300;
int intervalLED5 = 600;
int intervalLED4 = 900;
int intervalLED3 = 1200;

int intervalLED6LEFT = 300;
int intervalLED5LEFT = 600;
int intervalLED4LEFT = 900;
int intervalLED3LEFT = 1200;

void setup() {
  Serial.begin(9600);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  pinMode(6, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT);
}
void loop() {
  // get current time stamp
  // only need one for both if-statements
  unsigned long currentMillis = millis();
  /////////////////////////////RIGHT SIDE CODE
  if (digitalRead(RightSwitch) == HIGH) {
    if ((unsigned long)(currentMillis - previousMillisRight) >= intervalLED6) {
      digitalWrite(5, HIGH);
    }
    if ((unsigned long)(currentMillis - previousMillisRight) >= intervalLED5) {
      digitalWrite(4, HIGH);
    }
    if ((unsigned long)(currentMillis - previousMillisRight)  >= intervalLED4) {
      digitalWrite(3, HIGH);
    }
    if ((unsigned long)(currentMillis - previousMillisRight)  >= intervalLED3) {
      digitalWrite(2, HIGH);
    }
  } else {
    //(digitalRead(RightSwitch) == LOW)
    previousMillisRight = currentMillis;
    digitalWrite(5, LOW);
    digitalWrite(4, LOW);
    digitalWrite(3, LOW);
    digitalWrite(2, LOW);
  }


  //////////////////////////////////////LEFT SIDE CODE
  if (digitalRead(LeftSwitch) == HIGH) {
    if ((unsigned long)(currentMillis - previousMillisLeft) >= intervalLED6LEFT) {
      digitalWrite(9, HIGH);
    }
    if ((unsigned long)(currentMillis - previousMillisLeft) >= intervalLED5LEFT) {
      digitalWrite(8, HIGH);
    }
    if ((unsigned long)(currentMillis - previousMillisLeft)  >= intervalLED4LEFT) {
      digitalWrite(7, HIGH);
    }
    if ((unsigned long)(currentMillis - previousMillisLeft)  >= intervalLED3LEFT) {
      digitalWrite(6, HIGH);
    }
  } else {
    //(digitalRead(LeftSwitch) == LOW)
    previousMillisLeft = currentMillis;
    digitalWrite(9, LOW);
    digitalWrite(8, LOW);
    digitalWrite(7, LOW);
    digitalWrite(6, LOW);
  }
}

Note that this version starts out with all LEDs off when the button is first pressed. You might consider making intervalLED6 zero, so that one lights up right away.

As Jremington pointed out, by making the intervals ints, you cannot have any interval larger than 32.767 seconds. Probably not important for now, but this will catch you out in your next project.

4

MorganS:
There's this rarely-used function provided by the Arduino framework called delay(). It's a baby function and totally unsuited to larger projects but maybe it is the right one to use here. The clue is this statement...

The "problem" with delay() is that the Arduino puts its metaphorical fingers in its ears and doesn't respond to any inputs. That - unusually - seems to be what you're asking for.

I believe this project is for brake and indicator lights on a vehicle, so delay() should 100% not be used as unresponsiveness in the system could cause an accident.

5

I wanted to come back to the couple of forum posts I made to post an update in case it helps anyone else out. The system I ended up going with includes a bank of 16 transistors, one PNP and one NPN for each light since the lights (and most other electronics for automotive applications for that matter) are common ground. Then I used two Arduino Unos-one for each side. Using two boards allowed me to use delay() in my code rather than millis(). This was the right move for this application because I could get delay() to work 100% of the time whereas I found millis() to be unreliable. These Arduinos work independently of each other at the moment, I have a plan to connect them later for even more cool tricks. I wanted to go with an Arduino set up to give me some room for creativity. A system that just used a few simple electrical components could have given me the same result but now I can program any sequence to run when I hit the brake/turn signal/hazards ect.

Some people had safety concerns with this. I tested this system extensively in the garage before driving with it. I have driven it several hundred miles at this point without any sort of failure. Part of the safety aspect here was using delay() rather than millis().

Another note about the code-the car supplies a small amount of voltage to the lights at all times for some reason and I had to use an analog input rather than a digital input to account for that.

For anyone doing this project, below I have posted the code I used for just one side. The code is the same no matter which side it is running. The way it is wired allows for me to do that. I have also posted the link to a YouTube video of the final product with just one of the sequences I came up with. I hope the video shows up and continues to work into the future.

const int threshold = 615;
void setup()
{
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
}

void loop()
{
  if (analogRead(A0) > threshold)
  {
    digitalWrite(4, HIGH);
    delay(50);
    digitalWrite(3, HIGH);
    digitalWrite(4, LOW);
    delay(70);
    digitalWrite(3, LOW);

    digitalWrite(2, HIGH);
    delay(50);
    digitalWrite(3, HIGH);
    delay(50);
    digitalWrite(4, HIGH);
    delay(50);
    digitalWrite(5, HIGH);
    delay(200);

    while (analogRead(A0) > threshold)
    {
      digitalWrite(2, HIGH);
      digitalWrite(3, HIGH);
      digitalWrite(4, HIGH);
      digitalWrite(5, HIGH);
    }
  }
  if (analogRead(A0) < threshold)
  { digitalWrite(2, LOW);
    digitalWrite(3, LOW);
    digitalWrite(4, LOW);
    digitalWrite(5, LOW);
  }
}
6

jremington:
Why do you redeclare these variables every pass through loop()? You can declare them "static", once.

  unsigned long currentMillis = millis();

unsigned long currentMillisLEFT = millis();

Um... Two variables right next to each other, both initialized to millis() definitely make no sense.

However, what would be the point of declaring them static, I wonder???

No, don't declare your local variables static, unless you really really really have to. A static variable is a notably "heavier" and more expensive entity than an automatic variable.

The rule of the thumb is: Use local automatic variables whenever you can, use static or global variables only when you really have to.

Remember that static variable "lives forever". It always occupies memory and requires reading from and writing to memory to persistently maintain it value between the function calls.

Meanwhile, an automatic variable can very often be completely eliminated by being optimized into a register. And even when it is not eliminated, the need to "spill the registers" to sync them with memory typically occurs much less often with automatic variables.

Which is why automatic variables produce more efficient code. If you don't need to maintain variable's value between the function calls, don't use static.

Where static would actually be quite appropriate are declarations like these

unsigned long previousMillisLED6LEFT = 0;
unsigned long previousMillisLED6 = 0;
unsigned long previousMillisX = 0;

unsigned long previousMillisXLEFT = 0;

These are global variables. Why are they global? I.e. why are they given external linkage? Do you really need to link to them from other translation units? No?

If no, then don't give them external linkage. Do this

static unsigned long previousMillisLED6LEFT = 0;
static unsigned long previousMillisLED6 = 0;
static unsigned long previousMillisX = 0;

static unsigned long previousMillisXLEFT = 0;

This will give them internal linkage. All namespace-scope variables (file-scope in C parlance) that don't have to be visible in other translation units should be given internal linkage. This will help the compiler to generate more efficient code when working with these variables.

Unfortunately, tutorials here on arduino.cc fail to convey this important point to beginners. Also description of static in the reference seems to present it as something that can only be used inside functions, which is not true. Although admittedly, dumping all this information on a beginner might be a bit too much...

7

One way to isolate the problem would be to create a simulation of the control logic and test the logic in the simulation. Thus if the logic works in the simulation then the problem probably lies in the hardware. The following link describes how a simulation for running two sets of LEDs is created; it also includes a sketch that is based on the simulation.

"A Software Development Workflow for LED Sequences"

8

Aceattack52:
I tested this system extensively in the garage before driving with it.

Oh, but those bugs are sneaky critters.

...
  if (analogRead(A0) > threshold)
...
    while (analogRead(A0) > threshold)
...
  if (analogRead(A0) < threshold)
...

What happens if the value returned by analogRead(A0) is greater than the threshold then exactly the threshold?

What happens if that state is maintained for an extended period of time?

What happens if the value returned by analogRead(A0) crosses the threshold as execution flows from top to bottom?