780558 - B2G RIL: expose cell location LAC/CID

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

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]]

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]]

Corresponding Gaia issue is in https://github.com/mozilla-b2g/gaia/issues/3176 .

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: IDL changes

Add an additional attribute `cell` of type nsIDOMMozMobileCellInfo to nsIDOMMozMobileConnectionInfo.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 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]]

This implementation is for GSM/WCDMA networks, and we'll need other attributes in nsIDOMMozMobileCellInfo for CDMA.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [DO NOT SUBMIT] for gaia lock screen verification

Comment 7

[Philipp von Weitershausen [:philikon]]

(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]]

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]]

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]]

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]]

(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]]

(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]]

(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]]

This issue should also block basecamp as bug 778093.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [DO NOT SUBMIT] for gaia lock screen verification : V2

use gsmLocationAreaCode & gsmCellId

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: IDL changes : V2

Address comment #7

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: RIL implementation : V2

Address comment #10

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: marionette test case

Comment 19

[Philipp von Weitershausen [:philikon]]

(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]]

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]]

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]

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]]

(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]]

(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]]

Depends on bug 782338 for marionette test failure fix.

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: RIL implementation : V3

Address Philipp's comment #19, #20, #21.

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: marionette test case : V2

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Corresponding QEmu GitHub pull request is in https://github.com/mozilla-b2g/platform_external_qemu/pull/3 .

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 30

[Phil Ringnalda (:philor)]

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]]

Attachment: Part 4: add MozMobileCellInfo to global interfaces list

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

trying https://tbpl.mozilla.org/?tree=Try&rev=c579f4827015

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b91c24f901
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c19283f1b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/011c3d6769d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7440244d7237

Comment 34

[Ed Morley [:emorley]]

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

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

@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]]

(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!