DEFECT DESCRIPTION:[GCF][STK]Text String:length is not ok when execute get input->27.22.4.3.1/8 REPRODUCING PROCEDURES: 1. Load a simcard to the phone which can send "get input"; 2. Execute 27.22.4.3.1/8:GET INPUT, digits only, SMS default alphabet, ME to echo text, ME supporting 8 bit data Message; 3. Terminal response is "81 03 01 23 00 02 02 82 81 83 01 00 8D A1 04 2A 2A..."-ko EXPECTED BEHAVIOUR: 3. Terminal response should be"81 03 01 23 00 02 02 82 81 83 01 00 8D 81 A1 04 2A 2A..."; Reporter's phone number: 0752-2639695 This issue block GCF. ASSOCIATE SPECIFICATION: TEST PLAN REFERENCE: TOOLS AND PLATFORMS USED: USER IMPACT: REPRODUCING RATE: For FT PR, Please list reference mobile's behavior:

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

Hi Pengfei, Here is the string entered by user: "input":"***1111111111###***2222222222###***3333333333###***4444444444###***5555555555###***6666666666###***7777777777###***8888888888###***9999999999###***0000000000###" and its length is 160 (HEX: A0). So the length for STK command is A0 + 1 = A1 May I know why do we need the byte '81' here? Thanks.

Hi Sean, After more analysis offlined, we found the root cause is that when encoding the Text string C-TLV with length larger than 128, the length encoding of C-TLV is not applied. The number of octets for encoding length value is varied from 1 to 4. [1] However, we always encode it with 1 octets. [2] The test data is provided in |TERMINAL RESPONSE: GET INPUT 1.8.1| of 27.22.4.3 GET INPUT in TS 102 384. We need to figure out a solution to encode the length with varied octets properly in ril_worker.js. [1] https://hg.mozilla.org/releases/mozilla-release/file/c13c3b4992cf/dom/system/gonk/ril_worker.js#l11857 [2] https://hg.mozilla.org/releases/mozilla-release/file/c13c3b4992cf/dom/system/gonk/ril_worker.js#l2901

Hi Bevis, Thanks for your information!

This is to 1. encode the length of "Text String" C-TLV properly with varied number of octets. 2. encode unpacked GSM7bit alphabet correctly. 3. Fix the logical problem when fallback the encoding from extension table. Hi Edgar, May I have your review for this change? Thanks!

Add |TERMINAL RESPONSE: GET INPUT 1.8.1| of 27.22.4.3.1 GET INPUT (normal) in TS 102 384 into our test coverage. Hi Edgar, May I have your review for this test case? Thanks!

Dear Bevis Tseng & Josh, This bug and bug1122330 both test pass with your patch comment7. Bevis you are awesome, many thanks.

(In reply to pengfei.huang from comment #9) > Dear Bevis Tseng & Josh, > This bug and bug1122330 both test pass with your patch comment7. > Bevis you are awesome, many thanks. \o/

Hi Bevis, Thanks! 2.0M+

Comment on attachment 8551658 [details] [diff] [review] Part 1 v1: Encode Length of "Text String" C-TLV in Varied Length of Octets. Review of attachment 8551658 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +7532,5 @@ > this.writeHexOctet(data & 0xFF); > } > }, > > + writeStringAs8BitUnpacked: function(aText) { Let's just follow the same coding style in ril_worker, e.g. no `a` prefix for argument. Thank you. @@ +7536,5 @@ > + writeStringAs8BitUnpacked: function(aText) { > + const langTable = PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT]; > + const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT]; > + > + let i, c, octet; One declaration per line and declare local variables as near to their use as possible. @@ +7538,5 @@ > + const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT]; > + > + let i, c, octet; > + let len = aText ? aText.length : 0; > + for (i = 0; i < len; i++) { for (let i .... @@ +7539,5 @@ > + > + let i, c, octet; > + let len = aText ? aText.length : 0; > + for (i = 0; i < len; i++) { > + c = aText.charAt(i); let c = ....; @@ +7540,5 @@ > + let i, c, octet; > + let len = aText ? aText.length : 0; > + for (i = 0; i < len; i++) { > + c = aText.charAt(i); > + octet = langTable.indexOf(c); let octet = ...; @@ +12165,5 @@ > GsmPDUHelper.writeSwappedNibbleBCDNum(Math.floor(seconds / 60) % 60); > GsmPDUHelper.writeSwappedNibbleBCDNum(seconds % 60); > }, > > + writeTextStringTlv: function(aText, aCoding) { Ditto, no 'a' prefix for argument in ril_worker. @@ +12169,5 @@ > + writeTextStringTlv: function(aText, aCoding) { > + let GsmPDUHelper = this.context.GsmPDUHelper; > + let writeHexOctet = GsmPDUHelper.writeHexOctet; > + let buf = []; > + GsmPDUHelper.writeHexOctet = function(aOctet) { I like this idea of putting the data into a cached area first until we know the whole length. ;) But per off-line discussion, please help to try to move this idea to worker_buf? Thank you.

Comment on attachment 8551659 [details] [diff] [review] Part 2 v1: Add Test Case to Improve Test Coverage. Review of attachment 8551659 [details] [diff] [review]: ----------------------------------------------------------------- Nice!!! Thank you.

Hi Kai-Zhen, 2.0M+ Thanks!

(In reply to Josh Cheng [:josh] from comment #14) > Hi Kai-Zhen, > 2.0M+ Thanks! Solution is not ready yet! Patch Part 1 has to be reviewed again.

address suggestions in comment 12.

Comment on attachment 8552410 [details] [diff] [review] Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets. Review of attachment 8552410 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +7189,5 @@ > + * Function of how the data to be written into temporary buffer. > + * > + * @return array of written octets. > + **/ > + writeWithBuffer: function(writeFunction) { No better idea than this. Thank you, Bevis.

Comment on attachment 8552410 [details] [diff] [review] Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets. [Approval Request Comment] Bug caused by (feature/regressing bug #): NA User impact if declined: Block partner's certificate for GCF/PTCRB test cases. Testing completed: Yes. Test case is also included. Risk to taking this patch (and alternatives if risky): No. String or UUID changes made by this patch:NA

Comment on attachment 8552410 [details] [diff] [review] Part 1 v2: Encode Length of "Text String" C-TLV in Varied Length of Octets. no need for v2.0

Comment on attachment 8551659 [details] [diff] [review] Part 2 v1: Add Test Case to Improve Test Coverage. [Approval Request Comment] Bug caused by (feature/regressing bug #): NA User impact if declined: Block partner's certificate for GCF/PTCRB test cases. Testing completed: Yes. Test case is also included. Risk to taking this patch (and alternatives if risky): No. String or UUID changes made by this patch:NA

update tryserver result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7430c3bd2de

https://hg.mozilla.org/integration/b2g-inbound/rev/c2e6c6585d11 https://hg.mozilla.org/integration/b2g-inbound/rev/4c6e5c134982

https://hg.mozilla.org/mozilla-central/rev/c2e6c6585d11 https://hg.mozilla.org/mozilla-central/rev/4c6e5c134982

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6c4b49b5a56e https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/27e754b3f8f9 https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/78a5a509b12b https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/72a504f92306

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ad92e87542e9 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/9c62a8c9c36d