HidController: change getInputReport to automatically call script

[Be-ing]

This way, scripts don't need to wrap calls to
controller.getInputReport with another call to their incomingData
function.

[Be-ing]

I have successfully used this with #4056 and libusb/hidapi#351 to initialize the state of my Kontrol S4 Mk3 when Mixxx starts.

[Be-ing]

@JoergAtGithub what do you think about this change?

[Be-ing]

Oops, there was a good reason we did it this way.

[Be-ing]

Since I couldn't get #4521 to work, I tried calling controller.getInputReport in my script's incomingData function to test if it actually created an infinite loop as I feared, and it does not.

[JoergAtGithub]

Why you removed the return value? How to access bits that are not linked to Mixxx controls now?
Now the API differs from the similar function getFeatureReport.

[Be-ing]

How to access bits that are not linked to Mixxx controls now?

That is up to the incomingData callback.

Now the API differs from the similar function getFeatureReport.

Yeah, that's ugly, but unless you know how to get #4521 to work, I think we'll have to live with that... :/

[JoergAtGithub]

I don't get the point why you want to change the type of the return value. It is working for getInputReport and getFeatureReport now. Why shouldn't it work anymore if you add a call of processInputReport.

This way it was implemented in my original code, before code review: https://github.com/JoergAtGithub/mixxx/blob/79e139285282701aeb66431a92162f3e3bdb6d94/src/controllers/hid/hidcontroller.cpp#L163

[Be-ing]

QList<int> is not a QByteArray and does not become a JavaScript Unit8Array, which is what is passed to incomingData by HidController::poll. Therefore the return value of getInputReport cannot be passed directly to incomingData.

[JoergAtGithub]

I use it this way in the Z2 mapping:

mixxx/res/controllers/Traktor-Kontrol-Z2-hid-scripts.js

Line 1919 in f4afa00

// Now read the status of both InputReports

[Be-ing]

Right, but your incomingData function does not rely on it being an ArrayBuffer. Mine does:

mixxx/res/controllers/Traktor-Kontrol-S4-MK3.js

Line 1974 in ab636db

this.inPackets[2].handleInput(data.buffer);

[JoergAtGithub]

I need the raw report data for the initialization of the Z2 in the Init section, before polling starts. This is why I implemented this function.
I recommend to implement the functionality you need as independent function. Maybe call it: ForcePoll(ReportId)

[Be-ing]

I don't understand what you're proposing. Isn't that what getInputReport already does?

[JoergAtGithub]

I propose to leave getInputReport as it is (returns the bytes of the specified input report and isn't calling processInputReport). This ensures that there are no unintended side effects during initialization of the device.

And implement a second API function that calls processInputReport but doesn't return anything.

[Be-ing]

I don't think it's a good idea to have almost identical functions in the API that only work slightly differently.

[Swiftb0y]

I don't think this is a good idea. IMO we should favor getting #4521 to work. My suggested workaround would be calling Uint8Array.from behind the scenes and then returning the QJSValue as a workaround. Not pretty either but better than whats proposed here imo. While the changes here technically make sense, they make the API very confusing to use.

I prefer hiding the uglyness in the methods on the edge between JS and C++ and not expose the uglyness into our Controller API.

[Be-ing]

My suggested workaround would be calling Uint8Array.from behind the scenes and then returning the QJSValue as a workaround.

This is not trivial because there is no QJSEngine in HidController. If we go that route we may as well use the existing code path with processInputReport.

[Swiftb0y]

Mhmm, then I'll favor keeping as is and waiting until the Qt issue has been fixed?
Can you explain the rationale for this PR a bit more, I don't currently see why this change is absolutely needed.

[Be-ing]

The current API requires two layers of boilerplate:

this.incomingData(Uint8Array.from(controller.getInputReport(2)));

instead of simply:

controller.getInputReport(2);

[Swiftb0y]

Why do we need to pass the data to this.incomingData?

[JoergAtGithub]

This is one ofthe use cases for getInputReport:
If you want to initialize e.g. the position of the faders, you need to actively poll them at startup after registering the controlls. Otherwise you've to wait until the fader is moved the first time. And because common-hid-packet-parser.js only responses to differences, you need to call IncomingData twice with different data, see an example here:
https://mixxx.discourse.group/t/knob-fader-button-initialization-code-snippet-for-hid-mappings/21482

The other use case is to determine controller internal status bits like the internal/external mixing mode in the Init section of the mapping.

[Swiftb0y]

Ah, so getInputReport causes the controller to report its complete entire state?
In that case that makes sense. Still, I'm opposed to this change because I consider it bad API design when a function called getInputReport which looks like a plain getter, to not return data and only cause a sideffect somewhere else. It may suit the particular usecase, but it makes the code harder to read if you don't know all the details of the API and it may also make other uses harder (as Joerg pointed out to determine other internal controller status that you want to handle in the location
you just called getInputReport and not in incomingData).

[Be-ing]

I think incomingData should be able to handle any data that the controller could send.

[Swiftb0y]

Right, but that was not my point. I was trying to send that I might want to call getInputReport in some scope and then expect to get that in that scope (since it just looks like a plain getter, like getFeatureReport) instead of it calling out to incomingData.

Do you understand my concerns that the proposed change will lead to a less consistent, more convoluted API?

[Be-ing]

What about renaming the function? Maybe requestInputReport?

[Swiftb0y]

that will only obscure the questionable design.

[Swiftb0y]

not having to write 36 extra chars does not legitimize this change.

[JoergAtGithub]

I need a functionality to get the raw data of any InputReport in the Init procedure of the mapping, otherwise I can no longer initialize the mapping to the state of the controller. This is why I implemented this API function.

If there would be only a function requestInputReport, that only calls processInputReport this would be impossible. Because the incomingData function wouldn't be executed until the Init procedure is finished.

[Be-ing]

waiting until the Qt issue has been fixed

The problem with this is that we need to decide on an API before a release is made with this feature.

[Be-ing]

If there would be only a function requestInputReport, that only calls processInputReport this would be impossible. Because the incomingData function wouldn't be executed until the Init procedure is finished.

I don't understand this. incomingData would be called immediately after requestInputReport was called, which the script would have control over.

[Swiftb0y]

If there would be only a function requestInputReport, that only calls processInputReport this would be impossible. Because the incomingData function wouldn't be executed until the Init procedure is finished.

I don't understand this. incomingData would be called immediately after requestInputReport was called, which the script would have control over.

That problem is exactly what I tried to point out earlier. When you call getInputReport you expect to get the input report right then and there as the return value of the function. Just calling incomingData will get you the data, but its passed to a function with a completely different context and would make code more convoluted. The API you are proposing will lead to less flexibility.

To demonstrate, a quick sketch:

const DummyController = {};
DummyController.incomingData = function (bytes) {

}

DummyController.init = function () {
    // need to query controller for some internal state so we can properly initialize the controller
    const buffer = getInputReport(0);
    if (buffer[1] == 0) {/* etc */}
    // ....
}

A usecase like this (which is what @JoergAtGithub described) would result in much more complex and difficult to maintain code.

[Swiftb0y]

The problem with this is that we need to decide on an API before a release is made with this feature.

I agree. Passing a buffer as a QList<int> is not pretty but the proposed requestInputReport alternative is worse.

[Swiftb0y]

I'm sorry if I sound condescending. I don't intend to insult your efforts here but I'm a bit frustrated because of the drama on the other PRs and the fact that I can't seem to convey the consequences of your proposal.

[uklotzde]

I agree with @Swiftb0y that adding hidden side-effects to functions like getInputReport() is the wrong approach. Please don't introduce shortcuts just because it seems convenient at first sight. This will inevitably lead to incomprehensible and unmaintainable code in the long term.

I didn't study the details, but keeping the code separated in two different functions QList<int> getInputReport() (result variant) and void ingestInputReport() (callback variant) seems to be more reasonable even at the cost of introducing some redundancy.

[Holzhaus]

Passing a buffer as a QList<int> is not pretty

By the way, what is the reason for that (getInputReport returning a QList<int>)? These are bytes, right? If so, we should use a QByteArray instead, so that gets converted to ArrayBuffer when passed to JS. That way, you can parse it using the much nicer DataView API.

[Swiftb0y]

@Holzhaus it doesn't work and is apparently a Qt bug, see #4521

[Be-ing]

@Swiftb0y or @Holzhaus maybe you could try getting #4521 to work?

[Be-ing]

I'm sorry if I sound condescending.

Nothing you've said here comes across as condescending.

[Be-ing]

Consensus seems to be against this, so let's reconsider #4521 instead.