Correct way of having loop end when required?

Question

I'm brand new to Arduinos. I'm currently trying to modify a script from here which at the moment, simply turns an LED on or off depending on which HTTP GET request it receives. However, I want to modify this to have the LED modulate on/off when presented with the On HTTP GET request and then simply turn off all together when presented with the Off HTTP GET request. However, I have two conflicting methods that I have found about how to achieve this; the first is:

int value = LOW;
    if (request.indexOf("/LED=ON") != -1)  {
      value = HIGH;
      do
      {
      digitalWrite(ledPin, HIGH);
      delay(3000);
      digitalWrite(ledPin, LOW);
      delay(5000);
      value = HIGH;
      } while (value = HIGH);
    }
    if (request.indexOf("/LED=OFF") != -1)  {
      digitalWrite(ledPin, LOW);
      value = LOW;
    }

Wheareas the other is:

  int value = LOW;
    if (request.indexOf("/LED=ON") != -1)  {
      value = HIGH;
      void loop() {
      digitalWrite(ledPin, HIGH);
      delay(3000);
      digitalWrite(ledPin, LOW);
      delay(5000);
      value = HIGH;
      }
    }
    if (request.indexOf("/LED=OFF") != -1)  {
      digitalWrite(ledPin, LOW);
      value = LOW;
    }

As you can see, the first uses do...while and the second uses loop(). I have a feeling that the first of the two scripts would be more suitable as it should cut out as soon as value is set to LOW (i.e. when requested to turn off)

However, I would really appreciate it if someone could look through the two scripts and tell me which of the two is the correct way to achieve this sort of cut off when the HTTP GET Off is requested (if either for them even is the correct way to do this!).

Thank you in advance for your help,

Kind regards, Tom

Edgar Bonet · Accepted Answer · 2018-07-21T09:06:40.433

As pointed out by Ignacio Vazquez-Abrams and jsotola, you need to use a state machine, like the one from the Blink Without Delay Arduino tutorial. Yours would be slightly more complicated because you need a way to turn it on and off, and because you use different on and off periods. Here is an example code that abstract the blinking logic into three functions:

update_led() takes care of the blinking, and it has to be called quite often in order to achieve a smooth blinking
start_blinking() and stop_blinking(), as their names suggest, are used to turn the blinking state machine on and off.

const uint32_t ON_TIME  = 3000;
const uint32_t OFF_TIME = 5000;

bool led_blinking;     // are we blinking the LED?
bool led_on;           // is the LED currently on?
uint32_t last_toggle;  // last time it toggled while blinking

void update_led() {
    if (!led_blinking) return;
    uint32_t now = millis();
    if (led_on && now - last_toggle >= ON_TIME) {
        digitalWrite(ledPin, LOW);
        led_on = false;
        last_toggle = now;
    }
    if (!led_on && now - last_toggle >= OFF_TIME) {
        digitalWrite(ledPin, HIGH);
        led_on = true;
        last_toggle = now;
    }
}

void start_blinking() {
    digitalWrite(ledPin, HIGH);
    led_blinking = true;
    led_on = true;
    last_toggle = millis();
}

void stop_blinking() {
    digitalWrite(ledPin, LOW);
    led_blinking = false;
    led_on = false;
}

You then have to call these functions from loop() as follows:

void loop() {
    // Keep the LED blinking if necessary.
    update_led();
    ...
    if (request.indexOf("/LED=ON") != -1)  {
        start_blinking();
    }
    if (request.indexOf("/LED=OFF") != -1)  {
        stop_blinking();
    }
    ...
}

Edit, in response to jsotola’s comments:

When answering the question, I was initially tempted to make a BlinkingLed class, with the functions as its public interface and the variables private. I resisted the temptation on the grounds that, given the apparent OP’s level of C++ knowledge, that could end up being well over his head. Instead, I used documentation as an informal way of separating the public from the private: you are only supposed to use what is documented, i.e. the functions. You don't “take a shortcut in your program and clear led_blinking directly”.

You could, of course, make the led_blinking variable part of the public interface. That could even be a nice simplification, as you don't need the start/stop function anymore. But then you loose control over the initial phase of the blinking pattern. That may not be an issue, but if you want the LED to turn on as soon as you set led_blinking to true, then you have to do something like

void update_led() {
    uint32_t now = millis();
    if (!led_blinking) {
        digitalWrite(ledPin, LOW);
        led_on = false;
        last_toggle = now - OFF_TIME;  // fix initial phase
        return;
    }
    ...
}

Note that the subtraction can overflow, but that's not as issue because it follows the rules of modular arithmetics.

I actually kind of like this option, with all the logic in a single function. This way the variables led_on and last_toggle can be made static local to the function, which provides true encapsulation without a class. However, from a pedagogic point of view, I think my initial proposal is easier to understand.

Correct way of having loop end when required?

1 Answers1