[P109] add button pin mode #5066

SirWant · 2024-05-31T15:54:09Z

Added pin mode selection for pulled down button modules.

TD-er · 2024-05-31T18:02:13Z

src/_P109_ThermOLED.ino

@@ -171,13 +171,15 @@ 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("invertbutton1"), P109_GET_BUTTON1_INVERT);


Hmm this is quite confusing.
Unchecked is pulled up?

What do you want to tell the user? Is it about the pull-up or invert up/down button?

Renamed variables.

TD-er · 2024-05-31T18:03:37Z

src/_P109_ThermOLED.ino

@@ -171,13 +171,15 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
      }

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


I assume these are optional selections, right?

tonhuisman · 2024-05-31T19:00:39Z

src/src/PluginStructs/P109_data_struct.cpp

  _varIndex        = event->BaseVarIndex;
  _relaypin        = P109_CONFIG_RELAYPIN;
  _relayInverted   = P109_GET_RELAY_INVERT;
+  _buttonInverted[0]   = P109_GET_BUTTON1_INVERT;
+  _buttonInverted[1]   = P109_GET_BUTTON2_INVERT;
+  _buttonInverted[2]   = P109_GET_BUTTON3_INVERT;
  _setpointTimeout = P109_CONFIG_SETPOINT_DELAY - P109_SETPOINT_OFFSET;


Hm, you don't seem to have Uncrustify installed or enabled, for formatting the source code. that does a lot for readability.
Docs on how to install & enable

tonhuisman · 2024-05-31T19:22:58Z

Can you also take care of updating the documentation? New screenshots (switch to Light mode please, to keep it all consistent with other documentation) and adding the new settings options and what they are supposed to do.

SirWant · 2024-06-06T08:17:05Z

Can you also take care of updating the documentation? New screenshots (switch to Light mode please, to keep it all consistent with other documentation) and adding the new settings options and what they are supposed to do.

This is the most difficult thing for me - I don’t speak (or write) English...

tonhuisman · 2024-06-06T08:43:12Z

src/src/PluginStructs/P109_data_struct.h

+# define P109_CONFIG_SETPOINT_DELAY     PCONFIG(8)
+# define P109_MODE_STATE_INITIAL        PCONFIG(9)


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.

tonhuisman · 2024-06-06T08:43:56Z

src/_P109_ThermOLED.ino

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


No need to cast an int to an int.

tonhuisman · 2024-06-06T08:44:29Z

src/_P109_ThermOLED.ino

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


No need to cast an int to an int.

tonhuisman · 2024-06-06T08:44:50Z

src/_P109_ThermOLED.ino

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


No need to cast an int to an int.

tonhuisman · 2024-06-06T08:44:55Z

src/_P109_ThermOLED.ino

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


No need to cast an int to an int.

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

tonhuisman · 2024-06-06T08:47:25Z

src/src/PluginStructs/P109_data_struct.cpp

+  _buttonNoIntPullup[0] = P109_GET_BUTTON1_NO_INTPULLUP;
+  _buttonNoIntPullup[1] = P109_GET_BUTTON2_NO_INTPULLUP;
+  _buttonNoIntPullup[2] = P109_GET_BUTTON3_NO_INTPULLUP;


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

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 )

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?

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

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.

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

case PLUGIN_SET_DEFAULTS: 😉

tonhuisman · 2024-06-07T07:54:41Z

src/_P109_ThermOLED.ino

@@ -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/


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

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

TD-er · 2024-06-07T10:21:37Z

src/src/PluginStructs/P109_data_struct.cpp

@@ -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))) {


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

So it seems like it is, we can only rename the checkbox to “Apply IntPullUp”, for example.

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?

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

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"

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.

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. :)

TD-er · 2024-06-07T10:23:45Z

src/_P109_ThermOLED.ino

+      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


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.

SirWant added 2 commits May 31, 2024 14:26

Add Button pin mode

4d93059

Add Button pin mode

62e13db

TD-er reviewed May 31, 2024

View reviewed changes

tonhuisman reviewed May 31, 2024

View reviewed changes

formatted, renamed variables, moved some variables to settings

4517850

tonhuisman suggested changes Jun 6, 2024

View reviewed changes

fixes, renamed variables

5b41b4d

tonhuisman reviewed Jun 7, 2024

View reviewed changes

TD-er reviewed Jun 7, 2024

View reviewed changes

Merge branch 'mega' into P109-Add-Button-pin-mode

2c04e1f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[P109] add button pin mode #5066

[P109] add button pin mode #5066

SirWant commented May 31, 2024 •

edited

Loading

TD-er May 31, 2024

SirWant Jun 6, 2024

TD-er May 31, 2024

tonhuisman May 31, 2024

SirWant Jun 6, 2024

tonhuisman commented May 31, 2024

SirWant commented Jun 6, 2024

tonhuisman Jun 6, 2024 •

edited

Loading

tonhuisman Jun 6, 2024

tonhuisman Jun 6, 2024

tonhuisman Jun 6, 2024

tonhuisman Jun 6, 2024

TD-er Jun 6, 2024

tonhuisman Jun 6, 2024

SirWant Jun 6, 2024

TD-er Jun 6, 2024

SirWant Jun 6, 2024

TD-er Jun 6, 2024

tonhuisman Jun 6, 2024

tonhuisman Jun 7, 2024

SirWant Jun 7, 2024

TD-er Jun 7, 2024

SirWant Jun 7, 2024

TD-er Jun 7, 2024

SirWant Jun 7, 2024

TD-er Jun 7, 2024

SirWant Jun 7, 2024

TD-er Jun 7, 2024

TD-er Jun 7, 2024

		@@ -171,13 +171,15 @@ boolean Plugin_109(uint8_t function, struct EventStruct *event, String& string)
		}

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

		# define P109_CONFIG_SETPOINT_DELAY PCONFIG(8)
		# define P109_MODE_STATE_INITIAL PCONFIG(9)

[P109] add button pin mode #5066

Are you sure you want to change the base?

[P109] add button pin mode #5066

Conversation

SirWant commented May 31, 2024 • edited Loading

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

tonhuisman commented May 31, 2024

SirWant commented Jun 6, 2024

tonhuisman Jun 6, 2024 • edited Loading

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

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

SirWant commented May 31, 2024 •

edited

Loading

tonhuisman Jun 6, 2024 •

edited

Loading