Trying to improve 'goto' solution

Hi all. I’m trying to find a solution to a flow issue (hehe) I’m having with a program I’ve written. In this section of code I am checking the states of two pushbutton switches (LOW reading is button depressed, HIGH is released; I’m using 10k pullup resistors) while flashing an LED 7 times. The pushbuttons are checked by a function then it returns to the flashing sequence. It does this quite frequently. At the end of 7 flashes the first button to be released gets a “victory” lightshow, then the program loops back to the start.

Where I’m having issues is if either button is released before the LED has flashed 7 times the code executes a check to determine which one was released first. If “player1” released it goes to player2 victory and vise versa. Where I’m not sure what to do is where line 32 is.

If I victory lightshow is executed from the pbCheck() function it returns to the flashing loop post-game and gets stuck in a loop where it just flashes the player 2 victory lightshow.

I would like it to go from the lightshow directly out of the if statements and return to the start of the loop() function.

I also have a goto statement, which I didn’t want to do but absolutely couldn’t figure out how to achieve the same result without it. All input is appreciated. I will post the full code here as it’s a pretty short program. I also apologize for the tight form factor… I still tend to rush through the lines for fear of forgetting what I was hoping to achieve :grinning:

  const int greenP1 = 9;
  const int greenP2 = 11;
  const int redLed = 10;
  const int player1 = 2;
  const int player2 = 13;

void setup() {
}

void loop() {
  
  pinMode(greenP1, OUTPUT);
  pinMode(greenP2, OUTPUT);
  pinMode(redLed, OUTPUT);
  pinMode(player1, INPUT);
  pinMode(player2, INPUT);

  digitalWrite(greenP1, LOW);
  digitalWrite(greenP2, LOW);
  analogWrite(redLed, 127);
  
  while (digitalRead(player1) == HIGH || digitalRead(player2) == HIGH){
    delay(50);
  }
    
  if (digitalRead(player1) == LOW && digitalRead(player2) == LOW){
     for(int i = 1; i <= 7; i++ ){
       for(int a = 1; a <= 5; a++){
        digitalWrite(redLed, HIGH);                //the start of the flashing led
          delay(50);
          pbCheck();
       }
       for(int b = 1; b <= 5; b++){  
        digitalWrite(redLed, LOW);
          delay(50);
          pbCheck();
       }     
     }
  }
      while(digitalRead(player1) == LOW && (digitalRead(player2) == LOW)){
        delay(30);
      }
        if (digitalRead(player1) == HIGH){      // Check here to improve... currently the game randomly selects a winner, make it wait for a signal (use a while loop?)
          player1Victory();
        {goto cheeseIt;}
        }
        if (digitalRead(player2) == HIGH){      // put a check here for state, game always checks for high
          player2Victory();
       
        }
     cheeseIt:;
} 


void pbCheck() {
  if(digitalRead(player1) == HIGH || digitalRead(player2) == HIGH){
    if(digitalRead(player1) == HIGH){
      delay(30);
       if(digitalRead(player1) == HIGH){
       player2Victory();
       {goto scram;}
      }
    }
    if(digitalRead(player2) == HIGH){
      delay(30);
       if(digitalRead(player2) == HIGH){
       player1Victory();
       }
    }
  } scram:;
}

void player1Victory() {

const int j = 62;
const int k = 250;
  
  for(int x = 1; x <= 8; x++){
    analogWrite(greenP2, 25);
    delay(j);
    analogWrite(greenP2, 0);
    delay(j);

    analogWrite(redLed, 70);
    delay(j);
    analogWrite(redLed, 0);
    delay(j);

    analogWrite(greenP1, 255);
    delay(j);
    analogWrite(greenP1, 0);
    delay(j);
  }
  for(int y = 1; y <= 8; y++){ 
    analogWrite(greenP1, 255);
    delay(k);
    analogWrite(greenP1, 0);
    delay(k);
  }
}

void player2Victory() {

const int j = 62;
const int k = 250;
  
  for(int x = 1; x <= 8; x++){
    analogWrite(greenP1, 25);
    delay(j);
    analogWrite(greenP1, 0);
    delay(j);

    analogWrite(redLed, 70);
    delay(j);
    analogWrite(redLed, 0);
    delay(j);

    analogWrite(greenP2, 255);
    delay(j);
    analogWrite(greenP2, 0);
    delay(j);
  }
  for(int y = 1; y <= 8; y++){ 
    analogWrite(greenP2, 255);
    delay(k);
    analogWrite(greenP2, 0);
    delay(k);
  }
}

Try replacing "goto" with "return".

Your " setup" looks a bit empty, and play1victory is virtually the same as player2victory.

A dose of auto-format wouldn't go amiss either.

I would suggest that you completely restructure your program. The code in loop() could be as simple as

void loop() {
   readButtons();
   updateGameState();
   flashLeds();
   flashVictory();
}

When you try to read inputs in different places you very quickly end up with spaghetti code that can't be understood or maintained.

Have a look at Planning and Implementing a Program. Also note that it uses millis() rather than delay() to manage timing without blocking.

...R

No goto needed.

const int greenP1 = 9;
const int greenP2 = 11;
const int redLed = 10;
const int player1 = 2;
const int player2 = 13;

void setup() {
  pinMode(greenP1, OUTPUT);
  pinMode(greenP2, OUTPUT);
  pinMode(redLed, OUTPUT);
  pinMode(player1, INPUT_PULLUP);  // Uses the internal pullup so you don't need an external one
  pinMode(player2, INPUT_PULLUP);  // Uses the internal pullup so you don't need an external one

  digitalWrite(greenP1, LOW);
  digitalWrite(greenP2, LOW);
  analogWrite(redLed, 127);
  delay(1000);
}

void loop() {
  int winner = 0;
  boolean fault = false;
  unsigned long startTime = millis();
  int flashes = 0;

  // Wait for both buttons to be released, signaling ready
  while (digitalRead(player1) == LOW || digitalRead(player2) == LOW) {
    delay(50);
  }

  // Let the players know that the timer has started
  digitalWrite(redLed, LOW);
  delay(1000);
  digitalWrite(redLed, HIGH);
  startTime = millis();

  // Flash the light while checking for faults
  while (winner == 0 && flashes < 14) {
    // See if Player 1 has jumped the gun
    if (digitalRead(player1) == LOW) {
      fault = true;
      winner = 2;
    }
    
    // See if Player 1 has jumped the gun
    if (digitalRead(player2) == LOW) {
      fault = true;
      winner = 1;
    }

    // Change the light every 1/4 second
    if (millis() - startTime >= 250) {
      startTime += 250;
      digitalWrite(redLed, !digitalRead(redLed));
      flashes++;
    }
  }

  // Wait for someone to press their button
  while (winner == 0) {
    if (digitalRead(player1) == LOW) {
      winner = 1;
    }
    if (digitalRead(player2) == LOW) {
      winner = 2;
    }
  }

  // A winner has been chosen, possibly by fault
  playerVictory(winner, fault);

  // Wait for both buttons to be pressed signaling players are ready for another try
  while (digitalRead(player1) == HIGH || digitalRead(player2) == HIGH) {
    delay(50);
  }
}

// You may want a different display if someone jumped the gun
void playerVictory(int player, boolean fault) {
  int winnerPin, looserPin;

  const int j = 62;
  const int k = 250;

  if (player == 1) {
    winnerPin = greenP1;
    looserPin = greenP2;
  } else {
    winnerPin = greenP2;
    looserPin = greenP1;
  }

  for (int x = 1; x <= 8; x++) {
    analogWrite(looserPin, 25);
    delay(j);
    analogWrite(looserPin, 0);
    delay(j);

    analogWrite(redLed, 70);
    delay(j);
    analogWrite(redLed, 0);
    delay(j);

    analogWrite(winnerPin, 255);
    delay(j);
    analogWrite(winnerPin, 0);
    delay(j);
  }
  for (int y = 1; y <= 8; y++) {
    analogWrite(winnerPin, 255);
    delay(k);
    analogWrite(winnerPin, 0);
    delay(k);
  }
}

No goto needed

Is there ever? :smiley:

Groove:
Is there ever? :smiley:

Well, yes, I was taught that there is anyway.

If you go down the path of making your code overly convoluted and difficult to read/follow, just to avoid using goto, that's a mistake. If it makes the structure of the code simpler and easier to follow, by handing a rare "exception" code path with a goto, then that's OK.

Having said that, I can't remember ever using goto on Arduino, and only once in recent memory have I used one in some VB I wrote at work.

Paul

Thanks for the help everyone. I realize that my code structure is pretty horrendous but I’m not sure I could get away with the huge simplification of the main loop as was suggested (though I’m sure it can be done with some more knowledge and time). Here is some more info on how the program is meant to run.

How they play:
• Each player waits for the red light to come on and stay on.
• Each player, in her/his own time pushes and holds down the button closest to
the player.
• Once both buttons are held down the red LED will flash at ½ second intervals
(250 ms off, 250 ms on). The light will flash 7 times (7 ON and 7 OFF).
• After the 7 flashes, the red light comes on and stays on. Each player will
attempt to be the first to release his/her button. The first to do so wins the
game.
• The green LED closest to the winning button will come on to indicate the
winner. 
2
Notes
1. If a game is initiated and either player releases her/his button early the other
player automatically wins. The winner’s green LED will come on.
2. After a game is finished the controller waits for 5 seconds and then resets the
game (bringing the red LED on) so that a new game can begin.

The assignment is due tomorrow and my current thoughts are to create a variable that changes from 0 to 1 when either victory function is executed, then have an if statement after the pbCheck() that returns out of the if-nest to restart the game. Is there a command to read a variable? I’ll post the area of code I’m talking about:

if (digitalRead(player1) == LOW && digitalRead(player2) == LOW) {
    for (int i = 1; i <= 7; i++ ) {
      for (int a = 1; a <= 5; a++) {
        digitalWrite(redLed, HIGH);                //the start of the flashing led
        delay(50);
        pbCheck();

       //after every pushbutton check this section checks to see if the gameOver variable has changed
       //and will exit out to the start of the void loop()
 
        if (gameOver == 1) {
          return;
        }
      }
      for (int b = 1; b <= 5; b++) {
        digitalWrite(redLed, LOW);
        delay(50);
        pbCheck();
        if (gameOver == 1) {
          return;
        }

…getting the gameOver variable from here:

int player1Victory() {

  const int j = 62;
  const int k = 250;

  for (int x = 1; x <= 8; x++) {
    analogWrite(greenP2, 25);
    delay(j);
    analogWrite(greenP2, 0);
    delay(j);

    analogWrite(redLed, 70);
    delay(j);
    analogWrite(redLed, 0);
    delay(j);

    analogWrite(greenP1, 255);
    delay(j);
    analogWrite(greenP1, 0);
    delay(j);
  }
  for (int y = 1; y <= 8; y++) {
    analogWrite(greenP1, 255);
    delay(k);
    analogWrite(greenP1, 0);
    delay(k);
  }
  return (gameOver = 1);
}
return (gameOver = 1);

You can return 1 and set a variable somewhere else by the return value. Or you can set gameOver to one and return gameOver. But this line as written doesn't make a lot of sense. It compiles, and might even do what you want. But it is confusing and should be changed.

Groove:
Is there ever? :smiley:

Virtually never.

Beginners do not need goto.

Virtually all code can be rewritten to not need it, and the result will be less convoluted.