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.

Sign up for GitHub

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

Jump to bottom

midi-components-0.0.js: Pot support for arbitrary maximums in 7-/14-bit handlers #4495

Merged
merged 2 commits into from
Jul 1, 2022
Merged

midi-components-0.0.js: Pot support for arbitrary maximums in 7-/14-bit handlers #4495

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
311eb1a
midi-components-0.0.js: Pot support for arbitrary maximums in 7-/14-b…
Swiftb0y Oct 30, 2021
2649ef6
refactor: reconstruct high-res input val instead of changing the inpu…
Swiftb0y Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions res/controllers/midi-components-0.0.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,16 +467,25 @@
// For the first messages, disregard the LSB in case
// the first LSB is received after the first MSB.
if (this.MSB === undefined) {
this.max = 127;
// a flaw in the Pot API does not mandate consumers
// to supply an accurate max value when using
// this with non-7-bit inputs. We cannot detect
// that in the constructor so set the max
// appropriately here
Comment on lines +472 to +474
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

if (this.max === Component.prototype.max) {
Copy link
Member

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.

Copy link
Member Author

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.

this.max = (1 << 14) - 1;
}
value = (value << 7) + (this._firstLSB ? this._firstLSB : 0);
this.input(channel, control, value, status, group);
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

this.max = 16383;
}
this.MSB = value;
},
inputLSB: function(channel, control, value, status, group) {
// Make sure the first MSB has been received
if (this.MSB !== undefined) {
this.input(channel, control, value, status, group);
} else {
this._firstLSB = value;
}
},
connect: function() {
Expand Down