From 4745d40414ca3385797acba8f2b52b504c89452e Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sun, 15 Nov 2020 11:21:00 +0100 Subject: [PATCH 01/27] Added HIDAPI version detection in FindHIDAPI.cmake Force static linkage if system HIDAP is older than 0.10.0 --- CMakeLists.txt | 7 ++++++- cmake/modules/FindHIDAPI.cmake | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 81175092ae1..7dbfedd2465 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2515,7 +2515,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(CMAKE_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 diff --git a/cmake/modules/FindHIDAPI.cmake b/cmake/modules/FindHIDAPI.cmake index 7f2f2fa6ed6..14e45645e89 100644 --- a/cmake/modules/FindHIDAPI.cmake +++ b/cmake/modules/FindHIDAPI.cmake @@ -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}") From a9203e3f291d31f2e476414644aff2b81c84ca52 Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Sun, 15 Nov 2020 13:36:57 +0100 Subject: [PATCH 02/27] Fixed CMAKE syntax error Suggested by Holzhaus Co-authored-by: Jan Holthuis --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7dbfedd2465..33179165ae1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2518,7 +2518,7 @@ option(HID "USB HID controller support" ON) if(CMAKE_VERSION VERSION_LESS "0.10.0") set(HIDAPI_STATIC ON) -else +else() cmake_dependent_option(HIDAPI_STATIC "Link HIDAPI library statically" OFF "HIDAPI_FOUND" ON) endif() if(HID) From 57270d4e62cd5f8a528f4c20a9c3048418d4ddf0 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Mon, 28 Dec 2020 00:32:30 +0100 Subject: [PATCH 03/27] Merged in upstream/Main --- src/controllers/hid/hidcontroller.cpp | 56 +++++++++++++++++++++++++++ src/controllers/hid/hidcontroller.h | 16 +++++++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 917387245b7..0a459864e7b 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -137,6 +137,35 @@ int HidController::close() { return 0; } +QList HidController::getInputReport(unsigned int reportID) { + Trace hidRead("HidController getInputReport"); + unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex]; + const int currentBufferIndex = (m_iPollingBufferIndex + 1) % kNumBuffers; + unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex]; + int bytesRead; + pCurrentBuffer[0] = reportID; + bytesRead = hid_get_input_report(m_pHidDevice, pCurrentBuffer, kBufferSize); + controllerDebug(bytesRead << "bytes received by hid_get_input_report" << getName() + << "serial #" << m_deviceInfo.serialNumber() + << "(including report ID of" << reportID << ")"); + bytesRead -= kReportIdSize; + + if (bytesRead == m_iLastPollSize && + memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) { + } else { + m_iLastPollSize = bytesRead; + m_iPollingBufferIndex = currentBufferIndex; + auto incomingData = QByteArray::fromRawData( + reinterpret_cast(pCurrentBuffer), bytesRead); + receive(incomingData, mixxx::Time::elapsed()); + } + QList dataList; + for (int i = 0; i < bytesRead; i++) { + dataList.append(pCurrentBuffer[i]); + } + return dataList; +} + bool HidController::poll() { Trace hidRead("HidController poll"); @@ -255,3 +284,30 @@ void HidController::sendFeatureReport( ControllerJSProxy* HidController::jsProxy() { return new HidControllerJSProxy(this); } + +QList HidController::getFeatureReport( + unsigned int reportID) { + unsigned char dataRead[kReportIdSize + kBufferSize]; + dataRead[0] = reportID; + + int bytesRead; + bytesRead = hid_get_feature_report(m_pHidDevice, + dataRead, + kReportIdSize + kBufferSize); + if (bytesRead == -1) { + 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 report ID of" << reportID << ")"); + } + QList dataList; + for (int i = 0; i < bytesRead; i++) { + dataList.append(dataRead[i]); + } + return dataList; +} diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 1f08b9945e9..085fe41f69f 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -55,8 +55,10 @@ class HidController final : public Controller { void sendBytesReport(QByteArray data, unsigned int reportID); void sendFeatureReport(const QList& dataList, unsigned int reportID); - // Returns a pointer to the currently loaded controller mapping. For internal - // use only. + QList getInputReport(unsigned int reportID); + QList getFeatureReport(unsigned int reportID); + + // Returns a pointer to the currently loaded controller mapping. For internal // use only. LegacyControllerMapping* mapping() override { return &m_mapping; } @@ -91,11 +93,21 @@ class HidControllerJSProxy : public ControllerJSProxy { m_pHidController->sendReport(data, length, reportID); } + Q_INVOKABLE QList getInputReport( + unsigned int reportID) { + return m_pHidController->getInputReport(reportID); + } + Q_INVOKABLE void sendFeatureReport( const QList& dataList, unsigned int reportID) { m_pHidController->sendFeatureReport(dataList, reportID); } + Q_INVOKABLE QList getFeatureReport( + unsigned int reportID) { + return m_pHidController->getFeatureReport(reportID); + } + private: HidController* m_pHidController; }; From fe3dce67e0a484cdd709c20bc3bc0291d0cc0f87 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Fri, 1 Jan 2021 22:18:56 +0100 Subject: [PATCH 04/27] Added missing QList reserve statements --- src/controllers/hid/hidcontroller.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 0a459864e7b..17ea09fbe1e 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -160,6 +160,7 @@ QList HidController::getInputReport(unsigned int reportID) { receive(incomingData, mixxx::Time::elapsed()); } QList dataList; + dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { dataList.append(pCurrentBuffer[i]); } @@ -306,6 +307,7 @@ QList HidController::getFeatureReport( << "(including report ID of" << reportID << ")"); } QList dataList; + dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { dataList.append(dataRead[i]); } From e3e6c62bf836207e9b5b4dbcd5550f63f6940c7a Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 2 Jan 2021 17:18:46 +0100 Subject: [PATCH 05/27] Fixed a signed/unsigned conversion error (QByteArray is an array of signed chars, but QString::number expects an unsigned 8 bit integer) --- src/controllers/controller.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/controllers/controller.cpp b/src/controllers/controller.cpp index 35d1e9517ed..459fe3d945a 100644 --- a/src/controllers/controller.cpp +++ b/src/controllers/controller.cpp @@ -125,7 +125,8 @@ void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) { } else { spacer = QStringLiteral(" "); } - message += QString::number(data.at(i), 16) + // QByteArray is an array of signed 8 bit chars, but QString::number needs an unsigned 8 bit integer + message += QString::number(static_cast(data.at(i)), 16) .toUpper() .rightJustified(2, QChar('0')) + spacer; From 77fa9b0f6693150123638ba24b712a0624ccc863 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 2 Jan 2021 17:37:07 +0100 Subject: [PATCH 06/27] Fixed wrong symbol in CMakeLists.txt --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f9b5bda65dd..a2f30e0890d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2531,7 +2531,7 @@ find_package(LibUSB) find_package(HIDAPI) option(HID "USB HID controller support" ON) -if(CMAKE_VERSION VERSION_LESS "0.10.0") +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) From aeaffe2c1278fae58ea9079dd5aac1854a0f41e3 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sun, 3 Jan 2021 00:26:43 +0100 Subject: [PATCH 07/27] No double/buffer compare logic as in poll needed for explicit read operation --- CMakeLists.txt | 7 +++++- src/controllers/hid/hidcontroller.cpp | 32 +++++++++++++++------------ 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a2f30e0890d..dd866e448c9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -108,7 +108,7 @@ if(MSVC) # http://msdn.microsoft.com/en-us/library/59a3b321.aspx # In general, you should pick /O2 over /Ox - add_compile_options($<$>:/O2>) + add_compile_options($<$>:/Od>) # Remove /RTC1 flag (conflicts with /O2) string(REGEX REPLACE "/RTC[^ ]*" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") @@ -1140,6 +1140,11 @@ add_executable(mixxx WIN32 src/main.cpp) set_target_properties(mixxx-lib PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY}") target_link_libraries(mixxx PUBLIC mixxx-lib) +# Visual Studio Debugger startup options + set_target_properties(mixxx PROPERTIES LINK_FLAGS_RELWITHDEBINFO "/SUBSYSTEM:CONSOLE") + set_target_properties(mixxx PROPERTIES COMPILE_DEFINITIONS_RELWITHDEBINFO "_CONSOLE") + #set_target_properties(mixxx PROPERTIES VS_DEBUGGER_COMMAND_ARGUMENTS "--developer") + # # Installation and Packaging # diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 17ea09fbe1e..d12daa55701 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -139,30 +139,32 @@ int HidController::close() { QList HidController::getInputReport(unsigned int reportID) { Trace hidRead("HidController getInputReport"); - unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex]; - const int currentBufferIndex = (m_iPollingBufferIndex + 1) % kNumBuffers; - unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex]; + unsigned char CurrentBuffer[kBufferSize]; int bytesRead; - pCurrentBuffer[0] = reportID; - bytesRead = hid_get_input_report(m_pHidDevice, pCurrentBuffer, kBufferSize); + CurrentBuffer[0] = reportID; + bytesRead = hid_get_input_report(m_pHidDevice, CurrentBuffer, kBufferSize); controllerDebug(bytesRead << "bytes received by hid_get_input_report" << getName() << "serial #" << m_deviceInfo.serialNumber() << "(including report ID of" << reportID << ")"); bytesRead -= kReportIdSize; - if (bytesRead == m_iLastPollSize && - memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) { - } else { - m_iLastPollSize = bytesRead; - m_iPollingBufferIndex = currentBufferIndex; - auto incomingData = QByteArray::fromRawData( - reinterpret_cast(pCurrentBuffer), bytesRead); - receive(incomingData, mixxx::Time::elapsed()); + if (bytesRead < 0) { + // -1 is the only error value according to hidapi documentation. Otherwise minimum possible value is 1, because 1 byte is for the reportID. + DEBUG_ASSERT(bytesRead < 0); + return QList(); } + + // Execute callback function in JavaScript mapping + // and print to stdout in case of --controllerDebug + auto incomingData = QByteArray::fromRawData( + reinterpret_cast(CurrentBuffer), bytesRead); + receive(incomingData, mixxx::Time::elapsed()); + + // Convert array of bytes read in a JavaScript compatible return type QList dataList; dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { - dataList.append(pCurrentBuffer[i]); + dataList.append(CurrentBuffer[i]); } return dataList; } @@ -306,6 +308,8 @@ QList HidController::getFeatureReport( << "serial #" << m_deviceInfo.serialNumber() << "(including report ID of" << reportID << ")"); } + + // Convert array of bytes read in a JavaScript compatible return type QList dataList; dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { From a1c2a73215316ecf2e013775f62e083236f676ce Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sun, 3 Jan 2021 22:38:48 +0100 Subject: [PATCH 08/27] Improved messages about returned number of read bytes --- src/controllers/hid/hidcontroller.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index d12daa55701..9574353ec8e 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -143,10 +143,14 @@ QList HidController::getInputReport(unsigned int reportID) { int bytesRead; CurrentBuffer[0] = reportID; bytesRead = hid_get_input_report(m_pHidDevice, CurrentBuffer, kBufferSize); + + // This does not match to the HIDAPI documentation, but without the decrement, I get no data consistent with HidController::poll() (tested only on Windows7) + bytesRead -= 1; + controllerDebug(bytesRead << "bytes received by hid_get_input_report" << getName() - << "serial #" << m_deviceInfo.serialNumber() - << "(including report ID of" << reportID << ")"); - bytesRead -= kReportIdSize; + << "serial #" << m_deviceInfo.serialNumber() + << "(including one byte for the report ID:" << QString::number(static_cast(reportID), 16).toUpper().rightJustified(2, QChar('0')) << ")" + ) if (bytesRead < 0) { // -1 is the only error value according to hidapi documentation. Otherwise minimum possible value is 1, because 1 byte is for the reportID. @@ -189,6 +193,7 @@ bool HidController::poll() { DEBUG_ASSERT(bytesRead == -1); return false; } else if (bytesRead == 0) { + // No packet was available to be read return true; } @@ -306,7 +311,7 @@ QList HidController::getFeatureReport( } else { controllerDebug(bytesRead << "bytes received by getFeatureReport from" << getName() << "serial #" << m_deviceInfo.serialNumber() - << "(including report ID of" << reportID << ")"); + << "(including one byte for the report ID:" << QString::number(static_cast(reportID), 16).toUpper().rightJustified(2, QChar('0')) << ")") } // Convert array of bytes read in a JavaScript compatible return type From 66feefdb5634125835feb49362a6ed99f44e0ef8 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 16 Jan 2021 10:42:56 +0100 Subject: [PATCH 09/27] Removed workaround for https://github.com/libusb/hidapi/issues/229 --- src/controllers/hid/hidcontroller.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 9574353ec8e..767f8714afa 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -144,9 +144,6 @@ QList HidController::getInputReport(unsigned int reportID) { CurrentBuffer[0] = reportID; bytesRead = hid_get_input_report(m_pHidDevice, CurrentBuffer, kBufferSize); - // This does not match to the HIDAPI documentation, but without the decrement, I get no data consistent with HidController::poll() (tested only on Windows7) - bytesRead -= 1; - 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(reportID), 16).toUpper().rightJustified(2, QChar('0')) << ")" From 37112d6a7436b1a4a08a9ca6a20cfe332dcb8301 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 16 Jan 2021 10:47:49 +0100 Subject: [PATCH 10/27] Code style fix --- src/controllers/hid/hidcontroller.cpp | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 767f8714afa..a4e8e03b32f 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -144,12 +144,16 @@ QList HidController::getInputReport(unsigned int reportID) { CurrentBuffer[0] = reportID; bytesRead = hid_get_input_report(m_pHidDevice, CurrentBuffer, 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(reportID), 16).toUpper().rightJustified(2, QChar('0')) << ")" - ) - - if (bytesRead < 0) { + 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(reportID), 16) + .toUpper() + .rightJustified(2, QChar('0')) + << ")") + + if (bytesRead < 0) { // -1 is the only error value according to hidapi documentation. Otherwise minimum possible value is 1, because 1 byte is for the reportID. DEBUG_ASSERT(bytesRead < 0); return QList(); @@ -306,9 +310,14 @@ QList HidController::getFeatureReport( 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(reportID), 16).toUpper().rightJustified(2, QChar('0')) << ")") + controllerDebug(bytesRead + << "bytes received by getFeatureReport from" << getName() + << "serial #" << m_deviceInfo.serialNumber() + << "(including one byte for the report ID:" + << QString::number(static_cast(reportID), 16) + .toUpper() + .rightJustified(2, QChar('0')) + << ")") } // Convert array of bytes read in a JavaScript compatible return type From f3668bb8642b47c45128f5bde77b013cb1e5280e Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 16 Jan 2021 12:32:29 +0100 Subject: [PATCH 11/27] Made returned array of getFeatureReport compatible with input array of sendFeatureReport --- src/controllers/hid/hidcontroller.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index a4e8e03b32f..edff9255b4f 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -166,6 +166,7 @@ QList HidController::getInputReport(unsigned int reportID) { receive(incomingData, mixxx::Time::elapsed()); // 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 dataList; dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { @@ -321,9 +322,10 @@ QList HidController::getFeatureReport( } // 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 dataList; - dataList.reserve(bytesRead); - for (int i = 0; i < bytesRead; i++) { + dataList.reserve(bytesRead - kReportIdSize); + for (int i = kReportIdSize; i < bytesRead; i++) { dataList.append(dataRead[i]); } return dataList; From 6d5e1bc406d83a81fb7391d296d83afe14067b0f Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 16 Jan 2021 17:00:07 +0100 Subject: [PATCH 12/27] Corrected error handling for returned bytesRead value --- src/controllers/hid/hidcontroller.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index edff9255b4f..dbb62551e21 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -153,9 +153,11 @@ QList HidController::getInputReport(unsigned int reportID) { .rightJustified(2, QChar('0')) << ")") - if (bytesRead < 0) { - // -1 is the only error value according to hidapi documentation. Otherwise minimum possible value is 1, because 1 byte is for the reportID. - DEBUG_ASSERT(bytesRead < 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(); } @@ -304,7 +306,10 @@ QList HidController::getFeatureReport( bytesRead = hid_get_feature_report(m_pHidDevice, dataRead, kReportIdSize + kBufferSize); - if (bytesRead == -1) { + 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( From 804bcffaf925d1223354256bb3b7dcba0d37a45f Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 16 Jan 2021 18:19:14 +0100 Subject: [PATCH 13/27] Extracted buffering and de-duplicating code for input reports into a private member function --- CMakeLists.txt | 7 +-- src/controllers/hid/hidcontroller.cpp | 62 ++++++++++++++------------- src/controllers/hid/hidcontroller.h | 3 +- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 126ab8977bc..57cbdffce16 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -108,7 +108,7 @@ if(MSVC) # http://msdn.microsoft.com/en-us/library/59a3b321.aspx # In general, you should pick /O2 over /Ox - add_compile_options($<$>:/Od>) + add_compile_options($<$>:/O2>) # Remove /RTC1 flag (conflicts with /O2) string(REGEX REPLACE "/RTC[^ ]*" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}") @@ -1136,11 +1136,6 @@ add_executable(mixxx WIN32 src/main.cpp) set_target_properties(mixxx-lib PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY}") target_link_libraries(mixxx PUBLIC mixxx-lib) -# Visual Studio Debugger startup options - set_target_properties(mixxx PROPERTIES LINK_FLAGS_RELWITHDEBINFO "/SUBSYSTEM:CONSOLE") - set_target_properties(mixxx PROPERTIES COMPILE_DEFINITIONS_RELWITHDEBINFO "_CONSOLE") - #set_target_properties(mixxx PROPERTIES VS_DEBUGGER_COMMAND_ARGUMENTS "--developer") - # # Installation and Packaging # diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index dbb62551e21..d4c2c60c8c1 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -137,12 +137,38 @@ int HidController::close() { return 0; } +void HidController::processInputReport(int& bytesRead, const int& currentBufferIndex) { + Trace process("HidController processInputReport"); + unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex]; + unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex]; + // Some controllers such as the Gemini GMX continuously send input reports even if it + // is identical to the previous send input report. If this loop processed all those redundant + // input report, it would be a big performance problem to run JS code for every input report and + // would be unnecessary. + // This assumes that the redundant input report all use the same report ID. In practice we + // have not encountered any controllers that send redundant input report 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_iLastPollSize && + memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) { + return; + } + m_iLastPollSize = bytesRead; + m_iPollingBufferIndex = currentBufferIndex; + auto incomingData = QByteArray::fromRawData( + reinterpret_cast(pCurrentBuffer), bytesRead); + receive(incomingData, mixxx::Time::elapsed()); +} + QList HidController::getInputReport(unsigned int reportID) { Trace hidRead("HidController getInputReport"); - unsigned char CurrentBuffer[kBufferSize]; int bytesRead; - CurrentBuffer[0] = reportID; - bytesRead = hid_get_input_report(m_pHidDevice, CurrentBuffer, kBufferSize); + + // Cycle between buffers so the memcmp below does not require deep copying to another buffer. + const int currentBufferIndex = (m_iPollingBufferIndex + 1) % kNumBuffers; + + m_pPollData[currentBufferIndex][0] = reportID; + bytesRead = hid_get_input_report(m_pHidDevice, m_pPollData[currentBufferIndex], kBufferSize); controllerDebug(bytesRead << "bytes received by hid_get_input_report" << getName() @@ -163,16 +189,14 @@ QList HidController::getInputReport(unsigned int reportID) { // Execute callback function in JavaScript mapping // and print to stdout in case of --controllerDebug - auto incomingData = QByteArray::fromRawData( - reinterpret_cast(CurrentBuffer), bytesRead); - receive(incomingData, mixxx::Time::elapsed()); + HidController::processInputReport(bytesRead, currentBufferIndex); // 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 dataList; dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { - dataList.append(CurrentBuffer[i]); + dataList.append(m_pPollData[currentBufferIndex][i]); } return dataList; } @@ -187,11 +211,9 @@ bool HidController::poll() { // 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[currentBufferIndex], kBufferSize); if (bytesRead < 0) { // -1 is the only error value according to hidapi documentation. DEBUG_ASSERT(bytesRead == -1); @@ -200,25 +222,7 @@ bool HidController::poll() { // 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(pCurrentBuffer), bytesRead); - receive(incomingData, mixxx::Time::elapsed()); + processInputReport(bytesRead, currentBufferIndex); } } diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 085fe41f69f..d6beec5b65a 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -48,6 +48,7 @@ class HidController final : public Controller { private: bool isPolling() const override; + void processInputReport(int& bytesRead, const int& currentBufferIndex); // For devices which only support a single report, reportID must be set to // 0x0. @@ -58,7 +59,7 @@ class HidController final : public Controller { QList getInputReport(unsigned int reportID); QList getFeatureReport(unsigned int reportID); - // Returns a pointer to the currently loaded controller mapping. For internal // use only. + // Returns a pointer to the currently loaded controller mapping. For internal use only. LegacyControllerMapping* mapping() override { return &m_mapping; } From 722ced860fe23bee98a26435840f09ffeefa9dba Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Sat, 30 Jan 2021 12:52:33 +0100 Subject: [PATCH 14/27] Update src/controllers/controller.cpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improved comment text as suggested by daschuer Co-authored-by: Daniel Schürmann --- src/controllers/controller.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/controller.cpp b/src/controllers/controller.cpp index 459fe3d945a..dc56f4a6181 100644 --- a/src/controllers/controller.cpp +++ b/src/controllers/controller.cpp @@ -125,7 +125,7 @@ void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) { } else { spacer = QStringLiteral(" "); } - // QByteArray is an array of signed 8 bit chars, but QString::number needs an unsigned 8 bit integer + // cast to quint8 to avoid that negative chars are for instance displayed as ffffffff instead of the desired ff message += QString::number(static_cast(data.at(i)), 16) .toUpper() .rightJustified(2, QChar('0')) + From bad9b114f41b1cdd6deb736d948432cb4a471465 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 30 Jan 2021 13:03:14 +0100 Subject: [PATCH 15/27] Renamed FindHIDAPI.cmake before resolving merge conflict --- cmake/modules/FindHIDAPI.cmake | 99 ---------------------------------- 1 file changed, 99 deletions(-) delete mode 100644 cmake/modules/FindHIDAPI.cmake diff --git a/cmake/modules/FindHIDAPI.cmake b/cmake/modules/FindHIDAPI.cmake deleted file mode 100644 index 14e45645e89..00000000000 --- a/cmake/modules/FindHIDAPI.cmake +++ /dev/null @@ -1,99 +0,0 @@ -# This file is part of Mixxx, Digital DJ'ing software. -# Copyright (C) 2001-2020 Mixxx Development Team -# Distributed under the GNU General Public Licence (GPL) version 2 or any later -# later version. See the LICENSE file for details. - -#[=======================================================================[.rst: -FindHIDAPI ----------- - -Finds the HIDAPI library. - -Imported Targets -^^^^^^^^^^^^^^^^ - -This module provides the following imported targets, if found: - -``HIDAPI::libusb`` - The hidapi-libusb library - -Result Variables -^^^^^^^^^^^^^^^^ - -This will define the following variables: - -``HIDAPI_FOUND`` - True if the system has the HIDAPI library. -``HIDAPI_INCLUDE_DIRS`` - Include directories needed to use HIDAPI. -``HIDAPI_LIBRARIES`` - Libraries needed to link to HIDAPI. -``HIDAPI_DEFINITIONS`` - Compile definitions needed to use HIDAPI. - -Cache Variables -^^^^^^^^^^^^^^^ - -The following cache variables may also be set: - -``HIDAPI_INCLUDE_DIR`` - The directory containing ``hidapi/hidapi.h``. -``HIDAPI_LIBRARY`` - The path to the hidapi-lbusb library. - -#]=======================================================================] - -find_package(PkgConfig QUIET) -if(PkgConfig_FOUND) - pkg_check_modules(PC_HIDAPI QUIET hidapi-libusb) -endif() - -find_path(HIDAPI_INCLUDE_DIR - NAMES hidapi.h - PATHS ${PC_HIDAPI_INCLUDE_DIRS} - PATH_SUFFIXES hidapi - DOC "HIDAPI include directory") -mark_as_advanced(HIDAPI_INCLUDE_DIR) - -find_library(HIDAPI_LIBRARY - NAMES hidapi-libusb - PATHS ${PC_HIDAPI_LIBRARY_DIRS} - DOC "HIDAPI library" -) -mark_as_advanced(HIDAPI_LIBRARY) - -include(FindPackageHandleStandardArgs) -find_package_handle_standard_args( - HIDAPI - DEFAULT_MSG - HIDAPI_LIBRARY - 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}") - set(HIDAPI_DEFINITIONS ${PC_HIDAPI_CFLAGS_OTHER}) - - if(NOT TARGET HIDAPI::LibUSB) - add_library(HIDAPI::LibUSB UNKNOWN IMPORTED) - set_target_properties(HIDAPI::LibUSB - PROPERTIES - IMPORTED_LOCATION "${HIDAPI_LIBRARY}" - INTERFACE_COMPILE_OPTIONS "${PC_HIDAPI_CFLAGS_OTHER}" - INTERFACE_INCLUDE_DIRECTORIES "${HIDAPI_INCLUDE_DIR}" - ) - endif() -endif() From 16059ebed883baa93d84dc43360c73e1dff80513 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sat, 30 Jan 2021 13:12:30 +0100 Subject: [PATCH 16/27] Merged Findhidapi.cmake and CMakeLists.txt after conflict with upstream/main --- CMakeLists.txt | 2 +- cmake/modules/Findhidapi.cmake | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9d2c28f5656..4488ce10c37 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2525,7 +2525,7 @@ find_package(LibUSB) find_package(hidapi) option(HID "USB HID controller support" ON) -if(HIDAPI_VERSION VERSION_LESS "0.10.0") +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) diff --git a/cmake/modules/Findhidapi.cmake b/cmake/modules/Findhidapi.cmake index 89847675831..f1ca6ab3636 100644 --- a/cmake/modules/Findhidapi.cmake +++ b/cmake/modules/Findhidapi.cmake @@ -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}") From 0a00939fef2bd0f65d28d483b0e7a9f09ce66833 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Tue, 2 Feb 2021 19:55:03 +0100 Subject: [PATCH 17/27] Replaced both function arguments from copy by reference, to copy by value --- src/controllers/hid/hidcontroller.cpp | 2 +- src/controllers/hid/hidcontroller.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index d4c2c60c8c1..1b466436926 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -137,7 +137,7 @@ int HidController::close() { return 0; } -void HidController::processInputReport(int& bytesRead, const int& currentBufferIndex) { +void HidController::processInputReport(int bytesRead, const int currentBufferIndex) { Trace process("HidController processInputReport"); unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex]; unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex]; diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index d6beec5b65a..a22d6146889 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -48,7 +48,7 @@ class HidController final : public Controller { private: bool isPolling() const override; - void processInputReport(int& bytesRead, const int& currentBufferIndex); + void processInputReport(int bytesRead, const int currentBufferIndex); // For devices which only support a single report, reportID must be set to // 0x0. From ff1a1cd010108a1bdf047099e1b5d7306ede7b23 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Tue, 2 Feb 2021 19:58:53 +0100 Subject: [PATCH 18/27] Removed prefix i for member variables of type integer --- src/controllers/hid/hidcontroller.cpp | 16 ++++++++-------- src/controllers/hid/hidcontroller.h | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 1b466436926..0c05dc4f811 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -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()); @@ -110,7 +110,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(); @@ -139,7 +139,7 @@ int HidController::close() { void HidController::processInputReport(int bytesRead, const int currentBufferIndex) { Trace process("HidController processInputReport"); - unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex]; + unsigned char* pPreviousBuffer = m_pPollData[m_PollingBufferIndex]; unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex]; // Some controllers such as the Gemini GMX continuously send input reports even if it // is identical to the previous send input report. If this loop processed all those redundant @@ -149,12 +149,12 @@ void HidController::processInputReport(int bytesRead, const int currentBufferInd // have not encountered any controllers that send redundant input report 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_iLastPollSize && + if (bytesRead == m_LastPollSize && memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) { return; } - m_iLastPollSize = bytesRead; - m_iPollingBufferIndex = currentBufferIndex; + m_LastPollSize = bytesRead; + m_PollingBufferIndex = currentBufferIndex; auto incomingData = QByteArray::fromRawData( reinterpret_cast(pCurrentBuffer), bytesRead); receive(incomingData, mixxx::Time::elapsed()); @@ -165,7 +165,7 @@ QList HidController::getInputReport(unsigned int reportID) { int bytesRead; // Cycle between buffers so the memcmp below does not require deep copying to another buffer. - const int currentBufferIndex = (m_iPollingBufferIndex + 1) % kNumBuffers; + const int currentBufferIndex = (m_PollingBufferIndex + 1) % kNumBuffers; m_pPollData[currentBufferIndex][0] = reportID; bytesRead = hid_get_input_report(m_pHidDevice, m_pPollData[currentBufferIndex], kBufferSize); @@ -211,7 +211,7 @@ bool HidController::poll() { // a problem in practice. while (true) { // Cycle between buffers so the memcmp below does not require deep copying to another buffer. - const int currentBufferIndex = (m_iPollingBufferIndex + 1) % kNumBuffers; + const int currentBufferIndex = (m_PollingBufferIndex + 1) % kNumBuffers; int bytesRead = hid_read(m_pHidDevice, m_pPollData[currentBufferIndex], kBufferSize); if (bytesRead < 0) { diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index a22d6146889..6972475559d 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -72,8 +72,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; + int m_PollingBufferIndex; friend class HidControllerJSProxy; }; From 17cad5ffadc6ff28b1f5728adc2ecfca8ba4657f Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Tue, 2 Feb 2021 20:55:19 +0100 Subject: [PATCH 19/27] Removed the need for local currentIndex variable, by reordering the code. Now no latching of the index is needed anymore, a index can taken direct from them member variable at any place --- src/controllers/hid/hidcontroller.cpp | 34 ++++++++++++--------------- src/controllers/hid/hidcontroller.h | 2 +- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 0c05dc4f811..35fadc43fc4 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -137,10 +137,10 @@ int HidController::close() { return 0; } -void HidController::processInputReport(int bytesRead, const int currentBufferIndex) { +void HidController::processInputReport(int bytesRead) { Trace process("HidController processInputReport"); - unsigned char* pPreviousBuffer = m_pPollData[m_PollingBufferIndex]; - unsigned char* pCurrentBuffer = m_pPollData[currentBufferIndex]; + 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 send input report. If this loop processed all those redundant // input report, it would be a big performance problem to run JS code for every input report and @@ -153,8 +153,9 @@ void HidController::processInputReport(int bytesRead, const int currentBufferInd 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; m_LastPollSize = bytesRead; - m_PollingBufferIndex = currentBufferIndex; auto incomingData = QByteArray::fromRawData( reinterpret_cast(pCurrentBuffer), bytesRead); receive(incomingData, mixxx::Time::elapsed()); @@ -164,11 +165,8 @@ QList HidController::getInputReport(unsigned int reportID) { Trace hidRead("HidController getInputReport"); int bytesRead; - // Cycle between buffers so the memcmp below does not require deep copying to another buffer. - const int currentBufferIndex = (m_PollingBufferIndex + 1) % kNumBuffers; - - m_pPollData[currentBufferIndex][0] = reportID; - bytesRead = hid_get_input_report(m_pHidDevice, m_pPollData[currentBufferIndex], kBufferSize); + 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() @@ -187,17 +185,18 @@ QList HidController::getInputReport(unsigned int reportID) { return QList(); } - // Execute callback function in JavaScript mapping - // and print to stdout in case of --controllerDebug - HidController::processInputReport(bytesRead, currentBufferIndex); - // 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 dataList; dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { - dataList.append(m_pPollData[currentBufferIndex][i]); + dataList.append(m_pPollData[m_PollingBufferIndex][i]); } + + // Execute callback function in JavaScript mapping + // and print to stdout in case of --controllerDebug + HidController::processInputReport(bytesRead); + return dataList; } @@ -210,10 +209,7 @@ 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. - const int currentBufferIndex = (m_PollingBufferIndex + 1) % kNumBuffers; - - int bytesRead = hid_read(m_pHidDevice, m_pPollData[currentBufferIndex], 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); @@ -222,7 +218,7 @@ bool HidController::poll() { // No packet was available to be read return true; } - processInputReport(bytesRead, currentBufferIndex); + processInputReport(bytesRead); } } diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 6972475559d..411705c56ab 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -48,7 +48,7 @@ class HidController final : public Controller { private: bool isPolling() const override; - void processInputReport(int bytesRead, const int currentBufferIndex); + void processInputReport(int bytesRead); // For devices which only support a single report, reportID must be set to // 0x0. From 8263641df6a85e83a708406636683bb832b0b6e4 Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Wed, 17 Feb 2021 23:41:04 +0100 Subject: [PATCH 20/27] Update src/controllers/hid/hidcontroller.cpp Co-authored-by: Be --- src/controllers/hid/hidcontroller.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 35fadc43fc4..c02b6aa01d8 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -142,11 +142,11 @@ void HidController::processInputReport(int bytesRead) { 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 send input report. If this loop processed all those redundant - // input report, it would be a big performance problem to run JS code for every input report and - // would be unnecessary. - // This assumes that the redundant input report all use the same report ID. In practice we - // have not encountered any controllers that send redundant input report with different report + // 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 && From d6142502be83103c12a14c3f13b42f1484a541ea Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Wed, 17 Feb 2021 23:47:16 +0100 Subject: [PATCH 21/27] Update src/controllers/hid/hidcontroller.h Co-authored-by: Be --- src/controllers/hid/hidcontroller.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 411705c56ab..33ac8ac3222 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -72,7 +72,7 @@ class HidController final : public Controller { static constexpr int kNumBuffers = 2; static constexpr int kBufferSize = 255; unsigned char m_pPollData[kNumBuffers][kBufferSize]; - int m_LastPollSize; + int m_lastPollSize; int m_PollingBufferIndex; friend class HidControllerJSProxy; From 19be7ffdc4f4e877d026f4cddc31ec0a3ac5ec18 Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Wed, 17 Feb 2021 23:47:30 +0100 Subject: [PATCH 22/27] Update src/controllers/hid/hidcontroller.h Co-authored-by: Be --- src/controllers/hid/hidcontroller.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 33ac8ac3222..af7e52e9c90 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -73,7 +73,7 @@ class HidController final : public Controller { static constexpr int kBufferSize = 255; unsigned char m_pPollData[kNumBuffers][kBufferSize]; int m_lastPollSize; - int m_PollingBufferIndex; + int m_pollingBufferIndex; friend class HidControllerJSProxy; }; From 79e139285282701aeb66431a92162f3e3bdb6d94 Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Thu, 18 Feb 2021 01:00:19 +0100 Subject: [PATCH 23/27] Changed member variables to camelCase Removed class name before private method call --- src/controllers/hid/hidcontroller.cpp | 34 +++++++++++++-------------- src/controllers/hid/hidcontroller.h | 2 +- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 63da9510832..23ebcf9579c 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -19,7 +19,7 @@ HidController::HidController( mixxx::hid::DeviceInfo&& deviceInfo) : m_deviceInfo(std::move(deviceInfo)), m_pHidDevice(nullptr), - m_PollingBufferIndex(0) { + m_pollingBufferIndex(0) { setDeviceCategory(mixxx::hid::DeviceCategory::guessFromDeviceInfo(m_deviceInfo)); setDeviceName(m_deviceInfo.formatName()); @@ -109,7 +109,7 @@ int HidController::open() { for (int i = 0; i < kNumBuffers; i++) { memset(m_pPollData[i], 0, kBufferSize); } - m_LastPollSize = 0; + m_lastPollSize = 0; setOpen(true); startEngine(); @@ -138,23 +138,23 @@ int HidController::close() { 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]; + 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 + // is identical to the previous send input report. If this loop processed all those redundant + // input report, it would be a big performance problem to run JS code for every input report and + // would be unnecessary. + // This assumes that the redundant input report all use the same report ID. In practice we + // have not encountered any controllers that send redundant input report 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 && + 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; - m_LastPollSize = bytesRead; + m_pollingBufferIndex = (m_pollingBufferIndex + 1) % kNumBuffers; + m_lastPollSize = bytesRead; auto incomingData = QByteArray::fromRawData( reinterpret_cast(pCurrentBuffer), bytesRead); receive(incomingData, mixxx::Time::elapsed()); @@ -164,8 +164,8 @@ QList 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); + 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() @@ -189,12 +189,12 @@ QList HidController::getInputReport(unsigned int reportID) { QList dataList; dataList.reserve(bytesRead); for (int i = 0; i < bytesRead; i++) { - dataList.append(m_pPollData[m_PollingBufferIndex][i]); + dataList.append(m_pPollData[m_pollingBufferIndex][i]); } // Execute callback function in JavaScript mapping // and print to stdout in case of --controllerDebug - HidController::processInputReport(bytesRead); + processInputReport(bytesRead); return dataList; } @@ -208,7 +208,7 @@ bool HidController::poll() { // There is no safety net for this because it has not been demonstrated to be // a problem in practice. while (true) { - int bytesRead = hid_read(m_pHidDevice, m_pPollData[m_PollingBufferIndex], 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); diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 7303ab35a5e..829acb4ff93 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -50,7 +50,7 @@ class HidController final : public Controller { QList getInputReport(unsigned int reportID); QList getFeatureReport(unsigned int reportID); - + const mixxx::hid::DeviceInfo m_deviceInfo; hid_device* m_pHidDevice; From 706ea80e7ab91af4e8728eb873564880430fbc2c Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sun, 21 Feb 2021 09:04:58 +0100 Subject: [PATCH 24/27] Removed call of incomingData from getInputReport due to the risk of infinite loops in mapping scripts Fixed some comments mentioned in review --- src/controllers/hid/hidcontroller.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index 23ebcf9579c..9126db1bac8 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -152,11 +152,14 @@ void HidController::processInputReport(int bytesRead) { memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) { return; } - // Cycle between buffers so the memcmp below does not require deep copying to another buffer. + // Cycle between buffers so the memcmp above does not require deep copying to another buffer. m_pollingBufferIndex = (m_pollingBufferIndex + 1) % kNumBuffers; m_lastPollSize = bytesRead; auto incomingData = QByteArray::fromRawData( reinterpret_cast(pCurrentBuffer), bytesRead); + + // Execute callback function in JavaScript mapping + // and print to stdout in case of --controllerDebug receive(incomingData, mixxx::Time::elapsed()); } @@ -174,9 +177,9 @@ QList HidController::getInputReport(unsigned int reportID) { << QString::number(static_cast(reportID), 16) .toUpper() .rightJustified(2, QChar('0')) - << ")") + << ")"); - if (bytesRead <= kReportIdSize) { + 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. @@ -191,11 +194,6 @@ QList HidController::getInputReport(unsigned int reportID) { 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 - processInputReport(bytesRead); - return dataList; } From fb7d30d3cbf4a0de05a6e7258c42f3c6421b6c7c Mon Sep 17 00:00:00 2001 From: JoergAtGithub Date: Sun, 21 Feb 2021 09:43:03 +0100 Subject: [PATCH 25/27] Added detailed usage description of the getInputReport and getFeatureReport --- src/controllers/hid/hidcontroller.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 829acb4ff93..8efafa92363 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -48,7 +48,22 @@ class HidController final : public Controller { void sendBytesReport(QByteArray data, unsigned int reportID); void sendFeatureReport(const QList& dataList, unsigned int reportID); + // getInputReport receives an input report on request. + // This can be used on startup, to initialize the knob positions in Mixxx + // to the physical position of the hardware knobs on the controller. + // The returned data structure for the input reports is the same, + // as in the polling functionality (incl. ReportID in first byte). + // The returned list can be used, to call the incomingData + // function of the common-hid-packet-parser. QList getInputReport(unsigned int reportID); + + // getFeatureReport receives a feature reports on request. + // HID doesn't support polling feature reports, therefore this is the + // only method to get this information. + // Usually single bits in a feature report need to be set without + // changeing the other bits. The returned list matches the input + // format of sendFeatureReport, this allows it to read, modify + // and send it back to the controller. QList getFeatureReport(unsigned int reportID); const mixxx::hid::DeviceInfo m_deviceInfo; From cfccc4c1865fff4c7c559015b30199debe357647 Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Sun, 21 Feb 2021 19:56:50 +0100 Subject: [PATCH 26/27] Fixed spelling of comment Co-authored-by: Be --- src/controllers/hid/hidcontroller.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 8efafa92363..20465180a35 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -49,11 +49,11 @@ class HidController final : public Controller { void sendFeatureReport(const QList& dataList, unsigned int reportID); // getInputReport receives an input report on request. - // This can be used on startup, to initialize the knob positions in Mixxx + // This can be used on startup to initialize the knob positions in Mixxx // to the physical position of the hardware knobs on the controller. - // The returned data structure for the input reports is the same, - // as in the polling functionality (incl. ReportID in first byte). - // The returned list can be used, to call the incomingData + // The returned data structure for the input reports is the same + // 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 getInputReport(unsigned int reportID); From 54a1f05d0d5d54c01582fffff6d864600b909133 Mon Sep 17 00:00:00 2001 From: JoergAtGithub <64457745+JoergAtGithub@users.noreply.github.com> Date: Sun, 21 Feb 2021 19:57:02 +0100 Subject: [PATCH 27/27] Fixed spelling of comment Co-authored-by: Be --- src/controllers/hid/hidcontroller.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 20465180a35..f84463302b2 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -60,10 +60,10 @@ class HidController final : public Controller { // getFeatureReport receives a feature reports on request. // HID doesn't support polling feature reports, therefore this is the // only method to get this information. - // Usually single bits in a feature report need to be set without - // changeing the other bits. The returned list matches the input - // format of sendFeatureReport, this allows it to read, modify - // and send it back to the controller. + // Usually, single bits in a feature report need to be set without + // 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 getFeatureReport(unsigned int reportID); const mixxx::hid::DeviceInfo m_deviceInfo;