Skip to content

Conversation

@jimon
Copy link
Contributor

@jimon jimon commented Nov 16, 2021

Description

Switch controllers generally have simple and full modes, our existing support relied on a controller being in simple mode, but any app (browser, steam, etc) could switch it to full mode and then we were not compatible with data layout at all. Also our previous implementation was lacking initial handshake.

Changes made

Implemented support for handshake, simple and full modes.

Notes

Needs some extra testing, I verified it works on Windows/macOS and USB/BT.
It seems controller is time sensitive to handshake sequence, so I implemented handshake restart in case if we're getting stuck somewhere (editor not refreshing, lost focus, etc)

@unity-cla-assistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Dmytro Ivanov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jimon jimon changed the title Adding proper support for Switch Pro controller Adding better support for Switch Pro controller Nov 16, 2021
@ekcoh
Copy link
Collaborator

ekcoh commented Nov 18, 2021

Agree with @mattdatwork that TODOs might fit into a JIRA ticket tied to Device Support epic rather than being forgotten TODOs in code.

@jimon jimon force-pushed the dmytro/fix-1369091 branch from 7582d06 to 67a8605 Compare November 19, 2021 16:28
@Meorge
Copy link

Meorge commented Dec 13, 2021

Thank you for linking this fork/pull request to me! I was able to use it to add Joy-Con and Pro Controller support, including HD Rumble, IMU data, and controller colors, in my repository here: https://github.com/Meorge/JoyConUnityInputSystem

If you think it makes sense, I could fork off of this branch and add my changes, so that the Input System also supports Joy-Cons and the extra controller features.

@jimon
Copy link
Contributor Author

jimon commented Dec 14, 2021

@Meorge I think I'll go merge this first, and if you can base your changes against this PR we might be able to incorporate them later.

One of the concerns I have right now is if you notice, PreProcessEvent can only shrink payloads or keep them same size, so size of SwitchProControllerHIDInputState is 6 bytes here is not a coincidence, it has to be that small to incorporate simple report which is 12 bytes .. So that might be one area that I need to refactor later on.

@Meorge
Copy link

Meorge commented Dec 14, 2021

@Meorge I think I'll go merge this first, and if you can base your changes against this PR we might be able to incorporate them later.

Sounds good to me! I've got some other things I need to work on, but I'll keep an eye on this and perhaps work on my own PR once this has been merged.

One of the concerns I have right now is if you notice, PreProcessEvent can only shrink payloads or keep them same size, so size of SwitchProControllerHIDInputState is 6 bytes here is not a coincidence, it has to be that small to incorporate simple report which is 12 bytes .. So that might be one area that I need to refactor later on.

The approach I took to this in my code was to not continue processing simple input reports (i.e. return false when they were received in IEventPreProcessor.PreProcessEvent()), but before that, send a request to the controller to switch to the full input mode. If my understanding is correct, according to this table, only processing full reports (with the ID of 0x30) would expand the maximum payload size to 49 bytes.

@jimon
Copy link
Contributor Author

jimon commented Dec 14, 2021

@Meorge According to this https://github.com/libsdl-org/SDL/blob/main/src/joystick/controller_type.h#L576-L597 there are bunch of controllers that are input only controllers, and their state struct https://github.com/libsdl-org/SDL/blob/main/src/joystick/hidapi/SDL_hidapi_switch.c#L118-L124 seems to be 7 bytes long.

And tbh I'm not sure about switching controllers to full mode, because then code that tries to read it as a HID gamepad starts spamming completely random data. E.g. Unity GetButton + Steam, because Steam switching the gamepad into full mode, and that breaks down HID layer ... So maybe keeping us into whatever mode the gamepad happens to be could be more pragmatic. SDL2 does switch to full mode but they restore to simple mode at shutdown, which is also interesting.

@jimon jimon force-pushed the dmytro/fix-1369091 branch from 67a8605 to efed90e Compare January 11, 2022 16:20
@jimon jimon force-pushed the dmytro/fix-1369091 branch from efed90e to ba7294a Compare January 12, 2022 10:17
@jimon jimon merged commit 7a48d09 into develop Jan 12, 2022
@jimon jimon deleted the dmytro/fix-1369091 branch January 12, 2022 14:21
@jimon jimon restored the dmytro/fix-1369091 branch January 12, 2022 14:34
@jimon jimon deleted the dmytro/fix-1369091 branch January 12, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants