Button Debounce doesn't work correctly

[W1zzardTPU]

#1720

@pgrawehr

There's an issue with this implementation, when the button is held down for longer then the debounce timeout. When it is released now, there will be a "pressed" event due to the bounces happening during release, and the desired "released" event is fired, due to the debouncing getting started by "pressed".

I solved this by using a timer for debounce that checks the current button state when finished. Something like below, same for HandleButtonReleased()

protected void HandleButtonPressed()
{
    _stateForAfterDebounce = true;  // keep track of the button state, even with bounces, so we can get the current state at the end of the debounce timeout

    if (IsPressed)  // already pressed
        return;
    if (_debounceTimer!=null)   // currently debouncing
        return;

    IsPressed = true;

    if (_debounceTime.TotalMilliseconds > 0)
    {
        _debounceTimer = new Timer((o) =>
        {
            _debounceTimer?.Dispose();
            _debounceTimer = null;

            if (_stateForAfterDebounce == false)  // Button might have been been released while we were in the debounce. Check current state and handle
                HandleButtonReleased();
        }, null, _debounceTime, Timeout.InfiniteTimeSpan);
    }

    ButtonDown?.Invoke(this, new EventArgs());

    if (IsHoldingEnabled)
    {
        _holdingTimer = new Timer(StartHoldingHandler, null, (int)_holdingMs, Timeout.Infinite);
    }
}

[krwq]

@W1zzardTPU do you feel like sending a PR with a fix? I'm sure there will be more people hitting this. Perhaps we should be doing debounce on both pressing and releasing button (I didn't look at the implementation so take what I'm saying with grain of salt)

[raffaeler]

I didn't have the time to take a look on this but it would be great being able to use a single timer.

[W1zzardTPU]

do you feel like sending a PR with a fix

Don't have the time unfortunately

Perhaps we should be doing debounce on both pressing and releasing button

Yeah of course, the same changes are needed for button release. I just pasted my pressing implementation, so you can get an idea of my approach

I didn't have the time to take a look on this but it would be great being able to use a single timer.

Can't think of a way to share the debounce and hold timers, it's a one-shot timer anyway.. I just copied the original "hold" implementation

[raffaeler]

I finally had some time to spend on this issue.
If I understood correctly the problem, the following code reproduces the behavior described in this issue.
Anyway this test passes. So I probably don't get the real issue.

Please note that the very first ReleaseButton will cause the pressed event. I did not expected it after a holding event, but this behavior is enforced by the tests written by the original author of this binding.

@W1zzardTPU Could you please take a quick look and tell me whether this test represents the issue?

[Fact]
public void If_Button_Is_Held_Down_Longer_Than_Debouncing()
{
    bool holding = false;
    bool doublePressed = false;
    int buttonDownCounter = 0;
    int buttonUpCounter = 0;
    int pressedCounter = 0;

    // holding is 2 secs, debounce is 1 sec
    TestButton button = new TestButton(TimeSpan.FromMilliseconds(1000));
    button.IsHoldingEnabled = true;

    button.Press += (sender, e) =>
    {
        pressedCounter++;
    };

    button.ButtonDown += (sender, e) =>
    {
        buttonDownCounter++;
    };

    button.ButtonUp += (sender, e) =>
    {
        buttonUpCounter++;
    };

    button.Holding += (sender, e) =>
    {
        holding = true;
    };

    button.DoublePress += (sender, e) =>
    {
        doublePressed = true;
    };

    // pushing the button. This will trigger the buttonDown event
    button.PressButton();
    Thread.Sleep(2200);
    // releasing the button. This will trigger the pressed and buttonUp event
    button.ReleaseButton();

    // now simulating hw bounces which should not be detected
    button.PressButton();
    button.ReleaseButton();
    button.PressButton();
    button.ReleaseButton();
    button.PressButton();
    button.ReleaseButton();

    Assert.True(buttonDownCounter == 1, "ButtonDown counter is wrong");
    Assert.True(buttonUpCounter == 1, "ButtonUp counter is wrong");
    Assert.True(pressedCounter == 1, "pressedCounter counter is wrong");
    Assert.True(holding, "holding");
    Assert.False(doublePressed, "doublePressed");
}

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[raffaeler]

The above test is now merged in the repo.
https://github.com/dotnet/iot/blob/main/src/devices/Button/tests/ButtonTests.cs#L299

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[W1zzardTPU]

I'm no longer working on my project.

"If I understood correctly the problem, the following code reproduces the behavior described in this issue.
Anyway this test passes. So I probably don't get the real issue."

Looks like it. I don't think adding a test that doesn't test the right thing makes sense at all.

[raffaeler]

Looks like it. I don't think adding a test that doesn't test the right thing makes sense at all.

I don't get what you mean. If you believe the behavior you saw is different, please describe it more precisely and I will try to reproduce it in a test.

[krwq]

@W1zzardTPU can you write a test which exercises your scenario?

[W1zzardTPU]

Still no time, for a quick test, try running a loop of random presses/releases to simulate bounce.

both before and after the sleep