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 14 commits
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
69 changes: 23 additions & 46 deletions src/controllers/bulk/bulkcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include <libusb.h>

#include <algorithm>

#include "controllers/bulk/bulksupported.h"
#include "controllers/defs_controllers.h"
#include "moc_bulkcontroller.cpp"
Expand Down Expand Up @@ -130,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<unsigned int>(value)) {
if (!ok || m_interfaceNumber != value) {
return false;
}
#endif

// Match found
return true;
Expand All @@ -148,23 +148,18 @@ 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_support_lookup* pDevice = std::find_if(
std::cbegin(bulk_supported), std::cend(bulk_supported), [this](const auto& dev) {
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->endpoints.in_epaddr;
m_outEndpointAddr = pDevice->endpoints.out_epaddr;
m_interfaceNumber = pDevice->endpoints.interface_number;

// XXX: we should enumerate devices and match vendor, product, and serial
if (m_phandle == nullptr) {
Expand All @@ -176,31 +171,18 @@ 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) {
qCWarning(m_logBase) << "Cannot claim interface for" << getName()
<< ":" << libusb_error_name(ret);
libusb_close(m_phandle);
return -1;
} else {
qCDebug(m_logBase) << "Claimed interface for" << getName();
}
if (int ret = libusb_claim_interface(m_phandle, m_interfaceNumber); ret < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid this confusing style with an assignment in an if condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this confusing? Its just a new syntax which makes sense here since I don't need ret outside the if-body
https://en.cppreference.com/w/cpp/language/if#if_statements_with_initializer

Copy link
Member

Choose a reason for hiding this comment

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

Because it break the flow when reading the code. You expect an if condition like everywhere else but you read the assignment, which is every where else in the line before. This reduces IMHO maintainability and lead to misunderstandings. The benefit of the tight scope is negligible here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The benefit of the tight scope is negligible here.

I disagree actually because if the variable does not get its own scope the variables storing the return values either need different names or the one variable needs to be reassigned. The different names is confusing because that leads me to believe that we're actually interested in this value while reassigning looks weird because the added mutability makes the code hard to reason about.

You expect an if condition like everywhere else but you read the assignment, which is every where else in the line before.

Well you only expect the condition there because you're not used to there being something else but this is a well designed and documented feature so its reasonable to expect it to be usable. The same applies to other "new" features such as lambdas or trailing return types.

Let me reiterate that this very much distinct from the relying on return value of operator= (eg. if (int var = f()) because that is mixing an init-statement with a condition while the latter one is clearly defined.

This is just a case of "Was der Bauer nicht kennt, frisst er nicht".

I won't insist on using this style, but resisting to embrace new and encouraged features and styles is a common pattern in code review. This is neither rewarding nor productive.

Copy link
Member

Choose a reason for hiding this comment

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

I have considered this a bit and it turns out that this is a general code style question, we should discuss on Zulip.

We have a plenty of places where we may consider to tighten the scope of a local variable, because we can. On the other hand we don't care about them, because it will just introduce boiler plate and nesting, interrupting the flow when reading the code. This is an issue when you only skim over the code, and especially when the if condition is broken over two lines of code.

I have no interest to ban this new statement from the Mixxx code entirely, but let's use it deliberately, backed up by a rule in our style guide.

This is here an edge case because we have two return values that will end up in the same variable of temporary nature otherwise. For now this is a new style, that does not fit to the rest of Mixxx. I am open to decide in any direction, but let's do it consistently.

I will not block the PR on this point, but prefer reusing ret here without a scope.

Copy link
Member

Choose a reason for hiding this comment

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

https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/C.2B.2B17.20if.20with.20init-statement

qCWarning(m_logBase) << "Cannot claim interface for" << getName()
<< ":" << libusb_error_name(ret);
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();
}
#endif

setOpen(true);
startEngine();
Expand Down Expand Up @@ -255,15 +237,10 @@ 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) {
qCWarning(m_logBase) << "Cannot release interface for" << getName()
<< ":" << libusb_error_name(ret);
}
if (int ret = libusb_release_interface(m_phandle, m_interfaceNumber); ret < 0) {
qCWarning(m_logBase) << "Cannot release interface for" << getName()
<< ":" << libusb_error_name(ret);
}
#endif
qCInfo(m_logBase) << " Closing device";
libusb_close(m_phandle);
m_phandle = nullptr;
Expand Down
13 changes: 6 additions & 7 deletions src/controllers/bulk/bulkcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,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::uint8_t m_interfaceNumber;
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 become an int, to match the usbapi type and to allow us to retain the 2.5 behaviour for the devices where we don't know the interface number by setting it to -1.

Copy link
Member Author

@Swiftb0y Swiftb0y Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

std::optional<std::uint8_t> would be so much more appropriate though.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, when starting on a plan surface.

Libusb has decided to use -1 for invalid, so we should IMHO not introduce another concept. The API will just work with -1 and has internal checks, no need for any has_value() checks, in our code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Libusb has decided to use -1 for invalid,

I was not able to find that definition. Can you link it to me?

The API will just work with -1 and has internal checks, no need for any has_value() checks, in our code.

Well, if we want to preserve how it works currently though, we need to check for -1 anyways, so we might as well check for has_value. I'm not sure if passing -1 deliberately is a good choice because it'll only make it more complicated to then handle the error (because we need to then distinguish from an error that occurred because we passed in -1 or we actually did pass in a garbage value we didn't account for).

Copy link
Member

Choose a reason for hiding this comment

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

It is just used literally in libusb like here:
https://github.com/libusb/libusb/blob/467b6a8896daea3d104958bf0887312c5d14d150/libusb/descriptor.c#L221
and
https://github.com/libusb/libusb/blob/467b6a8896daea3d104958bf0887312c5d14d150/libusb/os/windows_winusb.c#L2927

The only related macro is USB_MAXINTERFACES

Copy link
Member Author

@Swiftb0y Swiftb0y Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From what I could tell is that @DaSchauer wants to introduce a sentinel value (-1 currently) that determines whether we should claim the interface at all. At least the current code only claimed an interface when it was non-zero. Whether that is correct is dubious so I just implemented it to claim it unconditionally.

While I don't object to that outright (it worked until now apparently, does anybody know better what to?), I insist on using a typesafe sentinel (std::optional (specifically std::nullopt)).

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyways, I think we should try the state as currently implemented. None of us has a hercules controller anyways so its not like we can easily verify that this breaks something.

Copy link
Member

Choose a reason for hiding this comment

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

It is impossible to transfer data to or from an USB device without specifiing the interface (there might be a default, but it must be specified somehow). This is mandatory by the standard.
But hardcoding the Endpoint Addresses is not in sense of the standard, as these should be read back from the descriptors in the device, taking into account the currently active device configuration and alternate mode of the interface .

Copy link
Member

@daschuer daschuer Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Claiming interfaces has been introduced recently in this PR #13008.
The code must have been worked before at least on Linux without claiming the interfaces. I have read that the Kernel automatically detaches the driver and frees up the interface for the user space application without explicit claiming.

The libusb implementation is implemented in a way that a handle can claim one interface or more. Transferring data works with the endpoint address only, without specifying the interface. So I assume that the endpoint address alone is sufficient for transferring the data. Claiming the interface seems to be only an exclusive access thing.

Searching the forum has revealed that the interface number is not always 0.

It is impossible to transfer data to or from an USB device without specifying the interface

I am afraid this is wrong. My conclusion is that claiming the interface 0 here for all devices is also wrong, since we don't have the knowledge which interface to claim. As a workaround we may restore the pre #13008 by not claiming the interface indicated by -1 or std::nullopt if you wish.
So we may call the variable:
kClaimNoInterface = -1;
or
kInterfaceUnknown = -1;

Copy link
Member

Choose a reason for hiding this comment

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

I agree, if we don't know the correct interface number, we shouldn't assume that the device use 0.


QString m_manufacturer;
QString m_product;

Expand Down
16 changes: 7 additions & 9 deletions src/controllers/bulk/bulkenumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,12 @@ 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) {
return std::any_of(std::cbegin(bulk_supported),
std::cend(bulk_supported),
[&](const auto& dev) {
return dev.key.vendor_id == desc.idVendor && dev.key.product_id == desc.idProduct;
});
}

QList<Controller*> BulkEnumerator::queryDevices() {
Expand All @@ -43,7 +41,7 @@ QList<Controller*> 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) {
Expand Down
38 changes: 24 additions & 14 deletions src/controllers/bulk/bulksupported.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,28 @@
#pragma once

#include <cstdint>

struct bulk_device_id {
std::uint16_t vendor_id;
daschuer marked this conversation as resolved.
Show resolved Hide resolved
std::uint16_t product_id;
};

// A list of supported USB bulk devices

#pragma once
struct bulk_device_endpoints {
std::uint8_t in_epaddr;
std::uint8_t out_epaddr;
daschuer marked this conversation as resolved.
Show resolved Hide resolved
std::uint8_t interface_number;
Copy link
Member

Choose a reason for hiding this comment

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

interface_number is int in libusb. Is that an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

where do you get that from? its uint8 in the relevant struct. https://github.com/libusb/libusb/blob/467b6a8896daea3d104958bf0887312c5d14d150/libusb/libusb.h#L749

Copy link
Member

Choose a reason for hiding this comment

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

strange, everywhere else in libusb.h it is an int.

Copy link
Member

Choose a reason for hiding this comment

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

You point to bInterfaceNumber which is a byte. Everywhere else it is used as int because of -1 for invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, so int16_t or std::optional<std::uint8_t> or int? for the int you'll have to battle it out with @JoergAtGithub because he requested it

Copy link
Member

Choose a reason for hiding this comment

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

We have the option to revert the change here and carry on, there was no user visible issue with the old types, was it?

If we decide for a new type here we may adopt
int, because the API will convert the given value to int anyway. What does the 0 below actually mean? Reading the code and the comment, it could be an unused value what we may want to indicate with -1 or is it actually interface 0?

Where is the term "tweak" coming from and why is it referenced as value in the structure and not as tweak?

Copy link
Member

Choose a reason for hiding this comment

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

The interface number along with 0 have been introduced in a bulk action here:

5e162f8

What does 0 mean? Why are all 0?

Copy link
Member Author

@Swiftb0y Swiftb0y Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We have the option to revert the change here and carry on, there was no user visible issue with the old types, was it?

Not really. I can revert if that is seriously a condition to merge... But fixing user-visible issues is not really the point of a refactor, is it?

What does the 0 below actually mean? Reading the code and the comment, it could be an unused value what we may want to indicate with -1 or is it actually interface 0?

Its the default. 0 on most devices, but not all (thus the table).

Where is the term "tweak" coming from and why is it referenced as value in the structure and not as tweak?

Because those are the nonstandard values we actually want to tweak. I can also call it "quirk", which is the terminology the linux kernel uses for this sort of thing.

Why are all 0?

Because the only device that has a non-zero interface has not been added yet.
https://github.com/acolombier/mixxx/blob/881a482f2000bf888fd9599922129981b00ac8aa/src/controllers/bulk/bulksupported.h#L13-L18

Copy link
Member

Choose a reason for hiding this comment

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

It's just numbering of the interfaces. We currently just support bulk devices from the manufacturer Hercules. And Hercules always use the first interface for bulk and the second for audio. This order is completely up to the manufacturer.

Copy link
Member

Choose a reason for hiding this comment

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

At least untill all numbers are known we need here int(-1) for invalid. see my other comment.

Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved
};

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;
struct bulk_support_lookup {
bulk_device_id key;
bulk_device_endpoints endpoints;
};

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}};
constexpr static bulk_support_lookup bulk_supported[] = {
{{0x06f8, 0xb105}, {0x82, 0x03, 0}}, // Hercules MP3e2
{{0x06f8, 0xb107}, {0x83, 0x03, 0}}, // Hercules Mk4
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly the MK4 is reported here at:
https://mixxx.discourse.group/t/hercules-mk4-hid-under-linux-and-1-11/13447/3
{0x83, 0x02, 0} and {0x83, 0x03, 5}
Can we have the endpoint address twice? Is that the same end point?

Copy link
Member Author

Choose a reason for hiding this comment

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

no idea. @JoergAtGithub may know.

Copy link
Member

Choose a reason for hiding this comment

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

Workaround: Add a TODO, and use -1 as interfac number.

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly the MK4 is reported here at: https://mixxx.discourse.group/t/hercules-mk4-hid-under-linux-and-1-11/13447/3 {0x83, 0x02, 0} and {0x83, 0x03, 5} Can we have the endpoint address twice? Is that the same end point?

This is not the same USB endpoint, because only the address 0x83 is identical, but not the number of the endpoint (0x02 vs. 0x03).

Endpoints and Interfaces are fields on different USB protocol layers:

  • Endpoints are raw hardware addresses which the operating systems USB kernel driver uses to prioritize the data transports with guranteed latency (Isochronous or Interupt transport mode) and non real-time transports (Control or Bulk transport mode)
  • Interfaces/Pipes are the a protocol level higher and depend not only on the hardware, but also on configuration from the host software (drivers like power-management or user mode applications). You can assign a Endpoint only to one USB Interface, otherwise you could get conflicting access. But an interface can run in different alternative modes (e.g. a camera can run in realtime mode for video using the endpoint Isocronous transfer mode or in high-resolution photo mode using the same endpoint in Bulk transfer mode instead).

See: https://beyondlogic.org/usbnutshell/usb5.shtml

Copy link
Member

Choose a reason for hiding this comment

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

I am still confused, because we don't use endpoint number in our code only the address.
Just double checked but I don't see the double 0x83 anymore, instead I found a double 0x81 with:
Interface 1 Endpoint Address 0x81
Interface 3 Endpoint Address 0x81
Interface 4 Endpoint Address 0x81

Copy link
Member

@JoergAtGithub JoergAtGithub Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This information is not unambiguous. If you talk about the transport layer, you need to specify the pair of Endpoint Address and Endpoint Number:
endpoint

Copy link
Member

Choose a reason for hiding this comment

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

It is still puzzling form me, because we do not consider the Interface number with Linux at all but it seams to work with the enpoint adress only, even without an Endpoint number. libusb_claim_interface() is written in a way that we may claim multiple interfaces with one handel.

mixxx/src/controllers/bulk/bulkcontroller.cpp

Line 199 in 4df2508

m_pReader = new BulkReader(m_phandle, m_inEndpointAddr);

libusb_bulk_transfer

Here we use only the enpoint adress
https://github.com/mixxxdj/mixxx/blob/4df25086d1aa060dcc3b2793bc5be20c7380e0a1/src/controllers/bulk/bulkcontroller.cpp#L42C18-L42C38

The underlying questions is what is the interface number for -1 and will it work like before?
How does the driver know which endpoint shall be used if there are tw owith the same address.
Or are they the same listed in two interfaces?

Copy link
Member

Choose a reason for hiding this comment

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

I am still confused, because we don't use endpoint number in our code only the address.
Just double checked but I don't see the double 0x83 anymore, instead I found a double 0x81 with:
Interface 1 Endpoint Address 0x81
Interface 3 Endpoint Address 0x81
Interface 4 Endpoint Address 0x81

I double checked this, and I found out, that the Endpoint-Number is part of the Endpoint-Address:
Each Interfaces can have up to 16 Endpoints and the address is a single byte build as follows:

  • Bits 0..3b Endpoint Number.
  • Bits 4..6b Reserved. Set to Zero
  • Bits 7 Direction 0 = Out, 1 = In (Ignored for Control Endpoints)

intftree

See https://beyondlogic.org/usbnutshell/usb5.shtml for reference

{{0x06f8, 0xb100}, {0x86, 0x06, 0}}, // Hercules Mk2
{{0x06f8, 0xb120}, {0x82, 0x03, 0}}, // Hercules MP3 LE / Glow
Copy link
Member

Choose a reason for hiding this comment

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

According to this post:
https://mixxx.discourse.group/t/ubuntu-mint-hercules-dj-control-glow-not-detected/16207
The in_epaddr of Hercules DJControl MP3 LE is 0x82 in interface 1.
So it looks like assuming 0 was wrong here.

Copy link
Member

Choose a reason for hiding this comment

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

The interface number is also there for Linux and the API documentation does not exclude the usage there:
But

libusb_kernel_driver_active(m_phandle, m_interfaceNumber)
libusb_claim_interface(m_phandle, m_interfaceNumber)

This is skipped in our implementation for Linux.

However I can read here https://github.com/libusb/libusb/blob/467b6a8896daea3d104958bf0887312c5d14d150/libusb/core.c#L2072C4-L2072C51:
// This functionality is not available on Windows.

But I think it is intended to be always called and then checked for LIBUSB_ERROR_NOT_SUPPORTED

Conclusion: something is broken here.

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 agree, but I'm doubtful there is much to fix if we don't know what and have no hardware to test.

Copy link
Member

Choose a reason for hiding this comment

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

@acolombier Do you have a device to test it?

My idea is that we need to remove all conditional code and just let libusb do the OS abstarction.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be ideal, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

This means also that we must use in the table above -1 for retaining the old behaviour that might be broken on windows, until we know the real interface number and enable the device on windows. What do you think?

Why is the situation on windows special?

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, it looks like it was working on Linux without specifying the interface when it has been introduced.

I hope @acolombier can give some insight as he has enabled bulk support for windows here:
5e162f8

Copy link
Member

Choose a reason for hiding this comment

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

I have the S4 Mk3 to test, which has non-zero interface. I have add the interface number, which was missing in the previous implementation, so I needed a none value to exclude the other device on which I couldn't confirm the value.

// This functionality is not available on Windows.

Without libusb_kernel_driver_active and libusb_claim_interface, I was unable to use my device on Windows. Note I have tested on a VM so perhaps there is some different behaviour to expect?

Copy link
Member

Choose a reason for hiding this comment

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

Also, note that libusb_kernel_driver_active/libusb_claim_interface should also be used on Linux. The reason for me excluding it is that the original Linux-only implementation didn't include it, and as I was implementing for Mac & Windows , I changed only the logic on these platform, but it appears to be a global oversight

Copy link
Member

Choose a reason for hiding this comment

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

libusb_claim_interface() is the one that is available on windows and probably the required one in your case.

};
34 changes: 24 additions & 10 deletions src/controllers/hid/hiddenylist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
daschuer marked this conversation as resolved.
Show resolved Hide resolved
} 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
};
32 changes: 11 additions & 21 deletions src/controllers/hid/hidenumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down