Revised bluetooth configurator, second attempt

[c0d3h4x0r]

I've fixed a couple defects in my own previous work in this branch, and I also went through and addressed all the points of concern you raised about function naming, function locals, and not restricting to only input devices. While this does continue (for now) to use my rewritten Python script for the pairing agent on the backend, that in no way prevents you from moving to some other backend tool or pairing agent script in the future, and it buys immediate UX and reliability improvements for the user while you (we?) figure out a longer-term plan for the back end. I (and plenty of other users I know) would very much appreciate if you would take a second look at this.

If the commit history alone is still too messy for your tastes, I'm happy to reconstruct the end result in a more incremental series of more clearly-labeled commits in a new branch. I just don't want to go to that trouble before knowing the end result is acceptable to you.

[c0d3h4x0r]

And, even if you ultimately still don't want to take these changes, I think there's been value (both for me in getting acclimated to your coding conventions and preferred working style, and for you in having a number of real issues brought to your attention and one working fix approach to them put in front of you) in pursuing this, so I really don't resent the time investment. Besides, I can always use my own branch as a workaround for the issues until they ultimately get fixed however they officially get fixed.

[c0d3h4x0r]

In the meanwhile, I'll look at what's available in bluez-tools, as well as the behavior of the "default agent" provided by bluez, and see if there are viable stock replacements for some of the new custom Python code I've written.

[c0d3h4x0r]

Per https://github.com/khvzak/bluez-tools, it provides the following tools:

• bt-adapter - for listing adapters and showing info about them
• bt-agent - pairing agent (very relevant)
• bt-device - for listing and showing device info, as well as pairing/connecting/trusting/disconnecting/removing devices (also relevant, and already being used by bluetooth.sh for several other things)
• bt-network - not seemingly relevant to current functionality
• bt-obex - not seemingly relevant to current functionality

So I will look into revising my branch to have bluetooth.sh use bt-agent and bt-device instead of the Python scripts I've got it using now. Keep in mind that the feasibility of doing so will depend upon the reliability and correctness of these tools, so I'll be evaluating that on my end as I go.

[c0d3h4x0r]

Looks like the latest release of bluez-tools available for Raspbian Buster is 2.0~20170911.0.7cb788c-2, which is much older than the most recent commit to https://github.com/khvzak/bluez-tools master. But from my tinkering around with the various bt-* commands included in that version, they all appear to work, including device discovery, pair-and-connect, and all the other needed operations. I should be able to revise bluetooth.sh to use these without much trouble, and then I can ditch all the custom Python code.

[c0d3h4x0r]

So, bad news... upon further investigation, that version of bluez-tools does not pair devices correctly. It implements the pairing agent incorrectly, such that when the user is supposed to be given a randomly-generated PIN or passkey by bluez to enter on their device (such as a Bluetooth keyboard), instead, the bt-agent incorrectly prompts the user to confirm that PIN or passcode on the host itself, which is wrong. This is a common mis-implementation of the bluez pairing agent caused by folks not reading the bluez documentation and understanding how pairing is supposed to work for that case; it's what motivated me to write my own pairing agent in Python in the first place.

I don't know if this has been corrected by changes committed to bluez-tools since that 20170911 release, but if it has, it will either require someone to build and upload a new apt-installable package that will work with Raspbian Buster, or it will require RetroPie-Setup to git clone and build a newer bluez-tools directly from source.

[c0d3h4x0r]

There have only been two commits to bluez-tools since September 2017, and neither of them address this problem, so if you want to use bluez-tools for pairing, we're going to have to fix it first.

I have filed an issue in the bluez-tools issue tracker here: khvzak/bluez-tools#41

[c0d3h4x0r]

Additionally, the bt-device --connect command -- used to pair and connect to a device -- does not accept any kind of "capability" command-line option to pass through to the pairing agent, even though bt-agent itself accepts such an option. That means bt-device --connect doesn't provide the usage interface that RetroPie's bluetooth.sh script requires for supplying the agent capability.

I perused the source code for bt-device and bt-agent, and at least as of right now, it would be much more work to come up with fixes for them than to maintain the Python scripts I've already written herein.

[c0d3h4x0r]

I've filed an issue on the lack of "capability" command-line option here: khvzak/bluez-tools#42

[c0d3h4x0r]

I'm fluent in C/C++ and will spend some time trying to come up with reasonable fixes to bluez-tools for these two issues, since that's clearly the desired long-term direction here.

[cmitu]

Is this PR ready to be tested/reviewed ?
It looks like it's missing some files - trying to pair a device immediately throws an error and the log shows

stdbuf: failed to run command ‘/home/pi/RetroPie-Setup/scriptmodules/supplementary/bluetooth/pair-device’: No such file or directory

[c0d3h4x0r]

That’s strange... maybe my file rename didn’t get picked up correctly by `git add -u`? I’ll double check and follow up. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Cristi Mitrana <[email protected]> Sent: Friday, July 10, 2020 11:01:24 PM To: RetroPie/RetroPie-Setup <[email protected]> Cc: Keith F. Kelly <[email protected]>; Author <[email protected]> Subject: Re: [RetroPie/RetroPie-Setup] Revised bluetooth configurator, second attempt (#3193) Is this PR ready to be tested/reviewed ? It looks like it's missing some files - trying to pair a device immediately throws an error and the log shows stdbuf: failed to run command ‘/home/pi/RetroPie-Setup/scriptmodules/supplementary/bluetooth/pair-device’: No such file or directory — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FRetroPie%2FRetroPie-Setup%2Fpull%2F3193%23issuecomment-656997214&data=02%7C01%7C%7Ca24765337a9a4e1bc37008d8255fd7d8%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637300440848403546&sdata=0IxJd97%2B5tArMNX2rBD%2BGC4542rxFjVmZ8mfWoxaV5U%3D&reserved=0>, or unsubscribe<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAF2EYJCIN6XF3DEV3YB7RNLR2753JANCNFSM4OWL2OPA&data=02%7C01%7C%7Ca24765337a9a4e1bc37008d8255fd7d8%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637300440848403546&sdata=DuXNk9X%2FexYzWhh49D6rvGeKoLHcNYnsZg%2BbL4Y8QZA%3D&reserved=0>.

[c0d3h4x0r]

Yep, crap, that's what happened -- I assumed git add -u would be smart enough to track a file rename, but apparently it's not, so my renamed-and-modified version of pair-input-device got lost. I'll reconstruct it and recommit it.

[c0d3h4x0r]

Fixed. Please try again.

[c0d3h4x0r]

I'm also about to push another small commit to address my own minor review items above.

[c0d3h4x0r]

PIN/passkey entry scenarios are still busted due to problematic prompting in the main pairing loop in bluetooth.sh. I'm working up a fix for that as well.

[c0d3h4x0r]

Fixed. This is now working again on my end.

[cmitu]

I re-tested with the latest changes.

1. (UI related) - the new info window is too small. For instance, when trying to pair a device the text is wrapped in the small dialog area.

2. (UI related) - The language to select one of the options/actions from a list of possible values is - Choose xyz/Choose an option - this is how it's used by other scriptmodules and I prefer it to the Which xyz do you want to ... ?. For consistency, I think the existing language should be kept.

3.(UI related) - We don't use really use progress dialogs in the scriptmodules. Besides the scanning dialog - which should give an indication of a scan in progress - I don't think we need a Working progress dialog for the 'Display all connected BT devices' or for 'Removing a BT device', these operation are not long lasting actions.

4. (BT) I had 5 BT devices already paired when started testing, 2 of them being PS3 controllers. The PS3 controllers are not shown in the paired devices listing. They're also not shown in the removing devices dialog.
   However, they're shown in every 'Pair a BT device' dialog, even if they're offline.
   Somehow, it looks like the PS3 controllers are detected as online, not as previously registered devices.

5. (BT) I paired a few BT devices, seems to be fine - but I noticed there's no prompt for the connection mode. Is this intentional or the display of the connection mode/PIN is determined dynamically ?

6. (BT) Listing existing devices seems improved - it only list the paired devices. Scanning and pairing seems to be similar to previous version. Removing devices is ok, though I don't think it warrants a progress dialog.

I'll have to try the PS3 pairing, this is - by far - the largest source of complaints regarding Bluetooth gamepads in the forum.

As a general comment on the PR, it should have a list of changes from the previous version.
The change of terminology (Connect -> Pair) looks ok and the re-arrangement of the menu items is better. To be honest though, the 'UX' seems largely unchanged.

Thank you for your time.

[c0d3h4x0r]

I re-tested with the latest changes.

Thank you for taking some time to try it out and provide feedback.

1. (UI related) - the new info window is too small.

By design. Large, mostly-empty dialogs don’t make visual sense, and are thus a bad convention.

2. (UI related) - The language to select one of the options/actions from a list of possible values

I acknowledge this is subjective. I’ll revert those text changes if it improves the chances of this change eventually getting accepted.

3.(UI related) - We don't use really use progress dialogs

A progress dialog is a dialog that shows a progress meter/bar. These are just informational message boxes. Any time a user initiates an action but gets zero visual feedback for longer than about 0.25 seconds is a bad user experience, as it creates the feel of a sluggish and unresponsive UI. Honestly, the current UI/UX screams “I was designed by engineers who treat usability as an afterthought”. This proposed change is trying to help remedy that for at least this specific area. If it leads to a broader discussion about design principles, even better :)

4. (BT) I had 5 BT devices already paired when started testing, 2 of them being PS3 controllers. The PS3 controllers are not shown in the paired devices listing. They're also not shown in the removing devices dialog.
   [...]
   I'll have to try the PS3 pairing, this is - by far - the largest source of complaints regarding Bluetooth gamepads in the forum.

I do not have a single PS3 controller to try to repro with. If you’re willing to grant me
some time, I’m willing to track down and order one on eBay or something so I can be sure PS3 controllers work just as well as anything else.

5. (BT) I paired a few BT devices, seems to be fine - but I noticed there's no prompt for the connection mode. Is this intentional or the display of the connection mode/PIN is determined dynamically ?

Yes, it’s intentional. That dialog isn’t for “connection mode”, it’s for a developer-internal concept (bluez pairing agent capability), which should never need to be shown to the user. Most users have no clue what they’re being asked or what the implications of the choice are anyway. With my changes, it is automatically determined based on whether the user has a keyboard, a gamepad, or no input device already attached.

As a general comment on the PR, it should have a list of changes from the previous version.

Yep — once I get this wrestled into a condition you all deem acceptable, then I’ll go back and reconstruct the end result through a cleaner and clearer series of incremental changes in another branch. I just don’t want to burn my time on that until everything else is squared away first.

The change of terminology (Connect -> Pair) looks ok and the re-arrangement of the menu items is better.

Thanks — I agree :)

To be honest though, the 'UX' seems largely unchanged.

That’s really not true. The current UX (which includes the reliability — or lack thereof — of the back end pairing script) is broken in a number of ways. Just trying to successfully scan for and pair a single device (especially something requiring a PIN or passkey) is a major exercise in frustration today. You can see a detailed description of the problems in my first PR on this topic (which has been closed).

[cmitu]

1. (UI related) - the new info window is too small.

By design. Large, mostly-empty dialogs don’t make visual sense, and are thus a bad convention.

I agree, but that's not a reason to cram 3 words on 2 lines.

3.(UI related) - We don't use really use progress dialogs

A progress dialog is a dialog that shows a progress meter/bar. These are just informational message boxes. Any time a user initiates an action but gets zero visual feedback for longer than about 0.25 seconds is a bad user experience, as it creates the feel of a sluggish and unresponsive UI. Honestly, the current UI/UX screams “I was designed by engineers who treat usability as an afterthought”. This proposed change is trying to help remedy that for at least this specific area. If it leads to a broader discussion about design principles, even better :)

Sorry, progress dialog was a bad choice. What I meant is that the operations waiting for those info boxes are not 'expensive' enough to warrant an info box.

I do not have a single PS3 controller to try to repro with. If you’re willing to grant me
some time, I’m willing to track down and order one on eBay or something so I can be sure PS3 controllers work just as well as anything else.

Let's see how PS3 works now, I'll test the devices that I have.

1. (BT) I paired a few BT devices, seems to be fine - but I noticed there's no prompt for the connection mode. Is this intentional or the display of the connection mode/PIN is determined dynamically ?

Yes, it’s intentional. That dialog isn’t for “connection mode”, it’s for a developer-internal concept (bluez pairing agent capability), which should never need to be shown to the user. Most users have no clue what they’re being asked or what the implications of the choice are anyway. With my changes, it is automatically determined based on whether the user has a keyboard, a gamepad, or no input device already attached.

I'm not an expert in this field, but most OS-es have similar prompts when pairing a device, so it's not something that a user hasn't encountered before. Technically though, aren't we excluding some devices from pairing - i.e. keyboard needing a PIN or some gamepads needing a confirmation ?

To be honest though, the 'UX' seems largely unchanged.

That’s really not true. The current UX (which includes the reliability — or lack thereof — of the back end pairing script) is broken in a number of ways. Just trying to successfully scan for and pair a single device (especially something requiring a PIN or passkey) is a major exercise in frustration today. You can see a detailed description of the problems in my first PR on this topic (which has been closed).

I'm looking at the module as a whole when I'm saying that seems largely unchanged.
The largest change is the back-end scripts for scan/pair - as you mentioned - and this seems to make scanning more reliable (i.e. no need re-scan if device doesn't show). But the scanning and pairing still take a long time, just as before.

[c0d3h4x0r]

Finding a genuine SONY dualshock3 sixaxis controller is difficult, but I think I managed to find a real one on eBay. It should arrive in about a week.

[cmitu]

Let's see how PS3 works now, I'll test the devices that I have.

Pairing seems to work fine - tested both controllers and an old 8Bitdo SN30. I removed the existing PS3 paired devices and the issue of them appearing as online during scanning doesn't seem to occur.

[c0d3h4x0r]

After re-pairing your PS3 controllers and turning them both off, does a subsequent pairing scan still show them as devices available to pair? If so, that’s going to be a bug in either the bt-device tool or bluez, because this bluetooth.sh just surfaces and displays what they report.

[cmitu]

After re-pairing your PS3 controllers and turning them both off, does a subsequent pairing scan still show them as devices available to pair?

No, it doesn't - I mentioned that. But I intend to re-test by pairing it via the current version and then running again with the PR pairing. I haven't looked at the Python scripts, Is there a way to turn on debugging and see more info ? That might shed some light.

[cmitu]

OK, re-did the PS3 connection tests and I think the issue comes from the fact that it's not listed as Paired.
With a connected and paired controller, here's how bt-device reports the controller:

bt-device -i E0:75:0A:A0:92:6C

[E0:75:0A:A0:92:6C]
  Name: Sony PLAYSTATION(R)3 Controller
  Alias: Sony PLAYSTATION(R)3 Controller [rw]
  Address: E0:75:0A:A0:92:6C
  Icon: input-gaming
  Class: 0x508
  Paired: 0
  Trusted: 1 [rw]
  Blocked: 0 [rw]
  Connected: 1
  UUIDs: [HumanInterfaceDevice]

Note that that it appears Trusted/Connected, but not Paired. The controller works fine like this (EmulationStation and games). As a result, the controller doesn't show on the list of Paired controller or in the Removal dialog and I think that the fact that's not Trusted makes it appear on the scan result.

[c0d3h4x0r]

The PS3 controller I ordered is scheduled to arrive on July 18, so I'll be able to try reproducing it then. The displayed Name value indicates your controller should be genuine.

[c0d3h4x0r]

As for your question about debugging info: yes, you can get helpful debug spew by passing the -d or --debug parameter to pair-device. You might want to try manually launching that script with that flag to perform the pairing and see if the debug output reveals anything interesting.

[c0d3h4x0r]

Got a notification just now that my PS3 controller has been delivered ahead of schedule. I’ll check it out and report back in the next day or two.

[c0d3h4x0r]

Got the controller, verified it’s genuine. Installed the sixaxis driver and tried pairing it. All the text prompts specific to PS3 controller pairing were displayed as expected. Bluetooth configurator claimed pairing was successful, but the controller’s four flashing LEDs said otherwise, so I tried again. Pairing was actually successful (according to the controller) on the second attempt, and I can use the controller to operate the menus afterward. However, the controller (even while paired and connected) does not appear in the “Display all paired Bluetooth devices” list.

I dropped to bash, ran bt-device --info, and see this:

Paired: 0

So it seems I’m reproducing the same issue on my end. I’ll investigate.

[c0d3h4x0r]

The problem appears to be with bluez itself. If I manually run bluetoothctl with scan on, and I then unplug the controller, hit the PS button, and replug it, bluetoothctl shows both of these lines upon replugging:

[NEW] Device 28:A1:83:43:BB:3C Sony PLAYSTATION(R)3 Controller
[DEL] Device 28:A1:83:43:BB:3C Sony PLAYSTATION(R)3 Controller

Because the device is “deleted” automatically (from bluez’s perspective) immediately after being found, all subsequent commands to try to pair, connect, or trust the device fail, because the device is not recognized by bluez as a device available for pairing.

I’m not sure how I managed to get the device to pair at all on my second attempt, but it seems more like a fortuitous accident than a reliably successful outcome.

[c0d3h4x0r]

Looks like the issue I was observing was caused by my having “enabled support for third-party controllers” during the installation of sixaxis. It’s non-obvious that enabling support for third-party controllers would break the ability to reliably pair OEM controllers, but apparently that is the case.

[c0d3h4x0r]

Alright, I think I see how my changes have broken this odd scenario. I’m working on a fix.

[c0d3h4x0r]

Very bizarre behavior and properties reported by the customhidsony driver that underlies the sixaxis package. Apparently, it never reports the controller as paired, even when it is. And if you attempt to initiate pairing to it, bluez will prompt the user for a PIN, which doesn’t make any sense because PS3 controllers do not require a PIN. I’ve tried supplying the usual default PINs (0000 an 1234) to it, but neither one appears to result in the device being reported as “Paired”. This looks to me like incorrect behavior of the customhidsony driver.

I’ll dig into that driver a bit to see what I can find, but if it’s not something that can be easily fixed or patched on the fly by way of using retropie_setup.sh, then I may just have to implement some special-case hacks around its peculiar behavior.

[c0d3h4x0r]

No simple change to customhidsony will fix the “Paired” property reporting afaict, so I’ve instead committed a few workarounds here to get PS3 controller pairing and display mostly working again.

However, I think I’m seeing an opportunity here to eliminate all the custom Python code and just use bluetoothctl itself to perform the actual pairing operation, and revert back to using bt-device for connecting and trusting. That would enable PS3 controller pairing to work more like it did before, and would greatly simplify the proposed changes. Give me a few days to implement a revised approach.

[joolswills]

Just to warn you this isn't going to be accepted. I am sorry, but want to make this clear before more time is spent.

I appreciate you have put time into this, but you started work on this without any initial discussion or agreement and you also said:

I think there's been value (both for me in getting acclimated to your coding conventions and preferred working style, and for you in having a number of real issues brought to your attention and one working fix approach to them put in front of you) in pursuing this, so I really don't resent the time investment

so I hope you are not too upset by this.

I'm also unsure why you would want to continue this (at least here), after your comment on the other PR of "Good luck with your bug farm". Unfortunately I don't feel this will ultimately work out sorry.

[c0d3h4x0r]

Alright then — I didn’t want to have to maintain my own public fork, but since that’s what it’s going to take, that’s what I’ll do.