Closed Bug 1010356 Opened 7 years ago Closed 6 years ago

Network location provider should try to send neighboring cell data

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S5 (4july)
tracking-b2g backlog

People

(Reporter: hschlichting, Assigned: jessica)

References

Details

(Whiteboard: [p=2])

Attachments

(3 files, 6 obsolete files)

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.
The RIL needs to support this. 
Trying to assign this to RIL component, but it isn't showing in the component list in bugzilla.
Component: Geolocation → RIL
Product: Core → Firefox OS
Jessica, can you please help this? I think we need to get neighbor cell information through RIL_REQUEST_GET_NEIGHBORING_CELL_IDS.
blocking-b2g: --- → backlog
Flags: needinfo?(jjong)
Sure. We'll add support for RIL_REQUEST_GET_NEIGHBORING_CELL_IDS.
Assignee: nobody → jjong
Flags: needinfo?(jjong)
Attached patch Part 1: interface changes. (obsolete) — Splinter Review
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.
Attachment #8441958 - Flags: feedback?(htsai)
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.
Attachment #8441958 - Flags: feedback?(htsai) → feedback+
Attached patch Part 1: interface changes, v2. (obsolete) — Splinter Review
Attachment #8441958 - Attachment is obsolete: true
Attached patch Part 2: implementation, v1. (obsolete) — Splinter Review
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.
Attachment #8444285 - Flags: review?(htsai)
Attachment #8444286 - Flags: review?(htsai)
Attached patch Part 3: test cases, v1. (obsolete) — Splinter Review
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.
(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
(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?
(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 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
Attachment #8444285 - Flags: review?(htsai)
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. :)
Attached patch Part 1: interface changes, v3. (obsolete) — Splinter Review
Changes since v2 (address review comment 15):
- add another callback function for failed cases
- refine comments
Attachment #8444285 - Attachment is obsolete: true
Attachment #8445656 - Flags: review?(htsai)
Attached patch Part 2: implementation, v2. (obsolete) — Splinter Review
Changes since v1:
- call the new function introduce in "Part 1, v3" on failed cases.
Attachment #8444286 - Attachment is obsolete: true
Attachment #8444286 - Flags: review?(htsai)
Attachment #8445657 - Flags: review?(htsai)
Comment on attachment 8445656 [details] [diff] [review]
Part 1: interface changes, v3.

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

Looks great!
Attachment #8445656 - Flags: review?(htsai) → review+
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.
Attachment #8445656 - Attachment is obsolete: true
Attachment #8445680 - Flags: review+
Changes since v1:
- modify test cases to match the new interface/function introduced in "Part 1, v3".
Attachment #8444313 - Attachment is obsolete: true
Attachment #8445681 - Flags: review?(htsai)
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?
Attachment #8445657 - Flags: review?(htsai) → review+
(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?
(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 on attachment 8445681 [details] [diff] [review]
Part 3: test cases, v2.

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

Nice!
Attachment #8445681 - Flags: review?(htsai) → review+
Changes since v2:
- remove the part where we set networkType to null if it's undefined, see comment 23.
Attachment #8445657 - Attachment is obsolete: true
Attachment #8445737 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=8ef10e1c0f57

try green!
Keywords: checkin-needed
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S5 (4july)
Blocks: 1032858
Blocks: 1032865
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.