I need help with a little code entry pad.

Hey there,
im stuck and i could do with some help.
I am making a little pretend security pad (pretend because it wont be used for any security). the set out is nine buttons, all with pull down resistors and then two led's. One red (ledOne) and one green (ledTwo). the idea is that i set a password (pass[]) and then when i button is hit it is entered into (attempt[]). If both arrays are equal to each other then the light turns from red to green. and then after 10 seconds changes back. At the moment it is only a test so the code isnt complete and it only compares attempt[0] to pass[0] (to see if it works). The actual issues are that: when any button is pressed the light changes, and if you leave it alone for too long (a minute or so) the red light doesnt turn off and the green light is only on when you hold the button.

Here's the code, please help:

// Project - 4 Button Code
int ledOne = 13;  // Red LED
int ledTwo = 12;  // Green LED
int x = 0; // Set x to 0
byte pass[] = {1,2}; // Pass Code To Enter
byte attempt[2]; // Entered Code; What people entered
byte buttons[] = {2,3,4,5,6,7,8,9,10}; // Set Buttons 
byte state[9]; // Set digitalRead states array in preperation
int time = 0; // set time measure "millis()"
int y = 0; // Another Variable set to 0

void setup() {
  for (x=0; x<9; x++){
    pinMode(buttons[x], INPUT); // For all the buttons set them to INPUT
  }
  pinMode(ledOne, OUTPUT);//set LEDS TO OUTPUT
  pinMode(ledTwo, OUTPUT);
  digitalWrite(ledOne, HIGH); // Turn Red Light On
}

void loop(){
  for (x=0; x<9; x++){// for each number 0-9
    state[x] = digitalRead(buttons[x]);// Set each digital read from 2-11 in to state[0-9]
  }
  for (x=0; x<9; x++){
    if (state[x] == HIGH){//see if any states are HIGH
      attempt[x] = state[x];//If they are, set Attempt[x] to whichever state it was
      time = millis(); // make time variable equal how long pgrams been running
    }
  }
  if (attempt[0] == pass[0]){// if attempt[0] is equal to pass[0], greenOn()
    greenOn();
  }
  if ((millis() - time) > 10000){//if time between last button press and now is 5 seconds set all code entries to 0
    for (x=0; x<9; x++){
      attempt[x] = 0;
    }
  }
  if (attempt[0] != pass[0]){// if attempt[0] is not pass[0] turn off green return to red
    greenOff();
  }
}

void greenOn(){
  digitalWrite(ledTwo, HIGH);
  digitalWrite(ledOne, LOW);
}

void greenOff(){
  digitalWrite(ledTwo, LOW);
  digitalWrite(ledOne, HIGH);
}

You have nothing in your code that waits for a key press, you just read all the buttons one after the other. Then you treat the resulting array like it was a distinct key press.
The array attempt has a size of 2 but you read it with an index of 0 to 8
You need an old state and a new state array and only look at things when the old state is not equal to the new state and if it is high.

Cheers, i was looking at it since i uploaded and corrected, so it still doesnt do everything i want, but it is better

Ok, nevermind, what i did didnt fix it. you wouldnt mind writing me a small example o fwhat you mean?

The trick is to keep posting code as you make improvements. Start off by trying to detect when any key is pressed and then released. Then move on to what key it is. There is a lot more code you need to write for this project.

Yeah... i see what you mean... Im going to rewrite, is it realistic that i use arrays to contain all the button pin integers?

Yes use an array but use bytes to save space. You need at least a past array to store the previous values in so you can see when they have changed.

OK, i started again, but i have a feeling i've already made a mistake:

int x = 0;
int ledOne = 13;
int ledTwo = 12;
byte buttons[] = {2,3,4,5,6,7,8,9,10};
byte pass[] = {1};
byte attempt[1];
byte state[9];

void setup(){
  pinMode(ledOne, OUTPUT);
  pinMode(ledTwo, OUTPUT);
  for(x=0; x<9; x++){
    pinMode(buttons[x], INPUT);
  }
  digitalWrite(ledOne, HIGH);
}


void loop(){
  for(x=0; x<9; x++){
    state[x] = digitalRead(buttons[x]);
  }
  
}

void Correct(){
  digitalWrite(ledOne, LOW);
  digitalWrite(ledTwo, HIGH);
}

void TimeOut(){
  digitalWrite(ledTwo, LOW);
  digitalWrite(ledOne, HIGH);
}

OK, i started again, but i have a feeling i've already made a mistake

No. So far, so good. The 1 element arrays need work, but I assume you have plans that dictate a larger array in the near future.

So as the last thing in the loop function you need to copy your state array into an oldState array.

can you write out a short example, i think i understand, but it would really help if i could see what you mean,

No you can do it yourself.
A for loop where oldState[index] is made equal to state[ index ]

OK, i will try my best and update the post if i have any new issues.

Just Updating.
I believe im doing it right at the moment, though feedback and corrections will be more then appreciated right now :slight_smile:

int y = 0; // Set y
int x = 0; // Set x
int ledOne = 13; //Set Red LED
int ledTwo = 12; //Set Green LED
byte buttons[] = {2,3,4,5,6,7,8,9,10}; //Set buttons to array
byte pass[] = {2}; // Set password
byte attempt[1]; // Set attempts array
byte state[9]; // Make states array
byte oldstate[9]; // When button is hit, info added here
int time = 0;

void setup(){
  pinMode(ledOne, OUTPUT);
  pinMode(ledTwo, OUTPUT);
  for(x=0; x<9; x++){
    pinMode(buttons[x], INPUT);
  }
  digitalWrite(ledOne, HIGH);
    for(x=0; x<9; x++){
    state[x] = digitalRead(buttons[x]); // Check states of each button
  }
  for(x=0; x<9; x++){
    oldstate[x] = state[x];//Send all data to different array
  }
}


void loop(){
  for(x=0; x<9; x++){
    state[x] = digitalRead(buttons[x]); // Check states of each button
  }
  for(x=0; x<9; x++){
    if(oldstate[x] != state[x]){ //If these new states are different then the old ones
      attempt[0] = buttons[x]; //Set attempt (Just first index for now) to buttons pressed
      time = millis();
    }
  }
  for(x=0; x<1; x++){
    if(attempt[0] == pass[0]){
      Correct();
    }
  }
  if(attempt[0] != pass[0] && (millis() - time) > 5000){
    TimeOut();
  }
}

void Correct(){
  digitalWrite(ledOne, LOW);
  digitalWrite(ledTwo, HIGH);
}

void TimeOut(){
  digitalWrite(ledTwo, LOW);
  digitalWrite(ledOne, HIGH);
}

at the moment its only a test still, so it only checks to see if attemp[0] and pass[0], will change that when finished. Also it works for what it is, except the timeout doesnt work.

No I said as the last thing in the loop not the setup.

I dont understand what you mean, what i wrote i thought worked...

Only because you haven't got round to using it.
The making a copy of state into oldState needs to occours every time round the loop. Not once at the end of start up.

if the oldstate array isnt made until the end of the loop, how am i meant to compare the the state with oldstate before its even made

if the oldstate array isnt made until the end of the loop, how am i meant to compare the the state with oldstate before its even made

The array is "made" when you declare it as a global variable. Is it populated with a bunch of default values, appropriate to the type of the array. Arrays of type int, byte, long, etc. are initialized to 0 (which corresponds to LOW).

You may need a little more work in loop(). Right now, you are setting the value in attempt[0] to the pin number that changed state. This sounds good, until you consider that releasing the switch also generates a state change. So, pressing a switch will affect attempt[0] and releasing the switch will affect attempt[0]. Is this what you want? Or, do you just want to know when switch i has been pressed?

Basically when a button is hit i want the arduino to register it, save it in attempt[]. and if attempt is equal to pass[] then the light changes