| Message ID | 20240616163055.75174-1-max@enpas.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v1] hid-playstation: DS4: Update rumble and lightbar together | expand |
Hi Max, Thanks for your patch. I'm a little hesitant to make changes here. The console itself does the updates similar to this, though there are different userspace kind of ways there to send things (so some paths may kind of do what you saw). These flags are often used also to enable other features in the output report (e.g. audio settings or many others). This driver is used in a lot of devices. Different vendors ranging e.g. car manufacturers, phones, TVs,.. have their own unit / regression tests suites to test integration. Where they send FF rumble for example and check the bits in the output reports We went through a lot of pain with many of such vendors (and are still going through some). I would rather not shake things up for such a narrow use cases for non-official devices. Thanks, Roderick On Sun, Jun 16, 2024 at 9:40 AM Max Staudt <max@enpas.org> wrote: > > Some 3rd party gamepads expect updates to rumble and lightbar together, > and setting one may cancel the other. > > This patch maximises compatibility by always sending rumble and lightbar > updates whenever updates are sent to the gamepad: valid_flag0 is always > >= 0x03. > > Further background reading: > > - Apparently the PS4 always sends rumble and lightbar updates together: > > https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2 > > - 3rd party gamepads may not implement lightbar_blink, and may simply > ignore updates with 0x07 set, according to: > > https://github.com/Ryochan7/DS4Windows/pull/1839 > > This patch leaves the lightbar blink feature as-is: Likely, most users > are unaware that it exists, hence it's unlikely that a packet with > 0x07 set will even be sent in practice. Let's leave the function there, > so users of first-party gamepads can continue to use it. > --- > drivers/hid/hid-playstation.c | 55 ++++++++++++++++++++--------------- > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c > index e7c309cfe3a0..986a4ca8b664 100644 > --- a/drivers/hid/hid-playstation.c > +++ b/drivers/hid/hid-playstation.c > @@ -387,12 +387,10 @@ struct dualshock4 { > bool update_bt_poll_interval; > uint8_t bt_poll_interval; > > - bool update_rumble; > uint8_t motor_left; > uint8_t motor_right; > > /* Lightbar leds */ > - bool update_lightbar; > bool update_lightbar_blink; > bool lightbar_enabled; /* For use by global LED control. */ > uint8_t lightbar_red; > @@ -2092,8 +2090,6 @@ static int dualshock4_led_set_brightness(struct led_classdev *led, enum led_brig > } > } > > - ds4->update_lightbar = true; > - > spin_unlock_irqrestore(&ds4->base.lock, flags); > > dualshock4_schedule_work(ds4); > @@ -2143,26 +2139,39 @@ static void dualshock4_output_worker(struct work_struct *work) > > spin_lock_irqsave(&ds4->base.lock, flags); > > - if (ds4->update_rumble) { > - /* Select classic rumble style haptics and enable it. */ > - common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR; > - common->motor_left = ds4->motor_left; > - common->motor_right = ds4->motor_right; > - ds4->update_rumble = false; > - } > + /* > + * PS4 seems to send 0xf3 updates by default, according to a HCI trace: > + * https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2 > + * > + * This seems to be a very compatible value with third-party pads: > + * https://github.com/Ryochan7/DS4Windows/pull/1839 > + * > + * However, hid-playstation in v6.10 does not set the upper nibble, > + * and neither does hid-sony in v6.2, or BlueRetro. We should stick > + * to that for now, to minimise the chance of unexpected changes. > + * > + * So let's always update rumble and lightbar at the same time, with > + * the upper nibble cleared, resulting in valid_flag0 == 0x03. > + * Hopefully this will maximise compatibility with third-party pads. > + */ > + common->valid_flag0 = DS4_OUTPUT_VALID_FLAG0_MOTOR | > + DS4_OUTPUT_VALID_FLAG0_LED; > > - if (ds4->update_lightbar) { > - common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_LED; > - /* Comptabile behavior with hid-sony, which used a dummy global LED to > - * allow enabling/disabling the lightbar. The global LED maps to > - * lightbar_enabled. > - */ > - common->lightbar_red = ds4->lightbar_enabled ? ds4->lightbar_red : 0; > - common->lightbar_green = ds4->lightbar_enabled ? ds4->lightbar_green : 0; > - common->lightbar_blue = ds4->lightbar_enabled ? ds4->lightbar_blue : 0; > - ds4->update_lightbar = false; > - } > + common->motor_left = ds4->motor_left; > + common->motor_right = ds4->motor_right; > + > + /* Compatible behavior with hid-sony, which used a dummy global LED to > + * allow enabling/disabling the lightbar. The global LED maps to > + * lightbar_enabled. > + */ > + common->lightbar_red = ds4->lightbar_enabled ? ds4->lightbar_red : 0; > + common->lightbar_green = ds4->lightbar_enabled ? ds4->lightbar_green : 0; > + common->lightbar_blue = ds4->lightbar_enabled ? ds4->lightbar_blue : 0; > > + /* > + * Output reports updating lightbar_blink will simply be ignored > + * by incompatible controllers, since valid_flag0 == 0x07. > + */ > if (ds4->update_lightbar_blink) { > common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_LED_BLINK; > common->lightbar_blink_on = ds4->lightbar_blink_on; > @@ -2459,7 +2468,6 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e > return 0; > > spin_lock_irqsave(&ds4->base.lock, flags); > - ds4->update_rumble = true; > ds4->motor_left = effect->u.rumble.strong_magnitude / 256; > ds4->motor_right = effect->u.rumble.weak_magnitude / 256; > spin_unlock_irqrestore(&ds4->base.lock, flags); > @@ -2520,7 +2528,6 @@ static void dualshock4_set_default_lightbar_colors(struct dualshock4 *ds4) > ds4->lightbar_green = player_colors[player_id][1]; > ds4->lightbar_blue = player_colors[player_id][2]; > > - ds4->update_lightbar = true; > dualshock4_schedule_work(ds4); > } > > -- > 2.39.2 > >
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? 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. 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. 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? 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? Hopeful greetings, Max
Hi Max, See my comments inline. Thanks, Roderick On Thu, Jun 20, 2024 at 12:26 PM Max Staudt <max@enpas.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 >
Hi Roderick, On 7/9/24 01:07, Roderick Colenbrander wrote: > 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. Thank you so, so much for double checking this. It's always great to have someone who can speak authoritatively on such matters and eliminate the guesswork. > 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. Licensed controllers... That will be my next patch set, apologies in advance :) They need quite a few quirks, too... And as it turns out, my previous patches have laid a lot of ground work for them :) > Correct the validation tests are all uhid based, which is the best > which can be done. Please correct me if I'm getting the wrong idea here, but what I read between the lines and from your email address is that this is something in Sony's interest. So an idea comes to mind: Maybe somewhere inside Sony, there exists something like a DS4 simulator at the HID level, which could serve as a foundation for improving the tests? That would get the tests much closer to the gold standard, which is using a real controller. If not, then maybe there is protocol documentation that could help test writers in creating more precise tests? > 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. Hm, but downstream users misconfiguring kernels is not our fault, is it? In that case, the tests actually do their work correctly if they show that something is amiss. > 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). I see. But still, that is just typical downstream risk of building on behaviour that the kernel does not provide guarantees for. I know first-hand that backporting is a lot of work and easy to get wrong, but this is the first time that I hear that as a reason to stop improving the mainline kernel. Hence my confusion here. > Not that I wouldn't want these kind of patches, but I have to weigh > both sides. Thanks for your understanding, and hence my offer to help if I somehow can... > The pain on addressing things downstream and in Android conformance > tests is quite painful. Hm, I can somewhat imagine this. I've heard that Android conformance is quite strict. Given Sony's supposed interest (see above), I guess it would be a worthwhile investment to make the tests more robust? We could just hold off on this patch for a while until downstream has better tests... What would be a timeline for this to trickle downstream? > We would also have both code paths used in the wild forever, because > existing 6.6 devices wouldn't change behavior. Well, that's kind of the point of LTS releases, if I'm not mistaken... > (The official Android tests are kind of kernel version agnostic as > they work across multiple kernel and Android versions. Hm, sounds to me like the Android test framework is broken if it cannot be kernel-specific in such cases. What's required in order to improve this? Max
Hi Max, On Wed, Jul 10, 2024 at 8:35 AM Max Staudt <max@enpas.org> wrote: > > Hi Roderick, > > > On 7/9/24 01:07, Roderick Colenbrander wrote: > > 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. > > Thank you so, so much for double checking this. It's always great to > have someone who can speak authoritatively on such matters and eliminate > the guesswork. > > > > 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. > > Licensed controllers... That will be my next patch set, apologies in > advance :) Oh I see. Hm, we have been doing a lot of work in this space for both PS4 and PS5 controllers. We just didn't submit it yet and need to clean up the code a bit as we have some internal demand for this as well. We need to get our hands on some more of these controllers, validate before we were about to share the work. There are some details to these controllers (on the console it is kind of a separate driver even). > > They need quite a few quirks, too... And as it turns out, my previous > patches have laid a lot of ground work for them :) > > > > Correct the validation tests are all uhid based, which is the best > > which can be done. > > Please correct me if I'm getting the wrong idea here, but what I read > between the lines and from your email address is that this is something > in Sony's interest. > > So an idea comes to mind: Maybe somewhere inside Sony, there exists > something like a DS4 simulator at the HID level, which could serve as a > foundation for improving the tests? That would get the tests much closer > to the gold standard, which is using a real controller. > > If not, then maybe there is protocol documentation that could help test > writers in creating more precise tests? Unfortunately none of the documentation for our controllers is public. Just read between the lines in the code, which we cover with some clues here and there :) > > > 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. > > Hm, but downstream users misconfiguring kernels is not our fault, is it? > In that case, the tests actually do their work correctly if they show > that something is amiss. > > > > 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). > > I see. But still, that is just typical downstream risk of building on > behaviour that the kernel does not provide guarantees for. I know > first-hand that backporting is a lot of work and easy to get wrong, but > this is the first time that I hear that as a reason to stop improving > the mainline kernel. Hence my confusion here. > > > > Not that I wouldn't want these kind of patches, but I have to weigh > > both sides. > > Thanks for your understanding, and hence my offer to help if I somehow > can... > > > > The pain on addressing things downstream and in Android conformance > > tests is quite painful. > > Hm, I can somewhat imagine this. I've heard that Android conformance is > quite strict. > > Given Sony's supposed interest (see above), I guess it would be a > worthwhile investment to make the tests more robust? We could just hold > off on this patch for a while until downstream has better tests... What > would be a timeline for this to trickle downstream? > > > > We would also have both code paths used in the wild forever, because > > existing 6.6 devices wouldn't change behavior. > > Well, that's kind of the point of LTS releases, if I'm not mistaken... > > > > (The official Android tests are kind of kernel version agnostic as > > they work across multiple kernel and Android versions. > > Hm, sounds to me like the Android test framework is broken if it cannot > be kernel-specific in such cases. What's required in order to improve this? It is a bit of a long process we work on with Google. Some initial fixing of some of the bugs will be for this year to make sure the 6.6 tests pass properly. But any work to maybe handle multiple behaviors, that's quite tricky and would take quite a bit of time to be honest. Considering how widely Android and all these devices are used, I'm hesitant to make changes to not cause regressions. Even just a simple one can take forever to trickle down. > > > Max > > Roderick
On 7/17/24 09:26, Roderick Colenbrander wrote: >> If not, then maybe there is protocol documentation that could help >> test writers in creating more precise tests? > > Unfortunately none of the documentation for our controllers is > public. Just read between the lines in the code, which we cover with > some clues here and there :) I've seen the clues and they are certainly an improvement of hid-playstation over hid-sony that I am grateful for :) As for the documentation, I didn't mean that you should open it to the wide public (although that would be fabulous!). Rather, I suggested providing what's needed to the people writing the downstream tests that we've been going on about for a while now. The utility of tests is... rather limited, if they don't verify an actual specification, but just a specific code's behaviour. After all, these tests would fail even when talking to a real PlayStation 4. >>> (The official Android tests are kind of kernel version agnostic >>> as they work across multiple kernel and Android versions. >> >> Hm, sounds to me like the Android test framework is broken if it >> cannot be kernel-specific in such cases. What's required in order >> to improve this? > > It is a bit of a long process we work on with Google. Some initial > fixing of some of the bugs will be for this year to make sure the 6.6 > tests pass properly. But any work to maybe handle multiple behaviors, > that's quite tricky and would take quite a bit of time to be honest. Sure, time is not the issue, as long as there is a clear light of correct software at the end of the tunnel :) Are these tests open source? Can kernel patchers run the tests easily (without installing a full Android SDK) and see which tests their patches would break? > Considering how widely Android and all these devices are used, I'm > hesitant to make changes to not cause regressions. Even just a simple > one can take forever to trickle down. Apologies, I am still confused. With v6.2, hid-playstation gained DS4 support. With v6.3, hid-sony lost DS4 support. DS4 support was removed from hid-sony just one minor release after it was included in hid-playstation. I didn't see any hesitation there, despite the wire protocol seeing significant changes compared to hid-sony, which had been deployed for almost a decade. Given how quickly the DS4 code was removed from hid-sony, why are fixes suddenly difficult, less than two years into the hid-playstation era, after around 8 years of DS4 support in hid-sony? The change from hid-sony to hid-playstation caused a regression on a real-world device, on Android phones: I have an 8BitDo controller that quirkily emulates a DS4, and which worked fine with my old Android phone, with a kernel from the hid-sony days. With v6.3, this controller broke, and this went into LTS v6.6. Does this kind of regression, which has clearly spread across the Android ecosystem with v6.6, deserve to be fixed? As a generalised question, how about controllers that work on Android phones with kernel v6.1 (hid-sony), but not with v6.6 (hid-playstation), because of protocol changes that don't affect first-party controllers. Do we accept upstream regressions on actual, physical devices for the sake of passing downstream tests? Max
On second thought, maybe we can sidestep this discussion entirely by using a new Kconfig to have both states configurable. Something like: CONFIG_HID_PLAYSTATION_V6_6_ANDROID_TEST_COMPAT Improvements such as my patch would be turned off when this flag is enabled, and the driver behaviour reverted to the v6.6 LTS behaviour. So Android kernels would enable it, and everyone else (especially desktop distributions) can leave it off. Then, once the Android test suite is fixed, the Kconfig can be removed. Looking at the Android release cadence in recent years, and the timeline of DS4 support in hid-playstation and in the v6.6 LTS kernel, I presume this would be in 1-2 years. This way, upstream can continue improving the driver, without triggering Android test failures. How does this sound? Max
On Jul 21 2024, Max Staudt wrote: > On 7/17/24 09:26, Roderick Colenbrander wrote: > > > If not, then maybe there is protocol documentation that could help > > > test writers in creating more precise tests? > > > > Unfortunately none of the documentation for our controllers is public. > > Just read between the lines in the code, which we cover with some clues > > here and there :) > > I've seen the clues and they are certainly an improvement of > hid-playstation over hid-sony that I am grateful for :) > > As for the documentation, I didn't mean that you should open it to the > wide public (although that would be fabulous!). Rather, I suggested > providing what's needed to the people writing the downstream tests that > we've been going on about for a while now. > > The utility of tests is... rather limited, if they don't verify an > actual specification, but just a specific code's behaviour. After all, > these tests would fail even when talking to a real PlayStation 4. > > > > > > (The official Android tests are kind of kernel version agnostic > > > > as they work across multiple kernel and Android versions. > > > > > > Hm, sounds to me like the Android test framework is broken if it > > > cannot be kernel-specific in such cases. What's required in order to > > > improve this? > > > > It is a bit of a long process we work on with Google. Some initial > > fixing of some of the bugs will be for this year to make sure the 6.6 > > tests pass properly. But any work to maybe handle multiple behaviors, > > that's quite tricky and would take quite a bit of time to be honest. > > Sure, time is not the issue, as long as there is a clear light of > correct software at the end of the tunnel :) > > Are these tests open source? > > Can kernel patchers run the tests easily (without installing a full > Android SDK) and see which tests their patches would break? > > > > Considering how widely Android and all these devices are used, I'm > > hesitant to make changes to not cause regressions. Even just a simple > > one can take forever to trickle down. > > Apologies, I am still confused. > > With v6.2, hid-playstation gained DS4 support. With v6.3, hid-sony lost > DS4 support. > > DS4 support was removed from hid-sony just one minor release after it > was included in hid-playstation. I didn't see any hesitation there, > despite the wire protocol seeing significant changes compared to > hid-sony, which had been deployed for almost a decade. > > Given how quickly the DS4 code was removed from hid-sony, why are fixes > suddenly difficult, less than two years into the hid-playstation era, after > around 8 years of DS4 support in hid-sony? Ouch, that is worrisome. > > > The change from hid-sony to hid-playstation caused a regression on a > real-world device, on Android phones: > > I have an 8BitDo controller that quirkily emulates a DS4, and which > worked fine with my old Android phone, with a kernel from the hid-sony > days. With v6.3, this controller broke, and this went into LTS v6.6. > > Does this kind of regression, which has clearly spread across the Android > ecosystem with v6.6, deserve to be fixed? it does, yeah. > > > As a generalised question, how about controllers that work on Android phones > with kernel v6.1 (hid-sony), but not with v6.6 (hid-playstation), because of > protocol changes that don't affect first-party controllers. Do we accept > upstream regressions on actual, physical devices for the sake of passing > downstream tests? The upstream rule is simple: no regressions (that we know about). The argument that downstream tests are hard to do is not correct for upstream. As a general rule of thumb, upstream doesn't care about downstream at all. Regressions is all we care, and a bad test from downstream is not a correct justification to reject a fix upstream. Please understand Roderick that I am not taking side. I perfectly understand the downstream challenges, but we can not refuse a regression fix because the new patch breaks a downstream test. I followed the thread and Max seemed to be OK with waiting and I assumed it was not critical. But if we know about a regression in a device we supported, we should find a solution for it. BTW, that's the reason why I finally managed to push the hid-tools tests in the seftests dir of the kernel directly. Now each kernel has its own set of tests, and there is no more discrepancies between tests and regressions that are found. Cheers, Benjamin > > > > Max
On Jul 22 2024, Max Staudt wrote: > On second thought, maybe we can sidestep this discussion entirely by using a > new Kconfig to have both states configurable. > > Something like: > > CONFIG_HID_PLAYSTATION_V6_6_ANDROID_TEST_COMPAT Please don't. As explained in the other email, we don't care about downstream, so we can not take these never ending ifdef. > > Improvements such as my patch would be turned off when this flag is enabled, > and the driver behaviour reverted to the v6.6 LTS behaviour. So Android > kernels would enable it, and everyone else (especially desktop > distributions) can leave it off. > > Then, once the Android test suite is fixed, the Kconfig can be removed. > Looking at the Android release cadence in recent years, and the timeline of > DS4 support in hid-playstation and in the v6.6 LTS kernel, I presume this > would be in 1-2 years. > > This way, upstream can continue improving the driver, without triggering > Android test failures. Is there anyway to detect that the device can not support the current behavior? And if so then dynamically switch to the new behaviour? Cheers, Benjamin > > > How does this sound? > > Max >
On 7/23/24 01:46, Benjamin Tissoires wrote: >> As a generalised question, how about controllers that work on >> Android phones with kernel v6.1 (hid-sony), but not with v6.6 >> (hid-playstation), because of protocol changes that don't affect >> first-party controllers. Do we accept upstream regressions on >> actual, physical devices for the sake of passing downstream tests? > > The upstream rule is simple: no regressions (that we know about). > The argument that downstream tests are hard to do is not correct for > upstream. As a general rule of thumb, upstream doesn't care about > downstream at all. Regressions is all we care, and a bad test from > downstream is not a correct justification to reject a fix upstream. > > Please understand Roderick that I am not taking side. I perfectly > understand the downstream challenges, but we can not refuse a > regression fix because the new patch breaks a downstream test. > > I followed the thread and Max seemed to be OK with waiting and I > assumed it was not critical. But if we know about a regression in a > device we supported, we should find a solution for it. Thanks for clarifying the general rule about no regressions! I asked in the general sense, because I needed to know how the "no regressions" rule works in clear-cut cases, and that the move from v6.2 to v6.3 indeed counts as introducing regressions, and that they are worth fixing even if they break downstream tests. If you're interested: The 8BitDo controller failed due to a request that was a warning in hid-sony, and then became a hard error in hid-playstation. This regression was fixed in 46089080a8e1. Please let me clarify the current patch, since it's maybe less clear-cut. Do you have guidance on this as well? With this patch, I intend to port over a behaviour from hid-sony, but I can (currently?) only test it on a controller which also needs a behaviour that appeared in the kernel with hid-playstation (it's the 0x12 feature report that Roderick mentioned previously). So it's not a clear-cut regression, but still I am porting a behaviour that was there before. Hence I am trying to merge the behaviour of both drivers, for maximum compatibility. Since it's possible that other controllers also worked better with hid-sony's output reports, I've looked for third-party hints, and mentioned them in my code comments and commit message, to better gauge what the most compatible path forward may be. It seems like a middle ground between both drivers is the most compatible, so my patch takes this into account. Where does this fall on the continuum of patches to take in or not? And as an even more general question: Do downstream tests count as things that we don't break when fixing real devices? > BTW, that's the reason why I finally managed to push the hid-tools > tests in the seftests dir of the kernel directly. Now each kernel has > its own set of tests, and there is no more discrepancies between > tests and regressions that are found. Great, thank you! Max
On 7/23/24 01:49, Benjamin Tissoires wrote: > Is there anyway to detect that the device can not support the current > behavior? And if so then dynamically switch to the new behaviour? Sadly, no, otherwise I would have used that already :( Also, the change that my patch makes is in the "dialect" of the wire protocol. This seemingly changes zero in the original Sony device's behaviour, both from what my tests have shown, and from what I gather from Roderick's emails. HID traces on the web confirm that the PS4 may output such or similar reports as well. Hence I don't understand why we would want to make a distinction at all, if the driver can simply only speak the "dialect" that works on most devices. Max
On Mon, Jul 22, 2024 at 12:31 PM Max Staudt <max@enpas.org> wrote: > > On 7/23/24 01:49, Benjamin Tissoires wrote: > > Is there anyway to detect that the device can not support the current > > behavior? And if so then dynamically switch to the new behaviour? > > Sadly, no, otherwise I would have used that already :( > > > Also, the change that my patch makes is in the "dialect" of the wire > protocol. This seemingly changes zero in the original Sony device's > behaviour, both from what my tests have shown, and from what I gather > from Roderick's emails. HID traces on the web confirm that the PS4 may > output such or similar reports as well. Hence I don't understand why we > would want to make a distinction at all, if the driver can simply only > speak the "dialect" that works on most devices. > > > > Max > Sorry for my late reply. I just got back from vacation. Just catching up on emails. In general as Max also mentioned it is not a true regression as various of these devices didn't work before. I have been thinking about how to handle things. Preferably it would be some type of quirk. Just changing the patterns is not a good idea also in case hypothetically other features were get added (volume control for the speaker, microphone settings,..) and other features which work through the same output report and various of these devices probably won't handle (or it is hard to predict). I have been trying to think of ways to realize a quirk. I think we need to search it in the HID reports. Either do some tests on reports we know aren't supported (ugly). It can also be that the calibration data is invalid (zeros, which we now initialize to a default). Or perhaps is the firmware/hardware version related HID report returning anything interesting? Thanks, Roderick
Hi Roderick, Welcome back, and thanks for replying! Comments below :) On 8/9/24 08:11, Roderick Colenbrander wrote: > Preferably it would be some type of quirk. Agreed. > Just changing the patterns is not a good idea also in case > hypothetically other features were get added (volume control for the > speaker, microphone settings,..) and other features which work through > the same output report and various of these devices probably won't > handle (or it is hard to predict). I think at this point the discussion and/or your concerns may have diverged because of misunderstandings. I do not intend to touch any upper bits in valid_flag0. I've been thinking about this, and my conclusion is that both my patch (v1) and its description were trying too hard to squeeze in as much context as possible. I think they were too hard to understand - sorry! So to make things easier to understand for everyone, I have sent a v2 of the patch. It's really short now, and it should be obvious that I am not touching any flag bits that the driver didn't touch already. Hopefully this can get everyone on the same page - could you please have a look? Maybe this patch is actually fine with you. > I have been trying to think of ways to realize a quirk. I think we > need to search it in the HID reports. Either do some tests on reports > we know aren't supported (ugly). It can also be that the calibration > data is invalid (zeros, which we now initialize to a default). Or > perhaps is the firmware/hardware version related HID report returning > anything interesting? I'll need to have a look, but there's a catch here: Bluetooth devices can't be tested for some of the quirks that USB devices have, for example the MAC address report that clones usually don't implement. I wish to fix both classes of devices, since I've seen a quirky BT device some (longer) time ago. Could you please look at the new PATCHv2 first? Maybe that one is fine with you, and it all boils down to me just not presenting things well in v1. Thanks :) Max
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c index e7c309cfe3a0..986a4ca8b664 100644 --- a/drivers/hid/hid-playstation.c +++ b/drivers/hid/hid-playstation.c @@ -387,12 +387,10 @@ struct dualshock4 { bool update_bt_poll_interval; uint8_t bt_poll_interval; - bool update_rumble; uint8_t motor_left; uint8_t motor_right; /* Lightbar leds */ - bool update_lightbar; bool update_lightbar_blink; bool lightbar_enabled; /* For use by global LED control. */ uint8_t lightbar_red; @@ -2092,8 +2090,6 @@ static int dualshock4_led_set_brightness(struct led_classdev *led, enum led_brig } } - ds4->update_lightbar = true; - spin_unlock_irqrestore(&ds4->base.lock, flags); dualshock4_schedule_work(ds4); @@ -2143,26 +2139,39 @@ static void dualshock4_output_worker(struct work_struct *work) spin_lock_irqsave(&ds4->base.lock, flags); - if (ds4->update_rumble) { - /* Select classic rumble style haptics and enable it. */ - common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_MOTOR; - common->motor_left = ds4->motor_left; - common->motor_right = ds4->motor_right; - ds4->update_rumble = false; - } + /* + * PS4 seems to send 0xf3 updates by default, according to a HCI trace: + * https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2 + * + * This seems to be a very compatible value with third-party pads: + * https://github.com/Ryochan7/DS4Windows/pull/1839 + * + * However, hid-playstation in v6.10 does not set the upper nibble, + * and neither does hid-sony in v6.2, or BlueRetro. We should stick + * to that for now, to minimise the chance of unexpected changes. + * + * So let's always update rumble and lightbar at the same time, with + * the upper nibble cleared, resulting in valid_flag0 == 0x03. + * Hopefully this will maximise compatibility with third-party pads. + */ + common->valid_flag0 = DS4_OUTPUT_VALID_FLAG0_MOTOR | + DS4_OUTPUT_VALID_FLAG0_LED; - if (ds4->update_lightbar) { - common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_LED; - /* Comptabile behavior with hid-sony, which used a dummy global LED to - * allow enabling/disabling the lightbar. The global LED maps to - * lightbar_enabled. - */ - common->lightbar_red = ds4->lightbar_enabled ? ds4->lightbar_red : 0; - common->lightbar_green = ds4->lightbar_enabled ? ds4->lightbar_green : 0; - common->lightbar_blue = ds4->lightbar_enabled ? ds4->lightbar_blue : 0; - ds4->update_lightbar = false; - } + common->motor_left = ds4->motor_left; + common->motor_right = ds4->motor_right; + + /* Compatible behavior with hid-sony, which used a dummy global LED to + * allow enabling/disabling the lightbar. The global LED maps to + * lightbar_enabled. + */ + common->lightbar_red = ds4->lightbar_enabled ? ds4->lightbar_red : 0; + common->lightbar_green = ds4->lightbar_enabled ? ds4->lightbar_green : 0; + common->lightbar_blue = ds4->lightbar_enabled ? ds4->lightbar_blue : 0; + /* + * Output reports updating lightbar_blink will simply be ignored + * by incompatible controllers, since valid_flag0 == 0x07. + */ if (ds4->update_lightbar_blink) { common->valid_flag0 |= DS4_OUTPUT_VALID_FLAG0_LED_BLINK; common->lightbar_blink_on = ds4->lightbar_blink_on; @@ -2459,7 +2468,6 @@ static int dualshock4_play_effect(struct input_dev *dev, void *data, struct ff_e return 0; spin_lock_irqsave(&ds4->base.lock, flags); - ds4->update_rumble = true; ds4->motor_left = effect->u.rumble.strong_magnitude / 256; ds4->motor_right = effect->u.rumble.weak_magnitude / 256; spin_unlock_irqrestore(&ds4->base.lock, flags); @@ -2520,7 +2528,6 @@ static void dualshock4_set_default_lightbar_colors(struct dualshock4 *ds4) ds4->lightbar_green = player_colors[player_id][1]; ds4->lightbar_blue = player_colors[player_id][2]; - ds4->update_lightbar = true; dualshock4_schedule_work(ds4); }
Some 3rd party gamepads expect updates to rumble and lightbar together, and setting one may cancel the other. This patch maximises compatibility by always sending rumble and lightbar updates whenever updates are sent to the gamepad: valid_flag0 is always >= 0x03. Further background reading: - Apparently the PS4 always sends rumble and lightbar updates together: https://eleccelerator.com/wiki/index.php?title=DualShock_4#0x11_2 - 3rd party gamepads may not implement lightbar_blink, and may simply ignore updates with 0x07 set, according to: https://github.com/Ryochan7/DS4Windows/pull/1839 This patch leaves the lightbar blink feature as-is: Likely, most users are unaware that it exists, hence it's unlikely that a packet with 0x07 set will even be sent in practice. Let's leave the function there, so users of first-party gamepads can continue to use it. --- drivers/hid/hid-playstation.c | 55 ++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 24 deletions(-)