The default bug view has changed. See this FAQ.

B2G RIL: expose cell location LAC/CID

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

Trunk
mozilla17
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(5 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 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

5 years ago
Corresponding Gaia issue is in https://github.com/mozilla-b2g/gaia/issues/3176 .
(Assignee)

Updated

5 years ago
Assignee: nobody → vyang
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 649976 [details] [diff] [review]
Part 1: IDL changes

Add an additional attribute `cell` of type nsIDOMMozMobileCellInfo to nsIDOMMozMobileConnectionInfo.
Attachment #649976 - Flags: superreview?(jonas)
Attachment #649976 - Flags: review?(philipp)
(Assignee)

Comment 4

5 years ago
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`.
Attachment #649977 - Flags: review?(philipp)
(Assignee)

Comment 5

5 years ago
This implementation is for GSM/WCDMA networks, and we'll need other attributes in nsIDOMMozMobileCellInfo for CDMA.
(Assignee)

Comment 6

5 years ago
Created attachment 649979 [details] [diff] [review]
[DO NOT SUBMIT] for gaia lock screen verification
(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+
(Assignee)

Comment 11

5 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

5 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

5 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)

Updated

5 years ago
Blocks: 710489
(Assignee)

Comment 14

5 years ago
This issue should also block basecamp as bug 778093.
blocking-basecamp: --- → ?
(Assignee)

Comment 15

5 years ago
Created attachment 650812 [details] [diff] [review]
[DO NOT SUBMIT] for gaia lock screen verification : V2

use gsmLocationAreaCode & gsmCellId
Attachment #649979 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 650813 [details] [diff] [review]
Part 1: IDL changes : V2

Address comment #7
Attachment #649976 - Attachment is obsolete: true
Attachment #649976 - Flags: superreview?(jonas)
Attachment #650813 - Flags: superreview?(jonas)
(Assignee)

Comment 17

5 years ago
Created attachment 650814 [details] [diff] [review]
Part 2: RIL implementation : V2

Address comment #10
Attachment #649977 - Attachment is obsolete: true
Attachment #650814 - Flags: review?(philipp)
(Assignee)

Comment 18

5 years ago
Created attachment 650815 [details] [diff] [review]
Part 3: marionette test case
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+
(Assignee)

Comment 23

5 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

5 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

5 years ago
Whiteboard: [LOE:L]
(Assignee)

Updated

5 years ago
Whiteboard: [LOE:L] → [LOE:S]
(Assignee)

Comment 25

5 years ago
Depends on bug 782338 for marionette test failure fix.
Depends on: 782338
(Assignee)

Comment 26

5 years ago
Created attachment 651990 [details] [diff] [review]
Part 2: RIL implementation : V3

Address Philipp's comment #19, #20, #21.
Attachment #650814 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 651991 [details] [diff] [review]
Part 3: marionette test case : V2
Attachment #650815 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
Corresponding QEmu GitHub pull request is in https://github.com/mozilla-b2g/platform_external_qemu/pull/3 .
(Assignee)

Comment 29

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

5 years ago
Created attachment 652007 [details] [diff] [review]
Part 4: add MozMobileCellInfo to global interfaces list
(Assignee)

Comment 32

5 years ago
trying https://tbpl.mozilla.org/?tree=Try&rev=c579f4827015
(Assignee)

Updated

5 years ago
Target Milestone: mozilla17 → ---
(Assignee)

Comment 33

5 years ago
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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.