barbeau / gpstest

The #1 open-source Android GNSS/GPS test program
Apache License 2.0
1.69k stars 358 forks source link

GPSTest can leave GNSS receiver active on back-button (when not configured to do so) #683

Open gdt opened 3 months ago

gdt commented 3 months ago

Describe the bug Starting and exiting GPSTest quickly can result in the GPS receiver being left on. What I view as the normal mode is to start the receiver when launching the program and to turn it off on exit.

To Reproduce First, set configuration:

  1. "run gnss in background" is unchecked
  2. 'force full gnss measurements" is checked
  3. interval is 1
  4. auto-start gnss is checked

Steps to reproduce the behavior:

  1. start with the app never having been started, probably
  2. possibly, have run Network Survey, and perhaps even have it running in background
  3. push the app icon in the launcher
  4. as soon as the screen starts to change, push the back button
  5. observe the "n sats, etc" GPSTest notification.

Expected behavior System is as before - GPS receiver not running, no excess battery drain.

Observed behavior I saw the notification, and pulling down the shade showed few sats received. After 10s of seconds, maybe 30, I saw a fix.

This has happened to me multiple times, entirely by accident. Basically I wanted to start the app that was in the same position in launcher page 1, whereas GPSTest is in page 2.

App, Device and Android version:

Speculation

I suspect this is a threading/sync hazard. It smells like firing off a subscribe-to-GPS call, and then on back button checking if GPS is on and not cancelling it because it's off and then the 'ok you are subscribed" call arriving. If so needs separate variable for desired/not-desired and state transition to on needs to check that and stop. But I'm totally making this up.

barbeau commented 3 months ago

Thanks for the report!

Can anyone reproduce this with a stock Android device (not running a custom ROM)?

gdt commented 3 months ago

I hope you ask that question about any particular ROM :-) It's certainly fair to question if this is a ROM bug, but that doesn't seem particularly more likely with custom vs stock-foo vs stock-bar. FWIW, I haven't been hearing anything remotely like this about anything on Calyx. All the related gripes are "this program that uses proprietary APIs isn't working", meaning Google Play location services vs the AOSP APIs.

And of course, if anyone else can reproduce, that's great. I was not able to do so this morning when writing the bug report, but I have seen it actually happen twice. Seems pretty obviously timing sensitive.

I'm pretty sure it isn't a ROM issue, though, because the notification is showing entirely reasonable data, so GPSTest is receiving updates, and knows it is in background. I should have mentioned that I think (only 98%) that I then re-opened GPSTest, waited a few seconds, and exited, and that led to the notification going away.

I'll try to have a look at the code.

barbeau commented 3 months ago

Yes, standard ROM question :).

If you see this again, are you able to dismiss the notification by tapping on the "Stop" button on the notification?

gdt commented 3 months ago

Thanks. Just pointing out that "stock" is N things , not 1 thing.

WIll check on dismissing; that's a good test.

gdt commented 3 months ago

I read code some. It looks like my speculation above might actually be right. The start triggers an intent to arrive and if the exit from the UI happens before it arrives, there is nothing to check that the main UI has exited. See ForegroundOnlyLocationService.kt:subscribeToLocationUpdates and the comment in it.

mikeglee commented 3 weeks ago

I don't see it as reasonable to assume a third-party ROM would behave as stable as the stock Android from Google. It is difficult enough to troubleshoot stock Android issues. I'm not experiencing any similar issues on Galaxy S10 Android 11.

gdt commented 3 weeks ago

I don't see it was reasonable to assume anything in particular about any code being stable. Dismissing Calyx as flaky in particular seems unwarranted.

Did you see the comment above where I found the bug by code reading? Did you read the code? Do you see what I mean about lack of synchronization?