While Statements: LED's Won't turn off after "if to break" statement????

I have a menu set up, that displays options for 4 LED’s (Yellow, Red, Green, and White) that when selected “blink” off and on. They run in a loop, until a button is pressed.

When the button is pressed, the leds are supposed to go off :0 and then I am supposed to return to the menu.

I can return to the menu fine, but the LED will stay on, not blinking, just constant on…

I do have 220K resistors installed between the -LED pin an the ardino board, with the +LED going to the +5v rail on a bread board. The LED’s work fine

I have never coded C/ardino before so most of this is copypasta from other peoples work and a little “figuring thing out”

This is my code for one of the selected menu options. The other LED’s are the same, save for the LED pin is different, ofc.

void white( char* pMenuText, void *pUserData )
{
  int xz = 0;
  int buttonState = 0;
  while (xz < 100) {
  int led = 26;
  pinMode(led, OUTPUT);
  g_menuLCD.ClearLCD();
  g_menuLCD.getLCD()->setCursor( 0,0 );
  g_menuLCD.getLCD()->print("LED's Are On");
  g_menuLCD.getLCD()->setCursor( 0,1 );
  g_menuLCD.getLCD()->print("Program: White");
  pinMode(led, OUTPUT); 
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(500);
  digitalWrite(led, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);               // wait for a second
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(500); 
  buttonState = digitalRead(28);  // wait for a second
  if (buttonState == HIGH) {
   digitalWrite(led, LOW);
   break;
   
  }
  }
}

I do have 220K resistors installed between the -LED pin an the ardino board, with the +LED going to the +5v rail on a bread board. The LED's work fine

Does setting the LED pin on the Arduino LOW turn the LED on or off ? If I understand your description correctly it sounds to me as though it turns the LED on, although I doubt that you mean that you have use 220K resistors. Before you leave the LED function you set the LED pin LOW and turn the LED on.

I think you need to completely rewrite your code so it doesn’t use the delay() function. Use millis() and the technique in the Blink Without Delay example sketch. The demo several things at a time is an extended example.

Also, you should separate the business of checking the buttons from the business of displaying menus and flashing LEDs. If I understand things, you must have buttonState = digitalRead(28); in each of your colour functions. That is unnecessary duplication that just creates opportunities for error.

It would also make your code easier to follow if you put the pin number (28 in this case) into a variable with a meaningful name - for example buttonPin so that the code would be buttonState = digitalRead(buttonPin);

…R

UKHeliBob:

I do have 220K resistors installed between the -LED pin an the ardino board, with the +LED going to the +5v rail on a bread board. The LED's work fine

Does setting the LED pin on the Arduino LOW turn the LED on or off ? If I understand your description correctly it sounds to me as though it turns the LED on, although I doubt that you mean that you have use 220K resistors. Before you leave the LED function you set the LED pin LOW and turn the LED on.

Setting the led pin to low turns the LEDs off, the leds work fine while in the loop.

But, after the if condition is met, it's like it skips over that last "digitalWrite" function.

Thanks for your response aswell, Robin

That example may help longer term, as ultimately i plan to use the ardiuno to drive led strips... But i'd really like to know why the LEDs arent cutting off with the existing code ...

But, after the if condition is met, it's like it skips over that last "digitalWrite" function.

Put some Serial.prints inside the if test for the button press to make sure that the section of code is being run.

I am still confused about how the LEDs are wired. It sounds like the LED anode goes to 5V and the cathode goes to the Arduino pin via a resistor. Is that right and is the value of the resistor 220K ?

Correct, that is how the led is connected.

I had wanted to use serial.print to troubleshoot a different problem that I ended up fixing, but in unclear what serial print prints too?

Qdeathstar:
But i'd really like to know why the LEDs arent cutting off with the existing code ...

Is it possible they are going off but for such a short time that you don't notice?

You need to post all of your code so we can see how other stuff interacts.

...R

I cannot see the rest of your sketch, but this function is strange out of that context. Some of it make me wonder if we are missing parts of the function.

some comments/questions in the code

void white( char* pMenuText, void *pUserData )  //<<<<<you pass variables you don't seem to use?
{
  int xz = 0;//<<<<<<<<<<<<<<<  You never manipulate this variable in the function?
  int buttonState = 0;
  while (xz < 100) {
  int led = 26;  //<<<<<<<<<<<<<<<<<<<<<<<<<Usually done in setup()
  pinMode(led, OUTPUT);//<<<<<<<<<<<<<<<<Usually done in setup()
  g_menuLCD.ClearLCD();
  g_menuLCD.getLCD()->setCursor( 0,0 );
  g_menuLCD.getLCD()->print("LED's Are On");
  g_menuLCD.getLCD()->setCursor( 0,1 );
  g_menuLCD.getLCD()->print("Program: White");
  pinMode(led, OUTPUT); //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Again?
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(500);//<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<Lots of delays blocking the digitalRead(28) below
  digitalWrite(led, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);               // wait for a second
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(500); 
  buttonState = digitalRead(28);  // wait for a second
  if (buttonState == HIGH) {
   digitalWrite(led, LOW);
   break;
   
  }
  }
}

The reason that I’m passing variables i don’t seem to use is that i’m using script copied from other sources and mashed together. I wasn’t sure if those variables were being used to return to the menu after the function was completed, so i left them in just to be safe.

here is the entire code, its a mess… a lot of functions that i don’t use.

#include <LiquidCrystal.h>
#include "MenuEntry.h"
#include "MenuLCD.h"
#include "MenuManager.h"

const int buttonPin = 26;

const int LCDD7 = 11;
const int LCDD6 = 10;
const int LCDD5 = 9;
const int LCDD4 = 8;
const int LCDE  = 7;
const int LCDRS = 12;
int analogPin = 0;

MenuLCD g_menuLCD( LCDRS, LCDE, LCDD4, LCDD5, LCDD6, LCDD7, 16, 2);
MenuManager g_menuManager( &g_menuLCD);
unsigned int g_isDisplaying = false;
int g_timerTime = 23;
long g_timerRunning = false;
long g_timerTarget = 0;
long g_autoReset = false;
long g_stopMillis = 0;
long g_startMillis = 0;

void setupMenus()
{  
  g_menuLCD.MenuLCDSetup();  
  //Add nodes via a depth-first traversal order
  MenuEntry * p_menuEntryRoot;
  //Add root node
  //MenuEntry( char * menuText, void * userData/*=0*/, MENU_ACTION_CALLBACK_FUNC func);
  p_menuEntryRoot = new MenuEntry("Solid Colors", NULL, NULL);
  g_menuManager.addMenuRoot( p_menuEntryRoot );
  g_menuManager.addChild( new MenuEntry("White", NULL, white) );
  g_menuManager.addChild( new MenuEntry("Green", NULL, green) );
  g_menuManager.addChild( new MenuEntry("Red", NULL, red) );
  g_menuManager.addChild( new MenuEntry("Yellow", NULL, yellow) );
  
  g_menuManager.addSibling( new MenuEntry("Fading Colors", NULL, NULL ) );
  //Now we want to select the "Timer" entry so we can add children under that node
  //"Timer" is one down from "Stopwatch", so we issue the down command
  g_menuManager.MenuDown();
  g_menuManager.addChild( new MenuEntry("Lt Blue Drk Blue", NULL, NULL) );;
  g_menuManager.addChild( new MenuEntry("Orange, Green, Red", NULL, NULL) );
  g_menuManager.addChild( new MenuEntry("Yellow, Green, Blue", NULL, NULL) );
  //now move down to the "Time" node to add children by selecting the "Timer" node 
  //g_menuManager.MenuSelect(); <== Sub Sub Menu
  //Add "time"'s sibling "AutoReset" and select it
  g_menuManager.addSibling( new MenuEntry( "Chasing Colors", NULL, NULL) );
  g_menuManager.MenuDown();
  g_menuManager.addChild( new MenuEntry("Lt Blue Drk Blue", NULL, NULL) );;
  g_menuManager.addChild( new MenuEntry("Orange, Green, Red", NULL, NULL) );
  g_menuManager.addChild( new MenuEntry("Yellow, Green, Blue", NULL, NULL) );
  
  g_menuManager.addSibling( new MenuEntry( "Christmas Colors", NULL, NULL) );
  g_menuManager.MenuDown();
  g_menuManager.addChild( new MenuEntry("Red Green Blue", NULL, NULL) );;
  g_menuManager.addChild( new MenuEntry("Tinkle Twinkle", NULL, NULL) );
  g_menuManager.addChild( new MenuEntry("Christmas Tree", NULL, NULL) );
  
  g_menuManager.addSibling( new MenuEntry( "Holloween Colors", NULL, NULL) );
  g_menuManager.MenuDown();
  g_menuManager.addChild( new MenuEntry("Orange Chasing Purple", NULL, NULL) );;
  g_menuManager.addChild( new MenuEntry("Strobe", NULL, NULL) );
  g_menuManager.addChild( new MenuEntry("Pumpkins", NULL, NULL) );
  
  g_menuManager.addSibling( new MenuEntry( "Easter Colors", NULL, NULL) );
  g_menuManager.MenuDown();
  g_menuManager.addChild( new MenuEntry("Pink and Green", NULL, NULL) );;
  g_menuManager.addChild( new MenuEntry("Sunrise", NULL, NULL) );
  g_menuManager.addChild( new MenuEntry("Yellow Pink and Green", NULL, NULL) );
  
  g_menuManager.addSibling( new MenuEntry( "Patriotic Colors", NULL, NULL) );
  g_menuManager.MenuDown();
  g_menuManager.addChild( new MenuEntry("Red Blue White Solid", NULL, NULL) );;
  g_menuManager.addChild( new MenuEntry("Red Blue White Motion", NULL, NULL) );
  g_menuManager.addChild( new MenuEntry("Red Blue White Fade", NULL, NULL) );
  //Add "AutoReset"'s children
  //Use the built-in BOOL setting callbacks from MenuEntry.h: MenuEntry_Bool*CallbackFunc
  //g_menuManager.addChild( new MenuEntry( "Turn Reset On",  (void *) (&g_autoReset), MenuEntry_BoolTrueCallbackFunc ) );
  //g_menuManager.addChild( new MenuEntry( "Turn Reset Off", (void *) (&g_autoReset), MenuEntry_BoolFalseCallbackFunc ) );
  //g_menuManager.addChild( new MenuEntry("Back", (void *) &g_menuManager, MenuEntry_BackCallbackFunc) );

  //Add timer start and stop
  //g_menuManager.addSibling( new MenuEntry( "Countdown Start", NULL, TimerStartCallback) );
  //g_menuManager.addSibling( new MenuEntry( "Countdown Stop", NULL, TimerStopCallback) );
  //g_menuManager.addSibling( new MenuEntry("Back", (void *) &g_menuManager, MenuEntry_BackCallbackFunc) );
  
  //Get the selection state back to the root for startup and to add the last entry
  g_menuManager.SelectRoot();
  //g_menuManager.addSibling( new MenuEntry( "Credits", NULL, CreditsCallback) );
  //Make sure the menu is drawn correctly after all changes are done
  g_menuManager.DrawMenu();

  //g_menuManager.addSibling( new MenuEntry( "Draw Smiley", NULL, SmileyCallback) );
  
  g_menuLCD.getLCD()->createChar( 0, g_smiley );
  g_menuLCD.getLCD()->createChar( 1, g_frown );
}

I had to split it up into two sections, >9500 characters.

void setup() 
{           
  pinMode(buttonPin, INPUT);
  Serial.begin(115200);
  Serial.print("Ready.");
  setupMenus();
}



void loop() 
{
  int state;
  int x = analogRead (0);
 
  //Check analog values from LCD Keypad Shield
  if (x < 5) {
    state = 0;
  } else if (x < 100){
    //Right
    state = 1;
  } else if (x < 200){
   //Up
    state = 2;
  } else if (x < 400){
   //Down
    state = 3;
  } else if (x < 600){
    //Left
    state = 4;
  } else if (x < 800){
    //Select
    state = 5;
  }
     //If we have changed Index, saves re-draws.
   if (state != 0) {
      if (state == 2) {
          delay(200);
          g_menuManager.DoMenuAction( MENU_ACTION_UP );
      } else if (state == 3) {
          delay(200);
          g_menuManager.DoMenuAction( MENU_ACTION_DOWN );
      } else if (state == 1) {
          delay(200);
          if( g_isDisplaying )
              {
               g_isDisplaying = false;
               g_menuManager.DrawMenu();
              } 
      } else if (state == 4) { 
          delay(200);  
          g_menuManager.DoMenuAction( MENU_ACTION_BACK );
      } else if (state == 5) {
          delay(200);
          g_menuManager.DoMenuAction( MENU_ACTION_SELECT );
         }
      }
      //Save the last State to compare.
      state = 0; 
   //Small delay
  delay(5);
      
void white( char* pMenuText, void *pUserData )
{
  int xz = 0;
  int buttonState = 0;
  while (xz < 100) {
  int led = 26;
  pinMode(led, OUTPUT);
  g_menuLCD.ClearLCD();
  g_menuLCD.getLCD()->setCursor( 0,0 );
  g_menuLCD.getLCD()->print("LED's Are On");
  g_menuLCD.getLCD()->setCursor( 0,1 );
  g_menuLCD.getLCD()->print("Program: White");
  pinMode(led, OUTPUT); 
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(500);
  digitalWrite(led, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);               // wait for a second
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(500); 
  buttonState = digitalRead(28);  // wait for a second
  if (buttonState == HIGH) {
   digitalWrite(led, LOW);
   break;
   
  }
  }
}

void green( char* pMenuText, void *pUserData )
{
  int xz = 0;
  int buttonState = 0;
  while (xz < 100) {
  int led = 24;
  g_menuLCD.ClearLCD();
  g_menuLCD.getLCD()->setCursor( 0,0 );
  g_menuLCD.getLCD()->print("LED's Are On");
  g_menuLCD.getLCD()->setCursor( 0,1 );
  g_menuLCD.getLCD()->print("Program: Green");
  pinMode(led, OUTPUT); 
  digitalWrite(led, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);               // wait for a second
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(1000); 
  buttonState = digitalRead(28);  // wait for a second
  if (buttonState == HIGH) {
   digitalWrite(led, LOW);
   break;
  }
  }
}

void red( char* pMenuText, void *pUserData )
{
  int xz = 0;
  int buttonState = 0;
  while (xz < 100) {
  int led = 30;
  g_menuLCD.ClearLCD();
  g_menuLCD.getLCD()->setCursor( 0,0 );
  g_menuLCD.getLCD()->print("LED's Are On");
  g_menuLCD.getLCD()->setCursor( 0,1 );
  g_menuLCD.getLCD()->print("Program: Red");
  pinMode(led, OUTPUT); 
  digitalWrite(led, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);               // wait for a second
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(1000); 
  buttonState = digitalRead(28);  // wait for a second
  if (buttonState == HIGH) {
   digitalWrite(led, LOW);
   break;
  }
  }
}

void yellow( char* pMenuText, void *pUserData )
{
  int xz = 0;
  int buttonState = 0;
  while (xz < 100) {
  int led = 32;
   g_menuLCD.ClearLCD();
  g_menuLCD.getLCD()->setCursor( 0,0 );
  g_menuLCD.getLCD()->print("LED's Are On");
  g_menuLCD.getLCD()->setCursor( 0,1 );
  g_menuLCD.getLCD()->print("Program: Yellow");
  pinMode(led, OUTPUT); 
  digitalWrite(led, HIGH);   // turn the LED on (HIGH is the voltage level)
  delay(1000);               // wait for a second
  digitalWrite(led, LOW);    // turn the LED off by making the voltage LOW
  delay(1000); 
  buttonState = digitalRead(28);  // wait for a second
  if (buttonState == HIGH) {
   digitalWrite(led, LOW);
   break;
  }
  }
}
}

I had wanted to use serial.print to troubleshoot a different problem that I ended up fixing, but in unclear what serial print prints too?

It prints to the Serial monitor. Look for Tools/Serial Monitor in the IDE.

Correct, that is how the led is connected.

So you really do have a 220K resistor in series with the LED and the LED is basically connected between 5V and the Arduino pin. I strongly suspect that the resistor is actually 220 ohms, but if that is the case the LED would be on when the Arduino pin goes LOW

Sorry, yes 220 ohm.

I’ve already tried switching the setting from “LOW” to “HIGH” but there is zero effect on the LEDs.

I also put a delay before and after the instruction to turn the LED off but it had no effect on the led…

For the serial print, how would you view the output?

in your arduino app

Select: Tools
then: Serial Monitor

I've already tried switching the setting from "LOW" to "HIGH" but there is zero effect on the LEDs.

I think that says it all. Does the LED flash on/off with a 1 second period when in the function ?

Why is there a while loop in the function ? As the value of xz is not changed in the loop it is completely unnecessary.

BulldogLowell:
in your arduino app

Select: Tools
then: Serial Monitor

Thank you, that should make a lot of my endeavors easier...

UKHeliBob:

I've already tried switching the setting from "LOW" to "HIGH" but there is zero effect on the LEDs.

I think that says it all. Does the LED flash on/off with a 1 second period when in the function ?

Why is there a while loop in the function ? As the value of xz is not changed in the loop it is completely unnecessary.

The LEDs blink off and on while in the function. I'm using a while function because without it, the light only blinks once. I want it to blink until I press a button. The while loop accomplished that, but instead of shutting the led off, it stays on after the button press.

Thanks for all the help, there are quite a few things I've learned so far.

I’m using a while function because without it, the light only blinks once. I want it to blink until I press a button.

Then why not use while (digitalRead(28) == LOW)instead of using a variable that never changes to keep the while loop going ?

As a matter of interest have you got pull down resistors on the input pins or are they at a floating voltage when the button is not pressed ?

Qdeathstar:
Sorry, yes 220 ohm.

I’ve already tried switching the setting from “LOW” to “HIGH” but there is zero effect on the LEDs.

I also put a delay before and after the instruction to turn the LED off but it had no effect on the led…

For the serial print, how would you view the output?

You are using a while loop with delays and frankly speaking, you are not off to a good start there. both result in blocking which makes your program difficult to debug… hence your problems.

I’m going to steer you off track for a moment.

try looking at and loading the sketch that follows and watch the progression of its output as you press your button or enter a character using the Serial Monitor.

I tried to match it up to your hardware settings. Your pushbutton on pin28 and an LED on pin13 (PWM pin).

look at what’s happening in this basic sketch and perhaps it will convince you to re-think your approach with yours.

#include <math.h>
int ledPin = 13;  //should be a PWM pin
int buttonPin = 28;  //buttonPin
int lastPressed;
int state = 0;
unsigned long startTime;

void setup() 
{                
  // initialize the digital pin as an output.
  pinMode(ledPin, OUTPUT);
  pinMode(buttonPin, INPUT);  
}

void loop()
{
  int pressed = digitalRead(buttonPin); 
  if (pressed)
  {
    if (pressed != lastPressed)
    {
      state++;
    }
  }
  lastPressed = pressed;
  if (Serial.available())
  {
    char myChar = Serial.read();
    state++;
  }
  if (state > 4) state = 0;// add more light tricks by adding states
  if (state == 0)
  {
    myFade();
  }
  else if (state == 1)
  {
    fastBlink();
  }
  else if (state == 2)
  {
    slowBlink();
  }
  else if (state == 3)
  {
    digitalWrite(ledPin, HIGH);//Stays On
  }
  else if (state == 4) 
  {
    digitalWrite(ledPin, LOW);//Off
  }
}

void myFade()
{
  float val = (exp(sin(millis()/2000.0*PI)) - 0.36787944)*108.0;
  analogWrite(ledPin, val);
}

void fastBlink()
{
  if (millis() - startTime >= 50UL)
  {
    digitalWrite(ledPin, !digitalRead(ledPin));
    startTime += 50UL;
  }
}

void slowBlink()
{
  if (millis() - startTime >= 500UL)
  {
    digitalWrite(ledPin, !digitalRead(ledPin));
    startTime += 500UL;
  }
}

I have posted this sketch before, I hope you find it helpful and fun to learn.

Would you need a separate function for each led?