issue with flicker on button inputs when not pressed, yes I'm using input_pullup

I’m working on a racing arcade, implementing the original buttons and analog inputs to a USB HID device. Buttons close between the target pin and the ground connection on the arduino Leonardo. Buttons show up as pressed correctly when pressed, but when not pressed, they’re flickering randomly, is it a code issue?

#include "HID-Project.h"

const int pinLed = LED_BUILTIN;
const int pinButton = 2;

void setup() {
  pinMode(pinLed, OUTPUT);
  pinMode(pinButton, INPUT_PULLUP);
  Gamepad.begin();
}

void loop() {
  float steeringmin=0;
  float steeringmax=1023;
  float brakemin=0;
  float brakemax=1023;
  float accelmin=0;
  float accelmax=1023; 
  float ruddermin=0;
  float ruddermax=1023;
  float min=-32767;
  float max=32767;
  float diff=max-min;


  float steer = (float)analogRead(0)/(steeringmax-steeringmin)*diff + min;
  float accel = ((float)analogRead(1)/(accelmax-accelmin))*diff + min;
  float brake = ((float)analogRead(2)/(brakemax-brakemin))*diff + min;
  float rudder = ((float)analogRead(3)/(ruddermax-ruddermin))*diff + min;
  // on second thought, I just could have multiplied one thing by 64 to get the result

  int up = digitalRead(0);
  int left = digitalRead(1);
  int down = digitalRead(2);
  int right = digitalRead(3);
  int start = digitalRead(4);
  int back = digitalRead(5);
  int gearup = digitalRead(6);
  int geardown = digitalRead(7);
  int gearleft = digitalRead(8);
  int gearright = digitalRead(9);
  int gearreverse = digitalRead(10);
    
  if ((accel <= 3) and (accel >= -3)) {
    accel=0;
  }
  if ((brake <= 3) and (brake >= -3)) {
    brake=0;
  }
  if ((rudder <= 3) and (rudder >= -3)) {
    rudder=0;
  }

 if (gearup == 0)
 { 
   if (gearleft == 0)
   {
    Gamepad.press(7);
   }
   else if (gearright == 0)
   {
    Gamepad.press(11);
   }
   else {
   Gamepad.press(9);
   }
 }

 else if (geardown == 0)
 { 
   if (gearleft == 0)
   {
    Gamepad.press(8);
   }
   else if (gearright == 0)
   {
    Gamepad.press(12);
   }
   else {
   Gamepad.press(10);
   }
 }
 else {
   Gamepad.release(7);
   Gamepad.release(8);
   Gamepad.release(9);
   Gamepad.release(10);
   Gamepad.release(11);
   Gamepad.release(12);
 }
 if (gearreverse == 0)
 { 
   Gamepad.press(13);
 } else {
   Gamepad.release(13);
 }

 if (up == 0)
 { 
   Gamepad.press(3);
 } else {
   Gamepad.release(3);
 }
 if (left == 0)
 { 
   Gamepad.press(4);
 } else {
   Gamepad.release(4);
 }
 if (down == 0)
 { 
   Gamepad.press(5);
 } else {
   Gamepad.release(5);
 }
 if (right == 0)
 { 
   Gamepad.press(6);
 } else {
   Gamepad.release(6);
 }
 if (start == 0)
 { 
   Gamepad.press(1);
 } else {
   Gamepad.release(1);
 }
 if (back == 0)
 { 
   Gamepad.press(2);
 } else {
   Gamepad.release(2);
 }

 
 Gamepad.xAxis((int)steer);
 Gamepad.yAxis((int)accel);
 Gamepad.rxAxis((int)brake);
 Gamepad.ryAxis((int)rudder);
 Gamepad.write();
}

Why are you using floats ? (Stick just to digitalRead and AnalogRead integer values)

You only have an INPUT_PULLUP on one pin (2). What about the others?

And please use code tags and not a table to show you code; far easier to copy. Maybe edit your original post for this.

Note:
The intent of the declarations for the pins in the beginning of your code is to use them everywhere, not only in setup
e.g.

digitalRead(pinButton)

instead of

digitalRead(2)

Good catch sterretje - i blindly trusted the title which would have been the obvious mistake but OP seemed to understand that part so only looked at the loop... the devil is always in the details :slight_smile:

Hi,
Welcome to the forum.

Please read the first post in any forum entitled how to use this forum.
http://forum.arduino.cc/index.php/topic,148850.0.html then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.

Can you please post a copy of your circuit, in CAD or a picture of a hand drawn circuit in jpg, png?

Thanks.. Tom... :slight_smile:

J-M-L:
Why are you using floats ? (Stick just to digitalRead and AnalogRead integer values)

I didn't write all of this, it's about 90% someone else's 'functional' code, so it's mostly how it was made. Since they clearly had much more experience than me, I trusted their judgement on things like this. Should it be changed?

sterretje:
You only have an INPUT_PULLUP on one pin (2). What about the others?

Ok, so this is probably the cause. This code is mostly made by someone else who said that it was functional with at least 4 buttons connected to it, I expanded it to 11. I figured it had what was needed to duplicate the pullup to the additional pins. I'm not sure how to fix the code to correct this, should it just have the following?:

pinMode(0, INPUT_PULLUP);

pinMode(1, INPUT_PULLUP);
pinMode(2, INPUT_PULLUP);
pinMode(3, INPUT_PULLUP);
pinMode(4, INPUT_PULLUP);
pinMode(5, INPUT_PULLUP);
pinMode(6, INPUT_PULLUP);
etc...

TomGeorge:
Hi,
Welcome to the forum.

Please read the first post in any forum entitled how to use this forum.
http://forum.arduino.cc/index.php/topic,148850.0.html then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.

Can you please post a copy of your circuit, in CAD or a picture of a hand drawn circuit in jpg, png?

Thanks.. Tom... :slight_smile:

I didn't put in all the inputs, but it's more of the same for the remainder.

I think you should learn a little about the use of arrays for pins and pin states. And as said before, don’t use things like digitalRead(0); if you ever want to change that, you might have a hard time finding every place that you have to change.

const int pinLed = LED_BUILTIN;
const int buttonPins[] =  {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
int buttonStates[sizeof(buttonPins) / sizeof(buttonPins[0])];

Now in setup(), you can loop through the button pins using e.g. a for-loop and set them to use INPUT_PULLUP.

void setup()
{

  for (int cnt = 0; cnt < sizeof(buttonPins) / sizeof(buttonPins[0]); cnt++)
  {
    pinMode(buttonPins[cnt], INPUT_PULLUP);
  }
}

And in loop() you can read them all in the same way.

  for (int cnt = 0; cnt < sizeof(buttonStates) / sizeof(buttonStates[0]); cnt++)
  {
    buttonStates[cnt] = digitalRead(buttonPins[cnt]);
  }

Now the only ‘problem’ left is accessing the array elements using sensible names; place the below near the top of your code

// sensible names for index into the arrays
#define BTN_UP 0
#define BTN_LEFT 1
#define BTN_DOWN 2
#define BTN_RIGHT 3
#define BTN_START 4
#define BTN_BACK 5
#define BTN_GEARUP 6
#define BTN_GEARDOWN 7
#define BTN_GEARLEFT 8
#define BTN_GEARRIGHT 9
#define BTN_GEARREVERSE 10

And in loop, you can access the pinStates like e.g.

  if (buttonStates[BTN_UP] == LOW)
  {
    if (buttonStates[BTN_GEARLEFT] == LOW)
    {

    }
    else if ( ... )
    {

      ...
      ...
      ...

  }

If you now ever have to change move a button to another pin, you just change the pin number in the array.

In the last step, you can combine the buttonPins and buttonStates in a struct. But that’s another chapter :wink:

That's great, but I do arduino programming about twice a year, and there's no way I'll remember what that code means next time I look at it 2 years from now. I rarely do any programming, and that's WAY above my level of understanding of code. It would be great to learn, but it's just going to make it more confusing when I go back to it in a few years when I'm troubleshooting the arcade cabinet because I'm not going to remember how that was done.

So reiterating my last question, would this be sufficient to fix the issue with the pullups not on all pins I'm using to just add this?

pinMode(0, INPUT_PULLUP);
pinMode(1, INPUT_PULLUP);
pinMode(2, INPUT_PULLUP);
pinMode(3, INPUT_PULLUP);
pinMode(4, INPUT_PULLUP);
pinMode(5, INPUT_PULLUP);
pinMode(6, INPUT_PULLUP);
etc...

instead of the current

pinMode(pinButton, INPUT_PULLUP);

Hi,
Please take sterretje's advice and code with proper variable names, you will learn a lot doing it and naming constants and variables with meaningful names will help;

It would be great to learn, but it's just going to make it more confusing when I go back to it in a few years when I'm troubleshooting the arcade cabinet because I'm not going to remember how that was done.

with minimizing your confusing.

How important is this project, and why do you only code twice a year?

Tom.... :slight_smile:
PS, Writing code can take a few minutes to hours to days, depending on the job, but in all cases PLEASE consider how easy you make the code to read.
If you want to zip out meaningful and efficient code in ten minutes and then leave it, you have the wrong mindset.

TomGeorge:
Please take sterretje's advice and code with proper variable names, you will learn a lot doing it and naming constants and variables with meaningful names will help;
with minimizing your confusing.

I already am using proper variable names...
int up = digitalRead(0);
int left = digitalRead(1);
int down = digitalRead(2);
int right = digitalRead(3);
int start = digitalRead(4);
int back = digitalRead(5);
int gearup = digitalRead(6);
int geardown = digitalRead(7);
int gearleft = digitalRead(8);
int gearright = digitalRead(9);
int gearreverse = digitalRead(10);

seems pretty clear to me about what they're doing.

The issue I'm having isn't a variable issue, it's a pullup implementation issue.

At this point, I'm not changing the code, the suggestion is too complex, I don't understand the code he's suggesting, and for some reason, nobody will confirm if the simple solution I suggested would fix it. I just installed physical pullup resistors and that's fixed it without having to reprogram.

How important is this project, and why do you only code twice a year?

not super important, it's an arcade cabinet for personal use.

I don't work in programming. I'm predominantly a biologist/chemist, so I only program when I need an Arduino to do something for a project, which has been about 5 times in the last 2-3 years.

Yes setting the pinMode to INPUT_PULLUP on all the buttons pins would do it

The advise on variable names was to add

const byte upPin = 0; // give your pin a name 
....
int up = digitalRead(upPin); // use upPin instead of 0 everywhere needed.

Ok, that makes more sense worded that way. In this code, the only place I'm calling the pin is the int value assignment and the pullup at the beginning, so doing this isn't saving me any code in this sketch.

Sure your case is super simple here

in the general case, having those at the begining of the code helps understand in one quick look what needs to be wired where easily without digging into the code. Also declaring them as const byte won't use any extra memory usually as the compiler will just replace the values in the pinMode or digitalRead calls for you, so it's as efficient memorywise, just a better coding practice