Last Comment Bug 780558 - B2G RIL: expose cell location LAC/CID
: B2G RIL: expose cell location LAC/CID
Status: RESOLVED FIXED
[LOE:S]
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
:
Mentors:
Depends on: 782338 852918
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2012-08-05 23:41 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2013-03-20 03:51 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Part 1: IDL changes (2.04 KB, patch)
2012-08-07 23:40 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: RIL implementation (13.53 KB, patch)
2012-08-07 23:46 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: feedback+
Details | Diff | Splinter Review
[DO NOT SUBMIT] for gaia lock screen verification (495 bytes, patch)
2012-08-07 23:53 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
[DO NOT SUBMIT] for gaia lock screen verification : V2 (1.10 KB, patch)
2012-08-10 00:35 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: IDL changes : V2 (2.61 KB, patch)
2012-08-10 00:37 PDT, Vicamo Yang [:vicamo][:vyang]
jonas: superreview+
Details | Diff | Splinter Review
Part 2: RIL implementation : V2 (12.66 KB, patch)
2012-08-10 00:39 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 3: marionette test case (1.22 KB, patch)
2012-08-10 00:39 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: RIL implementation : V3 (13.94 KB, patch)
2012-08-14 21:13 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 3: marionette test case : V2 (2.07 KB, patch)
2012-08-14 21:14 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 4: add MozMobileCellInfo to global interfaces list (943 bytes, patch)
2012-08-14 23:00 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review

Description Vicamo Yang [:vicamo][:vyang] 2012-08-05 23:41:56 PDT
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.
Comment 1 Vicamo Yang [:vicamo][:vyang] 2012-08-06 00:32:18 PDT
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.
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-08-06 00:34:39 PDT
Corresponding Gaia issue is in https://github.com/mozilla-b2g/gaia/issues/3176 .
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-08-07 23:40:12 PDT
Created attachment 649976 [details] [diff] [review]
Part 1: IDL changes

Add an additional attribute `cell` of type nsIDOMMozMobileCellInfo to nsIDOMMozMobileConnectionInfo.
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-08-07 23:46:56 PDT
Created attachment 649977 [details] [diff] [review]
Part 2: RIL implementation

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`.
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-08-07 23:48:32 PDT
This implementation is for GSM/WCDMA networks, and we'll need other attributes in nsIDOMMozMobileCellInfo for CDMA.
Comment 6 Vicamo Yang [:vicamo][:vyang] 2012-08-07 23:53:31 PDT
Created attachment 649979 [details] [diff] [review]
[DO NOT SUBMIT] for gaia lock screen verification
Comment 7 Philipp von Weitershausen [:philikon] 2012-08-08 11:31:47 PDT
(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 Philipp von Weitershausen [:philikon] 2012-08-08 11:32:34 PDT
Comment on attachment 649976 [details] [diff] [review]
Part 1: IDL changes

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

See comment 7.
Comment 9 Philipp von Weitershausen [:philikon] 2012-08-08 11:34:06 PDT
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 Philipp von Weitershausen [:philikon] 2012-08-08 11:38:41 PDT
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
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-08-08 20:40:42 PDT
(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
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-08-08 20:41:49 PDT
(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.
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-08-08 22:35:14 PDT
(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.
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-08-08 22:56:22 PDT
This issue should also block basecamp as bug 778093.
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-08-10 00:35:11 PDT
Created attachment 650812 [details] [diff] [review]
[DO NOT SUBMIT] for gaia lock screen verification : V2

use gsmLocationAreaCode & gsmCellId
Comment 16 Vicamo Yang [:vicamo][:vyang] 2012-08-10 00:37:03 PDT
Created attachment 650813 [details] [diff] [review]
Part 1: IDL changes : V2

Address comment #7
Comment 17 Vicamo Yang [:vicamo][:vyang] 2012-08-10 00:39:11 PDT
Created attachment 650814 [details] [diff] [review]
Part 2: RIL implementation : V2

Address comment #10
Comment 18 Vicamo Yang [:vicamo][:vyang] 2012-08-10 00:39:56 PDT
Created attachment 650815 [details] [diff] [review]
Part 3: marionette test case
Comment 19 Philipp von Weitershausen [:philikon] 2012-08-12 17:54:59 PDT
(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 Philipp von Weitershausen [:philikon] 2012-08-12 18:03:40 PDT
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...
Comment 21 Philipp von Weitershausen [:philikon] 2012-08-12 18:05:51 PDT
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.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-08-13 17:55:08 PDT
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?
Comment 23 Vicamo Yang [:vicamo][:vyang] 2012-08-13 19:32:31 PDT
(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 :)
Comment 24 Vicamo Yang [:vicamo][:vyang] 2012-08-13 19:44:18 PDT
(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.
Comment 25 Vicamo Yang [:vicamo][:vyang] 2012-08-14 20:45:35 PDT
Depends on bug 782338 for marionette test failure fix.
Comment 26 Vicamo Yang [:vicamo][:vyang] 2012-08-14 21:13:30 PDT
Created attachment 651990 [details] [diff] [review]
Part 2: RIL implementation : V3

Address Philipp's comment #19, #20, #21.
Comment 27 Vicamo Yang [:vicamo][:vyang] 2012-08-14 21:14:02 PDT
Created attachment 651991 [details] [diff] [review]
Part 3: marionette test case : V2
Comment 28 Vicamo Yang [:vicamo][:vyang] 2012-08-14 21:16:52 PDT
Corresponding QEmu GitHub pull request is in https://github.com/mozilla-b2g/platform_external_qemu/pull/3 .
Comment 30 Phil Ringnalda (:philor) 2012-08-14 22:22:49 PDT
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.
Comment 31 Vicamo Yang [:vicamo][:vyang] 2012-08-14 23:00:51 PDT
Created attachment 652007 [details] [diff] [review]
Part 4: add MozMobileCellInfo to global interfaces list
Comment 32 Vicamo Yang [:vicamo][:vyang] 2012-08-15 05:15:59 PDT
trying https://tbpl.mozilla.org/?tree=Try&rev=c579f4827015
Comment 35 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-08-25 03:24:05 PDT
@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 Philipp von Weitershausen [:philikon] 2012-08-25 09:25:17 PDT
(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!

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