Hi Luke, thank you for taking the time to reply. And everyone else, thank you for putting up with my broken line spacing, because of Gmail. And perhaps the lack of in-reply-to because of mailto. Hopefully in-reply-to works this time ;). First off, understand that I do not mean to attack your work. I tried to make my response helpful to you. In fact, I took a lot of time in writing it and preemptively saved you a lot of work in testing your patchset so you do not have to spend time reaching the same conclusions that I have had to. Provided you heed my comments of course, which is not clear from your reply. As I currently represent what is the largest Linux ROG Ally community, I hope it is reasonable that it is in my and my community's best interest that an unstable patch which could affect Linux Ally support should be vetted before it becomes part of the mainline kernel. Once it becomes part of the kernel, workarounds around it will become very hard. We have achieved near perfect controller support through userspace, so I would hate to jeopardize that. In my previous email, I gave you my preliminary thoughts without having time to test your patchset. Of course, as I noted in that email, I will be testing and integrating support for your patchset, personally, with an Ally X unit I will be getting access to close to the end of August. I do not think there is value in arguing, therefore I am not happy continuing the discussion under this tone. Hopefully, in a few days you, will have another look at my comments, with a level head, and address them. I still believe that they are valid and that if they are fixed, I am more than happy with your patchset merging into the kernel. Since you raised some technical points in your response, let me disambiguate. > You're repeating information that has come directly from me. Indeed, this specific point (XInput being deprecated) came from you. I am just bringing everyone up to speed, since I feel your patch missed some important context. > I have many records from many MCU updates. It doesn't happen. (referring to > relying to the endpoint descriptor instead of HID Usages) In my opinion, using the standard Usage Page and Usage the controller reports remains the proper solution (this is what the Windows driver does). Remember that if there is a breakage due to a firmware update, users will become unable to use their device as they can not update the kernel. Using `desc.bEndpointAddress` may be appropriate for a userspace tool or an out-of-tree kernel driver, but perhaps not for the mainline kernel. I am happy to be proven wrong. > "Attempted"... I *did*. You've failed to notice that what I've set is > what is reported by the HID report. I am a bit confused here. I thought the purpose of your patch was to convert the HID report into what xpad would export. That means respecting xpad, not a random HID report. In this case, the absinfo (with `input_set_abs_params`) needs to be set according to what is set by xpad, which is signed and from -32768 to 32767 (referencing both the Linux Gamepad Specification and the out-of-tree driver xpadneo which seems to be the prominent driver providing support for controllers similar to Ally X, i.e., Xbox One bluetooth controllers). I know from experience that Handheld Daemon will have a problem with this. But alas, I was not referring to Handheld Daemon being the problem here: it is simple enough to fix it in there and I will do it when I add support for your patchset (would rather avoid doing it or doing it and having to revert it of course). I was moreso referring to other userspace applications without this privilege. As for why I have to add support for your patchset, it is simply because it being there changes the controller mappings, so I simply need to add an if statement that uses the standard XInput mappings when it is available. This is not to say that the end result will be as reliable as without your driver, as Handheld Daemon will then be at the mercy of your kernel driver. So please, do extended testing and I think with e.g., ChimeraOS 46.2 being released yesterday with your patchset, you will get some valuable feedback soon enough. I know that I have spent multiple weeks already optimizing my implementation, having released it close to a month ago. Which is also why I am not in a rush to add support for your experimental patchset. > It is a very different story in a kernel driver... (referring to the 80ms > delay used for Xbox+A) Please understand that I have spent weeks of effort debugging and optimizing the Side Menu behavior of Handheld Daemon. In fact, it currently implements three different ways of opening the Steam Side Menu (keyboard, extest through the gamescope X11 socket, and as a last fallback as Xbox + A). When I say 80ms is not enough, I know it is not enough. Otherwise Handheld Daemon would be using 80ms. The rest is conjecture. > It is done on a worker. It is not blocking the kernel.... > (referring to xbox+a holding a spinlock to send the key combo with msleep) Repeating myself: > > In addition, you are freezing the kernel **driver** to send those > > commands for 240ms which is around 100 reports. Which in my opinion will become more like 300ms. Freezing the controller for that long is not ideal (I know you are not freezing the kernel). Please revisit this. > Please describe how you think it is broken? (referring to `mcu_powersave`) Quoting myself: > > This feature remains broken when the device is at low TDPs and unplugged. e.g., when the Ally is set on its quiet mode and or is below 12W, and is suspended unplugged, with Steam and a game running. In this case, the USB controller of the Ally simply does not wake up and RGB breaks. The occurrence of this given those conditions is around 40% of the time. This includes testing with or without your DMI table patch by the way. > Does not work how? (referring to `ally_mcu_usb_switch`) Seems like a DMI table always sets it to 0. I do not know why. However I do know that as part of our validation on the distribution Bazzite which took place prior to you submitting your patch, we tested both adding an or for ally x and a dmi table, and the dmi table did not work. Post submitting your patch, there was a 5 day brief period where both the unstable ChimeraOS kernel and the CachyOs kernel integrated your patch before they also integrated the patch I am replying to (which makes Handheld Daemon not work; for now). During this period both the original ally and the ally x regressed when `mcu_powersave` was disabled. > I test, mate. With the default kernel and empty userspace. Unfortunately, this is not how users interact with my kernel patches and Handheld Daemon. They usually play games with SteamUI running in the background. Often, this includes setting an aggressively low TDP, and multiple suspends back-to-back while in game. This is the standard I hold myself to. And I would expect no less from a mainline kernel driver. I hope I replied to all your technical claims. If I missed any, I am happy to clarify and expand where appropriate. Again, good luck with your patchset. Hopefully, it will merge with 6.12 and Ally X owners will get an acceptable result without the need for userspace tools (albeit without gyro, back buttons, and RGB being part of the controller). Best regards, Antheas
