Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[P109] add button pin mode #5066

Open
wants to merge 5 commits into
base: mega
Choose a base branch
from
Open

[P109] add button pin mode #5066

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d93059
Add Button pin mode
SirWant May 31, 2024
62e13db
Add Button pin mode
SirWant May 31, 2024
4517850
formatted, renamed variables, moved some variables to settings
SirWant Jun 6, 2024
5b41b4d
fixes, renamed variables
SirWant Jun 7, 2024
2c04e1f
Merge branch 'mega' into P109-Add-Button-pin-mode
TD-er Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 31 additions & 31 deletions src/_P109_ThermOLED.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
- /control?cmd=thermo,mode,0 Switch heating off, absolutely do nothing until further notice

------------------------------------------------------------------------------------------
Copyleft Nagy Sándor 2018 - https://bitekmindenhol.blog.hu/
Copyleft Nagy Sandor 2018 - https://bitekmindenhol.blog.hu/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think that should have been changed...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too, apparently it happened on the clipboard...

------------------------------------------------------------------------------------------
2022-12-08 tonhuisman: Add Relay invert state option, reorder config option Contrast
Add setpoint delay option, switch relay after delay seconds
Expand Down Expand Up @@ -138,16 +138,16 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
# ifndef LIMIT_BUILD_SIZE
case PLUGIN_WEBFORM_SHOW_GPIO_DESCR:
{
const char* separator = event->String1.c_str();
const char *separator = event->String1.c_str();
string = strformat(
F("Btn L: %s%sBtn R: %s%sBtn M: %s%sRelay: %s"),
formatGpioLabel(CONFIG_PIN1, false).c_str(),
separator,
formatGpioLabel(CONFIG_PIN2, false).c_str(),
separator,
formatGpioLabel(CONFIG_PIN3, false).c_str(),
separator,
formatGpioLabel(P109_CONFIG_RELAYPIN, false).c_str());
formatGpioLabel(CONFIG_PIN1, false).c_str(),
separator,
formatGpioLabel(CONFIG_PIN2, false).c_str(),
separator,
formatGpioLabel(CONFIG_PIN3, false).c_str(),
separator,
formatGpioLabel(P109_CONFIG_RELAYPIN, false).c_str());
success = true;
break;
}
Expand All @@ -170,41 +170,41 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
}
}

addFormPinSelect(PinSelectPurpose::Generic_input, F("Button left/down"), F("taskdevicepin1"), CONFIG_PIN1);
addFormCheckBox(F("Button left/down Pull mode (0=IntPullUp, 1=Input)"), F("NoIntPullupButton1"), P109_GET_BUTTON1_NO_INTPULLUP);
addFormPinSelect(PinSelectPurpose::Generic_input, F("Button right/up"), F("taskdevicepin2"), CONFIG_PIN2);
addFormCheckBox(F("Button right/up Pull mode (0=IntPullUp, 1=Input)"), F("NoIntPullupButton2"), P109_GET_BUTTON2_NO_INTPULLUP);
addFormPinSelect(PinSelectPurpose::Generic_input, F("Button mode"), F("taskdevicepin3"), CONFIG_PIN3);
addFormCheckBox(F("Button Mode Pull mode (0=IntPullUp, 1=Input)"), F("NoIntPullupButton3"), P109_GET_BUTTON3_NO_INTPULLUP);
addFormPinSelect(PinSelectPurpose::Generic_output, F("Relay"), F("heatrelay"), P109_CONFIG_RELAYPIN);
addFormPinSelect(PinSelectPurpose::Generic_input, F("Button left/down"), F("taskdevicepin1"), CONFIG_PIN1);
addFormCheckBox(F("Button left/down Pull mode (1=IntPullUp, 0=Input)"), F("IntPullupButton1"), P109_GET_BUTTON1_INT_PULL_UP == 0); // Inverted
addFormPinSelect(PinSelectPurpose::Generic_input, F("Button right/up"), F("taskdevicepin2"), CONFIG_PIN2);
addFormCheckBox(F("Button right/up Pull mode (1=IntPullUp, 0=Input)"), F("IntPullupButton2"), P109_GET_BUTTON2_INT_PULL_UP == 0); // Inverted
addFormPinSelect(PinSelectPurpose::Generic_input, F("Button mode"), F("taskdevicepin3"), CONFIG_PIN3);
addFormCheckBox(F("Button Mode Pull mode (1=IntPullUp, 0=Input)"), F("IntPullupButton3"), P109_GET_BUTTON3_INT_PULL_UP == 0); // Inverted

addFormPinSelect(PinSelectPurpose::Generic_output, F("Relay"), F("heatrelay"), P109_CONFIG_RELAYPIN);

addFormCheckBox(F("Invert relay-state (0=on, 1=off)"), F("invertrelay"), P109_GET_RELAY_INVERT);

{
const __FlashStringHelper *options4[] = { F("0.2"), F("0.5"), F("1"), F("1.5"), F("2"), F("3"), F("4"), F("5") };
const int optionValues4[] = { 2, 5, 10, 15, 20, 30, 40, 50 };
addFormSelector(F("Hysteresis"), F("hyst"), 8, options4, optionValues4, static_cast<int>(P109_CONFIG_HYSTERESIS * 10.0f));
addFormSelector(F("Hysteresis"), F("hyst"), NR_ELEMENTS(optionValues4), options4, optionValues4, static_cast<int>(P109_CONFIG_HYSTERESIS * 10.0f));
}

{
const __FlashStringHelper *options5[] = { F("5"), F("10"), F("15"), F("20"), F("30"), F("45"), F("60"), F("90") };
const int optionValues5[] = { 5, 10, 15, 20, 30, 45, 60, 90 };
addFormSelector(F("Timeout default"), F("timeout"), 8, options5, optionValues5, static_cast<int>(P109_CONFIG_TIMEOUT));
addFormSelector(F("Timeout default"), F("timeout"), NR_ELEMENTS(optionValues5), options5, optionValues5, P109_CONFIG_TIMEOUT);
addUnit('m');
}

{
const __FlashStringHelper *options6[] = { F("1.5"), F("2"), F("2.5"), F("3"), F("3.5"), F("4"), F("4.5"), F("5") };
const int optionValues6[] = { 90, 120, 150, 180, 210, 240, 270, 300 };
addFormSelector(F("Timeout MAX"), F("timeout_max"), 8, options6, optionValues6, static_cast<int>(P109_CONFIG_MAX_TIMEOUT));
addFormSelector(F("Timeout MAX"), F("timeout_max"), NR_ELEMENTS(optionValues6), options6, optionValues6, P109_CONFIG_MAX_TIMEOUT);
addUnit('h');
}

{
const __FlashStringHelper *options7[] = { F("5"), F("10"), F("15"), F("20"), F("25"), F("30") };
const int optionValues7[] = { 5, 10, 15, 20, 25, 30 };
addFormSelector(F("Timeout step"), F("timeout_step"), 6, options7, optionValues7, static_cast<int>(P109_CONFIG_STEP_TIMEOUT));
addFormSelector(F("Timeout step"), F("timeout_step"), NR_ELEMENTS(optionValues7), options7, optionValues7, static_cast<int>(P109_CONFIG_STEP_TIMEOUT));
addUnit('m');
}

Expand All @@ -219,11 +219,11 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
addFormNumericBox(F("Delay on setpoint change"), F("setpdelay"), P109_CONFIG_SETPOINT_DELAY - P109_SETPOINT_OFFSET, 1, 10);
addUnit('s');
}

{
const __FlashStringHelper *options8[] = { F("Auto"), F("Off"), F("Manual") };
const int optionValues8[] = { 1, 0, 2 };
addFormSelector(F("Default mode"), F("mode"), 3, options8, optionValues8, static_cast<int>(P109_MODE_STATE_INITIAL));
addFormSelector(F("Default mode"), F("mode"), NR_ELEMENTS(optionValues8), options8, optionValues8, static_cast<int>(P109_MODE_STATE_INITIAL));
}

success = true;
Expand All @@ -240,16 +240,16 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
P109_CONFIG_HYSTERESIS = (getFormItemInt(F("hyst")) / 10.0f);
P109_CONFIG_TIMEOUT = getFormItemInt(F("timeout"));
P109_CONFIG_MAX_TIMEOUT = getFormItemInt(F("timeout_max"));
P109_CONFIG_STEP_TIMEOUT = getFormItemInt(F("timeout_step"));
P109_CONFIG_SETPOINT_DELAY = getFormItemInt(F("setpdelay")) + P109_SETPOINT_OFFSET;
P109_CONFIG_STEP_TIMEOUT = getFormItemInt(F("timeout_step"));
P109_MODE_STATE_INITIAL = getFormItemInt(F("mode"));
uint32_t lSettings = 0u;
bitWrite(lSettings, P109_FLAG_TASKNAME_IN_TITLE, isFormItemChecked(F("ptask")));
bitWrite(lSettings, P109_FLAG_ALTERNATE_HEADER, !isFormItemChecked(F("palt"))); // Inverted
bitWrite(lSettings, P109_FLAG_RELAY_INVERT, isFormItemChecked(F("invertrelay")));
bitWrite(lSettings, P109_FLAG_BUTTON1_NO_INTPULLUP, isFormItemChecked(F("NoIntPullupButton1")));
bitWrite(lSettings, P109_FLAG_BUTTON2_NO_INTPULLUP, isFormItemChecked(F("NoIntPullupButton2")));
bitWrite(lSettings, P109_FLAG_BUTTON3_NO_INTPULLUP, isFormItemChecked(F("NoIntPullupButton3")));
bitWrite(lSettings, P109_FLAG_TASKNAME_IN_TITLE, isFormItemChecked(F("ptask")));
bitWrite(lSettings, P109_FLAG_ALTERNATE_HEADER, !isFormItemChecked(F("palt"))); // Inverted
bitWrite(lSettings, P109_FLAG_RELAY_INVERT, isFormItemChecked(F("invertrelay")));
bitWrite(lSettings, P109_FLAG_BUTTON1_INT_PULL_UP, !isFormItemChecked(F("IntPullupButton1"))); // Inverted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "inverted" stuff is exactly what I was afraid of that would happen.
This is extremely error prone and a nightmare to debug or verify.

bitWrite(lSettings, P109_FLAG_BUTTON2_INT_PULL_UP, !isFormItemChecked(F("IntPullupButton2"))); // Inverted
bitWrite(lSettings, P109_FLAG_BUTTON3_INT_PULL_UP, !isFormItemChecked(F("IntPullupButton3"))); // Inverted
P109_FLAGS = lSettings;

{
Expand Down
24 changes: 12 additions & 12 deletions src/src/PluginStructs/P109_data_struct.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,14 @@ bool P109_data_struct::plugin_init(struct EventStruct *event) {
delete _display;
_display = nullptr;
}
_taskIndex = event->TaskIndex;
_varIndex = event->BaseVarIndex;
_relaypin = P109_CONFIG_RELAYPIN;
_relayInverted = P109_GET_RELAY_INVERT;
_buttonNoIntPullup[0] = P109_GET_BUTTON1_NO_INTPULLUP;
_buttonNoIntPullup[1] = P109_GET_BUTTON2_NO_INTPULLUP;
_buttonNoIntPullup[2] = P109_GET_BUTTON3_NO_INTPULLUP;
_setpointTimeout = P109_CONFIG_SETPOINT_DELAY - P109_SETPOINT_OFFSET;
_taskIndex = event->TaskIndex;
_varIndex = event->BaseVarIndex;
_relaypin = P109_CONFIG_RELAYPIN;
_relayInverted = P109_GET_RELAY_INVERT;
_buttonIntPullup[0] = !P109_GET_BUTTON1_INT_PULL_UP;
_buttonIntPullup[1] = !P109_GET_BUTTON2_INT_PULL_UP;
_buttonIntPullup[2] = !P109_GET_BUTTON3_INT_PULL_UP;
_setpointTimeout = P109_CONFIG_SETPOINT_DELAY - P109_SETPOINT_OFFSET;

if (P109_CONFIG_DISPLAYTYPE == 1) {
_display = new (std::nothrow) SSD1306Wire(P109_CONFIG_I2CADDRESS, Settings.Pin_i2c_sda, Settings.Pin_i2c_scl);
Expand Down Expand Up @@ -123,7 +123,7 @@ bool P109_data_struct::plugin_init(struct EventStruct *event) {

for (uint8_t pin = 0; pin < 3; ++pin) {
if (validGpio(PIN(pin))) {
pinMode(PIN(pin), _buttonNoIntPullup[pin] ? INPUT : INPUT_PULLUP);
pinMode(PIN(pin), _buttonIntPullup[pin] ? INPUT_PULLUP : INPUT);
}
}

Expand Down Expand Up @@ -201,7 +201,7 @@ bool P109_data_struct::plugin_ten_per_second(struct EventStruct *event) {
if (_initialized) {
uint32_t current_time;

if (validGpio(CONFIG_PIN1) && (_buttonNoIntPullup[0] ? digitalRead(CONFIG_PIN1) : !digitalRead(CONFIG_PIN1))) {
if (validGpio(CONFIG_PIN1) && (_buttonIntPullup[0] ? !digitalRead(CONFIG_PIN1) : digitalRead(CONFIG_PIN1))) {
current_time = millis();

if (_buttons[0] + P109_BUTTON_DEBOUNCE_TIME_MS < current_time) {
Expand All @@ -210,7 +210,7 @@ bool P109_data_struct::plugin_ten_per_second(struct EventStruct *event) {
}
}

if (validGpio(CONFIG_PIN2) && (_buttonNoIntPullup[1] ? digitalRead(CONFIG_PIN2) : !digitalRead(CONFIG_PIN2))) {
if (validGpio(CONFIG_PIN2) && (_buttonIntPullup[1] ? !digitalRead(CONFIG_PIN2) : digitalRead(CONFIG_PIN2))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way how it is used, suggests it is actually just an "invert".
But on the other hand, the flag is used to either set the pin to INPUT_PULLUP or INPUT.
So I find it quite confusing and I think we really should first go back to what you try to ask the user.

Maybe these should not be implied but just offered as a separate setting/checkbox:

  • Enable pull-up
  • Invert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems like it is, we can only rename the checkbox to “Apply IntPullUp”, for example.
2024-6-7 13-57-47

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why so explicit?
It is about the GPIO for the "Mode button", so "Mode button pull-up" would be enough, right?
It is obviously an input signal, so no need to clarify it as it will only confuse people.

N.B. "Button mode" suggests it is some different kind of button, like toggle switch or push button.
"Mode button" suggests you change the mode of something by using this button.
So what is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s right, I propose to rename it “Enable IntPullUp for Button mode”, “Enable IntPullUp for Button right/up”, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not "Mode Button Pullup" ?
It has a checkbox, so it is clear it is an enable/disable option.
It is part of the options for "Mode Button", so "for" is not needed.
And "Int" is also not clear to users as the pull-up option in all other plugins etc. is not using "int" anywhere.

Also it doesn't make sense to have the word "button" first, as I explained before.
Unless "Button mode" is about a special mode for that button. (e.g. different types of buttons, different behavior of that button, etc)
Otherwise if it is a button to access some mode function, then it is the "mode button"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that we are hampered by the subtleties of our languages ) Therefore, I suggest you formulate the final version. But, I don't completely agree about Int. For example, in P001 the checkbox is “Internal PullUp”, and this is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm didn't know that and have to look into this.
If it is indeed using that exact string (and I don't doubt it is, otherwise you wouldn't make this argument), then it should be the same here.
Glad you're being "stubborn" (in a good sense) and try to be as consistent as can be, which is a good habit as a programmer. :)

current_time = millis();

if (_buttons[1] + P109_BUTTON_DEBOUNCE_TIME_MS < current_time) {
Expand All @@ -219,7 +219,7 @@ bool P109_data_struct::plugin_ten_per_second(struct EventStruct *event) {
}
}

if (validGpio(CONFIG_PIN3) && (_buttonNoIntPullup[2] ? digitalRead(CONFIG_PIN3) : !digitalRead(CONFIG_PIN3))) {
if (validGpio(CONFIG_PIN3) && (_buttonIntPullup[2] ? !digitalRead(CONFIG_PIN3) : digitalRead(CONFIG_PIN3))) {
current_time = millis();

if (_buttons[2] + P109_BUTTON_DEBOUNCE_TIME_MS < current_time) {
Expand Down
44 changes: 22 additions & 22 deletions src/src/PluginStructs/P109_data_struct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,27 +51,27 @@ const char flameimg[] PROGMEM = {
# define P109_CONFIG_RELAYPIN PCONFIG(4)
# define P109_CONFIG_TIMEOUT PCONFIG(5)
# define P109_CONFIG_MAX_TIMEOUT PCONFIG(6)
# define P109_CONFIG_STEP_TIMEOUT PCONFIG(7)
# define P109_CONFIG_SETPOINT_DELAY PCONFIG(7)
# define P109_CONFIG_HYSTERESIS PCONFIG_FLOAT(0)
# define P109_CONFIG_SETPOINT_DELAY PCONFIG(8)
# define P109_MODE_STATE_INITIAL PCONFIG(9)
# define P109_CONFIG_STEP_TIMEOUT PCONFIG_LONG(1)
# define P109_MODE_STATE_INITIAL PCONFIG_LONG(2)

# define P109_SETPOINT_OFFSET 1 // 1 second offset, to allow changing unset (0) to default

# define P109_FLAGS PCONFIG_ULONG(0)
# define P109_FLAG_TASKNAME_IN_TITLE 0
# define P109_FLAG_ALTERNATE_HEADER 1
# define P109_FLAG_RELAY_INVERT 2
# define P109_FLAG_BUTTON1_NO_INTPULLUP 3
# define P109_FLAG_BUTTON2_NO_INTPULLUP 4
# define P109_FLAG_BUTTON3_NO_INTPULLUP 5
# define P109_FLAG_BUTTON1_INT_PULL_UP 3
# define P109_FLAG_BUTTON2_INT_PULL_UP 4
# define P109_FLAG_BUTTON3_INT_PULL_UP 5

# define P109_GET_TASKNAME_IN_TITLE bitRead(P109_FLAGS, P109_FLAG_TASKNAME_IN_TITLE)
# define P109_GET_ALTERNATE_HEADER bitRead(P109_FLAGS, P109_FLAG_ALTERNATE_HEADER)
# define P109_GET_RELAY_INVERT bitRead(P109_FLAGS, P109_FLAG_RELAY_INVERT)
# define P109_GET_BUTTON1_NO_INTPULLUP bitRead(P109_FLAGS, P109_FLAG_BUTTON1_NO_INTPULLUP)
# define P109_GET_BUTTON2_NO_INTPULLUP bitRead(P109_FLAGS, P109_FLAG_BUTTON2_NO_INTPULLUP)
# define P109_GET_BUTTON3_NO_INTPULLUP bitRead(P109_FLAGS, P109_FLAG_BUTTON3_NO_INTPULLUP)
# define P109_GET_BUTTON1_INT_PULL_UP bitRead(P109_FLAGS, P109_FLAG_BUTTON1_INT_PULL_UP)
# define P109_GET_BUTTON2_INT_PULL_UP bitRead(P109_FLAGS, P109_FLAG_BUTTON2_INT_PULL_UP)
# define P109_GET_BUTTON3_INT_PULL_UP bitRead(P109_FLAGS, P109_FLAG_BUTTON3_INT_PULL_UP)

struct P109_data_struct : public PluginTaskData_base {
P109_data_struct();
Expand Down Expand Up @@ -99,19 +99,19 @@ struct P109_data_struct : public PluginTaskData_base {
float _prev_timeout = P109_TIMEOUT_STATE_UNSET;
float _save_setpoint = P109_SETPOINT_STATE_UNSET;

int8_t _lastWiFiState = P109_WIFI_STATE_UNSET;
uint8_t _prev_heating = P109_HEATING_STATE_UNSET;
uint8_t _prev_mode = P109_MODE_STATE_UNSET;
uint8_t _taskIndex = 0;
uint8_t _varIndex = 0;
uint8_t _changed = 0;
uint8_t _saveneeded = 0;
uint8_t _setpointDelay = 0;
uint8_t _setpointTimeout = 0;
int8_t _relaypin = -1;
bool _relayInverted = false;
bool _buttonNoIntPullup[3] = { false, false, false };
uint8_t _timeout = 15;
int8_t _lastWiFiState = P109_WIFI_STATE_UNSET;
uint8_t _prev_heating = P109_HEATING_STATE_UNSET;
uint8_t _prev_mode = P109_MODE_STATE_UNSET;
uint8_t _taskIndex = 0;
uint8_t _varIndex = 0;
uint8_t _changed = 0;
uint8_t _saveneeded = 0;
uint8_t _setpointDelay = 0;
uint8_t _setpointTimeout = 0;
int8_t _relaypin = -1;
bool _relayInverted = false;
bool _buttonIntPullup[3] = { true, true, true };
uint8_t _timeout = 15;
String _last_heater;

String _title;
Expand Down