Closed Bug 1102023 Opened 7 years ago Closed 7 years ago

IccId contains unexpected value (e.g. '*' and '#')

Categories

(Firefox OS Graveyard :: RIL, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 wontfix, b2g-v2.0M verified, b2g-v2.1 wontfix, b2g-v2.1S fixed, b2g-v2.2 verified)

RESOLVED FIXED
2.2 S2 (19dec)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- verified
b2g-v2.1 --- wontfix
b2g-v2.1S --- fixed
b2g-v2.2 --- verified

People

(Reporter: sync-1, Assigned: edgar)

References

Details

Attachments

(5 files, 1 obsolete file)

Created an attachment (id=1021956)
 pic
 
 DEFECT DESCRIPTION:
 ->The ICCID display error.
 
 REPRODUCING PROCEDURES:
 ->Settings ->Device information ->More information ->ICCID
 KO:If ICCID have 'A' character, the mobile show it as '*'; If ICCID have 'b' character, the mobile show it as '#'.
 
 EXPECTED BEHAVIOUR:
 ->Display normally.
 
 ANALYSE:
 ril_worker.js function semiOctetToBcdChar() treat 'A' as '*', 'B' as '#'.
 
 REPRODUCING RATE:
 100%
 0752-2639344/61344
 
 For FT PR, Please list reference mobile's behavior:
Attached image pic
Hi Gary,
Could you please help to check the problem? Thanks!
Blocks: Woodduck
Flags: needinfo?(gchen)
blocking-b2g: --- → 2.0M?
redirect to sku, should be on his radar.
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js 

Hi Sku,
   Why do we have this design?
Flags: needinfo?(gchen)
Flags: needinfo?(sku)
Moving to the RIL component.
Component: General → RIL
Hi Edgar:
 Could we have your comment here first?

Thanks!!
Shawn
Flags: needinfo?(echen)
The '*' and '#' are from extended BCD coding [1]. But from the spec [2], the coding of IccId should be BCD, not extended BCD coding. We should correct this.

However, 0xa and 0xb are not a valid BCD value and they will be mapped to "" [3] if we follow the same error handling that we have now.

Please file a new bug if this isn't meet your expectation.

[1] See TS 131.102, table 4.4 Extended BCD coding
[2] See TS 102.221, clause 13.2 EF ICCID (ICC Identification)
[3] We had a similar issue before, see bug 976491.
Flags: needinfo?(sku)
Flags: needinfo?(echen)
This is a generic bug, change the title accordingly
Summary: [FFOS2.0][Woodduck][Settings][ICCID]The ICCID display error. → IccId contains unexpected value (e.g. '*' and '#')
Assignee: nobody → echen
See Also: → 976491
blocking-b2g: 2.0M? → 2.0M+
HI Edgar,
Could you help to provide ETA date? Thanks for your help!
Flags: needinfo?(echen)
Flags: needinfo?(echen)
Whiteboard: ETA:12/12
Blocks: Woodduck_P2
Attached patch WIP, patch, v1 (obsolete) — Splinter Review
Attached patch Patch, v2Splinter Review
Attachment #8534294 - Attachment is obsolete: true
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

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

Separate BCD and extended BCD helper to two, semiOctetToBcdChar/readSwappedNibbleBcdString and semiOctetToExtendedBcdChar/readSwappedNibbleExtendedBcdString.
And correct decoding icc to use BCD helper, instead of extended BCD helper.

Hi hsinyi, may I have your review, thank you.
Attachment #8534933 - Flags: review?(htsai)
Dear Tao Li,
The patch is currently under review. If you need to verify the issue on your side before it land, please try patch https://bugzilla.mozilla.org/attachment.cgi?id=8534933
Thanks!
Flags: needinfo?(tao.li.hz)
Dear Josh,
   It is not OK. Maybe the following function have bug.
@@ -12679,21 +12733,22 @@ ICCRecordHelperObject.prototype = {
 
   /**
    * Read the ICCID.
    */
   readICCID: function() {
     function callback() {
       let Buf = this.context.Buf;
       let RIL = this.context.RIL;
+      let GsmPDUHelper = this.context.GsmPDUHelper;
 
       let strLen = Buf.readInt32();
       let octetLen = strLen / 2;
       RIL.iccInfo.iccid =
-        this.context.GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
+        GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
       // Consumes the remaining buffer if any.
       let unReadBuffer = this.context.Buf.getReadAvailable() -
                          this.context.Buf.PDU_HEX_OCTET_SIZE;
       if (unReadBuffer > 0) {
         this.context.Buf.seekIncoming(unReadBuffer);
       }
       Buf.readStringDelimiter(strLen);

If I modify "GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);" to "GsmPDUHelper.readSwappedNibbleExtendedBcdString(octetLen, true);". The bug disappear.
Flags: needinfo?(tao.li.hz)
(In reply to land from comment #13)
> Dear Josh,
>    It is not OK. Maybe the following function have bug.
> @@ -12679,21 +12733,22 @@ ICCRecordHelperObject.prototype = {
>  
>    /**
>     * Read the ICCID.
>     */
>    readICCID: function() {
>      function callback() {
>        let Buf = this.context.Buf;
>        let RIL = this.context.RIL;
> +      let GsmPDUHelper = this.context.GsmPDUHelper;
>  
>        let strLen = Buf.readInt32();
>        let octetLen = strLen / 2;
>        RIL.iccInfo.iccid =
> -        this.context.GsmPDUHelper.readSwappedNibbleBcdString(octetLen,
> true);
> +        GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
>        // Consumes the remaining buffer if any.
>        let unReadBuffer = this.context.Buf.getReadAvailable() -
>                           this.context.Buf.PDU_HEX_OCTET_SIZE;
>        if (unReadBuffer > 0) {
>          this.context.Buf.seekIncoming(unReadBuffer);
>        }
>        Buf.readStringDelimiter(strLen);
> 
> If I modify "GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);" to
> "GsmPDUHelper.readSwappedNibbleExtendedBcdString(octetLen, true);". The bug
> disappear.

It's strange. I have verified the patch locally, It works good.
Hi Tao Li, could you help to capture the log for me (start capturing at the beginning of device
boot-up)? Thank you.
Flags: needinfo?(tao.li.hz)
Whiteboard: ETA:12/12 → ETA:12/19
Attached file mtklog.rar
Patch log as the attachment.
Flags: needinfo?(tao.li.hz)
Flags: needinfo?(echen)
Dear Edgar,

The function readICCID() always call the function readSwappedNibbleBcdString(). I think it should call the function readSwappedNibbleExtendedBcdString(). Maybe it is a slip of the pen.

Before your patch:
   /**
    * Read the ICCID.
    */
   readICCID: function() {
     function callback() {
       ......
       RIL.iccInfo.iccid =
        this.context.GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);

After your patch:
   /**
    * Read the ICCID.
    */
   readICCID: function() {
     function callback() {
       ......
       let GsmPDUHelper = this.context.GsmPDUHelper;
 
       ......
       RIL.iccInfo.iccid =
         GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

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

Looks good to me, thank you!
Attachment #8534933 - Flags: review?(htsai) → review+
Hi Tao, I tested this patch again, but it works good.
Could you help to double confirm again?

(In reply to land from comment #16)
> Dear Edgar,
> 
> The function readICCID() always call the function
> readSwappedNibbleBcdString(). I think it should call the function
> readSwappedNibbleExtendedBcdString(). Maybe it is a slip of the pen.

I modified readSwappedNibbleBcdString() in this patch, now it decodes data as BCD, not extended BCD. Calling readSwappedNibbleBcdString() in readICCID() is expected, actually.
Flags: needinfo?(echen)
https://hg.mozilla.org/mozilla-central/rev/0d3dfb9b73b6
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: ETA:12/19
Target Milestone: --- → 2.2 S2 (19dec)
If IccId is "898600751914a9800454". Your patch will delete the charactor "a", show IccId as "8986007519149800454".

Pls see the following code-flow for the root cause:
1. 
   readICCID: function() {
     function callback() {
       RIL.iccInfo.iccid =
         GsmPDUHelper.readSwappedNibbleBcdString(octetLen, true);
2.
   readSwappedNibbleBcdString: function(pairs, suppressException) {
     let str = "";
     for (let i = 0; i < pairs; i++) {
       ......
       str += this.semiOctetToBcdChar(nibbleL, suppressException);
       if (nibbleH != 0x0F) {
         str += this.semiOctetToBcdChar(nibbleH, suppressException);
       }
     }
 
     return str;
   },

3. Here, if IccId charactor don't belong to bcdChars, It will be treat as "".
   bcdChars: "0123456789",
   semiOctetToBcdChar: function(semiOctet, suppressException) {
     if (semiOctet >= this.bcdChars.length) {
       if (suppressException) {
         return "";
       } else {
         throw new RangeError();
       }
     }
 
     return this.bcdChars.charAt(semiOctet);
   },
(In reply to land from comment #22)
> If IccId is "898600751914a9800454". Your patch will delete the charactor
> "a", show IccId as "8986007519149800454".

Hi Tao, this is an expected behavior of this patch, please see commit #6.
0xa isn't a valid BCD value, so we map it to "" for error handling. Thank you.
I know it is not a valid BCD value. BUT some SIM card's IccId field include this character.
Maybe you can treat BCD value [a | b | c | d | e] as '*' to show for user.
Hi Land,
According to http://www.etsi.org/deliver/etsi_ts/131100_131199/131102/04.15.00_60/ts_131102v041500p.pdf
Table 4.4: Extended BCD coding 

and

http://www.etsi.org/deliver/etsi_ts/102200_102299/102221/08.02.00_60/ts_102221v080200p.pdf
Clause 13.2 EF ICCID (ICC Identification)

The ICCID is presented by BCD instead of Extended BCD, that's why we remove A, B.

If you need to deal with some SIM card's IccId field. That would be customization effort and TCL can implement this base on our code. Thanks!
Hi Kai-Zhen,
Can you also land this patch on 2.0M? Thanks!
Flags: needinfo?(kli)
Dear Josh,
  OK, I will try it, thanks.
Keywords: checkin-needed
Does this need a v2.1 approval request as well?
Flags: needinfo?(echen)
Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: Settings ->Device information ->More information ->ICCID shows unexpected value (e.g. *, # ... etc).
Testing completed: Patch includes xpcshell test.
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: None.
Flags: needinfo?(echen)
Attachment #8534933 - Flags: approval-mozilla-b2g34?
This issue has been successfully verified on Flame v2.2 and Woodduck v2.0.
See attachment: verified_v2.0m.png and verified_v2.2.png
Reproduce rate: 0/4

Woodduck 2.0 build:
Gaia-Rev        1f11c58d7ad7b1eb2e08db98154ff4f20aa760ed
Gecko-Rev       c9a727cb1d1ea0b5a82c64a360c03dbc35421d59
Build-ID        20141223050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1419282327
FW-Date         Tue Dec 23 05:05:50 CST 2014

Flame 2.2 build:
Gaia-Rev        ca6e91e09ef3ab417a0f6b6d6668d43597d85700
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/b915a50bc6be
Build-ID        20141222040204
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141222.072850
FW-Date         Mon Dec 22 07:29:01 EST 2014
Bootloader      L1TC00011880
Hi Steven, do we need to uplift this fix to 2.1s?
Flags: needinfo?(styang)
TCL bug already fixed, pls close it.
Comment on attachment 8534933 [details] [diff] [review]
Patch, v2

Given this is already fixed on TCl side and its too late to take RIL changes like these on 2.1 I am minusing these. We can add land this on specific partner branches as necessary.
Attachment #8534933 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34-
Vincent, let's pick it up for 2.1S.
Flags: needinfo?(styang) → needinfo?(vliu)
You need to log in before you can comment on or make changes to this bug.