Having more issues...

system · March 24, 2012, 3:56pm

Having even more issues, for a short while i thought i had cracked it. all my code makes sense and im pretty sure its right.
But obviously not. Its still in test mode so the code isnt absolutely final with the if statements near the bottom, but thats not the issue.
The issue is when the right button on my code entry is pressed, the red light (ledOne) doesnt turn off. Anybody know whats wrong?

// Project Awesome. Code Entry With Four Buttons
#define BTNCNT 9
#define ATTCNT 2
int a = 0; //set a
int y = 0; // Set y
int x = 0; // Set x
int ledOne = 13; //Set Red LED
int ledTwo = 12; //Set Green LED
byte buttons[BTNCNT] = {2,3,4,5,6,7,8,9,10}; //Set buttons to array
byte pass[ATTCNT] = {2,3}; // Set password
byte attempt[ATTCNT]; // Set attempts array
byte state[ATTCNT]; // Make states array
byte oldstate[ATTCNT]; // When button is hit, info added here
int time = 0;

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


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

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

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

udoklein · March 24, 2012, 6:40pm

The first comment does not fit the BTNCNT
BTNCNT should be a constant that is derived from sizeof(buttons) and should get a meaningful name
ATTCNT should be a constant that is derived from sizeof(pass) and should get a meaningful name
you use int to declare LED pins
you call the LEDs ledOne and ledTwo, that is the variable names give no clue about the meaning of the leds
Correct() and TimeOut() are basically the inverses of each others
6a) they should have better names like Correct() and Incorrect()
6b) they should be combined into one function e.g. void IndicateInputState(boolean correct)
setup does not initialize ledTwo
you have global variables a,x,y with unclear semantics, variables (and especially global variables) need reasonable names
you comment on the global variables that you set them, this is redundant repetition of the code
the loop

  for(x=0; x<2; x++){
    if(attempt[0] == pass[0]){
      Correct();
    }
  }

does not evaluate x inside the loop
11) you use the global variable x as a loop index
12) the comment in the line

    if(oldstate[x] != state[x]){ //If these new states are different then the old ones

redundantly repeats the code
13) the condition

    if(oldstate[x] != state[x]){ //If these new states are different then the old ones
      if(state[x] == HIGH)

would be better understood if it was written as

    if(oldstate[x] == LOW && state[x] == HIGH) {

The code

  if ((millis() - time) > 10000){
    for(x=0;x<ATTCNT;x++){
      attempt[x] = 0;
      y=0;
    }
  }
  if(y > 3){
    for(x=0;x<ATTCNT;x++){
      attempt[x] = 0;
    }
    y=0;
  }

does the same thing in two different if statements. However it implements it in two different ways.
why not

  if(y > 3 || (millis() - time) > 10000){
    for(x=0;x<ATTCNT;x++){
      attempt[x] = 0;
    }
    y=0;
  }

you do not initialize the state and attempts array, however for the attempts array you comment "set attempts array"
you do not state what this code is supposed to do, it is almost impossible to infer from code that fails what the specification might be

system · March 24, 2012, 6:47pm

I don't care for your corrections. They're not relevant to my question. Id prefer if you had looked at it and tried to see what was wrong, and if you couldn't to move on and actually help someone else rather then tearing apart a person (who has been coding for just over a weeks) code.

Thanks for the reply none the less, of anyone else can help please do post.

udoklein · March 24, 2012, 7:08pm

Well, they are relevant to your question.

You still did not describe what this code is supposed todo. How do you expect a fix if you just tell you think your code is good but it does not work. You say

The issue is when the right button on my code entry is pressed

But you give absolutey no hints what "pressing the right button" actually is.

Step 0: Calm down
Step 1: Accept the fact that nobody here but you knows what this code is supposed to do. Describe what the code is expected to do and how it fails to achieve this.
Step 2: Fix the obvious stuff first, ignoring good practice is not helpful if you want to fix your code

retrolefty · March 24, 2012, 7:15pm

and if you couldn't to move on and actually help someone else rather then tearing apart a person (who has been coding for just over a weeks) code.

I think you are confusing criticizing your code for criticizing you personally. They are not the same thing, but if you remain defencive about your code you would probably not benefit with any advice given? Peer review and constructive criticism is one of the most effective learning methods there is for programming. But you must be mature enough to understand that process for it to be effective.

Lefty

udoklein · March 24, 2012, 7:26pm

Thanks Lefty for pointing this one out.

system · March 24, 2012, 7:58pm

I do realise he wasn't criticizing me,
Ill explain what the code is meant to do:
Basically the idea is its a security pad (sort of).
When the buttons required are pressed, Correct() is run.
I realise also that some of the code is over complicated, but that isnt relevant at the moment, that i can fix and refine later, aswell as the repeated commands, which are there for a reason in most cases.
The only issue im hsving is trying to work out why ledOne does not turn off when the correct button, in this case button 2 is pressed.

// Project Awesome. Code Entry With Four Buttons
#define BTNCNT 9
#define ATTCNT 2
int a = 0; //set a
int y = 0; // Set y
int x = 0; // Set x
int ledOne = 13; //Set Red LED
int ledTwo = 12; //Set Green LED
byte buttons[BTNCNT] = {2,3,4,5,6,7,8,9,10}; //Set buttons to array
byte pass[ATTCNT] = {2,3}; // Set password
byte attempt[ATTCNT]; // Set attempts array
byte state[ATTCNT]; // Make states array
byte oldstate[ATTCNT]; // Added to at end of loop
int time = 0;

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


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

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

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

udoklein · March 24, 2012, 8:29pm

It is not "overly complicated". It is wrong. Unless you fix the obvious stuff it is pretty pointless to fix the suspected functional defect.

You did not provide the schematic. You set the button pins to input without pullups. Maybe you have neither pullup nor pulldown resistors so the pins are probably floating. Floating pins always behave erratic and unpredictable. You also give no hint how "pushing the button" actually will change the level of the pin.

With other words: you are still not giving all required details.

system · March 24, 2012, 8:34pm

I think Udo Klein has a great point but I would bet that a major pair of your program which has already been pointed out is that you aren't check the array correctly

for(x=0; x<2; x++){
    if(attempt[0] == pass[0]){
      Correct();
    }
  }

Should Be this

for(x=0; x<2; x++){
    if(attempt[x] == pass[x]){
      Correct();
    }
  }

*Edit spelled Udo Klein wrong (sorry about the typo)

system · March 24, 2012, 8:36pm

OK, the buttons are all connected with pull down resistors from pin 2-10, pin 12 & 13 are the leds. All is excpected at the moment is that when a button, stated in pass[] is pressed the correct() function runs, and then after 5 seconds it runs TimeOut(). and everything is back to where it started, im pretty sure all the wiring is correct as an older version, which did something a bit different but woth the same wiring worked fine.

Here is a slightly updated piece of code, slightly..:

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

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


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

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

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

system · March 24, 2012, 8:38pm

and yeah, ive changed

for(x=0; x<2; x++){
if(attempt[0] == pass[0]){
Correct();
}
}

to just the if statement for now, as that isnt the issue

system · March 24, 2012, 8:49pm

Time to add some debugging prints then. Add

Serial.begin(57600);

to setup(), and add

  Serial.print("attempt[0] <");
  Serial.print(attempt[0]);
  Serial.println(">");
  Serial.print("pass[0] <");
  Serial.print(pass[0]);
  Serial.println(">");

just before

  if(attempt[0] == pass[0]){

then check the serial monitor to satisfy yourself that it's doing what you think it should.

system · March 24, 2012, 8:58pm

erm... i havent used the serial monitor before so i dont know how to make it understandible

udoklein · March 24, 2012, 9:10pm

I found a major issue with your code. And if you would have followed my first set of "irrelevant proposals" this would have made this one obvious.

#define BTNCNT 9
#define ATTCNT 1

...

byte state[ATTCNT]; // Make states array

...

  for(x=0; x<BTNCNT; x++){
    state[x] = digitalRead(buttons[x]); // Check states of each button
  }

Your are writing out of bounds.

system · March 24, 2012, 9:12pm

what do you mean out of bounds?

system · March 24, 2012, 9:22pm

X is bigger then 1

your state array is declared only to 1 so

state[0] and state[1] are good

any thing over that are "out of bounds"

udoklein · March 24, 2012, 9:23pm

state[1] is already out of bounds

system · March 24, 2012, 9:49pm

well this is the code at the moment, and i cant see that issue

// Project Awesome. Code Entry With Four Buttons
#define BTNCNT 9
#define ATTCNT 1
int a = 0; //set a
int y = 0; // Set y
int x = 0; // Set x
int ledOne = 13; //Set Red LED
int ledTwo = 12; //Set Green LED
byte buttons[BTNCNT] = {2,3,4,5,6,7,8,9,10}; //Set buttons to array
byte pass[ATTCNT] = {3}; // Set password
byte attempt[ATTCNT]; // Set attempts array
byte state[ATTCNT]; // Make states array
byte oldstate[ATTCNT]; // When button is hit, info added here
int time = 0;

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


void loop(){
  for(x=0; x<BTNCNT; x++){
    state[x] = digitalRead(buttons[x]); // Check states of each button
  }
  for(x=0; x<BTNCNT; x++){
    if(oldstate[x] != state[x]){ //If these new states are different then the old ones
      if(state[x] == HIGH)
      {
        attempt[y] = buttons[x]; //Set attempt[y]
        y++; // add 1 to y
        time = millis(); // set time to millis()
      }
    }
  }
  if(attempt[0] == pass[0]){ // attempt equals pass run: Correct()
    Correct();
  }
  if(attempt[0] != pass[0]){ // if attempt doesnt equal pass run timeout
    TimeOut();
  }
  if ((millis() - time) > 5000 || y > 3 ){
    for(x=0;x<ATTCNT;x++){
      attempt[x] = 0;
    }
    y=0;
  }
  for(x=0;x<BTNCNT;x++){
    oldstate[x] = state[x];
  }
}

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

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

system · March 24, 2012, 9:52pm

well this is the code at the moment, and i cant see that issue

Look closer

nickgammon · March 24, 2012, 9:52pm

#define ATTCNT 1
...
byte attempt[ATTCNT]; // Set attempts array

You have an array of size 1. Pretty useless in itself and you are writing to positions other than position 0.

Topic		Replies	Views
I need help with a little code entry pad. Programming Questions	42	9625	May 5, 2021
coding issue that I cant fix Programming Questions	10	1120	May 5, 2021
Need Guide from expert Project Guidance	5	484	May 5, 2021
Project Help Project Guidance	6	2140	May 5, 2021
Completly stumped on why this will not work Programming Questions	15	4140	May 5, 2021

Having more issues...

Related topics