Configurable GPIO buttons #274
Conversation
|
A build is available for testing: |
|
@probonopd I have pushed a working version with all of the suggested features. It would be good to get any feedback soon as I need to get back to my much more boring (but pays the bills) actual job! |
|
Thank you very much @diyelectromusic and @stevelittlefish. I think many MiniDexed users will be very happy! There is one issue I found: It breaks shortcuts
To reproduce:
My configuration: |
|
@probonopd This is not really fixable with the current implementation (I'm not saying it shouldn't be fixed!). How would you define which button is the shortcut button in the config? Bear in mind either select or back could be bound to double click so they can't really be used. It sounds like we need to bring the encoder button back in some capacity but this particular feature gets messy. How do you want this to work? |
|
The easiest fix would be to completely remove the select button, and reinstate the encoder switch pin. Then the encoder button would do the shortcut and also act as the select button. EDIT: Actually I think we should keep the buttons exactly as they are, and put the encoder back exactly how it was. The buttons should just not replace the encoder. We could add further configuration options to change what single, double and triple click do but that is separate to the features in this branch. Optionally you could unbind all of the encoder events and it would only function as a shift key for the shortcuts. The technical advantage of this is that the encoder and buttons are isolated from each other. If we had to implement the shortcut functionality from the buttons, it would mean the encoder and buttons would have to interact, as the encoder would need to detect that a button is held down, and then cancel the click and long press for that button when it moves. Alternatively, we would need to ditch the encoder library and handle the buttons and encoder together but this would be more effort than it is worth at this point. |
|
If we could use the old code for the encoder's button but the new configuration file esactly as it is now in this PR (which probably implies adding some code to make the action that happens when the encoder button is clicked/double-clicked/long-pressed configurable), I think that'd be the best of both worlds. Doable? |
|
I don't think it's really possible. First of all you have to specify which pin is the encoder button (i.e. pin 11). But the ky040 library supports triple click instead of long press, so the encoder can't do the same as the buttons. Personally I think long press is way better. If the ky040 library handles the encoder button at the same time as the ui buttons, then there is going to be a conflict where a long press is triggered while you are using the shortcut. It would be impossible to have, for example, click->select; double click->back; long press->home while also having it do the shortcut. If we really needed to get the encoder working with click, double click and long press the same as the buttons I think we would need to look for another encoder library or roll our own into the code base (it's not that complicated but I don't have time to do it). I'd prefer not to add triple click to the buttons because I don't think triple click is as good as long press, it would increase code complexity and also it adds more latency on detecting a click event as you have to wait longer to account for the triple click. Although it's a little messy, the best way to support all of the current functionality plus the configurable buttons is to add EncoderClickAction, EncoderDoubleClickAction and EncoderTripleClickAction as config variables. For the default config, I'd suggest having most of the options commented out at the bottom of the file. You could have all of the button config and the encoder actions commented out, so by default the config would be identical as it currently is on the master branch. This means out of the box you would get the current implementation with a single encoder. We could add a page on the wiki to show how to add the extra buttons. A common use case would be to unbind double click and triple click from the encoder button, and add a second button to do back with a click and home with a long press. |
|
I must admit that I wasn't quite sure why we took out the encoder switch... it seems much simpler to have an encoder mode and a switch mode for most "trivial" use cases but then allowing people to mix and match as they require if they are a little more willing to experiment. Another minor point is that removing the "enabled" flag also means that the code has to run and check all buttons even when disabled, but at the clock speed of a Pi, that isn't likely to be a problem really I guess. Just out of interest, did you measure the update period? I didn't get a chance to. I see you've added code to only scan if 100uS have passed... this will make the call to update quite "lumpy" but that might be fine (as I said elsewhere, I've not seen any documenting of the scheduler algorithms in play). If we're worried about updating every time, then different option (one I use in Arduino code quite a lot) would be to spread out the button scanning so it only checks one per call or something... but without measurements, I have no idea what would make a difference or not. Either way, if you're slowing it down, then I guess that answers the previous polling vs interrupt question! :) I do worry slightly that the whole single-double-long press thing has made things quite a bit more complicated for users in what was originally quite a simple thing to grasp... but I don't have a strong view either way :) I do think the default configuration ought to err on the side of simple though. Aside: why did you rename all the member variables? I thought they were following the naming convention used elsewhere? Again I don't really have a view, as long as it is all consistent, but I thought I was following this: https://github.com/probonopd/MiniDexed/wiki/Development#naming-conventions - so if I got that wrong, do let me know so I know for next time. Thanks, |
|
For my 2 cents on this, I'd be happy with encoder to change values, encoder
click for enter, and either encoder double click for back, or a separate
button for back (or both)
…On Tue, Jun 7, 2022 at 2:31 PM Stephen Brown ***@***.***> wrote:
I don't think it's really possible.
First of all you have to specify which pin is the encoder button (i.e. pin
11). But the ky040 library supports triple click instead of long press, so
the encoder can't do the same as the buttons. Personally I think long press
is way better.
If the ky040 library handles the encoder button at the same time as the ui
buttons, then there is going to be a conflict where a long press is
triggered while you are using the shortcut. It would be impossible to have,
for example, click->select; double click->back; long press->home while also
having it do the shortcut.
If we really needed to get the encoder working with click, double click
and long press the same as the buttons I think we would need to look for
another encoder library or roll our own into the code base (it's not that
complicated but I don't have time to do it).
I'd prefer not to add triple click to the buttons because I don't think
triple click is as good as long press, it would increase code complexity
and also it adds more latency on detecting a click event as you have to
wait longer to account for the triple click.
Although it's a little messy, the best way to support all of the current
functionality plus the configurable buttons is to add EncoderClickAction,
EncoderDoubleClickAction and EncoderTripleClickAction as config variables.
For the default config, I'd suggest having most of the options commented
out at the bottom of the file. You could have all of the button config and
the encoder actions commented out, so by default the config would be
identical as it currently is on the master branch. This means out of the
box you would get the current implementation with a single encoder.
We could add a page on the wiki to show how to add the extra buttons. A
common use case would be to unbind double click and triple click from the
encoder button, and add a second button to do back with a click and home
with a long press.
—
Reply to this email directly, view it on GitHub
<#274 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNBVNGKUYQZHGWCVSDL2ODVN6IPJANCNFSM5X5V4YKA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
It wasn't my suggestion, but it seems nice to only have 1 way of specifying buttons. I think we should probably add it back in though to support shortcuts, and also because the encoder library has no way to disable it - you have to bind it to a pin. (I bound it to GPIO0 as this pin is reserved.)
If all of the button pins are set to 0 it basically does nothing. The time to loop through all buttons and check that they are 0 is not going to be an issue, but if it was we could just internally disable the buttons when they are all 0 without having to have a separate config option for it.
I haven't measured it. If performance becomes an issue we could spread out the button scanning as you describe, but I'm not convinced it will make a noticeable difference. The reason I put in the limit was because I had to set the timers to extremely high values to handle long press. Also, as more stuff gets added to the code base and the update cycle takes longer the numbers may need to be adjusted. I'm assuming the timer value will not change so will remain consistent.
I think most of the button options should be commented out so as not to make the initial config more confusing. Having long press on the back button to return to home is nice though!
I actually never saw that page. I added quite a few variables and then I started getting confused and just changed all of the ones that I kept forgetting how they were spelt! It was a long day! Like nPin and pPin - I changed them to pinNumber and pin so I could remember which one was which! |
|
Another alternative solution would be to do this:
This means if you click pin 11 and then turn the encoder, it would adjust the parameter on all voices as it previously did, and then call a function in CUIButtons to reset the state of any buttons bound to pin 11. With the above solution we could rename the config variable as it doesn't actually have to be a button on the encoder, and it basically acts as a shift key for the encoder. I'd go with either this behaviour, or just re-instating the encoder alongside the buttons and you can choose whether to use the encoder button or separate buttons (or both). I feel like this has gone a little out of the original scope of just adding a separate back button! |
|
@stevelittlefish that approach you described in the post above sounds good to me as it would combine the best of both worlds if I understand it correctly. Do you think you could implement it? That would be trememdous. I know from other discussions that many users will appreciate this kind of configuration flexibility. |
|
@probonopd I will try to get it done, but my wife literally just gave birth so I'm pretty busy all of a sudden! |
|
Congratulations! |
|
@probonopd I'm going to try to get this done over the next few days. I know exactly what to do so it shouldn't take me too long. I'll try and push something that is testable - then next week I will do one last push to clean up variable names and tidy everything up. |
This reverts commit c050e04.
|
@probonopd I've pushed all of these changes now. Once you've tested and are happy with everything, let me know and I will remove all of the debug code and tidy some stuff up ready for merging. |
|
Thank you very much @stevelittlefish. Will test MiniDexed_2022-06-27-de1c9af now. |
|
In my quick test the following configuration worked well for me: I like it a lot! 👍 Definitely a great improvement. Note that I have set Did you consider making the |
Honestly I hate it when longpresses are triggered on release. It's very difficult to tell if you've held the button for long enough as there is no feedback, so you always end up holding it for longer than you need to, just to be sure. I think it's much more useable with a longer timeout, triggering as soon as the timeout is reached. Obviously that's just my opinion! |
One thing to note - LongPressTimeout must be longer than DoubleClickTimeout. If you set it shorter than DoubleClickTimeout, it will print an error to the HDMI and increase LongPressTimeout to match DoubleClickTimeout. So in your above example, it will have used 400ms for the Long Press. |
|
Makes sense... Would anyone volunteer to do a code review? Should be someone more versed in C++ than myself ;-) |
|
Can we get this merged? It's been sitting here for a while. |
|
Indeed. Looks like in the process of experimenting, I fried the input pins on my Raspberry Pi due to electrical issues on my side, so can't test right now. But it was looking good in my last tests, so if no one objects in the last second, I'll go ahead and merge it. Would you agree to making this the default? |
I think this is fine - but I haven't really played with it with this configuration. The way I was using it was to have LongPressTimeout really long (around 800ms), and to use double click for back, and long press for home. I don't have a strong opinion though - the whole point of this set of changes is that I wanted to have a separate back button! :-) |
Double click is now home and long press is back
|
@probonopd I've pushed a commit with the changes you suggested. |
|
Tested MiniDexed_2022-07-13-b2caa36, works very well for me. A big "Thank You" to everyone involved, especially @diyelectromusic and @stevelittlefish. |
|
Note to everyone updating to the new version: PLEASE NOTE:
|
|
Thanks all for your help getting this tested / ready for merge. Kevin @diyelectromusic - your first batch of changes saved me a lot of time and head scratching and the discussion around polling vs interrupts was very helpful - definitely changed the way I will think about this going forwards! |
|
Look Ma! No rotary encoder!
|
|
Oh, very nice :) Is that using a ssd1306 display too? |
|
Yes. |
|
What I think would be nice is if the left/right = prev/next buttons would continue to decrement/increment values while being hold down. E.g., when you change a numeric value, you would not have to press up to 127 times. (No long-press for these.) |
Technically, this is very easy to achieve. The thing to think about, however, is that it means those buttons behave differently to the other buttons. This causes 2 issues - first of all there is going to be a different handler for these buttons which means there are now 3 different places in the code where the buttons are handled (encoder, current buttons solution, and a special handler for these buttons). But perhaps more importantly, the ini file could get a little more complicated as there will be inconsistencies between the different buttons. Another thing to consider is would you want the (press and hold) shortcut to work with these buttons too, so they can switch between the different channels? I think it might be worth thinking about where you want to go with the user interface a bit longer term, considering the following:
I personally am not very impressed with the ky040 encoder library - even with hardware debounce on the encoder pins it still doesn't work 100% accurately for me, even though with the exact same circuit with an arduino nano works flawlessly. The other issue I have with it is that there is no way to unmap the encoder button. If you set it to 0 in the config it actually binds it to GPIO0. It's not a huge deal as this pin is not likely to ever be used for anything, but I'd like to be able to turn off the encoder button! I also find calling the encoder pins "clock" and "data" pretty confusing. Those are just a few points to think about. If you want to go ahead with your suggested change I'm happy to do the development work as I'm familiar with the button handling - let me know! |
|
Thanks @stevelittlefish. By now you know this code much, much better than me, so you are in the best position to make technical choices. I will add my 2 cents about the intended behavior:
That would be acceptable for me, as we use only the left/right = prev/next buttons for decrementing/incrementing values, for which we need the press-and-hold behavior.
I would not need the prese-and-hold behavior for the shortcut (while selecting the different TGs), because it only goes through 8 values (whereas where we need the press-and-hold behavior there are 127 values most of the time). But of course the shortcut should still be usable with the left/right = prev/next buttons (for selecting the different TGs). I hope my description is clear, if not please ask.
I fear I cannot add much usable information to this discussion, but:
For me neither - but I don't know how to improve the situation.
That's actually how the rotary encoder PCBs are labeled, hence the library is using those terms, even though they are technically incorrect. Sorry I can't be more helpful with the technical choices here, but I believe who implements this is best positioned to make technical decisions. I'd be very happy if you could give it a go @stevelittlefish. |
|
@probonopd I never implemented the shortcut for the buttons at all - it only works on the encoder. Do we need to add it for the buttons too? |
|
The button in the encoder is also just a button... nothing prevented me from using the same pin for just a plain button - which I did, and it worked for me iirc. Will resume my tests as soon as I have soldered my v2 prototype modules :) |
|
@probonopd I've been super busy with some work / life stuff and not got onto this. Can we open a new discussion for this set of features? Do you need me to create my branch first? |
|
For what it's worth I'm having a look at the feasibility of adding MIDI buttons, so in the interests of not having the code diverge too much at the same time, maybe there is no rush for an update right now? :) |
|
Thanks @stevelittlefish, no worries. We are not in a hurry. If you have some time, it'd be super cool if you could send a pull request for "shortcut for buttons" and/or "decrement/increment values while left/right = prev/next are being hold down". If you would like to discuss more regarding these features first, feel free to open new issues for them first. Thank you for your highly valuable contributions. |
|
Brings whole new meaning to "mini" Dexed
…On Thu, Jun 9, 2022 at 4:19 PM probonopd ***@***.***> wrote:
Congratulations!
—
Reply to this email directly, view it on GitHub
<#274 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNBVNFDTPD3OPBYA3C5QK3VOJGVZANCNFSM5X5V4YKA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Thanks @diyelectromusic and @stevelittlefish
(This is a continuation of #271 and includes that PR, so only one of the two should be merged eventually.)