Closed
Bug 780558
Opened 12 years ago
Closed 12 years ago
B2G RIL: expose cell location LAC/CID
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: vicamo, Assigned: vicamo)
References
Details
(Whiteboard: [LOE:S])
Attachments
(5 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
sicking
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Cell location is required in some countries like Brazil to display local networking information and it's now only reachable in the internal interface nsIRilContext, which is the type of `nsIRadioInterfaceLayer.rilContext`.
Brazil rules every mobile phone should provide a certain "network info widget" with different content based on network connection state. The widget MUST show the information in two lines:
First line : PLMN
Second line: <operator> <state> <area code>
The <operator> string is determined by MCC+MNC code; the <area code> is cell location LAC; the <state> is a string determined also by cell location LAC.
Assignee | ||
Comment 1•12 years ago
|
||
Cell location information is embedded in the response of Gonk ril parcel REQUEST_VOICE_REGISTRATION_STATE. RIL will have to enable location updates through REQUEST_SET_LOCATION_UPDATES first and disable it when no longer necessary. The Android framework has four APIs for related functions. They are:
1. enableSingleLocationUpdate
2. enableLocationUpdates
3. disableSingleLocationUpdate
4. disableLocationUpdates
An additional notifier notifyLocationChanged() is then invoked whenever an unsolicited REQUEST_VOICE_REGISTRATION_STATE parcel appears.
Assignee | ||
Comment 2•12 years ago
|
||
Corresponding Gaia issue is in https://github.com/mozilla-b2g/gaia/issues/3176 .
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vyang
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
Add an additional attribute `cell` of type nsIDOMMozMobileCellInfo to nsIDOMMozMobileConnectionInfo.
Attachment #649976 -
Flags: superreview?(jonas)
Attachment #649976 -
Flags: review?(philipp)
Assignee | ||
Comment 4•12 years ago
|
||
1. remove ril_worker message `celllocationchanged` for it will be integrated into {voice,data}registrationstatechange. Accommodate GonkGPSGeolocationProvider to the change as well.
2. implement new attribute `cell`.
Attachment #649977 -
Flags: review?(philipp)
Assignee | ||
Comment 5•12 years ago
|
||
This implementation is for GSM/WCDMA networks, and we'll need other attributes in nsIDOMMozMobileCellInfo for CDMA.
Assignee | ||
Comment 6•12 years ago
|
||
Comment 7•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo] from comment #5)
> This implementation is for GSM/WCDMA networks, and we'll need other
> attributes in nsIDOMMozMobileCellInfo for CDMA.
Do we know what they are? Would be nice to spell those out. Also, I wonder whether we should prefix them (gsmLocationAreaCode, gsmCellId).
Comment 8•12 years ago
|
||
Comment on attachment 649976 [details] [diff] [review]
Part 1: IDL changes
Review of attachment 649976 [details] [diff] [review]:
-----------------------------------------------------------------
See comment 7.
Attachment #649976 -
Flags: review?(philipp)
Comment 9•12 years ago
|
||
Comment on attachment 649976 [details] [diff] [review]
Part 1: IDL changes
Review of attachment 649976 [details] [diff] [review]:
-----------------------------------------------------------------
Also, don't forget to rev the UUID of nsIDOMMozMobileConnectionInfo!
Comment 10•12 years ago
|
||
Comment on attachment 649977 [details] [diff] [review]
Part 2: RIL implementation
Review of attachment 649977 [details] [diff] [review]:
-----------------------------------------------------------------
Obligatory question: tests?
::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +374,5 @@
> }
> + nsCOMPtr<nsIDOMMozMobileConnectionInfo> voice;
> + rilCtx->GetVoice(getter_AddRefs(voice));
> + if (voice) {
> + nsCOMPtr<nsIDOMMozMobileCellInfo> cell;
Pretty evil hack. :)
You're taking advantage of the fact that RadioInterfaceLayer::rilContext looks like the DOM objects (nsIDOMMozMobileConnectionInfo, nsIDOMMozMobileCellInfo). You should at the very least document this here and also in RadioInterfaceLayer.js (where we define rilContext) in comments.
::: dom/system/gonk/RILContentHelper.js
@@ +129,5 @@
> +
> + // nsIDOMMozMobileCellInfo
> +
> + lac: -1,
> + cid: -1
Default values should probably be `null`.
::: dom/system/gonk/RadioInterfaceLayer.js
@@ +518,5 @@
> voiceInfo.roaming != isRoaming ||
> + voiceInfo.type != radioTech ||
> + !voiceInfo.cell ||
> + voiceInfo.cell.lac != state.lac ||
> + voiceInfo.cell.cid != state.cid) {
Please rebase your patch, this code has recently changed (see bug 777057).
::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ -229,1 @@
> [scriptable, uuid(a6f6977e-f4ee-42b4-ae79-798c8c47c360)]
rev this UUID
Attachment #649977 -
Flags: review?(philipp) → feedback+
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> (In reply to Vicamo Yang [:vicamo] from comment #5)
> > This implementation is for GSM/WCDMA networks, and we'll need other
> > attributes in nsIDOMMozMobileCellInfo for CDMA.
>
> Do we know what they are? Would be nice to spell those out. Also, I wonder
> whether we should prefix them (gsmLocationAreaCode, gsmCellId).
For CDMA, there are another five different attributes (see 3GPP2 C.S0005-E):
1. BASE_ID: base station identification, unsigned decimal of 16 bits
2. BASE_LAT: base station latitude, signed decimal of 22 bits
3. BASE_LONG: base station longitude, signed decimal of 23 bits
4. AP_SID: access point system identification, unsigned decimal of 15 bits
5. AP_NID: access point network identification, unsigned decimal of 16 bits
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> Comment on attachment 649976 [details] [diff] [review]
> Part 1: IDL changes
>
> Review of attachment 649976 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Also, don't forget to rev the UUID of nsIDOMMozMobileConnectionInfo!
Ahh! I will, thank you.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 649977 [details] [diff] [review]
> Part 2: RIL implementation
>
> Review of attachment 649977 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Obligatory question: tests?
>
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +374,5 @@
> > }
> > + nsCOMPtr<nsIDOMMozMobileConnectionInfo> voice;
> > + rilCtx->GetVoice(getter_AddRefs(voice));
> > + if (voice) {
> > + nsCOMPtr<nsIDOMMozMobileCellInfo> cell;
>
> Pretty evil hack. :)
>
> You're taking advantage of the fact that RadioInterfaceLayer::rilContext
> looks like the DOM objects (nsIDOMMozMobileConnectionInfo,
> nsIDOMMozMobileCellInfo). You should at the very least document this here
> and also in RadioInterfaceLayer.js (where we define rilContext) in comments.
Aren't there already comments above for voice/data objects? {voice,data}.network also does the same trick and there is no comments for nsIDOMMozMobileNetoworkInfo either. Since they're both child attributes of nsIDOMMozMobileConnectionInfo, I thought the original comments are enough. I can update that if you still feel uncomfortable with this. Thank you.
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +518,5 @@
> > voiceInfo.roaming != isRoaming ||
> > + voiceInfo.type != radioTech ||
> > + !voiceInfo.cell ||
> > + voiceInfo.cell.lac != state.lac ||
> > + voiceInfo.cell.cid != state.cid) {
>
> Please rebase your patch, this code has recently changed (see bug 777057).
Thanks for your kindly remind. I will.
Assignee | ||
Comment 14•12 years ago
|
||
This issue should also block basecamp as bug 778093.
blocking-basecamp: --- → ?
Assignee | ||
Comment 15•12 years ago
|
||
use gsmLocationAreaCode & gsmCellId
Attachment #649979 -
Attachment is obsolete: true
Assignee | ||
Comment 16•12 years ago
|
||
Address comment #7
Attachment #649976 -
Attachment is obsolete: true
Attachment #649976 -
Flags: superreview?(jonas)
Attachment #650813 -
Flags: superreview?(jonas)
Assignee | ||
Comment 17•12 years ago
|
||
Address comment #10
Attachment #649977 -
Attachment is obsolete: true
Attachment #650814 -
Flags: review?(philipp)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #650815 -
Flags: review?(philipp)
Comment 19•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo] from comment #13)
> > > + nsCOMPtr<nsIDOMMozMobileConnectionInfo> voice;
> > > + rilCtx->GetVoice(getter_AddRefs(voice));
> > > + if (voice) {
> > > + nsCOMPtr<nsIDOMMozMobileCellInfo> cell;
> >
> > Pretty evil hack. :)
> >
> > You're taking advantage of the fact that RadioInterfaceLayer::rilContext
> > looks like the DOM objects (nsIDOMMozMobileConnectionInfo,
> > nsIDOMMozMobileCellInfo). You should at the very least document this here
> > and also in RadioInterfaceLayer.js (where we define rilContext) in comments.
>
> Aren't there already comments above for voice/data objects?
You're absolutely right. However, that comment doesn't point out that there's native code that *expects* these interfaces to be fulfilled. Maybe you can just amend the comment to point that out and also mention nsIDOMMozMobileCellInfo? Thanks!
Comment 20•12 years ago
|
||
Comment on attachment 650814 [details] [diff] [review]
Part 2: RIL implementation : V2
Review of attachment 650814 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments/questions below as well as comment 19 addressed.
::: dom/system/gonk/RadioInterfaceLayer.js
@@ +505,5 @@
> + voiceInfo.cell = null;
> + } else {
> + voiceInfo.cell = {
> + gsmLocationAreaCode: newInfo.lac,
> + gsmCellId: newInfo.cid
Registration info can change often during, would be nice if we didn't have to throw away the object every time, e.g.:
if (!voiceInfo.cell) {
voiceInfo.cell = {};
}
voiceInfo.cell.gsmLocationAreaCode = newInfo.lac;
voiceInfo.cell.gsmCellId: newInfo.cid;
Alternatively, you could make `newInfo` already have the right layout by changing _processCREG() in ril_worker.js accordingly.
::: dom/system/gonk/ril_worker.js
@@ +2247,5 @@
> }
>
> + // From TS 23.003, 0000 and 0xfffe are indicated that no valid LAI exists
> + // in MS. So we still need to report the '0000' as well.
> + let lac = parseInt(newState[1], 16);
Can this be NaN? For others values in the `newState` list, we use RIL.parseInt() which can return a default value if the string doesn't yield a valid number...
Attachment #650814 -
Flags: review?(philipp) → review+
Comment 21•12 years ago
|
||
Comment on attachment 650815 [details] [diff] [review]
Part 3: marionette test case
Review of attachment 650815 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/network/tests/marionette/test_mobile_voice_state.js
@@ +27,5 @@
> + let cell = voice.cell;
> + // Android emulator always reports [lac, cid] as [-1, -1].
> + // See external/qemu/telephony/android_modem.c:490, function amodem_reset().
> + is(cell.gsmLocationAreaCode, 65535, "GSM Location Area Code");
> + is(cell.gsmCellId, 4294967295, "GSM Cell Identification");
Should we report -1 as `null` perhaps, rather than as the max value of whatever integer size the value happens to be?
Also, this of course isn't a complete test case, but at least it's a good start and probably the most we can do without modifying the emulator's mock modem.
Attachment #650815 -
Flags: review?(philipp) → review+
Updated•12 years ago
|
blocking-basecamp: ? → +
Comment on attachment 650813 [details] [diff] [review]
Part 1: IDL changes : V2
Review of attachment 650813 [details] [diff] [review]:
-----------------------------------------------------------------
Should we also expect information about if the user is roaming or not? Or is it expected that gaia will know which network policy the user has?
Attachment #650813 -
Flags: superreview?(jonas) → superreview+
Assignee | ||
Comment 23•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #22)
> Comment on attachment 650813 [details] [diff] [review]
> Part 1: IDL changes : V2
>
> Review of attachment 650813 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Should we also expect information about if the user is roaming or not? Or is
> it expected that gaia will know which network policy the user has?
Roaming info is available through `nsIDOMMozMobileConnectionInfo.roaming` and the cell location info at the time will be available through newly added `nsIDOMMozMobileConnectionInfo.cell` attribute. Thanks :)
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> Comment on attachment 650815 [details] [diff] [review]
> Part 3: marionette test case
>
> Review of attachment 650815 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/network/tests/marionette/test_mobile_voice_state.js
> @@ +27,5 @@
> > + let cell = voice.cell;
> > + // Android emulator always reports [lac, cid] as [-1, -1].
> > + // See external/qemu/telephony/android_modem.c:490, function amodem_reset().
> > + is(cell.gsmLocationAreaCode, 65535, "GSM Location Area Code");
> > + is(cell.gsmCellId, 4294967295, "GSM Cell Identification");
>
> Should we report -1 as `null` perhaps, rather than as the max value of
> whatever integer size the value happens to be?
>
> Also, this of course isn't a complete test case, but at least it's a good
> start and probably the most we can do without modifying the emulator's mock
> modem.
There's a little problem in implementing qemu console commands to modify cell location at runtime. That is, I know little about the underlying transition process. Is there any event sequence that I'll have to implement at the same time? I have done a very simple command `gsm location <lac> <ci>` for interactive test but is still fighting with the naughty emualator.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:L]
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:L] → [LOE:S]
Assignee | ||
Comment 25•12 years ago
|
||
Depends on bug 782338 for marionette test failure fix.
Depends on: 782338
Assignee | ||
Comment 26•12 years ago
|
||
Address Philipp's comment #19, #20, #21.
Attachment #650814 -
Attachment is obsolete: true
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #650815 -
Attachment is obsolete: true
Assignee | ||
Comment 28•12 years ago
|
||
Corresponding QEmu GitHub pull request is in https://github.com/mozilla-b2g/platform_external_qemu/pull/3 .
Assignee | ||
Comment 29•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/a154933f2bef
http://hg.mozilla.org/integration/mozilla-inbound/rev/cec9aff342dc
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfba38edc205
Target Milestone: --- → mozilla17
Comment 30•12 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/70bee2ddfa2f - http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_interfaces.html?force=1 thinks that it knows about interfaces in global scope, and it was quite surprised to find a MozMobileCellInfo interface there.
Assignee | ||
Comment 31•12 years ago
|
||
Assignee | ||
Comment 32•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Target Milestone: mozilla17 → ---
Assignee | ||
Comment 33•12 years ago
|
||
Comment 34•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/26b91c24f901
https://hg.mozilla.org/mozilla-central/rev/90c19283f1b2
https://hg.mozilla.org/mozilla-central/rev/011c3d6769d0
https://hg.mozilla.org/mozilla-central/rev/7440244d7237
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 35•12 years ago
|
||
@vicamo, After digging into a phone connected to the Brazil mobile network, I found that can't see nsIDOMMozMobileCellInfo in the cell attribute. Is there anything wrong?
Comment 36•12 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #35)
> @vicamo, After digging into a phone connected to the Brazil mobile network,
> I found that can't see nsIDOMMozMobileCellInfo in the cell attribute. Is
> there anything wrong?
This bug has been fixed. Please file a new bug for any problems you see. You know the drill, set DEBUG_ALL = true gecko/dom/system/gonk/ril_consts.js, rebuild + reflash gecko, and attach logcat to the new bug. Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•