Preliminary support for Nintendo Switch Online controllers #35
Conversation
This change adds product IDs for the following Nintendo Switch Online controllers: * https://www.nintendo.com/store/products/sega-genesis-control-pad/ * https://www.nintendo.com/store/products/super-nintendo-entertainment-system-controller/ This allows the controllers to be detected by the `hid-nintendo` driver module.
This change adds preliminary support to the `hid-nintendo` driver for the SNES, NES, and Sega Genesis controllers sold for Nintendo Switch Online: * https://www.nintendo.com/store/products/nintendo-entertainment-system-controllers/ * https://www.nintendo.com/store/products/super-nintendo-entertainment-system-controller/ * https://www.nintendo.com/store/products/sega-genesis-control-pad/ In creating this commit, I made reference to earlier work by @nadiaholmquist, manually patching in these changes: * nadiaholmquist/linux@21a824e * nadiaholmquist/linux@bdf4626 In addition, I implemented preliminary Sega Genesis controller support. Known issues: * The Sega Genesis controller appears to work great over USB but not over Bluetooth. Instead, over Bluetooth, the controller reports its product ID as `0x2017` (the same as the SNES controller) instead of `0x201e` (the unique product ID for the Genesis controller). It is unclear to me if this is a bug in the hardware or in my implementation. * I tried to use names for the devices which are consistent with Nintendo USA's website (slightly shortened). Unsure if this is confusing. * I made some minor formatting changes. Unclear if I have violated any kernel code style rules. * I don't have the N64 controller, and so I cannot implement it at this time. This commit should be considered a preliminary draft for supporting these controllers. That said, this works well for me here (other than the Genesis Bluetooth issue noted above), and I have already begun using these changes locally. Thanks again to @nadiaholmquist for her work on those previous NES/SNES changes.
| if (jc_type_is_nescon(ctlr) || \ | ||
| jc_type_is_snescon(ctlr) || \ | ||
| jc_type_is_gencon(ctlr)) { |
There was a problem hiding this comment.
I am undecided on if I should define a helper macro to the effect of jc_type_is_nso to include all the NSO controllers at once. That said, they share a few characteristics that would make such a helper macro reasonable (lack of motion and rumble, for example).
Happy to get input on this.
drivers/hid/hid-nintendo.c
Outdated
| if (jc_type_is_nescon(ctlr)) { | ||
| /* set up dpad hat */ | ||
| input_set_abs_params(ctlr->input, ABS_HAT0X, -1, 1, 0, 0); | ||
| input_set_abs_params(ctlr->input, ABS_HAT0Y, -1, 1, 0, 0); | ||
|
|
||
| /* set up buttons */ | ||
| for (i = 0; nescon_button_inputs[i] > 0; i++) | ||
| input_set_capability(ctlr->input, EV_KEY, nescon_button_inputs[i]); | ||
|
|
||
| /* register the device here, we don't need any more setup */ | ||
| ret = input_register_device(ctlr->input); | ||
| if (ret) | ||
| return ret; | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
This conditional block, along with the following two, are highly duplicative. I split out these conditions from the changes I adapted. It seemed to me that the existing abstraction was starting to strain.
However, I may try to fold these again into one conditional in a future commit. For now, this works.
drivers/hid/hid-nintendo.c
Outdated
| USB_DEVICE_ID_NINTENDO_PROCON) }, | ||
| USB_DEVICE_ID_NINTENDO_PROCON) }, | ||
| { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_PROCON) }, | ||
| USB_DEVICE_ID_NINTENDO_PROCON) }, | ||
| { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_CHRGGRIP) }, | ||
| USB_DEVICE_ID_NINTENDO_CHRGGRIP) }, | ||
| { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_JOYCONL) }, | ||
| USB_DEVICE_ID_NINTENDO_JOYCONL) }, | ||
| { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_JOYCONR) }, | ||
| USB_DEVICE_ID_NINTENDO_JOYCONR) }, | ||
| { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_SNESCON) }, | ||
| { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_SNESCON) }, | ||
| { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_GENCON) }, | ||
| { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, | ||
| USB_DEVICE_ID_NINTENDO_GENCON) }, |
There was a problem hiding this comment.
Changed some alignment here for consistency's sake. Unclear if this is okay.
I revisited my the keys reported as inputs for the Sega Genesis controller and properly assigned those. This also makes Bluetooth work a bit better: the controller is still misrecognized, but the inputs match the SNES controller much more closely, so it's less noticeable.
Turns out this value isn't used for Bluetooth (or really anything right now), but for completeness' sake, I'm defining it here, with a value based on my debugging.
|
I suspect that the "controller type" field has taken on more important responsibilities since the time when it was reverse-engineered here a few years ago. The values, as they exist in this pull request, are correct, based on my testing. Meanwhile, the product and vendor IDs seem to be misleading. It's puzzling that the Genesis controller is able to send different product IDs based on whether it's using USB or Bluetooth. I still don't know whether this is the fault of my specific device or just a design "decision." In any case, before this set of changes is done (whether this is completed in this pull request or somewhere else), it's possible that it would be a good idea to rework the Below, I'm pasting a few chunks of logs from the kernel which include some debugging information I added. Pay attention to the Attaching the NSO SNES controller via USB:Attaching the NSO SNES controller via Bluetooth:Attaching the NSO Genesis controller via USB:Attaching the NSO Genesis controller via Bluetooth: |
The Genesis controller doesn't even have triggers, so this is redundant.
I accidentally added some extra whitespace while merging changes. This change removes it.
|
I don't know how long it's going to be before I can get in contact with Daniel, so this might sit for a little while in the meantime. Before I leave it for a while, I wanted to jot out tentative ideas for future improvements I might like to incorporate into my set of changes.
That's all I have for now, but if I think of something else, I'll add it to this list. |
This commit adds support to the `hid-nintendo` driver for the Nintendo Switch Online controllers for the NES, SNES, and Genesis. These changes were first implemented in: * DanielOgorchock/linux#35 The changes have been adapted here for the latest version of the `hid-nintendo` driver as of Linux kernel version 5.16.0 (see previous commit). For more about the controllers, see: * https://www.nintendo.com/store/products/nintendo-entertainment-system-controllers/ * https://www.nintendo.com/store/products/super-nintendo-entertainment-system-controller/ * https://www.nintendo.com/store/products/sega-genesis-control-pad/ Much of these changes make reference to work by Nadia Holmquist Pedersen, adapted from these commits and ported forward to the latest version of the `hid-nintendo` driver: * nadiaholmquist/linux@21a824e * nadiaholmquist/linux@bdf4626 In addition, I implemented Sega Genesis controller support. At this point, Bluetooth support for this controller is preliminary and incomplete. I have relicensed these changes under the GPL-2.0 in this repository, using the same licenses as the original. Signed-off-by: Emily Strickland <linux@emily.st>
|
Since I've gotten no response, I'm going to close this pull request for now. Feel free to reopen it if you'd like to continue the conversation about it here. I've decided to release these changes (ported to the latest version of the driver in the kernel tree) as a DKMS module located here: https://github.com/emilyst/hid-nintendo/. If anybody wants to use these changes, please see there. |
These changes should be considered a preliminary draft until they are acknowledged, approved, and merged.
Quoting from the second commit:
I will comment inline where I believe input is still warranted.