1177413 - Support GPS XTRA download feature

Status: RESOLVED WONTFIX

(Core :: DOM: Geolocation, defect)

Description

[:gerard-majax]

To help getting a fix faster, we should implement support for GPS XTRA downloading and injecting.

This should get triggered by the download_request_cb attached to the AGpsXtraInterface we configure from libloc_eng from Gecko. We should also be able to do it on our own:
 - downloading the xtra.bin file
 - injecting it in the API via inject_xtra_data.

Comment 1

[:gerard-majax]

Attachment: agps+xtra2.diff

WIP hack:
 - injecting time (probably wrong UTC time)
 - injecting existing /data/xtra2.bin

GPS stack seems to react correctly to those but I still face:
 - no download_request_cb being called on GpsXtraInterface
 - no status_cb being called on AGpsInterface
 - quite slow fix

Comment 2

[:gerard-majax]

Attachment: Support GPS XTRA download and injection r=...

Comment 3

[Hanno Schlichting]

Comment on attachment 8631033 [details] [diff] [review]
Support GPS XTRA download and injection r=...

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

Just providing some nitpick comments, my C and FxOS knowledge isn't good enough to give proper feedback.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +56,5 @@
>  using namespace mozilla;
>  using namespace mozilla::dom;
>  
>  static const int kDefaultPeriod = 1000; // ms
> +static bool gDebug_isLoggingEnabled = true;

Did you mean to include this in the patch?

@@ +303,5 @@
> +      int length;
> +
> +      nsContentUtils::LogMessageToConsole("geo: XtraDownloadEvent opening xtra bin file\n");
> +
> +      FILE *f = fopen("/data/xtra2.bin", "rb");

Maybe make the path a constant? Also maybe check that the file is present before trying to open it.

Out of curiosity, do you know who puts the file in this place? The xtra data is only valid for a certain period of time, so unless some continuous process updates it, it won't help.

@@ +885,5 @@
>  #endif
>  
> +  if (mSupportsTimeInjection) {
> +    InjectTime((PR_Now() / PR_USEC_PER_MSEC) - (2 * 3600), (PR_Now() / PR_USEC_PER_MSEC) - (86400 * 1000 * 10), 150);
> +    GPSXtraDownloadCallback();

The first argument is confusingly of type GpsUtcTime. Do you know if that is GPS time or UTC time? GPS time isn't adjusted for leap seconds, so it's now 17 seconds ahead of UTC time.

In other places where we use PR_Now and GPS time we have to be really careful about this.

@@ +1255,5 @@
>        return NS_OK;
>      } else if (setting.mKey.EqualsASCII(kSettingDebugEnabled)) {
>        nsContentUtils::LogMessageToConsole("geo: received mozsettings-changed: logging\n");
> +      // gDebug_isLoggingEnabled =
> +      //  setting.mValue.isBoolean() ? setting.mValue.toBoolean() : false;

Did you mean to comment this out?

::: toolkit/modules/Sntp.jsm
@@ +10,5 @@
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  // Set to true to see debug messages.
> +let DEBUG = true;

Did you mean to include this in the patch?

Comment 4

[:gerard-majax]

Don't bother checking this patch, it's still WIP and just because I needed to rebase some stuff and did a "backup" of my hack.

Comment 5

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #1)
> Created attachment 8629874 [details] [diff] [review]
> agps+xtra2.diff
> 
> WIP hack:
>  - injecting time (probably wrong UTC time)
>  - injecting existing /data/xtra2.bin
> 
> GPS stack seems to react correctly to those but I still face:
>  - no download_request_cb being called on GpsXtraInterface
>  - no status_cb being called on AGpsInterface
>  - quite slow fix

FYI remaining work should be:
 - perform a real SNTP request to use in the inject_time call
 - really download xtra/xtra2 data instead of reading from system (with caching?)

Jamin, do you have some time to play with this?

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> (In reply to Alexandre LISSY :gerard-majax from comment #1)
> > Created attachment 8629874 [details] [diff] [review]
> > agps+xtra2.diff
> > 
> > WIP hack:
> >  - injecting time (probably wrong UTC time)
> >  - injecting existing /data/xtra2.bin
> > 
> > GPS stack seems to react correctly to those but I still face:
> >  - no download_request_cb being called on GpsXtraInterface
> >  - no status_cb being called on AGpsInterface
> >  - quite slow fix
> 
> FYI remaining work should be:
>  - perform a real SNTP request to use in the inject_time call
>  - really download xtra/xtra2 data instead of reading from system (with
> caching?)
> 
> Jamin, do you have some time to play with this?

Hi Alexandre,

Thank you for your effort.
I'm glad to work on this bug if you don't have time for it.

P.S. FxOS supports both NITZ and SNTP since Bug 874771 landed. However, it seems that SNTP has broken for a while. I will investigate on <gecko>/toolkit/modules/Sntp.jsm first and maybe file a bug later.

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #6)
> P.S. FxOS supports both NITZ and SNTP since Bug 874771 landed. However, it
> seems that SNTP has broken for a while. I will investigate on
> <gecko>/toolkit/modules/Sntp.jsm first and maybe file a bug later.

I filed a Bug 1187813 for SNTP since it's not working on FxOS m-c.
I plan to use system time from NITZ first and replace it by SNTP once Bug 1187813 is resovled.

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [WIP] bug1177413_XtraInjection_v0.patch

It's a draft patch for XTRA injection. (which don't includes the time injection)
I'll do more tests based on this patch on Monday.

Comment 9

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [WIP] [DO NOT COMMIT] Supprt Time injection by using system time for NITZ.

The time injection should use SNTP as the source in future.
Since SNTP isn't working on FxOS m-c, I use system time from NITZ first for the testing of XTRA injection.

Please refer to #comment7.

Comment 10

[:gerard-majax]

So Jamin is not working on this anymore I think. We still need to handle this and more specifically to assert how much this is working. In order to perform this, I suggest we rely on the changes in bug 1190337 and we do an extension to geolocation that will be only exposed in eng build. That extension will allow an app to get the satellite views and be able to compute for us the TTF.

Hence, I suggest:
 - extend geolocation for our debugging purpose with satellite views, constellations and as much as we can
 - write a debug geolocation app that will be able to:
   - reset the A-GPS data
   - request geolocation
   - track status of satellites
   - get us a proper time to fix value

With this we should be able to answer the question: "does it works?"

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I agree. Not too sure who will build this app...  It will be helpful for testing purposes.