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:

Hi Gary, Could you please help to check the problem? Thanks!

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?

Moving to the RIL component.

Hi Edgar: Could we have your comment here first? Thanks!! Shawn

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.

This is a generic bug, change the title accordingly

HI Edgar, Could you help to provide ETA date? Thanks for your help!

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.

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!

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.

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

Patch log as the attachment.

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!

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.

try looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=63e2606acee8

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!

Dear Josh, OK, I will try it, thanks.

Does this need a v2.1 approval request as well?

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.

Ryan, Thanks!

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?

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.

Vincent, let's pick it up for 2.1S.