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@xxxxxxxxxx> 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.
--
Daniel Ogorchock
