1010356 - Network location provider should try to send neighboring cell data

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hanno Schlichting]

The current code in http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js only uses information from the currently connected voice cell connection.

Better position estimates can be achieved based on tri-lateration of multiple visible cells. In order to support this, information about neighboring cells have to be sent to the service.

Earlier experiments tried to initiate a "cell scan" for additional cells and use the scan result. Initiating a scan caused the active voice/data connection to break, which is not acceptable.

To fix this issue there might be missing underlying RIL API's, which would need to be exposed.

Comment 1

[:garvan]

The RIL needs to support this. 
Trying to assign this to RIL component, but it isn't showing in the component list in bugzilla.

Comment 2

[Ken Chang[:kenkai]]

Jessica, can you please help this? I think we need to get neighbor cell information through RIL_REQUEST_GET_NEIGHBORING_CELL_IDS.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Sure. We'll add support for RIL_REQUEST_GET_NEIGHBORING_CELL_IDS.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes.

I am not sure where is the best place to put neighboringCells, under nsIRilContext or as a function under nsIRadioInterface? Hsinyi, may I have your opinion? Thanks.

Comment 5

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

Comment on attachment 8441958 [details] [diff] [review]
Part 1: interface changes.

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

I think nsIRilContext is the right place for now. After Edgar's IPDL work, the new attribute would be moved to a new gonk interface.

::: dom/mobileconnection/interfaces/nsIMobileCellInfo.idl
@@ +23,5 @@
>     */
>    readonly attribute long long gsmCellId;
>  
>    /**
> +   * Primary Scrambling Code for WCDMA networks.

Please add "(PSC)" after |Primary Scrambling Code| to have a clear definition of the abbreviation.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v2.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v1.

Comment 8

[Jessica Jong [:jessica] (inactive)]

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

Hsinyi, sorry for changing the interface again. As discussed, I am putting the 'getNeighboringCellIds()' under nsIRadioInterface, and decided not to use 'nsIMobileCellInfo' but add a new interface 'nsINeighboringCellInfo' instead.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: test cases, v1.

Comment 10

[Hanno Schlichting]

From my high-level point of view, this looks pretty good already. Jessica, did you look at CDMA networks as well? For those types of networks, our location service code would need to know the base station id, network id and system id of neighboring cells. I admit, I have no idea how this actually works on the RIL layer, so this might not be possible to add into the same interface.

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Hanno Schlichting [:hannosch] from comment #10)
> From my high-level point of view, this looks pretty good already. Jessica,
> did you look at CDMA networks as well? For those types of networks, our
> location service code would need to know the base station id, network id and
> system id of neighboring cells. I admit, I have no idea how this actually
> works on the RIL layer, so this might not be possible to add into the same
> interface.

Mmm.. it seems that neighboring cell list in CDMA networks is not supported yet, please refer to the RIL_NeighboringCell struct [1] defined in AOSP.

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L330-L341

Comment 12

[Hanno Schlichting]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #11)
> Mmm.. it seems that neighboring cell list in CDMA networks is not supported
> yet, please refer to the RIL_NeighboringCell struct [1] defined in AOSP.
> 
> [1]
> https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L330-L341

Ok, I looked a bit more and I think I know what's going on. Android has two sets of API's to get to neighboring cell information. On the high-level there's the older http://developer.android.com/reference/android/telephony/TelephonyManager.html#getNeighboringCellInfo%28%29 which only supports GSM/WCDMA. The underlying implementation of this is what seems to be available in our version of AOSP.

For Android API level 17 a new set of API's was introduced to also support CDMA and LTE. The high level API is http://developer.android.com/reference/android/telephony/TelephonyManager.html#getAllCellInfo%28%29 which returns various network type specific entities like CellInfoGsm, CellInfoCdma, CellIdentityCdma and CellSignalStrengthCdma. There's one info, identity and signal strength object per network type (gsm, wcdma, cdma, lte).

I've looked at the AOSP RIL code, and there's a number of changes from around July 2013. One of those is https://android.googlesource.com/platform/hardware/ril/+/83b1d8d9af8f7f7bf424af6faf229cfc04e0c896%5E2..83b1d8d9af8f7f7bf424af6faf229cfc04e0c896/ where include/telephony/ril.h was extended by all these network specific info, identity and signal strength structs, and also new RIL codes, for example "define RIL_REQUEST_GET_CELL_INFO_LIST 109" was introduced.

Going forward I think we want to split this issue into two. One about the simpler change of exposing the RIL_NeighboringCell information. Since a lot of phones are on GSM/WCDMA networks, this is helpful in itself. But in the longer term we also want to get support for CDMA/LTE and probably need the new API's and changes on the AOSP level to get this.

Does this make sense?

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Hanno Schlichting [:hannosch] from comment #12)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #11)
> > Mmm.. it seems that neighboring cell list in CDMA networks is not supported
> > yet, please refer to the RIL_NeighboringCell struct [1] defined in AOSP.
> > 
> > [1]
> > https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/
> > telephony/ril.h#L330-L341
> 
> Ok, I looked a bit more and I think I know what's going on. Android has two
> sets of API's to get to neighboring cell information. On the high-level
> there's the older
> http://developer.android.com/reference/android/telephony/TelephonyManager.
> html#getNeighboringCellInfo%28%29 which only supports GSM/WCDMA. The
> underlying implementation of this is what seems to be available in our
> version of AOSP.
> 
> For Android API level 17 a new set of API's was introduced to also support
> CDMA and LTE. The high level API is
> http://developer.android.com/reference/android/telephony/TelephonyManager.
> html#getAllCellInfo%28%29 which returns various network type specific
> entities like CellInfoGsm, CellInfoCdma, CellIdentityCdma and
> CellSignalStrengthCdma. There's one info, identity and signal strength
> object per network type (gsm, wcdma, cdma, lte).
> 
> I've looked at the AOSP RIL code, and there's a number of changes from
> around July 2013. One of those is
> https://android.googlesource.com/platform/hardware/ril/+/
> 83b1d8d9af8f7f7bf424af6faf229cfc04e0c896%5E2..
> 83b1d8d9af8f7f7bf424af6faf229cfc04e0c896/ where include/telephony/ril.h was
> extended by all these network specific info, identity and signal strength
> structs, and also new RIL codes, for example "define
> RIL_REQUEST_GET_CELL_INFO_LIST 109" was introduced.
> 
> Going forward I think we want to split this issue into two. One about the
> simpler change of exposing the RIL_NeighboringCell information. Since a lot
> of phones are on GSM/WCDMA networks, this is helpful in itself. But in the
> longer term we also want to get support for CDMA/LTE and probably need the
> new API's and changes on the AOSP level to get this.
> 
> Does this make sense?

Yes, you are totally right, sorry for not checking further. Took at glance on AOSP's implementation and getAllCellInfo() relies on the implementation of RIL_REQUEST_GET_CELL_INFO_LIST, which is available since b2g-jellybean [1] for us. Besides from gecko ril's implementation, we have to check whether vendor support this request or not.

I agree with you in that we can expose RIL_NeighboringCell information first, and add support for RIL_REQUEST_GET_CELL_INFO_LIST as follow-up.

Thanks for your findings. :)

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

Comment 14

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

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

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

::: dom/mobileconnection/interfaces/nsINeighboringCellInfo.idl
@@ +9,5 @@
> +{
> +  /**
> +   * result is an array of nsINeighboringCellInfo.
> +   */
> +  void neighboringCellIdsResult(in jsval result);

Since it's a callback for the request "getNeighboringCellIds" I think we need to notify both success and failed cases. Otherwise, how could the caller know if the request is done or not?

How about

void notifyGetNeighboringCellIds(in jsval result);
void notifyGetNeighboringCellIdsFailed();

@@ +16,5 @@
> +[scriptable, uuid(87dc222e-abb3-4342-95bf-626aa19fa20e)]
> +interface nsINeighboringCellInfo: nsISupports
> +{
> +  /**
> +   * Type of connection.

Type of radio, maybe?

@@ +49,5 @@
> +  readonly attribute long wcdmaPsc;
> +
> +  /**
> +   * In GSM, signalStrength is the received rssi.
> +   * In WCDMA, signalStrength is the level index of CPICH Received Signal Code Power.

Please also comment the possible ranges, thank you.

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +87,5 @@
>  
>    void getSmscAddress(in nsIMobileMessageCallback request);
> +
> +  /**
> +   * Misc.

Are you going to complete this? XD

Comment 15

[Jessica Jong [:jessica] (inactive)]

Thanks for the review, Hsinyi.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Comment on attachment 8444285 [details] [diff] [review]
> Part 1: interface changes, v2.
> 
> Review of attachment 8444285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/interfaces/nsINeighboringCellInfo.idl
> @@ +9,5 @@
> > +{
> > +  /**
> > +   * result is an array of nsINeighboringCellInfo.
> > +   */
> > +  void neighboringCellIdsResult(in jsval result);
> 
> Since it's a callback for the request "getNeighboringCellIds" I think we
> need to notify both success and failed cases. Otherwise, how could the
> caller know if the request is done or not?
> 
> How about
> 
> void notifyGetNeighboringCellIds(in jsval result);
> void notifyGetNeighboringCellIdsFailed();

Oh, per my implementation (part 2), an empty array is returned on failed cases. But it is clearer this way, thanks for the suggestion.

> 
> @@ +16,5 @@
> > +[scriptable, uuid(87dc222e-abb3-4342-95bf-626aa19fa20e)]
> > +interface nsINeighboringCellInfo: nsISupports
> > +{
> > +  /**
> > +   * Type of connection.
> 
> Type of radio, maybe?

Mmm.. how about 'Network Type'?

> 
> @@ +49,5 @@
> > +  readonly attribute long wcdmaPsc;
> > +
> > +  /**
> > +   * In GSM, signalStrength is the received rssi.
> > +   * In WCDMA, signalStrength is the level index of CPICH Received Signal Code Power.
> 
> Please also comment the possible ranges, thank you.

Will do.

> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +87,5 @@
> >  
> >    void getSmscAddress(in nsIMobileMessageCallback request);
> > +
> > +  /**
> > +   * Misc.
> 
> Are you going to complete this? XD

Haha, okay, will change to something more specific. :)

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v3.

Changes since v2 (address review comment 15):
- add another callback function for failed cases
- refine comments

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v2.

Changes since v1:
- call the new function introduce in "Part 1, v3" on failed cases.

Comment 18

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

Comment on attachment 8445656 [details] [diff] [review]
Part 1: interface changes, v3.

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

Looks great!

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v4.

Changes since v3:
- Per offline discussion with Hsinyi, remove "level index" in the comment of signalStrength for wcdma networks, to reflect real-life cases accurately.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: test cases, v2.

Changes since v1:
- modify test cases to match the new interface/function introduced in "Part 1, v3".

Comment 21

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

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

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

Thank you!!!

::: dom/system/gonk/ril_worker.js
@@ +6412,5 @@
> +  let num = Buf.readInt32();
> +
> +  for (let i = 0; i < num; i++) {
> +    let cellId = {};
> +    cellId.networkType = GECKO_RADIO_TECH[radioTech] || null;

I don't get "|| null". Can you explain?

Comment 22

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> Comment on attachment 8445657 [details] [diff] [review]
> Part 2: implementation, v2.
> 
> Review of attachment 8445657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you!!!
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +6412,5 @@
> > +  let num = Buf.readInt32();
> > +
> > +  for (let i = 0; i < num; i++) {
> > +    let cellId = {};
> > +    cellId.networkType = GECKO_RADIO_TECH[radioTech] || null;
> 
> I don't get "|| null". Can you explain?

Oh, this is to use 'null' in case 'undefined' is returned, which is not likely to happen and would be wrong if it happens. Should we keep it?

Comment 23

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #22)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> > Comment on attachment 8445657 [details] [diff] [review]
> > Part 2: implementation, v2.
> > 
> > Review of attachment 8445657 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thank you!!!
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +6412,5 @@
> > > +  let num = Buf.readInt32();
> > > +
> > > +  for (let i = 0; i < num; i++) {
> > > +    let cellId = {};
> > > +    cellId.networkType = GECKO_RADIO_TECH[radioTech] || null;
> > 
> > I don't get "|| null". Can you explain?
> 
> Oh, this is to use 'null' in case 'undefined' is returned, which is not
> likely to happen and would be wrong if it happens. Should we keep it?

I see, thank you. Then, not a strong opinion but I think we could remove it. If 'undefined' is returned, then it's a bug somewhere else. Better to fix this in the right place.

Comment 24

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

Comment on attachment 8445681 [details] [diff] [review]
Part 3: test cases, v2.

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

Nice!

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v3.

Changes since v2:
- remove the part where we set networkType to null if it's undefined, see comment 23.

Comment 26

[Jessica Jong [:jessica] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=8ef10e1c0f57

try green!

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/844b150a37e6
https://hg.mozilla.org/integration/b2g-inbound/rev/fa5bb1d382d0
https://hg.mozilla.org/integration/b2g-inbound/rev/7bd3451fd0bc

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/844b150a37e6
https://hg.mozilla.org/mozilla-central/rev/fa5bb1d382d0
https://hg.mozilla.org/mozilla-central/rev/7bd3451fd0bc