Closed Bug 715788 Opened 9 years ago Closed 8 years ago

Add A-GPS support for gonk

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla17
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: cjones, Assigned: kanru)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 11 obsolete files)

10.08 KB, patch
Details | Diff | Splinter Review
15.04 KB, patch
Details | Diff | Splinter Review
We'll want a-gps support the day after we land gps.
Assignee: nobody → kchen
Attached patch WIP patch (obsolete) — Splinter Review
Depends on: 736941
We need to get IMSI, MSISDN, mnc, mcc, lac (GSM only) and cid from RIL. We also need translation between APN and cid.
Depends on: 738558
Blocks: b2g-ril
Component: DOM: Events → DOM: Device Interfaces
QA Contact: events → device-interfaces
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
(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?
(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.
Whiteboard: [b2g:blocking+]
Priority: -- → P1
Since its priority is P1 now, 
I'll start to work on Bug 738558 to provide LAC and CID.

thanks
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Attached patch WIP patch (obsolete) — Splinter Review
Can get network information from RIL now.
Attachment #607115 - Attachment is obsolete: true
Depends on: 762426
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.
(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.
Depends on: 762760
Attached patch Part 2. Add A-GPS support. (obsolete) — Splinter Review
WIP.
Attachment #630499 - Attachment is obsolete: true
At a minimum, the new prefs need documenting. Haven't looked yet to see what else will.
Keywords: dev-doc-needed
The weird thing is, the gps driver on nexus-s does not request data connection at all..
(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?
(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.
Attached patch Add A-GPS support. (obsolete) — Splinter Review
WIP.

Use new RilContext interface to get ril data.
Attachment #632133 - Attachment is obsolete: true
Attachment #632132 - Attachment is obsolete: true
Attachment #640569 - Flags: review?(doug.turner)
Attached patch Part 2. Add A-GPS support. (obsolete) — Splinter Review
Some bugs need to be filed.
Attachment #635233 - Attachment is obsolete: true
Attachment #640570 - Flags: review?(doug.turner)
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 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.
(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.
Depends on: 772747
Depends on: 772748
Depends on: 772750
Attachment #640569 - Attachment is obsolete: true
Attachment #640569 - Flags: review?(doug.turner)
Attachment #640940 - Flags: review?(doug.turner)
Attached patch Part 2. Add A-GPS support. (obsolete) — Splinter Review
Attachment #640570 - Attachment is obsolete: true
Attachment #640570 - Flags: review?(doug.turner)
Attachment #640941 - Flags: review?(doug.turner)
Attached patch Part 2. Add A-GPS support. v1.1 (obsolete) — Splinter Review
Address nits and fix compatibility on GB/ICS/ICS+
Attachment #640941 - Attachment is obsolete: true
Attachment #640941 - Flags: review?(doug.turner)
Attachment #641421 - Flags: review?(doug.turner)
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.
Attachment #640940 - Flags: review?(doug.turner) → review+
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.
Attachment #641421 - Flags: review?(doug.turner) → review+
(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.
Attached patch Part 2. Add A-GPS support. v1.1 (obsolete) — Splinter Review
Attachment #641421 - Attachment is obsolete: true
Turn off debug.
Attachment #642742 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/039a4f5025e7
https://hg.mozilla.org/mozilla-central/rev/68f6f8dbb06d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
No longer depends on: 841360
You need to log in before you can comment on or make changes to this bug.