Last Comment Bug 715788 - Add A-GPS support for gonk
: Add A-GPS support for gonk
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: P1 normal (vote)
: mozilla17
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on: 772748 711300 736941 738558 762426 762760 772747 772750 841360
Blocks: b2g-ril b2g-product-phone
  Show dependency treegraph
 
Reported: 2012-01-05 20:18 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2015-05-27 20:27 PDT (History)
13 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
WIP patch (9.21 KB, patch)
2012-03-19 04:34 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
WIP patch (23.61 KB, patch)
2012-06-06 04:06 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 1. Refactor static functions to static member functions. (8.85 KB, patch)
2012-06-11 21:55 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 2. Add A-GPS support. (19.99 KB, patch)
2012-06-11 21:55 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Add A-GPS support. (19.40 KB, patch)
2012-06-21 03:14 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 1. Refactor static functions to static member functions. (8.94 KB, patch)
2012-07-10 04:40 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 2. Add A-GPS support. (15.96 KB, patch)
2012-07-10 04:41 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 1. Refactor static functions to static member functions. (10.26 KB, patch)
2012-07-10 23:35 PDT, Kan-Ru Chen [:kanru] (UTC+8)
dougt: review+
Details | Diff | Review
Part 2. Add A-GPS support. (15.99 KB, patch)
2012-07-10 23:35 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 2. Add A-GPS support. v1.1 (16.43 KB, patch)
2012-07-12 03:45 PDT, Kan-Ru Chen [:kanru] (UTC+8)
dougt: review+
Details | Diff | Review
Part 1. Refactor static functions to static member functions. (10.08 KB, patch)
2012-07-16 14:47 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 2. Add A-GPS support. v1.1 (15.04 KB, patch)
2012-07-16 14:48 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Part 2. Add A-GPS support. v1.1 (15.04 KB, patch)
2012-07-16 14:49 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-05 20:18:15 PST
We'll want a-gps support the day after we land gps.
Comment 1 Kan-Ru Chen [:kanru] (UTC+8) 2012-03-19 04:34:47 PDT
Created attachment 607115 [details] [diff] [review]
WIP patch
Comment 2 Kan-Ru Chen [:kanru] (UTC+8) 2012-03-21 20:42:39 PDT
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 2012-05-09 13:29:37 PDT
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] 2012-05-14 14:21:07 PDT
(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+8) 2012-05-14 19:25:41 PDT
(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 Huang[:allstars.chh] 2012-05-30 02:44:49 PDT
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+8) 2012-06-06 04:06:49 PDT
Created attachment 630499 [details] [diff] [review]
WIP patch

Can get network information from RIL now.
Comment 8 Philipp von Weitershausen [:philikon] 2012-06-07 12:01:56 PDT
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+8) 2012-06-07 19:07:15 PDT
(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+8) 2012-06-11 21:55:16 PDT
Created attachment 632132 [details] [diff] [review]
Part 1. Refactor static functions to static member functions.
Comment 11 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-11 21:55:46 PDT
Created attachment 632133 [details] [diff] [review]
Part 2. Add A-GPS support.

WIP.
Comment 12 Eric Shepherd [:sheppy] 2012-06-12 17:34:50 PDT
At a minimum, the new prefs need documenting. Haven't looked yet to see what else will.
Comment 13 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-19 21:10:24 PDT
The weird thing is, the gps driver on nexus-s does not request data connection at all..
Comment 14 Philipp von Weitershausen [:philikon] 2012-06-20 10:13:27 PDT
(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+8) 2012-06-20 19:36:49 PDT
(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+8) 2012-06-21 03:14:38 PDT
Created attachment 635233 [details] [diff] [review]
Add A-GPS support.

WIP.

Use new RilContext interface to get ril data.
Comment 17 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-10 04:40:19 PDT
Created attachment 640569 [details] [diff] [review]
Part 1. Refactor static functions to static member functions.
Comment 18 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-10 04:41:18 PDT
Created attachment 640570 [details] [diff] [review]
Part 2. Add A-GPS support.

Some bugs need to be filed.
Comment 19 Doug Turner (:dougt) 2012-07-10 09:28:49 PDT
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) 2012-07-10 09:36:04 PDT
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+8) 2012-07-10 17:57:23 PDT
(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+8) 2012-07-10 23:35:24 PDT
Created attachment 640940 [details] [diff] [review]
Part 1. Refactor static functions to static member functions.
Comment 23 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-10 23:35:53 PDT
Created attachment 640941 [details] [diff] [review]
Part 2. Add A-GPS support.
Comment 24 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-12 03:45:52 PDT
Created attachment 641421 [details] [diff] [review]
Part 2. Add A-GPS support. v1.1

Address nits and fix compatibility on GB/ICS/ICS+
Comment 25 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-12 03:46:27 PDT
https://tbpl.mozilla.org/?tree=Try&rev=3fc52cef81f9 try build
Comment 26 Doug Turner (:dougt) 2012-07-16 09:37:35 PDT
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) 2012-07-16 09:46:03 PDT
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+8) 2012-07-16 14:46:18 PDT
(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+8) 2012-07-16 14:47:37 PDT
Created attachment 642741 [details] [diff] [review]
Part 1. Refactor static functions to static member functions.
Comment 30 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-16 14:48:21 PDT
Created attachment 642742 [details] [diff] [review]
Part 2. Add A-GPS support. v1.1
Comment 31 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-16 14:49:48 PDT
Created attachment 642744 [details] [diff] [review]
Part 2. Add A-GPS support. v1.1

Turn off debug.

Note You need to log in before you can comment on or make changes to this bug.