(Solved) Problem with digitalRead()

Guys

I am currently building a ‘Simon Says’ (SS), with 4 levels of difficulty, and 16 round in each, for my grandaughter. Developing on a Uno, but will be ported to a Nano when complete.
The code for that works well, just needs some cleaning up.
Being me, I decided that it needed more. So I am looking to adapt it to have up to 4 players.
To make it easier on my head, I decided to write the select game/#players sketch, as a stand alone till I was happy with it. Needless to say, I am NOT happy.
I have removed almost all of it, but still I have the same issue.
As soon as the code starts to run, nothing being touched:-
This line

      if (digitalRead(buttonpin[i] == LOW))

just seems to be ignored. Doing a

Serial.println(digitalRead(buttonpin[i]));

both immediately prior and after tells me that buttonpin is at logic 1, but this line sees it as logic 0.
What very basic point have I overlooked?
```
*const byte buttonpin = {2, 4, 6, 8};
const byte leds = {3, 5, 7, 9};

byte buttonstate = 1;
byte lastbuttonstate = 1;
byte button_count_Flag = 0;
byte game = 0;
byte game_level = 0;
byte buttonpinnumber = {1, 2, 3, 4};
byte buttonnumber = 1;
byte previousbuttonnumber = 1;

unsigned long Test_Millis = 0;

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

pinMode(buttonpin[0], INPUT_PULLUP);
  pinMode(buttonpin[1], INPUT_PULLUP);
  pinMode(buttonpin[2], INPUT_PULLUP);
  pinMode(buttonpin[3], INPUT_PULLUP);
  pinMode(leds[0], OUTPUT);
  pinMode(leds[1], OUTPUT);
  pinMode(leds[2], OUTPUT);
  pinMode(leds[3], OUTPUT);
  Test_Millis = millis();
}

void loop()
{
  if (millis() - Test_Millis <= 5000ul)
  {

for (byte i = 0; i < 4; i++)
    {
/*    Serial.print("buttonnumber = ");
      Serial.println(buttonnumber);
      Serial.print("previousbuttonnumber = ");
      Serial.println(previousbuttonnumber);
      Serial.print("digitalRead(buttonpin[i]) = ");
      Serial.println(digitalRead(buttonpin[i]));
/
      if (digitalRead(buttonpin[i] == LOW))
      {
/
        Serial.print("digitalRead(buttonpin[i]) = ");
          Serial.println(digitalRead(buttonpin[i]));
          Serial.print("previousbuttonnumber = ");
          Serial.println(previousbuttonnumber);
*/
        if (buttonpinnumber[i] != previousbuttonnumber)
        {
          Serial.println(“Wrong button”);
          while (1);
        }
        buttonnumber = buttonpinnumber[i];
        previousbuttonnumber = buttonpinnumber[i];

buttonstate = digitalRead(buttonpin[i]);
      }
    }
  }
}*
```
The test bed is the one I’m using for the SS, and to prove the wiring was good, I loaded and ran the SS sketch.
TIA
Fof
test cct.pdf (9.37 KB)

IamFof:
This line

if (digitalRead(buttonpin[i] == LOW))

just seems to be ignored.

Of course. Try this:

if (digitalRead(buttonpin[i]) == LOW)

Durrr!!! >:(

I've been through the code more times than I care to think.
Couldn't see the tree for the woods. :slight_smile:

Brilliant. Thank you.

Fof

byte buttonpinnumber[] = {1, 2, 3, 4};
Serial.begin(9600);

On the UNO and the Nano pin 0 and 1 are used by the serial interface. If you use Serial in your sketch then you must not use pins 0 and 1 for anything.

byte game_level = 0;
byte buttonpinnumber[] = {1, 2, 3, 4};
byte buttonnumber = 1;
byte previousbuttonnumber = 1;

Also, help yourself by using clear to read and consistent variable names.

=>

byte gameLevel = 0;
byte buttonPins[] = {1, 2, 3, 4};
byte currentButtonNumber = 1;
byte previousButtonNumber = 1;

Delta_G
Not a problem. The array you quoted is just used to convert the actual pin #s, 2,4,6,8, to button numbers, 1,2,3,4.
Septillion
I agree that my variable naming needs attention. I just pop-in semi-suitable names as I go and amend on the fly, as needed. Once things are running, I then go through and clean things up and tidy up names.
Thanks guys for all your help. Karma to all.

Fof

Once things are running, I then go through and clean things up and tidy up names.

That is surely not the way to do it. Sensible, relevant variable and function names make for easier debugging and following the program flow. You need this before the program works as it should, not afterwards

IamFof:
Septillion
I agree that my variable naming needs attention. I just pop-in semi-suitable names as I go and amend on the fly, as needed. Once things are running, I then go through and clean things up and tidy up names.

That’s the wrong way around ::slight_smile: The compiler can’t care less about names. They are for YOU (and now us) to easily see what is happening and make it easier to debug and spot your errors. It’s like having trouble walking but only wanting to use the walker if you went to the shop and back and broke a hip in the meantime…

IamFof:
Delta_G
Not a problem. The array you quoted is just used to convert the actual pin #s, 2,4,6,8, to button numbers, 1,2,3,4.

You have a array, you can index over that array, the microcontroller knows how to increment by one, where on earth do you need the second array for…

if (buttonpinnumber[i] != previousbuttonnumber)
//is simply the same as:
if ((i + 1) != previousbuttonnumber)
//or even simply 
if (i != previousbuttonnumber)
//if you start counting from 0 like a normal programmer ;)

PS You have arrays, use them!

  for(byte i = 0; i < sizeof(ButtonPins); i+){
    pinMode(ButtonPins[i], INPUT_PULLUP);
    pinMode(LedPins[i], OUTPUT);
  }

Assuming same number of buttons and leds and fixing variable names.

@Septillion

I stand chastised (but unrepentent :slight_smile: ).
I do, accept your comments, but at 74, old habits die hard.

Re: Arrays. This the first time I have had to get my teeth into them. I'm getting there, slowly. :slight_smile:

Your comment //if you start counting from 0 like a normal programmer ;). I do try to do this most of the time, but in this particular situation, where I use const byte buttonpin[] = {2, 4, 6, 8}; and byte buttonpinnumber[] = {1, 2, 3, 4}; The first are the pins the switches are connected to, while the second are to align, in my head, with the variables in the main sketch (ie the paddles on the Simon Says, 1,2,3 & 4).
At the moment, I am just attempting to get this particular function to be able to determine which of 4 game levels and how many players, up to 4, are selected, just using the paddles (buttons) 1-4.
Then I will have to merge them, which will probably require variable names to be changed, again. As well as a lot of new Arrays to hold all the variable & flag data for the 16 possible combinations.
Any tips/hints/pointers on using and manipulting arrays are very welcome. I like your ideas, but they don't make for readability. :cry: :cry:

Karma to you.

Fof

One tip would be to take advantage of the index :wink: I can get if you want to display level 1 to 4 or players one to 4. But:

buttonpinnumber[i] == (i + 1);

And will automatically scale :smiley:

I like.
The more I dig into Arrays, with these sketches, the more of these little "tricks" I will unearth. I hope.
The simple ways of doing things, are best.

Thanks again.

Fof