Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Keyboard] annepro2: fix bluetooth multi-pairing issue #17483

Merged
merged 1 commit into from
Nov 19, 2022

Conversation

coffeeyy
Copy link
Contributor

@coffeeyy coffeeyy commented Jun 26, 2022

This change fixes the bluetooth multi-pairing issue.

Description

The bluetooth broadcasting and connection functions in the annepro2_ble.c are incorrect, they do not handle the multi-pairing correctly.

The exiting broadcasting command (12 bytes in total) is the combination of the 11 bytes length machine code (which is stated in the array "ble_mcu_start_broadcast[11]") and the 1 byte length value (which is stated in the variable "port" - also known as the Broadcast ID) :

[              ble_mcu_start_broadcast (11 bytes)                 ] + [Broadcast ID 0-3 (1 byte)]
 0x7b, 0x12, 0x53, 0x00, 0x03, 0x00, 0x00, 0x7d, 0x40, 0x01, 0x00,            0x00-0x03

However, with above command, the annepro2 keyboard is always broadcasting the "ID 0" whatever we change the Broadcast ID to any value.

The problem is that the eleventh byte of the command is actually for describing the Broadcast ID, that's why the exiting code is always broadcasting the "ID 0" which makes the keyboard unable to pair with other devices with ID 1-3.

To resolve the problem, we can just simply modify the command to:

[            ble_mcu_start_broadcast (10 bytes)             ] + [Broadcast ID 0-3 (1 byte)] + [ 0x00 (1 byte)]
 0x7b, 0x12, 0x53, 0x00, 0x03, 0x00, 0x00, 0x7d, 0x40, 0x01,            0x00-0x03,                  0x00

The bluetooth connection command has the same issue, so the fix has also covered that issue in this Push Request.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

This change fixes the bluetooth multi-pairing issue.

** Description **

The bluetooth broadcasting and connection functions in the annepro2_ble.c are incorrect, they do not handle the multi-pairing correctly.

The exiting broadcasting command (12 bytes in total) is the combination of the 11 bytes length machine code (which is stated in the arrary "ble_mcu_start_broadcast[11]") and the 1 byte length value (which is stated in the variable "port" - also known as the Broadcast ID) :

[              ble_mcu_start_broadcast (11 bytes)                 ] + [Broadcast ID 0-3 (1 byte)]
 0x7b, 0x12, 0x53, 0x00, 0x03, 0x00, 0x00, 0x7d, 0x40, 0x01, 0x00,            0x00-0x03

However, with above command, the annepro2 keyboard is always broadcasting the "ID 0" whatever we change the Broadcast ID to any value.

The problem is that the eleventh byte of the command is actually for describing the Broadcast ID, that's why the exiting code is always broadcasting the "ID 0" which makes the keyboard unable to pair with other devices with ID 1-3.

To resolve the problem, we can just simply modify the command to:

[            ble_mcu_start_broadcast (10 bytes)             ] + [Broadcast ID 0-3 (1 byte)] + [ 0x00 (1 byte)]
 0x7b, 0x12, 0x53, 0x00, 0x03, 0x00, 0x00, 0x7d, 0x40, 0x01,            0x00-0x03,                  0x00

The bluetooth connection command has the same issue, so the fix has also covered that issue in this Push Request.
@tzarc
Copy link
Member

tzarc commented Jun 28, 2022

My annepro2 is in storage while I prepare to move house -- @bwisn / @Jpe230 can you confirm that this is indeed a fix, please?

@tzarc tzarc added the bug label Jun 28, 2022
@Dr-Ongo
Copy link

Dr-Ongo commented Jul 27, 2022

I've found this to be a major improvement. Switching between paired devices works as expected, but I have had to connect via USB before the keyboard will connect to any device over BT. Afterwards the keyboard can be unplugged and it continues to switch between devices

Edit: I was mistaken. Connecting a USB cable not necessary.

@mrhrzg
Copy link

mrhrzg commented Aug 2, 2022

This works for me as well. Thanks coffeeyy!
After a successful initial test, re-connecting and device switching still has issues.

@drashna drashna requested a review from a team August 3, 2022 00:12
@tzarc
Copy link
Member

tzarc commented Aug 5, 2022

For some reason my AP2 won't pair even with normal master branch, let alone this PR.

@Jpe230
Copy link
Contributor

Jpe230 commented Aug 5, 2022

For some reason my AP2 won't pair even with normal master branch, let alone this PR.

Try pressing the KC_AP2_BTX key for a couple of seconds... I don't know why it works but it works

@tzarc
Copy link
Member

tzarc commented Sep 5, 2022

Try pressing the KC_AP2_BTX key for a couple of seconds... I don't know why it works but it works

Nope, no difference, nor would I expect there to be considering the pairing is initiated through the serial link.

@thomazmoura
Copy link

Try pressing the KC_AP2_BTX key for a couple of seconds... I don't know why it works but it works

Nope, no difference, nor would I expect there to be considering the pairing is initiated through the serial link.

I've tried out cherry-picking this commit on my fork and it fixes it - but that said I usually have through some hoops to make bluetooth on any firmware (ObinsKit included). Usually the rain dances steps which seem to be more effective are:

  • Resetting bluetooth pairing on the board (either through Obinskit or through the KC_AP2_BT_UNPAIR on QMK) and forgetting the device anywhere it was paired before;
  • Change the bluetooth firmware version with Obinskit (when you flash QMK it keeps the last bluetooth firmware flashed from ObinsKit, so you should check the bluetooth works on the Obinskit version before actually flashing QMK - different versions tend to work better or worse with different devices and bluetooth versions - the last stable version is working fine for me on all my devices);
  • Turning off the board (and keeping it unplugged for at least 10 seconds;
  • Switching it on and off a few times quickly.

So far my experience with bluetooth with the QMK firmware and the fix on this PR has been as good as the official Obinskit firmware: works wonderfully after a lot of trial and error.

@bombonatti
Copy link

Just got AP2 yesterday, already downloaded QMK, doing few tests and realized that BT was not working as was on ObinsKit firmware.

Found this PR and I got my BT working again (BT1 on Linux, BT2 on Windows, BT3 on Android) I may easily swap between devices.

@drashna drashna requested a review from a team November 19, 2022 20:01
Copy link
Member

@tzarc tzarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't prove that it works given my own AP2, but going off others' reports that it works.

@tzarc tzarc merged commit 37c2714 into qmk:master Nov 19, 2022
thystips pushed a commit to thystips/qmk_firmware that referenced this pull request Nov 24, 2022
ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
elpekenin pushed a commit to elpekenin/qmk_firmware that referenced this pull request Dec 7, 2022
crembz pushed a commit to crembz/qmk_firmware that referenced this pull request Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants