HidController: return QByteArrays for get[Input/Feature]Report #4521

Be-ing · 2021-11-14T03:20:22Z

for compatibility with HidController::poll

Currently, passing the QList return value to the script's incomingData function requires using Uint8Array.from.

Unfortunately this is not working. Garbage data is returned to JavaScript. I do not understand why this does not work but #4520 does. Maybe it is the call to QJSEngine::toScriptValue that does the trick? But I thought Qt did that automatically... suggestions??

Be-ing · 2021-11-14T03:24:14Z

If we can't figure out how to make this work, I think we should merge #4520.

Be-ing · 2021-11-14T03:34:08Z

I tried copying into the QByteArray with a loop instead of the QByteArray constructor but that didn't work either:

diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp
index 68a4510d0e..8575681a23 100644
--- a/src/controllers/hid/hidcontroller.cpp
+++ b/src/controllers/hid/hidcontroller.cpp
@@ -189,8 +189,12 @@ QByteArray HidController::getInputReport(unsigned int reportID) {
         return QByteArray();
     }
 
-    return QByteArray(
-            reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);
+    QByteArray byteArray;
+    byteArray.reserve(bytesRead);
+    for (int i = 0; i < bytesRead; i++) {
+        byteArray[i] = (m_pPollData[m_pollingBufferIndex][i]);
+    }
+    return byteArray;
 }
 
 bool HidController::poll() {

Swiftb0y · 2021-11-14T13:38:22Z

Were you able to replicate this in a standalone example? If so would you mind filing a bug on Qt Bugtracker?

Be-ing · 2021-11-14T13:48:03Z

I did not make a minimal example to demonstrate this, but yes making a bug report upstream is a good idea.

Be-ing · 2021-11-17T14:55:02Z

I wonder if the issue is that the QByteArray data is not kept alive long enough?

https://doc.qt.io/qt-5/qtqml-cppintegration-data.html#data-ownership

When data is transferred from C++ to QML, the ownership of the data always remains with C++. The exception to this rule is when a QObject is returned from an explicit C++ method call: in this case, the QML engine assumes ownership of the object, unless the ownership of the object has explicitly been set to remain with C++ by invoking QQmlEngine::setObjectOwnership() with QQmlEngine::CppOwnership specified.
Additionally, the QML engine respects the normal QObject parent ownership semantics of Qt C++ objects, and will never delete a QObject instance which has a parent.

QByteArray does not inherit QObject.

Swiftb0y · 2021-11-17T15:10:23Z

Shouldn't be the problem. Currently, the QByteArray is copying the input buffer. So The QByteArray owns the data for the rest of its lifetime. Since we're returning the QByteArray from the Q_INVOCABLE Method, the ownership should be transferred to JS. Maybe the automatic ownership change is broken in Qt?

uklotzde · 2021-11-17T16:10:28Z

This is definitely not an ownership issue but maybe a type mapping issue?

As Jan pointed out the QByteArray is mapped to an ArrayBuffer object in JavaScript, and not to the built-in array type.

I consider it very unlikely that passing of QByteArray to QJSEngine is broken.

Be-ing · 2021-11-17T16:25:58Z

Oh, I understand the issue now. getInputReport returns a QByteArray which does get converted to a JavaScript ArrayBuffer, but ControllerScriptEngineLegacy has a hack for backwards compatibility to transform the ArrayBuffer to a Uint8Array, which requires a QJSEngine... https://github.com/mixxxdj/mixxx/blob/main/src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp#L98

Be-ing · 2021-11-17T16:31:15Z

This still requires two layers of boilerplate:

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

instead of the current situation with QList<int>:

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

I think this is still a good improvement over the current situation even if there is some boilerplate required.

Swiftb0y · 2021-11-17T16:32:37Z

I do not consider that code boilerplate really, but exposing a buffer as an ArrayBuffer instead of an array of ints is definitely an improvement.

Swiftb0y · 2021-11-17T16:33:32Z

So to reiterate, the code itself works right? We just get an ArrayBuffer instead of the Uint8Array we expected?

Be-ing · 2021-11-17T16:33:55Z

We just get an ArrayBuffer instead of the Uint8Array

Yes.

Be-ing · 2021-11-17T16:34:53Z

@JoergAtGithub what do you think about this?

for compatibility with HidController::poll

Holzhaus · 2021-11-17T17:24:30Z

We just get an ArrayBuffer instead of the Uint8Array

Yes.

Great, that is much better than an Uibt8Array, because you can use the DataView API with multi byte int/float support, etc.:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView

JoergAtGithub · 2021-11-17T19:10:39Z

I'm trying to get it work, but struggle a bit:
1.) (Most likely unrelated):
mixxx.exe --controller-debug is no longer working, all HIDDebug outputs are invisible
mixxx.exe --log-level debug is printing them

2.) HIDDebug seems to be incompatible to this type:
Before I could use
HIDDebug(controller.getFeatureReport(0xF1));
now I need
HIDDebug(new Uint8Array(controller.getFeatureReport(0xF1)));
instead

3.) For all bitwise (& | ~) operations on bytes, I need to convert to Uint8Array first. And such operations are needed for nearly any operation on raw HID report. Maybe there is a more elegant solution in JavaScript.

4.) The function sendFeatureReport has still QList as argument and no longer matchs to the output of getFeatureReport. The typical use case is for feature reports is to set or unset a single configuration bit, which worked with QList as following:

    let data =  = controller.getFeatureReport(0xF1);  // Read feature report 0xF1
    data[2] |= 0x08;                                  // Set bit 8 of byte to in report
    controller.sendFeatureReport(data, 0xF1);         // Write feature report back to the controler

But this is a solution I can use. For me it's less comfortable, but if it helps other, I would be fine with this.

Swiftb0y · 2021-11-17T17:31:27Z

src/controllers/hid/hidcontroller.cpp

-    }
-    return dataList;
+    return QByteArray::fromRawData(
+            reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);


reintepret_casts are more dangerous than static_casts.
would this also work?

Suggested change

reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);

static_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);

or maybe

Suggested change

reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);

static_cast<char*>(&m_pPollData[m_pollingBufferIndex]), bytesRead);

Nope, with the first:

/home/be/sw/mixxx/src/controllers/hid/hidcontroller.cpp: In member function ‘QByteArray HidController::getInputReport(unsigned int)’: /home/be/sw/mixxx/src/controllers/hid/hidcontroller.cpp:192:13: error: invalid ‘static_cast’ from type ‘unsigned char [255]’ to type ‘char*’ 192 | static_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

with the second:

/home/be/sw/mixxx/src/controllers/hid/hidcontroller.cpp: In member function ‘QByteArray HidController::getInputReport(unsigned int)’: /home/be/sw/mixxx/src/controllers/hid/hidcontroller.cpp:192:13: error: invalid ‘static_cast’ from type ‘unsigned char (*)[255]’ to type ‘char’ 192 | static_cast<char>(&m_pPollData[m_pollingBufferIndex]), bytesRead); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Ok I looked into the issue. The problem is not the array cast, its the unsigned-to-signed conversion.
https://godbolt.org/z/n974WvWz9

So the reinterpret cast is unavoidable as far as I can tell.

src/controllers/hid/hidcontroller.cpp

Be-ing · 2021-11-18T03:17:25Z

For all bitwise (& | ~) operations on bytes, I need to convert to Uint8Array first.

That is annoying.

The function sendFeatureReport has still QList as argument and no longer matchs to the output of getFeatureReport

Thank you for pointing this out. I pushed a commit to fix this and make sendFeatureReport take a QByteArray. So your JS example would be:

let data =  new Uint8Array(controller.getFeatureReport(0xF1));
data[2] |= 0x08;
controller.sendFeatureReport(data.buffer, 0xF1);

Alternatively you could use a DataView without creating a Uint8Array.

Be-ing · 2021-11-22T04:05:51Z

@JoergAtGithub does the new API work for you now that controller.sendFeatureReport also takes an ArrayBuffer?

JoergAtGithub · 2021-11-22T23:14:43Z

For me personally this works, I could fullfill all my use cases with this too.
But it's more difficult to use than the old solution. Before there was only one use case (calling incommingData function), where the user had to typecast - with this solution he needs a typecast for every use case I'm aware of.

Swiftb0y · 2021-11-22T23:49:49Z

It is very subjective whether a Uint8Array or a DataView is the preferred way of accessing a buffer, for this reason, as far as I'm aware of, the idiomatic design is to pass an ArrayBuffer in your API. Also See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Typed_arrays#web_apis_using_typed_arrays (the only exception is an ImageData.data which takes a Uint8ClampedArray since that is what most graphic APIs use).

Swiftb0y

LGTM
@JoergAtGithub are you fine with the proposed API changes?

JoergAtGithub · 2021-11-28T19:08:36Z

I've no strong opinion here. I found the data list easier to use in the mapping, but if you see benefits in ArrayBuffer, than merge this!

Swiftb0y · 2021-11-28T19:39:42Z

Great, thank you very much. Do you know what mappings are affected by this API change? Would you volunteer to fix them?

JoergAtGithub · 2021-11-28T21:27:27Z

I think only my Z2 mapping, and BEs S4 mapping are affected

-Now the arguments are identical with the similar function getFeatureReport -This array is no longer compatible with incomingData anyway, because the type changed in mixxxdj#4521 -And even before, it was only compatible, under the assumption, that the HID device uses ReportIds Note, that getInputReport is new for 2.4 and can be changed without breaking backward compatibility

Be-ing marked this pull request as draft November 14, 2021 03:20

Be-ing closed this Nov 14, 2021

Be-ing mentioned this pull request Nov 14, 2021

HidController: change getInputReport to automatically call script #4520

Closed

Be-ing reopened this Nov 17, 2021

HidController: return QByteArrays for get[Input/Feature]Report

7e450c6

for compatibility with HidController::poll

Be-ing force-pushed the hid_getInputReport_bytearray branch from 297113b to 7e450c6 Compare November 17, 2021 16:44

Be-ing marked this pull request as ready for review November 17, 2021 16:44

Swiftb0y reviewed Nov 17, 2021

View reviewed changes

HidController: take QByteArray for sendFeatureReport

c3e9479

HidController: avoid extra copy of data

2bdf17c

Swiftb0y approved these changes Nov 28, 2021

View reviewed changes

Swiftb0y merged commit 1209b46 into mixxxdj:main Nov 28, 2021

daschuer mentioned this pull request Dec 1, 2021

Minimixxx controller config #4350

Merged

Be-ing deleted the hid_getInputReport_bytearray branch December 5, 2021 19:23

JoergAtGithub mentioned this pull request Mar 1, 2022

HID Switch to force sending of duplicate data and cleanup of JS API functions #4692

Merged

JoergAtGithub mentioned this pull request May 1, 2023

Declarations for engine and controller scripting API #4759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HidController: return QByteArrays for get[Input/Feature]Report #4521

HidController: return QByteArrays for get[Input/Feature]Report #4521

Be-ing commented Nov 14, 2021

Be-ing commented Nov 14, 2021

Be-ing commented Nov 14, 2021

Swiftb0y commented Nov 14, 2021

Be-ing commented Nov 14, 2021

Be-ing commented Nov 17, 2021

Swiftb0y commented Nov 17, 2021

uklotzde commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Swiftb0y commented Nov 17, 2021

Swiftb0y commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Holzhaus commented Nov 17, 2021

JoergAtGithub commented Nov 17, 2021 •

edited

Loading

Swiftb0y Nov 17, 2021

Be-ing Nov 18, 2021

Swiftb0y Nov 28, 2021

Swiftb0y Nov 28, 2021 •

edited

Loading

Be-ing commented Nov 18, 2021 •

edited

Loading

Be-ing commented Nov 22, 2021

JoergAtGithub commented Nov 22, 2021

Swiftb0y commented Nov 22, 2021

Swiftb0y left a comment

JoergAtGithub commented Nov 28, 2021

Swiftb0y commented Nov 28, 2021

JoergAtGithub commented Nov 28, 2021

	reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);
	static_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);

	reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);
	static_cast<char*>(&m_pPollData[m_pollingBufferIndex]), bytesRead);

HidController: return QByteArrays for get[Input/Feature]Report #4521

HidController: return QByteArrays for get[Input/Feature]Report #4521

Conversation

Be-ing commented Nov 14, 2021

Be-ing commented Nov 14, 2021

Be-ing commented Nov 14, 2021

Swiftb0y commented Nov 14, 2021

Be-ing commented Nov 14, 2021

Be-ing commented Nov 17, 2021

Swiftb0y commented Nov 17, 2021

uklotzde commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Swiftb0y commented Nov 17, 2021

Swiftb0y commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Be-ing commented Nov 17, 2021

Holzhaus commented Nov 17, 2021

JoergAtGithub commented Nov 17, 2021 • edited Loading

Swiftb0y Nov 17, 2021

Choose a reason for hiding this comment

Be-ing Nov 18, 2021

Choose a reason for hiding this comment

Swiftb0y Nov 28, 2021

Choose a reason for hiding this comment

Swiftb0y Nov 28, 2021 • edited Loading

Choose a reason for hiding this comment

Be-ing commented Nov 18, 2021 • edited Loading

Be-ing commented Nov 22, 2021

JoergAtGithub commented Nov 22, 2021

Swiftb0y commented Nov 22, 2021

Swiftb0y left a comment

Choose a reason for hiding this comment

JoergAtGithub commented Nov 28, 2021

Swiftb0y commented Nov 28, 2021

JoergAtGithub commented Nov 28, 2021

JoergAtGithub commented Nov 17, 2021 •

edited

Loading

Swiftb0y Nov 28, 2021 •

edited

Loading

Be-ing commented Nov 18, 2021 •

edited

Loading