-
Notifications
You must be signed in to change notification settings - Fork 86
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
add MIDI Button actions and dec/inc trigger modes #767
Conversation
Build for testing: |
Thanks for submitting this. Is there a specific reason that the inc/dec only looks for specific values? I don't know how MIDI emulates an encoder, but am guessing having different values enables a "listening" system to distinguish between further rotations compared to a single on-off? So, a sequence of (say) 64-63-62 would show decreasing - that kind of thing? But we don't seem to be checking the raw value at all against previous values, even though it is logged? Is this why the reset/write(0) has been added? I wonder if we ought to be registering the changes of value - as written, I'm struggling to see the difference between just registering off<64<on - am I missing something? If we do add in code to do something with the raw values, should the full range be acted upon? If not, the range should perhaps be configurable, unless there is a MIDI standard way for encoding inc/dec (I haven't looked tbh). I like to ensure we're not becoming too specific to a particular controller, but a feature like this could support the same from other devices. At some point I'll see if there is any guidance for MIDI encoders (and see what my MiniLab does)... Thanks, |
Let's start with the basics - What do we want to achieve here that is not already possible? |
Arturia encoders can be set to Absolute, Relative 1, Relative 2, Relative 3 modes. From the Minilab mkII's manual:
The encoders of AKAI Mpk Mini 3 can also be set to Relative mode, which is the same as the Arturia Relative 1 mode. The encoder doesn't send other values besides those, so a reset state (0) is necessary to prevent it from staying in INC or DEC state. And that's why the raw value was checked instead of the LOW/HIGH. It can be extended to support all 3 relative modes. |
873faa2
to
2609c55
Compare
Build for testing: |
Ok, so my view would be that we define a relatively simple behaviour in MiniDexed and then rely on the ability of a MIDI controller to be configured with a mode that supports it. I think the simplest is to follow the MIDI "standard" for on and off and relate that to being CW/CCW for an encoder - so in that world rather than OFF < 64 <= ON we would have CCW < 64 < CW so anything less than 64 decrements and 64 or above will increment (edit 64 and above is "ON" for standard buttons). I'm nervous of hard-coding in specific controller mode values - especially the 13-15/17-19 mode which are all "OFF" in normal MIDI controller terms... If we were to allow some configurability, then again I'd say keep it simple and let the user override an encoders "static" value - i.e. the centre value. That would be 64 by default which would cover both modes 1 and 2 and could be changed to 16 if mode 3 is configured. This is all assuming we don't do anything with the specific values themselves - e.g. to measure "further away from the centre" as being "it was turned faster" and could (say) inc by 2 or 10 or something - but that is probably a bit OTT for what we need. Kevin |
Agree @diyelectromusic, that'd be a clever way to do it. What worries me most is that all features made for specific hardware may ultimately break with future changes in MiniDexed as I don't have a way to retest them. |
2609c55
to
3953a3a
Compare
Ok. Customization can be done later if needed. |
Build for testing: |
...now the question... will it work? :) |
3953a3a
to
1675c70
Compare
Now, yes. |
Build for testing: |
Let's not change the defaults in |
I think yes, because empty trigger is mapped to BtnTriggerNone in CUIButton::triggerTypeFromString(). |
I don't think I have understood your last post yet, so maybe you can elaborate a bit. Is this PR worthwhile to be merged with the defaults in |
Oh, sorry for the short description. Until this PR, it was not possible to set MIDI Button actions in minidexed.ini. The default value was CUIButton::BtnTriggerClick. But with this PR they can now be configured, similar to GPIO buttons. But the default action value of GPIO buttons is CUIButton::BtnTriggerNone. In order for MIDI buttons to work similarly to before, the MIDI button actions must now be configured to click in minidexed.ini. It's not clear to me whether the MiniDexed version is 0.x or 1.x-ish. |
Thanks for the explanation @soyersoyer. This project is not using semver (yet?). But we try to use POLA (the "principle of least astonishment"), meaning that we try to keep the defaults as plain as possible, and make advanced features opt-in. |
I would update the "# MIDI Button Navigation" section of Wiki/Files/minidexed.ini/MIDI Button Navigation to the one in minidexed.ini. And I would add this line also after the It is possible... paragraph. |
@soyersoyer and @diyelectromusic, do you think this is ready for merging? |
What I've since found out is that the very useful MenuEventPressAndStepUp / MenuEventPressAndStepDown exists and is not supported by CUIButtons. Maybe that could be included, although it would also affect the GPIO button navigation. But that could be a topic for another PR. |
Yes, let's go step by step. 👍 |
Another thing I'm thinking of is that the default ButtonAction and MIDIButtonAction could be click, and that way the minidexed.ini could be more concise. |
Is there anything else I need to add or remove? Or is this good as is? Or is this good for this project? I'd love to lighten my WIP list with this. |
@diyelectromusic wdyt? |
Ok, so just to be clear (I don't have the means to test this really) we're saying: we've fixed it for 64 = centre; and if 64 is received then it is ignored? But if <64 or >64 we register a dec/inc event. But we're not coding in any different levels of movement - it is just one inc/dec at a time. Is that correct? If so, then I see no reason not to go for it. But as I say, I've not tested this myself, just checked through the code. Kevin |
1675c70
to
4d5725a
Compare
Thanks for the review. In the case of the center value (64), the general event handling was run. What do you think about changing ButtonAction and MIDIButtonAction to click? |
Build for testing: |
I think that was the original default for MIDI buttons anyway - so that should be ok shouldn't it? Kevin |
btw - isn't that |
Also, should raw be initialised to MIDIPIN_CENTRE - and probably again only scoped within that "isMidiPin" block...? |
Yes, but the default value of ButtonAction (for GPIO) also needs to change to be consistent. And I don't know if that's okay.
Nice catch! |
Sorry - I'm not really following. Are you just talking about values in minidexed.ini or values in the code? |
4d5725a
to
aa647a3
Compare
In the minidexed.ini |
Build for testing: |
I think its fine to have MIDI buttons default to click in minidexed.ini. They'll only be enabled if the CC configuration is setup. I don't think GPIO buttons need to be the same in minidexed.ini. But also have no issue with leaving them blank in there either the same way as the GPIO buttons. Kevin |
aa647a3
to
dc30f4f
Compare
Okay, if it's fully omitted, then click, otherwise, the specified action. |
Build for testing: |
Err... I asked if you were talking about minidexed.ini or code and you said minidexed.ini... but you've changed the code? I'm now completely lost as to what the issue is here - the config default should definitely be "no action" in my view - we shouldn't really be guessing configurations when the configuration string is absent. I know there are a few exceptions, but don't think this should be one. Kevin |
dc30f4f
to
c9ad2b9
Compare
We misunderstood each other then. I took out the defaults. |
Build for testing: |
I think it fine in minidexed.ini to have action=click even though many other buttons have action= This provides a useful starting point for users, but also makes it obvious there is something configurable here. I know there are lots of options now... :) Kevin |
c9ad2b9
to
81bac41
Compare
Build for testing: |
It's very slow for me, it's no longer fun after ~two months. |
No description provided.