977725 - MLS Geolocation seeding GONK GPS Provider

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

On devices that we have a GPS stack, we should considering first using MLS to course positioning and use that to inject the geo stack.

Comment 1

[Doug Turner (:dougt)]

Attachment: bug_977725_patch

kanru, can you take a look?

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8394377 [details] [diff] [review]
bug_977725_patch

Review of attachment 8394377 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +191,1 @@
>  pref("geo.wifi.uri", "https://location.services.mozilla.com/v1/geolocate?key=%MOZILLA_API_KEY%");

MOZ_MOZILLA_API_KEY ?

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8394377 [details] [diff] [review]
bug_977725_patch

Review of attachment 8394377 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Nit: remove extra spaces

::: b2g/app/b2g.js
@@ +191,1 @@
>  pref("geo.wifi.uri", "https://location.services.mozilla.com/v1/geolocate?key=%MOZILLA_API_KEY%");

IIRC the key field is just a stub for now.

https://mozilla-ichnaea.readthedocs.org/en/latest/api/search.html writes:
> The API_KEY can currently be any byte string, for example a uuid.

@@ +191,2 @@
>  pref("geo.wifi.uri", "https://location.services.mozilla.com/v1/geolocate?key=%MOZILLA_API_KEY%");
> +pref("geo.wifi.logging.enabled", true);

Should this also be guarded by RELEASE_BUILD?

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +664,5 @@
> +  if (!coords) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // if we haven't seen anything  the GPS device for 1s,

Missing a word in the sentence? I guess it's "from".

Comment 4

[Hanno Schlichting]

Just making sure I understand the patch: In GonkGPSGeolocationProvider::Startup we get a mls-provider and ask it once for a coarse location, then inject that into the underlying GPS interface.

What's the lifetime of the GonkGPSGeolocationProvider? Is it started once at boot time for each phone or lazily loaded when a first GPS result is requested? Does it stay around or is it regularly shutdown / garbage collected? Trying to get a sense of how often this calls out to MLS.

There's a second part in GonkGPSGeolocationProvider::NetworkLocationUpdate::Update and the 1s timer. I don't quite get the context of when and how often that is called. Could you explain that logic a bit?

Other notes:

Do we have any insights into how GPS chips react to injection? Especially if we might inject wrong or very inaccurate positions? For example MLS might only provide a GeoIP based country estimation with an accuracy of many hundreds of kilometers. From what I understand that shouldn't cause the GPS unit to fail, but just be a bit slower as it has to consider many more possible satellites. But is that actually true? Simple test case: Inject a position in the middle of the ocean into the GPS chip, do a GPS lookup, measure time. Expected result: Should take quite a while, but give you a correct result regardless. Failure scenario: The GPS chip trusts the injection too much and doesn't give you a result at all.

Do we know how long the GPS chips store their last known position in the chips themselves, like they do for almanac/ephemeris data? Scenario: You do a GPS lookup, get a proper result. Reboot the phone. Does the GPS chip remember the last position? If so could we make things worse by providing an inaccurate position from MLS to the chip in this case? If my above assumption is correct, worst case here is increased TTFF, but maybe there is a way to avoid this case.

For testing this: Unfortunately it's chipset/RIL dependent, so can only be tested on real hardware :(

Comment 6

[Doug Turner (:dougt)]

> What's the lifetime of the GonkGPSGeolocationProvider?

It is owned by the geolocation system and is killed after it is no longer needed.  This patch doesn't change that.

> Is it started once at boot time for each phone or lazily loaded when a first GPS result is requested? 

It is started when the HTML5 Geolocation code first needs a location

> I don't quite get the context of when and how often that is called. Could you explain that logic a bit?

We are telling the GPS to tell us a location about every 1s.  That can be overridden by geo.default.update.  The bit I added was to use the network derived location if we haven't see a GPS location in that time period.

> Do we have any insights into how GPS chips react to injection? 

No.  All special sauce.

> specially if we might inject wrong or very inaccurate positions?

Could be really bad.  Tested on the Flame and it was not harmful.

> Do we know how long the GPS chips store their last known position in the chips themselves, like they do for almanac/ephemeris data?

No. :(  This, again, is all proprietary as near as I can tell.

I wish I had better answers on these items.  It is upsetting that I can't just peek into the code running the gps and see what it will do when I inject a position.  One thing we need to consider is that QC and others do replace this piece of code... So they know what they are doing (no black box development).  So, I am not terribly concerned about it.

Comment 7

[Doug Turner (:dougt)]

> Should this also be guarded by RELEASE_BUILD?

Maybe -- but right now, so many phones are misconfigured, I was hoping to be able to use this to see if the phone was at least getting a position from MLS.  Thoughts?

Comment 8

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/315e6657839c

(I removed the logging preference.  We can consider that later in a follow up).

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/315e6657839c

Comment 10

[[:fabrice] Fabrice Desré]

Doug, we probably miss another one to uplift. I rebased but this fails to compile with:

/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.cpp:272: warning: 'visibility' attribute ignored
/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.cpp: In member function 'void GonkGPSGeolocationProvider::RequestDataConnection()':
/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.cpp:404: warning: deprecated conversion from string constant to 'char*'
/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.cpp: At global scope:
/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.cpp:684: error: no 'nsresult GonkGPSGeolocationProvider::NetworkLocationUpdate::LocationUpdatePending()' member function declared in class 'GonkGPSGeolocationProvider::NetworkLocationUpdate'
/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.h:122: error: 'class GonkGPSGeolocationProvider::NetworkLocationUpdate' is private
/home/fabrice/dev/mozilla-b2g28_v1_3t/dom/system/gonk/GonkGPSGeolocationProvider.cpp:684: error: within this context

Comment 11

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/a1ee602fd67f

Comment 12

[:gerard-majax]

As discussed on IRC, bug 996998 may be linked to this.

Comment 13

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/40d27c8cb4d6