Refactored DiscoveryActivity #1927

Merged
ashimokawa merged 13 commits from TaaviE/Gadgetbridge:patch2 into master 1 year ago
TaaviE commented 1 year ago

This PR contains:

  • CompanionDeviceManager support - Fewer background restrictions if the bonding flow is completed (tested with iTag, requires Android 8+ and an option turned on)
  • Target 29 support - now FINE location is required
  • Increased scan speed - assumption is that people pair with nearby devices, lowering the threshold beacon count increases speed when results appear
  • Removed unnecessary BLE filter - GB itself filters devices anyways
  • Increased result count - scan all PHYs on newer Android, because there's no good reason why not to
  • iTag improvements - now gives a set of filters and clarifies bonding type, should either be required
  • Scan improvements - Now BT and BLE scans are ran sequentially
  • DiscoveryActivity UI is clearer - it's displayed which scan is running, flaws are shown sooner
  • Fixed bug when location wasn't enabled or in some way not granted
  • Fixed a few scan state machine bugs when things time out
  • ...and probably a few other minor changes
This PR contains: * CompanionDeviceManager support - Fewer background restrictions if the bonding flow is completed (tested with iTag, requires Android 8+ and an option turned on) * Target 29 support - now `FINE` location is required * Increased scan speed - assumption is that people pair with nearby devices, lowering the threshold beacon count increases speed when results appear * Removed unnecessary BLE filter - GB itself filters devices anyways * Increased result count - scan all PHYs on newer Android, because there's no good reason why not to * iTag improvements - now gives a set of filters and clarifies bonding type, should either be required * Scan improvements - Now BT and BLE scans are ran sequentially * DiscoveryActivity UI is clearer - it's displayed which scan is running, flaws are shown sooner * Fixed bug when location wasn't enabled or in some way not granted * Fixed a few scan state machine bugs when things time out * ...and probably a few other minor changes
Owner

@TaaviE

Cool.
The current activity is total crap, so any change can only make it beter.

Is target 29 required?

And is here anyone who can still test kitkat?
I do have one device with kitkat but it lacks ble :/

@TaaviE Cool. The current activity is total crap, so any change can only make it beter. Is target 29 required? And is here anyone who can still test kitkat? I do have one device with kitkat but it lacks ble :/
Poster

Is target 29 required?

In the near future? Yes.

> Is target 29 required? In the near future? Yes.
Owner

@TaaviE

Is target 29 required?

In the near future? Yes.

Is it required for the code or for google play?

@TaaviE >> Is target 29 required? > >In the near future? Yes. Is it required for the code or for google play?
Poster

@ashimokawa

Is it required for the code or for google play?

Yes and no to both aspects.

There are changes that are only required when targeting 29, but nothing that won't work on the lower target. Neither is 29 required for Google Play, current requirement is target 28 for new updates. However they will require 29 after 2. November, 2020.

@ashimokawa > Is it required for the code or for google play? Yes and no to both aspects. There are changes that are only required when targeting 29, but nothing that won't work on the lower target. Neither is 29 required for Google Play, current requirement is target 28 for new updates. However they will require 29 after 2. November, 2020.
Owner

@TaaviE

I see. So we would require the user to accept fine location to pair new devices when we target 29 instead of coarse location?

Apart from that question would you consider this PR ready to merge? ;)

@TaaviE I see. So we would require the user to accept fine location to pair new devices when we target 29 instead of coarse location? Apart from that question would you consider this PR ready to merge? ;)
Poster

On installation, yes, it'll display both location permissions. The permission promt dialog doesn't change I think.

I would love people to test on older Android versions, that scanning works for them. I did try to make sure it does work but it's Android's Bluetooth and it might surprise.

On installation, yes, it'll display both location permissions. The permission promt dialog doesn't change I think. I would love people to test on older Android versions, that scanning works for them. I did try to make sure it does work but it's Android's Bluetooth and it might surprise.
Poster

@ashimokawa

I did ask on the Element chat for people to give this PR a test drive but there weren't any volunteers, maybe you have had the chance?

@ashimokawa I did ask on the Element chat for people to give this PR a test drive but there weren't any volunteers, maybe you have had the chance?
Owner

@TaaviE

I asked someone with api 19, will report back as soon as I hear from him!

@TaaviE I asked someone with api 19, will report back as soon as I hear from him!
Poster

@ashimokawa

LjL on the Matrix chat tested the changes out on Android 7, and it worked there. So below the required API level for CompanionDeviceManager works.

@ashimokawa LjL on the Matrix chat tested the changes out on Android 7, and it worked there. So below the required API level for CompanionDeviceManager works.
Poster

@ashimokawa

I pushed a commit that should fix the issue you mentioned.

@ashimokawa I pushed a commit that should fix the issue you mentioned.
Poster

odavo32nof tested the PR out on KK and it didn't crash, and it did find devices.

But he also found out that there seems to be some sort of an issue in the flow of Mi Band pairing on KK, with or without the PR changes. They're investigating it.

odavo32nof tested the PR out on KK and it didn't crash, and it did find devices. But he also found out that there seems to be some sort of an issue in the flow of Mi Band pairing on KK, with or without the PR changes. They're investigating it.
Owner

@TaaviE

Thanks!
My plan is to try this out on two devices here and if it works merge and release 0.46.0 with this on saturday/sunday

@TaaviE Thanks! My plan is to try this out on two devices here and if it works merge and release 0.46.0 with this on saturday/sunday
Owner

@TaaviE

Unfortunately after building Gadgetbridge with this PR, Gadgetbridge no longer finds anything.
(Amazfit Bip, Mi Band 4, Fossil Hybrid HR and more are here on my desk)

@TaaviE Unfortunately after building Gadgetbridge with this PR, Gadgetbridge no longer finds anything. (Amazfit Bip, Mi Band 4, Fossil Hybrid HR and more are here on my desk)
Poster

That's suspicious, no errors in logcat or anything like that?

Just checking a theory, did you grant GB "always" location access or "only while in use"?

That's suspicious, no errors in logcat or anything like that? Just checking a theory, did you grant GB "always" location access or "only while in use"?
Poster

It's suspicious primarily because I really really can't see how that's possible, users have confirmed scanning does work on Android 4.4 (old BLE, old pairing), 5.0 (new* BLE, old pairing) and 10 (new BLE, new pairing).

It's suspicious primarily because I really really can't see how that's possible, users have confirmed scanning _does_ work on Android 4.4 (old BLE, old pairing), 5.0 (new* BLE, old pairing) and 10 (new BLE, new pairing).
Owner

@TaaviE

Your theory seems correct, has to be "always" .... hmmm....

@TaaviE Your theory seems correct, has to be "always" .... hmmm....
Poster

@ashimokawa

I will try and see if it's possible to detect non-"always" grants or request only "Always"/"None".

@ashimokawa I will try and see if it's possible to detect non-"always" grants or request only "Always"/"None".
Owner

@TaaviE

Any idea why it has to be "Always", I mean Gadgetbridge is clearly running :/

@TaaviE Any idea why it has to be "Always", I mean Gadgetbridge is clearly running :/
Poster

@ashimokawa

I pushed a commit that should alleviate that problem. Let me know if you see anything else.

@ashimokawa I pushed a commit that should alleviate that problem. Let me know if you see anything else.
Owner

@TaaviE

Thats better :)

Another problem I found is that in most cases the scan skip the classic bluetooth scan and jumps right to the BLE scan.

In that case the button state is not "stop" but "start" even though it is scanning.

The scannig state was alway broken, even before your changes, I just assume it just looks more broken now since you display more, but it was as broken under the hood as it is with this PR.

@TaaviE Thats better :) Another problem I found is that in most cases the scan skip the classic bluetooth scan and jumps right to the BLE scan. In that case the button state is not "stop" but "start" even though it is scanning. The scannig state was alway broken, even before your changes, I just assume it just looks more broken now since you display more, but it was as broken under the hood as it is with this PR.
Poster

In that case the button state is not “stop” but “start” even though it is scanning.

I'll try to reproduce and fix it this Sunday.

> In that case the button state is not “stop” but “start” even though it is scanning. I'll try to reproduce and fix it this Sunday.
Poster

@ashimokawa I think I fixed it. At least no matter how I tap the button it seems to be correct 😅

@ashimokawa I think I fixed it. At least no matter how I tap the button it seems to be correct 😅
Owner

@TaaviE
Thanks!

Those strings.xml conflicts suck, maybe it would be better to add new strings not all at the same location. (I did not check if you did or not)

In the past I always tried to give strings.xml structure but other just added strings to the bottom, it is a bit messy now.

@TaaviE Thanks! Those strings.xml conflicts suck, maybe it would be better to add new strings not all at the same location. (I did not check if you did or not) In the past I always tried to give strings.xml structure but other just added strings to the bottom, it is a bit messy now.
Poster

In the past I always tried to give strings.xml structure but other just added strings to the bottom, it is a bit messy now.

I'll try to sort them out in a later PR. The strings need cleanup/rename anyways.

> In the past I always tried to give strings.xml structure but other just added strings to the bottom, it is a bit messy now. I'll try to sort them out in a later PR. The strings need cleanup/rename anyways.
Poster

@ashimokawa I fixed the conflict.

@ashimokawa I fixed the conflict.
Owner

@TaaviE

Thanks. Will test later today.

@TaaviE Thanks. Will test later today.
Owner

@TaaviE

This activity is too broken, I cant even describe it. The button state and scanning states are totally messed up, classic bluetooth scan is skipped, etc.

IMO it is not your fault, this was extremely broken before as I said, just more obvious now.

I think we should release without this today.

@TaaviE This activity is too broken, I cant even describe it. The button state and scanning states are totally messed up, classic bluetooth scan is skipped, etc. IMO it is not your fault, this was extremely broken before as I said, just more obvious now. I think we should release without this today.
Poster

classic bluetooth scan is skipped, etc.

That does not happen for me. It must mean that the scan finishes. Do you see anything in the logs that would hint why it finishes?

> classic bluetooth scan is skipped, etc. That does not happen for me. It must mean that the scan finishes. Do you see anything in the logs that would hint why it finishes?
Owner

@TaaviE
I sent you a video on the thing formally known as matrix.

@TaaviE I sent you a video on the thing formally known as matrix.
Owner
2020-08-02 20:14:15.489 10241-10241/nodomain.freeyourgadget.gadgetbridge I/nodomain.freeyourgadget.gadgetbridge.activities.DiscoveryActivity: Starting BT discovery
2020-08-02 20:14:15.496 10241-10241/nodomain.freeyourgadget.gadgetbridge E/nodomain.freeyourgadget.gadgetbridge.activities.DiscoveryActivity: Discovery starting failed

Then BLE discovery starts spamming the log with the same devices again and again. Takes ages then finishes. Button state is "start discovery" all the time during scan.

``` 2020-08-02 20:14:15.489 10241-10241/nodomain.freeyourgadget.gadgetbridge I/nodomain.freeyourgadget.gadgetbridge.activities.DiscoveryActivity: Starting BT discovery 2020-08-02 20:14:15.496 10241-10241/nodomain.freeyourgadget.gadgetbridge E/nodomain.freeyourgadget.gadgetbridge.activities.DiscoveryActivity: Discovery starting failed ``` Then BLE discovery starts spamming the log with the same devices again and again. Takes ages then finishes. Button state is "start discovery" all the time during scan.
Poster

@ashimokawa Hmm, no clues in the log why adapter.startDiscovery() would fail?

@ashimokawa Hmm, no clues in the log why `adapter.startDiscovery()` would fail?
Poster

I have a fix for the UI glitch, so that's nice. I'd like to handle the scan starting failure a bit more gracefully, did you deny a permission?

I have a fix for the UI glitch, so that's nice. I'd like to handle the scan starting failure a bit more gracefully, did you deny a permission?
Owner

@TaaviE

Ok, it works once after toggling Bluetooth.
Then if I stop the classic BT scan by pressing the button it is broken until I toggle Bluetooth again.

@TaaviE Ok, it works once after toggling Bluetooth. Then if I stop the classic BT scan by pressing the button it is broken until I toggle Bluetooth again.
Poster

I'm investigating it at the moment. I would really like to get it shipped.

I'm investigating it at the moment. I would really like to get it shipped.
Poster

@ashimokawa Can you tell if it works better now? I'm still trying to reproduce what you were seeing, but it's really weird.

@ashimokawa Can you tell if it works better now? I'm still trying to reproduce what you were seeing, but it's _really_ weird.
ashimokawa merged commit fb70a07f64 into master 1 year ago
TaaviE deleted branch patch2 1 year ago
The pull request has been merged as fb70a07f64.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.