Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HidController: loop until no more messages are available on poll #2970

Merged
merged 18 commits into from
Oct 4, 2020
Merged

HidController: loop until no more messages are available on poll #2970

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c2e62c9
HidController: loop until no more messages are available on poll
Be-ing Jul 27, 2020
fe35428
HidController: do not copy incoming data before processing it
Be-ing Jul 27, 2020
ec70661
HidController: filter redundant messages
Be-ing Aug 2, 2020
1f84927
ControllerManager: make kPollInterval a static member
Be-ing Aug 2, 2020
a362551
HidController: use double buffering to avoid deep copy
Be-ing Sep 20, 2020
7cdc090
HidController: remove outdated comment
Be-ing Sep 20, 2020
cd281c1
HidController: use static constexpr for buffer size
Be-ing Sep 20, 2020
229e9f1
HidController: skip creating QByteArray if memcmp check fails
Be-ing Sep 21, 2020
ecc5d33
HidController: use modulo math for m_iPollingBufferIndex
Be-ing Sep 21, 2020
cea971a
HidController: introduce m_iNumBuffers constant
Be-ing Sep 21, 2020
d146e74
HidController: initialize buffers to 0
Be-ing Sep 22, 2020
a2e3267
HidController: set buffers to 0 upon opening device
Be-ing Sep 23, 2020
26bdbef
HidController: rename static constants
Be-ing Sep 23, 2020
cd283de
HidController: swap naming of pPreviousBuffer & pCurrentBuffer
Be-ing Sep 23, 2020
447a432
HidController: add more comments
Be-ing Sep 27, 2020
2343bbf
HidController: reorganize polling loop code
Be-ing Oct 1, 2020
018f1e2
HidController: remove reference to constant from comment
Be-ing Oct 1, 2020
dff0202
HidController: handle all possible return values from hidapi
Be-ing Oct 4, 2020
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
10 changes: 5 additions & 5 deletions src/controllers/controllermanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,18 @@
#include "controllers/bulk/bulkenumerator.h"
#endif

namespace {
// http://developer.qt.nokia.com/wiki/Threads_Events_QObjects

// Poll every 1ms (where possible) for good controller response
#ifdef __LINUX__
// Many Linux distros ship with the system tick set to 250Hz so 1ms timer
// reportedly causes CPU hosage. See Bug #990992 rryan 6/2012
const int kPollIntervalMillis = 5;
const mixxx::Duration ControllerManager::kPollInterval = mixxx::Duration::fromMillis(5);
#else
const int kPollIntervalMillis = 1;
const mixxx::Duration ControllerManager::kPollInterval = mixxx::Duration::fromMillis(1);
#endif

namespace {
/// Strip slashes and spaces from device name, so that it can be used as config
/// key or a filename.
QString sanitizeDeviceName(QString name) {
Expand Down Expand Up @@ -97,7 +97,7 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig)
QDir().mkpath(userPresets);
}

m_pollTimer.setInterval(kPollIntervalMillis);
m_pollTimer.setInterval(kPollInterval.toIntegerMillis());
connect(&m_pollTimer, SIGNAL(timeout()),
this, SLOT(pollDevices()));

Expand Down Expand Up @@ -359,7 +359,7 @@ void ControllerManager::pollDevices() {
}

mixxx::Duration duration = mixxx::Time::elapsed() - start;
if (duration > mixxx::Duration::fromMillis(kPollIntervalMillis)) {
if (duration > kPollInterval) {
m_skipPoll = true;
}
//qDebug() << "ControllerManager::pollDevices()" << duration << start;
Expand Down
2 changes: 2 additions & 0 deletions src/controllers/controllermanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class ControllerManager : public QObject {
ControllerManager(UserSettingsPointer pConfig);
virtual ~ControllerManager();

static const mixxx::Duration kPollInterval;

QList<Controller*> getControllers() const;
QList<Controller*> getControllerList(bool outputDevices=true, bool inputDevices=true);
ControllerLearningEventFilter* getControllerLearningEventFilter() const;
Expand Down
36 changes: 28 additions & 8 deletions src/controllers/hid/hidcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@

HidController::HidController(const hid_device_info& deviceInfo, UserSettingsPointer pConfig)
: Controller(pConfig),
m_pHidDevice(NULL) {
m_pHidDevice(nullptr),
m_iPollingBufferIndex(0) {
// Copy required variables from deviceInfo, which will be freed after
// this class is initialized by caller.
hid_vendor_id = deviceInfo.vendor_id;
Expand Down Expand Up @@ -243,13 +244,32 @@ int HidController::close() {
bool HidController::poll() {
Trace hidRead("HidController poll");

int result = hid_read(m_pHidDevice, m_pPollData, sizeof(m_pPollData) / sizeof(m_pPollData[0]));
if (result == -1) {
return false;
} else if (result > 0) {
Trace process("HidController process packet");
QByteArray outData(reinterpret_cast<char*>(m_pPollData), result);
receive(outData, mixxx::Time::elapsed());
int result = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

result does not need to be defined outside the loop. An infinite for loop with explicit return points would be much easier to follow.

Setting result to 1 before the loop is also conceptually incorrect and an ugly hack. The value represents the number of received bytes. But we haven't received anything yet and the value 1 is arbitrary, anticipating some assumptions of code in another (though near) context.

while (result > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This potential stalls the whole ControllerManager on low end devices. Slots like slotShutdown() are never called.
This is also high priority thread. A burning CPU loop here will also effect GUI performance badly.

Conclusion. This mus behave cooperative and end the loop after a maximum amount of time.
We need to create a kind of time slice.

Unfortunately the hid API does not offer flush() call to start over.

Copy link
Member

Choose a reason for hiding this comment

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

If this loops takes longer than the polling interval, I suggest to flush the remaining messages by reading them without processing, and then add quit the while loop.

This can also happen due to a script issue, so the user should be able to recognize the situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that before and it did not work.

Copy link
Member

Choose a reason for hiding this comment

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

With the latest commits I get the same results as with #2970 (comment).

Unfortunately the last commit is gone.

It is just that we need a kind of stop condition for this potential endless loop.
You may check if the poll timer has already expired again. If this happen, flush the buffer and break the loop waiting for the next cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this loops takes longer than the polling interval, I suggest to flush the remaining messages by reading them without processing, and then add quit the while loop.

Ah, the code that @codecat tested before did not flush the remaining messages. That may be why the state of the controller seemed frozen. But I am not sure how flushing the messages would work because it would only be done when the loop is taking too long, so wouldn't there not be any messages for the next loop? Also, wouldn't the timeout need to slightly less than the polling interval?

Copy link
Member

Choose a reason for hiding this comment

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

This is not too hypothetical. I think part of the original issue is that the mapping code consumes too much CPU.
The old code was not able to stall Mixxx in this case. This is.

Copy link
Member

Choose a reason for hiding this comment

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

don't know what a good time limit would be.

I would start with 50 % idle time vs processing time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the issue this PR fixes is that HID messages were not processed often enough because only one packet was processed every 5 ms. If the mapping code took too long, this branch would make the original problem worse, not better.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK I misunderstand that.
This is still against my experience as embedded developer, but if you are confident that we will have no issue with long running scripts here, it is OK for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So ready for merge?

// Rotate between two buffers so the memcmp below does not require deep copying to another buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Rotate between two buffers ..." -> "Cycle between disjunct input buffers ..."

"two" depends on the value of the constant

unsigned char* pCurrentBuffer = m_pPollData[m_iPollingBufferIndex];
if (m_iPollingBufferIndex == 0) {
m_iPollingBufferIndex = 1;
} else {
m_iPollingBufferIndex = 0;
}
unsigned char* pPreviousBuffer = m_pPollData[m_iPollingBufferIndex];

result = hid_read(m_pHidDevice, pCurrentBuffer, m_iBufferSize);
if (result == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use early returns and avoid all the nesting:

  • result < 0 -> return false
  • result == 0 -> return true
  • ...DEBUG_ASSERT(result > 0) and process the received package

Copy link
Contributor Author

@Be-ing Be-ing Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, this obviates the need for the hacky int result = 1 too: 2343bbf

return false;
} else if (result > 0) {
Trace process("HidController process packet");
auto byteArray = QByteArray::fromRawData(
reinterpret_cast<char*>(pCurrentBuffer), result);
// 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.
if (memcmp(pCurrentBuffer, pPreviousBuffer, m_iBufferSize) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be switchable by the mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not add complexity to the mapping system to avoid this trivial cost.

Copy link
Member

@Holzhaus Holzhaus Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

memcmp'ing 255 bytes 10 million times takes 4.82 milliseconds with -O0 on my laptop. I think the cost is neglible.

See: #2970 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that omitting update suits to all cases? I think this can be surprising.

continue;
Copy link
Member

Choose a reason for hiding this comment

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

If we decide not to make this code conditional, we need a more significant comment that a HID telegram is discarded here for all controllers anong with some words why we think that his is universal correct.

Copy link
Contributor Author

@Be-ing Be-ing Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree that there is any need for further explanation in the comment. If the current packet is the same as the last, there is no purpose processing it regardless of which controller is doing this.

Copy link
Member

Choose a reason for hiding this comment

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

Additional comments never hurts. It is better to be explicit.
The long discussion shows that there is a need IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is IMO obvious and explicit already.

Copy link
Member

Choose a reason for hiding this comment

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

This is normal for the author...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's too terse. It does not explain why duplicates are a problem. There is nothing wrong with a 3 line comment.

Copy link
Member

Choose a reason for hiding this comment

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

Please add this in addition to your comment:

// This loop discards all equal HID packages. This releases the js mapping code from idle work in case the controller repeat it's complete process state in a short loop. Similar checks in that mapping itself can be omitted.
We consider this as universal correct, because a protocol with different telegrams will likely have different content. The same is true for an event driven protocol.
This loop has the risk to be an high priority endless loop in case processing the mapping code takes longer than the controller update rate. This may stall other low priority task.
We have no safety net here, because we consider this as very unlikely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 447a432

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are the comments sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping

}
receive(byteArray, mixxx::Time::elapsed());
}
}

return true;
Expand Down
4 changes: 3 additions & 1 deletion src/controllers/hid/hidcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ class HidController final : public Controller {
hid_device* m_pHidDevice;
HidControllerPreset m_preset;

unsigned char m_pPollData[255];
static constexpr int m_iBufferSize = 255;
unsigned char m_pPollData[2][m_iBufferSize];
int m_iPollingBufferIndex;
};

#endif