From af065df81d0a55909ac608b5b631cd70412c3627 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Mon, 2 Sep 2024 19:47:56 +0200 Subject: [PATCH 1/4] refactor: modernize bulksupport allowlist * move equality comparision closer to type definition. * remove trailing null-entry in support array * modernize allowlist search --- src/controllers/bulk/bulkcontroller.cpp | 25 ++++++++++++------------- src/controllers/bulk/bulkenumerator.cpp | 18 +++++++++--------- src/controllers/bulk/bulksupported.h | 24 +++++++++++++----------- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/controllers/bulk/bulkcontroller.cpp b/src/controllers/bulk/bulkcontroller.cpp index 6a2d1212b65..6260c3d6b43 100644 --- a/src/controllers/bulk/bulkcontroller.cpp +++ b/src/controllers/bulk/bulkcontroller.cpp @@ -2,6 +2,8 @@ #include +#include + #include "controllers/bulk/bulksupported.h" #include "controllers/defs_controllers.h" #include "moc_bulkcontroller.cpp" @@ -148,23 +150,20 @@ int BulkController::open() { } /* Look up endpoint addresses in supported database */ - int i; - for (i = 0; bulk_supported[i].vendor_id; ++i) { - if ((bulk_supported[i].vendor_id == m_vendorId) && - (bulk_supported[i].product_id == m_productId)) { - m_inEndpointAddr = bulk_supported[i].in_epaddr; - m_outEndpointAddr = bulk_supported[i].out_epaddr; -#if defined(__WINDOWS__) || defined(__APPLE__) - m_interfaceNumber = bulk_supported[i].interface_number; -#endif - break; - } - } - if (bulk_supported[i].vendor_id == 0) { + const bulk_supported_t* pDevice = std::find_if( + std::cbegin(bulk_supported), std::cend(bulk_supported), [this](const auto& dev) { + return dev.vendor_id == m_vendorId && dev.product_id == m_productId; + }); + if (pDevice == std::cend(bulk_supported)) { qCWarning(m_logBase) << "USB Bulk device" << getName() << "unsupported"; return -1; } + m_inEndpointAddr = pDevice->in_epaddr; + m_outEndpointAddr = pDevice->out_epaddr; +#if defined(__WINDOWS__) || defined(__APPLE__) + m_interfaceNumber = pDevice->interface_number; +#endif // XXX: we should enumerate devices and match vendor, product, and serial if (m_phandle == nullptr) { diff --git a/src/controllers/bulk/bulkenumerator.cpp b/src/controllers/bulk/bulkenumerator.cpp index 917e60b68e3..71792b413b7 100644 --- a/src/controllers/bulk/bulkenumerator.cpp +++ b/src/controllers/bulk/bulkenumerator.cpp @@ -21,14 +21,14 @@ BulkEnumerator::~BulkEnumerator() { libusb_exit(m_context); } -static bool is_interesting(struct libusb_device_descriptor *desc) { - for (int i = 0; bulk_supported[i].vendor_id; ++i) { - if ((bulk_supported[i].vendor_id == desc->idVendor) && - (bulk_supported[i].product_id == desc->idProduct)) { - return true; - } - } - return false; +static bool is_interesting(const libusb_device_descriptor& desc) { + auto vendorId = desc.idVendor; + auto productId = desc.idProduct; + return std::any_of(std::cbegin(bulk_supported), + std::cend(bulk_supported), + [=](const auto& dev) { + return dev.vendor_id == vendorId && dev.product_id == productId; + }); } QList BulkEnumerator::queryDevices() { @@ -43,7 +43,7 @@ QList BulkEnumerator::queryDevices() { struct libusb_device_descriptor desc; libusb_get_device_descriptor(device, &desc); - if (is_interesting(&desc)) { + if (is_interesting(desc)) { struct libusb_device_handle* handle = nullptr; err = libusb_open(device, &handle); if (err) { diff --git a/src/controllers/bulk/bulksupported.h b/src/controllers/bulk/bulksupported.h index 4d39b0d1ba3..4a86f0ec84f 100644 --- a/src/controllers/bulk/bulksupported.h +++ b/src/controllers/bulk/bulksupported.h @@ -1,18 +1,20 @@ -// A list of supported USB bulk devices - #pragma once -typedef struct bulk_supported { - unsigned short vendor_id; - unsigned short product_id; - unsigned char in_epaddr; - unsigned char out_epaddr; - unsigned int interface_number; -} bulk_supported_t; +#include + +// A list of supported USB bulk devices + +struct bulk_supported_t { + std::uint16_t vendor_id; + std::uint16_t product_id; + std::uint8_t in_epaddr; + std::uint8_t out_epaddr; + std::uint8_t interface_number; +}; -static bulk_supported_t bulk_supported[] = { +constexpr static bulk_supported_t 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 - {0, 0, 0, 0, 0}}; +}; From bbbbb1a6d617af7935f4dbea8bfc85a163cb24b7 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Mon, 2 Sep 2024 20:01:51 +0200 Subject: [PATCH 2/4] refactor: modernize hid_denylist * replace magic (unnamed) values with proper constants * use idiomatic array iteration --- src/controllers/hid/hiddenylist.h | 34 +++++++++++++++++++-------- src/controllers/hid/hidenumerator.cpp | 32 +++++++++---------------- 2 files changed, 35 insertions(+), 31 deletions(-) diff --git a/src/controllers/hid/hiddenylist.h b/src/controllers/hid/hiddenylist.h index f62c3303e06..4011b98318b 100644 --- a/src/controllers/hid/hiddenylist.h +++ b/src/controllers/hid/hiddenylist.h @@ -1,21 +1,35 @@ #pragma once -typedef struct hid_denylist { +// TODO: unify this with the invalid interfacenumber from the bulkenumerator +constexpr static int kInvalidInterfaceNumber = -1; +constexpr static unsigned short kAnyValue = 0x0; + +struct hid_denylist_t { unsigned short vendor_id; unsigned short product_id; unsigned short usage_page; unsigned short usage; - int interface_number; -} hid_denylist_t; + int interface_number = kInvalidInterfaceNumber; +}; /// USB HID device that should not be recognized as controllers -constexpr hid_denylist_t hid_denylisted[] = { - {0x1157, 0x300, 0x1, 0x2, -1}, // EKS Otus mouse pad (OS/X,windows) - {0x1157, 0x300, 0x0, 0x0, 0x3}, // EKS Otus mouse pad (linux) - {0x04f3, 0x2d26, 0x0, 0x0, -1}, // ELAN2D26:00 Touch screen - {0x046d, 0xc539, 0x0, 0x0, -1}, // Logitech G Pro Wireless +constexpr static hid_denylist_t hid_denylisted[] = { + {0x1157, 0x300, 0x1, 0x2}, // EKS Otus mouse pad (OS/X,windows) + {0x1157, 0x300, kAnyValue, kAnyValue, 0x3}, // EKS Otus mouse pad (linux) + {0x04f3, 0x2d26, kAnyValue, kAnyValue}, // ELAN2D26:00 Touch screen + {0x046d, 0xc539, kAnyValue, kAnyValue}, // Logitech G Pro Wireless // The following rules have been created using the official USB HID page // spec as specified at https://usb.org/sites/default/files/hut1_4.pdf - {0x0, 0x0, 0x0D, 0x04, -1}, // Touch Screen - {0x0, 0x0, 0x0D, 0x22, -1}, // Finger + { + kAnyValue, + kAnyValue, + 0x0D, + 0x04, + }, // Touch Screen + { + kAnyValue, + kAnyValue, + 0x0D, + 0x22, + }, // Finger }; diff --git a/src/controllers/hid/hidenumerator.cpp b/src/controllers/hid/hidenumerator.cpp index c4127ae16aa..af4c33232f5 100644 --- a/src/controllers/hid/hidenumerator.cpp +++ b/src/controllers/hid/hidenumerator.cpp @@ -33,36 +33,26 @@ bool recognizeDevice(const hid_device_info& device_info) { } // Exclude specific devices from the denylist. - bool interface_number_valid = device_info.interface_number != -1; - const int denylist_len = sizeof(hid_denylisted) / sizeof(hid_denylisted[0]); - for (int bl_index = 0; bl_index < denylist_len; bl_index++) { - hid_denylist_t denylisted = hid_denylisted[bl_index]; + for (const hid_denylist_t& denylisted : hid_denylisted) { // If vendor ids are specified and do not match, skip. - if (denylisted.vendor_id && device_info.vendor_id != denylisted.vendor_id) { + if (denylisted.vendor_id != kAnyValue && + device_info.vendor_id != denylisted.vendor_id) { continue; } // If product IDs are specified and do not match, skip. - if (denylisted.product_id && device_info.product_id != denylisted.product_id) { + if (denylisted.product_id != kAnyValue && + device_info.product_id != denylisted.product_id) { continue; } // Denylist entry based on interface number - if (denylisted.interface_number != -1) { - // Skip matching for devices without usage info. - if (!interface_number_valid) { - continue; - } - // If interface number is present and the interface numbers do not - // match, skip. - if (device_info.interface_number != denylisted.interface_number) { - continue; - } + // If interface number is present and the interface numbers do not + // match, skip. + if (denylisted.interface_number != kInvalidInterfaceNumber && + device_info.interface_number != denylisted.interface_number) { + continue; } // Denylist entry based on usage_page and usage (both required) - if (denylisted.usage_page != 0 && denylisted.usage != 0) { - // Skip matching for devices with no usage_page/usage info. - if (device_info.usage_page == 0 && device_info.usage == 0) { - continue; - } + if (denylisted.usage_page != kAnyValue && denylisted.usage != kAnyValue) { // If usage_page is different, skip. if (device_info.usage_page != denylisted.usage_page) { continue; From f034482228449e8223db5abec9c7e22311334d28 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Mon, 9 Sep 2024 14:21:28 +0200 Subject: [PATCH 3/4] refactor: restructure `bulk_supported` to emphasize kv structure --- src/controllers/bulk/bulkcontroller.cpp | 10 +++++----- src/controllers/bulk/bulkenumerator.cpp | 6 ++---- src/controllers/bulk/bulksupported.h | 24 ++++++++++++++++-------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/controllers/bulk/bulkcontroller.cpp b/src/controllers/bulk/bulkcontroller.cpp index 6260c3d6b43..24c9e1ee7b0 100644 --- a/src/controllers/bulk/bulkcontroller.cpp +++ b/src/controllers/bulk/bulkcontroller.cpp @@ -151,18 +151,18 @@ int BulkController::open() { /* Look up endpoint addresses in supported database */ - const bulk_supported_t* pDevice = std::find_if( + const bulk_support_lookup* pDevice = std::find_if( std::cbegin(bulk_supported), std::cend(bulk_supported), [this](const auto& dev) { - return dev.vendor_id == m_vendorId && dev.product_id == m_productId; + return dev.key.vendor_id == m_vendorId && dev.key.product_id == m_productId; }); if (pDevice == std::cend(bulk_supported)) { qCWarning(m_logBase) << "USB Bulk device" << getName() << "unsupported"; return -1; } - m_inEndpointAddr = pDevice->in_epaddr; - m_outEndpointAddr = pDevice->out_epaddr; + m_inEndpointAddr = pDevice->endpoints.in_epaddr; + m_outEndpointAddr = pDevice->endpoints.out_epaddr; #if defined(__WINDOWS__) || defined(__APPLE__) - m_interfaceNumber = pDevice->interface_number; + m_interfaceNumber = pDevice->endpoints.interface_number; #endif // XXX: we should enumerate devices and match vendor, product, and serial diff --git a/src/controllers/bulk/bulkenumerator.cpp b/src/controllers/bulk/bulkenumerator.cpp index 71792b413b7..4435037ff11 100644 --- a/src/controllers/bulk/bulkenumerator.cpp +++ b/src/controllers/bulk/bulkenumerator.cpp @@ -22,12 +22,10 @@ BulkEnumerator::~BulkEnumerator() { } static bool is_interesting(const libusb_device_descriptor& desc) { - auto vendorId = desc.idVendor; - auto productId = desc.idProduct; return std::any_of(std::cbegin(bulk_supported), std::cend(bulk_supported), - [=](const auto& dev) { - return dev.vendor_id == vendorId && dev.product_id == productId; + [&](const auto& dev) { + return dev.key.vendor_id == desc.idVendor && dev.key.product_id == desc.idProduct; }); } diff --git a/src/controllers/bulk/bulksupported.h b/src/controllers/bulk/bulksupported.h index 4a86f0ec84f..49d3f05a636 100644 --- a/src/controllers/bulk/bulksupported.h +++ b/src/controllers/bulk/bulksupported.h @@ -2,19 +2,27 @@ #include -// A list of supported USB bulk devices - -struct bulk_supported_t { +struct bulk_device_id { std::uint16_t vendor_id; std::uint16_t product_id; +}; + +// A list of supported USB bulk devices + +struct bulk_device_endpoints { std::uint8_t in_epaddr; std::uint8_t out_epaddr; std::uint8_t interface_number; }; -constexpr static bulk_supported_t 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 +struct bulk_support_lookup { + bulk_device_id key; + bulk_device_endpoints endpoints; +}; + +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 }; From 45363b7c3107c41f224b44184972a821345f29b2 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Fri, 13 Sep 2024 21:57:05 +0200 Subject: [PATCH 4/4] refactor: remove OS-specific hidapi code --- src/controllers/bulk/bulkcontroller.cpp | 38 +++++++------------------ src/controllers/bulk/bulkcontroller.h | 14 ++++----- src/controllers/bulk/bulksupported.h | 13 +++++---- 3 files changed, 26 insertions(+), 39 deletions(-) diff --git a/src/controllers/bulk/bulkcontroller.cpp b/src/controllers/bulk/bulkcontroller.cpp index 24c9e1ee7b0..aacced6ecbb 100644 --- a/src/controllers/bulk/bulkcontroller.cpp +++ b/src/controllers/bulk/bulkcontroller.cpp @@ -132,12 +132,10 @@ bool BulkController::matchProductInfo(const ProductInfo& product) { return false; } -#if defined(__WINDOWS__) || defined(__APPLE__) value = product.interface_number.toInt(&ok, 16); - if (!ok || m_interfaceNumber != static_cast(value)) { + if (!ok || m_interfaceNumber != value) { return false; } -#endif // Match found return true; @@ -161,9 +159,7 @@ int BulkController::open() { } m_inEndpointAddr = pDevice->endpoints.in_epaddr; m_outEndpointAddr = pDevice->endpoints.out_epaddr; -#if defined(__WINDOWS__) || defined(__APPLE__) m_interfaceNumber = pDevice->endpoints.interface_number; -#endif // XXX: we should enumerate devices and match vendor, product, and serial if (m_phandle == nullptr) { @@ -175,31 +171,21 @@ int BulkController::open() { qCWarning(m_logBase) << "Unable to open USB Bulk device" << getName(); return -1; } - -#if defined(__WINDOWS__) || defined(__APPLE__) - if (m_interfaceNumber && libusb_kernel_driver_active(m_phandle, m_interfaceNumber) == 1) { - qCDebug(m_logBase) << "Found a driver active for" << getName(); - if (libusb_detach_kernel_driver(m_phandle, 0) == 0) - qCDebug(m_logBase) << "Kernel driver detached for" << getName(); - else { - qCWarning(m_logBase) << "Couldn't detach kernel driver for" << getName(); - libusb_close(m_phandle); - return -1; - } + if (libusb_set_auto_detach_kernel_driver(m_phandle, true) == LIBUSB_ERROR_NOT_SUPPORTED) { + qCDebug(m_logBase) << "unable to automatically detach kernel driver for" << getName(); } - if (m_interfaceNumber) { - int ret = libusb_claim_interface(m_phandle, m_interfaceNumber); - if (ret < 0) { + 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(ret); + << ":" << libusb_error_name(error); libusb_close(m_phandle); return -1; } else { qCDebug(m_logBase) << "Claimed interface for" << getName(); } } -#endif setOpen(true); startEngine(); @@ -254,15 +240,13 @@ int BulkController::close() { stopEngine(); // Close device -#if defined(__WINDOWS__) || defined(__APPLE__) - if (m_interfaceNumber) { - int ret = libusb_release_interface(m_phandle, m_interfaceNumber); - if (ret < 0) { + 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(ret); + << ":" << libusb_error_name(error); } } -#endif qCInfo(m_logBase) << " Closing device"; libusb_close(m_phandle); m_phandle = nullptr; diff --git a/src/controllers/bulk/bulkcontroller.h b/src/controllers/bulk/bulkcontroller.h index f7376c8ccf9..f2f1b0e9416 100644 --- a/src/controllers/bulk/bulkcontroller.h +++ b/src/controllers/bulk/bulkcontroller.h @@ -2,6 +2,7 @@ #include #include +#include #include "controllers/controller.h" #include "controllers/hid/legacyhidcontrollermapping.h" @@ -72,13 +73,12 @@ class BulkController : public Controller { // Local copies of things we need from desc - unsigned short m_vendorId; - unsigned short m_productId; - unsigned char m_inEndpointAddr; - unsigned char m_outEndpointAddr; -#if defined(__WINDOWS__) || defined(__APPLE__) - unsigned int m_interfaceNumber; -#endif + std::uint16_t m_vendorId; + std::uint16_t m_productId; + std::uint8_t m_inEndpointAddr; + std::uint8_t m_outEndpointAddr; + std::optional m_interfaceNumber; + QString m_manufacturer; QString m_product; diff --git a/src/controllers/bulk/bulksupported.h b/src/controllers/bulk/bulksupported.h index 49d3f05a636..a7bf6c916d6 100644 --- a/src/controllers/bulk/bulksupported.h +++ b/src/controllers/bulk/bulksupported.h @@ -1,6 +1,7 @@ #pragma once #include +#include struct bulk_device_id { std::uint16_t vendor_id; @@ -12,7 +13,9 @@ 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. + // these devices are likely unusable on windows without claiming the correct interface. + std::optional interface_number; }; struct bulk_support_lookup { @@ -21,8 +24,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, std::nullopt}}, // Hercules MP3e2 + {{0x06f8, 0xb107}, {0x83, 0x03, std::nullopt}}, // Hercules Mk4 + {{0x06f8, 0xb100}, {0x86, 0x06, std::nullopt}}, // Hercules Mk2 + {{0x06f8, 0xb120}, {0x82, 0x03, std::nullopt}}, // Hercules MP3 LE / Glow };