Closed ewaldc closed 1 year ago
Hi @ewaldc - thanks so much 😀. I think I ran into the same issues while testing, and fixed it in the main branch. I just updated the filters-previews-tests
branch & sync'd it with what's in main. After a bit of testing, it doesn't look like I broke anything 🤞. Please let me know how it goes! If all is well, I will close this issue. Thanks again...
Hello @santansarah, that is a fix at the speed of light! I managed to fix it in a different way, but your change is simpler (even though I don't understand it yet 😟). I will test it and let you know.
On the A52S of my spouse, I also ran into a similar issue with the Google Play version. Somehow the bluetooth service is not able to turn on the Bluetooth adapter ("endless" loop requesting permission to turn on Bluetooth eventually ending in similar crash). After killing the app (or waiting for it to crash), it then takes about a minute to turn Bluetooth on via Settings. A reboot fixes it. Need to test with some other apps. Samsung has a special capability for BLE devices to auto-turn-on Bluetooth (e.g. Samsung Buds) when the device is nearby (in the case of the buds, the holder also must be opened). It's a nice feature but probably difficult to implement without synchronization issues. So probably a defect in the Samsung Android 13 ROM... Lots of folks seems to have BLE issues since the Android 13 update of their Samsung phones...
Hey @ewaldc - it was only fast b/c I already had it done; I remember spending some time on that re-write a while back 😀, and LOTS of testing.
Interesting about the A52S....at the very least, I should probably put in some sort of safety net/max attempts flag so it won't loop like that. I'll also check out Crashlytics & see if I can get any info there.
Hello @santansarah, Sorry for the delay, had to sit a few times on my knees and come up with a good story to borrow my spouse phone again..
The fix works: no more crashes. For my app, this soluton does not work though as the scan screen is not the main screen. So the permission must be gathered by the BLEObserver. I leveraged the (unused) PermissionManager code that was already in your code :smile:
Once again, thanks for the great videos and education on best-in-class, modern Android app architecture. Android development can be hugely confusing and overwhelming, even for an experienced programmer. It seems there is a lack of leadership and clear view on Android software development architecture, resulting in (too) many choices, opinions, promotion of overly complex architectural approaches (e..g. Dagger) etc.
It was refreshing to see someone who has such a clear view on the architecture choices and who explains it all well in a structured way. You have great talent!
PS. The bluetooth turn on issue remains on the A52s. I noticed Samsung released another June update, just days after the previous one.
Logcat trace during the endless permission request loop:
D/InputMethodManager: startInputInner - Id : 0
I/InputMethodManager: startInputInner - mService.startInputOrWindowGainedFocus
I/ViewRootImpl@47c3c7a[MainActivity]: MSG_WINDOW_FOCUS_CHANGED 0 0
D/BluetoothAdapter: onBluetoothServiceUp: android.bluetooth.IBluetooth$Stub$Proxy@7b34593
D/BluetoothAdapter: onBluetoothServiceDown
D/BluetoothAdapter: onBluetoothServiceUp: android.bluetooth.IBluetooth$Stub$Proxy@93999d0
D/BluetoothAdapter: onBluetoothServiceDown
D/BluetoothAdapter: onBluetoothServiceUp: android.bluetooth.IBluetooth$Stub$Proxy@1440dc9
D/BluetoothAdapter: onBluetoothServiceDown
D/BluetoothAdapter: onBluetoothServiceUp: android.bluetooth.IBluetooth$Stub$Proxy@8a34cce
D/BluetoothAdapter: onBluetoothServiceDown
D/InputTransport: Input channel destroyed: 'ClientS', fd=118
D/BluetoothAdapter: onBluetoothServiceUp: android.bluetooth.IBluetooth$Stub$Proxy@442a7ef
D/BluetoothAdapter: onBluetoothServiceDown
D/BluetoothAdapter: onBluetoothServiceUp: android.bluetooth.IBluetooth$Stub$Proxy@16682fc
D/BluetoothAdapter: onBluetoothServiceDown
I/ViewRootImpl@47c3c7a[MainActivity]: stopped(false) old = false
D/BleObserver: onResume called...
During this time the BLE system indicator stays gray, meaning the OS does not consider that BLE is on. Even by hand I can not turn it on. Once rebooted, the problem is fixed except when I turn BLE on before the system is fully initialized (all apps optimized): then the phone just turns off!
No need to waste time on this issue...
Hi @ewaldc , thanks so much, I really appreciate it! That's awesome that you used the unused PermissionManager - nice 🥳. A quick Google search seems to come up with all sorts of Bluetooth issues for the A52s - thank you for bringing it to my attention; it's good to know for future troubleshooting. Best of luck & fun with all of your projects and coding adventures 🎸.
Hi @santansarah, I know I closed the issue because there is no more crash and my original problem is solved.
But just FYI, I tested 3 other BLE apps on the A52 and A52S (e.g. Nordic Semi nRF Connect) including some (messy) self-written BLE code and they don't cause the endless permission request loop, nor do any BLE turn-on issues exist when I stop these apps. afterwards.
I think it may be related to this accompanist issue. As I was reading and testing that accompanist code, there seems something not right. Will open a new issue, once I get a clearer view on things.
Hey @ewaldc,
OK - I tested this morning on my Android 13/Pixel with no issues - I tried to deny
several times, then allow, etc. I even tried it fresh from the Google Play Store too; it seems to work OK when I enable/disable Bluetooth. If I have time today, I'll run some more tests too. It does sound like it could be an accompanist issue...thanks so much for investigating!
Hi @santansarah, My tests this morning seem to indicate the the BLE issues on the A52 and A52s are indeed related to the Accompanist permission code as per the issue mentioned above.
I substituted my permissionManager code for the Accompanist code and it now works fine for my app on the A52. I tried to quickly port it to the BleScanner app and although it works well for the BleObserver (request permissiom, automatically start scanning (again) when adapter is enabled etc.), it requires more code changes in the home*.kt (and maybe ScanViewModel). Although the isScanning StateFlow is correctly updated the Scan UI keeps saying it's not scanning. Probably it still needs the removal of all Accompanist code form the composable(s) altogether...
Since it's not fully working, it makes no sense to publish on Github. Hence I have attached the permissionManager code used and the modified BleObserver FYI. While I left the LifecycleObserver code in there, it does not seem to be helpful. In fact, I noticed a sequence of pause/resume cycles when the user is slow at granting permission. Hence, it makes no sense to release the broadcastreceiver during those early permission request cycles. I think it is possible though to release the broadcast receiver once all permissions have been granted but I have not added that in the uploaded code.
The Accompanist issue is IMHO caused by the fact that architecturally permission requests should not be made in composables (who can recompose at any time and in parallel) but in e.g. a viewmodel. In practice, the Compose state objects are not thread-safe. Even when using Snapshort.withMutableSnapshot, compose can crash as I learned the hard way.
Hi @ewaldc OK - very interesting. I wonder if you added a bit of debounce/delay somewhere in the process, if that would help the issue? Either way, it sounds like your solution is much more stable. This will definitely change how/if I use accompanist permissions in the future! Thanks so much for all of your hard work troubleshooting this issue - I really appreciate it 🎉👍 Sarah
While my project is leveraging BleScanner, it is not a scanning app and thus the scan screen is not the first screen. On the other hand, the BLEObserver and (filtered) scan for the users and other (FTMS) fitness devices are the amongst the first threads/coroutines that start. However, since there is no (initial) scan UI (Scan UI is rather a debugging aid), I needed to replace the Accompanist Permission code (as it only runs in a Composable) to gain BLE permissions.
I found the two issues (crash dump and endless permission loop) rather by accident: I wasn't getting some GATT notifications on my spouse phone (the A52s) and was wondering if I broke your GATT code somewhere. So I decided to test it on the "ble-scanner-filters-preview" branch to see if the problem was manifesting itself there as well, but never managed to get that far...
I scanned the Accompanist code again today and did not find anything obviously wrong. So one might be fortunate with a delay. On the other hand, the onpause/onresume lifecycle states during permission request may contribute to the problem, although this is apparently a known (but unknown to me) behaviour since API 23 (see stackoverflow). Personally though, I moved all non UI code out of composables and execute all coroutines/threads/business logic in the viewmodel(s) and many "mysterious" compose related crashes disappeared.
For permissions finally, the consensus seems to be: request them in onCreate()
of viewmodel/activity/fragment (see also here)
Ewald
First of all: many thanks for this great library and your videos. As a Linux kernel developer, I am a total newbie wrt Kotlin/Android/Compose/Koin etc. and have learned a lot from your architectural approach. I am creating a Zwift-like (but open source) fitness app (FTMS) and was looking to leverage some of your BLE and scan UI code.
I installed the "ble-scanner-filters-preview" branch for testing but it (occasionally) crashes at (fresh) start-up:
Since it does not always happen, my first guess is a race condition (one of the few things kernel developers are good in :-) ) between the
BleObserver
(launch of BLE enable intent) andHome.kt
where all the correct permissions are requested. I think in Android 12+ theBleObserver
must request permissions first when the Bluetooth adapter is not enabled and wants to turn it on. Probably just a small code change to fix this as all the supporting code is already there...Ewald