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.

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

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

Merged
merged 15 commits into from
Apr 5, 2022

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented Mar 1, 2022

I added a switch to force the sending of HID OutputReports with identical data, as needed by @Be-ing to support the S4 MK3 mapping: #4585 (comment)

After looking at the JavaScript API, I found out, that
Q_INVOKABLE void send(const QList<int>& data, unsigned int length = 0) override
is only there to comply with the inheritance of controller.c. But this function is practically useless. It doesn't send raw bytes as the siblings for MIDI and BULK, because this is a functionality, which the HID protocoll doesn't support. Instead it sends a OutputReport with hardcoded ReportID 0. I wonder, if this function was ever used anywhere.

Than there is
Q_INVOKABLE void send(const QList<int>& data, unsigned int length, unsigned int reportID)
which has already one unused argument (length).

Both send functions use QLIST as argument, while since #4521, the other functions of the JavaScript API use QByteArray.

I added a new API function with QByteArray
Q_INVOKABLE void sendOutputReport(unsigned int reportID, const QByteArray& dataArray, resendUnchangedReport = false)
and extended the old send function backward compatibility:
Q_INVOKABLE void send(const QList<int>& dataList, unsigned int length, unsigned int reportID, resendUnchangedReport = false)
Note: I found no way to make an overloaded function work, which support both QList or QByteArray. Independend of the type in JavaScript, always the same function was called.

Furthermore I noticed, that the comments describing these JavaScript API functions are no longer apply since #4521, I rewrote them (in Doxygen syntax). Especially the returned array of getInputReport became incompatible to the incommingdata function. And this compatibility was the only reason to prepend the reportId to the data array.

Now the HID API functions are:

Unchanged since 2.3:
Q_INVOKABLE void send(const QList<int>& dataList, unsigned int length = 0) override
IMHO this function makes no sense, but I kept it for backward compatibility

Extended since 2.3:
Q_INVOKABLE void send(const QList<int>& dataList, unsigned int length, quint8 reportID, bool resendUnchangedReport = false)
Added optional argument resendUnchangedReport - Note that the Length argument is unused and it uses QList

New in 2.4 and now unified in content and type of the data array:
Q_INVOKABLE void sendOutputReport(quint8 reportID, const QByteArray& dataArray, bool resendUnchangedReport = false)
Q_INVOKABLE QByteArray getInputReport(quint8 reportID)
Q_INVOKABLE void sendFeatureReport(quint8 reportID, const QByteArray& reportData)
Q_INVOKABLE QByteArray getFeatureReport(quint8 reportID)

This is now similar to the basic IO functions in all other HID APIs I know - as well in naming as in order of arguments

-With data as QList or QByteArray
-Without the unused length argument
-With a new argument to disable skipping of OutputReports with redundant data
Removed not working overloaded sendOutputReport with argument type QList
Added missing sentence in comment
src/controllers/hid/hidcontroller.h Outdated Show resolved Hide resolved
src/controllers/hid/hidiooutputreport.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Mar 18, 2022

@JoergAtGithub could you take care of @uklotzde's review comments above?

I would prefer to not add yet another function to the API for this one niche use case.

@Be-ing Be-ing requested a review from uklotzde March 25, 2022 14:57
… (both are introduced after 2.3).

ReportId is now first argument, as in all other HID APIs I know.
-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
@JoergAtGithub JoergAtGithub force-pushed the hidSendDuplicateData branch from cf9d059 to 2c3b119 Compare April 3, 2022 09:13
@JoergAtGithub JoergAtGithub changed the title WIP: HID Switch to force sending of duplicate data and cleanup of send functions HID Switch to force sending of duplicate data and cleanup of send functions Apr 3, 2022
@JoergAtGithub JoergAtGithub marked this pull request as ready for review April 3, 2022 21:39
@JoergAtGithub JoergAtGithub changed the title HID Switch to force sending of duplicate data and cleanup of send functions HID Switch to force sending of duplicate data and cleanup of JS API functions Apr 3, 2022
@daschuer
Copy link
Member

daschuer commented Apr 4, 2022

Does it make sense to adjust res/controllers/common-hid-packet-parser.js to the new API?

src/controllers/hid/hidcontroller.h Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.h Show resolved Hide resolved
src/controllers/hid/hidcontroller.h Show resolved Hide resolved
@JoergAtGithub
Copy link
Member Author

Does it make sense to adjust res/controllers/common-hid-packet-parser.js to the new API?

In fact, this is what I had done for testing this PR. There is only one single line to change:

controller.send(data, data.length, this.reportId);

this needs to be changed to:
controller.sendOutputReport(this.reportId, Uint8Array.from(data).buffer);

@daschuer
Copy link
Member

daschuer commented Apr 4, 2022

In fact, this is what I had done for testing this PR. There is only one single line to change:

For my understanding this saves some CPU cycles due to the QByteArray, right? Would you mind to add this commit?

This PR looks good to me.

@uklotzde: Can we merge?

@JoergAtGithub
Copy link
Member Author

For my understanding this saves some CPU cycles due to the QByteArray, right? Would you mind to add this commit?

I don't know, what's the more efficient implementation. I would prefer to do it in a follow-up PR for common-hid-packet-parser.js

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this follow-up PR. Reordering the parameters to reflect the order within the actual data make sense. Not touching the Q_INVOKABLE functions goes without saying.

The data is copied twice into QByteArray? I didn't check if this behavior is unchanged from before.

LGTM

@uklotzde uklotzde merged commit 2703c31 into mixxxdj:main Apr 5, 2022
@daschuer daschuer added this to the 2.4.0 milestone Jun 21, 2022
JoergAtGithub added a commit to JoergAtGithub/mixxx that referenced this pull request Mar 4, 2023
JoergAtGithub added a commit to JoergAtGithub/mixxx that referenced this pull request Mar 4, 2023
acolombier pushed a commit to acolombier/mixxx that referenced this pull request Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants