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

Jump to bottom

Modernize Hid/Bulk Lists #13622

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Modernize Hid/Bulk Lists #13622

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3e2b8e9
refactor: modernize bulksupport allowlist
Swiftb0y Sep 2, 2024
2355ace
refactor: modernize hid_denylist
Swiftb0y Sep 2, 2024
2c2b47f
fixup! refactor: modernize hid_denylist
Swiftb0y Sep 3, 2024
4f4c424
fixup! refactor: modernize bulksupport allowlist
Swiftb0y Sep 5, 2024
14f0ea8
fixup! refactor: modernize bulksupport allowlist
Swiftb0y Sep 5, 2024
7b712fa
fixup! refactor: modernize bulksupport allowlist
Swiftb0y Sep 5, 2024
4d58f6b
fixup! refactor: modernize bulksupport allowlist
Swiftb0y Sep 6, 2024
a914b05
fixup! refactor: modernize bulksupport allowlist
Swiftb0y Sep 6, 2024
c29c42e
refactor: restructure `bulk_supported` to emphasize kv structure
Swiftb0y Sep 9, 2024
8b7c661
fixup! refactor: modernize hid_denylist
Swiftb0y Sep 9, 2024
7420032
fixup! refactor: modernize hid_denylist
Swiftb0y Sep 9, 2024
564a318
fixup! refactor: restructure `bulk_supported` to emphasize kv structure
Swiftb0y Sep 9, 2024
46cf164
fixup! refactor: restructure `bulk_supported` to emphasize kv structure
Swiftb0y Sep 11, 2024
4df2508
refactor: remove OS-specific hidapi code
Swiftb0y Sep 13, 2024
18c8ce4
fixup! refactor: remove OS-specific hidapi code
Swiftb0y Sep 18, 2024
cc182df
fixup! fixup! refactor: remove OS-specific hidapi code
Swiftb0y Sep 18, 2024
60541eb
fixup! refactor: remove OS-specific hidapi code
Swiftb0y Sep 18, 2024
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
22 changes: 14 additions & 8 deletions src/controllers/bulk/bulkcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,14 @@ int BulkController::open() {
qCDebug(m_logBase) << "unable to automatically detach kernel driver for" << getName();
}

if (int ret = libusb_claim_interface(m_phandle, m_interfaceNumber); ret < 0) {
qCWarning(m_logBase) << "Cannot claim interface for" << getName()
<< ":" << libusb_error_name(ret);
libusb_close(m_phandle);
return -1;
if (m_interfaceNumber.has_value()) {
int error = libusb_claim_interface(m_phandle, *m_interfaceNumber);
if (error < 0) {
qCWarning(m_logBase) << "Cannot claim interface for" << getName()
<< ":" << libusb_error_name(error);
libusb_close(m_phandle);
return -1;
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved to the inner scope

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops of course. fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, of course. done

qCDebug(m_logBase) << "Claimed interface for" << getName();
}
Expand Down Expand Up @@ -237,9 +240,12 @@ int BulkController::close() {
stopEngine();

// Close device
if (int ret = libusb_release_interface(m_phandle, m_interfaceNumber); ret < 0) {
qCWarning(m_logBase) << "Cannot release interface for" << getName()
<< ":" << libusb_error_name(ret);
if (m_interfaceNumber.has_value()) {
int error = libusb_release_interface(m_phandle, *m_interfaceNumber);
if (error < 0) {
qCWarning(m_logBase) << "Cannot release interface for" << getName()
<< ":" << libusb_error_name(error);
}
}
qCInfo(m_logBase) << " Closing device";
libusb_close(m_phandle);
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/bulk/bulkcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <QAtomicInt>
#include <QThread>
#include <optional>

#include "controllers/controller.h"
#include "controllers/hid/legacyhidcontrollermapping.h"
Expand Down Expand Up @@ -76,7 +77,7 @@ class BulkController : public Controller {
std::uint16_t m_productId;
std::uint8_t m_inEndpointAddr;
std::uint8_t m_outEndpointAddr;
std::uint8_t m_interfaceNumber;
std::optional<std::uint8_t> m_interfaceNumber;

QString m_manufacturer;
QString m_product;
Expand Down
12 changes: 7 additions & 5 deletions src/controllers/bulk/bulksupported.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <cstdint>
#include <optional>

struct bulk_device_id {
std::uint16_t vendor_id;
daschuer marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -12,7 +13,8 @@ struct bulk_device_id {
struct bulk_device_endpoints {
std::uint8_t in_epaddr;
std::uint8_t out_epaddr;
std::uint8_t interface_number;
// we may not know the interface, in which case we should not try to claim it.
std::optional<std::uint8_t> interface_number = std::nullopt;
Copy link
Member

Choose a reason for hiding this comment

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

The initializer is redundant. I would rather make it explicit in the table, that the value is unknown and we need to fill it to make it work on Windows and macOs. In addition a brief to-do comment in the table is useful. We may also file a bug as a reminder that these devices are not yet usable with windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

are we sure they only don't work under windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. But it looks like @acolombier as added the claiming to enable the controllers in windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I saw that. but maybe it also just didn't work under windows because libusb didn't claim the right interface automatically (since from what I could tell an interface needs to be claimed anyways in order to do any operation on it). Do we want to rely on the automatic claiming (if it exists across all backends at all)?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like libusb is not able to automatically claim an interface. The auto feature is regarding detaching the kernel driver which is not supported on Windows.
But we need to test, I have no setup for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guessed so because I saw auto_claim in winusb but upon further research its not used anywhere else. I don't have a setup for it either, but I guess we can continue not claiming it until someone with the setup files an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Getting libusb running under Windows requires further changes. It is not functional yet, as we need to register a device driver (e.g. the generic WinUSB driver that comes with Windows) for the particular device type. This could be done using https://github.com/pbatard/libwdi but requires elevated user rights. Either in the Mixxx installer or at runtime of Mixxx itself with temporary elevated rights.
This is why BULK is default off for Windows.

};

struct bulk_support_lookup {
Expand All @@ -21,8 +23,8 @@ struct bulk_support_lookup {
};

constexpr static bulk_support_lookup bulk_supported[] = {
{{0x06f8, 0xb105}, {0x82, 0x03, 0}}, // Hercules MP3e2
{{0x06f8, 0xb107}, {0x83, 0x03, 0}}, // Hercules Mk4
{{0x06f8, 0xb100}, {0x86, 0x06, 0}}, // Hercules Mk2
{{0x06f8, 0xb120}, {0x82, 0x03, 0}}, // Hercules MP3 LE / Glow
{{0x06f8, 0xb105}, {0x82, 0x03}}, // Hercules MP3e2
{{0x06f8, 0xb107}, {0x83, 0x03}}, // Hercules Mk4
{{0x06f8, 0xb100}, {0x86, 0x06}}, // Hercules Mk2
{{0x06f8, 0xb120}, {0x82, 0x03}}, // Hercules MP3 LE / Glow
};