Closed
Bug 715788
Opened 13 years ago
Closed 12 years ago
Add A-GPS support for gonk
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: cjones, Assigned: kanru)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 11 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
We'll want a-gps support the day after we land gps.
Reporter | ||
Updated•13 years ago
|
Blocks: b2g-product-phone
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → kchen
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
We need to get IMSI, MSISDN, mnc, mcc, lac (GSM only) and cid from RIL. We also need translation between APN and cid.
Updated•13 years ago
|
Blocks: b2g-ril
Component: DOM: Events → DOM: Device Interfaces
QA Contact: events → device-interfaces
Reporter | ||
Comment 3•13 years ago
|
||
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•13 years ago
|
||
(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?
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Updated•13 years ago
|
Whiteboard: [b2g:blocking+]
Updated•13 years ago
|
Priority: -- → P1
Since its priority is P1 now,
I'll start to work on Bug 738558 to provide LAC and CID.
thanks
Updated•13 years ago
|
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
Assignee | ||
Comment 7•13 years ago
|
||
Can get network information from RIL now.
Attachment #607115 -
Attachment is obsolete: true
Comment 8•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
(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.
Assignee | ||
Comment 10•13 years ago
|
||
Comment 12•13 years ago
|
||
At a minimum, the new prefs need documenting. Haven't looked yet to see what else will.
Keywords: dev-doc-needed
Assignee | ||
Comment 13•13 years ago
|
||
The weird thing is, the gps driver on nexus-s does not request data connection at all..
Comment 14•13 years ago
|
||
(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?
Assignee | ||
Comment 15•13 years ago
|
||
(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.
Assignee | ||
Comment 16•13 years ago
|
||
WIP.
Use new RilContext interface to get ril data.
Attachment #632133 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #632132 -
Attachment is obsolete: true
Attachment #640569 -
Flags: review?(doug.turner)
Assignee | ||
Comment 18•12 years ago
|
||
Some bugs need to be filed.
Attachment #635233 -
Attachment is obsolete: true
Attachment #640570 -
Flags: review?(doug.turner)
Comment 19•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #640569 -
Attachment is obsolete: true
Attachment #640569 -
Flags: review?(doug.turner)
Attachment #640940 -
Flags: review?(doug.turner)
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #640570 -
Attachment is obsolete: true
Attachment #640570 -
Flags: review?(doug.turner)
Attachment #640941 -
Flags: review?(doug.turner)
Assignee | ||
Comment 24•12 years ago
|
||
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)
Assignee | ||
Comment 25•12 years ago
|
||
Comment 26•12 years ago
|
||
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 27•12 years ago
|
||
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+
Assignee | ||
Comment 28•12 years ago
|
||
(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.
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #640940 -
Attachment is obsolete: true
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #641421 -
Attachment is obsolete: true
Assignee | ||
Comment 31•12 years ago
|
||
Turn off debug.
Attachment #642742 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 32•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/039a4f5025e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f6f8dbb06d
Flags: in-testsuite-
Keywords: checkin-needed
Comment 33•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/039a4f5025e7
https://hg.mozilla.org/mozilla-central/rev/68f6f8dbb06d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•