Help with simplifiying code

Projects Programming
1

Hey guys,

currently creating a game where a light will show in a pattern and you have to press the corresponding button in the same pattern, the pattern grows after every round. Im currently debugging and came across an issue and was hoping someone knows a solution.

the issue is when the code gets to the 'while loop', my serial monitor just seems to spit out "while loop", meaning it is disregarding the 'for' loop.

const int lights[4] = {8, 9, 10, 11};
const int buttons[4] = {3, 4, 5, 6};
int sequence[30];
int currentSize = 0;
int roundNumber = 1;

void setup() {
for(int i; i<4; i++){
  pinMode(lights[i], OUTPUT);
  pinMode(buttons[i], INPUT_PULLUP);
}
randomSeed(analogRead(0));
Serial.begin(9600);
}

void loop() {
  sequence[currentSize++] = random(4);
  Serial.println("loop started");
  
  for(int i; i<roundNumber; i++){
    int correctLight = sequence[i] + 8;
    Serial.println("showing Light");
    digitalWrite(correctLight, HIGH);
    delay(1000);
    digitalWrite(correctLight, LOW);
  }
  
  
  for(int i; i<roundNumber; i++){
   int correctButton = sequence[i] + 3;
    Serial.println("creating sequence loop");
    while(true) {
      Serial.println("while loop");
      delay(100);
      for (int i; i<4; i++) {
        Serial.println("for loop");
        if (buttons[i] == HIGH) {
          Serial.println("button press");
          int buttonPressed = buttons[i];
          if (buttonPressed == correctButton){
              roundNumber++;
              Serial.println("correct press");
            break;
          }else{
            endGame();
            delay(1000);
            break;
          }
        }
      }
    }
  }
}

void endGame() {
  roundNumber = 1;
  currentSize = 0;
  Serial.println("game ended");
  for(int i; i<4; i++){
    digitalWrite(buttons[i], HIGH);
  }
  delay(1000);
  for(int i; i<4; i++){
    digitalWrite(buttons[i], LOW);
  }
}

any solutions would be great!!

Thanks,
Alex

2

For nested for loops, there's a whole alphabet to use - no need to re-use i, because you're messing with everyone's mind when you do that, even if you get away with it now and then.
"Which i do they think they're incrementing..."

1 Like
3

Try initializing i, otherwise, it might start with random junk:

      for (int i=0; i<4; i++) {
3 Likes
4

Fixed your repeat variables, and your initialization. Try this:

const int lights[4] = {8, 9, 10, 11};
const int buttons[4] = {3, 4, 5, 6};
int sequence[30];
int currentSize = 0;
int roundNumber = 1;

void setup() {
for(int i; i<4; i++){
  pinMode(lights[i], OUTPUT);
  pinMode(buttons[i], INPUT_PULLUP);
}
randomSeed(analogRead(0));
Serial.begin(9600);
}

void loop() {
  sequence[currentSize++] = random(4);
  Serial.println("loop started");
  
  for(int i=0; i<roundNumber; i++){
    int correctLight = sequence[i] + 8;
    Serial.println("showing Light");
    digitalWrite(correctLight, HIGH);
    delay(1000);
    digitalWrite(correctLight, LOW);
  }
  
  for(int j=0; j<roundNumber; j++){
   int correctButton = sequence[j] + 3;
    Serial.println("creating sequence loop");
    while(true) {
      Serial.println("while loop");
      delay(100);
      for (int k=0; k<4; k++) {
        Serial.println("for loop");
        if (buttons[k] == HIGH) {
          Serial.println("button press");
          int buttonPressed = buttons[k];
          if (buttonPressed == correctButton){
              roundNumber++;
              Serial.println("correct press");
            break;
          }else{
            endGame();
            delay(1000);
            break;
          }
        }
      }
    }
  }
}

void endGame() {
  roundNumber = 1;
  currentSize = 0;
  Serial.println("game ended");
  for(int i; i<4; i++){
    digitalWrite(buttons[i], HIGH);
  }
  delay(1000);
  for(int i; i<4; i++){
    digitalWrite(buttons[i], LOW);
  }
}
2 Likes
5

where do you read the button?
where is the value of buttons [i] set?

1 Like
6

Thanks alot,

this solved the problem, still a lot of debugging i need to do, but i should be able to work it all out.

-Alex

7

Missed one:

8

I believe he left that one, as it happens in the setup, so it only happens once. Meaning it will automatically be initialised as 0 on set up and won’t be repeated meaning the variable doesn’t need to be changed back to a 0.

-Alex

1 Like
9

Nevertheless, fix it. Why make a reader wonder?

Except

is incorrect. That it is more likely to be zero may be true, but nothing guarantees it.

a

2 Likes
10

There can be a couple routines called before setup:

11

sorry im very new to arduino and c++ programming, can you explain what this is meant to help with???

Cheers,
-Alex

12

Sorry, I was pointing out that routines called before setup() have the potential to dirty the stack and make the uninitialized variables non-zero.

1 Like
13

Here's a test I just did. Many times it printed 0, but then...

i is 22595


...Program finished with exit code 0
Press ENTER to exit console.

Here' the code.

# include <stdio.h>

int main()
{
    for (int i; ; ) {
        printf("i is %d\n", i);
        break;
    }

    return 0;
}

and @DaveX was right to point out that whatever you do get depends on what might have come before.

I don't think anything in the startup code explicitly zeros out non-used memory, so you are indeed grabbing whatever the preceding function calls left on the stack.

Curiously I was unable to get the compiler to issue a warning. IMO this deserves one.

a7

1 Like
14

Hey guys, im relatively new to coding and only just started learning Arduino. I made this simple code that shows a sequence of lights, then makes the user click buttons in the same order of the lights showing.

I have finally got the code to work, but i feel like it is really chunky and kinda hard to read, do you have any suggestions how to make the code simpler and easier to read.

const int lights[4] = {8, 9, 10, 11};
const int buttons[4] = {3, 4, 5, 6};
int sequence[30];
int currentSize = 0;
int roundNumber = 0;

void setup() {
for(int i=0; i<4; i++){
  pinMode(lights[i], OUTPUT);
  pinMode(buttons[i], INPUT);
}
pinMode(7, OUTPUT);
randomSeed(analogRead(0));
Serial.begin(9600);
}

void loop() {
  roundNumber++;
  sequence[currentSize++] = random(4);
  Serial.println("loop started");
  
  for(int i=0; i<roundNumber; i++){
    int correctLight = sequence[i] + 8;
    Serial.print("Light: ");
    Serial.println(correctLight - 7);
    digitalWrite(correctLight, HIGH);
    delay(1000);
    digitalWrite(correctLight, LOW);
    delay(100);
  }
  
  for(int j=0; j<roundNumber; j++){
   int correctButton = sequence[j] + 3;
   int buttonPressed = 0;
    Serial.print("Button: ");
    Serial.println(correctButton - 2);
    Serial.print("Round Number: ");
    Serial.println(roundNumber);
    while(true) {
      delay(100);
      for (int k=0; k<4; k++) {
        if (digitalRead(buttons[k]) == HIGH) {
          Serial.println("button press");
          buttonPressed = buttons[k];
          while(true){
            if (digitalRead(buttonPressed) == LOW) {
              if (buttonPressed == 0){
                continue;
              }if (buttonPressed == correctButton){
                Serial.println("correct press");
                goto endLoop;
              }if (buttonPressed != correctButton) {
                endGame();
                goto endLoop;
              }
            }
          }
        }
      }
      
      
    }
    endLoop:
    delay(1); //here as a placement, otherrwise syntax error occurs
  }
}


void endGame() {
  roundNumber = 0;
  currentSize = 0;
  Serial.println("game ended");
  for(int i=0; i<4; i++){
    digitalWrite(lights[i], HIGH);
  }
  delay(2000);
  for(int k=0; k<4; k++){
    digitalWrite(lights[k], LOW);
  }
  delay(2000);
}

All feedback is welcome, trying to improve my coding.

Thanks alot,
Alex

15

First, pleas ask a moderator to attach this to the thread you were working.

It helps helpers, avoids repeating stuff and will save you lotsa questions.

Plus we see more of you, and knowing what you are up to and how you got into the mess you are in now would be helpful.

So… this mess.

First, watch

then lose the goto. There is zero need for you to rely on the goto statement, and working to eliminate it will force you to think about the algorithm and will very probably result, along with de-nesting, in a better sketch.

You seem to know enough to be dangerous, an exciting time, but learning and reading other ppls' code is essential, more so at this point in your travels perhaps than at any other.

As I may have said in the other thread (see? make me go look?) creativity in coding is def a thing, but so are the very well established practices and patterns that you may not yet know enough about.

Lastly, if you aren't, make use of the IDE Autoformat tool.

a7

16

The topics have been merged

@exotikdoggo You can change topic title if you want to

17

thanks alot for the detailed response, where do you recommend i find others codes to read?

Alex

18

Thanks @UKHeliBob .

@exotikdoggo I must curse and complain as you have peovoked an old interest in Simon and I have been wasting spending time playing the game. :expressionless:

But back to the sketch.

I will show here in pseudocode, a way for programmers to talk about code without getting into any pesky detaills, how your sketch might be structured.

start with a sequence one unit long

loop forever

SHOW:
  loop over the sequence, lighting LEDs and making tones
  until you've done the entire thing

PLAY:
  loop
    get user input and
        see if it is correct

  until the user finiches or makes an error

PUNISH:
  if the user messed up do <whatever> and reset/start a new game

REWARD: if the user finched the current length sequence do <whatever>

   and GROW: if she did the entire sequence, add to the sequence

HTH

Right now my version is busted... something I did too late or too something is making it show very odd behaviour, an error that is not quite anything I can even see how it is doing. So for playing, I am using an older version.

Later: will I go into the storage area and hope that my Simon is somewhere it should be, the batteries haven't leaked if I left them in and the thing lights up like it's 1979?

a7

1 Like
19

look this over

  • uses functions to separate parts of code
  • event and state driven making it easier to read the code
const byte PinLed  [4] = {8, 9, 10, 11};
const byte PinBut [4] = {3, 4, 5, 6};
enum { Off = LOW, On = HIGH };

const int Nbut = sizeof(PinBut);
byte butState [Nbut];

const int Nseq = 5;
int seq [Nseq];
int idx;
int roundNum = 1;

const unsigned long MsecPeriod = 4000;
      unsigned long msec0;
      unsigned long msec;

char s [90];

// -----------------------------------------------------------------------------
// return button index if pressed
int
getButton ()
{
    for (int i=0; i<Nbut; i++)  {
        byte but =  digitalRead (PinBut[i]);
        if (butState [i] != but)  {
            butState [i]  = but;
            delay (20);             // debounce

            if (LOW == but)  {
                sprintf (s, " getButton: %d", i);
                Serial.println (s);
                return i;
            }
        }
    }
    return -1;
}

// -----------------------------------------------------------------------------
void youLose ()
{
    for (int i=0; i<Nbut; i++)
        digitalWrite (PinLed[i], On);
    delay (200);

    for (int k=0; k<Nbut; k++)
        digitalWrite (PinLed[k], Off);
    delay (1000);
}

// ---------------------------------------------------------
void youWon ()
{
    for (int i=0; i<Nbut; i++)  {
        digitalWrite (PinLed[i], On);
        delay (200);
    }
    for (int k=0; k<Nbut; k++)  {
        digitalWrite (PinLed[k], Off);
        delay (200);
    }
}

// ---------------------------------------------------------
void
dispLedSeq ()
{
    Serial.println ("dispLed:");
    for (int n = 0; n < roundNum; n++)  {
        Serial.println (seq [n]);
        digitalWrite (PinLed [seq [n]], On);
        delay (500);
        digitalWrite (PinLed [seq [n]], Off);
        delay (500);
    }
    Serial.println (" dispLed:");
}

// ---------------------------------------------------------
enum { S_Disp, S_Chk, S_Lose, S_BeatGame };
int state = S_Disp;

void loop ()
{
    msec = millis ();

    if (S_Disp == state)  {
        dispLedSeq ();

        state = S_Chk;
        msec0 = millis ();               // start timer
        idx   = 0;
    }

    else if (S_Chk == state)  {
        if (msec - msec0 >= MsecPeriod)  {
            msec0 += MsecPeriod;
            Serial.println (" timeout - you lose");
            state  = S_Lose;
        }

        int but = getButton ();
        if (0 <= but)  {
            sprintf (s, "loop: idx %2d, led %d, but %d", idx, seq [idx], but);
            Serial.println (s);

            if (seq [idx++] != but)  {
                Serial.println (" wrong button - you lose");
                state = S_Lose;
            }

            else if (roundNum == idx)  {
                if (++roundNum < idx)  {
                    Serial.println (" you beat the game");
                    state = S_BeatGame;
                }
                else  {
                    youWon ();
                    Serial.println (" you won");
                    state = S_Disp;
                }
            }
        }
    }

    else if (S_Lose == state)  {
        youLose ();
    }

    else if (S_BeatGame == state)  {
        youWon ();
    }
}


// -----------------------------------------------------------------------------
void setup () {
    Serial.begin (9600);

    for (int i=0; i<Nbut; i++)  {
        pinMode      (PinLed[i], OUTPUT);
        digitalWrite (PinLed[i], Off);

        pinMode (PinBut[i], INPUT);
        butState [i] = digitalRead (PinBut[i]);
    }

    randomSeed (analogRead (0));

    for (int n = 0; n < Nseq; n++)
        seq [n] = random (Nbut-1);

    Serial.println ("Start");
    state = S_Disp;
}
21

@exotikdoggo, you may well be at a point where having solutions handed yuo to "look over" or "consider" would be worth the time.

If you want to see the magic rabbit and read, play and learn, I time spent on this project would be well spent:

as it seems to match your goals for the functionality, is along the lines I have suggested and looks like you were heading (I haven't looked closely at your code and uses plain coding plainly.

@gcjr's code is dense and hard to read, even if you know what you are looking for (or at) only more so if you don't. Let's call it an acquired taste, one which I have failed to develop after seeing solutions like this for years. There may be ppl who are in the perfect place to gets stretched by it

So maybe one day, but not necessarily.

a7