Cht-core: Make sure we are storing all debug / output from android when trying to obtain GPS

[SCdF]

We often have trouble getting great GPS information against reports.

We should make sure that we're conveying as much debug and error information onto reports. For example, it looks like we're catching errors from Android and just logging them on Android (which means we won't see them): https://github.com/medic/medic-android/blob/master/src/main/java/org/medicmobile/webapp/mobile/MedicAndroidJavascript.java#L124-L126

[garethbowen]

Do you think the GPS error should be stored on the report or did you have somewhere else in mind (feedback doc, telemetry, etc)?

As far as I can tell the lines you've indicated aren't used in the webapp (or maybe at all?), instead we use the native browser geolocation feature via our Geolocation service. Unfortunately we just warn when an error occurs and don't store it anywhere, eg: https://github.com/medic/medic/blob/master/webapp/src/js/controllers/reports-add.js#L58

[SCdF]

Well we should definitely work out which ones are used and delete the rest!

I think we should make sure we're using the most accurate and detailed GPS location tooling available (ie if we're not using Android does using Android give us better / more detailed information?), and yeah, I think storing it against the report is fine. I don't think we need to store stack traces or anything so it shouldn't be massive?

[SCdF]

We should also add Telemetry to make it easier to get a general overview. What this looks like is pretty dependent on what kind of data we get. The simplest would be a single telemetry field with 1 === successful GPS lock, and 0 === failure. If we can, it might be cool to try to record something more granular (maybe around the level of accuracy?)

[garethbowen]

Scheduled. This is important to help us work out how to improve our GPS data.

[garethbowen]

We should consider using the android location service when available and falling back to geolocation when running directly in the browser for improved data quality and reliability.

[garethbowen]

It'd be worth reaching out to our partners who are getting higher success rates with GPS to help inform the strategy here.

[ecsalomon]

I’ve observed three different schemas for recording GPS data in forms (both within and across projects):

{
    “fields”: {
        “geolocation”: {
            “speed”: ,
            “heading”: ,
            “accuracy”: ,
            “altitude”: ,
            “latitude”: ,
            “longitude”: ,
            “altitudeAccuracy”: 
        }
    }
}

{
    “geolocation”: {
        “speed”: ,
        “heading”: ,
        “accuracy”: ,
        “altitude”: ,
        “latitude”: ,
        “longitude”: ,
        “altitudeAccuracy”: 
    }
}

{
    “fields”: {
        “inputs”: {
            “meta”: {
                “location”: {
                    “lat”: ,
                    “long”: ,
                    “error”: ,
                    “message”: 
                }
            }
        }
    }
}

Why do these differences arise? Are they from different phone types, different app versions, different methods of getting GPS data, something else? The altitude field is quite nice for some of the GPS concepts the Research and Learning team would like to work on, but the error messages are also nice to have, and it seems like we never get both.

[SCdF]

I think it's different app versions, combined with different form configurations.

The first two are I believe the correct locations for reports and contacts respectively. The last is the "old way", where fields/inputs is how you inject data into a report that isn't entered by the user (you'll see other info there if for example you're completing a task whatever info is needed like that patient_id will be injected into their and then moved around as needed).

[mouriceb]

Had a brainstorming session with some of colleagues on how to improve the geolocation data especially at household registration and came up with the following which could probably be worked on at service design stage. Our main challenge now is that GPS data is mostly collected in the background with a focus on minimising battery drain, but may be important to overlook this at household registration.

https://docs.google.com/document/d/1fp3qkSAhvWAf7TuROQJ9Bxs6932wT1F19tfm0aJU-F8/edit

CC: @leah-ngaari @JenniferMuli @kitsao

[MaxDiz]

R&L team ideas on alternative solutions to current GPS. It may be worth looking into a replacement or supplement using https://plus.codes/
https://github.com/medic/medic-impact/issues/174

[brian256]

Have we considered GPS as a standard feature for Supervisor workflows? Most programs have stronger support (access to charging, office space et al) for supervisor actions that would triangulate CHP work. This is a consideration for BRAC.

[SCdF]

This ticket is about an improvement in 3.9 to increase the quality of GPS reporting. To avoid it being lost, conversations about longer term strategies about GPS, alternatives etc should be held elsewhere.

[ecsalomon]

Re https://github.com/medic/cht-core/issues/5917#issuecomment-563389934: See report here. @garethbowen

[garethbowen]

Deferring this to 3.10.0 so we can get 3.9.0 out as soon as possible - please contact me if you consider this more urgent.

[garethbowen]

Following on from this comment I'm growing more and more confident that we should use the Android Location service by default, only using the browser version when it's not available.

[SCdF]

@garethbowen agreed. I actually thought that's what we were doing, but it looks like we went browser only a few years ago: #3781

(code still exists in medic-android so we can bring it back to life as need be)

I'm going to do a bit of research on android vs. browser and you and I should chat when you're around in case you've already done the same and we can combine our brains!

[SCdF]

It's the end of my day: turns out I cannot find anything that would imply that using native android apis over browser ones gives you more accuracy. I'm not saying that it's not possible (it's what I presumed), just that my research didn't turn it up. I think this is important because the browser apis are _much_ easier to work with than the Android ones, and we probably want to use them properly anyway.

Our prior Android code also used getLastKnownLocation as opposed to requestLocationUpdates along with a listener. This could have made it pretty inaccurate, which may have been why we moved away from it.

Things we should consider / look into:

• new toggle setting that requires that users give us GPS access (android or browser). Partners who need this for whatever reason (eg they are doing a study where geolocation is important) can enable it.
• using browser watchPosition api or android's requestLocationUpdates api to continually try to refine GPS position down for the duration a form is open until it's completed or we get to a certain m range (ie 10m).
• Are we allowed to use better (easier to use, better for battery life etc) gps apis like the fused api that are shipped with play services, even if we're not wanting to be tied to the play store?

I still have tabs to work through, but I wanted to post this now so @garethbowen can read it over his day and respond if its relevant :+1:

[SCdF]

Talked with Gareth. Here are three things we're looking at, as an evolution of our GPS strategy:

• Make sure we are catching all debug info we can, somewhere. Against the form, or in telemetry etc
• Change our JS code to use web geo api as optimally as possible
• Change our android code to use native apis as optimally as possible (this will likely come later)

[SCdF]

@garethbowen / @abbyad current prod code does two things that are potentially confusing:

• Has the timeout for how long it's willing to wait for geo data set at 5 minutes, but completely ignores this and if it doesn't get a geo lock in time it just passes geo as undefined
• Only overwrites geo data if it has any, leaving old geo on edited docs arbitrarily

The former is probably one of the reasons we often don't have geo data honestly. The code I'm working on drops that timeout to 30s and actually adheres to it, which seems more correct to me. 30s is a made up number, we can come to some conclusion about what that should be.

The latter is more weird. For now the change attempts to copy this logic, but because it's also storing the error you can get into situations where you have old errors and new geolocations (or vice versa), which is even worse than having just the geo data detached from the edit.

This seems wrong, and it is a hard thing to answer as the _real_ correct answer is that we shouldn't really allow people to edit docs at all (ie only allow amendment docs that are attached).

Interested in your thoughts on how this should work on edits:

• Always rewrite the latest geo or geoError
• Never rewrite geo data even if this time we got a lock and last time we didn't
• Only rewrite the latest geo if it gets a lock, otherwise ignore (and don't log new geoError)
• Something else

[abbyad]

Has the timeout for how long it's willing to wait for geo data set at 5 minutes, but completely ignores this and if it doesn't get a geo lock in time it just passes geo as undefined

What is the time in "doesn't get a geo lock in time" -- is that 5 mins, or based on user behaviour, like completing the form?

As for ammendments, I think that is a bigger question to address than just for GPS, as it has to do with our data model and auditing. We should open a separate issue for that. cc @MaxDiz

[SCdF]

What is the time in "doesn't get a geo lock in time" -- is that 5 mins, or based on user behaviour, like completing the form?

Exactly, like completing the form

[SCdF]

OK PR for webapp changes here: https://github.com/medic/cht-core/pull/6535

[SCdF]

PR for removing the periodic gps call in android: https://github.com/medic/medic-android/pull/110

[garethbowen]

AT instructions:

• Make sure you install the 5917-goodbye-periodic-gpsing branch of medic-android. It shouldn't make any different as it just removes dead code but we need to prove it's actually dead code!
• Install the 5917_better-gps branch of cht-core.
• Test submitting reports on various mobile devices, and desktop browsers, in lots of places, with GPS turned off, inside and outside, on the move, and anything else you can think of. You can see the geolocation information stored on the report in CouchDB.
• Ensure editing reports works by appending to a log so we can see the history of geolocation fetches.
• It would be good to test what happens if the browser takes longer than 30 seconds to respond to us. Submission should block for 30 seconds and then store a timeout error. I don't know how to get into this state but possibly by having GPS turned on but not having reception for the satellite signal...

[newtewt]

@garethbowen was there any update on whether to hold off on this until we make more changes?

[garethbowen]

Yes, if there are other things to AT it would be better to hold off on this one until https://github.com/medic/medic-android/issues/111 is done.

[dianabarsan]

I'm going to have to add a few changes to cht-core to support permission changes in medic-android.
Does anyone have any objections to pushing changes to the already existent branch?

The changes I'm making will warrant checking that Geolocation still works, so it will maybe shave some time if both would be AT-ed at once?

[garethbowen]

Does anyone have any objections to pushing changes to the already existent branch?

Makes sense to me.

[dianabarsan]

This is ready for AT again.

This should be tested against two medic-android branches:

1. master
2. 5917-goodbye-periodic-gpsing

The test against master should assure backwards compatibility with older medic-android versions. Alos, please test this in a browser.

When testing against 5917-goodbye-periodic-gpsing on Android >= 6, you should get a permissions request popup when creating/editing a report / completing a task.
The permission request will keep appearing on the next create/edit/task, until you accept or "deny and don't show again".
For Android 5 and 4, you should get no permissions requests, as in these versions, permissions are granted automatically when the app is installed.

At least once, please wait for at least 30 seconds before you click on allowing the permission. (this tests whether starting the watcher - which has a 30s timeout - actually waits until the permission request is responded to).

[ngaruko]

Not sure if these three elements (altitudeAccuracy, heading, speed) have any value to our application, they seem to always log null values @dianabarsan ?

"geolocation_log": [
    {
      "timestamp": 1596689607020,
      "recording": {
        "latitude": -xxxxx,
        "longitude": xxxxx,
        "altitude": 47.5,
        "accuracy": 28.52899932861328,
        "altitudeAccuracy": null,
        "heading": null,
        "speed": null
      }
    },
    {
      "timestamp": 1596689665488,
      "recording": {
        "latitude": xxxxxxxx,
        "longitude": xxxxx,
        "altitude": 46,
        "accuracy": 44.2599983215332,
        "altitudeAccuracy": null,
        "heading": null,
        "speed": null
      }
    }
  ],
  "geolocation": {
    "latitude": xxxx,
    "longitude": xxxxxx,
    "altitude": 46,
    "accuracy": 44.2599983215332,
    "altitudeAccuracy": null,
    "heading": null,
    "speed": null
  },

[dianabarsan]

I haven't seen any non-null values for those properties either.

I think including all Geolocation coords properties was done in the interest of complete accuracy and transparency about geolocation results. This is, actually, the title of the issue: "Make sure we are storing all output .... "

Here is a list of GeolocationCoordinates properties: https://developer.mozilla.org/en-US/docs/Web/API/GeolocationCoordinates

altitude
altitudeAccuracy
heading
latitude
longitude
speed

[ngaruko]

• It would be good to test what happens if the browser takes longer than 30 seconds to respond to us. Submission should block for 30 seconds and then store a timeout error. I don't know how to get into this state but possibly by having GPS turned on but not having reception for the satellite signal...

I have tested everything, except this. Need to find a underground parking or tunnel with no GPS signal. Did you want to try this @newtewt ? Or how important is this @garethbowen ?

[dianabarsan]

Not really a "valid test case", but I managed to trigger the 30s timeout on an Android 5 device, where I had installed an app called "Permission Manager" (https://play.google.com/store/apps/details?id=com.ovmobile.appopslauncher&hl=en) and disallowed the Location permission for the Medic app with it.

I thought it was an interesting interaction.
Android reports the permission as being granted, but the Geolocation service doesn't get a lock.
I wasn't worried about it, because Android 5 (and lower) don't have a straight forward way of disallowing permissions and the "Permission Manager" app is probably doing something hacky under the hood.

[mrjones-plip]

Need to find a underground parking or tunnel with no GPS signal.

I am very happy to offer some hacking techniques in a professional setting ; ) @ngaruko et al. - we can just use a Faraday Bag (or apparently a "booster bag") to test GPS on but with no signal (and no WiFi signal too!)

[abbyad]

On a local 3.9ish version (6291-case-id branch), as a offline user I see the following violation in the console:

[Violation] Only request geolocation information in response to a user gesture.

This seems to be related to Google location recommendations.

For AT, it would be good to check and report on the the console messages before and after the geolocation permission changes to see if it affects the warning.

[ngaruko]

Thanks @abbyad . The warning does not show on the branch.
On a separate note, there are seems to be some discrepancies with the accuracy from something like "accuracy": 875.3489990234375 (old) to "accuracy": 20 - hard to tell whether that's a significant metric/improvement on this feature. (as it is somehow inconsistent)
Will move this to ready.

[garethbowen]

@ngaruko That means the accuracy of the implementation improved from a 95% confidence interval of 875m to 20m, ie: lower is better.

[dianabarsan]

Merged into master.