Serial.println runs in an infinite loop in void function

[ssmith3603]

Hi, I am working on a project and I am just starting to test my code. I ran my code and my RGB LED was working for a short while but my for-loops were acting weird. I changed a few lines and now my Serial.println at the beginning of my function is running in an infinite loop in the Serial Monitor. My code should follow:

int redLED = 2;
int greenLED = 3;
int blueLED = 4;
int redbutton = 5;
int bluebutton = 6;
int greenbutton = 7;
int buttons[] = {redbutton,bluebutton,greenbutton};
int sensorRed = digitalRead(redbutton);
int sensorBlue = digitalRead(bluebutton);
int sensorGreen = digitalRead(greenbutton);

bool fact = true;
int presses = 0;
int seq[8];
void setup()
{
  Serial.begin (9600);
  
  //setting the LEDs
  pinMode(redLED, OUTPUT);
  pinMode(greenLED, OUTPUT);
  pinMode(blueLED, OUTPUT);

  //setup pinmode for the Simon Game
  pinMode(redbutton, INPUT_PULLUP);
  pinMode(greenbutton, INPUT_PULLUP);
  pinMode(bluebutton, INPUT_PULLUP);
}
void loop()
{
  simonGame();
}
void lose()
{}
void simonGame()
{
  Serial.println("Welcome to Simon!");
  Serial.println("Try to play the sequence back by pressing the switches.");
  bool game = true;
  while(game == true){
    for(int t = 0; t < sizeof(seq)-1; t++){
      seq[t] = random(0,4);
      Serial.println(seq[t]);
    }
    for(int i = 0; i < sizeof(seq)-1; i++){
      if(seq[i] == 1){
        analogWrite(redLED, 255);
        delay(1000);
        analogWrite(redLED, 0);
        delay(1000); 
      }
      else if(seq[i] == 2){
        analogWrite(blueLED, 255);
        delay(1000);
        analogWrite(blueLED, 0);
        delay(1000); 
      }
      else if(seq[i] == 3){
        analogWrite(greenLED, 255);
        delay(1000);
        analogWrite(greenLED, 0);
        delay(1000); 
      }
      else{
        break;
      }}
  
    //wait for player input (via the switches)
    //initialize the count of switches pressed to 0
    int switch_count = 0;
    int picked[8];
    //keep accepting player input until the number of
    //items in the sequence is reached
    for(int p = 0; p < sizeof(seq)-1; p++){
      bool pressed = false;
      //so long as no switch is currently pressed...
      while(pressed == false){
        if(sensorRed == HIGH){
          picked[p] = 1;
          pressed = true;
        }
        else if(sensorBlue == HIGH){
          picked[p] = 2;
          pressed = true;
        }
        else if(sensorGreen == HIGH){
          picked[p] = 3;
          pressed = true;
        }
        else{
          break;
        } }
      if(picked[p] == seq[p]) {
        break;
        game = false;}
      else{
      lose();
      }}}}

Let me know if you have any suggestions. Thank you!

[J-M-L]

@ssmith3603

for(int t = 0; t < sizeof(seq)-1; t++){

You overflow your array. Sizeof gives you the number of bytes allocated, not the number of entries and as your array is not composed of bytes you have 2 (or 4 depending on your arduino) times more bytes than entries.

add this just after your array declaration

const int entryCount = sizeof seq / sizeof seq[0]; // better type to use would be size_t`

And use entryCount in your for loop (don’t use both -1 and strictly inferior at the same time or you won’t get the last entry)

If you overflow an array this is undefined behavior territory and the card may reboot which would cause the constant printing (I have not checked the rest of the code)

[Klaus_K]

A few things I noted about your code.

This does not do what I suspect you think it does. This initializes a variable with a value read from a pin. This happens only once, it does not make the variable magically change when the button does.

int sensorRed = digitalRead(redbutton);

You use delay(). This is the best time to unlearn that. delay() is a waste of processing cycles and stops the rest of your sketch from running.

Have a look at the following example on how to time parts of your code.

File -> Examples -> 02.Digital -> BlinkWithoutDelay

You use while(). Same as delay. Try to make sure your loop runs as often as possible. Your code should not wait for things to happen. Your example would benefit from a state machine. I like to use a function for that with switch-case and a state variable. The state machine handles the "app" at the top level and calls functions for things that need to happen.

You call simonGame from loop but then you have a single very long function with lots of logic inside. Divide your code to make it easier to read.

I do not believe your comments are helpful and some are incorrect. If you believe you need a comment see if you should rewrite the code instead. Comments should be used for something that is not in the code and cannot be written as code.

[ssmith3603]

Thank you for all of your advice! Unfortunately, I was converting this code from python, which I had written at a different time. I got very confused trying to convert the language instead of just starting over. That is why some of the comments are there, I haven't gone back and changed some of them yet. I have used switch-case before! I am a little rusty, but no time like the present to relearn. Again, thank you!

[ssmith3603]

Hi, thank you for the clarification of functions. I edited my code and it is working much better than before! I still have a lot of troubleshooting in other areas but this was very helpful, thank you!