Fix race condition causing lost events #9
Conversation
If the ISR set the state of button after ret was set but before button was set to open, the event was lost
doubleClickTicks was being decremented twice, which caused things to not work right when double clicks were disabled.
Moved definition so compile works when WITHOUT_BUTTON is defined. this is a duplicate of AndyHunti's change
defaults to enabled. controlled with setButtonHeldEnabled
after fixing the double decrement on doubleClickTicks, things felt sluggish, so reduced ENC_DOUBLECLICKTIME to 400
Moved static variables to be private variables instead so that each instance of the object has it's own copy.
The table lookup method was not working because the tbl variable was not signed. Also HALFSET was always used because the #ifndef needs to be #if
Added new constructor for button only initialization. Changes all pin definitions to int8_t so a negative pin number will disable the button operation, or also the encoder operation. There was a previously an issue with the default button pin being -1, but stored unsigned was pin 255, so the button logic was running unless you explicitly specified pin 0 - this caused pin 0 to be unusable for a button. Note that if you previously specified pin 0 in your sketch to disable the button operation, then this should be changed to -1
Also shows the new button-only constructor and disabling hold/release and double click
…y enable pin 0 To maintain backward compatibility, buttons by default wont work on pin 0, however setButtonOnPinZeroEnabled(true) can be used if a button is desired on pin zero. Recommendation is to use pin -1 to disable button logic rather than pin 0
Added condition to include of AVR libraries
Button must stay down at least 1 full tick to register. This filters out out transient spikes that registered as clicks. This problem is likely due to a dodgy button and/or case vibration, but having this software fix does not affect normal operation.
@platformio Library Registry manifest file
Changed #IF to positive logic to include only if AVR
This reverts commit 2ad751f.
This reverts commit c471bc1.
Allows library to compile on the Arduino Due
…ttons Added methods setDoubleClickTime, setHoldTime. Moved default times to header file. Changed doubleClickTicks to 16 bits. Added currentMillis variable into the service method to save calls to millis() and replace now variable..
…condition fixed Multiple buttons can be put on one analog pin by defining a range for the analog input that presents a button press. Note that this method only really works for one button pressed at a time. Multiple buttons pressed at a time may not register at all, or register as the wrong button. A new constructir was added that allowed the analog input range to be specified. Also added noInterrupts to getButton to cure race condition
Also made steps variable volitile like the other variables used in getValue
|
can you please clean up the PR so that I can review the actual changes? You did a lot of formatting changes, please undo them as well. |
|
At this point no I will not undo anything. I fixed and enhanced so much
stuff that I consider my fork to be the master and I have been directing
everyone to my fork. If you want to take my fork in its entirety and merge
it with yours I would be happy to create a pull request for everything, but
I will not unwind anything I have done unless it is a bug.
I really appreciate the work you did and I really liked the concept of how
your library worded (that is why I started with yours) but after a year
since I forked it, the improvements and enhancement I have done are
significant.
Please look at my fork. If you want it all, I will submit it all to be
merged.
…On Jan 15, 2017 6:46 AM, "0xPIT" ***@***.***> wrote:
can you please clean up the PR so that I can review the actual changes?
You did a lot of formatting changes, please undo them as well.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APx9EIdBc_A4ae7z2Gu0XrEGaOpvPlPNks5rSgckgaJpZM4HdsdD>
.
|
|
You haven't replies back. Do you want to merge my whole fork? I think if you look at all the commits I did you will see that I put a lot of effort into this library and going back through all of that to undo things would be a whole lot of work that adds no value. I think it would be a shame to not take all the fixes and enhancements I have done. |
This seems to be a better default as inexpensive e-bay encoders use 4
Removed the ClickEncoder constructor for multiplexed analog buttons. AnalogButton is now a child class that can be used to defined these buttons. Also added a DIgitalButton child class for using a stand alone buttons (without an encoder). The ClickEncoder constructor for a stand alone button still exists, but consider it depricated in favor of DigitalButton.
Using ClickEncoder to define a stand alone button has been depricated. Use DigitalButton instead.
|
Thanks a lot, I'll try to review this on the weekend! |
ClickEncoder.cpp
Outdated
|
|
||
| if (pinA >= 0 && pinA >= 0) { |
There was a problem hiding this comment.
Why double check pinA? Do you mean pinA and pinB instead?
|
I think you are correct. I will do another commit to fix it
…On Feb 14, 2017 1:48 PM, "Jürgen Skrotzky" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In ClickEncoder.cpp
<#9 (review)>:
>
+ if (pinA >= 0 && pinA >= 0) {
Why double check pinA? Do you mean pinA and pinB instead?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#9 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APx9EFKNfsC_YToh_tbfBdWIevVf3SD6ks5rcfcMgaJpZM4HdsdD>
.
|
|
I've manage to review most changes last night. Would you be willing to provide smaller PRs? |
|
It was never intended to be one big pull request. It started with just the first commit, but GITHUB for whatever reason just keeps adding subsequent commits onto the original old pull request. Each commit is a very reasonable scope, but don't know how to break things up so each commit is a pull request. |
|
I think all the pull requests you have accepted are already in my fork (although perhaps done slightly differently) If you are happy with everything, then I think we can resolve all the conflicts. The only thing I can think that you might not want to include is the analog button support - putting multiple buttons (or encoder buttons) on one analog pin. It is perhaps not quite thematic with the library, but it got me out of a jam where I had no pins left. The limitation is that pressing multiple buttons at the same time wont work right (not an issue for my application) If you want everything but this feature, I could add a commit to pull it out. |
* Add files via upload * Add files via upload
Fixed timing compare
Use math instead of bit operations to sign is never lost
Add reset Encoder method. Improved timing. All timing is now done with Millis instead of counting "clicks", making operation more consistent if calls to service happen on an odd schedule.
fully safe casting
If the ISR set the state of button after ret was set but before button
was set to open, the event was lost