Closed Bug 977725 Opened 6 years ago Closed 6 years ago

MLS Geolocation seeding GONK GPS Provider

Categories

(Core :: DOM: Geolocation, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(1 file)

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.
Summary: MLS Geolocation seeing GONK GPS Provider → MLS Geolocation seeding GONK GPS Provider
Attached patch bug_977725_patchSplinter Review
kanru, can you take a look?
Attachment #8394377 - Flags: review?(kchen)
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 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".
Attachment #8394377 - Flags: review?(kchen) → review+
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 :(
Duplicate of this bug: 965191
> 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.
> 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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/315e6657839c

(I removed the logging preference.  We can consider that later in a follow up).
https://hg.mozilla.org/mozilla-central/rev/315e6657839c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
blocking-b2g: --- → 1.3T+
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
Flags: needinfo?(dougt)
As discussed on IRC, bug 996998 may be linked to this.
You need to log in before you can comment on or make changes to this bug.