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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Enpei]

+++ 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.

Comment 1

[Francis Lee [:frlee]]

according to RIL triage meeting, since this is not in v1.2 user story, move to 1.3?.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 3

[Enpei]

Thanks for your suggestion jaoo, I've updated the title.

Comment 4

[Georgia Wang]

It's because

Comment 5

[Georgia Wang]

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

Comment 6

[Georgia Wang]

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

[Francis Lee [:frlee]]

according to triage result, this is a basic feature, changed to 1.3+

Comment 8

[Georgia Wang]

Attachment: Part1: add getCDMAIMSI function to get IMSI,MCC,MNC.

Comment 9

[Georgia Wang]

Attachment: Part2: getCDMAIMSI/decodeIMSI xpcshell test.

Comment 10

[Georgia Wang]

> 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.

Comment 11

[Georgia Wang]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=c260b9a21067

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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:...

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 14

[Georgia Wang]

Attachment: Part1: add getIMSI_M function to get IMSI,MCC,MNC.

Comment 15

[Georgia Wang]

Attachment: Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA.

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

Comment 16

[Georgia Wang]

Attachment: Part1: add getIMSI_M function to get IMSI,MCC,MNC.

Update decodeIMSIValue function.

Comment 17

[Georgia Wang]

Try server link: https://tbpl.mozilla.org/?tree=Try&rev=430fea4ec859

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 19

[Georgia Wang]

Attachment: Part1: add getIMSI_M function to get IMSI,MCC,MNC. v2

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 21

[Georgia Wang]

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 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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)

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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/

Comment 25

[Georgia Wang]

(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)

Comment 26

[Georgia Wang]

Attachment: 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

Comment 27

[Georgia Wang]

Attachment: Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v2

Add more test cases.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 29

[Georgia Wang]

(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.

Comment 30

[Georgia Wang]

Attachment: 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)

Comment 31

[Georgia Wang]

Attachment: Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v3

Add test for scaning fetchRuimRecords.

Comment 32

[Georgia Wang]

Try:  https://tbpl.mozilla.org/?tree=Try&rev=4a7678731375

Comment 33

[Georgia Wang]

Attachment: Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v3

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 36

[Georgia Wang]

Attachment: Part1: add getIMSI_M function to get IMSI,MCC,MNC. v5.

Thanks for Yoshi's review~~
Just modify naming base on Comment 34.

Comment 37

[Georgia Wang]

(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.

Comment 38

[Georgia Wang]

Attachment: 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.

Comment 39

[Georgia Wang]

Try: https://tbpl.mozilla.org/?tree=Try&rev=30f83ab62943

Comment 40

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 41

[Georgia Wang]

Attachment: Part2: getIMSI_M and decodeIMSI xpcshell test for CDMA. v5

Remove efIds, modify do_check_eq base on Comment 40 :)

Comment 42

[Georgia Wang]

Attachment: Part1: add getIMSI_M function to get IMSI,MCC,MNC. v5.

Update Part1 again....

Comment 43

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c793f79e8349
https://hg.mozilla.org/integration/b2g-inbound/rev/d105dc40d301

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c793f79e8349
https://hg.mozilla.org/mozilla-central/rev/d105dc40d301

Comment 45

[Enpei]

Verified on Wasabi with APTG UIM.

Gaia:     e42fdf6a6b77038b0a2cd46b9193378d84794c20
Gecko:    0e2820f33eb0cbe5a087e94b1832cdf0b4eaacca
BuildID   20131125113502
Version   28.0a1