Add flydigi vader3 support (no doubt, clobbering xbox support) #451
Conversation
This probably merits a fork and cleanup of irrelevant code - maybe pulling in the xow driver as a reference for dongle support, and xpad for usb support (although I didn't even test wired, as I wanted to use wireless).
|
Hey all - just got a Flydigi Vader 3 Pro a couple hours ago, and as I already did the relevant work to make it usable over bluetooth locally (by using this repo as a reference point, since I had tried it for the xbox elite series 2 a couple weeks ago) - I am creating this draft PR to get advice - do you think there would be benefit to support generic xbox controllers in this code? Would it be more fitting for me to try to push some of the corrected button mappings to the Linux kernel? (the ones that it registers by default on dinput mode are...odd - no doubt problems on the hardware side - but the Z key comes through as a Return press). If neither of those seem ideal, maybe a license respecting fork where I focus on more Flydigi Vader 3 support would be beneficial to the controller community? When this project started, did rumble work with the xbox remotes? That's the one thing that still hasn't worked on this one (rumble + bluetooth) - rumble works via the dongle (but only on x-input mode, not dinput) - and of course, x-input blocks the 6 special keys this controller has from registering, as they're bound controller-side by the app (and can't do multi-key combos). The linux hid-generic driver with the dongle does not work well however, it doesn't seem to show any activity at all under evtest, but maybe I need to add a custom udev rule. |
|
Code Climate has analyzed commit 091d8bc and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
It looks like your device is different enough that it should be handled by the generic HID driver in the kernel - you don't need xpadneo to support it. Actually, you would need to convert the HID reports to match Xbox controllers to be properly supported in games (as outlined in the commit comments).
Also, there's no such thing as "pulling the xow driver". xow is not a HID driver, it's a uinput driver in user-space, this is a HID driver in kernel space.
But there's actually an idea of reviving the idea of xow but instead of creating a uinput device, it would create a uhid device: The GIP protocol is almost identical to the HID procotol and could be easily converted, so xpadneo would be the frontend driver, and an xow fork would be the transport driver - much like bluez for BLE HID devices.
| /* fixup buttons to Linux codes */ | ||
| USAGE_MAP(0x90001, MAP_STATIC, EV_KEY, BTN_A), /* A */ | ||
| USAGE_MAP(0x90002, MAP_STATIC, EV_KEY, BTN_B), /* B */ | ||
| USAGE_MAP(0x90003, MAP_STATIC, EV_KEY, BTN_X), /* X */ |
There was a problem hiding this comment.
This structure should not be changed, it is essential to support exactly those keymaps (because we patch the VID/PID). Instead, patch the HID descriptor in xpadneo_report_fixup() and the HID reports in xpadneo_raw_event() to match the xpadneo_usage_maps[].
There was a problem hiding this comment.
Looking at pictures of the device, it has two additional buttons at the center (house and circle), and also buttons C and Z. These are not part of the Xbox spec and must be moved outside the range of the first 10 buttons: The Xbox spec supports exactly 10 well defined buttons in exactly that order: A,B,X,Y,LB,RB,Select,Start,LS,RS - no more, and none inserted. There's an additional Guide button at position 11 - but it's actually not meant to be dealt with by games, thus it's not part of the 10 well defined buttons. The Windows XINPUT spec (and thus what Proton uses) supports exactly 16 buttons maximum, no more. So every excess button in the keymap must be masked.
So either your device MUST not report as an Xbox controller, or exactly match those expectations.
You can patch the HID descriptors and HID reports to move the additional buttons to the BTN_TRIGGER_HAPPY range. We already moved some buttons there, especially the paddles have been moved there.
Other than that, I can only repeat: DO NOT misuse xpadneo to support this device if you don't take the above comments serious. DO NOT fork this project to support the controller, if you do not remove the VID/PID patching and do not remove support for MS devices. Really, I DO NOT want this device show up in random SDL controllerdb mappings destroying all the effort we've put into unifying all Xbox devices into a single virtual model currently properly supported by SDL and Steam.
Everything you do is reverting all these efforts and making the device show up like it would do with the hid-generic driver probably, but then probably (mis-)use our VID/PID patching technique to make the device visible to SDL and games as an Xbox controller - which it isn't in HID mode (aka Bluetooth). Instead, stick to hid-generic and add a SDL controllerdb mapping to make it visible in Steam and games. Ask the SDL project to properly support your model with hid-generic. It may be the easier way.
Please, do NOT submit a controllerdb patch to SDL while using this Frankenstein patched xpadneo driver.
Don't get me wrong: If done properly, we could support your device in xpadneo. But it is important to know the consequences of doing it the wrong way. A lot of effort went into getting to the point where we are now.
See also #286. This is also about how we should probably avoid HID patching in the future and thus stop VID/PID patching, too. That change would then render your controller useless with xpadneo because your initial idea of using xpadneo is probably because games do not see D7D7:0041 as an Xbox controller. OTOH, there's an idea of xpadneo converting other supported controllers to Xbox controllers (e.g. Switch mode of the Gulikit controllers). In this case, your controller would fit the bill of xpadneo. But it MUST be done properly: No changes to the 10 first buttons in the map, no changes to the axes.
| @@ -112,23 +112,33 @@ struct usage_map { | |||
| #define USAGE_IGN(u) USAGE_MAP(u, MAP_IGNORE, 0, 0) | |||
|
|
|||
| static const struct usage_map xpadneo_usage_maps[] = { | |||
| // 90009 and 9000a, sent when trigger fully released | |||
There was a problem hiding this comment.
Not sure what happens here... Is this a special function of your model? Then we should document why it exists and how to fit it into the event model.
There was a problem hiding this comment.
Honestly, not sure, it was observed while watching the evtest output.
There was a problem hiding this comment.
From the video presentations, the triggers seem to have microswitches, maybe at the end or at the beginning of the pressure range. It should make a click sound if you press the trigger: Try to figure out how it reacts on pressure and release.
The Steam controller has a similar feature where you could use this feature to aim down sights holding the trigger, then give a little more pressure to fire using the microswitches at the end of the pressure range.
| } | ||
|
|
||
| input_set_abs_params(xdata->gamepad, ABS_X, abs_min, abs_max, 32, deadzone); | ||
| input_set_abs_params(xdata->gamepad, ABS_Y, abs_min, abs_max, 32, deadzone); | ||
| input_set_abs_params(xdata->gamepad, ABS_RX, abs_min, abs_max, 32, deadzone); |
There was a problem hiding this comment.
Probably the same as above: Do not change the keymap/axismap structure and this should be okay and stay untouched.
| @@ -919,7 +942,8 @@ static int xpadneo_event(struct hid_device *hdev, struct hid_field *field, | |||
| case ABS_RY: | |||
| /* Linux Gamepad Specification */ | |||
| if (param_gamepad_compliance) { | |||
| input_report_abs(gamepad, usage->code, value - 32768); | |||
There was a problem hiding this comment.
This reverts param_gamepad_compliance. Do not change this. If you want to report 0..65535, disable param_gamepad_compliance instead. The reason behind this is that the HID descriptor says the axis have ranges from 0..65535 although it could perfectly fine say -32768..32767 (HID support negative values but they require a few more bytes, so that's probably why it wasn't used). But Linux gamepad spec says that gamepads need to have auto-centering sticks idling at position 0, so we just subtract 32768 - and also corrected the axis scale above.
| deadzone = 0; | ||
| /* abs_min = -32768; */ | ||
| /* abs_max = 32767; */ | ||
| abs_min = -127; |
There was a problem hiding this comment.
If your model reports 0..255 as the axis range, you need to switch cases here for your model. But I rather suggest to instead rescale to the Xbox controller range and patch the HID reports accordingly because we patch the VID/PID during device discovery. Otherwise your changes won't match games expectations.
As pointed out above: Do not try to mask the controller as an Xbox controller using xpadneo if you don't accept to exactly match the Xbox controller spec for the 6 axis and first 10 buttons. If you can change your code to match exactly that spec and expose the other buttons in the trigger happy range of key codes, then we can accept the code into xpadneo and allow it to be masked as an Xbox controller using our VID/PID patching technique.
The problem seems to be that hid-generic doesn't care about game input devices having more buttons than the
A fork MUST NOT use our VID/PID patching technique, otherwise it would create a mess in the SDL controller support. You can use the driver as a base, with support for VID/PID patching ripped out (or changed to other IDs) and support for MS devices ripped out, too. But then you are probably left with a thin driver that does hardly more than hid-generic.
Yes. It was one of the initial ideas of creating this driver in the first place because back at that time, the kernel did not support rumble for Bluetooth controllers (and still doesn't for some models). And also, button mapping was a mess because of very creative HID descriptors which changed with each firmware update - thanks Microsoft.
You need to find the HID report ID in the HID descriptor for rumble. Look in the docs folder to get an idea how to find it. In theory, we could detect the ID by looking at the descriptor. But currently it's hardwired to ID 0x03 (in the header file via a macro). Also, additional buttons MUST NOT bleed into other keymap ranges (see input-event-codes.h in the kernel). This is really a problem with the hid-generic driver but also some other drivers, e.g. hid-microsoft doesn't care about this problem for controllers but some other devices. If this happens, rather move those buttons to the trigger happy range, then talk to the SDL devs to properly auto-assign these to their correct function. This obviously needs a specialized thin HID driver.
Yes, you may need to add a udev rule. In GIP mode (USB or dongle), it may not create an event device but just an input and hidraw device (via the kernel). A udev rule could fix that. Or the device is just missing a proper udev tag/property which obviously a rule could also fix. Also, hid-generic won't load a rumble driver or implement rumble by itself. hid-microsoft is a driver which uses hid-generic but implements model-specific quirks and also attaches a rumble driver for supported models. You could create a similar driver (actually, I based some ideas of xpadneo off of hid-microsoft and hid-logitech-{dj,hidpp}). Every driver which implements support for known VID/PID will automatically use that driver instead of falling back to hid-generic, at least in newer kernels and if the module is loaded already. For older kernels or unloaded modules, you need to rebind the driver with udev rules as we do in our rules. Obviously, this is rather intended for modules built into the kernel (so VID/PID matching the specialized driver is already known to the kernel). |
hid-xpadneo/src/hid-xpadneo.c
Outdated
| @@ -865,20 +885,23 @@ static int xpadneo_input_configured(struct hid_device *hdev, struct hid_input *h | |||
|
|
|||
| if (param_gamepad_compliance) { | |||
| hid_info(hdev, "enabling compliance with Linux Gamepad Specification\n"); | |||
| abs_min = -32768; | |||
| abs_max = 32767; | |||
| deadzone = 0; | |||
There was a problem hiding this comment.
This overrides the disable_deadzones behavior above.
|
@kakra thank you so much for taking time to wade through this "Frankenstein" of a patch 😆 (such an apt term). This is my first foray into anything driver/controller related, so your feedback is very appreciated. What is VID/PID? I see the acronym was used often. Obviously, this effort has been a lot of guess/check/trial-and-error, where I'm slowly understanding more of the inner workings as I fiddle with the current code. If I can get the rumble to work (trying that today), given that you seem somewhat receptive to the concept (not necessarily the current code 😄 ) I would be interested in cleaning it up and doing it the "right way" to get into the driver itself, and moving beyond "break things to verify the POC is possible". I do like the controller registering in my limited game tests as an Xbox one (it does so over dongle with the default drivers as well) - at least, in my test game (FF14 via wine), as that causes the in game controller detection and bindings to work out of the box. It sounds like for the alternatively mapped codes (where many of the codes this controller produces overlap with other codes the Xbox defines) - instead of altering the top level usage map as I have done, there is a different layer (HID?) where I can make special accommodations to alter the codes from this controller to the expected Xbox codes? |
You're welcome.
Vendor ID, product ID... It's an 8-digit hex ID used to identify the vendor/product/model in the form of We use PID patching to mask the device as a different device to work around mismatching controllerdb entries in SDL, and also to trick games into using the generic controller layout of the original Xbox controller which has exactly 10 buttons. This way we bypassed problems with games seeing wrong mappings in either dinput or xinput. Nowadays, dinput is mostly gone, older games may use it and Proton probably ships work-arounds for that (dinput doesn't have analog triggers).
Yes, it will take some effort to wrap your head around that - I've gone through this for a long time. The key is understanding the central role of the mapping struct: This is only used to mask off mappings bits we do not want to show or emulate, and it's used to turn on bits for buttons we emulate/moved. The magic happens in the event handlers which actually swap or move bits in the reports, patch the descriptor, and steal input events from the kernel default handler and process them internally instead.
There's a plan to support non-Xbox controllers after v0.11 or v0.12. With v0.10 I will reshape the code to be more convenient and modular so we can better support new concepts. That's the time when your concept could actually land in the code. But until then we have to be careful how to present the device to user-space, otherwise we will create an almost unsolvable mess with SDL and Steam Input again. I've understood your code as POC and thus didn't raise any concerns there, I just wanted to give hints where to move lines properly for better integration. Code style seems fine, and cleanup will follow. It should be possible to get it into a state where it doesn't interfere with functions of existing devices. But as said, there will be refactoring with v0.10 which would require you to manually re-apply your changes to the codebase. Since v0.11 or v0.12 is far into the future, a separate driver may be more useful. Since some buttons are part of the Switch controller layout, you could look into the Switch HID driver, too. Maybe it's more easy to put some patches there. Whatever you do: If you plan on modifying an existing driver and plan to merge such a patch as part of the driver, ensure that you do not insert new buttons within existing keymaps because SDL doesn't look at kernel key symbols but it rather counts buttons. So if we have this bitmap (as xpadneo uses): and you enable the C and Z bit, you would make C the third button, that moves X and Y to position 4 and 5, but SDL would continue to think that button 3 and 4 are X and Y, and never see button 5 and 6, and thus your button C would act as X in SDL, and button Y and Z would be dead (or rather, bleed into the following gamepad buttons, and cut off later buttons instead, but you get the idea). I collected a lot of thoughts and notes about that in #286. Instead, you would need to keep
Then there's no other way then ensuring we only have the default 10 Xbox buttons in the lower bit range - do not enable bits for C,Z or other none-Xbox-buttons. If you want to still enable the buttons, you'd have to do some tricks in the HID report. You can enable mapping bits beyond Important: The XINPUT spec of Windows supports at most 16 buttons. 10 are from the original Xbox layout, an 11th bit is used for the Guide button but v0.10 moves that to a different sub-device. So there are six bits left, XINPUT puts the digipad into this bitmap, too: 4 bits, see https://learn.microsoft.com/en-us/windows/win32/api/xinput/ns-xinput-xinput_gamepad. So this leaves two additional bits which are usually already occupied by e.g. a Share button. But this also doesn't belong in the XINPUT spec so v0.10 moves the Share button to a sub-device, too. SDL works a little bit different and would have a hard time using sub-devices. So the plan is to move the sub-device buttons back into the main device bitmap but to the trigger happy range, otherwise with xpadneo, SDL cannot see any button beyond the 10 standard buttons. I think, the Guide button already moved to back to the main device.
The struct just enables what user-space sees: It tells user-space which buttons are actually on the controller. The problem is: SDL ignores the symbols attached to the bitmap positions, it just counts buttons. For some older Xbox controllers it expects button 0,1,2,3 for A,B,X,Y, and for newer controllers it expects button 0,1,3,4 for A,B,X,Y - thus it skips a seemingly dead/blind C button. BTW, which is why we patch the PID so we go with the original bitmap layout which also many old games expect on the jsdev device (which is crafted from the evdev device by the kernel). To respect the original Xbox button layout, all non-default buttons must be enabled in the trigger happy range (otherwise they bleed into the range for digitizer pens). SDL nowadays has heuristics to properly detect our trigger happy buttons by - again - counting enabled bits in the bitmap. So new buttons must go after the range of bits already set, otherwise the mapping implied by SDL and most games shifts across buttons. You only get proper events if you adjust everything properly. You'd start with only adding your C/Z button (and maybe others) to the end of the struct, using proper trigger happy positions. Then change to event and raw handlers to properly return events if you see those bits. Use a sync event if you're done reporting a full frame of events. A frame of events is used the following way: The kernel receives a raw HID report, then generates one event per keymap bit in the report, and for each axis change in the report. It then syncs the event to tell user-space that the report has been fully processed. If you don't sync between button press and release, user-space would not see a change in the button state (because it would be pressed and released within the same frame), or rather: it will see just the last event from that frame for that button, and if the resulting state doesn't differ from the previous frame, that event didn't happen. If you sync too often, user-space cannot detect that all events come from a single report packet - but that's probably never an issue. A series of events makes up such a frame, a sync signals the end of the frame. BTW: The struct only enables base-model buttons. There's logic in the device setup functions which enable model specific mapping bits or disable bits that we report through sub-devices ( |
|
I think I created a parsed report that matches other controllers: HID Descriptor for Flydigi Vader3 Pro (Linux mode)Hex dump of the controller descriptor: Parsed descriptor: Unlike most the others in the docs directory, this seems to only have a single report id (changing the hardcoded header value from 0x03 to 0x01 does not cause rumble to function). Is there anything that can be done (doing some offset in this report_id to the right area?) - or is this output indicative of a lack of support at the hardware level for rumble over bluetooth? Also, is the report_descriptor impacted/produced by the driver itself (in which case it would always be incorrect here, as I am piggybacking on the xbox driver for a non-xbox controller), or is it coming from a lower level (direct device hardware data via the kernel or something)? Also, managed to get my first kernel panic that crashed my desktop in 15 years of GNU/Linux usage by fiddling with rumble settings that were not valid 😂 |
Yes, there is no rumble function in the descriptor. Report 1 is a report that sends data from the device to the computer. You cannot use it in the other direction. As you can see, the other docs have
There's no such thing as a fantasy offset into the report. The report clearly defines report sizes (in bits) and report counts for different usages (buttons, axes, ...) and a communication direction (input to the host, output from the host). Each of those report usage definitions (sizes, counts) accounts for the offset into the report. And yes, this means: no rumble for this controller in Bluetooth mode...
Yes and no. The original descriptor comes from the HID device. xpadneo does patch that before user-space sees or the kernel parses it - so yes, it is partially crafted by the driver. Actually, there exist HID devices without HID descriptors. In that case, a binary HID descriptor blob would be hard-coded into the driver. Something similar happens in drivers which support devices with known-broken HID descriptors: sometimes it's easier to just replace it instead of patching it. Xbox controller HID descriptors have one quirk in common: They end with a bogus null byte. In my patcher, I remove it if I detect it. I also look at specific byte offsets to replace the bytes with HID descriptions that more closely match what Linux user-space expects from an Xbox controller.
This is easy: If you don't close all handles or fail to free memory, or use memory after free, the kernel will probably explode more or less silent in front of you. Dangling handles often cause the kernel to freeze on shutdown because it waits for an event that never happens. Or if you free memory with dangling handles, another subsystem may access freed memory and cause a memory access error which usually results in a kernel panic. Sometimes it may be better to spawn your test kernel with the module in a qemu machine with a directly booted kernel. |
|
@kakra I did a from-scratch implementation vs a fork, but I did re-use modified versions of your install.sh/uninstall.sh/Makefiles - I hope that's alright (there should not be any remnants that cross over with xpadneo, beyond the notice I put in the README.md to let users know what I assumed were yours/original project author's copyrights on those supplementary files). https://github.com/ahungry/vader3 In doing that, I got a lot more insight into the raw_event vs event areas, and managed to take your advice (remapping in events vs changing things in the input_mapping), as well as using the TRIGGER_HAPPY ranges. If/when you want to add official third party xbox-mode controller support for generic devices, feel free to take anything you see for this project (the default bindings to these new ones are worked out at least) or give me a ping and I can aim for that official pull request. Thanks again! |
|
@ahungry I've added some comments in ahungry/vader3@b662d12 |
This probably merits a fork and cleanup of irrelevant code - maybe pulling in the xow driver as a reference for dongle support, and xpad for usb support (although I didn't even test wired, as I wanted to use wireless).