-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
midi-components-0.0.js: Pot support for arbitrary maximums in 7-/14-bit handlers #4495
Conversation
// this with non-7-bit inputs. We cannot detect | ||
// that in the constructor so set the max | ||
// appropriately here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't that be detected in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we don't know how the component is being used until either the input
handler (for regular 7-bit precision) or the inputLSB/inputMSB
handlers (for 14-bit precision) have been called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this requirement later is a breaking API change, so something we should reserve for the ES6 rework.
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474 and mixxxdj#4495
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474 and mixxxdj#4495
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474 and mixxxdj#4495
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
This PR is marked as stale because it has been open 90 days with no activity. |
ping. I am using this in the pending Reloop ready mapping and it works so far. |
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
This PR is marked as stale because it has been open 90 days with no activity. |
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you I have left some comments.
// this with non-7-bit inputs. We cannot detect | ||
// that in the constructor so set the max | ||
// appropriately here | ||
if (this.max === Component.prototype.max) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand the condition. When does this happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It checks if the user did not specify a custom max value. If that is the case, the maximum is not 2^7-1
but 2^14-1
since we're in the MSB handler. If the user did specify a custom max value, then this was intentionally chosen to be correct. As explained in the comment, this is a workaround for the flawed API.
} | ||
var maxSaved = this.max; | ||
// maximum value of MSB | ||
this.max = this.max >> 7; | ||
this.input(channel, control, value, status, group); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary change a member to before calling a function looks hacky.
Why is this code required? Is this an initial call with low precision?
Let's say we have 8 bit precision, than the MSB is 1 for the max value.
inValueScale() has div by 0 with that and will not work well with other small bit counts.
Can this call be removed?
Alternative we may leave max untouched just set the this.MSB before and pass 0 as value.
By the way, every input function does
```
if (this.MSB !== undefined) {
value = (this.MSB << 7) + value;
}
That logic can be moved to inputLSB() so that value is always the truth an the single precision code dos not suffering form the penalty of checking this.MSB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary change a member to before calling a function looks hacky.
I agree, but the entire this.MSB !== undefined
branch is a hack...
Why is this code required? Is this an initial call with low precision?
Yes.
Can this call be removed?
Maybe. I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with removing this call (and by extension the entire branch) is that requesting the initial controller state will break if the LSB is received before the MSB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the LSB received before the MSB, the whole implementation is broken!
Consider going from 126 to 129
MSB LSB
126 = 0000000.1111110
127 = 0000000.1111111
128 = 0000001.0000000
129 = 0000001.0000001
Wen the send order is actually swapped the MSB from the first value is paired with the LSB from the next:
LSB MSB
126 = 1111110.0000000
127 = 1111111.0000000
128 = 0000000.0000001
129 = 0000001.0000001
result:
MSB MSB
127 = 0000000.1111111
0 = 0000000.0000000
129 = 0000001.0000001
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the C Domain we allow both orders:
mixxx/src/controllers/midi/midicontroller.cpp
Line 318 in 7672cf1
if (mapping.options.testFlag(MidiOption::FourteenBitMSB)) { |
The requirement is that both messages are send with nothing else in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, we discussing no longer the scope of this PR.
I have not understand the controller state issue, that requires the discussed branch.
In the C domain we also do not know the order in advance. We collect MSB and LSB messages and when both is there we calculate the value. We discard a stored MSB or LSB when any other message is send. This ensures that both are actually belong together.
Of cause that can ticked as well, but at least better than always using the wrong order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, we discussing no longer the scope of this PR.
So whats the point then? I know the design is flawed. So will we agree on that and discuss whats actually in the scope of this PR or will we blow it 10x as we usually do?
In the C domain we also do not know the order in advance.
Are you sure? Thats what I figured the distinction between MidiOption::FourteenBitLSB
and MidiOption::FourteenBitMSB
was.
I have not understand the controller state issue, that requires the discussed branch.
Ok going back on topic. We don't know in which order inputLSB
and inputMSB
will be called so to basically avoid throwing an error deeper into the callstack we ignore the LSB message and then when we receive the MSB, we use that as the LSB as a single-shot workaround. Does that answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, we discussing no longer the scope of this PR.
So whats the point then? I know the design is flawed. So will we agree on that and discuss whats actually in the scope of this PR or will we blow it 10x as we usually do?
That was my point, not blowing up this PR.
I have now understand the issue.
Are you sure? Thats what I figured the distinction between MidiOption::FourteenBitLSB and MidiOption::FourteenBitMSB was.
Yes, pretty sure. We even have a test for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for the solution here, wan may to shift the "value" left, instead of sifting the "max" right.
If we in addition store the discarded LSB we can put it in and have a fully valid message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I understand your proposal. Thanks. I just pushed the commit containing your suggestions.
Thank you for picking this PR up. |
…it handlers previously, it was not possible for 14-bit controls to have maxima that are not (2^14)-1. Some controller might only have 3-bits of resolution in their MSB. This commit allows for arbitrary precision in MSB and LSB by specifying an appropriate value as the components max property.
224f73c
to
2649ef6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I have filed a bug for the pending issue: |
Thank you |
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512
Initial commit of a mapping for the Reloop Ready controller. The Padmodes are not implemented and just default to the hotcues right now. The mapping currently depends on mixxxdj#4474, mixxxdj#4495 and mixxxdj#4512 fix(controllers): Reloop Ready fx2 level knob Co-authored-by: majekw <majek@w7i.pl> various features (i'll squash later anyways) * Add option to correct for offset tempo fader notches * Improve LED feedback of various buttons when pressing and shifting * add eject function to load button when shift-pressing * remove dad code * turn of big pads when shutting down * fix 4th pad in ManualLoopPadMode (loop_in_goto) * Add beatJumpMode for ScratchBank * fix slip button Add pitch play mode (WIP) (untested)
This fixes a regression introduced in mixxxdj#4495 and reported as mixxxdj#11814
previously, it was not possible for 14-bit controls to have maxima
that are not (2^14)-1. Some controller might only have 3-bits of
resolution in their MSB. This commit allows for arbitrary precision
in MSB and LSB by specifying an appropriate value as the components
max property.