Help...what is going on?

Hello, All. I have a sketch which uses a custom routine to check for button presses. What I want to do is be able to read a button press, record that, then do something when the button is released. This avoids a gillion loop() runs reading the same button state (active). I also want to know when the button has been held for a long period. This all seems to work just fine.

My project has an LCD screen which I turn off after a period of inactivity. A short button press should just turn it on, a long press should turn it on and do something.

My problem is this: when I short press the button...sometimes it executes the code to turn the LCD on, many times it doesn't. Long pressing the button initiates the action...I have no problems with that functionality. But the short press is making me nuts. I have some junk Serial.print statements for debugging: one prints when the button is short pressed, right before the function return true; occurs. Yet, in the calling routine, the next statements do not execute. I've condensed my main sketch to this code section below which reproduces the problem. Below that I am including the serial monitor output I get. I can find no pattern to the number of times it fails before it runs properly (pressing the button gets me the Serial.print in the button checking routine but not the follow-up action, then I get the action some number of presses later). The serial monitor output presented is from the start after uploading the code and is not consistent from run to run.

I feel insane typing that...this doesn't make sense to me. I'm still very new to the Arduino and to C++ (but not programming)...I'm hoping it's something dumb I've done.

Thank you.

--HC


//this is a playground project to dial-in the button routines
#include <Wire.h>
#include <hd44780.h> 
#include <hd44780ioClass/hd44780_I2Cexp.h> // this changed from Dev Rev 2

//pin definitions from the actual CCD code are left for clarity
//IO pin definitions
#define DimmerPin 3     //pin to connect to the LCD backpack LCD power for PWM brightness control

//button input pins
//leaving 3 buttons open for future use, 9, 10, 11
#define OPENpin     7      //manual open button
#define CLOSEpin    8     //manual close button

#define CycleTime 10000     //seconds in millis to run the motor output(s)

//timeout for dimming the display
const int ScreenSaver = 10000;      //starting at 10,000ms, 10 seconds
static unsigned long LastSSTime = 0;    //the last millis() that any button was pressed
static byte ButtonState = 0b11111111;        //8 bits to record past button state to detect rising edge (release) of a button **FOR ACTIVE LOW**
static bool ScreenOnYN;      //var to know if the screen is currently on or not

hd44780_I2Cexp lcd(0x27); // declare lcd object: auto locate & auto config expander chip

// LCD geometry
const int LCD_COLS = 20;
const int LCD_ROWS = 4;

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

    //be sure all IO are HIGH
    digitalWrite(OPENpin,       HIGH);        //active LOW, all have pull-ups
    digitalWrite(CLOSEpin,      HIGH);       //active LOW

    pinMode(OPENpin,        INPUT);        //input to OPEN door
    pinMode(CLOSEpin,       INPUT);       //input to CLOSE door

    lcd.begin(LCD_COLS, LCD_ROWS);
}

void loop()
{
    bool ButtonLongPress = false;

    if (DetectButtonPress(OPENpin, ButtonLongPress) == false)    //button state change not detected, let's see if it's actively being held for long-press
        //what this does is to initiate the action of a long-press event without waiting for state change (button release)
        {
            if (ButtonLongPress == true)        //button is being pressed (but not state change to release) but is long-pressed
                {
                    Serial.println("Open button not released, but long press event detected");
                    BacklightOn();      //running this here so that each time the button is pressed it updates the screensaver countdown
                    //OpenDoor();         //this function effectively delays for the duration of the open/close event allowing the user to release the button
                    //DoorOpenYN = true;
                }
        }

    //Serial.println("Calling DetectButtonPress 2");
    if (DetectButtonPress(OPENpin, ButtonLongPress))
        //here the button is pressed and released and if it was not long-pressed, we'll turn the backlight on
        {
            Serial.println("button press");
            if (ButtonLongPress == false)
                {
                    Serial.println("Open button pressed, not long pressed");
                    BacklightOn();      //this is here because pressing any button should turn on the backlight if it's off
                }
        }

    //check to activate screensaver
    if (millis() - LastSSTime > ScreenSaver)
        {
            BacklightOff();
        }

}

bool DetectButtonPress(int ButtonPin, bool &LongPress)
  {
    static unsigned long int LastButtonPress = 0;
    int buttonBit = 0;

    buttonBit = ButtonPin - 7;  //buttons are on pins sequentially from pin 7-11
    LongPress = false;          //default value

    if (digitalRead(ButtonPin) == LOW && ButtonState & 1 << buttonBit)      //input pins for 5 buttons: 7, 8, 9, 10, 11
        //we read ButtonPin as LOW (active low/pressed) *and* ButtonState bit AND 1 = 1 (bit is HIGH/inactive)
        //this is a state change from inactive to active (unpressed to pressed)
        {
        //the button is pressed (LOW) and the ButtonState bit is HIGH, we just detected a press
        ButtonState ^= 1 << buttonBit;       //toggle bit 1 to 0 so we know it's PRESSED

        LastButtonPress = millis();   //set our press timer

        return false;   //the button was just detected as down and it is a change
        }

    if (digitalRead(ButtonPin) && (ButtonState & 1 << buttonBit) == 0)
        //we read ButtonPin as HIGH (inactive high/released) *and* ButtonState bit = 0 (bit is LOW/active)
        //this is a state change from active to inactive (pressed to unpressed)
        {
            ButtonState ^= 1 << buttonBit;       //toggle bit 1 to 1 (HIGH/inactive), button is released

            if (millis() - LastButtonPress > 1000)
            {
                LongPress = true;         //this is returned only if the button is released but was held > line above
            }
            
            Serial.println("buttonpin high released");
            return true;    //button has state changed from active to inactive, released
        }
        
    if (digitalRead(ButtonPin) == LOW && (ButtonState & 1 << buttonBit) == 0)
        //we read ButtonPin as LOW (active LOW/pressed) *and* ButtonState bit 1 = 0 (bit is LOW/active)
        //this is a continuing button press
        {

            if (millis() - LastButtonPress > 1000)
                {
                    LongPress = true;    
                }
          
            return false;    //returning false because I want this function to return true only on state change.  however, long-pressing the button should immediately intiate the action
        }

    //still here means the button has not changed state and, if pressed, it's not a long press
    return false;
        
  }

void BacklightOn()
    {
        lcd.backlight();
        analogWrite(DimmerPin, 150);
        ScreenOnYN = true;
        LastSSTime = millis();
    }

void BacklightOff()
    {
        lcd.noBacklight();
        ScreenOnYN = false;
    }

I ran this just prior to writing this post (I've been at this for a few hours today and I have run it many times as I tried to figure it out). Below, in a code section for better readability, is the output I got. You can match the printed output to the code section to try to follow how this is (or is not) working. The two lines "button press" and "Open button pressed not long pressed" indicate the code executed as expected. Multiple lines "buttonpin high released" without those two lines indicate the code did not execute as expected.

buttonpin high released
button press
Open button pressed, not long pressed
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
buttonpin high released
button press
Open button pressed, not long pressed

You need to break this into small parts. Start with the button read. When you have that down, try your next step... here...

https://docs.arduino.cc/built-in-examples/digital/Button/

I think I'm good on that. I have the button debounced (RC) and with a pull-up, active LOW. It reads the buttons correctly. What's messing with my head is that the routine that checks the button state reads the button as released, Serial.println() to let me know that, then the very next statement is return true; yet the calling routine behaves as if it received a false.

--HC

The problem is no pullups on your inputs (at least, that's what I saw when I tried it here with no external pullups). Change to

   pinMode(OPENpin, INPUT_PULLUP);   //input to OPEN door
   pinMode(CLOSEpin, INPUT_PULLUP);  //input to CLOSE door

and things calm down. Still logic problems to be dealt with, but no endless released messages.

1 Like

Well, that's a mixed bag. I have pull-ups but I tried the _PULLUP on the pin and it actually got better. It's not consistently working, but it's working more frequently. I only tested it about 40 times and on one compile. What still messes with my head is that the code that prints "buttonpin high released" is right before the return true; which should still be caught at the calling code but it's not (consistently, anyway). But...this is microcontrollers, maybe some feng shui voodoo?

I'll go back over my RC stuff to pull-up and debounce tomorrow (after midnight here, bed is not far off). I looked it over carefully now, but maybe revisiting that second resistor (see below) and changing it to a 1k.

I have 10k - btn - 10k - 0.1uf - GND. The button connects to GND, input pin to juncture of the second 10k and the 0.1uf.

Thank you.

--HC

  • For de-bouncing switches, all you need to do is sample them every 50ms.
    Look for a change in state.

In my experience, word descriptions of circuits such as you have tried are not a productive exercise. Yours did not help my understanding of what you have connected. It sounds like your inputs may only being pulled up to around 1/2Vcc, which would be bad. Much better to put pen to paper, draw a rough schematic and take a picture of the result.

And there's no feng-shui voodoo going on. But you really ought to think about the effects of having two calls to DetectButtonPress in your loop function, and only one of the ifs they're in printing "button press" - and having no debounce code. it'll be obvious when you finally see it, I think.

As I said earlier, you've still got logic problems to deal with, and that's a big one.

What's the purpose of these parts of the code? Seems unnecessarily complex for the simple requirements you described.

Almost any of the many button libraries will give you a function similar to

  if (myButton == Released) {
    .. do something
  }

"btn" has pin-pairs that are Normally Closed and Normally Open. When using the internal R10k INPUT_PULLUP you want the pin and GND to use an open pair.

Untitled

1 Like

Riiiiight.

Just draw it. Your prose, I've tossed it around trying to see what you doing. Can't.

The button handling is somewhat of a tangle, at least it looks like it would only be able to handle activity on one button at a time due to what appear to be shared global variables.

This is clever, but locks in your pins. Use an array to hold the PIN numbers, then use indexing to get at buttons number 0, 1, 2 and so forth.

    buttonBit = ButtonPin - 7;  //buttons are on pins sequentially from pin 7-11

And here, very efficient, but confusing. Again, an array of type bool might make it clearer

Instead of

        ButtonState ^= 1 << buttonBit;       //toggle bit 1 to 0 so we know it's PRESSED

try

    buttonState[buttonNumber] = !buttonState[buttonNumber];

Or to reduce typing error possibilities at the expense of whatever is the expense

    buttonState[buttonNumber] ^= true;

But first, a schematic for the RC button debounce. Or just for now add a 20 ms delay to your loop and debug your hardware solution later.

a7

k..

First, you're right about the description. It seemed clear at the time but I'm sure I'd be confused if it weren't my words and in-hand board. Below is the schematic which I got from "embed with Elliot".

I chose hardware instead of software for debouncing partially because it was easy to implement it for multiple buttons (just duplicated parts) and because I am new to the embedded programming and keeping lines of code out that I'd have to debug and read through seemed appealing.

I have two calls because 1) I don't know it's not okay and 2) I'd then have to store those returned values in variables and do my checks against them. My general feeling towards coding for the ATMega328 is that is has very limited resources so conserving memory (not having a couple more variables) seemed like a good idea. To be clear: all the Serial.print stuff is only there after I found a problem with functionality and have tried to ferret that out; there isn't any need for any Serial output when this works properly. It's only there because that's where I felt I was encountering the problem. I can change the code to have one call and store the values but, at least for a little bit, I'm going to leave it as it is: if that is the failure point, I want to know it is. If I run across this kind of problem in the future, I'd like to know definitively what the cause is.

I agree that when the problem is found it will be obvious. While the kits and the concept of the Arduino are targeted at beginners, there is still a lot of brainy stuff that goes into making it work well (embedded programming and circuit design). I'm probably doing something stupid, I just can't see it yet.

--HC

What I'm trying to achieve with the bit logic is tracking the current state of the buttons. The project I'm on has 5 buttons and I'm trying to catch button presses on the release. What I'm wanting to avoid is the loop() running way faster than button presses and cycling several or many times and catching a "press" event on each loop. This would be the equivalent of "keyup" or "keydown" in Windows programming and I want "keyup". Again, being new to this (and always being prone to errors) I wasn't sure how to effectively catch a button press and act on it only one time. I chose to record the button state. I chose to use bits in a byte to save storage memory because having a state variable for each button seemed excessive. Plus, I can programatically check the buttons with the bit logic, one function for many buttons.

None of that I wrote above is saying it was okay or the best choice or even a good choice, but it is the logic I used to get to this answer, right or wrong.

Thank you.

--HC

Think about what happens when your first call to DetectButtonPress detects a button press, and your code tosses it silently.

Think about what happens when your second call does the same to button release.

Consider one call, like

if( DetectButtonPress(OPENpin, ButtonLongPress) ) {
   // do something when pressed
} else { 
  // do something else when not pressed
}

and see where that takes you.

And that is the pit into which it easy to fall when you don't know much. I didn't know there were "button libraries". I should have known that...the language has existed for decades and the Arduino for 15+ years I think. But, I didn't think of that.

As it sits, I don't believe my current problem of intermittent/inconsistent program functioning is in the button handler, or at least that's my perception (which could always be wrong).

When I'm done responding to everyone here, I will go look into button libraries.

Thank you.

--HC

First, I made an error. I confused one schematic for another on the site from which I got the debounce circuits. I shouldn't have used a 10k for the second resistor. That was from a circuit to be coupled with a 74HC14 as part of cleaning button presses. Just one error in a stack for me. I've posted the schematic in another post but, for clarity, here it is:

I've corrected my error on the breadboard but I still get the intermittent functionality of the code. That still seems to be in a function call which returns true yet isn't acted upon at the calling function as if it is true.

Back to your response:

I'm not saying I'm right in anything I've done here, just saying what I did, right or wrong. The global variable ButtonState is manipulated one bit at a time so it should handle the activity on multiple buttons. However, in use, the project is only to be interacted with one button at a time. "This" button then "that" button.

I like the button pin array, that would have made the code more reusable and flexible.

Yes, the bit logic stuff is cool and slim, but you are right, confusing. You'll notice the comment lines are plentiful to explain to myself what I'm doing...6 mos from now I won't be able to understand it without those comments. I was thinking the single byte to hold state for 8 buttons (I only need 5 here) would conserve memory resources. This may be academic and silly...I'm new.

Thank you again.

--HC

I just use INPUT_PULLUP, no need for an external resistor. The button press then is a LOW value. If you use a library, they may or may not invert that.

Thank Jesus, that worked! I made one change, replacing the second if (Detect... with just "else", compiled/uploaded...boom! 10 times in a row it has functioned as it was intended. I have the breadboard setup right next to the keyboard, even after the 10 times I've hit it periodically and it works every time.

When I went back to edit the code as you've suggested here, it was very clear I needed to do what you suggested. Code readability matters and I'd started out with stuff like: if (DetectButtonPress()) but had changed it to be more explicit as I was getting frustrated to ...(DetectButtonPress() == true.... This includes the !DetectButtonPress() after it which was changed to ...== false. When I looked at the two calls, back to back, to make the change you suggested it was immediately clear it should only have been an else not a separate if. I was too far into the weeds (and locked in the box) to see it myself, it took you pointing it out so clearly.

I've kept the original code in a comment block so I can go back and look it over, see if I can learn anything else from it. I would still like to know why it wasn't working, explicitly. Understanding exactly how it was failing would help with maybe avoiding the same kind of problem in the future. Maybe. I'm a pretty resourceful idiot. :wink:

Thank you for your help.

--HC

This is a personal thing: I like the tangible pullups. I very much understand about the active LOW and will keep that in mind when looking at the libraries.

My original reason for posting is corrected now. So, now, I'm going to look into such libraries.

Thank you.

--HC

1 Like