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.

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

use hidraw backend of hidapi instead of libusb backend #4054

Merged
merged 8 commits into from
Jul 7, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jul 3, 2021

This has two advantages:

  1. Bluetooth HID devices are supported. These are relatively rare, but we have had a request to support these before: https://mixxx.discourse.group/t/bluetooth-remote/16195/8
  2. HID reports of more than 64 bytes and libusb backend libusb/hidapi#274 does not affect the hidraw backend, so the hacks around this bug can be removed for the Kontrol S4 Mk2 & Mk3 mappings.

The disadvantage is that hid_get_input_report for explicitly requesting a specific input report is not yet implemented for the hidraw backend. I will work on this upstream. That is used by #3038 but not in any mappings currently in the main branch.

@github-actions github-actions bot added the build label Jul 3, 2021
@Be-ing Be-ing force-pushed the hidapi_hidraw branch 7 times, most recently from 6774b9f to 6b1650b Compare July 3, 2021 17:40
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 3, 2021

@JoergAtGithub what are you thoughts on this?

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 3, 2021

Note that libusb is still used as a fallback for BSD.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I don't have any objections. But since this is a major change I would like to collect more feedback before merging.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 4, 2021

Hopefully we can remove our own Findhidapi.cmake module soon since CMake support was just merged upstream.

@JoergAtGithub
Copy link
Member

JoergAtGithub commented Jul 6, 2021

I think hidraw would be the best solution for Linux, if libusb/hidapi#259 get implemented, which should be trivial. The only reason, that it's not in hidraw is, that the necessary API was not available in very old kernels.

What happens if a mapping uses hid_get_input_report? What does it return if called with hidraw backend? Random data, which initialize all knobs and faders to random positions at startup, would be a problem.

Removing this ugly fallback code from the mappings would mean, that they can't work with sytems like BSD anymore. But are there Mixxx builds for BSD?

@JoergAtGithub
Copy link
Member

Do hidraw devices work with the same udev settings as libusb devices, or must existing Linux users adapt there settings for hidraw?

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 6, 2021

Do hidraw devices work with the same udev settings as libusb devices, or must existing Linux users adapt there settings for hidraw?

No, I adapted the udev rule file in this branch.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 6, 2021

What happens if a mapping uses hid_get_input_report? What does it return if called with hidraw backend? Random data, which initialize all knobs and faders to random positions at startup, would be a problem.

I have not tested. This is no issue now though because no merged mapping uses it. I would like to try it with my S4 Mk3, so I'll work on implementing it upstream.

@JoergAtGithub
Copy link
Member

The following comment says udev rules are for HID and USB bulk:

# This udev rule allows Mixxx to access HID and USB Bulk controllers when running as a normal user

Didn't the later still need the old udev rules on Linux?

needed for hidraw backend of hidapi
The hidraw backend supports Bluetooth HID. This also works around
a bug in the libusb backend affecting the Native Instruments
Traktor Kontrol S4 Mk2 & Mk3:
libusb/hidapi#274
HID devices can use USB or Bluetooth.
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 6, 2021

As the comments lower in the udev rule file says, older Hercules and Native Instruments controllers are accessed via the USB Bulk endpoints through libusb, in addition to the newer HID controllers.

Merge?

@Holzhaus
Copy link
Member

Holzhaus commented Jul 6, 2021

Thank you.

I'm a bit worried because this backend only works on Linux. This means that any mapping for controllers that send more than 64 byte reports would either work on Linux or on Windows, but not both. This means we should try to help upstream to resolve that issue, even if it's not present on Linux with hidraw.

Regarding hid_get_input_report: Please add a comment near that method that explains that it's not working with hidraw.

Other than that, the PR looks good, but I have no HID controller to test it with. It would be good if someone confirms that it works fine.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 6, 2021

You're misunderstanding. The bug with splitting packets over 64 bytes only occurs with the libusb backend. It does not occur on Windows nor macOS. This PR is fixing that so it works the same on Linux as Windows and macOS.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 6, 2021

Regarding hid_get_input_report: Please add a comment near that method that explains that it's not working with hidraw.

Done: f17c8bf

@uklotzde
Copy link
Contributor

uklotzde commented Jul 6, 2021

Ok, then I'll do a quick test tomorrow to check if it actually works. Using a different controller would be helpful, if someone owns one.

@Holzhaus
Copy link
Member

Holzhaus commented Jul 7, 2021

You're misunderstanding. The bug with splitting packets over 64 bytes only occurs with the libusb backend. It does not occur on Windows nor macOS. This PR is fixing that so it works the same on Linux as Windows and macOS.

Thanks for clarifying. I had the impression that issues is present on all OSes and this only fixes it for Linux.

The point still applies for BSD, and although it still would be nice to have full compatibility on all OSes I think we can live with it.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 7, 2021

The point still applies for BSD, and although it still would be nice to have full compatibility on all OSes I think we can live with it.

BSDs are likely still affected, yes, and we don't officially support BSD. In practice I am doubtful that anyone has tried using a Kontrol S4 Mk2 or M3 with Mixxx on BSD. If you're out there, speak up and prove me wrong. :)

@uklotzde
Copy link
Contributor

uklotzde commented Jul 7, 2021

S4 MK3, Firmware v0.6.1, after installing the new .rules files:

info [Controller] Found HID device: { 17cc:1720 r61 | Usage: ff01:0000 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0001 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0001 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0030 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0001 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0030 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0030 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0030 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0080 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0080 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0080 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:0080 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:00d8 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:00d8 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:00f0 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:00f0 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:00e0 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }
info [Controller] Duplicate HID device, excluding { 17cc:1720 r61 | Usage: ff01:00e0 | Interface: #3 | Manufacturer: Native Instruments | Product: Traktor Kontrol S4 MK3 | S/N: 0XXXXXXX }

I can confirm that it works with the new mapping.

@Holzhaus Holzhaus merged commit b7f31ae into mixxxdj:main Jul 7, 2021
@Be-ing Be-ing deleted the hidapi_hidraw branch July 7, 2021 21:07
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 8, 2021

Regarding hid_get_input_report: Please add a comment near that method that explains that it's not working with hidraw.

I added a to-do card to the 2.4 project board.

@mcuee
Copy link

mcuee commented Sep 4, 2021

@uklotzde Just wondering if you can comment on this hidapi issue with regard to the Duplicate HID device under Linux hidraw backend. Please post the output of "lsusb -vvv" and "hidtest" (from hidapi) under Linux about your device (Traktor Kontrol S4 MK3). Thanks.
libusb/hidapi#298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants