Problem with code not looping in loop()

TL;DR: I'm having a problem with void loop(). It appears to be not looping. It executes my program once, but i can't get it to do it every time a button is pressed.

I want to trigger a function every time a button is pressed.

I have a command button which, when pressed, blinks a green LED 20 times and if I press a limit switch I need to blink a red LED 5 times.

I'm trying to keep my code easily readable and maintainable, soI've written functions for each; starting from the least complex function and nesting them within more complex functions. I'll spare the variable declarations in the following snippets, but the complete code is at the bottom of this post.

I've yet to make it a class, if I want to implement multiple objects, but for now I'm trying to nail down and iron out the core functions. Here's what I've done so far:

===============================================================================
1.- I started by blinking the green LED this way:


void blink_green_LED_10_times()
{
  if (number_of_green_LED_blinks < 20) // I know I said 10 blinks, but please see next comment on this quirk.
  {
    unsigned long current_time = millis();
    if (current_time - previous_time >= blink_interval)
    {
      previous_time = current_time;
      if (green_LED_state == LOW)
      {
        green_LED_state = HIGH;
      }

      else
      {
        green_LED_state = LOW;
      }

      digitalWrite(green_LED, green_LED_state); // QUIRK: Problem is that number_of_green_LED_blinks increases
      number_of_green_LED_blinks++;             //  when green_LED_state changes from low to high AND from high to low.
      Serial.println(number_of_green_LED_blinks);
    }
  }
}

===============================================================================
2.- I then wrote a function for the limit switch:

void blink_red_led_5_times()
{
  number_of_green_LED_blinks = 20;
  while (number_of_red_LED_blinks < 5)
  {
    digitalWrite(green_LED, LOW);
    digitalWrite(red_LED, HIGH);
    delay(back_off_interval);
    digitalWrite(red_LED, LOW);
    delay(back_off_interval);
    number_of_red_LED_blinks++;
  }
}

===============================================================================
3.- I then implemented the home switch by throwing the first two within a function which detects if the button was pressed:

void blink_green_LED_10_times_unless_homes_switch_is_pressed()
{
  home_switch_1 = digitalRead(button_D2);
  aft_home_switch_pressed = digitalRead(button_D3);
  if (home_switch_1 == HIGH && aft_home_switch_pressed == LOW)
  {
    blink_red_led_5_times();
  }

  else
  {
    blink_green_LED_10_times(); // Empty the syringe
  }
}

===============================================================================
4.- I then assigned it to a button with the following function:

void command_1()
{
  command_1_button = digitalRead(button_D4);
  while (command_1_button == HIGH)
  {
    blink_green_LED_10_times_unless_homes_switch_is_pressed();
  }
}

===============================================================================
5.- I drop command_1() inside loop():

void loop()
{
  command_1();
  // blink_green_LED_10_times_unless_homes_switch_is_pressed();
  // blink_red_led_5_times();
  // blink_green_LED_10_times();
}

===============================================================================
6.- And herein lies the problem: I upload the complete code(see bottom-most code) and it works just as expected. But if I press the command button again, nothing happens. It's almost as if void loop() isn't looping, but this would be insane. What am I doing wrong? Is it the function nesting?

NOTE:
Each step works fine by themselves. I commented and uncommented them as I progressed, just to check and see.

===============================================================================
Here's the complete code:

#define button_D2 2 // home switch
#define button_D4 4 // command button
#define green_LED 6
#define red_LED 7
int number_of_green_LED_blinks = 0;
int number_of_red_LED_blinks = 0;
unsigned long previous_time = 0;
unsigned long blink_interval = 250;
unsigned long back_off_interval = 125;
bool green_LED_state = LOW;
bool red_LED_state = LOW;
bool home_switch_1;
bool command_1_button;

void setup()
{
  Serial.begin(9600);
  pinMode(green_LED, OUTPUT);
  pinMode(red_LED, OUTPUT);
  pinMode(button_D2, INPUT);
  pinMode(button_D3, INPUT);
  pinMode(button_D4, INPUT);
  pinMode(button_D5, INPUT);
}

void loop()
{
  command_1();
  // blink_green_LED_10_times_unless_homes_switch_is_pressed();
  // blink_red_led_5_times();
  // blink_green_LED_10_times();
}

void command_1()
{
  command_1_button = digitalRead(button_D4);
  while (command_1_button == HIGH) // "if" statements don't work.
  {
    blink_green_LED_10_times_unless_homes_switch_is_pressed();
  }
}

void blink_green_LED_10_times_unless_homes_switch_is_pressed()
{
  home_switch_1 = digitalRead(button_D2);
  if (home_switch_1 == HIGH)
  {
    blink_red_led_5_times();
  }

  else
  {
    blink_green_LED_10_times(); // Empty the syringe
  }
}

void blink_red_led_5_times() // Move piston back 5 steps and stop, thus turning forward limit switch off (LOW).
{
  number_of_green_LED_blinks = 20;     // Hardcode stop. Stops piston from traveling forward.
  while (number_of_red_LED_blinks < 5) // If statements don't work here.
  {
    digitalWrite(green_LED, LOW);
    digitalWrite(red_LED, HIGH);
    delay(back_off_interval);
    digitalWrite(red_LED, LOW);
    delay(back_off_interval);
    number_of_red_LED_blinks++;
  }
}

void blink_green_LED_10_times()
{
  if (number_of_green_LED_blinks < 20) // WARNING 2: Piston stroke is 50mm. Multiply desired blinks by 2. Quirk of using green_LED_state. See next comment.
  {
    unsigned long current_time = millis();
    if (current_time - previous_time >= blink_interval)
    {
      previous_time = current_time;
      if (green_LED_state == LOW)
      {
        green_LED_state = HIGH;
      }

      else
      {
        green_LED_state = LOW;
      }

      digitalWrite(green_LED, green_LED_state); // QUIRK: Problem is that number_of_green_LED_blinks increases
      number_of_green_LED_blinks++;             //  when green_LED_state changes from low to high AND from high to low.
      Serial.println(number_of_green_LED_blinks);
    }
  }
}

No update in the loop

1 Like

where is command_1_button ever updated (i.e. read again) to Not be HIGH to break out of the while loop?

1 Like

How does it compile without button_d5 or button_d3 defined? This is not the sketch you use.

Hello turpialito

Use a logic analyzer to see what happens.

Insert Serial.println()´s at points of interrest.

Have a nice day and enjoy coding in C++.

1 Like

It is too looping in loop, trapped here in this endless loop:

loop(){...} is waiting for the endless while loop to finish.

Consider using 'if's along with "states" and "state machines"-- it sounds like your plan is for the thing to be idle, or blinking red or green for certain counts of lights. Your blink_green_LED_10_times() is a reasonable non-blocking routine that you could enable or disable just by changing the number_of_green_LED_blinks. You could move it directly into loop() and when it's time to do a series of green blinks, set number_of_green_LED_blinks=0; You could write a similar red-blink function, and put it in loop as well. (I'd write them in the opposite direction: count down to zero rather than up to 20, so you could use an if(number_of_green_LED_blinks == 0) to tell if you were done blinking, and initiate a series of blinks with number_of_green_LED_blinks = 20;)

Then your problem reduces to kicking off when which color led is supposed to be blinking, which is very unclear from your description.

I'm not sure I understand the logic you want but maybe code like this sloppy and untested partial code is close:

void setup(){
 ...
   number_of_green_LED_blinks = 100;
   number_of_red_LED_blinks = 100;
 ...
}

void loop(){
   command_1_button = digitalRead(button_D4);
   home_switch_1 = digitalRead(button_D2);
   if(home_switch == HIGH){
      number_of_red_LED_blinks = 0;
      number_of_green_LED_blinks = 100;
      green_LED_state = LOW;
   }

   if(command_1_button == HIGH){
      // start green and inhibit red:
      number_of_green_LED_blinks = 0;
      number_of_red_LED_blinks = 100;
      red_LED_state = LOW;
   }

   blink_green_LED_10_times();
   blink_red_LED_5_times_in_the_same_way_as_the_greenLED();

    // make sure the outputs match the desired states
    digitalWrite(green_LED, green_LED_state); 
    digitalWrite(red_LED, red_LED_state); 
}

If it isn't right, you can change the conditions based on other state variables like the blink counts. for example:

  // make sure red isn't still busy before initiating any green flashes:
if(number_of_red_LED_blinks < 20  && command_1_button == HIGH){
  // initiate green stuff
  ...
}

Your nesting logic is confusing things. Rather than the nesting and whiles, it's likely better to do the logic explicitly based on your state variables: {number_of_red_LED_blinks, number_of_red_LED_blinks}.

As a result of DaveX's brilliant code review, I can only add the following:

You have two main functions:

  1. set and start the timer
  2. check whether the timer has triggered.

These two functions must be executed independently within the loop() function.

Have a nice day and enjoy coding in C++.

1 Like

State machines worked. Looks like I was hell-bent on creating Rube Goldberg code. It's surprisingly easy for me to forget the K.I.S.S. principle. Thanks so much and cheers!

1 Like

Please post your working code.

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.