My first class but why does it work correctly?

[OmeJozz]

Hi all, (i hope i have chosen the correct catagory)

I have recently started learning C++ with the learncpp website (awesome website), and i finaly made it to chapter 14 (classes).

Just for practice i wrote a small class for two buttons and as a result the serial monitor should print “Button”. It worked, but i couldn’t get only the rising edge so it would keep printing “Button” while i held the button down.

So i cheated a little bit and looked in the library of ezButton and adjusted my class and now it works like a charm.

But i can’t seem to figure out why it works correctly (only reacting to a rising edge).

As far as i can see the conditions for a “true” in my buttonPressed() function don’t seem to change while i hold the button down.

Can someone please explain to me what i’m missing here?

Thank you.

class Button {
private:
  int m_pinNumber{};
  bool m_pullup{ false };
  bool m_pressed{ false };

  int m_previousSteadyState{};
  int m_lastSteadyState{};
  int m_lastBouncyState{};
  unsigned long m_lastDebounceTime{ 0 };
  unsigned long m_debounceDelay{ 50 };


public:
  Button(int pin, bool pullup)
    : m_pinNumber{ pin }, m_pullup{ pullup } {
    if (m_pullup) {
      pinMode(m_pinNumber, INPUT_PULLUP);
    } else {
      pinMode(m_pinNumber, INPUT);
    }

    m_previousSteadyState = digitalRead(m_pinNumber);
    m_lastSteadyState = m_previousSteadyState;
    m_lastBouncyState = m_previousSteadyState;
  }


  bool buttonPressed() {
    if (m_previousSteadyState == 1 && m_lastSteadyState == 0) {
      return true;
    } else {
      return false;
    }
  }

  void buttonLoop() {

    int buttonRead = digitalRead(m_pinNumber);  //Read button pin

    if (buttonRead != m_lastBouncyState) {  //Button changed
      m_lastDebounceTime = millis();        //Reset debounce timer
      m_lastBouncyState = buttonRead;
    }

    if (millis() - m_lastDebounceTime > m_debounceDelay) {  //reading longer then debounce time
      m_previousSteadyState = m_lastSteadyState;
      m_lastSteadyState = buttonRead;
    }
  }
};  // class Button

Button knop(26, true);
Button knop2(25, false);

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

void loop() {
  knop.buttonLoop();
  knop2.buttonLoop();

  if (knop.buttonPressed()) {
    Serial.println("Button");
  }
  if (knop2.buttonPressed()) {
    Serial.println("Button 2");
  }
}

[red_car]

@OmeJozz

if (m_previousSteadyState == 1 && m_lastSteadyState == 0)

This is the key line of code. The buttonPressed routine only returns true when the button state changes from 0 to 1 (from “not pressed” to “pressed”).

The real problem here is that you have chosen terrible variable names. “last” and “previous” mean the same thing and therefore your code is very confusing. Replace “last” with “current” and see how much better the code reads…

[xfpd]

@red_car

problem here is that you have chosen terrible variable name

Very true.

Also, when I "wire" my button for using the internal pullup resistor (pin >> button N.O. >> button N.O. >> GND), using "false" in the second argument does not recognize a button press and release.

@OmeJozz

Button knop(26, true); // this works with pin >> button N.O. >> button N.O. >> GND
Button knop2(25, false);  // this is not recognized button press/release wired like previous button

[sterretje]

You should not access hardware in constructors. It's not guaranteed that the settings (e.g. pinMode) are not modified before main() is called. It works for AVR and a number of other Arduino boards but there is one board / core where it does not work; I can´t remember which one.

Write a begin() method where you do the initialisation.

[build_1971]

Some esp32 functionality is not available before main is called. Took me a lot of time to find that out. So, indeed, best to not do this...

[6v6gt]

That is the classic button debouncing algorithm where the state of a button is considered to have changed only if the new state has been steady for X ms, in your case 50.

@OmeJozz

(only reacting to a rising edge)

If you were to look at the voltage on the arduino pin during a press of the button it would appear to be the falling edge because the button, the way you have wired it to be compatible with INPUT_PULLUP, causes a transition from HIGH to LOW on being pressed.

[alto777]

@OmeJozz

But i can’t seem to figure out why it works correctly

That's always the trouble with code you find waiting around.

The concept as it has been explained is "state change detection", or edge detection. It's one of the two main issues you run up against dealing with pushbuttons.

It is described with an example in the IDE. Here's a version that goes a bit further so you can see all of what goes into edge detection.

Storing Button State

I normally point ppl to the IDE Examples/02.Digital/StateChangeDetection sketch, and the article that goes with it. Here is an adjustment and reduction of that example, which actually works. And somewhat less code to slog through to get the basic idea. One of the original's flaws is that it does not handle Try it here [Wokwi_badge] State Change Detection This code also follows the IPO model, which is a very useful and very used model for many the things we do with the Arduino. IPO Pr…

It uses a trick to essentially ignore the other part of button handling: debouncing the contacts. That's important but can be so set aside for moment so can focus on the logic of the this press compared to a previous press, state change maybe stuff.

Both edge detection and debouncing involve just a few lines of code; understanding them even if you can't yet write them out on a blank piece of paper.

There are many tutorials and examples for button handling, check out a few and trace through the code to see how each accomplishes the same essential tasks.

a7

[OmeJozz]

Sorry, i'm struggeling a bit with how to get nice looking quotes. Can't seem to find some good (newbie) information on this so i'll reply this way.

@red_car

"The real problem here is that you have chosen terrible variable names."

Thank you for this tip. I changed the variable names and, after a bit more checking the code, i think i get it.
After 50ms m_previousSteadyState and m_lastSteadyState (now m_currentSteadyState) are different so the buttonPressed() function returns "true".
Next round though the code m_previousSteadyState gets updated and the buttonPressed() function returns "false".
It all seems a lot clearer now, thanks.

@xfpd

using "false" in the second argument does not recognize a button press and release.

Button knop(26, true);
Button knop2(25, false);

I'm not shure i understand what you mean to say here.
I have two buttons. One normal pushbutton and the other is a KY-040 rotary switch. The second has its own pullup resistor. So i only use the bool to set the internal pullup for the pushbutton.

@sterretje

Write a begin() method where you do the initialisation.

Thank you for this. Because this is my first attempt writing a class, i can use any help i can get.
I assume you mean that i should make a member function begin() to set the pinModes and call this in setup?
Like i wrote, i'm just starting to realy learn C++ for a little while now, but i assume that setup() in arduino is actualy inside the C++ main()?

@6v6gt

I know my remark on the "rising edge" is wrong. Sorry for that.
I understand that the pin is actualy looking for a falling edge, but i actualy ment the rising edge of the physical action of pushing the button.
I could have explained that better, but that's the way my mind works. I know i'm a bit messed up...
Again, sorry for the confusion.

@alto777

Thank you for the link, i'm gonna read through that and the "IPO Programming Model" link later today.
This can only help me understand things better i think.

Thanks everybody for helping me out, getting a little bit better at this every day.

[GolamMostafa]

@OmeJozz

setup() in arduino is actualy inside the C++ main()?

Look here:

int main(void)
{
    init();              // Initialize hardware (timers, ADC, etc.)

    setup();             // Your setup() runs once

    while (true)
    {
        loop();          // Your loop() runs repeatedly

        if (serialEventRun) {
            serialEventRun();  // Optional serial event handling
        }
    }

    return 0;
}

[sterretje]

@OmeJozz

I assume you mean that i should make a member function begin() to set the pinModes and call this in setup?

Yes. The begin() method must be public so it can be called from setup().

[OmeJozz]

I made the begin() function and it works like a charm, thanks for the advice.
For now i call begin() for each button (so 2 calls in my case), i assume this is (for now at least) the best way to do this?

Allmost forgot...
@GolamMostafa
I checked the arduino main.cpp and found what you wrote, thanks.

[UKHeliBob]

Please post your revised sketch

[OmeJozz]

Below the sketch as it is now.

class Button {
private:
  int m_pinNumber{};
  bool m_pullup{ false };
  bool m_pressed{ false };

  int m_previousSteadyState{};
  int m_currentSteadyState{};
  int m_lastBouncyState{};
  unsigned long m_lastDebounceTime{ 0 };
  unsigned long m_debounceDelay{ 50 };


public:
  Button(int pin, bool pullup)
    : m_pinNumber{ pin }, m_pullup{ pullup } {
  }

  void begin() {
    if (m_pullup) {
      pinMode(m_pinNumber, INPUT_PULLUP);
    } else {
      pinMode(m_pinNumber, INPUT);
    }

    m_previousSteadyState = digitalRead(m_pinNumber);
    m_currentSteadyState = m_previousSteadyState;
    m_lastBouncyState = m_previousSteadyState;
  }


  bool buttonPressed() {
    if (m_previousSteadyState == 1 && m_currentSteadyState == 0) {
      return true;
    } else {
      return false;
    }
  }

  void buttonLoop() {

    int buttonRead = digitalRead(m_pinNumber);  //Read button pin

    if (buttonRead != m_lastBouncyState) {  //Button changed
      m_lastDebounceTime = millis();        //Reset debounce timer
      m_lastBouncyState = buttonRead;
    }

    if (millis() - m_lastDebounceTime > m_debounceDelay) {  //reading longer then debounce time
      m_previousSteadyState = m_currentSteadyState;
      m_currentSteadyState = buttonRead;
    }
  }
};  // class Button

Button knop(26, true);
Button knop2(25, false);

void setup() {
  Serial.begin(115200);
  knop.begin();
  knop2.begin();
}

void loop() {
  knop.buttonLoop();
  knop2.buttonLoop();

  if (knop.buttonPressed()) {
    Serial.println("Button");
  }
  if (knop2.buttonPressed()) {
    Serial.println("Button 2");
  }
}

[build_1971]

My compliments for your willingness to learn and clear coding!
Keep up the good work!

[build_1971]

You might write this:

@OmeJozz

 bool buttonPressed() {
    if (m_previousSteadyState == 1 &&        m_currentSteadyState == 0) {
      return true;
    } else {
      return false;
    }
  }

As:

 bool buttonPressed() {
    if (m_previousSteadyState == 1 && m_currentSteadyState == 0) {
      return true;
    }
    return false;    
  }

This way it is easy to see that the function always returns a valid value...

[GolamMostafa]

@OmeJozz

1.
Thank you for the class-based sketch. However, I find it somewhat difficult to follow exactly what is happening in your code.

2.
I would appreciate it if you could move all the function definitions, including the constructor, outside the class body, as shown below. Also, please include sufficient comments explaining the purpose of key lines of code.

//class creation and declration
class Button
{
     private:
          variable declarations

     public:
         member functions declrtaion
};

//object creation
Button knob1(arg1, arg2);
Button knob2(arg1, arg2);

void setup()
{

}

void loop()
{


}

//definitions of the class member functions

//definitions of user functions

3. And then organize the code of Step-2 nto three files:

.ino
.h
.cpp

4. Please, briefly explain the objectives of your sketch in Post #1. Also, kindly mention the Arduino board you are using. I will try to reformat your sketch in a more student-friendly way.

5. This is your constructor functon taken from post #1. Can you please, explain the purpose of your constructor? And also, explain its execution mechanism.

Button(int pin, bool pullup) : m_pinNumber{ pin }, m_pullup{ pullup } 
{
    if (m_pullup) 
    {
        pinMode(m_pinNumber, INPUT_PULLUP);
    } 
    else 
    {
        pinMode(m_pinNumber, INPUT);
    }

    m_previousSteadyState = digitalRead(m_pinNumber);
    m_lastSteadyState = m_previousSteadyState;
    m_lastBouncyState = m_previousSteadyState;
}

[OmeJozz]

@build_1971
Thanks for the advice, i've adjusted the function. Looks e little better this way.

To be honest, before actualy learning C++ i got most of my information from tutorials and learned some bad habits that way. I'm trying to un-learn some of these.
Getting there slowly, but getting there eventualy.

[GolamMostafa]

@OmeJozz

To be honest, before actualy learning C++ i got most of my information from tutorials and learned some bad habits that way. I'm trying to un-learn some of these.
Getting there slowly, but getting there eventualy.

Ask yourself if you know the meanings of various fields of a class template (Fig-1) of C++.

Figure-1:

[OmeJozz]

@GolamMostafa
Well i think i understand the basics of the class by now (took me a little while), but i also know there is so much more to learn.

This was actualy just a practice for me to learn to use classes, so i forgot to add comments
I will update this one with comments, try to split it in a .ino .h and .cpp file. something i also need to practice, so this will be good for me too.

I'll get on this this evening and when i'm done i will add the files to this post and i will try to answer you other questions.

But first... dinner time (i'm kinda hungry)

[gcjr]

the class has unnecessary code making it more difficult to read. It seems newbies think such extra code makes their code more professional, when simple is better

i see no reason for both buttonLoop() and buttonPressed() unless the class could monitor multiple buttons in which case buttonLoop() would be called once for all buttons and buttonPressed() called to determine if individual buttons pressed.

not clear why buttonLoop() updates the button state (e.g. m_previousSteadyState) but buttonPressed () evaluates the states to determine if a button was pressed

is there any need to capture more than the current state of the button?

and shouldn't the timer logic abort any processing, just returning false instead of doing partial processing?

couldn't buttonPressed() more simply be

    bool buttonPressed ()
    {
        unsigned long msec = millis ();
        if (msec - msec0 < MsecDelay)
            return false;

        int but = digitalRead (Pin);
        if (butState != but)  {
            butState  = but;
            msec0     = msec;

            if (LOW == but)
                return true;
        }

        return false;
    }

why use a boolean to specify the pullup value instead of the actual value?

    void begin ()
    {
        pinMode (Pin, pullup);
        butState = digitalRead (Pin);
    }

why not use shorter names and why the m_ prefix?

class Button {
  private:
    int Pin;
    int pullup;

    int butState;

    unsigned long       msec0;
    const unsigned long MsecDelay = 50;

  public:
    Button (int pin, int pullup) : Pin { pin }, pullup { pullup }
    { }

@OmeJozz

I'm trying to un-learn some of these.

Bjarne Stroustrup warned coders to only use the features of the language necessary for their needs. Of course knowing the syntax of a language does not mean you know how to use the language.

The Elements of Programming Style is one of the best, certainly the shortest (181 pgs) books explaining how to write good code. It indentifies the faults and weaknesses of textbook programs and explains how to correct them.