1032858 - Add support for RIL_REQUEST_GET_CELL_INFO_LIST to expose neighboring cell data

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hanno Schlichting]

+++ This bug was initially created as a clone of Bug #1010356 +++

As part of 1010356 we found two separate API's to get to neighboring cell data, mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1010356#c12

The now closed bug implemented support for one of them (via RIL_REQUEST_GET_NEIGHBORING_CELL_IDS). Support for RIL_REQUEST_GET_CELL_INFO_LIST is still missing. This requires changes to the RIL, using an updated AOSP version and making sure partner RIL's also support this.

Jessica, could you make sure this is filed correctly and assign to someone that can put this into the right backlog?

Comment 1

[Jessica Jong [:jessica] (inactive)]

(In reply to Hanno Schlichting [:hannosch] from comment #0)
> +++ This bug was initially created as a clone of Bug #1010356 +++
> 
> As part of 1010356 we found two separate API's to get to neighboring cell
> data, mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1010356#c12
> 
> The now closed bug implemented support for one of them (via
> RIL_REQUEST_GET_NEIGHBORING_CELL_IDS). Support for
> RIL_REQUEST_GET_CELL_INFO_LIST is still missing. This requires changes to
> the RIL, using an updated AOSP version and making sure partner RIL's also
> support this.
> 
> Jessica, could you make sure this is filed correctly and assign to someone
> that can put this into the right backlog?

Sure, I will take a look at this.

From Bug 1032865 comment 0, you mentioned that the implemented API (RIL_REQUEST_GET_NEIGHBORING_CELL_IDS) exposes incomplete cell ids for GSM and UMTS network. As I know, RIL_REQUEST_GET_CELL_INFO_LIST does return all of the current cell information, including serving and neighboring cell. However, the information exposed is almost the same as RIL_REQUEST_GET_NEIGHBORING_CELL_IDS for GSM/UMTS, it just adds a timestamp; other extra information, e.g. mcc/mnc, in CellIdentityGsm/CellIdentityWcdma are already known. So I wonder what is the difference between these two requests, besides from the support of CDMA and LTE cell information.

I am not familiar with how these cell info are used, so just asking to be sure. :)
Thanks.

Comment 2

[Hanno Schlichting]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #1)
> From Bug 1032865 comment 0, you mentioned that the implemented API
> (RIL_REQUEST_GET_NEIGHBORING_CELL_IDS) exposes incomplete cell ids for GSM
> and UMTS network. As I know, RIL_REQUEST_GET_CELL_INFO_LIST does return all
> of the current cell information, including serving and neighboring cell.
> However, the information exposed is almost the same as
> RIL_REQUEST_GET_NEIGHBORING_CELL_IDS for GSM/UMTS, it just adds a timestamp;
> other extra information, e.g. mcc/mnc, in CellIdentityGsm/CellIdentityWcdma
> are already known. So I wonder what is the difference between these two
> requests, besides from the support of CDMA and LTE cell information.

I'll try to give too much information, disregard what you already know :-)

My comment on incomplete data is based on observations we did with our Android based app (MozStumbler), so this might not apply directly to FxOS. It's also partly guesswork, so we might be wrong.

For our purposes we need a full unique identifier for each cell consisting of five pieces of information. Depending on the network type, these are slightly different but often exposed from the same APIs. The various new CellIdentity classes in Android describe these well. I've also tried to document them as part the service side implementation at http://mozilla-ichnaea.readthedocs.org/en/latest/cell.html.

For GSM:
network type, mcc, mnc, 16bit location area code, 16bit cell id

For UMTS/WCDMA:
network type, mcc, mnc, 16bit location area code, 28bit cell identity

For LTE:
network type, mcc, mnc, 16bit tracking area code, 28bit cell identity

For CDMA:
network type, mcc, system id, network id, base station id

The mcc and mnc are the ones of each actual cell. With multiple SIM cards and roaming, these can be different from the mcc/mnc of the SIM card. And if you are close to a country border you also sometimes see cell from both countries, so different mobile country codes and potentially different mobile network codes.

What we observed so far, is that the RIL_REQUEST_GET_NEIGHBORING_CELL_IDS based API's for GSM/UMTS often only give you the mcc/mnc and for UMTS the additional primary scrambling code. But you don't actually get the location area or cell id from these.

On the other hand the RIL_REQUEST_GET_CELL_INFO_LIST based API's seem to at least sometimes give you those additional pieces. In our Android app, this seems to be dependent on the Android version, radio chipset and radio device driver. For example Nexus 4 / 5 devices with a recent Android version often seem to have this data. Many other devices only give incomplete records via either API.

We haven't done a real formal study of this, so this is partly guesswork and we might be wrong. If you actually find any code that might be responsible for this behavior, I'd be grateful for any hints or explanations.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Thank you Hanno for the throughout explanation!

It seems that RIL_REQUEST_GET_CELL_INFO_LIST does expose a little bit more information. But as you mentioned, it is dependent on the Android version, radio chipset and radio device driver.

I am testing on Flame, and RIL_REQUEST_GET_CELL_INFO_LIST returns with some information. I will try to finish the implementation in gecko, then we can compare the results. :)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v1.

Hi, this is the interface for the cell info list request. I made it a little bit simpler by combining Android's cell identity and signal strength. Please let me know if this works for you. Thanks.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8450860 [details] [diff] [review]
Part 1: interface changes, v1.

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

Looks good :)

::: dom/mobileconnection/interfaces/nsICellInfo.idl
@@ +39,5 @@
> +
> +  /*
> +   * Registration state of this cell. !0 if this cell is registered.
> +   */
> +  readonly attribute long registered;

What are the valid values for "registered?"

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +92,5 @@
>     * Cell Info functionality.
>     */
>    void getNeighboringCellIds(in nsINeighboringCellIdsCallback callback);
> +
> +  void getCellInfoList(in nsICellInfoListCallback callback);

As an API user, the names of the two functions are ambiguous. Could we add some comments mentioning the differentiation, e.g. getNeighboringCellIds is for acquiring gsm and umts?

I do want to merge these two functions into one as it seems to me |getCellInfoList| cover all the information |getNeighboringCellIds| provide. However, I am convinced by Jessica's concern that considering partner vendor capability, we don't have a good way to detect which command is supported. It's safer to deprecate |getNeighboringCellIds| later.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Comment on attachment 8450860 [details] [diff] [review]
> Part 1: interface changes, v1.
> 
> Review of attachment 8450860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good :)

Thank you Hsinyi for the feedback!

> 
> ::: dom/mobileconnection/interfaces/nsICellInfo.idl
> @@ +39,5 @@
> > +
> > +  /*
> > +   * Registration state of this cell. !0 if this cell is registered.
> > +   */
> > +  readonly attribute long registered;
> 
> What are the valid values for "registered?"

Valid values are 0 and 1. Maybe we should use boolean instead?

> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +92,5 @@
> >     * Cell Info functionality.
> >     */
> >    void getNeighboringCellIds(in nsINeighboringCellIdsCallback callback);
> > +
> > +  void getCellInfoList(in nsICellInfoListCallback callback);
> 
> As an API user, the names of the two functions are ambiguous. Could we add
> some comments mentioning the differentiation, e.g. getNeighboringCellIds is
> for acquiring gsm and umts?

Sure, will add some comments to make it clear.

> 
> I do want to merge these two functions into one as it seems to me
> |getCellInfoList| cover all the information |getNeighboringCellIds| provide.
> However, I am convinced by Jessica's concern that considering partner vendor
> capability, we don't have a good way to detect which command is supported.
> It's safer to deprecate |getNeighboringCellIds| later.

You are right, |getCellInfoList| can cover all the information |getNeighboringCellIds| provide. Do you mean to deprecate |getNeighboringCellIds| or to provide both APIs but merge the implementation in ril? If we had a reference phone for ril, we could focus the implementation on that phone. :)

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #6)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> > Comment on attachment 8450860 [details] [diff] [review]
> > Part 1: interface changes, v1.
> > 
> > Review of attachment 8450860 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good :)
> 
> Thank you Hsinyi for the feedback!
> 
> > 
> > ::: dom/mobileconnection/interfaces/nsICellInfo.idl
> > @@ +39,5 @@
> > > +
> > > +  /*
> > > +   * Registration state of this cell. !0 if this cell is registered.
> > > +   */
> > > +  readonly attribute long registered;
> > 
> > What are the valid values for "registered?"
> 
> Valid values are 0 and 1. Maybe we should use boolean instead?
> 

Boolean seems proper! Please do :)

> > 
> > I do want to merge these two functions into one as it seems to me
> > |getCellInfoList| cover all the information |getNeighboringCellIds| provide.
> > However, I am convinced by Jessica's concern that considering partner vendor
> > capability, we don't have a good way to detect which command is supported.
> > It's safer to deprecate |getNeighboringCellIds| later.
> 
> You are right, |getCellInfoList| can cover all the information
> |getNeighboringCellIds| provide. Do you mean to deprecate
> |getNeighboringCellIds| or to provide both APIs but merge the implementation
> in ril? If we had a reference phone for ril, we could focus the
> implementation on that phone. :)

Ideally, I'd like to see we have only one general API eventually and that means we need to merge the implementation in ril if needed.

Comment 8

[Hanno Schlichting]

Comment on attachment 8450860 [details] [diff] [review]
Part 1: interface changes, v1.

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

Looks really good to me. The API exposes everything we'll need for location services. Thanks!

Comment 9

[Jessica Jong [:jessica] (inactive)]

(In reply to Hanno Schlichting [:hannosch] from comment #8)
> Comment on attachment 8450860 [details] [diff] [review]
> Part 1: interface changes, v1.
> 
> Review of attachment 8450860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks really good to me. The API exposes everything we'll need for location
> services. Thanks!

Thanks for the feedback. The implementation will be based on this interface. :)

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v2.

Changes since v1:
- use boolean instead of long for attribute 'registered'.
- add comments to cell info APIs.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v1.

Here is the implementation. Hsinyi, may I have your review?

Comment 12

[Jessica Jong [:jessica] (inactive)]

While writing the test case, I found that this request will crash emulator's rild because RIL_REQUEST_GET_CELL_INFO_LIST (request number 109) is not defined in the ril commands [1]. However, if we add RIL_REQUEST_GET_CELL_INFO_LIST in ril_commands.h by adding:
|{RIL_REQUEST_GET_CELL_INFO_LIST, dispatchVoid, responseCellInfoList}|,
we will need to backport the whole patch [2], which is a huge.
I am thinking whether to do all this here or leave it to Bug 1035089, when we add support for RIL_REQUEST_GET_CELL_INFO_LIST on the emulator.


[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/libril/ril_commands.h#L127
[2] https://android.googlesource.com/platform/hardware/ril/+/8a9e02161271505de274db0c3a88087056dd5dfc

Comment 13

[Hanno Schlichting]

One more thing related to the signal strength comments. I'm guessing you copied over the explanations for the various signal strength values from a lower level interface? If that's the case, than this is probably all we can do.

If that's not the case, than we should look at those values a bit more. Signal strength is often ill-defined and Android's high-level API's get things wrong quite a bit. In practice it is most often in the wonderfully named Arbitrary Strength Unit (ASU, http://en.wikipedia.org/wiki/Mobile_phone_signal#ASU), which has different ranges for GSM/UMTS/LTE and is based on different standards, not just TS 27.007 8.5. For a nice little rant about all the things which are wrong in Android see for example https://groups.google.com/forum/#!topic/android-platform/xeFvdEA14oA

Probably nothing we can do here to fix the mess Android made, just thought I mention this.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #12)
> While writing the test case, I found that this request will crash emulator's
> rild because RIL_REQUEST_GET_CELL_INFO_LIST (request number 109) is not
> defined in the ril commands [1]. However, if we add
> RIL_REQUEST_GET_CELL_INFO_LIST in ril_commands.h by adding:
> |{RIL_REQUEST_GET_CELL_INFO_LIST, dispatchVoid, responseCellInfoList}|,
> we will need to backport the whole patch [2], which is a huge.

The advantage of backporting this patch is that it does fake a simple response (only one cell info) for RIL_REQUEST_GET_CELL_INFO_LIST in reference-ril [3]. So, we may actually have a test case to verify it.

[3] https://android.googlesource.com/platform/hardware/ril/+/8a9e02161271505de274db0c3a88087056dd5dfc%5E!/#F8

> I am thinking whether to do all this here or leave it to Bug 1035089, when
> we add support for RIL_REQUEST_GET_CELL_INFO_LIST on the emulator.
> 
> 
> [1]
> https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/libril/
> ril_commands.h#L127
> [2]
> https://android.googlesource.com/platform/hardware/ril/+/
> 8a9e02161271505de274db0c3a88087056dd5dfc

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Hanno Schlichting [:hannosch] from comment #13)
> One more thing related to the signal strength comments. I'm guessing you
> copied over the explanations for the various signal strength values from a
> lower level interface? If that's the case, than this is probably all we can
> do.
> 
> If that's not the case, than we should look at those values a bit more.
> Signal strength is often ill-defined and Android's high-level API's get
> things wrong quite a bit. In practice it is most often in the wonderfully
> named Arbitrary Strength Unit (ASU,
> http://en.wikipedia.org/wiki/Mobile_phone_signal#ASU), which has different
> ranges for GSM/UMTS/LTE and is based on different standards, not just TS
> 27.007 8.5. For a nice little rant about all the things which are wrong in
> Android see for example
> https://groups.google.com/forum/#!topic/android-platform/xeFvdEA14oA
> 
> Probably nothing we can do here to fix the mess Android made, just thought I
> mention this.

Hi Hanno, thanks for the information.
Yes, the interface is based on the lower level interface defined in ril.h [1], and it is what we get from the lower level (modem). It is strange though, that the rssi in RIL_NeighboringCell does differentiate between GSM and UMTS, but the signalStrength in RIL_GW_SignalStrength, RIL_SignalStrengthWcdma and RIL_LTE_SignalStrength, which should be newer, all have the same range.
And from my observations on Flame, for RIL_REQUEST_GET_CELL_INFO_LIST, the signal strength reported for GSM and UMTS does range between (0-31).

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/b2g-jellybean/include/telephony/ril.h#L635-713

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8452208 [details] [diff] [review]
Part 1: interface changes, v2.

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

Sorry for some naming changes. Except them, looks good!

::: dom/mobileconnection/interfaces/nsICellInfo.idl
@@ +34,5 @@
> +
> +  /**
> +   * Network type. One of the CELL_INFO_TYPE_* constants.
> +   */
> +  readonly attribute long cellInfoType;

s/cellInfoType/type  as this is exposed to nsICellInfo...

@@ +206,5 @@
> +
> +  /**
> +   * 28-bit Cell Identity, 0..268435455, INT_MAX if unknown.
> +   */
> +  readonly attribute long ci;

s/ci/cid

@@ +211,5 @@
> +
> +  /**
> +   * Physical cell id, 0..503, INT_MAX if unknown.
> +   */
> +  readonly attribute long pci;

s/pci/pcid

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8452211 [details] [diff] [review]
Part 2: implementation, v1.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +104,5 @@
>  const RADIO_POWER_OFF_TIMEOUT = 30000;
>  const SMS_HANDLED_WAKELOCK_TIMEOUT = 5000;
>  const HW_DEFAULT_CLIENT_ID = 0;
>  
> +const INT_MAX_VALUE = 2147483647;

s/INT_MAX_VALUE/INT32_MAX/

@@ +1091,5 @@
> +};
> +
> +function CellInfo() {}
> +CellInfo.prototype = {
> +  networkType: null,

netowrkType? or cellInfoType?

@@ +1093,5 @@
> +function CellInfo() {}
> +CellInfo.prototype = {
> +  networkType: null,
> +  registered: false,
> +  timestampType: 0,

please use Ci.nsICellInfo.TIMESTAMP_TYPE_UNKNOWN

@@ +1105,5 @@
> +  classID: GSMCELLINFO_CID,
> +  classInfo: XPCOMUtils.generateCI({
> +    classID:          GSMCELLINFO_CID,
> +    classDescription: "GsmCellInfo",
> +    flags:            Ci.nsIClassInfo.DOM_OBJECT,

Looks we don't need this flag as we don't expose this info object to DOM.

@@ +1127,5 @@
> +  classID: WCDMACELLINFO_CID,
> +  classInfo: XPCOMUtils.generateCI({
> +    classID:          WCDMACELLINFO_CID,
> +    classDescription: "WcdmaCellInfo",
> +    flags:            Ci.nsIClassInfo.DOM_OBJECT,

ditto.

@@ +1150,5 @@
> +  classID: LTECELLINFO_CID,
> +  classInfo: XPCOMUtils.generateCI({
> +    classID:          LTECELLINFO_CID,
> +    classDescription: "LteCellInfo",
> +    flags:            Ci.nsIClassInfo.DOM_OBJECT,

ditto.

@@ +1158,5 @@
> +  // nsILteCellInfo
> +
> +  mcc: INT_MAX_VALUE,
> +  mnc: INT_MAX_VALUE,
> +  ci: INT_MAX_VALUE,

s/ci/cid

@@ +1159,5 @@
> +
> +  mcc: INT_MAX_VALUE,
> +  mnc: INT_MAX_VALUE,
> +  ci: INT_MAX_VALUE,
> +  pci: INT_MAX_VALUE,

s/pci/pcid

@@ +1177,5 @@
> +  classID: CDMACELLINFO_CID,
> +  classInfo: XPCOMUtils.generateCI({
> +    classID:          CDMACELLINFO_CID,
> +    classDescription: "CdmaCellInfo",
> +    flags:            Ci.nsIClassInfo.DOM_OBJECT,

ditto.

::: dom/system/gonk/ril_worker.js
@@ +6448,5 @@
> +  let cellInfoList = [];
> +  let num = Buf.readInt32();
> +  for (let i = 0; i < num; i++) {
> +    let cellInfo = {};
> +    cellInfo.cellInfoType = Buf.readInt32();

nit: s/cellInfoType/type/

@@ +6463,5 @@
> +        cellInfo.cid = Buf.readInt32();
> +        if (cellInfo.cellInfoType == CELL_INFO_TYPE_WCDMA) {
> +          cellInfo.psc = Buf.readInt32();
> +        }
> +

nit: remove the empty line, maybe?

@@ +6473,5 @@
> +        cellInfo.systemId = Buf.readInt32();
> +        cellInfo.basestationId = Buf.readInt32();
> +        cellInfo.longitude = Buf.readInt32();
> +        cellInfo.latitude = Buf.readInt32();
> +

ditto.

@@ +6486,5 @@
> +        cellInfo.mnc = Buf.readInt32();
> +        cellInfo.ci = Buf.readInt32();
> +        cellInfo.pci = Buf.readInt32();
> +        cellInfo.tac = Buf.readInt32();
> +

ditto.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v3.

Changes since v2:
- s/cellInfoType/type
- s/ci/cid
- s/pci/pcid

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v2.

Thank you, hsinyi. May I have your review again?

Changes since v1:
- s/INT_MAX_VALUE/INT32_MAX/
- s/cellInfoType/type
- s/ci/cid
- s/pci/pcid
- Use Ci.nsICellInfo.TIMESTAMP_TYPE_UNKNOWN as the default value for timestampType
- Remove Ci.nsIClassInfo.DOM_OBJECT flag
- Remove extra empty lines

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8456036 [details] [diff] [review]
Part 2: implementation, v2.

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

yayaya

Comment 21

[Jessica Jong [:jessica] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=233c9650ebfa

try green!

Comment 22

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #14)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #12)
> > While writing the test case, I found that this request will crash emulator's
> > rild because RIL_REQUEST_GET_CELL_INFO_LIST (request number 109) is not
> > defined in the ril commands [1]. However, if we add
> > RIL_REQUEST_GET_CELL_INFO_LIST in ril_commands.h by adding:
> > |{RIL_REQUEST_GET_CELL_INFO_LIST, dispatchVoid, responseCellInfoList}|,
> > we will need to backport the whole patch [2], which is a huge.
> 
> The advantage of backporting this patch is that it does fake a simple
> response (only one cell info) for RIL_REQUEST_GET_CELL_INFO_LIST in
> reference-ril [3]. So, we may actually have a test case to verify it.
> 
> [3]
> https://android.googlesource.com/platform/hardware/ril/+/
> 8a9e02161271505de274db0c3a88087056dd5dfc%5E!/#F8
> 
> > I am thinking whether to do all this here or leave it to Bug 1035089, when
> > we add support for RIL_REQUEST_GET_CELL_INFO_LIST on the emulator.
> > 
> > 
> > [1]
> > https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/libril/
> > ril_commands.h#L127
> > [2]
> > https://android.googlesource.com/platform/hardware/ril/+/
> > 8a9e02161271505de274db0c3a88087056dd5dfc

Per offline discussion with Hsinyi, since the current implementation of requestGetCellInfoList() in reference-ril.c is not quite right, we will reimplement it in Bug 1035089 and attach test cases by then.

Comment 23

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/6474f3075112
https://hg.mozilla.org/integration/b2g-inbound/rev/e69bc093068c

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6474f3075112
https://hg.mozilla.org/mozilla-central/rev/e69bc093068c