Curious Error with goto: [SOLVED]

Hi! I've been trying to create some software that runs a rocket control panel thingy. Essentially, I want to exit all my loops and go straight to the beginning of the void loop() loop, but I can't seem to find a command to do this other than goto: I know that using goto is discouraged, please don't mention that much but I need to use it unless I can find another way. Here is the error I've been getting:

LCD_Rocket_Countdown.ino: In function 'void onDisconnect()':
LCD_Rocket_Countdown:89: error: label 'top' used but not defined

Even though I properly declared the goto!

void loop() {
  top:
void onDisconnect() {
  lcd.clear();
  lcd.print("ERROR: Ignitor");
  lcd.setCursor(0,1);
  lcd.print("was unplugged!  ");
  delay(5000);
  goto top;
}

Here is the pastebin link for the full code:

You can't jump out of one function and into another.
The label is only valid in the function in which it is declared.

Pete

There's nothing wrong with goto for exiting a nested loop. That's actually the one place where its use is considered typical. However, you appear to be trying to use goto to break out of nested functions which is not possible. The label has to be within the same function. Instead, change your function to return a value, and have your calling function break out of its loop depending on that value.

Normal convention for return codes is 0 == success; -int == error; +int == good status (like bytes read etc.). Or, you can return 0 == false, non-zero = true. Whatever makes sense in your application.

P.S., there is a long jump function in C that acts like a non-local goto. But that's just dirty.

First of all, I don't think you can use a goto from within one function to jump outside that function, they are not at the same scope level. (I'm rusty, but I think you can use setjump() and longjump() to do that.) You could use a flag variable (e.g., leaveHere) and set it after the return from onDisconnect() and then test it so the rest of the code is skipped. I'm not at my machine, but something like the following gives you an idea of one approach. (Ugly, but it may work.)

void loop() {
  int leaveHere = 0;
  top:
  contState = digitalRead(contPin);
  if(contState == LOW) {
    lcd.clear();
    lcd.print("Ready to Launch!");
    lcd.setCursor(0,1);
    lcd.print("Press button...");
    delay(100); //Quick anti-flicker delay
    countdown = countVal;
    buttonState = digitalRead(buttonPin);
    if(buttonState == LOW) {
      lcd.clear();
      lcd.print("Launching in:");
      for(countdown > 0; countdown--;) {
        contState = digitalRead(contPin);
        lcd.setCursor(0,1);
        lcd.print(countdown + 1);
        lcd.print(' ');
        lcd.print("second(s)  ");
        delay(1000);
        if(contState == HIGH) {
          onDisconnect();
          leaveHere = 1;
          break;
        }
      }
      if (leaveHere == 0) {
        lcd.clear();
        lcd.print("Igniting Engine");
        digitalWrite(RelayPin, HIGH);
        delay(1000*delayTime);
        digitalWrite(RelayPin, LOW);
        contState = digitalRead(contPin);
        if(contState == LOW) {
          lcd.clear();
          lcd.print("Warning: Rocket");
          lcd.setCursor(0,1);
          lcd.print("not ignited!   ");
          delay(2500);
          lcd.clear();
        }else {
          lcd.clear();
          lcd.print("WHOOOOSH!       ");
          delay(2500);
        } 
      }   // end if (leaveHere...  
    } else {
      if (leaveHere == 0) {
        lcd.clear();
        lcd.print("ERROR: Ignitor");
        lcd.setCursor(0,1);
        lcd.print("not plugged in!");
        delay(1000);  
      }
    }
  }
}

AVR-LIBC has longjmp() -- avr-libc: setjmp.h File Reference

I am 1,000,000% confident you don't need it here.

Hmm…it looks like loop() is one big function…

UPDATE: I tried it out, calling loop() brought me to the top! Yeah! And I’ll look at that dirty long_jmp thing…might be useful…
Thanks for the help! :slight_smile: :slight_smile:

As was pointed out… can’t be done inter-function.

But, the good news is that you don’t need a goto. Ever. (What, NEVER?). Well, hardly ever!

You should really have posted your entire code. I happened to be on a sacrificial machine when I saw your question, so I followed your link to grab the code. Anyway, here’s the modified code. I haven’t tested it, because I am not about to wire up the circuit, but it should work. Do a search, in the code, on “lar3” (my nicknumber), to find the changed lines.

#include <LiquidCrystal.h>
int RelayPin = 6; //Relay!
int delayTime = 5; //Seconds to hold the relay open
int buttonState = 0; //The state of the button (1, 0)
int buttonPin = 7; //Pin of the button. Make sure to connect the other end to GND, not 5V
int countdown = 0; //Don't edit this, edit countVal instead!
int countVal = 15; //Time (in seconds) you have to run.
int contPin = 8; //The Pin for the continutity sensor
int contState = 0; //State of continutity
LiquidCrystal lcd(12, 11, 5, 4, 3, 2); //LCD Config Pins
void setup() 
{
  //LCD Columns,Rows
  lcd.begin(16, 2);
  lcd.setCursor(0, 0);
  lcd.print("Welcome to");
  lcd.setCursor(0, 1);
  lcd.print("RocketLaunch 4.4");
  delay(1750);
  pinMode(RelayPin, OUTPUT); //Set the relay as an output
  pinMode(buttonPin, INPUT); //Self-Explanitory 
  pinMode(contPin, INPUT);
  digitalWrite(buttonPin, HIGH); //Pull-Up Resistor
  digitalWrite(contPin, HIGH); //Pull-Up Resistor
  lcd.clear();
}

void loop() {
  bool disconn = false;                    // lar3 - removed filthy goto. added a flag to signify 'disconnected'

  contState = digitalRead(contPin);
  if(contState == LOW) {
    lcd.clear();
    lcd.print("Ready to Launch!");
    lcd.setCursor(0,1);
    lcd.print("Press button...");
    delay(100); //Quick anti-flicker delay
    countdown = countVal;
    buttonState = digitalRead(buttonPin);
    if(buttonState == LOW) {
        lcd.clear();
      lcd.print("Launching in:");
      for(countdown > 0; countdown--;) {
        contState = digitalRead(contPin);
        lcd.setCursor(0,1);
        lcd.print(countdown + 1);
        lcd.print(' ');
        lcd.print("second(s)  ");
        delay(1000);
        if(contState == HIGH) {
          onDisconnect();               // lar3 - still calls onDisconnect
          disconn = true;                 // lar3 -  but then sets a flag to tell us we did
        }
      }
      if ( ! disconn ) {                    // checks disconn flag, and executes following if block is not disconn
        lcd.clear();
        lcd.print("Igniting Engine");
        digitalWrite(RelayPin, HIGH);
        delay(1000*delayTime);
        digitalWrite(RelayPin, LOW);
        contState = digitalRead(contPin);
        if(contState == LOW) {
          lcd.clear();
          lcd.print("Warning: Rocket");
          lcd.setCursor(0,1);
          lcd.print("not ignited!   ");
          delay(2500);
          lcd.clear();
        }
        else {
          lcd.clear();
          lcd.print("WHOOOOSH!       ");
          delay(2500);
        }       
      }                                           //lar3 - end od if (! disconn) block,  falls out of   if(contState == LOW) block
    }
  }  
  else {                                    // lar3 - this does not execute because it's the else for  if(contState == LOW) 
    lcd.clear();
    lcd.print("ERROR: Ignitor");
    lcd.setCursor(0,1);
    lcd.print("not plugged in!");
    delay(1000);  
  }
}

void onDisconnect() {
  lcd.clear();
  lcd.print("ERROR: Ignitor");
  lcd.setCursor(0,1);
  lcd.print("was unplugged!  ");
  delay(5000);                                     //  lar3 - removed scummy goto someone left here to rot.
}

OMG, no. No no no. Neither.

If you call loop() from within a function called by loop(), you're into what's called "recursion". Like those Russian nesting dolls. You can only do that so many times before you run out of stack space, not to mention it's horrible practice.

No one should ever under any circumstances use longjmp. It's not "useful," its use indicates poor design. I only brought it up to stave off pedantic "yes, you actually can jump out of functions ... with longjmp!" replies.

Longjmp is one of those things you read about in the library reference and go, "huh -- didn't know that existed." And then you never speak of it again. It's like having the knowledge that you can ward off infection after a jellyfish sting by peeing on your wound. It doesn't mean you drink up a Sobe and go hunting for jellyfish. It means, that's good incentive not to get stung by jellyfish. :wink:

lar3ry:
As was pointed out… can’t be done inter-function.

But, the good news is that you don’t need a goto. Ever. (What, NEVER?). Well, hardly ever!

You should really have posted your entire code. I happened to be on a sacrificial machine when I saw your question, so I followed your link to grab the code. Anyway, here’s the modified code. I haven’t tested it, because I am not about to wire up the circuit, but it should work. Do a search, in the code, on “lar3” (my nicknumber), to find the changed lines.

#include <LiquidCrystal.h>

int RelayPin = 6; //Relay!
int delayTime = 5; //Seconds to hold the relay open
int buttonState = 0; //The state of the button (1, 0)
int buttonPin = 7; //Pin of the button. Make sure to connect the other end to GND, not 5V
int countdown = 0; //Don’t edit this, edit countVal instead!
int countVal = 15; //Time (in seconds) you have to run.
int contPin = 8; //The Pin for the continutity sensor
int contState = 0; //State of continutity
LiquidCrystal lcd(12, 11, 5, 4, 3, 2); //LCD Config Pins
void setup()
{
 //LCD Columns,Rows
 lcd.begin(16, 2);
 lcd.setCursor(0, 0);
 lcd.print(“Welcome to”);
 lcd.setCursor(0, 1);
 lcd.print(“RocketLaunch 4.4”);
 delay(1750);
 pinMode(RelayPin, OUTPUT); //Set the relay as an output
 pinMode(buttonPin, INPUT); //Self-Explanitory
 pinMode(contPin, INPUT);
 digitalWrite(buttonPin, HIGH); //Pull-Up Resistor
 digitalWrite(contPin, HIGH); //Pull-Up Resistor
 lcd.clear();
}

void loop() {
 bool disconn = false;                    // lar3 - removed filthy goto. added a flag to signify ‘disconnected’

contState = digitalRead(contPin);
 if(contState == LOW) {
   lcd.clear();
   lcd.print(“Ready to Launch!”);
   lcd.setCursor(0,1);
   lcd.print(“Press button…”);
   delay(100); //Quick anti-flicker delay
   countdown = countVal;
   buttonState = digitalRead(buttonPin);
   if(buttonState == LOW) {
       lcd.clear();
     lcd.print(“Launching in:”);
     for(countdown > 0; countdown–:wink: {
       contState = digitalRead(contPin);
       lcd.setCursor(0,1);
       lcd.print(countdown + 1);
       lcd.print(’ ');
       lcd.print("second(s)  ");
       delay(1000);
       if(contState == HIGH) {
         onDisconnect();               // lar3 - still calls onDisconnect
         disconn = true;                 // lar3 -  but then sets a flag to tell us we did
       }
     }
     if ( ! disconn ) {                    // checks disconn flag, and executes following if block is not disconn
       lcd.clear();
       lcd.print(“Igniting Engine”);
       digitalWrite(RelayPin, HIGH);
       delay(1000*delayTime);
       digitalWrite(RelayPin, LOW);
       contState = digitalRead(contPin);
       if(contState == LOW) {
         lcd.clear();
         lcd.print(“Warning: Rocket”);
         lcd.setCursor(0,1);
         lcd.print("not ignited!   ");
         delay(2500);
         lcd.clear();
       }
       else {
         lcd.clear();
         lcd.print("WHOOOOSH!       ");
         delay(2500);
       }      
     }                                           //lar3 - end od if (! disconn) block,  falls out of   if(contState == LOW) block
   }
 }  
 else {                                    // lar3 - this does not execute because it’s the else for  if(contState == LOW)
   lcd.clear();
   lcd.print(“ERROR: Ignitor”);
   lcd.setCursor(0,1);
   lcd.print(“not plugged in!”);
   delay(1000);  
 }
}

void onDisconnect() {
 lcd.clear();
 lcd.print(“ERROR: Ignitor”);
 lcd.setCursor(0,1);
 lcd.print("was unplugged!  ");
 delay(5000);                                     //  lar3 - removed useful goto someone left here to be used.
}

Thanks, but gotos have there uses, I just used it wrong. Answer me this: Whats so bad about gotos?
P.S loop() is a function and if you read my post, I did mention I wanted to go to the start of the loop(). I just called, loop() and lo and behold! It worked. I think that what I call the ‘time capsule’ can be annoying…Just sayin’. Though there is longjmp(), but I’d say that’s, well, obsolete and dilapidated. (Can a function be dilapidated?)

SirNickity:
OMG, no. No no no. Neither.

If you call loop() from within a function called by loop(), you're into what's called "recursion". Like those Russian nesting dolls. You can only do that so many times before you run out of stack space, not to mention it's horrible practice.

No one should ever under any circumstances use longjmp. It's not "useful," its use indicates poor design. I only brought it up to stave off pedantic "yes, you actually can jump out of functions ... with longjmp!" replies.

Longjmp is one of those things you read about in the library reference and go, "huh -- didn't know that existed." And then you never speak of it again. It's like having the knowledge that you can ward off infection after a jellyfish sting by peeing on your wound. It doesn't mean you drink up a Sobe and go hunting for jellyfish. It means, that's good incentive not to get stung by jellyfish. :wink:

And the loop() from inside the code isn't an old BASIC thing like the hideous, evil:

10
20 goto 10

since it's protected by the fact that it only occurs when a person trips over the wire. It's not like a button press or something...plus loop() isn't doing much 99% of the time. And loop() isn't called by loop() its called by onDisconnect(). I get what your saying about the russian dolls, this was just an odd circumstance. And I never use 'time capsules
(a.k.a those extra boolean things) since there worse than goto! (really, confusion? talk about confusion)

waterlubber:
And the loop() from inside the code isn't an old BASIC thing like the hideous, evil:

10

20 goto 10

That's not recursion, that's just changing the pointer for the next line to be executed -- in this case, in an infinite loop. What you had going on was this:

void loop() {
    ...
    onDisconnect() {
        ...
        void loop() {
            ...
            onDisconnect() ....

Calling loop() in your code doesn't "restart loop", it starts a new loop() within onDisconnect(), which is still within the first loop(). The program has to track the parent (calling) function's location so that it can continue executing where it left off, so each time you do this, you take away a little bit more memory in order to track that stuff. Eventually, you will run out. On a micro with 2K RAM, some of which is used by other stuff, it doesn't take long.

The severity of this problem depends on whether your loop() function ever ends, or if you have a while() or for() loop within that keeps it going indefinitely. If you let it return (to Arduino's built in main(), which just calls loop() again -- after the previous loop() exits!), then you'll eventually walk back down that recursive stack and arrive at your original loop() again. It's still an awful horrible practice though -- one that will brand you forever as One That Codes In A Most Awful, Horrible Manner. You don't want that. You'll be on a mailing list that sends bi-hourly updates of other awful, horrible things coders do. You'll never be the same after that. Ask anyone that uses Java, or VB.

waterlubber:
since it's protected by the fact that it only occurs when a person trips over the wire. It's not like a button press or something...plus loop() isn't doing much 99% of the time.

Recursion: (n) See: recursion.

Now, recursion isn't inherently evil. It's quite useful, in fact, for doing things like scanning directories for a particular filename. When you find a subdirectory, you call the same function with the starting point set at the subdir, which then may call itself again with subdirs of that subdir, and so on. The thing you have to remember, though, is that you need to guarantee there's a finite point at which the recursion stops. In a filesystem, it will end when there are no more subdirectories. (This is why you would want to avoid following symlinks / shortcuts though, which could end up pointing back to a parent directory and causing the recursion to be infinite -- or at least until the stack is exhausted.)

waterlubber:
And loop() isn't called by loop() its called by onDisconnect().

Yes, loop() is called by onDisconnect(), which is called by loop(). So it's recursion once-removed. Still recursion.

waterlubber:
And I never use 'time capsules (a.k.a those extra boolean things) since there worse than goto! (really, confusion? talk about confusion)

... uh, what? :~

waterlubber:
Answer me this: Whats so bad about gotos?

Nothing in particular, actually. It's just bad form. Goto is mostly a relic of older, less sophisticated languages that lacked the program flow capabilities that better languages (like C) have. It's still there because it does have unique uses, like breaking out of nested loops. It's not inherently evil to use goto, it's just one of those things that, when you find yourself using it, you should question if there isn't maybe a better way...? For error-handling within loops (when something breaks, bail out, throw an error, clean up, and exit) it's sometimes the only way to achieve the desired result without bending the code into submission for no other reason than to avoid using a goto statement.

waterlubber:
if you read my post, I did mention I wanted to go to the start of the loop(). I just called, loop() and lo and behold! It worked.

Not really. As others have pointed out you now have a new loop. You can probably get away with doing this maybe 1000 times, maybe not.

Answer me this: Whats so bad about gotos?

Now you can go right ahead and use goto if you want to. We can't stop you. But I promise you'll be back in a couple of weeks with some sort of obscure bug or crash.

In your case your problem is easily solved. Your code, omitting irrelevant parts is:

void loop() {
  top:

...

          if(contState == HIGH) {
            onDisconnect();
          }
...
}
void onDisconnect() {
...
  goto top;
}

All you have to do is this:

void loop() {

...

          if(contState == HIGH) {
            onDisconnect();
            return;
          }
...
}
void onDisconnect() {
...
}

Problem solved. After onDisconnect you return from loop() which means it immediately restarts, from the top. No goto. No calling loop from within onDisconnect. Easy. Try to put the word "goto" out of your head and think of ways of structuring the code to simplify it.

waterlubber, I have to tell you, it's a brave man who will use infinite recursion in a program that will actually set off an incendiary device if something goes wrong. And you can be sure that something WILL go wrong when you do it. The problem lies in having new local variables clobber other variables, local or global, because they get instantiated anew every time you rerun function code that hasn't properly terminated. This doesn't sound so bad, but things can go wrong 67.5 nanoseconds at a time, and there is no way to predict what variables will get clobbered, in what order, or how long it will take for something IMPORTANT to go wrong.

You REALLY don't want to be the one that leans over to disarm your rocket when that happens.

And by the way, booleans are not evil. They serve a very good purpose, acting as flags for future actions.

If you REALLY must use some sort of quick exit without using booleans (no idea why you would want to), you can always use Nick's suggestion to use a return. It's not the right thing to do, but at least you'll be able to look your grandchildren in the eyes and not have to fib when they ask you if you were a good programmer.

lar3ry:
but things can go wrong 67.5 nanoseconds at a time,

62.5 nanoseconds, so it's worse than that. :wink:

Drat! And I know I shouldn't trust hurried mental arithmetic. :wink:

This whole thing reminds me of the guy that wanted a flying mass of metal with spinning razor blades er.... quadcopter to be able to follow someone with a cell phone.

I have participated in my share of rocket launches over the years and been "range safety officer" while doing so.
(During the Cheap Access to Space competition. We didn't win anything, but we sure made some cool rockets!)

I do not know what rockets we are talking about, but the rules I enforced were:

The igniters are short circuited until the moment the rocket is in the launcher, everything is checked and readied and you have to break the short circuit in order to be able to hook up the igniters to the firing cable. And that happens after everybody else has left the launcher, and the RSO says OK to do it.

The firing cable is kept short circuited, except to check continuity and to hook it up to the firing box at the latest possible moment.

The the safety key is hanging around the neck of the RSO, except when used to check the firing box and to fire the rocket.

The launching system must be kept as simple as possible, and the only switch you can really trust, is physical separation.