Button counter stops decreasing after 2 pushes

Projects General Guidance
1

HI, im working on a Prop Scifi laser gun, im using an arduino nano r4, dfminiplayer, and waveshare 1.69" lcd screen.

I was using the waveshare supplied driver but switched to Adafruit gfx library because it was like 500ms faster to refresh the counter on the display.

Anyway right now I'm stumped that my counter stops decreasing after it gets to 14, even if I keep pushing the button.

I can use the button with the arduino button count tutorial and it works as expected.here

thanks for reading and I appreciate your help.

#include <Adafruit_GFX.h>    // Core graphics library
#include <Adafruit_ST7735.h> // Hardware-specific library for ST7735
#include <Adafruit_ST7789.h> // Hardware-specific library for ST7789
#include <SPI.h>
#include "SoftwareSerial.h"
#include "DFRobotDFPlayerMini.h"
// Define the pin connections
#define TFT_CS        10
#define TFT_RST        8 // Or set to -1 and connect to Arduino RESET pin
#define TFT_DC         7
const int buttonPin1 = 2;  // the number of the pushbutton pins
const int buttonPin2 = 3; //RELOAD SWITCH
const int ledPin =  6;      // the number of the LED pin
static const uint8_t PIN_MP3_TX = 4; // Connects to DFMINI module's RX
static const uint8_t PIN_MP3_RX = 5; // Connects to DFMINI module's TX
Adafruit_ST7789 tft = Adafruit_ST7789(TFT_CS, TFT_DC, TFT_RST);
float p = 3.1415926;
//** define colors
#define black  0x0000
#define BLACK  0x0000
#define WHITE   0xffff
#define GREY   0x8410
#define GREY   0x8C71
#define DGREY  0x8C71
#define BLUE   0x181F
#define PINK   0xFB9F

SoftwareSerial softwareSerial(PIN_MP3_RX, PIN_MP3_TX);
// Create the Player object
DFRobotDFPlayerMini player;

int buttonState1 = 0, buttonState2 = 1;  // variable for reading the pushbuttons status
int preState1 = 0, preState2 = 1;
int counter = 16;

void setup() {
    // initialize the LED pin as an output
  //LED flashes during decrement of counter
  pinMode(ledPin, OUTPUT);
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin1, INPUT);
  pinMode(buttonPin2, INPUT);
  Serial.begin(9600);
  // Init serial port for DFPlayer Mini
  softwareSerial.begin(9600);
  
  // Start communication with DFPlayer Mini
  if (player.begin(softwareSerial)) {
    // Set volume to maximum (0 to 30).
    player.volume(30);
  } 
  else { 
    Serial.println("Connecting to DFPlayer Mini failed!" );
  }
  Serial.begin(9600);
  tft.init(240, 280);                // Init ST7789 280x240
  Serial.println(F("Initialized"));
  Serial.println("16");
  tft.fillScreen(GREY);
  tft.setRotation(1);  //screen horizontal

}

void loop() {
  // read the state of the pushbutton
  buttonState1 = digitalRead(buttonPin1);
  buttonState2 = digitalRead(buttonPin2);

if (counter > 0) { 
  if (buttonState1 == HIGH && preState1 == LOW ){ 
    counter--; 
    player.playMp3Folder(2); 
    digitalWrite(ledPin, HIGH); 
    delay(300); digitalWrite(ledPin, LOW); 
    Serial.println(counter); preState1 = 1;
  } 
  else (buttonState1 == 0);{ 
    preState1 = 0;
  }
}
}
2

Check the format of if / else statements. The conditional test after the else has no effect. Did you mean "else if"?

3

I have not looked at your code in detail but I notice that if counter is greater than 0 then preState1 is unconditionally set to 0. Is that what you intended to happen ?

Formatted better your loop() function looks like this

void loop()
{
    // read the state of the pushbutton
    buttonState1 = digitalRead(buttonPin1);
    buttonState2 = digitalRead(buttonPin2);

    if (counter > 0)
    {
        if (buttonState1 == HIGH && preState1 == LOW)
        {
            counter--;
            player.playMp3Folder(2);
            digitalWrite(ledPin, HIGH);
            delay(300);
            digitalWrite(ledPin, LOW);
            Serial.println(counter);
            preState1 = 1;
        }
        else
            (buttonState1 == 0);
        {
            preState1 = 0;
        }
    }
}
4

Are you certain it is not how you write the "counter" to the screen?

5

Do you have pull-down resistors wired to these pins? You will need them to prevent your pins from floating. Better to use INPUT_PULLUP and wire your buttons between the pin and ground. No external resistors required. It will change the logic of your tests (HIGH==Not pressed, LOW==Pressed)

6

i have 5v to button on one side, and button wired to resistor to ground and button to pin 2

7

Stripped of the unused button, display and MP3 player stuff, here's what remains of your sketch:

const int ledPin =  6; 
const int buttonPin1 = 2;             // the number of the pushbutton pins

int buttonState1 = 0;  // variable for reading the pushbuttons status
int preState1 = 0;
int counter = 16;

void setup() {
   // initialize the LED pin as an output
   //LED flashes during decrement of counter
   pinMode(ledPin, OUTPUT);
   // initialize the pushbutton pin as an input:
   pinMode(buttonPin1, INPUT);
   Serial.begin(9600);
}

void loop() {
   // read the state of the pushbutton
   buttonState1 = digitalRead(buttonPin1);

   if( counter > 0 ) {
      if( buttonState1 == HIGH && preState1 == LOW ) {
         counter--;
         digitalWrite(ledPin, HIGH);
         delay(300);
         digitalWrite(ledPin, LOW);
         Serial.println(counter);
         preState1 = 1;
      } else
         (buttonState1 == 0); // <--- compares buttonState1 to 0, does nothing with that
      {
         preState1 = 0; // <-- always executed 
      }
   }
}

As others have, I question whether (buttonState1 == 0); is doing what you think it's doing. Because it's doing nothing. If you had intended to have preState1 = 0; executed if (buttonState1 == 0); is true, you're missing an if and need to get rid of the trailing '`', i.e.

      } else if (buttonState1 == 0) {
         preState1 = 0;
      }

I uploaded that cut down sketch to an Uno, put a pull down resistor on pin 2 and and LED and serial resistor on pin 6. Grounding pin 2 does cause a count down, which as expected stops at 0.

8

I get the same reaction! but when I put the audio file line back in it stops again. player.playMp3Folder(2);

9

DFPlayer recommend the TX pin on 2 and RX pin on 3, so when I use them and put button pins on 4 & 5 it works.

10

You're using an R4, aren't you? So you have Serial1 on pins 0 & 1, a hardware serial port that is completely separately from Serial. Why not use that rather than SoftwareSerial?

11

I'm a prop builder with some background in HTML & CSS.
This is my first project with a micro-controler so I am not familiar with the software serial or serial ports. It sounds like something I should look into though, thanks.

12

It's not directly related to your issue but, you're mixing variables.

if (counter > 0) { 
  if (buttonState1 == HIGH && preState1 == LOW ){ 
    counter--; 
    player.playMp3Folder(2); 
    digitalWrite(ledPin, HIGH); 
    delay(300); digitalWrite(ledPin, LOW); 
    Serial.println(counter); preState1 = 1;
  } 
  else (buttonState1 == 0);{ 
    preState1 = 0;
  }

buttonState1 is compared to HIGH and to zero. preState1 is compared to LOW and set to zero/one.

HIGH/LOW are not guaranteed to be equivalent to true/false, 1/0.

Consider making buttonState1 and preState1 booleans and using the ternary operator thusly.

buttonState1 = digitalRead(buttonPin1) == HIGH ? true : false;

Then you can use something like:

if(buttonState1)
  { // some operations
  }

If for nothing else than consistency's sake at least make all the comparisons and assignments for these two variables either HIGH/LOW or 1/0.

YMMV

13

Yeth.

This is a good solution:

buttonState1 = digitalRead(buttonPin1) == HIGH ? true : false;

Switches are read in order to determine whether they are closed or open, pressed or not pressed. So a boolean variable would be a better fit.

And HIGH and LOW depend on the wiring and can almost feel like "magic numbers" in context: yes HIGH, but is that pressed or not pressed?

So here's three lines:

// up top a manifest constant. 
const auto PRESSED = LOW;  // what means the button is pressed?

//... 

// later 
bool doIt = digitalRead(somePin) == PRESSED;  // true if button pressed.

// and in subsequent use, it's boolean
  if (doIt) {
// stuff to do when then switch is closed, pushbutton (NO) being pressed.
  }

The auto qualifier neatly sidesteps the issue of the return type of digitalRead(), the assignment to the bool is the end of the issue.

The '==' operator means the RHS is already true or false, so we can lose the ternary operator.

This exzct issue crops around digitalWrite(), which takes as its sole argument the same HIGH and LOW only. We lost the ternary operator above but it's a language feature we can exploit here. If you've gotten this far, you don't need the nitty-grit; here's two lines:

// LED displays the boolean ledState
  digitalWrite(somePin, ledState ? ON : OFF);

// one line LED toggle
  digitalWrite(somePin, digitalRead(somePin) == OFF ? ON : OFF);

Doing makes for code that is bulletproof and makes all the comments, which I only used for clarity here, gratuitous: the code speaks for itself.


I stripped the above of my personal ranting and often expressed opinion about the API. Suffice it to say it was a stupid choice made too long ago. Oh, in my opinion. Yes, for reasons, but I hope there was at least one person on the room who was on the other side of a vigorous argument about the matter. :expressionless:

a7

14

Added to my notebook. :slightly_smiling_face:

15

I disagree. I am afraid for beginners that code speaks a foreign language and is the stuff of nightmares. The ternary operator alone is enough to confirm to beginners that programming is a black art which only wizards would understand.

For them it would be better to split it into multiple lines to make it obvious what is going on.

If anything the single line code requires more comments than the 3 line version and I challenge you to write that comment

The multi line version, however, can use sensible variable names and a succinct comment for each line if they are needed at all

    byte pinState = digitalRead(somePin);  //read the state of the pin
    if (pinState == ON)                    //invert the state read from the pin
    {
        pinState = OFF;
    }
    else
    {
        pinState = ON;
    }
    digitalWrite(pinState, somePin);    //write the inverted state to the pin

The multi line version also has the advantage that the value of intermediate variables can be printed if necessary to keep track of what is going on. The number of lines could, of course, be gradually reduced in a number of ways once the basic principles are understood.

In summary, as I see it, your 1 line version is great for you but not for a beginner

16

As long as you are willing to accept that that use of digitalRead() violates the API.

I agree that the three line version is better in some ways of measuring better. I go for clarity over cleverness and generally don't like one-liners, either.

And I have publicly offered to eat my hat if TPTB ever do such a stupid thing as break that use by cutting the grass under our feet. That is to say I don't think it will happen.

I'll cop to directly using digitalRead() and digitalWrite() as if the API allowed use of 0, false, non-zero and true as one might hope.

Eveyone does. But at some point beginners become non-beginners, at which point they might like to know what an API is and why they should care; we who pretend to expertise should lead by example.

So i say it when it seems like good time to do.

Meanwhile

digitalWrite(pinState, somePin);

might not be just what you wanna do no matter. :expressionless:

a7

17

WHOOPS !
on my part

[sarcasm]Or maybe I did that to illustrate that debugging the multi line version is easier than debugging the all in one version[/sarcasm] :grinning_face:

18

Haha, my thought was you were perhaps making a subtle point about those

an admitted weakness of mine, naming stuff.

a7

19

does "PRESSED = LOW" if the buttons are wired from 5v to the arduino pin and a 10k resistor to G?

20

No. That's a pin in the so-called pulled down configuration.

So normally the pin is pulled to ground by the resistor, and woukd read LOW.

When you connect 5 volts to the input pin, it overcomes the resistor and presents 5 volts at the pin, that then reads HIGH.

For reasons pp usually use pulled up inputs, with with the internal pullup (pin mode INPUT_PULLUP, an external resistor wired between the pin and Vcc or it might be the natural state of a sensor, which sensor would be called active low.

a7