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 functions to read back the status of an HID controller (like MIDI SYSEX) #3317

Merged
merged 32 commits into from
Feb 21, 2021
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4745d40
Added HIDAPI version detection in FindHIDAPI.cmake
JoergAtGithub Nov 15, 2020
a9203e3
Fixed CMAKE syntax error
JoergAtGithub Nov 15, 2020
57270d4
Merged in upstream/Main
JoergAtGithub Dec 27, 2020
ec00deb
Merge remote-tracking branch 'upstream/main' into add_HID_readback
JoergAtGithub Dec 31, 2020
c65ea6b
Merge remote-tracking branch 'upstream/main' into add_HID_readback
JoergAtGithub Jan 1, 2021
fe3dce6
Added missing QList reserve statements
JoergAtGithub Jan 1, 2021
e3e6c62
Fixed a signed/unsigned conversion error (QByteArray is an array of s…
JoergAtGithub Jan 2, 2021
77fa9b0
Fixed wrong symbol in CMakeLists.txt
JoergAtGithub Jan 2, 2021
aeaffe2
No double/buffer compare logic as in poll needed for explicit read op…
JoergAtGithub Jan 2, 2021
a1c2a73
Improved messages about returned number of read bytes
JoergAtGithub Jan 3, 2021
1015854
Merge remote-tracking branch 'upstream/main' into add_HID_readback
JoergAtGithub Jan 10, 2021
66feefd
Removed workaround for https://github.com/libusb/hidapi/issues/229
JoergAtGithub Jan 16, 2021
37112d6
Code style fix
JoergAtGithub Jan 16, 2021
f3668bb
Made returned array of getFeatureReport compatible with input array o…
JoergAtGithub Jan 16, 2021
6d5e1bc
Corrected error handling for returned bytesRead value
JoergAtGithub Jan 16, 2021
804bcff
Extracted buffering and de-duplicating code for input reports into a …
JoergAtGithub Jan 16, 2021
722ced8
Update src/controllers/controller.cpp
JoergAtGithub Jan 30, 2021
bad9b11
Renamed FindHIDAPI.cmake before resolving merge conflict
JoergAtGithub Jan 30, 2021
5ab9726
Merge remote-tracking branch 'upstream/Main' into add_HID_readback
JoergAtGithub Jan 30, 2021
16059eb
Merged Findhidapi.cmake and CMakeLists.txt after conflict with upstre…
JoergAtGithub Jan 30, 2021
0a00939
Replaced both function arguments from copy by reference, to copy by v…
JoergAtGithub Feb 2, 2021
ff1a1cd
Removed prefix i for member variables of type integer
JoergAtGithub Feb 2, 2021
17cad5f
Removed the need for local currentIndex variable, by reordering the c…
JoergAtGithub Feb 2, 2021
8263641
Update src/controllers/hid/hidcontroller.cpp
JoergAtGithub Feb 17, 2021
d614250
Update src/controllers/hid/hidcontroller.h
JoergAtGithub Feb 17, 2021
19be7ff
Update src/controllers/hid/hidcontroller.h
JoergAtGithub Feb 17, 2021
ba53b75
Merge branch 'main' into add_HID_readback
JoergAtGithub Feb 17, 2021
79e1392
Changed member variables to camelCase
JoergAtGithub Feb 18, 2021
706ea80
Removed call of incomingData from getInputReport due to the risk of i…
JoergAtGithub Feb 21, 2021
fb7d30d
Added detailed usage description of the getInputReport and getFeature…
JoergAtGithub Feb 21, 2021
cfccc4c
Fixed spelling of comment
JoergAtGithub Feb 21, 2021
54a1f05
Fixed spelling of comment
JoergAtGithub Feb 21, 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
7 changes: 6 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2530,7 +2530,12 @@ find_package(LibUSB)
# USB HID controller support
find_package(hidapi)
option(HID "USB HID controller support" ON)
cmake_dependent_option(HIDAPI_STATIC "Link HIDAPI library statically" OFF "HIDAPI_FOUND" ON)

if(hidapi_VERSION VERSION_LESS "0.10.0")
set(HIDAPI_STATIC ON)
else()
cmake_dependent_option(HIDAPI_STATIC "Link HIDAPI library statically" OFF "HIDAPI_FOUND" ON)
endif()
if(HID)
target_sources(mixxx-lib PRIVATE
src/controllers/hid/hidcontroller.cpp
Expand Down
12 changes: 12 additions & 0 deletions cmake/modules/Findhidapi.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,18 @@ find_package_handle_standard_args(
hidapi_INCLUDE_DIR
)

# Version detection
if (EXISTS "${hidapi_INCLUDE_DIR}/hidapi.h")
file(READ "${hidapi_INCLUDE_DIR}/hidapi.h" hidapi_H_CONTENTS)
string(REGEX MATCH "#define HID_API_VERSION_MAJOR ([0-9]+)" _dummy "${hidapi_H_CONTENTS}")
set(hidapi_VERSION_MAJOR "${CMAKE_MATCH_1}")
string(REGEX MATCH "#define HID_API_VERSION_MINOR ([0-9]+)" _dummy "${hidapi_H_CONTENTS}")
set(hidapi_VERSION_MINOR "${CMAKE_MATCH_1}")
string(REGEX MATCH "#define HID_API_VERSION_PATCH ([0-9]+)" _dummy "${hidapi_H_CONTENTS}")
set(hidapi_VERSION_PATCH "${CMAKE_MATCH_1}")
set(hidapi_VERSION "${hidapi_VERSION_MAJOR}.${hidapi_VERSION_MINOR}.${hidapi_VERSION_PATCH}")
endif ()

if(hidapi_FOUND)
set(hidapi_LIBRARIES "${hidapi_LIBRARY}")
set(hidapi_INCLUDE_DIRS "${hidapi_INCLUDE_DIR}")
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) {
} else {
spacer = QStringLiteral(" ");
}
message += QString::number(data.at(i), 16)
// cast to quint8 to avoid that negative chars are for instance displayed as ffffffff instead of the desired ff
message += QString::number(static_cast<quint8>(data.at(i)), 16)
JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved
.toUpper()
.rightJustified(2, QChar('0')) +
spacer;
Expand Down
134 changes: 107 additions & 27 deletions src/controllers/hid/hidcontroller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ HidController::HidController(
mixxx::hid::DeviceInfo&& deviceInfo)
: m_deviceInfo(std::move(deviceInfo)),
m_pHidDevice(nullptr),
m_iPollingBufferIndex(0) {
m_PollingBufferIndex(0) {
setDeviceCategory(mixxx::hid::DeviceCategory::guessFromDeviceInfo(m_deviceInfo));
setDeviceName(m_deviceInfo.formatName());

Expand Down Expand Up @@ -109,7 +109,7 @@ int HidController::open() {
for (int i = 0; i < kNumBuffers; i++) {
memset(m_pPollData[i], 0, kBufferSize);
}
m_iLastPollSize = 0;
m_LastPollSize = 0;

setOpen(true);
startEngine();
Expand All @@ -136,6 +136,69 @@ int HidController::close() {
return 0;
}

void HidController::processInputReport(int bytesRead) {
Trace process("HidController processInputReport");
unsigned char* pPreviousBuffer = m_pPollData[(m_PollingBufferIndex + 1) % kNumBuffers];
unsigned char* pCurrentBuffer = m_pPollData[m_PollingBufferIndex];
// Some controllers such as the Gemini GMX continuously send input reports even if it
// is identical to the previous input report. If this loop processed all those redundant
// input reports, it would be a big performance problem to run JS code for every input report
// plus it would be unnecessary.
// This assumes that the redundant input reports all use the same report ID. In practice we
// have not encountered any controllers that send redundant input reports with different report
// IDs. If any such devices exist, this may be changed to use a separate buffer to store
// the last input report for each report ID.
if (bytesRead == m_LastPollSize &&
memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) {
return;
}
// Cycle between buffers so the memcmp below does not require deep copying to another buffer.
m_PollingBufferIndex = (m_PollingBufferIndex + 1) % kNumBuffers;
JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved
m_LastPollSize = bytesRead;
auto incomingData = QByteArray::fromRawData(
reinterpret_cast<char*>(pCurrentBuffer), bytesRead);
receive(incomingData, mixxx::Time::elapsed());
Copy link
Member

Choose a reason for hiding this comment

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

The time should be measured as early as possible to a void a jitter do to suspends in the code before. The best would be to ask hidapi for it. But it looks like that is not implemented, is it?

Copy link
Member

Choose a reason for hiding this comment

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

By the way, we have no prefix rule for i.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the prefix i !
I'm not sure about what should be changed on the time measurement. I didn't wrote this code I just moved it on the request from uklotze above, to get rid of the code duplication. What should be measured here - the execution time of the JavaScript call which is implemented in receive, or the execution time of HIDAPI, which is called outside of this function?

}

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

m_pPollData[m_PollingBufferIndex][0] = reportID;
bytesRead = hid_get_input_report(m_pHidDevice, m_pPollData[m_PollingBufferIndex], kBufferSize);

controllerDebug(bytesRead
<< "bytes received by hid_get_input_report" << getName()
<< "serial #" << m_deviceInfo.serialNumber()
<< "(including one byte for the report ID:"
<< QString::number(static_cast<quint8>(reportID), 16)
.toUpper()
.rightJustified(2, QChar('0'))
<< ")")

if (bytesRead <= kReportIdSize) {
// -1 is the only error value according to hidapi documentation.
// 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>();
}

// Convert array of bytes read in a JavaScript compatible return type
// For compatibilty with the array provided by HidController::poll the reportID is contained as prefix
QList<int> dataList;
JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved
dataList.reserve(bytesRead);
for (int i = 0; i < bytesRead; i++) {
dataList.append(m_pPollData[m_PollingBufferIndex][i]);
}

// Execute callback function in JavaScript mapping
// and print to stdout in case of --controllerDebug
Be-ing marked this conversation as resolved.
Show resolved Hide resolved
HidController::processInputReport(bytesRead);
JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved

return dataList;
}

bool HidController::poll() {
Trace hidRead("HidController poll");

Expand All @@ -145,38 +208,16 @@ bool HidController::poll() {
// There is no safety net for this because it has not been demonstrated to be
// a problem in practice.
while (true) {
// Cycle between buffers so the memcmp below does not require deep copying to another buffer.
unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex];
const int currentBufferIndex = (m_iPollingBufferIndex + 1) % kNumBuffers;
unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex];

int bytesRead = hid_read(m_pHidDevice, pCurrentBuffer, kBufferSize);
int bytesRead = hid_read(m_pHidDevice, m_pPollData[m_PollingBufferIndex], kBufferSize);
if (bytesRead < 0) {
// -1 is the only error value according to hidapi documentation.
DEBUG_ASSERT(bytesRead == -1);
return false;
} else if (bytesRead == 0) {
// No packet was available to be read
return true;
}

Trace process("HidController process packet");
// Some controllers such as the Gemini GMX continuously send input packets even if it
// is identical to the previous packet. If this loop processed all those redundant
// packets, it would be a big performance problem to run JS code for every packet and
// would be unnecessary.
// This assumes that the redundant packets all use the same report ID. In practice we
// have not encountered any controllers that send redundant packets with different report
// IDs. If any such devices exist, this may be changed to use a separate buffer to store
// the last packet for each report ID.
if (bytesRead == m_iLastPollSize &&
memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) {
continue;
}
m_iLastPollSize = bytesRead;
m_iPollingBufferIndex = currentBufferIndex;
auto incomingData = QByteArray::fromRawData(
reinterpret_cast<char*>(pCurrentBuffer), bytesRead);
receive(incomingData, mixxx::Time::elapsed());
processInputReport(bytesRead);
}
}

Expand Down Expand Up @@ -254,3 +295,42 @@ void HidController::sendFeatureReport(
ControllerJSProxy* HidController::jsProxy() {
return new HidControllerJSProxy(this);
}

QList<int> HidController::getFeatureReport(
unsigned int reportID) {
unsigned char dataRead[kReportIdSize + kBufferSize];
Be-ing marked this conversation as resolved.
Show resolved Hide resolved
dataRead[0] = reportID;

int bytesRead;
bytesRead = hid_get_feature_report(m_pHidDevice,
dataRead,
kReportIdSize + kBufferSize);
if (bytesRead <= kReportIdSize) {
// -1 is the only error value according to hidapi documentation.
// Otherwise minimum possible value is 1, because 1 byte is for the reportID,
// the smallest report with data is therefore 2 bytes.
qWarning() << "getFeatureReport is unable to get data from" << getName()
<< "serial #" << m_deviceInfo.serialNumber() << ":"
<< mixxx::convertWCStringToQString(
hid_error(m_pHidDevice),
kMaxHidErrorMessageSize);
} else {
controllerDebug(bytesRead
<< "bytes received by getFeatureReport from" << getName()
<< "serial #" << m_deviceInfo.serialNumber()
<< "(including one byte for the report ID:"
<< QString::number(static_cast<quint8>(reportID), 16)
.toUpper()
.rightJustified(2, QChar('0'))
<< ")")
}

// Convert array of bytes read in a JavaScript compatible return type
// For compatibilty with input array HidController::sendFeatureReport, a reportID prefix is not added here
QList<int> dataList;
dataList.reserve(bytesRead - kReportIdSize);
for (int i = kReportIdSize; i < bytesRead; i++) {
dataList.append(dataRead[i]);
}
return dataList;
}
18 changes: 16 additions & 2 deletions src/controllers/hid/hidcontroller.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,17 @@ class HidController final : public Controller {

private:
bool isPolling() const override;
void processInputReport(int bytesRead);

// For devices which only support a single report, reportID must be set to
// 0x0.
void sendBytes(const QByteArray& data) override;
void sendBytesReport(QByteArray data, unsigned int reportID);
void sendFeatureReport(const QList<int>& dataList, unsigned int reportID);

QList<int> getInputReport(unsigned int reportID);
QList<int> getFeatureReport(unsigned int reportID);
JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved

const mixxx::hid::DeviceInfo m_deviceInfo;

hid_device* m_pHidDevice;
Expand All @@ -55,8 +59,8 @@ class HidController final : public Controller {
static constexpr int kNumBuffers = 2;
static constexpr int kBufferSize = 255;
unsigned char m_pPollData[kNumBuffers][kBufferSize];
int m_iLastPollSize;
int m_iPollingBufferIndex;
int m_lastPollSize;
JoergAtGithub marked this conversation as resolved.
Show resolved Hide resolved
int m_pollingBufferIndex;

friend class HidControllerJSProxy;
};
Expand All @@ -77,11 +81,21 @@ class HidControllerJSProxy : public ControllerJSProxy {
m_pHidController->sendReport(data, length, reportID);
}

Q_INVOKABLE QList<int> getInputReport(
unsigned int reportID) {
return m_pHidController->getInputReport(reportID);
}

Q_INVOKABLE void sendFeatureReport(
const QList<int>& dataList, unsigned int reportID) {
m_pHidController->sendFeatureReport(dataList, reportID);
}

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

private:
HidController* m_pHidController;
};