Make extended controller information available for device selection #13896

JoergAtGithub · 2024-11-17T20:00:56Z

This is the first PR, where I make use of new hidapi capabilities. It provides all available information that is useful for device selection to the user/mapping developer. In follow-ups I will use this foundation to dig deeper in the provided Plug&Play information about collections and report data structures.

This PR:

Show the physical transport protocol if available USB, Bluetooth, Firewire, ...
Shows different icons for MIDI, HID and USB Bulk-Controllers
Clearly seperates device properties and mapping poperties in the controller preferences
Replaces the hardcoded constant for HID Usages by the official JSON reference file provided on usb.org
Hides all properties, where no data are available (e.g. USB Interface-No. for a Portmidi-Controller).
All Properties have human readable labels now, instead of the old concatenated device category string
Devendored the outdated hidapi 0.11.1 copy in the lib folder - which is much older than required by now.

Note:

The number of lines changed lines looks much bigger as it is, due to devendoring hidapi and adding the Usage JSON file from usb.org.
I grouped the changes in seperated commits for easier review

…1.2 copy in the lib directory.

Refactor: Use getters and improve protocol handling

…worthy than the human-readable VendorString and ProductString provided by the device driver

… JSON attachment from HID Usage Table 1.5 PDF file)

Refactored recognizeDevice to be a member of HidEnumerator

Swiftb0y

thank you very much for this valuable PR. I only have one high-level complaint. Codestyle nitpicks follow in a separate review.

src/controllers/hid/hidusagetables.h

src/controllers/controller.h

Swiftb0y

couple nitpicks

src/controllers/dlgprefcontrollers.cpp

src/controllers/dlgprefcontroller.cpp

src/controllers/hid/hiddevice.cpp

Swiftb0y · 2024-11-18T15:03:20Z

src/controllers/dlgprefcontrollerdlg.ui

+         <item row="0" column="0">
+          <widget class="QGroupBox" name="groupBoxDeviceInfo">
+           <property name="sizePolicy">
+            <sizepolicy hsizetype="Preferred" vsizetype="Preferred">
+             <horstretch>0</horstretch>


are these device infos visible all the time? Are the relevant to all users all the time? Would it make sense to hide it behind --developer or --controller-debug?

--controllerDebug and later an always visible, collapsible region?

This informaton is also intended for the end-user! An end-user needs it to identify the correct device out of many with similar names. It should be visible for the end-user by default therefore.
But once the end-user identified the correct device, and the mapping is successful loaded and working, he no longer needs these details.

right, but how user friendly is that? In the long-term we probably need a different mechanism for identifying devices...

Yes, the long term target is Plug&Play. That is why I make the device data accessible in this first step.

nice. So leave like this for now and --developer once plug'n'play is achieved?

…esdata.h. Added hid_usagepages_json2cppheader.py script to create C++ header from official JSON source Modified related classes accordingly.

…isible

… to dbg

…num into a utility function and call it n ctor init list Reordered the member initializations in the constructor to match the order of their declarations

Swiftb0y · 2024-11-24T21:15:13Z

src/controllers/controller.cpp

+    static const QMap<PhysicalTransportProtocol, QString> protocolMap = {
+            {PhysicalTransportProtocol::USB, QStringLiteral("USB")},
+            {PhysicalTransportProtocol::BlueTooth, QStringLiteral("Bluetooth")},
+            {PhysicalTransportProtocol::I2C, QStringLiteral("I2C")},
+            {PhysicalTransportProtocol::SPI, QStringLiteral("SPI")},
+            {PhysicalTransportProtocol::FireWire, QStringLiteral("Firewire - IEEE 1394")},
+            {PhysicalTransportProtocol::UNKNOWN, tr("Unknown")}};
+
+    return protocolMap.value(protocol, tr("Unknown"));


why the map over a switch-case?

It doesn't matter here, but in general, a map scales better in lookup performance. So it's just use of the common pattern.

It doesn't matter here, but in general, a map scales better in lookup performance. So it's just use of the common pattern.

I actually highly doubt that when they keys are effectively ints.

Swiftb0y · 2024-11-24T21:18:36Z

src/controllers/controller.h


+enum class PhysicalTransportProtocol : uint8_t {


is the explicit type necessary for ABI compatibility?

Swiftb0y · 2024-11-24T21:27:52Z

src/controllers/hid/hiddevice.h

+    bool isValid() const {
+        return !getProductString().isNull() && !getSerialNumber().isNull();
+    }


are you sure all hid devices have a productstring and serial number they report? Why do exactly these attributes make the device "valid"? What does valid mean in this context? Usually objects that are not valid result in UB if you call most member functions on them, but thats not the case here, afaict.

I didn't touch this line in this PR - and do not plan to.

But I can tell you, that only USB HID devices can store a product string. But the numeric VID & PID exist for all HID devices. This is because the HID descriptors are not specified with fields for strings. They just contain the numerical index of a string in the USB String Descriptor. And non-USB devices simply have no USB String Descriptor to reference.
But it seems the fields are always filled by the OS, in case of my Lenovo Thinkpad with an I2C Touchpad from Elan, Windows reports "Microsoft" "HIDI2C Device" with S/N 9999.

whoops yeah sorry about the complaint. The diff split weirdly so I didn't notice that this was already here previously. Then disregard the comment.

Swiftb0y · 2024-11-24T21:31:51Z

src/controllers/hid/hidusagetables.h

+class HidUsageTables {
+  public:
+    HidUsageTables() = default;
+    static QString getUsagePageDescription(uint16_t usagePage);
+    static QString getUsageDescription(uint16_t usagePage, uint16_t usage);
+};


convert to namespace instead? There is no member data.

Swiftb0y · 2024-11-24T21:38:30Z

tools/hid_usage_tables.h

empty file?

JoergAtGithub added 11 commits November 17, 2024 08:56

Raised hidapi requirement to 0.14.0 and devendored the old hidapi 0.1…

6b6c374

…1.2 copy in the lib directory.

Remove pConfig from BulkEnumerator and Hss1394Enumerator

0173691

Removed DeviceCategory class and related code.

fe7d799

Display MIDI, HID, BULK protocol icons in preferences tree

faaea4a

New UI with two column layout for device info and mapping info

af5f865

Refactor: Use getters and improve protocol handling

Added VID/PID hex numbers read from the hardware, that are more trust…

a8bd097

…worthy than the human-readable VendorString and ProductString provided by the device driver

Load and parse HID usage tables from official usb.org JSON (extracted…

664c9bf

… JSON attachment from HID Usage Table 1.5 PDF file)

Delete all hard coded HID Usage-Page constants

e7e560b

Moved HID constants to mixxx::hid namespace in hidenumerator.cpp

08c0db7

Refactored recognizeDevice to be a member of HidEnumerator

Refactor DeviceInfo formatting and update QDebug operator

d614e57

Added comment about release_number field

b23d05a

github-actions bot added build code quality controller backend labels Nov 17, 2024

Swiftb0y reviewed Nov 18, 2024

View reviewed changes

src/controllers/hid/hidusagetables.h Outdated Show resolved Hide resolved

Swiftb0y reviewed Nov 18, 2024

View reviewed changes

src/controllers/controller.h Show resolved Hide resolved

Swiftb0y reviewed Nov 18, 2024

View reviewed changes

Updated HidUsageTables to use static data structure from hidusagetabl…

84eed81

…esdata.h. Added hid_usagepages_json2cppheader.py script to create C++ header from official JSON source Modified related classes accordingly.

JoergAtGithub force-pushed the controller_device_info1 branch from d74a1c5 to 84eed81 Compare November 24, 2024 12:18

JoergAtGithub added 6 commits November 24, 2024 13:48

Remove default case in switch statement to make warning by -Wswitch v…

23643e2

…isible

Optimizing the temporary allocations in operator<< and directly write…

62a80f9

… to dbg

Unified physicalTransport enum to string conversion

632d685

Removed the official usb.org JSON HID Usage Tables file from the sources

5c64096

Introduce formatHex in dlgprefcontroller

a6fab09

Moved conversion of hidapi bus typte to physical transport protocol e…

3882a86

…num into a utility function and call it n ctor init list Reordered the member initializations in the constructor to match the order of their declarations

JoergAtGithub force-pushed the controller_device_info1 branch from f73b245 to 3882a86 Compare November 24, 2024 17:13

JoergAtGithub requested a review from Swiftb0y November 24, 2024 17:55

Swiftb0y reviewed Nov 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make extended controller information available for device selection #13896

Make extended controller information available for device selection #13896

JoergAtGithub commented Nov 17, 2024 •

edited

Loading

Swiftb0y left a comment

Swiftb0y left a comment

Swiftb0y Nov 18, 2024

ronso0 Nov 24, 2024

JoergAtGithub Nov 24, 2024

Swiftb0y Nov 24, 2024

JoergAtGithub Nov 24, 2024

Swiftb0y Nov 24, 2024

Swiftb0y Nov 24, 2024

JoergAtGithub Nov 24, 2024

Swiftb0y Nov 25, 2024

Swiftb0y Nov 24, 2024

Swiftb0y Nov 24, 2024

JoergAtGithub Nov 24, 2024

Swiftb0y Nov 25, 2024

Swiftb0y Nov 24, 2024

Swiftb0y Nov 24, 2024

Make extended controller information available for device selection #13896

Are you sure you want to change the base?

Make extended controller information available for device selection #13896

Conversation

JoergAtGithub commented Nov 17, 2024 • edited Loading

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoergAtGithub commented Nov 17, 2024 •

edited

Loading