Trips do not detect trip end even after stopping #567
Comments
|
Haven't received logs from Ryan yet, but do have logs from Andy. There are basically only sporadic updates to the location during this period, with the large gaps indicated. |
|
This behavior seems to be consistent with the app running in the background, without a foreground service
But why is the foreground service workaround not working? Could this be related to #571? |
|
This does in fact seem to be related to #571. In the case where the trip doesn't work, we have equal numbers of foreground service start and destroy. But in the case where it did work, we had one extra start So in the second case, we must have had a persistent foreground service, while in the first case, we would not. It looks like the approach of starting a foreground service for each callback (which was arguably lame in the first place) is no longer working. |
|
Wait, if we look at the counts, they are not that different |
|
ok, so I don't fully understand what is going on here, but after the initial startup, the start and destroy clearly occur in pairs. And at the beginning, there seems to be one extra start from a location service than a destroy |
|
Ah there was indeed a re-entrant start, and a single destroy |
|
But we see the same behavior in the failure case, although it happens much after the tracking. In fact, there are 3 starts. Why don't we see the same frequent updates in that case? |
Not sure. But we are going to change this anyway, so let's try to change to a single, persistent foreground service and see if that fixes the problem. |
|
What we do now is to start an intermittent foreground service every time we want to do anything in the background. This has us starting not just So there are two ways in which we can start a longer-lived foreground service:
The second option seems more robust since we will always have the foreground service running and will avoid missing foreground issues like the ones in this bug. In Android 11 (API 30, there are also going to be additional restrictions on foreground services although we do have the Let's try the second option and switch to the first option only if we need to. |
|
Hm, android doesn't like us to use foreground services for passive location tracking. But we can't get frequent updates unless we do. Maybe we should try the first option first though. |
|
Couple of background notes before we start the refactor:
Instead, we call the We started this foreground pattern because without it, background services don't start Due to restrictions on
on background apps launching background services
|
|
As a quick check on where this was called before, in at least one case, it was due to a restart in the ongoing state |
|
In the other cases, it seems like we do call if from So the current service lifecycles are:
|
|
Right now, we have the foreground service started from the FSM. This is very principled, but incorrect, because we need to perform some operations before the FSM is launched, at the start of the trip. One option is to launch the foreground service first and use it to launch the GeofenceService. In this case, the service lifecycle would be:
|
|
I think we can also make an argument for Option 1 since we are tracking the location, albeit passively. And we can set it up so that users can click the ongoing service notification to start and stop trips manually, which would be useful, right? |
|
Let's check with some users and make a decision. |
|
For the record, the lifecycle for option 1 would be:
Everything else works as usual. Expose a service interface and allow the FSM to bind to it for status updates and icon changes. |
|
Instant feedback from users:
So going to start with Option 1 given time budget. Once we have a long-lived service working reliably, should be possible to shorten its duration later. |
|
Final question: how do we send message updates to the service so that it shows something meaningful? We could restart it multiple times with multiple intents, but that seems a bit lame. Instead, we can expose a binding (e.g. https://stackoverflow.com/questions/3514287/android-service-startservice-and-bindservice |
|
To simplify this, and only show the foreground service when we are actively tracking, we can use a BroadcastReceiver for the |
|
Turns out that even if we turn the service off as part of "Stop tracking", we restart it as part of the re-binding in |
|
Cleaned up the code around binding a bit so that we bind on create and unbind on destroy. This ensures that we don't restart the service all the time, and fixes the issue where the state is not reflected properly. One question that I still have with the debugging is why the Let's spend a really short time to review. |
|
Ok, this is why: and then as part of setting the new state, we check the permissions and settings, which resets the FSM in start state We can't fix that without changing the meaning of initialize in the FSM, and I don't plan to right now. start/stop tracking is not a common use case anyway. |
|
Tested downgrading location permissions. This crashes the service, and does not launch the geofence service to generate a tracking error. If we can detect that the |
|
Detected using intent which is |
|
Turning off location services completely doesn't do anything automatically. In particular, I don't seem to get a |
|
We are definitely not getting a |
|
Turned off on M's phone at 1:52pm. |
|
And this is not a background v/s foreground issue because |
|
At any rate, this is not a regression from v0.0.1, so let's go on. Still not quite sure how/why this worked on Stan's phone but not even on the older version of emTripLog. I wonder if I can file an issue against GooglePlayServices for this code. |
|
Retesting on emTripLog: everything valid at 2:18. |
|
However, testing on a slightly older version of android (with the same APK) does work.
|
|
There might be a race when we are in the start state, we get an initialize, and we are not able to create a geofence (for whatever reason). Need to experiment further. |
|
I can sort of reproduce this by:
Once we have automated tests, we should have some codemonkey stuff around this to see if we can reproduce. |
|
This seems to be the bug for weird FSM issues. And here's a new one. Ryan turned his tracking off And then the app got updated, which generated an initialize. The initialize transition generated a tracking error, and then we got stuck in an infinite loop of tracking errors. If we are in |
If we are stopping all the location tracking anyway, then do we care if there are problems with the location? No, we don't. Because if we do, we run into an infinite loop if we are in the tracking_stopped state and we turn location services off. Note that this is a perfectly valid case which should not generate an error. But because we recheck, we generate a tracking error, which tries to get us to stop everything, which then rechecks and generates a tracking error.... ad infinitum. e-mission/e-mission-docs#567 (comment)
Reported by Ryan
and Andy
The text was updated successfully, but these errors were encountered: