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

Cleanup getButtonState(), fix button handling bugs #1331

Merged
merged 3 commits into from
Jul 25, 2022

Conversation

nyanpasu64
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?

Refactoring, bug fix

  • What is the current behavior?

  • What is the new behavior (if this is a feature change)?

  • Changed: Do not generate a spurious BUTTON_BOTH when releasing a two-button long-press. Previously, if you released the two buttons not at the same time, every check starting at the first release would update previousStateChange, and the second release would misinterpret previousStateChange as a button press time and act like a two-button short-press.

    • If desired, the new code could be changed to always generate BUTTON_BOTH releasing a two-button long-press.
  • Fixed: Every single-button press generates either one press-release event or 1+ long-press events (assuming I didn't write any bugs). Previously if you didn't hold a button until the first check at >timeout ticks, it would never trigger BUTTON_*_LONG, and if you released it after the last check at <timeout ticks, it would not trigger BUTTON_*_SHORT.

    • Note that short-pressing both buttons, quickly releasing one button, then long-holding the other button will not register a long-press of one or both buttons, but will register a short-press of both buttons when you release the second button. This behavior feels fairly natural to me, and also occurs on the original firmware, but you'll have to decide whether this is OK or not.
  • Other information:

Backstory

I made a previous attempt at rewriting getButtonState(), I thought I had written a correct function, tested it and didn't find any behavioral issues in practice, then the next day I read over the code and found some more bugs/edge cases to fix. At that point, the code was 50 lines longer than what I started out with, making the function not fit in a single screen and hard to read the whole thing at once. After testing the result, I decided to revert a change where releasing one of two buttons in a two-button long-press can produce a single-button long-press, since the user may not have intended that.

I decided to create a new branch without my changes, and decided to make the most surgical changes possible to fix behavioral bugs. I'm a lot more satisfied with the outcome this time around. Though in practice, there's little difference between the original code and this PR from the perspective of a user (I've never noticed lost button presses at the border between short and long press myself).


I'd like a check that in the "Don't send short-presses after releasing long-presses" commit, I added mutations and checks to longPressed in the right spots. I read over the code myself and it seems to check out, but I'm very fallible.

I very much would want code review and a second opinion, that removing the if ((xTaskGetTickCount() - previousStateChange) < timeout) check for short-presses is correct (in commit "Fix button presses between short and long being ignored entirely"). I think it will never accidentally produce a short-press when you don't want it, but I very much could be wrong.

This does not upgrade timestamps to 64-bit TickType_t, which is needed to avoid some integer overflow bugs at 2^32 milliseconds (49.7 days). I will test the behavior of integer overflow and make a PR with the necessary fixes if I have a chance.

@Ralim Ralim merged commit de2972e into Ralim:dev Jul 25, 2022
@nyanpasu64 nyanpasu64 deleted the cleanup-button-handling branch July 25, 2022 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants