715788 - Add A-GPS support for gonk

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We'll want a-gps support the day after we land gps.

Comment 1

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

Attachment: WIP patch

Comment 2

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

We need to get IMSI, MSISDN, mnc, mcc, lac (GSM only) and cid from RIL. We also need translation between APN and cid.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

philikon was kind enough to share the magical snippet needed to get at the MSISDN (from JS).

  this.mRIL = Cc["@mozilla.org/telephony/system-worker-manager;1"]
                .getService(Ci.nsIInterfaceRequestor)
                .getInterface(Ci.nsIRadioInterfaceLayer);
  this.mRIL.radioState.icc.MSISDN

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #2)
> We need to get IMSI, MSISDN, mnc, mcc, lac (GSM only) and cid from RIL. We
> also need translation between APN and cid.

We can expose all this info. As comment 3 points out, some of it is already exposed... How does it all feed into the A-GPS functionality?

Comment 5

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

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> (In reply to Kan-Ru Chen [:kanru] from comment #2)
> > We need to get IMSI, MSISDN, mnc, mcc, lac (GSM only) and cid from RIL. We
> > also need translation between APN and cid.
> 
> We can expose all this info. As comment 3 points out, some of it is already
> exposed... How does it all feed into the A-GPS functionality?

After the A-GPS engine is initialized, it will request to connect to the SUPL APN and then request these information via callbacks.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Since its priority is P1 now, 
I'll start to work on Bug 738558 to provide LAC and CID.

thanks

Comment 7

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

Attachment: WIP patch

Can get network information from RIL now.

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 630499 [details] [diff] [review]
WIP patch

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +363,5 @@
> +  dom::workers::RuntimeService::AutoSafeJSContext cx;
> +  JSAutoByteString str;
> +
> +  if (!JSVAL_IS_PRIMITIVE(radioState)) {
> +    JSObject* jsRadioState = JSVAL_TO_OBJECT(radioState);

At this point I think I'd be more comfortable with a spec'ed (interface) radioState. We can easily break this code by simply refactoring something in the RIL. With an explicit interface, that'd be harder.

Comment 9

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

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Comment on attachment 630499 [details] [diff] [review]
> WIP patch
> 
> Review of attachment 630499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +363,5 @@
> > +  dom::workers::RuntimeService::AutoSafeJSContext cx;
> > +  JSAutoByteString str;
> > +
> > +  if (!JSVAL_IS_PRIMITIVE(radioState)) {
> > +    JSObject* jsRadioState = JSVAL_TO_OBJECT(radioState);
> 
> At this point I think I'd be more comfortable with a spec'ed (interface)
> radioState. We can easily break this code by simply refactoring something in
> the RIL. With an explicit interface, that'd be harder.

Agreed. This piece of code is temporary so that I can test the A-GPS functionality.

Comment 10

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

Attachment: Part 1. Refactor static functions to static member functions.

Comment 11

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

Attachment: Part 2. Add A-GPS support.

WIP.

Comment 12

[Eric Shepherd [:sheppy]]

At a minimum, the new prefs need documenting. Haven't looked yet to see what else will.

Comment 13

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

The weird thing is, the gps driver on nexus-s does not request data connection at all..

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #13)
> The weird thing is, the gps driver on nexus-s does not request data
> connection at all..

Not sure it's a valid datapoint, but my personal phone is a Nexus S and the GPS sucks. :)

Anyway, what happens on the Otoro?

Comment 15

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

(In reply to Philipp von Weitershausen [:philikon] from comment #14)
> (In reply to Kan-Ru Chen [:kanru] from comment #13)
> > The weird thing is, the gps driver on nexus-s does not request data
> > connection at all..
> 
> Not sure it's a valid datapoint, but my personal phone is a Nexus S and the
> GPS sucks. :)

Well, it seems Nexus S will use any data connection currently on.

> Anyway, what happens on the Otoro?

I'd like to test on Otoro, but the we don't have working binary driver yet.

Comment 16

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

Attachment: Add A-GPS support.

WIP.

Use new RilContext interface to get ril data.

Comment 17

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

Attachment: Part 1. Refactor static functions to static member functions.

Comment 18

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

Attachment: Part 2. Add A-GPS support.

Some bugs need to be filed.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 640569 [details] [diff] [review]
Part 1. Refactor static functions to static member functions.

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +15,5 @@
>  
>  NS_IMPL_ISUPPORTS1(GonkGPSGeolocationProvider, nsIGeolocationProvider)
>  
> +/* static */ GonkGPSGeolocationProvider* GonkGPSGeolocationProvider::sSingleton;
> +/* static */ GpsCallbacks

I do not understand why you have static in comments here.

@@ +49,5 @@
>                                                          location->accuracy,
>                                                          location->bearing,
>                                                          location->speed,
>                                                          location->timestamp);
> +  // Can be used from any thread.

can it?

@@ +195,2 @@
>  
> +  nsCOMPtr<nsIThread> initThread;

Suppose GonkGPSGeolocationProvider::Init took a long time to startup.  During that time, the user wanted to quit.  Does this thread prevent that?  Should you cancel the thread during shutdown?

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 640570 [details] [diff] [review]
Part 2. Add A-GPS support.

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

::: b2g/app/b2g.js
@@ +168,5 @@
>  
>  // base url for the wifi geolocation network provider
>  pref("geo.wifi.uri", "https://maps.googleapis.com/maps/api/browserlocation/json");
>  
> +// geolocation gps provider, for testing only!

we probably want to put these in one of the testing js prefs files if it is only for testings?

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +154,5 @@
> +    GonkGPSGeolocationProvider::GetSingleton();
> +
> +  nsCOMPtr<nsIRunnable> event;
> +  switch (status->status) {
> +  case GPS_REQUEST_AGPS_DATA_CONN:

style nit:  case should be two spaces in from where switch starts.

@@ +160,5 @@
> +    NS_DispatchToMainThread(event);
> +    break;
> +  case GPS_RELEASE_AGPS_DATA_CONN:
> +    event = NS_NewRunnableMethod(provider, &GonkGPSGeolocationProvider::ReleaseDataConnection);
> +    NS_DispatchToMainThread(event);

You could move the: NS_DispatchToMainThread(event); out of the switch statement.

@@ +250,5 @@
>  
>  void
> +GonkGPSGeolocationProvider::RequestDataConnection()
> +{
> +  // TODO: Bug xxxx - We should ask NetworkManager to open SUPL type

file bug.  Update comment.

@@ +252,5 @@
> +GonkGPSGeolocationProvider::RequestDataConnection()
> +{
> +  // TODO: Bug xxxx - We should ask NetworkManager to open SUPL type
> +  // connection for us.
> +  nsAdoptingString apnName = Preferences::GetString("geo.gps.apn.name");

const nsAdoptingString& ^^

@@ +308,5 @@
> +  mRIL->GetRilContext(getter_AddRefs(rilCtx));
> +
> +  AGpsRefLocation location;
> +
> +  // TODO: Bug xxxx - Support CDMA A-GPS

File bug. update comment.

@@ +473,5 @@
> +
> +/** nsIRILDataCallback interface **/
> +
> +NS_IMETHODIMP
> +GonkGPSGeolocationProvider::DataCallStateChanged(nsIRILDataCallInfo* aDataCall)

can aDataCall ever be null?

@@ +481,5 @@
> +  PRUint32 callState;
> +  nsresult rv = datacall->GetState(&callState);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsAutoString _apn;

nit: apn, not _apn

@@ +491,5 @@
> +
> +  NS_ConvertUTF16toUTF8 currentApn(_apn);
> +  nsAdoptingCString agpsApn = Preferences::GetCString("geo.gps.apn.name");
> +
> +  // TODO: Bug xxxx - handle data call failed case. Depends on bug 744700

Same.

Comment 21

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

(In reply to Doug Turner (:dougt) from comment #19)
> Comment on attachment 640569 [details] [diff] [review]
> Part 1. Refactor static functions to static member functions.
> 
> Review of attachment 640569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +15,5 @@
> >  
> >  NS_IMPL_ISUPPORTS1(GonkGPSGeolocationProvider, nsIGeolocationProvider)
> >  
> > +/* static */ GonkGPSGeolocationProvider* GonkGPSGeolocationProvider::sSingleton;
> > +/* static */ GpsCallbacks
> 
> I do not understand why you have static in comments here.

Because it is a static member variable. Do you want me to remove it?

> @@ +49,5 @@
> >                                                          location->accuracy,
> >                                                          location->bearing,
> >                                                          location->speed,
> >                                                          location->timestamp);
> > +  // Can be used from any thread.
> 
> can it?

At least in current nsGeolocation impl. But no, in the idl it says main thread only. I will update.

> 
> @@ +195,2 @@
> >  
> > +  nsCOMPtr<nsIThread> initThread;
> 
> Suppose GonkGPSGeolocationProvider::Init took a long time to startup. 
> During that time, the user wanted to quit.  Does this thread prevent that? 
> Should you cancel the thread during shutdown?

Good catch! I don't think we could cancel the init. If that happens we probably want to stop the engine after it has been fully initialized. But then the user can start the gps again when we are waiting the engine... I think we could use the init thread to queue these requests up.

(In reply to Doug Turner (:dougt) from comment #20)
> Comment on attachment 640570 [details] [diff] [review]
> Part 2. Add A-GPS support.
> 
> Review of attachment 640570 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/app/b2g.js
> @@ +168,5 @@
> >  
> >  // base url for the wifi geolocation network provider
> >  pref("geo.wifi.uri", "https://maps.googleapis.com/maps/api/browserlocation/json");
> >  
> > +// geolocation gps provider, for testing only!
> 
> we probably want to put these in one of the testing js prefs files if it is
> only for testings?

I mean manual testing. I put the warning here because the google supl server cannot be use on production without google's approve.

> @@ +473,5 @@
> > +
> > +/** nsIRILDataCallback interface **/
> > +
> > +NS_IMETHODIMP
> > +GonkGPSGeolocationProvider::DataCallStateChanged(nsIRILDataCallInfo* aDataCall)
> 
> can aDataCall ever be null?

It shouldn't be! I can add a assert here.

Comment 22

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

Attachment: Part 1. Refactor static functions to static member functions.

Comment 23

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

Attachment: Part 2. Add A-GPS support.

Comment 24

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

Attachment: Part 2. Add A-GPS support. v1.1

Address nits and fix compatibility on GB/ICS/ICS+

Comment 25

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

https://tbpl.mozilla.org/?tree=Try&rev=3fc52cef81f9 try build

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 640940 [details] [diff] [review]
Part 1. Refactor static functions to static member functions.

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +16,5 @@
>  NS_IMPL_ISUPPORTS1(GonkGPSGeolocationProvider, nsIGeolocationProvider)
>  
> +/* static */ GonkGPSGeolocationProvider* GonkGPSGeolocationProvider::sSingleton;
> +/* static */ GpsCallbacks
> +GonkGPSGeolocationProvider::mCallbacks = {

please remove all /* static */ comments.

@@ +178,5 @@
> +  }
> +
> +  NS_DispatchToMainThread(NS_NewRunnableMethod(this, &GonkGPSGeolocationProvider::StartGPS));
> +
> +  return;

no need for a return statement here.

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 641421 [details] [diff] [review]
Part 2. Add A-GPS support. v1.1

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

with nits fixed and answers/fixes for questions.

::: b2g/app/b2g.js
@@ +168,5 @@
>  
>  // base url for the wifi geolocation network provider
>  pref("geo.wifi.uri", "https://maps.googleapis.com/maps/api/browserlocation/json");
>  
> +// geolocation gps provider, for testing only!

I do not think you should commit these values.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +29,2 @@
>  
>  /* static */ GonkGPSGeolocationProvider* GonkGPSGeolocationProvider::sSingleton;

please remove all /* static */ comments

@@ +163,5 @@
>  {
>  }
>  
> +/* static */ void
> +GonkGPSGeolocationProvider::AGPSStatusCallback(AGpsStatus* status)

I am assuming you don't need to test for null.  Docs in include/hardware/gps.h don't suggest either way.

@@ +337,5 @@
> +      PRUint16 mnc;
> +      icc->GetMcc(&mcc);
> +      icc->GetMnc(&mnc);
> +      location.u.cellID.mcc = mcc;
> +      location.u.cellID.mnc = mnc;

why not?

icc->GetMcc(&location.u.cellID.mcc);
icc->GetMcn(&location.u.cellID.mnc);

@@ +347,5 @@
> +      PRUint32 cid;
> +      cell->GetLac(&lac);
> +      cell->GetCid(&cid);
> +      location.u.cellID.lac = lac;
> +      location.u.cellID.cid = cid;

similar question.

Comment 28

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

(In reply to Doug Turner (:dougt) from comment #27)
> Comment on attachment 641421 [details] [diff] [review]
> Part 2. Add A-GPS support. v1.1
> 
> Review of attachment 641421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> with nits fixed and answers/fixes for questions.
> 
> ::: b2g/app/b2g.js
> @@ +168,5 @@
> >  
> >  // base url for the wifi geolocation network provider
> >  pref("geo.wifi.uri", "https://maps.googleapis.com/maps/api/browserlocation/json");
> >  
> > +// geolocation gps provider, for testing only!
> 
> I do not think you should commit these values.

Removed.
  
> > +/* static */ void
> > +GonkGPSGeolocationProvider::AGPSStatusCallback(AGpsStatus* status)
> 
> I am assuming you don't need to test for null.  Docs in
> include/hardware/gps.h don't suggest either way.

Android impl doesn't check either. Added a assertion anyway.

Comment 29

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

Attachment: Part 1. Refactor static functions to static member functions.

Comment 30

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

Attachment: Part 2. Add A-GPS support. v1.1

Comment 31

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

Attachment: Part 2. Add A-GPS support. v1.1

Turn off debug.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/039a4f5025e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f6f8dbb06d

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/039a4f5025e7
https://hg.mozilla.org/mozilla-central/rev/68f6f8dbb06d