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

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

Merged
merged 3 commits into from
Nov 28, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e450c6
HidController: return QByteArrays for get[Input/Feature]Report
Be-ing Nov 14, 2021
c3e9479
HidController: take QByteArray for sendFeatureReport
Be-ing Nov 18, 2021
2bdf17c
HidController: avoid extra copy of data
Be-ing Nov 18, 2021
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
24 changes: 9 additions & 15 deletions src/controllers/hid/hidcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ void HidController::processInputReport(int bytesRead) {
receive(incomingData, mixxx::Time::elapsed());
}

QList<int> HidController::getInputReport(unsigned int reportID) {
QByteArray HidController::getInputReport(unsigned int reportID) {
Trace hidRead("HidController getInputReport");
int bytesRead;

Expand All @@ -185,17 +185,11 @@ QList<int> HidController::getInputReport(unsigned int reportID) {
// Otherwise minimum possible value is 1, because 1 byte is for the reportID,
// the smallest report with data is therefore 2 bytes.
DEBUG_ASSERT(bytesRead <= kReportIdSize);
return QList<int>();
return QByteArray();
}

// Convert array of bytes read in a JavaScript compatible return type
// For compatibility with the array provided by HidController::poll the reportID is contained as prefix
QList<int> dataList;
dataList.reserve(bytesRead);
for (int i = 0; i < bytesRead; i++) {
dataList.append(m_pPollData[m_pollingBufferIndex][i]);
}
return dataList;
return QByteArray::fromRawData(
reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);
Copy link
Member

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@Swiftb0y Swiftb0y Nov 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

}

bool HidController::poll() {
Expand Down Expand Up @@ -287,7 +281,7 @@ ControllerJSProxy* HidController::jsProxy() {
return new HidControllerJSProxy(this);
}

QList<int> HidController::getFeatureReport(
QByteArray HidController::getFeatureReport(
unsigned int reportID) {
unsigned char dataRead[kReportIdSize + kBufferSize];
dataRead[0] = reportID;
Expand Down Expand Up @@ -318,10 +312,10 @@ QList<int> HidController::getFeatureReport(

// Convert array of bytes read in a JavaScript compatible return type
// For compatibility with input array HidController::sendFeatureReport, a reportID prefix is not added here
QList<int> dataList;
dataList.reserve(bytesRead - kReportIdSize);
QByteArray byteArray;
byteArray.reserve(bytesRead - kReportIdSize);
Be-ing marked this conversation as resolved.
Show resolved Hide resolved
for (int i = kReportIdSize; i < bytesRead; i++) {
dataList.append(dataRead[i]);
byteArray[i - 1] = dataRead[i];
}
return dataList;
return byteArray;
}
8 changes: 4 additions & 4 deletions src/controllers/hid/hidcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class HidController final : public Controller {
// as in the polling functionality (including ReportID in first byte).
// The returned list can be used to call the incomingData
// function of the common-hid-packet-parser.
QList<int> getInputReport(unsigned int reportID);
QByteArray getInputReport(unsigned int reportID);

// getFeatureReport receives a feature reports on request.
// HID doesn't support polling feature reports, therefore this is the
Expand All @@ -64,7 +64,7 @@ class HidController final : public Controller {
// changing the other bits. The returned list matches the input
// format of sendFeatureReport, allowing it to be read, modified
// and sent it back to the controller.
QList<int> getFeatureReport(unsigned int reportID);
QByteArray getFeatureReport(unsigned int reportID);

const mixxx::hid::DeviceInfo m_deviceInfo;

Expand Down Expand Up @@ -96,7 +96,7 @@ class HidControllerJSProxy : public ControllerJSProxy {
m_pHidController->sendReport(data, length, reportID);
}

Q_INVOKABLE QList<int> getInputReport(
Q_INVOKABLE QByteArray getInputReport(
unsigned int reportID) {
return m_pHidController->getInputReport(reportID);
}
Expand All @@ -106,7 +106,7 @@ class HidControllerJSProxy : public ControllerJSProxy {
m_pHidController->sendFeatureReport(dataList, reportID);
}

Q_INVOKABLE QList<int> getFeatureReport(
Q_INVOKABLE QByteArray getFeatureReport(
unsigned int reportID) {
return m_pHidController->getFeatureReport(reportID);
}
Expand Down