Skip to content

Commit

Permalink
Fix performance issues by slow hid_write command of HIDAPI, which can…
Browse files Browse the repository at this point in the history 
… block the controller thread for several milliseconds per OutputReport:

-Ensure that the ring-buffer of InputReports is polled between hid_write of two OutputReports
-Move HID IO in a dedicated thread, this allows the JavaScript controller mapping to continue, while HIDAPI waits for completion of the data transfer of the OutputReport
-Skip sending of OutputReports, if the data didn't changed - as already implemented for polled InputReports
-Added timing information to --controllerDebug output, that mapping developers can understand the timing behavior
  • Loading branch information
@JoergAtGithub
JoergAtGithub committed Dec 27, 2021
1 parent ab55358 commit 387bc55
Show file tree
Hide file tree
Showing 5 changed files with 438 additions and 193 deletions.
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2775,6 +2775,7 @@ if(HID)
endif()
target_sources(mixxx-lib PRIVATE
src/controllers/hid/hidcontroller.cpp
src/controllers/hid/hidio.cpp
src/controllers/hid/hiddevice.cpp
src/controllers/hid/hidenumerator.cpp
src/controllers/hid/legacyhidcontrollermapping.cpp
Expand Down
264 changes: 89 additions & 175 deletions src/controllers/hid/hidcontroller.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,12 @@
#include "util/time.h"
#include "util/trace.h"

namespace {
constexpr int kReportIdSize = 1;
constexpr int kMaxHidErrorMessageSize = 512;
} // namespace

HidController::HidController(
mixxx::hid::DeviceInfo&& deviceInfo)
: Controller(deviceInfo.formatName()),
m_deviceInfo(std::move(deviceInfo)),
m_pHidDevice(nullptr),
m_pollingBufferIndex(0) {
m_pHidIo(nullptr) {
setDeviceCategory(mixxx::hid::DeviceCategory::guessFromDeviceInfo(m_deviceInfo));

// All HID devices are full-duplex
Expand Down Expand Up @@ -104,15 +99,52 @@ int HidController::open() {
return -1;
}

// This isn't strictly necessary but is good practice.
for (int i = 0; i < kNumBuffers; i++) {
memset(m_pPollData[i], 0, kBufferSize);
}
m_lastPollSize = 0;

setOpen(true);
startEngine();

if (m_pHidIo != nullptr) {
qWarning() << "HidIo already present for" << getName();
} else {
m_pHidIo = new HidIo(m_pHidDevice,
std::move(m_deviceInfo),
m_logBase,
m_logInput,
m_logOutput);
m_pHidIo->setObjectName(QString("HidIo %1").arg(getName()));

connect(m_pHidIo,
&HidIo::receive,
this,
&HidController::receive,
Qt::QueuedConnection);

connect(this,
&HidController::getInputReport,
m_pHidIo,
&HidIo::getInputReport,
Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread

connect(this,
&HidController::sendOutputReport,
m_pHidIo,
&HidIo::sendOutputReport,
Qt::QueuedConnection);

connect(this,
&HidController::getFeatureReport,
m_pHidIo,
&HidIo::getFeatureReport,
Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread
connect(this,
&HidController::sendFeatureReport,
m_pHidIo,
&HidIo::sendFeatureReport,
Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread

// Controller input needs to be prioritized since it can affect the
// audio directly, like when scratching
m_pHidIo->start(QThread::HighPriority);
}
return 0;
}

Expand All @@ -124,95 +156,60 @@ int HidController::close() {

qCInfo(m_logBase) << "Shutting down HID device" << getName();

// Stop the reading thread
if (m_pHidIo == nullptr) {
qWarning() << "HidIo not present for" << getName()
<< "yet the device is open!";
} else {
disconnect(m_pHidIo,
&HidIo::receive,
this,
&HidController::receive);

disconnect(this,
&HidController::getInputReport,
m_pHidIo,
&HidIo::getInputReport);

disconnect(this,
&HidController::sendOutputReport,
m_pHidIo,
&HidIo::sendOutputReport);

disconnect(this,
&HidController::getFeatureReport,
m_pHidIo,
&HidIo::getFeatureReport);
disconnect(this,
&HidController::sendFeatureReport,
m_pHidIo,
&HidIo::sendFeatureReport);

m_pHidIo->stop();
hid_set_nonblocking(m_pHidDevice, 1); // Quit blocking
qDebug() << "Waiting on IO thread to finish";
m_pHidIo->wait();
}

// Stop controller engine here to ensure it's done before the device is closed
// in case it has any final parting messages
// in case it has any final parting messages
stopEngine();

if (m_pHidIo != nullptr) {
delete m_pHidIo;
m_pHidIo = nullptr;
}

// Close device
qCInfo(m_logBase) << "Closing device";
// hid_close is not thread safe
// All communication to this hid_device must be completed, before hid_close is called.
hid_close(m_pHidDevice);
setOpen(false);
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 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 &&
memcmp(pCurrentBuffer, pPreviousBuffer, bytesRead) == 0) {
return;
}
// 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<char*>(pCurrentBuffer), bytesRead);

// Execute callback function in JavaScript mapping
// and print to stdout in case of --controllerDebug
receive(incomingData, mixxx::Time::elapsed());
}

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

m_pPollData[m_pollingBufferIndex][0] = reportID;
// FIXME: implement upstream for hidraw backend on Linux
// https://github.com/libusb/hidapi/issues/259
bytesRead = hid_get_input_report(m_pHidDevice, m_pPollData[m_pollingBufferIndex], kBufferSize);

qCDebug(m_logInput) << 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 QByteArray();
}

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

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

// This loop risks becoming a high priority endless loop in case processing
// the mapping JS code takes longer than the controller polling rate.
// This could stall other low priority tasks.
// 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);
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;
}
processInputReport(bytesRead);
}
}

bool HidController::isPolling() const {
return isOpen();
Expand All @@ -224,96 +221,13 @@ void HidController::sendReport(QList<int> data, unsigned int length, unsigned in
foreach (int datum, data) {
temp.append(datum);
}
sendBytesReport(temp, reportID);
emit sendOutputReport(temp, reportID);
}

void HidController::sendBytes(const QByteArray& data) {
sendBytesReport(data, 0);
}

void HidController::sendBytesReport(QByteArray data, unsigned int reportID) {
// Append the Report ID to the beginning of data[] per the API..
data.prepend(reportID);

int result = hid_write(m_pHidDevice, (unsigned char*)data.constData(), data.size());
if (result == -1) {
qCWarning(m_logOutput) << "Unable to send data to" << getName() << ":"
<< mixxx::convertWCStringToQString(
hid_error(m_pHidDevice),
kMaxHidErrorMessageSize);
} else {
qCDebug(m_logOutput) << result << "bytes sent to" << getName()
<< "serial #" << m_deviceInfo.serialNumber()
<< "(including report ID of" << reportID << ")";
}
}

void HidController::sendFeatureReport(
const QByteArray& reportData, unsigned int reportID) {
QByteArray dataArray;
dataArray.reserve(kReportIdSize + reportData.size());

// Append the Report ID to the beginning of dataArray[] per the API..
dataArray.append(reportID);

for (const int datum : reportData) {
dataArray.append(datum);
}

int result = hid_send_feature_report(m_pHidDevice,
reinterpret_cast<const unsigned char*>(dataArray.constData()),
dataArray.size());
if (result == -1) {
qCWarning(m_logOutput) << "sendFeatureReport is unable to send data to"
<< getName() << "serial #" << m_deviceInfo.serialNumber()
<< ":"
<< mixxx::convertWCStringToQString(
hid_error(m_pHidDevice),
kMaxHidErrorMessageSize);
} else {
qCDebug(m_logOutput) << result << "bytes sent by sendFeatureReport to" << getName()
<< "serial #" << m_deviceInfo.serialNumber()
<< "(including report ID of" << reportID << ")";
}
emit sendOutputReport(data, 0);
}

ControllerJSProxy* HidController::jsProxy() {
return new HidControllerJSProxy(this);
}

QByteArray 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 <= 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.
qCWarning(m_logInput) << "getFeatureReport is unable to get data from" << getName()
<< "serial #" << m_deviceInfo.serialNumber() << ":"
<< mixxx::convertWCStringToQString(
hid_error(m_pHidDevice),
kMaxHidErrorMessageSize);
} else {
qCDebug(m_logInput) << 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 compatibility with input array HidController::sendFeatureReport, a reportID prefix is not added here
QByteArray byteArray;
byteArray.reserve(bytesRead - kReportIdSize);
auto featureReportStart = reinterpret_cast<const char*>(dataRead + kReportIdSize);
return QByteArray(featureReportStart, bytesRead);
}
Loading

0 comments on commit 387bc55

Please sign in to comment.