Skip to content

Commit

Permalink
Replaced DirectConnections by Mutex-Protected function calls
Browse files Browse the repository at this point in the history 
Use standard Event Loop for HidIoThread
Removed no longer needed implementation of IsPolling
  • Loading branch information
@JoergAtGithub
JoergAtGithub committed Jan 2, 2022
1 parent 4ebaec9 commit 8399b46
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 111 deletions.
68 changes: 16 additions & 52 deletions src/controllers/hid/hidcontroller.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ HidController::HidController(
: Controller(deviceInfo.formatName()),
m_deviceInfo(std::move(deviceInfo)),
m_pHidDevice(nullptr),
m_pHidIo(nullptr) {
m_pHidIoThread(nullptr) {
setDeviceCategory(mixxx::hid::DeviceCategory::guessFromDeviceInfo(m_deviceInfo));

// All HID devices are full-duplex
Expand Down Expand Up @@ -102,48 +102,32 @@ int HidController::open() {
setOpen(true);
startEngine();

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

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

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

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

connect(this,
&HidController::getFeatureReport,
m_pHidIo,
&HidIoThread::getFeatureReport,
Qt::DirectConnection); // Enforces syncronisation of mapping and IO thread
connect(this,
&HidController::sendFeatureReport,
m_pHidIo,
&HidIoThread::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);
m_pHidIoThread->start(QThread::HighPriority);
m_pHidIoThread->startPollTimer();
}
return 0;
}
Expand All @@ -157,47 +141,33 @@ int HidController::close() {
qCInfo(m_logBase) << "Shutting down HID device" << getName();

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

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

disconnect(this,
&HidController::sendOutputReport,
m_pHidIo,
m_pHidIoThread,
&HidIoThread::sendOutputReport);

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

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

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

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

// Close device
Expand All @@ -209,12 +179,6 @@ int HidController::close() {
return 0;
}



bool HidController::isPolling() const {
return isOpen();
}

void HidController::sendReport(QList<int> data, unsigned int length, unsigned int reportID) {
Q_UNUSED(length);
QByteArray temp;
Expand Down
45 changes: 18 additions & 27 deletions src/controllers/hid/hidcontroller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,31 +40,9 @@ class HidController final : public Controller {
int close() override;

private:
bool isPolling() const override;

signals:
// 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 (including ReportID in first byte).
// The returned list can be used to call the incomingData
// function of the common-hid-packet-parser.
QByteArray getInputReport(unsigned int reportID);

void sendOutputReport(const QByteArray& data, unsigned int reportID);

void sendFeatureReport(const QByteArray& reportData, 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
// 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.
QByteArray getFeatureReport(unsigned int reportID);

private:
// For devices which only support a single report, reportID must be set to
// 0x0.
Expand All @@ -73,7 +51,7 @@ class HidController final : public Controller {
const mixxx::hid::DeviceInfo m_deviceInfo;

hid_device* m_pHidDevice;
HidIoThread* m_pHidIo;
HidIoThread* m_pHidIoThread;
std::shared_ptr<LegacyHidControllerMapping> m_pMapping;

friend class HidControllerJSProxy;
Expand All @@ -95,19 +73,32 @@ class HidControllerJSProxy : public ControllerJSProxy {
m_pHidController->sendReport(data, length, 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 (including ReportID in first byte).
// The returned list can be used to call the incomingData
// function of the common-hid-packet-parser.
Q_INVOKABLE QByteArray getInputReport(
unsigned int reportID) {
return emit m_pHidController->getInputReport(reportID);
return m_pHidController->m_pHidIoThread->getInputReport(reportID);
}

Q_INVOKABLE void sendFeatureReport(
const QByteArray& reportData, unsigned int reportID) {
emit m_pHidController->sendFeatureReport(reportData, reportID);
m_pHidController->m_pHidIoThread->sendFeatureReport(reportData, 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
// 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.
Q_INVOKABLE QByteArray getFeatureReport(
unsigned int reportID) {
return emit m_pHidController->getFeatureReport(reportID);
return m_pHidController->m_pHidIoThread->getFeatureReport(reportID);
}

private:
Expand Down
49 changes: 33 additions & 16 deletions src/controllers/hid/hidio.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include <hidapi.h>

#include <QTimer>

#include "controllers/defs_controllers.h"
#include "controllers/hid/legacyhidcontrollermappingfilehandler.h"
#include "moc_hidio.cpp"
Expand Down Expand Up @@ -67,25 +69,35 @@ HidIoThread::HidIoThread(hid_device* device,
m_logInput(logInput),
m_logOutput(logOutput),
m_pHidDevice(device),
m_deviceInfo(std::move(deviceInfo)) {
m_deviceInfo(std::move(deviceInfo)),
m_HidDeviceMutex(QT_RECURSIVE_MUTEX_INIT),
mPollTimerId(0) {
// 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;
}

void HidIoThread::run() {
atomicStoreRelaxed(m_stop, 0);
while (atomicLoadRelaxed(m_stop) == 0) {
poll();
usleep(1000);
void HidIoThread::timerEvent(QTimerEvent* event) {
poll();
}

void HidIoThread::startPollTimer() {
if (mPollTimerId == 0) {
mPollTimerId = startTimer(1, Qt::PreciseTimer);
}
}

void HidIoThread::stop() {
printf("Stop HidIoThread\n");
killTimer(mPollTimerId);
quit();
}

void HidIoThread::poll() {
Trace hidRead("HidIoThread poll");

auto lock = lockMutex(&m_HidDeviceMutex);
// 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.
Expand Down Expand Up @@ -134,6 +146,7 @@ void HidIoThread::processInputReport(int bytesRead) {

QByteArray HidIoThread::getInputReport(unsigned int reportID) {
auto startOfHidGetInputReport = mixxx::Time::elapsed();
auto lock = lockMutex(&m_HidDeviceMutex);
int bytesRead;

m_pPollData[m_pollingBufferIndex][0] = reportID;
Expand All @@ -159,26 +172,29 @@ QByteArray HidIoThread::getInputReport(unsigned int reportID) {
return QByteArray();
}

return QByteArray::fromRawData(
reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);
// Return deep-copy of the polled data, for thread safety.
return QByteArray(reinterpret_cast<char*>(m_pPollData[m_pollingBufferIndex]), bytesRead);
}

void HidIoThread::sendOutputReport(const QByteArray& data, unsigned int reportID) {
if (m_outputReports.find(reportID) == m_outputReports.end()) {
std::unique_ptr<HidIoReport> pNewOutputReport;
m_outputReports[reportID] = std::make_unique<HidIoReport>(
reportID, m_pHidDevice, std::move(m_deviceInfo), m_logOutput);
{
auto lock = lockMutex(&m_HidDeviceMutex);
if (m_outputReports.find(reportID) == m_outputReports.end()) {
std::unique_ptr<HidIoReport> pNewOutputReport;
m_outputReports[reportID] = std::make_unique<HidIoReport>(
reportID, m_pHidDevice, std::move(m_deviceInfo), m_logOutput);
}
// SendOutputReports executes a hardware operation, which take several milliseconds
m_outputReports[reportID]->sendOutputReport(data);
}
// SendOutputReports executes a hardware operation, which take several milliseconds
m_outputReports[reportID]->sendOutputReport(data);

// Ensure that all InputReports are read from the ring buffer, before the next OutputReport blocks the IO again
poll(); // Polling available Input-Reports is a cheap software only operation, which takes insignificiant time
}

void HidIoThread::sendFeatureReport(
const QByteArray& reportData, unsigned int reportID) {
auto startOfHidSendFeatureReport = mixxx::Time::elapsed();
auto lock = lockMutex(&m_HidDeviceMutex);
QByteArray dataArray;
dataArray.reserve(kReportIdSize + reportData.size());

Expand Down Expand Up @@ -213,6 +229,7 @@ void HidIoThread::sendFeatureReport(
QByteArray HidIoThread::getFeatureReport(
unsigned int reportID) {
auto startOfHidGetFeatureReport = mixxx::Time::elapsed();
auto lock = lockMutex(&m_HidDeviceMutex);
unsigned char dataRead[kReportIdSize + kBufferSize];
dataRead[0] = reportID;

Expand Down
36 changes: 20 additions & 16 deletions src/controllers/hid/hidio.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "controllers/controller.h"
#include "controllers/hid/hiddevice.h"
#include "util/compatibility/qatomic.h"
#include "util/compatibility/qmutex.h"
#include "util/duration.h"

class HidIoReport {
Expand Down Expand Up @@ -34,38 +35,41 @@ class HidIoThread : public QThread {
const RuntimeLoggingCategory& logInput,
const RuntimeLoggingCategory& logOutput);

void stop() {
atomicStoreRelaxed(m_stop, 1);
}
void stop();

static constexpr int kNumBuffers = 2;
static constexpr int kBufferSize = 255;
unsigned char m_pPollData[kNumBuffers][kBufferSize];
int m_lastPollSize;
int m_pollingBufferIndex;
const RuntimeLoggingCategory m_logBase;
const RuntimeLoggingCategory m_logInput;
const RuntimeLoggingCategory m_logOutput;
QByteArray getInputReport(unsigned int reportID);
void sendFeatureReport(const QByteArray& reportData, unsigned int reportID);
QByteArray getFeatureReport(unsigned int reportID);
void startPollTimer();

signals:
/// Signals that a HID InputReport received by Interrupt triggered from HID device
void receive(const QByteArray& data, mixxx::Duration timestamp);

public slots:
QByteArray getInputReport(unsigned int reportID);
void sendOutputReport(const QByteArray& reportData, unsigned int reportID);
void sendFeatureReport(const QByteArray& reportData, unsigned int reportID);
QByteArray getFeatureReport(unsigned int reportID);

protected:
void run();
void timerEvent(QTimerEvent* event) override;

private:
static constexpr int kNumBuffers = 2;
static constexpr int kBufferSize = 255;
unsigned char m_pPollData[kNumBuffers][kBufferSize];
int m_lastPollSize;
int m_pollingBufferIndex;
const RuntimeLoggingCategory m_logBase;
const RuntimeLoggingCategory m_logInput;
const RuntimeLoggingCategory m_logOutput;

void poll();
void processInputReport(int bytesRead);
hid_device* const
m_pHidDevice; // const pointer to the C data structure, which hidapi uses for communication between functions
const mixxx::hid::DeviceInfo m_deviceInfo;
QAtomicInt m_stop;
std::map<unsigned char, std::unique_ptr<HidIoReport>> m_outputReports;

// Must be locked when using the m_pHidDevice and it's properties, which is not thread-safe for hidapi backends
QT_RECURSIVE_MUTEX m_HidDeviceMutex;
int mPollTimerId;
};

0 comments on commit 8399b46

Please sign in to comment.