Message-ID: <CAEc3jaC-Tmd2XtK9H2sipBJAhCf16dMWx46r8Hs4p9At3LC_Jg@mail.gmail.com>
Date: Mon, 8 Jul 2024 09:07:56 -0700
From: Roderick Colenbrander <thunderbird2k@...il.com>
To: Max Staudt <max@...as.org>
Cc: Roderick Colenbrander <roderick.colenbrander@...y.com>, Jiri Kosina <jikos@...nel.org>, 
	Benjamin Tissoires <benjamin.tissoires@...hat.com>, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] hid-playstation: DS4: Update rumble and lightbar together

Hi Max,

See my comments inline.

Thanks,
Roderick

On Thu, Jun 20, 2024 at 12:26 PM Max Staudt <max@...as.org> wrote:
>
> Hi Roderick,
>
> So as far as I understand, my suggested driver behaviour is sound, because both the console's own behaviour as well as other drivers show that flags == 0x03 is working perfectly fine with original controllers. Is my understanding correct?
>

The console behavior (I checked the code) does use the flags as well
like I do. The architecture there between usermode/kernel is a bit
different, so in some cases flags do get set when not needed.

Various devices tried to capture bit patterns and see what kind of
worked even though not really right. (Officially licensed controllers
are a different story they use different hid reports.) We didn't know
other devices did this wrong.

> In fact, hid-sony used to send these updates at the same time (it had flags == 0x07), so for some 3rd party controllers, the move to hid-playstation has already been a regression in the FF/lightbar department.
>
Kind of a regression, but not entirely. Many devices didn't work prior
in hid-sony, because sensor coefficients were 0 or they didn't handle
some reports (e.g. the one to get mac address, which was changed to a
more compatible one in hid-playstation).

> Do you see a way to move forward with this patch and get it merged, even if it is with some delay? Is there something that I can improve?
>
>
> As for downstream users' regression tests, this argument confuses me. Could you please give me a bit of help here?
>
> My understanding, so far, is as follows:
>
> Tests checking the FF bit should not fail if, say, the lightbar bit is also set. If they fail, then that means that the test is too sensitive. After all, the patch does not change anything from userspace's point of view, nor from the actual human user's point of view. The DualShock 4 behaves all the same, and it's just the wire protocol that changes.
>
> So if a downstream user wishes to do a full end-to-end integration test, technically they would need to connect a real DualShock 4 and test that. But I can see that this is not easily automatable ;) so they may test at the HID level instead. The result is that, depending on how they structure their tests, they might no longer be testing end-to-end, but rather testing an implementation and its quirks. This bears the risk that the test will fail because of a legitimate change in the driver, or elsewhere in the kernel.

Correct the validation tests are all uhid based, which is the best
which can be done. There is the hid-tools one, but the one which we
help out with, but the key one is the Android ones. We have so many
problems with these. Mostly because of vendors not enabling e.g. FF
support or LED support other things. The kernel landscape for Android
was very fragmented and has been getting better in the last few years
(binary kernels instead of vendors hacking up their own kernel builds
based on e.g. a fork from Qualcomm forked from Android kernel forked
from upstream).

> I suppose this is what you want to avoid, but... isn't avoiding such changes the reason why LTS kernels exist?
>
> And there is only one LTS kernel with this driver, v6.6, released 8 months ago. How did it become necessary to ossify the driver's wire behaviour in this time frame?

The main new Android kernel (public knowledge) is now 6.6 and many new
devices due later this year/early next year will use it.  The eco
system is a lot wider now and the drivers are used a lot on non-mobile
devices (cars, televisions, chromecast,..). Occassionally driver
patches are also backported from upstream to older Android kernels
(patches have to be merged upstream first).

> Hence I'm confused why changing the wire protocol upstream, without breaking any functionality on the original controller or backporting to LTS kernels, creates problems. Either the tests are correctly written in a way to not be affected by this change, mimicking the original gamepad which is unaffected. Or the tests are protected from breaking by using LTS kernels. In the latter case, the tests will break on a kernel version bump - legitimately so, and fixing them should be easy.
>
>
> Am I missing something?
>
>
> Do you see a way to get this patch in?
>
> Would it help you to have some time for warning your downstream contacts to stabilise their tests, and I could re-send this patch in 6 months from now?

Not that I wouldn't want these kind of patches, but I have to weigh
both sides. The pain on addressing things downstream and in Android
conformance tests is quite painful. We would also have both code paths
used in the wild forever, because existing 6.6 devices wouldn't change
behavior. (The official Android tests are kind of kernel version
agnostic as they work across multiple kernel and Android versions.

>
> Hopeful greetings,
> Max
>

Powered by blists - more mailing lists