| Message ID | 20190603060644.10338-1-djogorchock@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID: joycon | expand |
Hey, I am working on android on the nintendo switch (typing this mail on it :) and am trying to use this driver to support joycons with their full functionality, however, when trying to connect while this driver is in the kernel, the request for calibration times out (err 110) and prevents them from working at all. At first i thought it was due to the latency issue android has with joycons, but even after applying the fix for that for that it still doesn't work. Do you have any ideas why this is happening? Sun Jun 02 19:06:55 GMT-11:00 2019 Daniel J. Ogorchock : > Version 5 changes: > - Removed sysfs interface to control motor frequencies. > - Improved rumble reliability by using subcommands to set it. > - Changed mapping of the SL/SR triggers on the joy-cons to map to > whichever triggers they lack (e.g. a left joycon's sl/sr map to > TR and TR2). This allows userspace to distinguish between the > normal and S triggers. > - Minor refactors > Version 4 changes: > - Added support for the Home button LED for the pro controller and > right joy-con > - Changed name from hid-switchcon to hid-joycon > - Added rumble support > - Removed ctlr->type and use hdev->product instead > - Use POWER_SUPPLY_CAPACITY_LEVEL enum instead of manually translating > to capacity percentages > - Misc. minor refactors based on v3 feedback > > Version 3 changes: > - Added led_classdev support for the 4 player LEDs > - Added power_supply support for the controller's battery > - Made the controller number mutex static > - Minor refactoring/style fixes based on Roderick's feedback from v2 > > Version 2 changes: > - Switched to using a synchronous method for configuring the > controller. > - Removed any pairing/orientation logic in the driver. Every > controller now corresponds to its own input device. > - Store controller button data as a single u32. > - Style corrections > > Daniel J. Ogorchock (5): > HID: joycon: add nintendo switch controller driver > HID: joycon: add player led support > HID: joycon: add power supply support > HID: joycon: add home led support > HID: joycon: add rumble support > > MAINTAINERS | 6 + > drivers/hid/Kconfig | 24 + > drivers/hid/Makefile | 1 + > drivers/hid/hid-ids.h | 3 + > drivers/hid/hid-joycon.c | 1414 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 1448 insertions(+) > create mode 100644 drivers/hid/hid-joycon.c > > -- > 2.21.0 > >
I had a look and found the mail you were referring to, however the bug was fixed in 4.8 which should be fine as I am using 4.9. Testing using v1 of the driver, which uses workqueues resulted in the same issue. I also tested using HID_CONNECT_DRIVER in probe, then after init HID_CONNECT_HIDRAW, this didn't work either (here is logcat https://paste.ee/p/5VHga - len 12 packets are standard input ones and dmesg https://paste.ee/p/oXVsq). That driver you probably linked works because it doesn't actually send anything, just takes in the raw hid events. Would the joycons always reply to a get calib request? On Sat, Jun 15, 2019 at 8:41 AM Daniel Ogorchock <djogorchock@gmail.com> wrote: > > > On Thu, Jun 13, 2019, 16:35 billy <blaws05@gmail.com> wrote: >> >> >> Hey, >> I am working on android on the nintendo switch (typing this mail on it :) and am trying to use this driver to support joycons with their full functionality, however, when trying to connect while this driver is in the kernel, the request for calibration times out (err 110) and prevents them from working at all. At first i thought it was due to the latency issue android has with joycons, but even after applying the fix for that for that it still doesn't work. Do you have any ideas why this is happening? > > > Hi Billy, > > What Android kernel version are you using? I think I recall someone else telling me they were getting calibration timeouts on Android. The driver found here was working for them: > > https://gitlab.com/pjranki/joycon-linux-kernel/blob/master/drivers/hid/hid-joycon.c > > I think it's because of my driver relying on hid_device_io_start() to allow for retrieving calibration in the probe. I vaguely remember Benjamin describing somewhat recent changes that made that possible. Maybe moving all the calls after hid_device_io_start into a workqueue would be a good sanity check if that's the problem. > >> >> >> Sun Jun 02 19:06:55 GMT-11:00 2019 Daniel J. Ogorchock : >> >> > Version 5 changes: >> > - Removed sysfs interface to control motor frequencies. >> > - Improved rumble reliability by using subcommands to set it. >> > - Changed mapping of the SL/SR triggers on the joy-cons to map to >> > whichever triggers they lack (e.g. a left joycon's sl/sr map to >> > TR and TR2). This allows userspace to distinguish between the >> > normal and S triggers. >> > - Minor refactors >> > Version 4 changes: >> > - Added support for the Home button LED for the pro controller and >> > right joy-con >> > - Changed name from hid-switchcon to hid-joycon >> > - Added rumble support >> > - Removed ctlr->type and use hdev->product instead >> > - Use POWER_SUPPLY_CAPACITY_LEVEL enum instead of manually translating >> > to capacity percentages >> > - Misc. minor refactors based on v3 feedback >> > >> > Version 3 changes: >> > - Added led_classdev support for the 4 player LEDs >> > - Added power_supply support for the controller's battery >> > - Made the controller number mutex static >> > - Minor refactoring/style fixes based on Roderick's feedback from v2 >> > >> > Version 2 changes: >> > - Switched to using a synchronous method for configuring the >> > controller. >> > - Removed any pairing/orientation logic in the driver. Every >> > controller now corresponds to its own input device. >> > - Store controller button data as a single u32. >> > - Style corrections >> > >> > Daniel J. Ogorchock (5): >> > HID: joycon: add nintendo switch controller driver >> > HID: joycon: add player led support >> > HID: joycon: add power supply support >> > HID: joycon: add home led support >> > HID: joycon: add rumble support >> > >> > MAINTAINERS | 6 + >> > drivers/hid/Kconfig | 24 + >> > drivers/hid/Makefile | 1 + >> > drivers/hid/hid-ids.h | 3 + >> > drivers/hid/hid-joycon.c | 1414 ++++++++++++++++++++++++++++++++++++++ >> > 5 files changed, 1448 insertions(+) >> > create mode 100644 drivers/hid/hid-joycon.c >> > >> > -- >> > 2.21.0 >> > >> > >> >>
Hi,
Thanks for the patchset. Some questions and comments on the driver:
1. In the rumble patch, there are some functions that are only used if
CONFIG_JOYCON_FF is enabled. Otherwise, the compiler complains about
unused functions. Could you guard these with #ifdef?
2. Inside hid-ids, most of the defines use tabs, but the current patch
uses spaces. Could you change to use tabs?
3. Currently, the driver quits and prints an error if it is not able to
read the calibration data. That means, if the device is being simulated
using /dev/uhid, it is necessary to also properly respond to these
low-level requests for flash memory reading. This is doable, but is a
lot of work. Any way to allow the driver to continue to function even if
calibration data is missing? Maybe just print a warning that the sticks
won't function well?
4. The word "synchronous" is mispelled
5. Currently, in this driver, the DPAD presses generate key events.
However, most of the other drivers for today's popular controllers, like
DS4 and xbox, produce axis events for these. Is it possible to use axis
in this driver, in order to make it easier for applications to handle
these?
6. There is currently a compiler error in joycon_rumble_amplitudes.
The last (max) amplitude is not a compile-time constant. Suggest to
revise as follows:
static const u16 joycon_max_rumble_amp = 1003;
...
static const struct joycon_rumble_amp_data joycon_rumble_amplitudes[] = {
...
{ 0xc8, 0x0072, joycon_max_rumble_amp }
};
7. In hid-joycon.c, there are currently a lot of defines for different
commands, such as JC_INPUT_BUTTON_EVENT. My personal preference is to
use const u8 for these for type safety.
Hi Siarhei, Thank you for the feedback. I'm about to submit a revised patchset that addresses most of your points along with some other improvements. I'll be sure to CC you on it. See inline below for my response: On Thu, Sep 12, 2019 at 6:08 AM Siarhei Vishniakou <svv@google.com> wrote: > > Hi, > > Thanks for the patchset. Some questions and comments on the driver: > > 1. In the rumble patch, there are some functions that are only used if > CONFIG_JOYCON_FF is enabled. Otherwise, the compiler complains about > unused functions. Could you guard these with #ifdef? > Fixed in v6 > 2. Inside hid-ids, most of the defines use tabs, but the current patch > uses spaces. Could you change to use tabs? > Fixed in v6 > 3. Currently, the driver quits and prints an error if it is not able to > read the calibration data. That means, if the device is being simulated > using /dev/uhid, it is necessary to also properly respond to these > low-level requests for flash memory reading. This is doable, but is a > lot of work. Any way to allow the driver to continue to function even if > calibration data is missing? Maybe just print a warning that the sticks > won't function well? > Added default calibration for v6 > 4. The word "synchronous" is mispelled > Fixed > 5. Currently, in this driver, the DPAD presses generate key events. > However, most of the other drivers for today's popular controllers, like > DS4 and xbox, produce axis events for these. Is it possible to use axis > in this driver, in order to make it easier for applications to handle > these? > The left joy-con controller uses four individual buttons rather than a classic dpad, so I think using the up/down/left/right keys is appropriate. The pro controller does have an actual dpad, but the inputs are reported identically to the left joy-con, and I wanted the driver to report them the same way to userspace (also keeps the driver code cleaner). > 6. There is currently a compiler error in joycon_rumble_amplitudes. > The last (max) amplitude is not a compile-time constant. Suggest to > revise as follows: > static const u16 joycon_max_rumble_amp = 1003; > ... > static const struct joycon_rumble_amp_data joycon_rumble_amplitudes[] = { > ... > { 0xc8, 0x0072, joycon_max_rumble_amp } > }; > Fixed in v6. I'm not sure why gcc wasn't complaining about this for me. The people working on the android port to the nintendo switch were getting the same error, and everything I've read online implies it is not considered a compile time constant in C. Maybe newer gccs are more flexible in ignoring the standard in this case. > 7. In hid-joycon.c, there are currently a lot of defines for different > commands, such as JC_INPUT_BUTTON_EVENT. My personal preference is to > use const u8 for these for type safety. > > Switched the vast majority of the defines to static const uX types in v6.
Version 5 changes: - Removed sysfs interface to control motor frequencies. - Improved rumble reliability by using subcommands to set it. - Changed mapping of the SL/SR triggers on the joy-cons to map to whichever triggers they lack (e.g. a left joycon's sl/sr map to TR and TR2). This allows userspace to distinguish between the normal and S triggers. - Minor refactors Version 4 changes: - Added support for the Home button LED for the pro controller and right joy-con - Changed name from hid-switchcon to hid-joycon - Added rumble support - Removed ctlr->type and use hdev->product instead - Use POWER_SUPPLY_CAPACITY_LEVEL enum instead of manually translating to capacity percentages - Misc. minor refactors based on v3 feedback Version 3 changes: - Added led_classdev support for the 4 player LEDs - Added power_supply support for the controller's battery - Made the controller number mutex static - Minor refactoring/style fixes based on Roderick's feedback from v2 Version 2 changes: - Switched to using a synchronous method for configuring the controller. - Removed any pairing/orientation logic in the driver. Every controller now corresponds to its own input device. - Store controller button data as a single u32. - Style corrections Daniel J. Ogorchock (5): HID: joycon: add nintendo switch controller driver HID: joycon: add player led support HID: joycon: add power supply support HID: joycon: add home led support HID: joycon: add rumble support MAINTAINERS | 6 + drivers/hid/Kconfig | 24 + drivers/hid/Makefile | 1 + drivers/hid/hid-ids.h | 3 + drivers/hid/hid-joycon.c | 1414 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 1448 insertions(+) create mode 100644 drivers/hid/hid-joycon.c