Simon Code help

I am incredibly frustrated with my first Arduino code. I am trying to build a Simon game, hardware and software, with out looking at the work of others.

I came to the point in the code where I am starting to rip out the hair from my head. Practicly everything works perfect accept the restart mechanism. So what I have so far is the following. Arduino generates a pattern on 50 random numbers and stores it in an array (ledA_) that would light up corresponding LEDS. After it would play back the LEDs and wait for user input and store it in a different array (buttonA*) that would be used to compare user pattern to the generated pattern. I got it to check if the patterns are the same, but it fails in restarting the game. Meaning I want it to generate a new patter to be used to light up corresponding leds and build up them up in displaying and imputing 1 to 1,2 to 1,2,3 and so on._
Here is my code, I have only built in the check mechanism into val3 or button that is connected to pin 3. Someone of you might suggest a different way to write the code, that might be 100% true and better, but I simply want to CRACK THIS SOB and get it working.
The problem is that if I deliberately fail the pattern, it starts to light up random LEDs with out end.
I am begging for someone to help me, please.
Thank you
_
> int val2,val3,val4,val5; //Button Debounce Step 1*_
> int zval2, zval3, zval4, zval5; //Button Debounce Step 2
> int state2,state3,state4,state5; //Button Debounce First state
> int ledA[50]; //LED array used to store rnd #(8,9,10,11) to light up leds in those pins
> int buttonA[50]; //Button array used store button presses that get translated into nubmers (8,9,10,11)
> int z=0; //Game Level
> int y=0; //Loop Level Display
> int t=0; //Loop Level Input
>
> void setup(){
> pinMode(2, INPUT); //.................button
> pinMode(3, INPUT); //.................button
> pinMode(4, INPUT); //.................button
> pinMode(5, INPUT); //.................button
> pinMode(8, OUTPUT); //Input/Output setup
> pinMode(9, OUTPUT); //.................
> pinMode(10, OUTPUT); //.................
> pinMode(11, OUTPUT); //.................
> pinMode(12, OUTPUT); //.................Piezo
> Serial.begin(9600); //Random seed
> randomSeed(analogRead(0)); //Random seed
> state2=digitalRead(2); //Button comparison
> state3=digitalRead(3); //Button comparison
> state4=digitalRead(4); //Button comparison
> state5=digitalRead(5); //Button comparison
> }
>
> void loop(){
>
> while(z<=49){
> ledA[z]=random(8,12); //Generates random numbers as the game goes on.
>
> //The following loop (y<=z)plays back generated pattern of the LEDs up to the current level
*> while(y<=z){ *
*> digitalWrite(ledA[y],HIGH); *
*> delay(500); *
*> digitalWrite(ledA[y],LOW); *
> delay (500);
> y++;
> }
> /*Loop (t<=z) Waits for an appropiet number of buttons to be pressed. Checks that corresponding buttons are pressed.Currently only butt
> on that is connected to pin 3(val3) has the check mechanism, others simply advance the counter with out seeing if corresponding buttons has been pressed*/
> while(t<=z){
> val2=digitalRead(2); //used in debouncing momentary switches
> val3=digitalRead(3);
> val4=digitalRead(4);
> val5=digitalRead(5);
> delay(10);
> zval2=digitalRead(2);
> zval3=digitalRead(3);
> zval4=digitalRead(4);
*> zval5=digitalRead(5); //end in debounce Comparison *
> if(val2==zval2){ //Here is the loop "input code for each button//
> if(val2!=state2){
> if(val2==LOW){ //detection when button is released
> buttonA[t]=8; //storage for a corresponding number into an array
> t++;
> }
> }
> state2=val2;
> } //input code for first button ends here
> if(val3==zval3){
> if(val3!=state3){
> if(val3==LOW){
> buttonA[t]=9;
> if(ledA[t]!=buttonA[t]){ //this is the button that has the check code built in. If it fails the game needs to be restarted
> z=-1; // Z is set to -1 because ones it exits (t<=z) loop the will be a delay(500) and z++ will make it into z=0
> t=0;
> y=0;
> break;
> }
> t++;
> }
> }
> state3=val3;
> }
> if(val4==zval4){
> if(val4!=state4){
> if(val4==LOW){
> buttonA[t]=10;
> t++;
> }
> }
> state4=val4;
> }
> if(val5==zval5){
> if(val5!=state5){
> if(val5==LOW){
> buttonA[t]=11;
> t++;
> }
> }
> state5=val5;
> }
>
> }
> delay(500);
> z++;
> y=0;
> t=0;
> }
>
> }
Filesonic

int ledA[50];                                   //LED array
int buttonA[50];                                //Button array

How many buttons and LEDs are there? An int can hold a value from -32768 to 32767. It's unlikely that you have any negative pin numbers, and it's unlikely that you have 32,700+ pins. The byte type would be more appropriate for these arrays, and would take half the space.

int z=0;                                        //Game Level
int y=0;                                        //Loop Level Display
int t=0;                                        //Loop Level Input

Well, those variable names sure match the comments.

    randomSeed(analogRead(0));

This is NOT a good way to generate a random seed.

    state2=digitalRead(2);            //Button comparison
    state3=digitalRead(3);            //Button comparison
    state4=digitalRead(4);            //Button comparison
    state5=digitalRead(5);            //Button comparison

Why are you reading switch states in setup? Does that comment really aid in understanding what the code is doing?

          Serial.println(z);
          Serial.println(y);
          Serial.println(t);

Three numbers appear in the serial monitor. How the heck do you know which is which?

          Serial.print("z="); Serial.println(z);
          Serial.print("y="); Serial.println(y);
          Serial.print("t="); Serial.println(t);

Takes only marginally longer to type, compile, and execute, and, yet, you know exactly what each number means.

            if(val2==zval2){
                if(val2!=state2){
                    if(val2==LOW){
                        buttonA[t]=8;
                        t++;
                }}
                state2=val2;
            }

Each { and } belongs on it's own line.

            if(val2==zval2){
                if(val2!=state2){
                    if(val2==LOW){
                        buttonA[t]=8;
                        t++;
                }}
                state2=val2;
            }
            if(val3==zval3){
                if(val3!=state3){
                    if(val3==LOW){
                        buttonA[t]=9;
                       if(ledA[t]!=buttonA[t]){
                         z=-1;
                        
                          break;}
                        t++;
                }}
                state3=val3;
            }

Sure seems to me that the action for each switch should be the same. When they are not, as in this case, there should be some comments that explain why the actions are different.

Hey PaulS,

Thank you for the reply and comments.
I have four LEDs and 4 buttons. The reason for having an array is to store a patter of random generated numbers from 8-11 wich will light up LEDs that are connected to those pins.

An array buttonsA is used to store user input. I equate button press of button 2 to be 8, number 8 gets store in an array, later used to compare if the arrrays coresponding parts are the same

Regards to randomseed, this is an example I got from arduino site, it said that this is . good way. Could you explain why this not a good way in your opinion?

In creating a debouncing script, I took an example from ladyada's example where they were read in the setup. The reason I could think of is to know the inisial state?

The action on the switch reading should be the same. The thing is that I only built in the check mechanism into the switch that is connected with pin 3, val3. This is done for testing purposes. This is were the problem is, even though it detects if the wrong switch is pressed it does not restart the game properly. I was hoping this where people would be able to help me out why

The use of arrays is good. The size (int) is not. The data would fit just as easily if the size was byte, and the data would take up half as much room.

Could you explain why this not a good way in your opinion?

The value read from an unconnected analog pin is by no means random. Using it as though it was is not smart. There have been alternatives discussed on the forum. There really does not appear to be a "best" way to get a truly random seed value from a non-random piece of hardware.

The reason I could think of is to know the inisial state?

Unless you plan on pressing random buttons during setup(), the initial state for all switches is unpressed (whatever that means in your code (HIGH or LOW)).

This is were the problem is, even though it detects if the wrong switch is pressed it does not restart the game properly.

            if(val3==zval3){
                if(val3!=state3){
                    if(val3==LOW){
                        buttonA[t]=9;
                       if(ledA[t]!=buttonA[t]){
                         z=-1;
                        
                          break;}
                        t++;
                }}
                state3=val3;
            }

So, if the wrong switch is pressed, and the wrong switch is connected to pin 3, you set z to -1 and call break. Now, break is used to get out of a for or while loop, so the program will skip the rest of the while(t <= z) loop. The poor placement of { and } and random indenting (and complete lack of comments) makes it hard to determine where the program goes when it breaks out of the while loop. But, it appears as though it then reaches the end of loop(), so the loop() function ends, and gets called again.

At that time, z is -1, which is less than 50, so the first while loop starts and ledA[-1] is assigned a value. Oops.

Ones I get home I will create comments on the code and repost it. Not sure how to deal with random indentations but will see what I can do

kclv1988:
Ones I get home I will create comments on the code and repost it. Not sure how to deal with random indentations but will see what I can do

The IDE will reformat your code for you. Control-T or you can get to it through the menus. While you're at it, it would be a lot easier to read if your variables had longer, more meaningful names.

wildbill thank you for the Ctrl-T tip helped a lot. Now I added comments to the code it, in hopes that it will help you guys understand it. If something is unclear please let me know and I will try my best to explain. I have also made a video demonstrating where the code fails.

int val2,val3,val4,val5; //Button Debounce Step 1
int zval2, zval3, zval4, zval5; //Button Debounce Step 2
int state2,state3,state4,state5; //Button Debounce First state
int ledA[50]; //LED array used to store rnd #(8,9,10,11) to light up leds in those pins
int buttonA[50]; //Button array used store button presses that get translated into nubmers (8,9,10,11)
int z=0; //Game Level
int y=0; //Loop Level Display
int t=0; //Loop Level Input

void setup(){
pinMode(2, INPUT); //.................button
pinMode(3, INPUT); //.................button
pinMode(4, INPUT); //.................button
pinMode(5, INPUT); //.................button
pinMode(8, OUTPUT); //Input/Output setup
pinMode(9, OUTPUT); //.................
pinMode(10, OUTPUT); //.................
pinMode(11, OUTPUT); //.................
pinMode(12, OUTPUT); //.................Piezo
Serial.begin(9600); //Random seed
randomSeed(analogRead(0)); //Random seed
state2=digitalRead(2); //Button comparison
state3=digitalRead(3); //Button comparison
state4=digitalRead(4); //Button comparison
state5=digitalRead(5); //Button comparison
}

void loop(){

while(z<=49){
ledA[z]=random(8,12); //Generates random numbers as the game goes on.

//The following loop (y<=z)plays back generated pattern of the LEDs up to the current level
while(y<=z){
digitalWrite(ledA[y],HIGH);
delay(500);
digitalWrite(ledA[y],LOW);
delay (500);
y++;
}
/Loop (t<=z) Waits for an appropiet number of buttons to be pressed. Checks that corresponding buttons are pressed.Currently only butt
on that is connected to pin 3(val3) has the check mechanism, others simply advance the counter with out seeing if corresponding buttons has been pressed
/
while(t<=z){
val2=digitalRead(2); //used in debouncing momentary switches
val3=digitalRead(3);
val4=digitalRead(4);
val5=digitalRead(5);
delay(10);
zval2=digitalRead(2);
zval3=digitalRead(3);
zval4=digitalRead(4);
zval5=digitalRead(5); //end in debounce Comparison
if(val2==zval2){ //Here is the loop "input code for each button//
if(val2!=state2){
if(val2==LOW){ //detection when button is released
buttonA[t]=8; //storage for a corresponding number into an array
t++;
}
}
state2=val2;
} //input code for first button ends here
if(val3==zval3){
if(val3!=state3){
if(val3==LOW){
buttonA[t]=9;
if(ledA[t]!=buttonA[t]){ //this is the button that has the check code built in. If it fails the game needs to be restarted
z=-1; // Z is set to -1 because ones it exits (t<=z) loop the will be a delay(500) and z++ will make it into z=0
t=0;
y=0;
break;
}
t++;
}
}
state3=val3;
}
if(val4==zval4){
if(val4!=state4){
if(val4==LOW){
buttonA[t]=10;
t++;
}
}
state4=val4;
}
if(val5==zval5){
if(val5!=state5){
if(val5==LOW){
buttonA[t]=11;
t++;
}
}
state5=val5;
}

}
delay(500);
z++;
y=0;
t=0;
}

}

Here is the link to the video I made

http://www.filesonic.com/file/2081106254/SAM_0049.MP4

Is there any reason you're not using serial debug prints?

AWOL:
Is there any reason you're not using serial debug prints?

I did I just removed them from the code.

at the very end where it says

delay(500);
z++;
y=0;
t=0;
}
}
right after t=0 I did a print out for z,y,t and all of them come out to be 0. I simply cant figure it out, thats why I came here.

any one??

Any debug output?

right after t=0 I did a print out for z,y,t and all of them come out to be 0. I simply cant figure it out, thats why I came here.

You set z to -1 when the wrong key is pressed, and break out of the while loop. Then, you increment z by 1, to 0, and set y and z to 0.

Then, you can't figure out while all three are 0? I'm stunned.

PaulS:

right after t=0 I did a print out for z,y,t and all of them come out to be 0. I simply cant figure it out, thats why I came here.

You set z to -1 when the wrong key is pressed, and break out of the while loop. Then, you increment z by 1, to 0, and set y and z to 0.

Then, you can't figure out while all three are 0? I'm stunned.

No the z t y needs to to be zeros, it resets the counters. What I cant figure out is how it behaves after the reset. I have made a small video to show you what happens

What I cant figure out is how it behaves after the reset.

Well, then, add more Serial.print()s. That was what AWOL was suggesting. What are the values of y, z, and t at the start of loop()? What are they at the end of the outer while loop?

Add Serial.print() statements in key places. How do you define a key place? That's the tricky part. Start by print those three variables at the start and end of loop. If they consistently match what you expect, then they can be deleted, as too many consume a lot of memory and slow the program down.

Add/remove statements before/after points where program flow changes direction. Of the before and after values are not what you think they should be, either the before value is wrong, or the program changed the value in a way that you didn't expect. In either case, having data is better than not having data, and eventually you will figure out where you are setting values wrong/not setting values correctly, or where your expectations are wrong.

I got it. The problem was that I did not update the state of the button after it detects that the wrong button was pressed. It was looping threw it.

Big thanks for everyones help

I got it.

Feels good, doesn't it?