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
+98 −46
Open

[P109] add button pin mode #5066

Show file tree
Hide file tree
Changes from 3 commits
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
45 changes: 41 additions & 4 deletions src/_P109_ThermOLED.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,17 +171,41 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
}

addFormPinSelect(PinSelectPurpose::Generic_input, F("Button left/down"), F("taskdevicepin1"), CONFIG_PIN1);
Copy link
Member

Choose a reason for hiding this comment

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

I assume these are optional selections, right?

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);

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));
}

{
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to cast an int to an int.

Copy link
Member

Choose a reason for hiding this comment

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

also please use the NR_ELEMENTS(optionValues5) macro instead of a hardcoded "8" here.

addUnit('m');
}

{
const __FlashStringHelper *options4[] = { F("0.2"), F("0.5"), F("1") };
const int optionValues4[] = { 2, 5, 10 };
addFormSelector(F("Hysteresis"), F("hyst"), 3, options4, optionValues4, static_cast<int>(P109_CONFIG_HYSTERESIS * 10.0f));
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to cast an int to an int.

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to cast an int to an int.

addUnit('m');
}

{
Expand All @@ -195,6 +219,12 @@ 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));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to cast an int to an int.

}

success = true;
break;
Expand All @@ -208,11 +238,18 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
P109_CONFIG_CONTRAST = getFormItemInt(F("contrast"));
P109_CONFIG_RELAYPIN = getFormItemInt(F("heatrelay"));
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_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")));
P109_FLAGS = lSettings;

{
Expand Down
31 changes: 17 additions & 14 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,11 +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;
_setpointTimeout = P109_CONFIG_SETPOINT_DELAY - P109_SETPOINT_OFFSET;
_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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this change of name removes any confusion...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I thought this: by default (including in this plugin) the buttons are IntPullUp. Therefore, if the button is PullUp or PullDown it is NO_INTPULLUP. Of course I accept your suggestions )

Copy link
Member

Choose a reason for hiding this comment

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

It is often a bad idea to name parameters to do some "inverse" action as it will often lead to bugs.
Also "NO_xxx" suggests it isn't available.

So maybe call them "INT_PULL_DOWN" or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then let it be INT_PULL_UP (and the function logic is reverse to the current one). OK?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but then you must make sure the defaults are dealt with.
When you don't set the defaults, the value should be assumed 0.

There is a specific PLUGIN_CALL_.... for loading the defaults which will only be called when you set the plugin to a task.

Copy link
Contributor

Choose a reason for hiding this comment

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

here is a specific PLUGIN_CALL_.... for loading the defaults

case PLUGIN_SET_DEFAULTS: 😉

_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 @@ -120,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), INPUT_PULLUP);
pinMode(PIN(pin), _buttonNoIntPullup[pin] ? INPUT : INPUT_PULLUP);
}
}

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

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

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

if (validGpio(CONFIG_PIN2) && !digitalRead(CONFIG_PIN2)) {
if (validGpio(CONFIG_PIN2) && (_buttonNoIntPullup[1] ? digitalRead(CONFIG_PIN2) : !digitalRead(CONFIG_PIN2))) {
current_time = millis();

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

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

if (_buttons[2] + P109_BUTTON_DEBOUNCE_TIME_MS < current_time) {
Expand Down Expand Up @@ -437,10 +440,10 @@ void P109_data_struct::actionLeft(struct EventStruct *event) {
break;
}
case 2: { // manual on mode, timer dec
UserVar.setFloat(event->TaskIndex, 3, UserVar[event->BaseVarIndex + 3] - P109_BUTTON_DEBOUNCE_TIME_MS);
UserVar.setFloat(event->TaskIndex, 3, UserVar[event->BaseVarIndex + 3] - P109_CONFIG_STEP_TIMEOUT * 60);

if (UserVar[event->BaseVarIndex + 3] < 0) {
UserVar.setFloat(event->TaskIndex, 3, 5400);
UserVar.setFloat(event->TaskIndex, 3, P109_CONFIG_MAX_TIMEOUT * 60);
}
_prev_timeout = P109_TIMEOUT_STATE_UNSET;
break;
Expand All @@ -461,9 +464,9 @@ void P109_data_struct::actionRight(struct EventStruct *event) {
break;
}
case 2: { // manual on mode, timer dec
UserVar.setFloat(event->TaskIndex, 3, UserVar[event->BaseVarIndex + 3] + P109_BUTTON_DEBOUNCE_TIME_MS);
UserVar.setFloat(event->TaskIndex, 3, UserVar[event->BaseVarIndex + 3] + P109_CONFIG_STEP_TIMEOUT * 60);

if (UserVar[event->BaseVarIndex + 3] > 5400) {
if (UserVar[event->BaseVarIndex + 3] > P109_CONFIG_MAX_TIMEOUT * 60) {
UserVar.setFloat(event->TaskIndex, 3, 60);
}
_prev_timeout = P109_TIMEOUT_STATE_UNSET;
Expand All @@ -482,7 +485,7 @@ void P109_data_struct::actionMode(struct EventStruct *event) {
break;
}
case 1: { // auto mode, next
setMode(F("m"), F("5"));
setMode(F("m"), toString(P109_CONFIG_TIMEOUT));
break;
}
case 2: { // manual on mode, next
Expand Down
38 changes: 25 additions & 13 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 @@ -40,7 +40,6 @@ const char flameimg[] PROGMEM = {
# define P109_TIMEOUT_STATE_UNSET 32768.0f
# define P109_HEATING_STATE_UNSET 255
# define P109_MODE_STATE_UNSET 255
# define P109_MODE_STATE_INITIAL 1
# define P109_BUTTON_DEBOUNCE_TIME_MS 300
# define P109_DEFAULT_SETPOINT_DELAY (5 + 1) // Seconds + 1 before the relay state is changed after the setpoint is changed
# define P109_DELAY_BETWEEN_SAVE 30000 // 30 seconds
Expand All @@ -50,18 +49,29 @@ const char flameimg[] PROGMEM = {
# define P109_CONFIG_DISPLAYTYPE PCONFIG(2)
# define P109_CONFIG_CONTRAST PCONFIG(3)
# define P109_CONFIG_RELAYPIN PCONFIG(4)
# define P109_CONFIG_SETPOINT_DELAY PCONFIG(5)
# define P109_CONFIG_TIMEOUT PCONFIG(5)
# define P109_CONFIG_MAX_TIMEOUT PCONFIG(6)
# define P109_CONFIG_STEP_TIMEOUT PCONFIG(7)
# define P109_CONFIG_HYSTERESIS PCONFIG_FLOAT(0)
# define P109_CONFIG_SETPOINT_DELAY PCONFIG(8)
# define P109_MODE_STATE_INITIAL PCONFIG(9)
Copy link
Contributor

@tonhuisman tonhuisman Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

PCONFIG() only handles 8 values, 0..7, so using above 7 will cause issues with the next task values. You can use PCONFIG_LONG() in range 0..3, if not already used for something else.


# 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_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)

struct P109_data_struct : public PluginTaskData_base {
P109_data_struct();
Expand Down Expand Up @@ -89,17 +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;
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;
String _last_heater;

String _title;
Expand Down