Issue 508 - Update Location Services (v2) #1336
Conversation
heyjamesknight
commented
Aug 10, 2017
heyjamesknight
commented
- Added LocationClient
- Hides old Android/new Play Services implementations behind a shared Interface.
- Moved old Location code into AndroidLocationClient.
- Created new GoogleLocationClient which uses Play Service's FusedLocationProvider to provide Location updates.
- Updated Location Activities to use the new LocationClient
- GeoPointActivity, GeoPointMapActivity, GeoPointOsmMapActivity, GeoShapeGoogleMapActivity, GeoTraceGoogleMapActivity, GeoTraceOsmMapActivity were updated.
- GeoShapeOsmActivity was left using the legacy code, as the new LocationClient does not provide a way to determine if specific LocationProviders are available.
- Added Unit Tests for LocationClient implementations.
- Added Robolectric tests for Activities.
- Removed legacy InfoLogger from 2013.
|
Copying @lognaturel's comment from the previous PR:
|
|
@mmarciniak90 this one is going to be super fun to test because it has to be done outside! Also, for verifying GeoTrace, we're going to need to do some moving around so hopefully you can take the opportunity for a nice walk with a backpack full of phones. 😊 The goal here is to make sure that the behavior of the geo widgets is either the same or improved in terms of how long it takes to acquire high-accuracy readings and resource usage (battery/memory). Unfortunately, documentation isn't super clear on what the behavior used to be and what improvements we should expect. In particular, with the previous APIs we had no control or visibility in how often the position was refreshed. @jknightco examined the behavior of his Nexus 5 and went for a refresh frequency of 5 seconds. With the old APIs, different devices may have had dramatically different behavior so it might be helpful to establish some kind of baseline. I'm planning on taking a 30 minute walk with a geotrace from v1.9.1 in automatic mode taking readings every 20 seconds. I'll then report the accuracy of those readings and the change in battery %. Then I'll do the same route with this new branch and compare. I will also try to do the same in a vehicle. If anyone has other ideas for verifying the behavior on real devices, that would be great. In general, the details of these widgets aren't deeply documented so as we're identifying their actual behavior it would be helpful to write things down. |
|
@mmarciniak90 if you have any questions about expected behavior on any of the Activities (or anything related to these updates) I believe I have a relatively good understanding of what each should be doing at this point. |
|
I finished first part of tests. I have observed improvements on all devices.
|
|
That's great, @mmarciniak90! Is that indoors by any chance? Outdoors I can get to 4.551 pretty quickly with both versions. |
|
I finished an outdoors tests. Results were compared on version 1.9.1 and on apk from pr.
ODK version 1.9.1
ODK version 1.9.1
ODK pr/#1336
ODK pr/#1336
|
|
Thanks, @mmarciniak90! Interesting about the location provider problem, hopefully there's a fix for that. I went on a couple of geotrace walks today. I had some initial frustrations because currently traces reset when the screen locks or the orientation changes. I was hoping for an accuracy comparison but accuracy is not currently collected in traces. I have documented these things in an idealized geo widget spec that we can continue evolving so we can make a set of improvements at once.
No significant difference in power consumption. The traces looked similar so there aren't any wild changes in accuracy. |
|
I tested this pr today. |
|
Hey folks, back from Greece, sorry for the delays. Looking into what's wrong today. |
|
Looks to be a timing issue related to when getLocationAvailability() returns true with the new Fused Location API. Would explain why it always shows with GPS and only sometimes with high priority—sometimes we already have a cached location ready with High Accuracy whereas GPS is always going to require a fix. Looking to see what's the best route forward here. |
|
Fixed the error, but still not receiving location updates when only GPS is on. Looking into it now. |
|
Ha Ha! The bug was inside me all along: I walked out of the city center and was able to finally get a GPS signal no problem. Derp. The good news is everything should be working great now. |
|
Wow, cities are something! I don't have a device running Android < 6.0 to verify on but I did verify that all modes work on my devices. @grzesiek2010 could you please take another verification pass? |
|
Sure I'll do that. |
|
I've tested geotrace using two devices. I've used automatic mode recording one point every 30sec. Location mode : High accuracy
Location mode : GPS Only
Summarizing both tables new functionality probably takes a little less battery or the same in the worst case. I have also noticed that I lost my location in a few cases using I was in transits of course it's not a fake data from one place 😃 I'll add results of testing simple location widget when I finish. |
|
GeoTrace is actually a place where we can see some pretty drastic battery improvements—I didn't realize previously that the user was able to set how often they wanted to record a data point. We could change the update frequency based on that value and see some pretty serious improvements. |
|
@jknightco Very true! The ideal would be for the update to happen right before the next point is collected, right? Any ideas on how you'd set it based on the user preference? Exactly matching it doesn't seem right. Especially in a vehicle, if the update frequency is too low values could get pretty far off. I'm imagining that if the trace refresh is 1 minute and the update frequency is also 1 minute I could just miss getting a new position before the next point is taken. |
|
Yeah, I think we should probably go with a fraction of the user requested
time. Let me look at exactly how fastest interval and interval work and see
if we can come up with a combination that makes sense.
…On Fri, Sep 1, 2017 at 7:17 PM Hélène Martin ***@***.***> wrote:
@jknightco <https://github.com/jknightco> Very true! The ideal would be
for the update to happen right before the next point is collected, right?
Any ideas on how you'd set it based on the user preference? Exactly
matching it doesn't seem right. Especially in a vehicle, if the update
frequency is too low values could get pretty far off. I'm imagining that if
the trace refresh is 1 minute and the update frequency is also 1 minute I
could just miss getting a new position before the next point is taken.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1336 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWrAdetQl_7so65V1O4B7V_tEz4M3M5ks5seDwegaJpZM4Oz2Nd>
.
|
|
I tested a 'GPS Only' mode. I verified this fix on devices with Android: v4.2, v4.4, v5.1. |
|
and I can confirm it works properly using the lowest supported API 16. I used Samsung Galaxy Young S6310 Android 4.1.2. |
|
Let's merge this first since it's now a clear improvement. We're still in our after-release freeze since we'll almost certainly have to put out a 1.10.2 release for getodk/javarosa#180 so we'll wait until that's done. |
|
@lognaturel Merge should be ready to go now, pending CI passes. |
|
Confirmed during release testing. @opendatakit-bot unlabel "needs testing" |
