Feature/button control #9

GrzegorzWojnarowicz · 2022-03-18T20:29:04Z

Keep that in mind

add Button class

alternative version of button_pressed method

fix button_pressed method

Selvan66 · 2022-03-19T00:56:53Z

lib/button-control/inc/button.hpp

+#pragma once
+
+#include <Arduino.h>
+const unsigned long debounce_delay = 75;


From line 4-14: Hide this constants/enums to class because, this values are integrated with the class and should not be accessible for other when include this file

Selvan66 · 2022-03-19T00:58:12Z

lib/button-control/src/button.cpp

+ Press output = NOT_PRESSED;
+ int readState = digitalRead(pinNum);
+
+ if (readState == high_button_state && (button_active == false))


For line 34 and 39: Why used high_button_state when you can use just HIGH it's more cleaner

Selvan66 · 2022-03-19T01:11:02Z

lib/button-control/src/button.cpp

+ unsigned long time_diff = release_time - press_time < 0 ? 0 : release_time - press_time; 
+ output = time_diff > long_press ? PRESSED_LONG :
+ ((time_diff > medium_press) && (time_diff <= long_press)) ? PRESSED_MEDIUM :
+ ((time_diff > 0) && (time_diff <= medium_press)) ? PRESSED_SHORT : NOT_PRESSED;


You can split this to if but i don't see too much problem with this if its stay in that form.
But what i would change for sure you can cut this if because if time_diff > long_press is false it's for sure time_diff <= long_press so you don't need to write it in else statement. Same to medium_press.

Yes, it looks really complicated and too long.

You can create a separate private methods for some if statements, for example: check_long(), check_short(), check_medium() etc.

xHeler

Clean code

xHeler · 2022-03-19T23:23:08Z

lib/button-control/inc/button.hpp

+const unsigned long medium_press = 1000;
+const unsigned long long_press = 5000;
+
+enum Press {


Press what ? Maybe change to -> PressState or smth give more information

xHeler · 2022-03-19T23:25:45Z

lib/button-control/inc/button.hpp

+ short pinNum;
+ unsigned long press_time;
+ unsigned long release_time;
+ bool button_active;
+ unsigned long time_diff;


Two different name convention. Ones you use camelCase in the other hand you use snake_case

xHeler · 2022-03-19T23:26:31Z

lib/button-control/src/button.cpp

+Button::~Button()
+{
+}


Button::~Button() {} - in one line looks more sexy

delipl · 2022-03-19T23:24:39Z

lib/button-control/inc/button.hpp

+
+#include <Arduino.h>
+const unsigned long debounce_delay = 75;
+const int high_button_state = HIGH;


unnecessary use just HIGH

delipl · 2022-03-19T23:26:07Z

lib/button-control/inc/button.hpp

+ PRESSED_LONG = 3
+};
+
+class Button


Please move the public things just under class name.
It is easier to read and use then the public methods are just under the class name.

delipl · 2022-03-19T23:27:46Z

lib/button-control/inc/button.hpp

+const unsigned long medium_press = 1000;
+const unsigned long long_press = 5000;
+
+enum Press {


please add unknown state :D

you can also use enum class

If the values of enum are in sequence you don't have to assign the value.

delipl · 2022-03-19T23:31:02Z

lib/button-control/inc/button.hpp

+class Button
+{
+private:
+ short pinNum;


There are two types of the cases. pinNum and press_time . Chose one and implement to others :D camelCase or snake_case

Use explicite types of ints. https://en.cppreference.com/w/cpp/types/integer . The pinNum can be uint8_t - unsigned int 8 bit. 0 - 255.

delipl · 2022-03-19T23:37:43Z

lib/button-control/src/button.cpp

+ unsigned long time_diff = release_time - press_time < 0 ? 0 : release_time - press_time; 
+ output = time_diff > long_press ? PRESSED_LONG :
+ ((time_diff > medium_press) && (time_diff <= long_press)) ? PRESSED_MEDIUM :
+ ((time_diff > 0) && (time_diff <= medium_press)) ? PRESSED_SHORT : NOT_PRESSED;


Yes, it looks really complicated and too long.

delipl · 2022-03-19T23:38:35Z

lib/button-control/inc/button.hpp

+ unsigned long time_diff;
+public:
+ Button(int buttonNum);
+ ~Button();


If the destructor is empty just don't use it ;)

delipl · 2022-03-19T23:42:34Z

lib/button-control/src/button.cpp

+ unsigned long time_diff = release_time - press_time < 0 ? 0 : release_time - press_time; 
+ output = time_diff > long_press ? PRESSED_LONG :
+ ((time_diff > medium_press) && (time_diff <= long_press)) ? PRESSED_MEDIUM :
+ ((time_diff > 0) && (time_diff <= medium_press)) ? PRESSED_SHORT : NOT_PRESSED;


You can create a separate private methods for some if statements, for example: check_long(), check_short(), check_medium() etc.

change pressed_button method from millis to interrupts change from class Button to namespace ns_button

Selvan66

I liked the solution in the class more than in the namespace, but if others think it might stay that way, I have no problem with that.

Just write some example program to use this code.

GrzegorzWojnarowicz · 2022-03-26T13:26:07Z

It is in namespace now just because interrupts had problems with classes. The problem is that when interrupt happens it calls a function (in my class it was a method, and it wouldn't know which instance of this method to call). I've seen that someone told that it may be done by static methods and address them using switch case, but he said that it's not a perfect solution.

xHeler · 2022-03-26T14:05:54Z

lib/button-control/inc/button.hpp

+ const unsigned long medium_press = 1000;
+ const unsigned long long_press = 5000;
+
+ enum Button_state {


Why not put them all and create button class ?

Because we need static functions as callbacks to the interrupts.
I think that the buttons have to be something static.

I looked at it and it turns out that you can use lambda expressions in attachInterrupt(), you only need to attach the library #include <FunctionalInterrupt.h> from esp

xHeler · 2022-03-26T14:06:27Z

I'd like to have examples directory with simple example of usage your buttons.
And please create README.md with information about library.

delipl

Add this pattern to function with argument. Please split this to .hpp and .cpp

if (press_time[i] != 0)
        {
            if ((millis() - press_time[i]) > debounce_delay)
            {
                release_time[i] = millis();
            }
        }else{
            if ((millis() - release_time[i]) > debounce_delay)
            {
                press_time[i] = millis();
            }
        }
    ```

change button_pressed function into set_current_state add get_current_state function use as state-machine using get_current_state function

add README file with class description and usage example

GrzegorzWojnarowicz added 3 commits March 12, 2022 04:01

#2 Hardware button handler

d38737f

add Button class

#2 Hardware button handler

53ca006

alternative version of button_pressed method

#2 Hardware button handler

eefe708

fix button_pressed method

Selvan66 requested changes Mar 19, 2022

View reviewed changes

xHeler reviewed Mar 19, 2022

View reviewed changes

delipl requested changes Mar 19, 2022

View reviewed changes

#2 Hardware button handler

80a93d9

change pressed_button method from millis to interrupts change from class Button to namespace ns_button

Selvan66 reviewed Mar 26, 2022

View reviewed changes

xHeler reviewed Mar 26, 2022

View reviewed changes

delipl requested changes Mar 26, 2022

View reviewed changes

GrzegorzWojnarowicz added 2 commits April 3, 2022 23:54

#2 Hardware button handler

a7e1155

change button_pressed function into set_current_state add get_current_state function use as state-machine using get_current_state function

#2 Hardware button handler

415441a

add README file with class description and usage example

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/button control #9

Feature/button control #9

GrzegorzWojnarowicz commented Mar 18, 2022 •

edited

Selvan66 Mar 19, 2022

Selvan66 Mar 19, 2022

Selvan66 Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

xHeler left a comment

xHeler Mar 19, 2022

xHeler Mar 19, 2022

xHeler Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

delipl Mar 19, 2022

Selvan66 left a comment

GrzegorzWojnarowicz commented Mar 26, 2022

xHeler Mar 26, 2022

delipl Mar 26, 2022

Selvan66 Mar 27, 2022

xHeler commented Mar 26, 2022

delipl left a comment •

edited

Feature/button control #9

Are you sure you want to change the base?

Feature/button control #9

Conversation

GrzegorzWojnarowicz commented Mar 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xHeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Selvan66 left a comment

Choose a reason for hiding this comment

GrzegorzWojnarowicz commented Mar 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xHeler commented Mar 26, 2022

delipl left a comment • edited

Choose a reason for hiding this comment

GrzegorzWojnarowicz commented Mar 18, 2022 •

edited

delipl left a comment •

edited