How to avoid goto statements and labels?

Using Arduino Programming Questions
1

This is one of my first attempts to write a program (which I plan to implement eventually on a tiny85).

I eventually got it to work, but there are a number of goto statements and labels in it. Apparently these are not such a good idea. I have tried to re-write the program using functions, but after going round and round in circles I end up with at least two goto statements. Besides, none of them work.

Can someone point me in the right direction? I don’t want to get into bad habits.

The function is very simple. There is a relay and a spring-loaded switch. If the switch is held closed for less than a time holdT nothing should happen. If it is held for longer than holdT the relay should toggle “on”, and remain in that condition provided the switch is released within a time waitT. If the switch is then closed (and released) again, the relay should immediately revert to the “off” condition.
Here is a listing of the program that works. I also attach a kind of flow-chart.

int stalkSwitch = 10;      //= 1 (Pin 6/8 of tiny85)
int relay = 9;             //= 0 (Pin 5/8 of tiny85)
int bleep = 11;
int waitT = 400; // wait time
int holdT = 700; // hold time
int bleepT = 40; // bleep time
long int timerStart;
long int timer;
//
void setup() {
  pinMode (stalkSwitch, INPUT);
  pinMode (relay, OUTPUT);
  pinMode (bleep, OUTPUT);
}
void loop () {
  digitalWrite (relay, LOW);
  digitalWrite (bleep, LOW);
  endflash:
  while (digitalRead (stalkSwitch) == LOW);
  timerStart = millis ();
  repeat:
  if (digitalRead (stalkSwitch) == LOW) goto endflash;
  else {
    timer = millis() - timerStart;
    if (timer < holdT) goto repeat;
    else goto toggle;
  }
  toggle:
  digitalWrite (relay, HIGH);
  digitalWrite (bleep, HIGH);
  delay (bleepT);
  digitalWrite (bleep, LOW);
  delay (waitT - bleepT);
  while (digitalRead (stalkSwitch) == LOW);
}

flowchart.pdf (24.1 KB)

2

The first thing to understand is that loop() is called over and over. Writing code to circumvent that is never a good idea.

The next thing to understand is that comments are useful. There are none in your code to explain what you are trying to do.

The third is that goto is almost never required. I've been programming in C for 30 years, and have never needed to use goto.

3

Instead of a flowchart you should draw a state diagram. From the state diagram it's fairly easy to write the code. From the description it looks like you have a few states of interest:

0 Idle: Relay is open and switch is open
if switch is closed, start timer and go to state 1

1 Switching on:
if switch is open, go back to state 0
if timer reaches holdT, turn on the relay and go to state 2

2 Waiting for off:
if switch is closed, turn off relay and go to state 3

3: Off:
if switch is open, go to state 0

4

To expand on johnwasser's suggestion, your code should look something like this:

enum State { s_Idle, s_SwitchingOn, s_On, s_Off };

static State currentState = s_Idle;

void loop()
{
  switch (currentState)
  {
   case s_Idle:
     if (digitalRead (stalkSwitch) == HIGH)
     {
       timerStart = millis ();
       currentState = s_SwitchingOn;
     }
     break;

   case s_SwitchingOn:
     ... // I leave you to write the rest
  }
}
5

Fun stuff ...

typedef unsigned long       millis_t;

const uint8_t   pinRELAY            =  9;   //= 0 (Pin 5/8 of tiny85)
const uint8_t   pinBUTTON           = 10;   //= 1 (Pin 6/8 of tiny85)
const uint8_t   pinBLEEPER          = 11;

const uint8_t   RELAY_OPEN          = LOW;
const uint8_t   RELAY_CLOSE         = HIGH;

const uint8_t   BUTTON_NOT_RRESSED  = LOW;
const uint8_t   BUTTON_PRESSED      = HIGH;

const uint8_t   BLEEP_OFF           = LOW;
const uint8_t   BLEEP_ON            = HIGH;

const millis_t  dmsWAIT             = 400UL;
const millis_t  dmsHOLD             = 700UL;
const millis_t  dmsBLEEP            = 40UL;

void loop()
{
    if ( BUTTON_PRESSED == digitalRead(pinBUTTON) )
    {
        millis_t    dms = millis();

        do
        {
            if ( BUTTON_NOT_RRESSED == digitalRead(pinBUTTON) )
            {
                return;
            }

        } while ( (millis() - dms) < dmsHOLD );
        
        digitalWrite(pinRELAY, RELAY_CLOSE);
    
        digitalWrite(pinBLEEPER, BLEEP_ON);
        delay(dmsBLEEP);
        digitalWrite(pinBLEEPER, BLEEP_OFF);
    
        delay(dmsWAIT - dmsBLEEP);

        while ( BUTTON_RRESSED == digitalRead(pinBUTTON) )
        {   }
    }
    
    digitalWrite(pinBLEEPER, BLEEP_OFF);
    digitalWrite(pinRELAY,   RELAY_OPEN);
}

void setup()
{
    pinMode(pinBLEEPER, OUTPUT);
    pinMode(pinRELAY,   OUTPUT);
    pinMode(pinBUTTON,  INPUT);

    digitalWrite(pinBLEEPER, BLEEP_OFF);
    digitalWrite(pinRELAY,   RELAY_OPEN);
}
6

Actually implementing a state machine is the one situation in which I'd say using goto is OK, but
only if the state machine was being machine-translated into C code(!) Using a state variable and
switch statement is better structure for humans.

7

johnwasser:
Instead of a flowchart ...

dc42:
To expand on johnwasser's suggestion...

Thank you and all the others who have responded. This is exactly the kind of guidance I needed. Your break-down, johnwasser, of the system into the various states is spot-on.

Back to the drawing board...

8

I have now re-written the program almost from scratch (changing some of the variable names) and following the suggestions of johnwasser and dc42.

I have come up with something that compiles, but it doesn't work as it is supposed to. Code below. I have used plenty of comments this time, so hopefully it will be clear what I am trying to achieve.

I have used 4 states. As far as I can tell state_One does work, but there seems to be something wrong with state_Three. The program doesn't wait for "holdT" but immediately turns the relay on when the switch is closed. When the relay has latched "on", it correctly switches off again when the switch is closed a second time.

Two questions: (1) Is the "switch" structure I have used basically in order, and (2) can anyone tell me why the program doesn't work?

/*
FUNCTION OF SYSTEM:

There is a relay which is normally de-energized (off), and a
spring-loaded switch which is normally open (low).

If the switch is closed for less than a time "holdT" nothing happens.
If it is closed for at least "holdT" the relay is latched in the "on"
condition and a bleeper sounds for a short time to indicate that the
switch can be released.  If the switch is released the relay remains
on.

If the switch is again closed, the relay is switched off immediately
and when the switch is thereafter released the system
returns to the initial condition.
*/
int switchPIN = 10;           //= 1 (Pin 6/8 of tiny85)
int relayPIN = 9;             //= 0 (Pin 5/8 of tiny85)
int bleepPIN = 11;            //= 2 (Pin 7/8 of tiny85)
int holdT = 700; // hold time
int bleepT = 40; // bleep time
long int timerStart;
long int timer;
//
void setup() {
  pinMode (switchPIN, INPUT); //Switch normally open (LOW)
  pinMode (relayPIN, OUTPUT); //Drive pin for relay
  pinMode (bleepPIN, OUTPUT); //Drive pin for audible bleeper
}
enum SystemState {state_One, state_Two, state_Three, state_Four};
static SystemState currentState = state_One; // ? must read up about this
//
void loop () {
  switch (currentState) {
    //----------
    case state_One: //Relay off. Switch released (open).
    while (digitalRead (switchPIN) == LOW); //Churning. Waiting for switch to close.
    timerStart = millis (); //Start timer when switch closes,
    currentState = state_Two; //..and go to state_Two.
    break;
    //----------  
    case state_Two: //Relay still off. Read switch condition.
    if (digitalRead (switchPIN) == LOW) currentState = state_One; //Return to state_One if switch open,
    currentState = state_Three; //..otherwise go to state_Three.
    break;
    //----------  
    case state_Three: //Relay still off. Switch now closed. Read timer.
    timer = millis() - timerStart;
    if (timer < holdT) currentState = state_Two; //Return to state_Two if timer not yet timed out,
    digitalWrite (relayPIN, HIGH); //..otherwise switch relay on,
    digitalWrite (bleepPIN, HIGH); //..and give a short bleep to indicate that switch can be released.
    delay (bleepT);
    digitalWrite (bleepPIN, LOW);
    while (digitalRead (switchPIN) == HIGH); //Churning. Waiting for switch to be released.
    currentState = state_Four;  //Go to state_Four when switch released.
    break;
    //----------  
    case state_Four: //Relay latched on. Switch now open.
    while (digitalRead (switchPIN) == LOW);  //Churning. Waiting for switch to close.
    delay (10); //Wait after switch first closes, to allow switch to settle in closed condition,
    digitalWrite (relayPIN, LOW); //..and switch relay off.
    while (digitalRead (switchPIN) == HIGH); //Churning. Waiting for switch to be released.
    currentState = state_One; //Go to state_One when switch released.
    break;
  }
}
9 
    if (digitalRead (switchPIN) == LOW) currentState = state_One; //Return to state_One if switch open,
    currentState = state_Three; //..otherwise go to state_Three.

The comment says "otherwise...". The code says "unconditionally...".

10

Your "states" should probably have more meaningful names. Meaningful names make the code easier to understand and debug.

You probably shouldn't have those churn loops. If you want to wait for something to happen, have a separate state to wait in an a state to go to when the event you are waiting for occurs.

To make your controls more responsive you should do the same for delay(). Get rid of calls to delay() by having a state where you check for a timer.

11

PaulS:
The comment says "otherwise...". The code says "unconditionally...".

You’re right. I misunderstood the C++ tutorial. I have fixed the error (there was a similar one in state_Three). I also had to add another delay(10) somewhere to deal with a switch-bounce problem. The program now works as intended.

12

johnwasser:
Your "states" should probably have more meaningful names. Meaningful names make the code easier to understand and debug.

You probably shouldn't have those churn loops. If you want to wait for something to happen, have a separate state to wait in an a state to go to when the event you are waiting for occurs.

To make your controls more responsive you should do the same for delay(). Get rid of calls to delay() by having a state where you check for a timer.

Thanks. I’ll bear that in mind. I've now got the program to work.

13

lloyddean:
Fun stuff ...

This looks like a very sophisticated bit of code you have written. Unfortunately, I have to learn to crawl before trying to walk. Let alone run. Maybe one day I’ll be able to understand (and even use) what you have produced.