atar-axis / xpadneo

Advanced Linux Driver for Xbox One Wireless Controller (shipped with Xbox One S)
https://atar-axis.github.io/xpadneo/
GNU General Public License v3.0
1.95k stars 112 forks source link

Rumble needs ERTM enabled to work reliably #272

Closed kakra closed 3 years ago

kakra commented 3 years ago

With the L2CAP patch (which enabled you to connect with ERTM enabled) getting some more thorough testing, I've found the following interesting behavior: Rumble actually works stable (no controller crashes) if ERTM is enabled.

The L2CAP patch is currently in the process of being upstreamed, probably appearing in kernel 5.12, and maybe backported.

Consequently, I'll be linking rumble-related issues to this, possibly closing them as duplicate.

This means that you should enable ERTM. But it won't work unless your distribution includes the L2CAP patch in the kernel.

Also, tests showed that the controller has to be removed from Bluetooth and re-paired after toggling ERTM, otherwise the connection may be quite unstable or not establish at all.

orbea commented 3 years ago

Can you link to the L2CAP patch please?

kakra commented 3 years ago

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=98d2c3e1731007acf03addf83c863df6694beb95, also linked in #198.

orbea commented 3 years ago

Thanks, I noticed now its also part of https://github.com/kakra/linux/pull/11. I will need to retest after removing and repairing my xbox one gamepad.

kakra commented 3 years ago

It has been part of it since kernel 5.4 which I was using. That explains why I was not seeing some of the problems people reported. I actually used the driver with ERTM disabled for the past weeks (months?) and could reproduce many issues, now I turned ERTM on again and at least the rumble problems went away.

Mikaka27 commented 3 years ago

Hi, I've updated my manjaro linux to 5.12 kernel. Then I removed disabling of ertm from /etc/modprobe.d/99-xpadneo-bluetooth.conf After removing and repairing gamepad it connects successfully with ertm enabled.

What would be the way forward? Could we add a check for kernel version in dkmx.post_install script to not disable ertm if kernel is 5.12 or later?

kakra commented 3 years ago

What would be the way forward? Could we add a check for kernel version in dkmx.post_install script to not disable ertm if kernel is 5.12 or later?

Yes, that would probably be a solution I was thinking about, too. But I wanted to wait for actual kernel versions to emerge in distributions, so we can actually test such an implementation. If you feel like you could add such a check, I'd happily take pull requests. Otherwise, you'd need to wait a few days until I implemented it, and I'd like you to test that then.

I think this is a good step forward. Ultimately, I'd rather completely remove the ERTM stuff from xpadneo (there are patches in my queue for this). I'm rethinking this when the LTS kernel comes out which will probably be 5.15 or 5.16 at the end of the year.

Mikaka27 commented 3 years ago

I can try to create a pull request later today.

ionenwks commented 3 years ago

I'll do something similar for the official Gentoo ebuild if that sounds good, previous ERTM warning if <5.12, otherwise different warning to suggest re-enabling + re-pair only if disabled ertm is detected (currently it nags if enabled instead).

kakra commented 3 years ago

@ionenwks Thanks, I'm actually doing development on Gentoo but never checked the ebuild because I'm running directly from the xpadneo source anyways. But I looked at the ebuild lately and it looks fine. BTW: From v0.10 forward (and current master), you may pass in the current version directly to the make file through VERSION="v${PV}-gentoo" (see this change). There will be a few more changes probably soon which affect the 9999 ebuild. If you like, I can tag your username in such pull-requests so you can test it first. Or we check if it's possible to test the 9999 ebuild in the Azure CI pipeline but I don't know if they support Gentoo.

BTW: The re-connect loop is actually not really due to ERTM. In fact, ERTM enabled makes the gamepad run more stable - it just won't pair in this case most of the times unless the L2CAP patch is there. In Gentoo it's easy to put the L2CAP patch into /etc/portage/patches/sys-kernel/gentoo-sources (which is what I did). The ebuild could suggest that. The patch is included and could be installed along the docs.

The reconnect loop more likely stems from other factors:

  1. Surrounding RF noise from nearby Bluetooth devices (but most likely not, but MS suggests that)
  2. KDE Plasma constantly running BT inquiries (scan for devices): This happens if kdeconnect is installed with Bluetooth support: https://github.com/bluez/bluez/issues/123 (may affect other desktop environments which scan for BT devices)
  3. Unsupported order of Bluetooth events: https://github.com/bluez/bluez/issues/127 (most common, no work-around known)
ionenwks commented 3 years ago

@kakra The ebuild's addition is fairly recent (since mid-March), currently managed through proxy-maint project in Gentoo but I'm working toward gaining direct access for faster updates.

I'll adjust the ERTM warning to be more accurate, thanks for the info. Adding a brief mention of the patch + installing in docs also sounds fair should users not want to upgrade to 5.12 yet. (Edit: hoping it gets backported so it won't really matter, then I'll cleanup minus a check to encourage re-enabling ERTM if anyone still has it disabled)

I'm thankful for the recent version handling changes, it allowed to remove workarounds. Currently it's working as expected and reporting v0.9.1 / v0.9-62-g994958f without manually specifying anything by reading the VERSION file. Adding -gentoo is not something we usually do unless we're making custom code changes upstream need to know about.

I already test the live ebuild fairly regularly alongside new kernel versions, and users are expected to use releases more than -9999, so don't worry too much about breaking it.

kakra commented 3 years ago

encourage re-enabling ERTM if anyone still has it disabled

aka "reverting to default".

I'm thankful for the recent version handling changes

It was actually tailored that way for exactly that purpose. I found that many package maintainers jumped some loops and hoops here, and I still don't like the result 100%, I'd prefer removing the VERSION text file completely so I wouldn't need to carry it around as an extra patch in the tagged releases. But I'm not sure yet how to solve this. At least, the file is gone in version built directly from git master. But this is getting off-topic now. If you need any help with the ebuild, feel free to open an issue report. I'm proxy-maintained for a few ebuilds myself.

kakra commented 3 years ago

Should be fixed by https://github.com/atar-axis/xpadneo/pull/288