1168064 - [B2G] Got wrong cell info when using GetCellInfoList

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Alphan Chen [:alchen]]

Attachment: logcat with RIL LOG

I file bug 1166174 for functionality.
In the same time, I used the workaround provided by Edgar.
(remove this line :https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#5632)

The patch can work.
However, I met the following symptom.

1. wrong cell info. (cid = -1)
I/GeckoConsole(  211): Alphan-NotifyGetCellInfoList: count=5
I/GeckoConsole(  211): Alphan-cell[0] type=1, registered=1
I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11701, cid=11032
I/GeckoConsole(  211): Alphan-cell[1] type=1, registered=0
I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11701, cid=11033
I/GeckoConsole(  211): Alphan-cell[2] type=1, registered=0
I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=12902, cid=15382
I/GeckoConsole(  211): Alphan-cell[3] type=1, registered=0
I/GeckoConsole(  211): Alphan-mcc=0, mnc=0, lac=0, cid=-1
I/GeckoConsole(  211): Alphan-cell[4] type=1, registered=0
I/GeckoConsole(  211): Alphan-mcc=0, mnc=0, lac=0, cid=-1


2. wrong cell info (mcc, mnc,lac, cid all wrong)
I/GeckoConsole(  211): Alphan-NotifyGetCellInfoList: count=2
I/GeckoConsole(  211): Alphan-cell[0] type=4, registered=1
I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11115, cid=9111196
I/GeckoConsole(  211): Alphan-cell[1] type=4, registered=0
I/GeckoConsole(  211): Alphan-mcc=2147483647, mnc=2147483647, lac=2147483647, cid=2147483647

Comment 1

[Alphan Chen [:alchen]]

Attachment: Test patch for getCellInfoList

Here is the patch for getCellInfoList.
You can trigger "getCellInfoList" by using "Geoloc..." app.
1. Open "Geoloc..." app
2. Use "Get current"

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to Alphan Chen [:alchen] from comment #0)
> Created attachment 8610058 [details]
> logcat with RIL LOG
> 
> I file bug 1166174 for functionality.
> In the same time, I used the workaround provided by Edgar.
> (remove this line
> :https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#5632)
> 
> The patch can work.
> However, I met the following symptom.
> 
> 1. wrong cell info. (cid = -1)
> I/GeckoConsole(  211): Alphan-NotifyGetCellInfoList: count=5
> I/GeckoConsole(  211): Alphan-cell[0] type=1, registered=1
> I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11701, cid=11032
> I/GeckoConsole(  211): Alphan-cell[1] type=1, registered=0
> I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11701, cid=11033
> I/GeckoConsole(  211): Alphan-cell[2] type=1, registered=0
> I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=12902, cid=15382
> I/GeckoConsole(  211): Alphan-cell[3] type=1, registered=0
> I/GeckoConsole(  211): Alphan-mcc=0, mnc=0, lac=0, cid=-1
> I/GeckoConsole(  211): Alphan-cell[4] type=1, registered=0
> I/GeckoConsole(  211): Alphan-mcc=0, mnc=0, lac=0, cid=-1
> 
> 
> 2. wrong cell info (mcc, mnc,lac, cid all wrong)
> I/GeckoConsole(  211): Alphan-NotifyGetCellInfoList: count=2
> I/GeckoConsole(  211): Alphan-cell[0] type=4, registered=1
> I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11115, cid=9111196
> I/GeckoConsole(  211): Alphan-cell[1] type=4, registered=0
> I/GeckoConsole(  211): Alphan-mcc=2147483647, mnc=2147483647,
> lac=2147483647, cid=2147483647

2147483647 is INT32_MAX, which means 'unknown', please refer to [1].
However, cid should not be -1, it should be INT32_MAX if it's unknown, I'll take a look at this case.

[1] http://hg.mozilla.org/mozilla-central/file/43f2f0c506ea/dom/mobileconnection/interfaces/nsICellInfo.idl

Comment 3

[Alphan Chen [:alchen]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> > 2. wrong cell info (mcc, mnc,lac, cid all wrong)
> > I/GeckoConsole(  211): Alphan-NotifyGetCellInfoList: count=2
> > I/GeckoConsole(  211): Alphan-cell[0] type=4, registered=1
> > I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11115, cid=9111196
> > I/GeckoConsole(  211): Alphan-cell[1] type=4, registered=0
> > I/GeckoConsole(  211): Alphan-mcc=2147483647, mnc=2147483647,
> > lac=2147483647, cid=2147483647
> 
> 2147483647 is INT32_MAX, which means 'unknown', please refer to [1].

Oh, I see.
But do we really need this info with all unknown value?
In my opinion, we should ignore this info before replying the result.

> However, cid should not be -1, it should be INT32_MAX if it's unknown, I'll
> take a look at this case.
>
Thanks.
 
> [1]
> http://hg.mozilla.org/mozilla-central/file/43f2f0c506ea/dom/mobileconnection/
> interfaces/nsICellInfo.idl

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Alphan Chen [:alchen] from comment #3)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> > > 2. wrong cell info (mcc, mnc,lac, cid all wrong)
> > > I/GeckoConsole(  211): Alphan-NotifyGetCellInfoList: count=2
> > > I/GeckoConsole(  211): Alphan-cell[0] type=4, registered=1
> > > I/GeckoConsole(  211): Alphan-mcc=466, mnc=97, lac=11115, cid=9111196
> > > I/GeckoConsole(  211): Alphan-cell[1] type=4, registered=0
> > > I/GeckoConsole(  211): Alphan-mcc=2147483647, mnc=2147483647,
> > > lac=2147483647, cid=2147483647
> > 
> > 2147483647 is INT32_MAX, which means 'unknown', please refer to [1].
> 
> Oh, I see.
> But do we really need this info with all unknown value?
> In my opinion, we should ignore this info before replying the result.

These are values reported by modem, we could help to filter them though, we just don't know which of the values are valid to gps? should we skip the cell info when all values are 'unknown' or?

> 
> > However, cid should not be -1, it should be INT32_MAX if it's unknown, I'll
> > take a look at this case.
> >
> Thanks.

I could reproduce this on TWM sim card, CHT sim card does not have this issue. Those are values reported by modem. We could filter the case of cid, since it is not in the valid range, however it is hard to filter the case of mcc, mnc and lac, since 0 is in the valid range.

D/RILC    (  312): [0132]< GET_CELL_INFO_LIST {[0: type=1,registered=1,timeStampType=3,timeStamp=68038396798 GSM id: mcc=466,mnc=97,lac=11701,cid=11033, gsmSS: ss=19,ber=99],[1: type=1,registered=0,timeStampType=3,timeStamp=68038396798 GSM id: mcc=466,mnc=97,lac=11701,cid=11032, gsmSS: ss=19,ber=99],[2: type=1,registered=0,timeStampType=3,timeStamp=68038396798 GSM id: mcc=466,mnc=97,lac=11701,cid=21742, gsmSS: ss=22,ber=99],[3: type=1,registered=0,timeStampType=3,timeStamp=68038396798 GSM id: mcc=466,mnc=97,lac=11701,cid=11031, gsmSS: ss=18,ber=99],[4: type=1,registered=0,timeStampType=3,timeStamp=68038396798 GSM id: mcc=0,mnc=0,lac=0,cid=-1, gsmSS: ss=18,ber=99],[5: type=1,registered=0,timeStampType=3,timeStamp=68038396798 GSM id: mcc=466,mnc=97,lac=11701,cid=21743, gsmSS: ss=18,ber=99]}

>  
> > [1]
> > http://hg.mozilla.org/mozilla-central/file/43f2f0c506ea/dom/mobileconnection/
> > interfaces/nsICellInfo.idl

Comment 5

[Alphan Chen [:alchen]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #4)
> (In reply to Alphan Chen [:alchen] from comment #3)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #2) 
> 
> These are values reported by modem, we could help to filter them though, we
> just don't know which of the values are valid to gps? should we skip the
> cell info when all values are 'unknown' or?
> 
Actually, the value is meaningless for gps.
We collect the those information combined with location to mozilla location service(MLS) as a database.
So if they are unknown, this data is not worthy to collect.
I don't think the data with unknown value is valid for anyone who want to use it.


> > 
> > > However, cid should not be -1, it should be INT32_MAX if it's unknown, I'll
> > > take a look at this case.
> > >
> > Thanks.
> 
> I could reproduce this on TWM sim card, CHT sim card does not have this
> issue. Those are values reported by modem. We could filter the case of cid,
> since it is not in the valid range, however it is hard to filter the case of
> mcc, mnc and lac, since 0 is in the valid range.
> 
Got it.
We can just filter this case by cid.


As a user, I would like to get valid data from the result.
Otherwise, I need to filter those garbage by my own.
In my opinion, it would be nice to cover this part in API level.
Thanks for your help.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Alphan Chen [:alchen] from comment #5)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #4)
> > (In reply to Alphan Chen [:alchen] from comment #3)
> > > (In reply to Jessica Jong [:jjong] [:jessica] from comment #2) 
> > 
> > These are values reported by modem, we could help to filter them though, we
> > just don't know which of the values are valid to gps? should we skip the
> > cell info when all values are 'unknown' or?
> > 
> Actually, the value is meaningless for gps.
> We collect the those information combined with location to mozilla location
> service(MLS) as a database.
> So if they are unknown, this data is not worthy to collect.
> I don't think the data with unknown value is valid for anyone who want to
> use it.

I think you misunderstood my question. What I meant is should we skip the cell info when all values are unknown or when some/any of the value is unknown? cause maybe the whole cell info is unsable when, for example, mcc/mnc is unknown.

For now, we'll just skip the cell info when _all_ values are unknown.

> 
> 
> > > 
> > > > However, cid should not be -1, it should be INT32_MAX if it's unknown, I'll
> > > > take a look at this case.
> > > >
> > > Thanks.
> > 
> > I could reproduce this on TWM sim card, CHT sim card does not have this
> > issue. Those are values reported by modem. We could filter the case of cid,
> > since it is not in the valid range, however it is hard to filter the case of
> > mcc, mnc and lac, since 0 is in the valid range.
> > 
> Got it.
> We can just filter this case by cid.
> 
> 
> As a user, I would like to get valid data from the result.
> Otherwise, I need to filter those garbage by my own.
> In my opinion, it would be nice to cover this part in API level.
> Thanks for your help.

Comment 7

[Alphan Chen [:alchen]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #6)
> (In reply to Alphan Chen [:alchen] from comment #5)
> > (In reply to Jessica Jong [:jjong] [:jessica] from comment #4)
> 
> I think you misunderstood my question. What I meant is should we skip the
> cell info when all values are unknown or when some/any of the value is
> unknown? cause maybe the whole cell info is unsable when, for example,
> mcc/mnc is unknown.


I really misunderstood the question. :(

The data we need are the following:
      "cellId":11032,
      "locationAreaCode":11701,
      "mobileCountryCode":466,
      "mobileNetworkCode":97,
      "asu":22

> 
> For now, we'll just skip the cell info when _all_ values are unknown.
> 

No problem.

Comment 8

[Jessica Jong [:jessica] (inactive)]

I'll take care of this!

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Edgar, may I have your review on this? Thanks.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8612769 [details] [diff] [review]
patch, v1.

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

Thanks for taking this, Jessica. Please see my comments below.

::: dom/mobileconnection/interfaces/nsICellInfo.idl
@@ +60,5 @@
>    readonly attribute long long timestamp;
>  };
>  
>  [scriptable, uuid(6345967c-61fc-45a1-8362-39e9261df052)]
>  interface nsIGsmCellInfo : nsICellInfo

The inheriting interfaces also requires an uuid bump.
You can use the mach tool in gecko folder, e.g. `./mach update-uuids nsICellInfo`, which will update nsICellInfo's uuid as well as any inheriting interfaces.

@@ +89,2 @@
>     */
>    readonly attribute long signalStrength;

What about just reusing the UNKNOWN_VALUE for signalStrength? I know the 99 is from the ril.h and also TS 27.007, but with this we could have a consistent interface design. What do you think?

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #10)
> Comment on attachment 8612769 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8612769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking this, Jessica. Please see my comments below.
> 
> ::: dom/mobileconnection/interfaces/nsICellInfo.idl
> @@ +60,5 @@
> >    readonly attribute long long timestamp;
> >  };
> >  
> >  [scriptable, uuid(6345967c-61fc-45a1-8362-39e9261df052)]
> >  interface nsIGsmCellInfo : nsICellInfo
> 
> The inheriting interfaces also requires an uuid bump.
> You can use the mach tool in gecko folder, e.g. `./mach update-uuids
> nsICellInfo`, which will update nsICellInfo's uuid as well as any inheriting
> interfaces.

Will do, thanks for the info.

> 
> @@ +89,2 @@
> >     */
> >    readonly attribute long signalStrength;
> 
> What about just reusing the UNKNOWN_VALUE for signalStrength? I know the 99
> is from the ril.h and also TS 27.007, but with this we could have a
> consistent interface design. What do you think?

Yes, we could use UNKNOWN_VALUE for all fields, it would be easier for users to identify it.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Edgar, I have addressed review comment 10 and also fix the case for neighboring cell. Could you take a look again? Thanks.

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8614542 [details] [diff] [review]
patch, v2.

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

Nice, thank you.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Thanks, Edgar.

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=833297eadd56

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/619668c3dbc2

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 17

[Jessica Jong [:jessica] (inactive)]

This has been landed in m-c:
https://hg.mozilla.org/mozilla-central/rev/619668c3dbc2

Comment 18

[Jessica Jong [:jessica] (inactive)]

Alphan, this has been fixed. Note that you'll still get unknown values for cell identity if there is a valid signal strength, since we filter out cell info when all values are unknown. Please let us know if you find any other issue. Thanks.