Closed Bug 780558 Opened 8 years ago Closed 8 years ago

B2G RIL: expose cell location LAC/CID

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Whiteboard: [LOE:S])

Attachments

(5 files, 5 obsolete files)

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.
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.
Corresponding Gaia issue is in https://github.com/mozilla-b2g/gaia/issues/3176 .
Assignee: nobody → vyang
Status: NEW → ASSIGNED
Attached patch Part 1: IDL changes (obsolete) — Splinter Review
Add an additional attribute `cell` of type nsIDOMMozMobileCellInfo to nsIDOMMozMobileConnectionInfo.
Attachment #649976 - Flags: superreview?(jonas)
Attachment #649976 - Flags: review?(philipp)
Attached patch Part 2: RIL implementation (obsolete) — Splinter Review
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)
This implementation is for GSM/WCDMA networks, and we'll need other attributes in nsIDOMMozMobileCellInfo for CDMA.
(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 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 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 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+
(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
(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.
(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.
Blocks: b2g-ril
This issue should also block basecamp as bug 778093.
blocking-basecamp: --- → ?
use gsmLocationAreaCode & gsmCellId
Attachment #649979 - Attachment is obsolete: true
Address comment #7
Attachment #649976 - Attachment is obsolete: true
Attachment #649976 - Flags: superreview?(jonas)
Attachment #650813 - Flags: superreview?(jonas)
Attached patch Part 2: RIL implementation : V2 (obsolete) — Splinter Review
Address comment #10
Attachment #649977 - Attachment is obsolete: true
Attachment #650814 - Flags: review?(philipp)
Attached patch Part 3: marionette test case (obsolete) — Splinter Review
Attachment #650815 - Flags: review?(philipp)
(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 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 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+
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+
(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 :)
(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.
Whiteboard: [LOE:L]
Whiteboard: [LOE:L] → [LOE:S]
Depends on bug 782338 for marionette test failure fix.
Depends on: 782338
Address Philipp's comment #19, #20, #21.
Attachment #650814 - Attachment is obsolete: true
Attachment #650815 - Attachment is obsolete: true
Corresponding QEmu GitHub pull request is in https://github.com/mozilla-b2g/platform_external_qemu/pull/3 .
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.
Target Milestone: mozilla17 → ---
@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?
(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!
Depends on: 852918
You need to log in before you can comment on or make changes to this bug.