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.

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

[BUG] Longpress in Switch input - Switch #4862

Open
iz8mbw opened this issue Oct 29, 2023 · 25 comments
Open

[BUG] Longpress in Switch input - Switch #4862

iz8mbw opened this issue Oct 29, 2023 · 25 comments
Labels
Type: Bug Considered a bug

Comments

@iz8mbw
Copy link
Contributor

iz8mbw commented Oct 29, 2023

Hello.
As I described here I notices that EVENT 10 (the long press event) is always generated also if the press duration of push button is less than what is configured in "Longpress min. interval (ms)".
Example: I have Long press interval set to 600 ms but if I press very fast I have two EVENTs, first one is "1" and just after I have "10".
So, EVENT 10 (longpress) is generated also if I press the push button for a time less than 600 ms.

More, EVENT 0 (or 1) is sent anyway/always just before the EVENT 10 (or 11) during a Lonpress.

In my opinion the Push Button in Switch input should works like this in "chronological" order IF Long press feature is NOT enabled:

  1. Push Button pressed, so electrical change on GPIO to 1 (or 0)
  2. Immediately generate EVENT 1 (or 0)

since in this case the Long press feature is NOT enabled in the configuration of the "Switch input" we don't care of the button release (or button release time) so to be more fast the EVENT should be generated as soon as possible, so do NOT wait for button release to generate the EVENT 1 (or 0).

Instead IF the Longpress feature is enabled in the configuration of the "Switch input", we MUST take care of the release of the button.
So, IF the button is pressed for a time LESS than Logpress Interval, works like that:

  1. Push Button pressed, so electrical change on GPIO to 1 (or 0)
  2. No EVENT done yet by ESPEasy since it MUST wait for button release
  3. Push Button released BEFORE the Logpress Interval, so electrical change on GPIO to 0 (or 1)
  4. Immediately generate EVENT 0 (or 1) [EVENT 0 or 1 since we are in the condition of Longpress = NO]

Instead, IF the button is pressed for a time EQUAL or GREATER than Logpress Interval, works like that:

  1. Push Button pressed, so electrical change on GPIO to 1 (or 0)
  2. No EVENT done yet by ESPEasy since it MUST wait for button release
  3. Push Button released AFTER the Logpress Interval, so electrical change on GPIO to 0 (or 1)
  4. Immediately generate EVENT 10 (or 11) [EVENT 10 or 11 since we are in the condition of Longpress = YES]

now with Longpress enabled and Push Button pressed for LESS than Logpress Interval it works like that:

  1. Push Button pressed for LESS than Logpress Interval, so electrical change on GPIO to 1 (or 0)
  2. EVENT 1 (or 0) generated by ESPEasy AND EVENT 10 (or 11) generated by ESPEasy

How is it now, the Logpress feature is not usable.
Similar Issue without a solution: #2696

Thanks.

@chromoxdor
Copy link
Contributor

chromoxdor commented Oct 29, 2023

Example: I have Long press interval set to 600 ms but if I press very fast I have two EVENTs, first one is "1" and just after I have "10".
So, EVENT 10 (longpress) is generated also if I press the push button for a time less than 600 ms.

That is a normal behaviour..
If for example "longpress" is set to "active on HIGH" and you have enabled the internal pullup and your button therefore switches on LOW, every time you release the button the state becomes HIGH and this triggers eventually a "longpress"

In general there could be an option for longpress like "disable/enable trigger shortpress before longpress"

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Oct 29, 2023

In my easy scenario I have a push button connected to a Input GPIO with a pull-down resistor (so GPIO is connected to GND via the resistor).
So default is LOW.
When I press the push button I provide 3V3 to that GPIO to have the State changes to HIGH. It works perfectly.
Well, if I enable the Longpress in "Switch input", as ESPEasy is it now, I cannot manage the LONGPRESS because I have two issues:

  1. If I press and release very quickly I have two EVENTs, first one is "1" and just after I have "10".

So why Longpress STATE (10) is generated if I press the push button for a very very short time?

  1. If I press, leave pressed for long time and after I release I have both EVENT1 (short press) and EVET10 (longpress).

Also here, we should manage Shortpress and Longpress and generate the appropriate EVENT according.

@chromoxdor
Copy link
Contributor

chromoxdor commented Oct 29, 2023

So why Longpress STATE (10) is generated if I press the push button for a very very short time?

What happens when you change "Longpress event:" to Active only on <the opposite of the current status>

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Oct 29, 2023

happens that the "Longpress event:" is NOT generated (also if I press and leave push button pressed for some seconds):

35084414: EVENT: swdown#State=1
35084423: ACT : taskvalueset,dummyswdown,state,1
35084429: ACT : taskrun,dummyswdown
35084435: Dummy: value 1: 1
35084448: EVENT: dummyswdown#State=1
35084478: ACT : Publish,espeasy_1/rollingshutterdownn,0
35084483: ACT : Let,8,0

but I suppose in this case it's correct.

@chromoxdor
Copy link
Contributor

chromoxdor commented Oct 29, 2023

So why Longpress STATE (10) is generated if I press the push button for a very very short time?

Ok.. let´s figure this out in the forum.... i don´t think its a bug since i couldn´t reproduce it.

@chromoxdor
Copy link
Contributor

chromoxdor commented Oct 29, 2023

so there is a bug.
Here is what i wrote in the forum:

You are right.. this seems to be bug indeed. It does create a longpress event when longpress event is set to "active on low". (every time you release the button there will be a longpress as it is supposed to be)

The combination "switch type" = "push button active low" & "longpress event " = "active on high" does not work at all.

@chromoxdor
Copy link
Contributor

chromoxdor commented Oct 29, 2023

Did some testing.
Yeah it is confusing but in a way it makes sense.

The longpress event is triggered only if the button type and the longpress event type are matching.

eg. button type: Push Button Active High & Longpress event = Active on High -> works
button type: Push Button Active High & Longpress event = Active on LOW -> doesn´t

Which makes sense.. who needs a longpress event when the button is released.

Edit: Which also begs the question.. do we need the longpress event selection? It could be kind of set automatically regarding the switch type leaving it a en/disable option only

@TD-er TD-er added the Type: Bug Considered a bug label Oct 29, 2023
@tonhuisman
Copy link
Contributor

I'm surprised that a Push Button config actually works with a long-press, as the nature of the Push Button, AFAICS, is that it changes state on the push or release of that button. No proper way of having a long-press in that configuration.
Long-press should work properly when configured as a Normal Switch, but, IIRC, it does send both the State=1 and State=10 (or 11) events for a long-press...

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Oct 29, 2023

ok, sorry but I see/read a lot of confusion :-)
Can you confirm what I discovered is a bug?
What I expect is to have the Longpress function working with a simple push button and without using Rules to "workaround" it.
I mean, if an user must use Rules to let it working this means it does not work as is it implemented.
Sorry @tonhuisman but a Longpress on a Normal Switch does not make sense, a Nornal Switch is 1 or 0, like a Switch of the light at our house,
Long press and Doubleclick are "feature" available only on push button.

@chromoxdor
Copy link
Contributor

I'm surprised that a Push Button config actually works with a long-press, as the nature of the Push Button,

if you use a latching push button maybe.. but in the plugin you simulate a latching push button with a non latching one... so it makes sense. unless i am mistaken

@TD-er
Copy link
Member

TD-er commented Oct 29, 2023

For a push button setup, it makes sense to have a long press.
Not for a "normal" switch.

However, I think we need to make things a bit more clear conceptually.
Now we act on a "push down" or "release" state as what is configured by the user.

When you only want to get an event for longpress or short press, then we must only send out an event when the opposite happens as what the user has configured.

"Active Low" typically means the input state is normally "high" and when you press a button the logic level is pulled to GND.
So what should a user expect?

  1. Event immediately when the button is pressed?
  2. Event when the button is released and the event type (longpress/shortpress) is based on how long the button was pressed?

If you really don't want a short-pressed event when a long pressed is intended, then only the 2nd option should be used.
However, this will cause a delay in event sending till the button is released.

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Oct 29, 2023

Hi also to you @TD-er

For a push button setup, it makes sense to have a long press.
Not for a "normal" switch.

Totally agree.

When you only want to get an event for longpress or short press, then we must only send out an event when the opposite happens as what the user has configured.

Exactly!

So what should a user expect?

Event immediately when the button is pressed?
Event when the button is released and the event type (longpress/shortpress) is based on how long the button was pressed?

It depend.
IF Longpress (or doubleclick) is NOT enabled (so user does not want this feature) THEN:
Immediately generate EVENT 1 (or 0) at push button press (so do NOT wait for button release since it does not care)

Instead, IF Longpress (or doubleclick) is enabled we MUST take care of the button release in order to understand if we MUST generate the LONGPRESS or DOUBLECLICK event or a simply "normal push" EVENT.
So if the release time is EQUAL OR GRATER than "Longpress min. interval" THEN user want a LONGPRESS so generate EVENT10 (or 11), ELSEIF the release time is LESS than "Longpress min. interval" THEN user want a "simply normal push" EVENT1 (or 0).

@TD-er
Copy link
Member

TD-er commented Oct 29, 2023

We can make it as elaborate as you describe. With "elaborate" I mean it either needs lots of options to choose from, or nested options and at least quite some documentation to make it clear what to expect...

Or... we can try to make it simple:

  • Option to send event directly when push button is pressed
  • Option to send event when the button has been released.

The 'release' event should then have 2 event values:

  • value indicating long or short pressed
  • time in msec the button was "active".

I think this is quite intuitive and flexible at the same time, what do you think?

@TD-er
Copy link
Member

TD-er commented Oct 29, 2023

Oh and to make sure we won't open another Pandora's box here, I am even thinking about making a separate plugin ID for input and output GPIO.
Then we could also make it available for the GPIO extenders if we like, but all in little steps and not making a one-plugin-should-do-it-all and end up with the complete mess we now have for the GPIO handling.

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Oct 29, 2023

We can make it as elaborate as you describe. With "elaborate" I mean it either needs lots of options to choose from, or nested options and at least quite some documentation to make it clear what to expect...

Or... we can try to make it simple:

  • Option to send event directly when push button is pressed
  • Option to send event when the button has been released.

The 'release' event should then have 2 event values:

  • value indicating long or short pressed
  • time in msec the button was "active".

I think this is quite intuitive and flexible at the same time, what do you think?

Sorry but I dont' like so much events for each state (push, release, etc...)
we end up becoming slaves of Rules and ESPEasy becomes ESPRules :)

@TD-er
Copy link
Member

TD-er commented Oct 29, 2023

That's why I mention the word "Option", so you can enable/disable sending events on either rising or falling edge of the signal.

This also makes me think about whether we should keep the "invert" option as it makes things really complex.
Well at least having only a single "invert" is making things complex.

@chromoxdor
Copy link
Contributor

chromoxdor commented Oct 29, 2023

I don´t see why not having just one en/disbable option for longpress and make it like @iz8mbw suggesetd:

  • short press on release if not yet the time for long press when enabled
  • immediate shortpress when pressed when longpress is disabled

And keep the three options for switch type.

so inverted or not we get an e.g. always the same longpress event... let´s say 10
so an active high long press as well as an active low long press makes a 10
and a switch the same... (there we have the one necessary inverted option)
Like i said..who needs a longpress event when the button is released

@TD-er
Copy link
Member

TD-er commented Oct 29, 2023

Well the reason why we should not "just-do-this-or-that" is simply the reason how we ended up in this mess of completely out of control and unmanageble complexity of something as basic as handling a single GPIO pin.

We have been 'fighting' this switch plugin for over 6 years already and right now we at least stripped about 10k of compiled binary code away from the switch plugin and split off quite some totally unrelated functionality.
But still we see issues being reported about basic GPIO handling which is not working, not fast enough, non-intuitive and simply buggy.
And on top of that we still can't have very simple basic input handling and output handling as it has become way too complex.

And about the added complexity of something as basic as "invert" for the entire plugin...
Sometimes you only need to have the output inverted and sometimes the input.
This alone already brings questions about the naming like "rising edge" or "low".

I was already "done" with this complexity of the switch plugin back in 2018 or '19, but we keep on patching it and I think now it is time to just make 2 simple plugins to replace P001-Switch:

  • GPIO input
  • GPIO output

This so we can make it work and then if it is working well we just let P001 go (or migrate it using a setting migration)
Making a new plugin also allows us some freedom in chosing intuitive settings/behavior over remaining compatible with all settings out there.

ESP32 does have very nice hardware filtering options which we don't use.
We don't use interrupts, etc. so it is a miracle we get to do what it does now.
And being a miracle also means it is code nobody wants to touch as it may break down when even looking at it since nobody really truly understands how it is working like it does.
Such code (not intuitive, nobody understands it) is something you really do not want to have in your code base as it really paralyses development and maintainance.

It is absolutely too complex for what it needs to do.

@chromoxdor
Copy link
Contributor

Thanks for the perspective... i only viewed from the input side.
2 separate plugins definitely would make things more intuitive for the end user

@TommoT1
Copy link

TommoT1 commented Oct 30, 2023

Would it be an option to leave everythig as it is?
Right now there is the possibillitiy to select options wich don´t work togeter - ok, BUT everything is possible, e.g. you can react on every button press AND on longpress and none is filtered away by some marked options or not, you have to to this little filtering in the rules if you need to.
If we change the status quo, there will be proabably more code needed and in the outcome some less possibillities.
In general espEASY is quite intuitive but on some solutions you need the help of the forum...
Just my opinion......
Regards Tom

@TD-er
Copy link
Member

TD-er commented Oct 30, 2023

@TommoT1 Well that's one reason I was thinking about making new plugins for the GPIO interaction.
So people have the option to leave it as it is and also to have the option to make the best choices as we learned from the past.

As always, with ESPEasy we aim to make it intuitive and simple for anyone and still allow to do all the advanced stuff for those who are more experienced.

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Oct 30, 2023

OK, so we go on for new Plugin for Switch Input and new Plugin for Switch Output.
@TD-er Let us know the progress of the implementation.

I suggest the new Plugins should take care about the ESP32 Internal Pull-up and Pull-down resistor included in the board, see: #4800

Thanks!

@TD-er
Copy link
Member

TD-er commented Oct 30, 2023

Yep, for the input plugin adding all supported types of pull-up and -down and also open drain should be added as well as a selector for the various strengths of the pull resistors (ESP32 does have several strengths available)

For both input and output you can also define how a pin should react to deep sleep.
You can even lock a pin to a specific state during deep sleep, however this may add considerable to the power consumption during deep sleep.

And for the ESP8266 it is possible to read out a single register and read the pin values of all pins in a single call.
This way you can simply create an interrupt callback for all required pins and use the same callback function for every pin.
Then you only need to read a single register and just store a mask of the changed pins with its timestamp (or delta) so you can process it later outside the ISR call.

For ESP32 you can use RMT to capture some sequence in memory and thus allow for very accurate filtering on debounce or noise filtering.

For the ESP32 I need to look into the options for the output plugin as the ESP32 does have some very very nice hardware options to generate extremely timing critical sequences and also combine several pins to lock them. (allowing pins to change state at exactly the same time)
This can be very useful for things like controlling H-bridges or phase locked actions.
You can combine GPIO pins via the internal MUX, allowing to 'clone' a signal to another pin.

I have to look into all of these as some might be more useful outside of these plugins and some might be useful to a lot more plugins and thus need to be part of the ESPEasy internals so those filters/features can be used anywhere a plugin/command may need to interact with a GPIO pin.

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Jan 2, 2024

Hi @TD-er
do you have a plan about this fixing this?
Thanks.

@iz8mbw
Copy link
Contributor Author

iz8mbw commented Apr 4, 2024

Hi @TD-er can you work on this?
It's an OLD bug.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Considered a bug
Projects
None yet
Development

No branches or pull requests

5 participants