[wasabi][CDMA] Unable to automatically set correct APN due to gecko is not returning correct MCC and MNC number.

VERIFIED FIXED in Firefox 28

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: echu, Assigned: gwang)

Tracking

unspecified
1.3 Sprint 6 - 12/6
x86
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

(Whiteboard: [FT:RIL])

Attachments

(2 attachments, 13 obsolete attachments)

3.97 KB, patch
Details | Diff | Splinter Review
7.13 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #904984 +++

Need to add APN settings of CDMA carriers to the database. For example Sprint in the US and in Asia Pacific Telecom in Taiwan.

Based on bug 904984 comment 20, Gecko part should be checked for this bug.
(Reporter)

Updated

5 years ago
blocking-b2g: --- → koi?
Summary: Need to add APN settings of CDMA carriers to the database for QA testing → [wasabi] Need to add APN settings of CDMA carriers to the database for QA testing
Whiteboard: [FT:RIL]

Comment 1

5 years ago
according to RIL triage meeting, since this is not in v1.2 user story, move to 1.3?.
blocking-b2g: koi? → 1.3?
According to comment #20 from bug 904984 [1] the issue is because gecko is not returning the operator numeric value (MCC and MNC codes) correctly, is that right? If so IMHO It could be better to change the tittle of the bug to reflect better what the issue is.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=904984#c20
(Reporter)

Comment 3

5 years ago
Thanks for your suggestion jaoo, I've updated the title.
Summary: [wasabi] Need to add APN settings of CDMA carriers to the database for QA testing → [wasabi][CDMA] Unable to automatically set correct APN due to gecko is not returning correct MCC and MNC number.

Updated

5 years ago
Assignee: nobody → echen
(Assignee)

Updated

5 years ago
Assignee: echen → gwang
(Assignee)

Comment 4

5 years ago
It's because
(Assignee)

Comment 5

5 years ago
It's due to CDMA RUIM need another way to read IMSI.
The origin REQUEST_IMSI function is just for SIM/USIM.

We need to add GET_IMSI and parsing functions for CDMA.
If CDMA IMSI is correct, the original "parseMccMncFromImsi" function should work.

PS. EF_IMSI_T = 0x6F23
    EF_IMSI_M = 0x6F22

    // EF_IMSI_T(M) Encoding from TIA/EIA-95-B 6.3.1 and RUIM 3GPP2 C.S0023-C 3.4.3/3.4.2
    // 1st byte: IMSI_Class (IMSI is 15 digits if class 0; less than 15 digits if class 1)
    // 2nd and 3rd bytes: IMSI_S2 (2 bytes)
    // 4th to 6th bytes: IMSI_S1 (3 bytes) 
    // 7th byte: IMSI_11_12 (1 byte)
    // 8th byte: IMSI_Programmed and IMSI_Addr_Num
    // 9th and 10th bytes: IMSI_MCC

    //IMSI = MCC + NMSI
    //MCC = IMSI_MCC (3 digit
    //NMSI = MNC + MSIN
    //MNC = IMSI_11_12 (extra digits are stored here if IMSI is larger than 12 digits, 7 bits, 1 byte)
    //MSIN = MIN
    //MIN = IMSI_S2 + IMSI_S1 (10 digits, 34 bits, 5 bytes)
    //IMSI_S2: First 3 digits of MIN (10 bits, 2 bytes)
    //IMSI_S1:Second 3 digits of MIN (10 bits) + Thousands digit (4 bits) + Last 3 digits (10 bits) (total 3 bytes)

    //MCC: Mobile Country Code
    //MNC: Mobile Network Code
    //MSIN: Mobile Station Identification Number
    //NMSI: National Mobile Station Identity
    //IMSI: International Mobile Station Identity
    //MIN: Mobile Identification Number
(Assignee)

Comment 6

5 years ago
Test on Wasabi with APBW's SIM card to read IMEI_M (CSIM 6F22).
We can get below value from parcel: 0,48,0,48,0,101,0,53,0,48,0,51,0,101,0,101,0,99,0,97,0,49,0,55,0,53,0,101,0,56,0,48,0,54,0,51,0,48,0,49.
Which decoded as below Hex: 0x00, 0x5e, 0x03, 0xee, 0xca, 0x17, 0x5e, 0x80, 0x63, 0x01.
I'll check if this can resolve as correct CDMA IMSI.

Comment 7

5 years ago
according to triage result, this is a basic feature, changed to 1.3+
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → 1.3 Sprint 6 - 12/6
(Assignee)

Comment 8

5 years ago
Created attachment 821539 [details] [diff] [review]
Part1: add getCDMAIMSI function to get IMSI,MCC,MNC.
(Assignee)

Comment 9

5 years ago
Created attachment 821540 [details] [diff] [review]
Part2: getCDMAIMSI/decodeIMSI xpcshell test.
(Assignee)

Comment 10

5 years ago
> We need to add GET_IMSI and parsing functions for CDMA. 
> PS. EF_IMSI_T = 0x6F23
>     EF_IMSI_M = 0x6F22

I've try on several USIM, EF_IMSI_T(6F23) is not programmed but show programmed at 8th byte. 
    // 8th byte: IMSI_Programmed and IMSI_Addr_Num
Took Android as reference, we only read IMSI_M here.
(Assignee)

Updated

5 years ago
Attachment #821539 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #821540 - Flags: review?(allstars.chh)
Comment on attachment 821539 [details] [diff] [review]
Part1: add getCDMAIMSI function to get IMSI,MCC,MNC.

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

::: dom/system/gonk/ril_worker.js
@@ +12959,5 @@
>    },
>  
>    /**
> +   * Get IMSI_M from CSIM/RUIM.
> +   * See 3GPP2 C.S0023 Sec. 3.4.2

C.S0065 section 5.2.2

@@ +12961,5 @@
>    /**
> +   * Get IMSI_M from CSIM/RUIM.
> +   * See 3GPP2 C.S0023 Sec. 3.4.2
> +   */
> +  getCDMAIMSI: function getCDMAIMSI() {

getIMSI_M

@@ +12968,5 @@
> +      let tempImsi = GsmPDUHelper.readHexOctetArray(strLen / 2);
> +      Buf.readStringDelimiter(strLen);
> +
> +      if ((tempImsi[7] & 0x80)) { //IMSI_M provisioned
> +        this.decodeIMSI(tempImsi);

RIL.iccInfoPrivate.imsi = this.decodeIMSI(tmp);

Try to modify some member variable and send notification in the same function.

@@ +12979,5 @@
> +          RIL.iccInfo.mnc = mccMnc.mnc;
> +          ICCUtilsHelper.handleICCInfoChange();
> +        }
> +      }
> +    }

add an extra line here.

@@ +12986,5 @@
> +  },
> +
> +  /**
> +   * Decode IMSI from IMSI_M
> +   * See TIA/EIA 95-B Sec. 6.3.1

C.S0005 section 2.3.1.3

@@ +12989,5 @@
> +   * Decode IMSI from IMSI_M
> +   * See TIA/EIA 95-B Sec. 6.3.1
> +   */
> +  decodeIMSI: function decodeIMSI(tempIMSI) {
> +    function binaryMapIMSI(length, tempcode) {

Can we call it decodeIMSIOctets or decodeIMSIValue?
or something more easier to understand?

And change the order of parameters

decodeIMSIValue(encoded, length)

@@ +12996,5 @@
> +      for (let i = 0; i < (length - 1); i++)
> +      {
> +        addOn = (addOn + 1) * 10;
> +      }
> +      addOn += 1;

Isn't 
let base = length === 3 ? 111 : 11; 
simpler?

@@ +12998,5 @@
> +        addOn = (addOn + 1) * 10;
> +      }
> +      addOn += 1;
> +
> +      let temp1 = tempcode + addOn;

let offset = encoded + base;

@@ +13005,5 @@
> +      let iszero = false;
> +
> +      for (let i = length - 1; i >= 0; i--) {
> +        if (iszero) {
> +          temp1 -= 1;

not sure why -1 here.

@@ +13009,5 @@
> +          temp1 -= 1;
> +          iszero = false;
> +        }
> +        temp2 = Math.floor(temp1 / 10);
> +        digit[i] = temp1 - temp2 * 10;

Can't we use % ?

@@ +13018,5 @@
> +      let str = "";
> +      for (let i = 0; i < length; i++) {
> +        str += String.fromCharCode(digit[i] + 48);
> +      }
> +      return str;

Can you try to make it simpler?
Try to remove iszero, and remove the int-to-string coversion, 
can't we try to take adventage of toString()?

@@ +13021,5 @@
> +      }
> +      return str;
> +    }
> +
> +    //MCC: 10 bits, 3 digits

nit, add a sp after //
// MCC:...
Attachment #821539 - Flags: review?(allstars.chh) → review-
Comment on attachment 821540 [details] [diff] [review]
Part2: getCDMAIMSI/decodeIMSI xpcshell test.

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

Can you also add tests for those util function you used?
Attachment #821540 - Flags: review?(allstars.chh)
(Assignee)

Comment 14

5 years ago
Created attachment 823917 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC.
Attachment #821539 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 823918 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a29bb63195f1
Attachment #821540 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 824480 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC.

Update decodeIMSIValue function.
Attachment #823917 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #823918 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #824480 - Flags: review?(allstars.chh)
Comment on attachment 824480 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC.

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

::: dom/system/gonk/ril_worker.js
@@ +12951,5 @@
>  
>  let RuimRecordHelper = {
>    fetchRuimRecords: function fetchRuimRecords() {
>      ICCRecordHelper.readICCID();
> +    this.IMSI_M();

Can you show me where is this function defined?
Attachment #824480 - Flags: review?(allstars.chh) → review-
(Assignee)

Comment 19

5 years ago
Created attachment 825715 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2
Attachment #824480 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #825715 - Flags: review?(allstars.chh)
Comment on attachment 825715 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2

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

Hi, Georgia
Before I am going to review this,
can you kindly answer my previous question first and explain what you have changed in this version?
this helps me when reviewing your patch.
Otherwise I have to start over again to fully review your patch, not just reviewing what you have changed in this version.

Thanks
Attachment #825715 - Flags: review?(allstars.chh)
(Assignee)

Comment 21

5 years ago
Hello Yoshi,

Beside coding style part, the modified parts of this patch:

1. Function name change from "getCDMAIMSI" to "getIMSI_M"

2. Return imsi in "decodeIMSI", so we can modify parameter and send chrome message nearby.
> +  RIL.iccInfoPrivate.imsi = this.decodeIMSI(tempImsi);
> +  RIL.sendChromeMessage({rilMessageType: "iccimsi", imsi: RIL.iccInfoPrivate.imsi});

3. Change "binaryMapIMSI" to "decodeIMSIValue". In order to test this function, I also separate it from decodeIMSI.

4. The most significant change is in "decodeIMSIValue"
@@ +13023
> +  decodeIMSIValue: function decodeIMSIValue(encoded, length) {
> +    let offset = length === 3 ? 111 : 11;
Simplify the offset assignment. 

@@ +13027
> +    if (value % 10 === 0 ) {
> +      value -= 10;
> +    }
> +    if (value % 100 < 10 ) {
> +      value -=100;
> +    }
> +    if (value > 1000) {
> +      value -= 1000;
> +    }
decoded =D3D2D1. If any of digit is 0, replace it as 10 in below function.
value = D3 x 100 + D2 x 10 + D3.
So in this section, if any digit of value is 0, we just -1 on it's next digit.
For example, value =  420 =  4 x 100 + 1 x 10 + 10. Decoded should be 410.
             value = 1050 = 10 x 100 + 4 x 10 + 10. Decoded should be 040. 

@@ +13037  
> +    let s = value.toString();
> +    while (s.length < length) {
> +      s = "0" + s;
> +    }
Use toString to get decoded string. Add "0" to length.


(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #20)
> Comment on attachment 825715 [details] [diff] [review]
> Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2
> 
> Review of attachment 825715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi, Georgia
> Before I am going to review this,
> can you kindly answer my previous question first and explain what you have
> changed in this version?
> this helps me when reviewing your patch.
> Otherwise I have to start over again to fully review your patch, not just
> reviewing what you have changed in this version.
> 
> Thanks

Hope my comment can answer your question:)
Comment on attachment 825715 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2

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

::: dom/system/gonk/ril_worker.js
@@ +10681,5 @@
>     * This function handles EFs for RUIM
>     */
>    getRuimEFPath: function getRuimEFPath(fileId) {
>      switch(fileId) {
> +      case ICC_EF_CSIM_IMSI_M:

nit, add // Fall through

@@ +12964,5 @@
> +   */
> +  getIMSI_M: function getIMSI_M() {
> +    function callback() {
> +      let strLen = Buf.readInt32();
> +      let tempImsi = GsmPDUHelper.readHexOctetArray(strLen / 2);

try to call it encodedImsi,
if you call it 'temp', I would think this value will be discarded, or IMSI will be updated,.. etc.

@@ +12967,5 @@
> +      let strLen = Buf.readInt32();
> +      let tempImsi = GsmPDUHelper.readHexOctetArray(strLen / 2);
> +      Buf.readStringDelimiter(strLen);
> +
> +      if ((tempImsi[7] & 0x80)) { //IMSI_M provisioned

looks like 7 is a magic number, 
can we try to const it, also for the consts below in decodeIMSI?

nit, try to add a space after //,
like 
// IMSI_M provisioned

And the same for below.

@@ +12980,5 @@
> +          ICCUtilsHelper.handleICCInfoChange();
> +        }
> +      }
> +    }
> +    ICCIOHelper.loadTransparentEF({fileId: ICC_EF_CSIM_IMSI_M,

nit, add an extra line before calling loadTransparntEF

@@ +12988,5 @@
> +  /**
> +   * Decode IMSI from IMSI_M
> +   * See 3GPP2 C.S0005 Sec. 2.3.1
> +   */
> +  decodeIMSI: function decodeIMSI(tempIMSI) {

s/tempIMSI/encodedImsi/

Also notice that when you call this function, the parameter is called tempImsi,
however in this function, the argument is tempIMSI, with IMSI all capitalized.

Again, try to make the naming consistent.
It's fine to choose IMSI or Imsi, just pick one and use it consistently.

@@ +12990,5 @@
> +   * See 3GPP2 C.S0005 Sec. 2.3.1
> +   */
> +  decodeIMSI: function decodeIMSI(tempIMSI) {
> +   //MCC: 10 bits, 3 digits
> +    let tempMCC = ((tempIMSI[9] & 0x03) << 8) + (tempIMSI[8] & 0xff);

try to remove the 'temp' prefix.
You can call it mcc_mp from specification,
or encodedMcc, ..etc

Same for below variables.

@@ +13003,5 @@
> +    let min2 = this.decodeIMSIValue(tempMIN2, 3);
> +
> +    //MIN1: 10+4+10 bits, 3+1+3 digits
> +    let tempMIN1first3 = ((tempIMSI[4] & 0xc0) >> 6) + ((tempIMSI[5] & 0xff) << 2);
> +    let min1f3 = this.decodeIMSIValue(tempMIN1first3, 3);

we can call it min1First3, or even min1First3Digits
to make it more clear.

@@ +13005,5 @@
> +    //MIN1: 10+4+10 bits, 3+1+3 digits
> +    let tempMIN1first3 = ((tempIMSI[4] & 0xc0) >> 6) + ((tempIMSI[5] & 0xff) << 2);
> +    let min1f3 = this.decodeIMSIValue(tempMIN1first3, 3);
> +
> +    let thousands = (tempIMSI[4] & 0x3c) >> 2;

How about call it fourthDigit?
And do the string conversion here.

So you don't have to do String.fromCharCode later.

@@ +13010,5 @@
> +    if  (thousands > 9) thousands  = 0;
> +    thousands += 48;
> +
> +    let tempMIN1last3 = ((tempIMSI[4] & 0x03) << 8) + (tempIMSI[3] & 0xff);
> +    let min1l3 = this.decodeIMSIValue(tempMIN1last3, 3);

same, min1Last3 or min1Last3Digits.

@@ +13026,5 @@
> +
> +    if (value % 10 === 0 ) {
> +      value -= 10;
> +    }
> +    if (value % 100 < 10 ) {

I guess you want 
if (value % 100 === 0) ?

@@ +13029,5 @@
> +    }
> +    if (value % 100 < 10 ) {
> +      value -=100;
> +    }
> +    if (value > 1000) {

here too,

if (value % 1000 === 0)
Attachment #825715 - Flags: review-
Comment on attachment 825715 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2

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

::: dom/system/gonk/ril_worker.js
@@ +13026,5 @@
> +
> +    if (value % 10 === 0 ) {
> +      value -= 10;
> +    }
> +    if (value % 100 < 10 ) {

Oh, I mean 
if ((value / 10) % 10) === 0)

@@ +13029,5 @@
> +    }
> +    if (value % 100 < 10 ) {
> +      value -=100;
> +    }
> +    if (value > 1000) {

if ((value / 100) % 10) === 0)
Comment on attachment 823918 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA.

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

::: dom/system/gonk/tests/test_ril_worker_ruim.js
@@ +73,5 @@
> + * Verify RuimRecordHelper.decodeIMSIValue
> + */
> +add_test(function test_decode_imsi_value() {
> +  let worker = newUint8Worker();
> +  function testDecodeImsiValue(encoded, length, espect) {

s/espect/expect/

@@ +84,5 @@
> +
> +  testDecodeImsiValue(90, 2, "01");
> +  testDecodeImsiValue(23, 2, "34");
> +  testDecodeImsiValue(199, 3, "200");
> +  testDecodeImsiValue(123, 3, "234");

Try to add more tests, specially the digit 0,
for example,

120
102
012

100
010
001

@@ +98,5 @@
> +  let helper = worker.GsmPDUHelper;
> +  let buf    = worker.Buf;
> +  let io     = worker.ICCIOHelper;
> +
> +  function testDecodeImsi(imsim, expectedImsi) {

imsi_m or imsiM, to make it clear there are actually two words.

Also Now you spell 'expected' correctly. \O/
Attachment #823918 - Flags: review?(allstars.chh)
(Assignee)

Comment 25

5 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #23)
> Comment on attachment 825715 [details] [diff] [review]
> Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2
> 
> Review of attachment 825715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +13026,5 @@
> > +
> > +    if (value % 10 === 0 ) {
> > +      value -= 10;
> > +    }
> > +    if (value % 100 < 10 ) {
> 
> Oh, I mean 
> if ((value / 10) % 10) === 0)

I just found in javascript, value / 10 will get floating point.. not integer
Thus the behavior is not expect.  To get integer, need "Math.floor(x)", which is more complex@@

> @@ +13029,5 @@
> > +    }
> > +    if (value % 100 < 10 ) {
> > +      value -=100;
> > +    }
> > +    if (value > 1000) {
> 
> if ((value / 100) % 10) === 0)
(Assignee)

Comment 26

5 years ago
Created attachment 827273 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v3

The major change:
1. nit
2. tempXXX -> encodedXXXX
3. add const for CSIM_IMSI_M_PROGRAMMED_BYTE
4. modify naming in decodeIMSI.
5. modify the logic in decodeIMSIValue
Attachment #825715 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
Created attachment 827276 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v2

Add more test cases.
Attachment #823918 - Attachment is obsolete: true
Comment on attachment 827273 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v3

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

Can you also test this patch with more CDMA SIMs?

::: dom/system/gonk/ril_consts.js
@@ +668,5 @@
>  this.USIM_TAG_NAME[ICC_USIM_EFCCP1_TAG] = "ccp1";
>  
> +// CDMA IMSI_M's programmed byte.
> +// 3GPP2 C.S0065 Sec. 5.2.2
> +this.CSIM_IMSI_M_PROGRAMMED_BYTE = 7;

Also try to const other bytes.

::: dom/system/gonk/ril_worker.js
@@ +4958,5 @@
>    }
>  
>    options.rilMessageType = "iccimsi";
>    options.imsi = this.iccInfoPrivate.imsi;
>    this.sendChromeMessage(options);

I'd like to know more details why removed this.
Previous this could work because at that time we used a SIM which has a GSM and a CDMA subscriptions.
And getIMSI will get the correct IMSI value from GSM subscription, even we switch to CDMA.

My question is,
Previously when we switch to CDMA mode, why getIMSI is still able to fetch the EF_IMSI from GSM subscription?
Since we will take AID in REQUEST_GET_IMSI and the AID between the GSM and CDMA subsciptions should be different.
Or are they sharing the same AID?

@@ +12980,5 @@
> +          ICCUtilsHelper.handleICCInfoChange();
> +        }
> +      }
> +    }
> +    

nit, extra spaces.

@@ +13006,5 @@
> +
> +    // MIN2: 10 bits, 3 digits
> +    let encodedMIN2 = ((encodedImsi[2] & 0x03) << 8) + (encodedImsi[1] & 0xff);
> +    let min2 = this.decodeIMSIValue(encodedMIN2, 3);
> + 

nit, remove extra space, and below.

@@ +13008,5 @@
> +    let encodedMIN2 = ((encodedImsi[2] & 0x03) << 8) + (encodedImsi[1] & 0xff);
> +    let min2 = this.decodeIMSIValue(encodedMIN2, 3);
> + 
> +    // MIN1: 10+4+10 bits, 3+1+3 digits
> +    let encodedMIN1first3 = ((encodedImsi[4] & 0xc0) >> 6) + ((encodedImsi[5] & 0xff) << 2);

Try camelCase.
encodedMIN1First3 with F capitalized.
encodedFourthDigit with D capitalized.

@@ +13015,5 @@
> +    let encodedFourthdigit = (encodedImsi[4] & 0x3c) >> 2;
> +    if (encodedFourthdigit > 9) {
> +      encodedFourthdigit = 0;
> +    }
> +    let fourthdigit = String.fromCharCode(encodedFourthdigit + 48);

encodedFourthDigit.toString()?

@@ +13034,5 @@
> +    let value = encoded + offset;
> +
> +    let base = 10;
> +    let temp = value;
> +    for (let i = 0; i < length; i++) {

for (let base = 10, temp = vale, i = 0,...

@@ +13035,5 @@
> +
> +    let base = 10;
> +    let temp = value;
> +    for (let i = 0; i < length; i++) {
> +      if ((temp % 10 === 0) && (value >= base)) {

What is the check for value >= base for?
When the encoded is 99 and length is 2
value will become 110, then 100, which is still >= 100.

When the encoded is 999, length is 3
value will be come 1110, 1100, 1000, will is still >= 1000.
(Assignee)

Comment 29

5 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #28)
> Comment on attachment 827273 [details] [diff] [review]
> Part1: add getIMSI_M function to get IMSI,MCC,MNC. v3
> 
> Review of attachment 827273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you also test this patch with more CDMA SIMs?
Currently I tried on one apbw SIM and one 中国电信 SIM.
I'll see if there's more available CDMA SIMs.

> ::: dom/system/gonk/ril_consts.js
> @@ +668,5 @@
> >  this.USIM_TAG_NAME[ICC_USIM_EFCCP1_TAG] = "ccp1";
> >  
> > +// CDMA IMSI_M's programmed byte.
> > +// 3GPP2 C.S0065 Sec. 5.2.2
> > +this.CSIM_IMSI_M_PROGRAMMED_BYTE = 7;
> 
> Also try to const other bytes.
Okey.....

> ::: dom/system/gonk/ril_worker.js
> @@ +4958,5 @@
> >    }
> >  
> >    options.rilMessageType = "iccimsi";
> >    options.imsi = this.iccInfoPrivate.imsi;
> >    this.sendChromeMessage(options);
> 
> I'd like to know more details why removed this.
> Previous this could work because at that time we used a SIM which has a GSM
> and a CDMA subscriptions.
> And getIMSI will get the correct IMSI value from GSM subscription, even we
> switch to CDMA.
> 

The notification is needed so I already put in getIMSI_M.
@@ +12971 @@
  if ((encodedImsi[CSIM_IMSI_M_PROGRAMMED_BYTE] & 0x80)) { // IMSI_M programmed
    RIL.iccInfoPrivate.imsi = this.decodeIMSI(encodedImsi);
    RIL.sendChromeMessage({rilMessageType: "iccimsi",
		           imsi: RIL.iccInfoPrivate.imsi});

> 
> @@ +13015,5 @@
> > +    let encodedFourthdigit = (encodedImsi[4] & 0x3c) >> 2;
> > +    if (encodedFourthdigit > 9) {
> > +      encodedFourthdigit = 0;
> > +    }
> > +    let fourthdigit = String.fromCharCode(encodedFourthdigit + 48);
> 
> encodedFourthDigit.toString()?
Oh yes, this can get same result and more clear.


> 
> @@ +13035,5 @@
> > +
> > +    let base = 10;
> > +    let temp = value;
> > +    for (let i = 0; i < length; i++) {
> > +      if ((temp % 10 === 0) && (value >= base)) {
> 
> What is the check for value >= base for?
> When the encoded is 99 and length is 2
> value will become 110, then 100, which is still >= 100.
> 
> When the encoded is 999, length is 3
> value will be come 1110, 1100, 1000, will is still >= 1000.

I add the check just to avoid value = 110 but run into third check ( -1000)...
But there's no need now since length = 2 will restrict this. I'll remove it.
(Assignee)

Comment 30

5 years ago
Created attachment 828511 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v4

1. nit
2. modify naming in decodeIMSI (ex encodedMIN1First3)
3. const all column in IMSI_M
4. move the define into for(let temp =...)
5. change String.fromCharCode() to toString();
6. remove unnecessary check  (value >= base)
Attachment #827273 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Created attachment 828513 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v3

Add test for scaning fetchRuimRecords.
Attachment #827276 - Attachment is obsolete: true
(Assignee)

Comment 33

5 years ago
Created attachment 828543 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v3
Attachment #828513 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #828511 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #828543 - Flags: review?(allstars.chh)
Comment on attachment 828511 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v4

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

Excellent, look back to your first patch in Comment 8,
you do have a lot progress here.

Thank you.

::: dom/system/gonk/ril_worker.js
@@ +12997,5 @@
> +   */
> +  decodeIMSI: function decodeIMSI(encodedImsi) {
> +    // MCC: 10 bits, 3 digits
> +    let encodedMCC = ((encodedImsi[CSIM_IMSI_M_MCC_BYTE + 1] & 0x03) << 8)
> +                    + (encodedImsi[CSIM_IMSI_M_MCC_BYTE] & 0xff);

nit, move + to the previous line, and below.

@@ +13012,5 @@
> +
> +    // MIN1: 10+4+10 bits, 3+1+3 digits
> +    let encodedMIN1First3 = ((encodedImsi[CSIM_IMSI_M_MIN1_BYTE + 2] & 0xff) << 2)
> +                          + ((encodedImsi[CSIM_IMSI_M_MIN1_BYTE + 1] & 0xc0) >> 6);
> +    let min1first3 = this.decodeIMSIValue(encodedMIN1First3, 3);

min1First3
Attachment #828511 - Flags: review?(allstars.chh) → review+
Comment on attachment 828543 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v3

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

::: dom/system/gonk/tests/test_ril_worker_ruim.js
@@ +98,5 @@
> +
> +  ruimHelper.fetchRuimRecords();
> +
> +  for (let i = 0; i < 5; i++ ) {
> +    do_check_eq(true, ifCalled[i]);

When some function is not called, 
can you also show which one is not being called?

Also when Bug912365 lands, at that time ruimHelper will take an argument, so maybe not all of the functions here will be called.
Therefore using a for-loop to check whole array might not be desirable here.

How are you going to handle that?

@@ +109,5 @@
> + * Verify RuimRecordHelper.decodeIMSIValue
> + */
> +add_test(function test_decode_imsi_value() {
> +  let worker = newUint8Worker();
> +  function testDecodeImsiValue(encoded, length, expect) {

add an extra line before this function,
I do spend a few seconds to find out where's testDecodeImsiValue.

@@ +114,5 @@
> +    let decoded = worker.RuimRecordHelper.decodeIMSIValue(encoded, length);
> +
> +    for (let i = 0; i < length; i++) {
> +      do_check_eq(decoded[i], expect[i]);
> +    }

Can't we do decode === expect?

@@ +138,5 @@
> +
> +/**
> + * Verify RuimRecordHelper.getIMSI_M
> + */
> +add_test(function test_get_cdma_imsi() {

update the test name
test_get_imsi_m

@@ +168,5 @@
> +    do_check_eq(imsi.length, expectedImsi.length);
> +
> +    for (let i = 0; i < expectedImsi.length; i++) {
> +      do_check_eq(imsi[i], expectedImsi[i]);
> +    }

ditto
Attachment #828543 - Flags: review?(allstars.chh)
(Assignee)

Comment 36

5 years ago
Created attachment 829116 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v5.

Thanks for Yoshi's review~~
Just modify naming base on Comment 34.
Attachment #828511 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #35)
> Comment on attachment 828543 [details] [diff] [review]
> Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v3
> 
> Review of attachment 828543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/tests/test_ril_worker_ruim.js
> @@ +98,5 @@
> > +
> > +  ruimHelper.fetchRuimRecords();
> > +
> > +  for (let i = 0; i < 5; i++ ) {
> > +    do_check_eq(true, ifCalled[i]);
> 
> When some function is not called, 
> can you also show which one is not being called?
> 
> Also when Bug912365 lands, at that time ruimHelper will take an argument, so
> maybe not all of the functions here will be called.
> Therefore using a for-loop to check whole array might not be desirable here.
> 
> How are you going to handle that?
> 

My idea is to have a function in the test with two input "expected called functions and efIds":

+ function testFetchRuimRecordes(expectCalled, efIds) {  
  //do check here, if actualy called function did not match expectCalled, just print it out 

+ let expectCalled = ["readICCID", "getIMSI_M", "readCST", "readCDMAHome", "getCdmaSubscription"];
+ testFetchRuimRecordes(expectCalled);

if there's no efIds input, expectCalled contains the whole 5 functions.
Once Bug912365 landed (or some change modify base on efIds input), we can add more test case like:

+ let expectCalled_1 = ["readICCID", "getIMSI_M", "readCST"];
+ let efIds_1 = [EF_XXXX, EF_YYYY];
+ testFetchRuimRecordes(expectCalled, efIds);


For below, I'll follow your suggestions.
(Assignee)

Comment 38

5 years ago
Created attachment 829171 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v4

=> Change name & check logic base on Comment 34.
=> Modify the fetch RUIM records test base on Comment 37.
Attachment #828543 - Attachment is obsolete: true
Attachment #829171 - Flags: review?(allstars.chh)
Comment on attachment 829171 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v4

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

::: dom/system/gonk/tests/test_ril_worker_ruim.js
@@ +74,5 @@
> +  let RIL = worker.RIL;
> +  let iccHelper = worker.ICCRecordHelper;
> +  let ruimHelper = worker.RuimRecordHelper;
> +
> +  function testFetchRuimRecordes(expectCalled, efIds) {

remove efIds.

@@ +101,5 @@
> +    if (efIds) {
> +      ruimHelper.fetchRuimRecords(efIds);
> +    } else {
> +      ruimHelper.fetchRuimRecords();
> +    }

Just ruimHelper.fetchRuimRecords();

We could add the efIds back once the other bug is ladned.

@@ +132,5 @@
> +      do_check_true(true);
> +    } else {
> +      do_print("decoded: " + decoded + " is not equal to expect: " + expect);
> +      do_check_true(false);
> +    }

do_check_eq(expect, decoded)

@@ +188,5 @@
> +      do_check_true(true);
> +    } else {
> +      do_print("imsi: " + imsi + " is not equal to expectedImsi: " + expectedImsi);
> +      do_check_true(false);
> +    }

ditto
Attachment #829171 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 41

5 years ago
Created attachment 829190 [details] [diff] [review]
Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v5

Remove efIds, modify do_check_eq base on Comment 40 :)
Attachment #829116 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 829191 [details] [diff] [review]
Part1: add getIMSI_M function to get IMSI,MCC,MNC. v5.

Update Part1 again....
Attachment #829171 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c793f79e8349
https://hg.mozilla.org/mozilla-central/rev/d105dc40d301
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 45

5 years ago
Verified on Wasabi with APTG UIM.

Gaia:     e42fdf6a6b77038b0a2cd46b9193378d84794c20
Gecko:    0e2820f33eb0cbe5a087e94b1832cdf0b4eaacca
BuildID   20131125113502
Version   28.0a1
Status: RESOLVED → VERIFIED
status-firefox28: --- → fixed
You need to log in before you can comment on or make changes to this bug.