Closed Bug 1123204 Opened 6 years ago Closed 6 years ago

[FFOS2.0][Woodduck][GCF][STK]Text String:length is not ok when execute get input->27.22.4.3.1/8

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0M+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S4 (23jan)
blocking-b2g 2.0M+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: sync-1, Assigned: bevis)

References

Details

Attachments

(3 files, 1 obsolete file)

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:
Attached file 27.22.4.3.1.8.rar
Hi Sean,
Could you please help to check the problem? Thanks!
Flags: needinfo?(selee)
blocking-b2g: --- → 2.0M?
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.
Flags: needinfo?(selee) → needinfo?(pengfei.huang.hz)
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
Flags: needinfo?(pengfei.huang.hz)
Hi Bevis, Thanks for your information!
Blocks: b2g-stk
Component: General → RIL
Assignee: nobody → btseng
Duplicate of this bug: 1117661
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!
Attachment #8551658 - Flags: review?(echen)
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!
Attachment #8551659 - Flags: review?(echen)
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+
Blocks: Woodduck
Status: NEW → ASSIGNED
blocking-b2g: 2.0M? → 2.0M+
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Blocks: 1117658
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.
Attachment #8551658 - Flags: review?(echen)
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.
Attachment #8551659 - Flags: review?(echen) → review+
Hi Kai-Zhen,
2.0M+ Thanks!
Flags: needinfo?(kli)
(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.
Flags: needinfo?(kli)
address suggestions in comment 12.
Attachment #8551658 - Attachment is obsolete: true
Attachment #8552410 - Flags: review?(echen)
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.
Attachment #8552410 - Flags: review?(echen) → review+
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
Attachment #8552410 - Flags: approval-mozilla-b2g37?
Attachment #8552410 - Flags: approval-mozilla-b2g34?
Attachment #8552410 - Flags: approval-mozilla-b2g32?
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
Attachment #8552410 - Flags: approval-mozilla-b2g32?
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
Attachment #8551659 - Flags: approval-mozilla-b2g37?
Attachment #8551659 - Flags: approval-mozilla-b2g34?
https://hg.mozilla.org/mozilla-central/rev/c2e6c6585d11
https://hg.mozilla.org/mozilla-central/rev/4c6e5c134982
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S4 (23jan)
Attachment #8551659 - Flags: approval-mozilla-b2g37?
Attachment #8551659 - Flags: approval-mozilla-b2g37+
Attachment #8551659 - Flags: approval-mozilla-b2g34?
Attachment #8551659 - Flags: approval-mozilla-b2g34+
Attachment #8552410 - Flags: approval-mozilla-b2g37?
Attachment #8552410 - Flags: approval-mozilla-b2g37+
Attachment #8552410 - Flags: approval-mozilla-b2g34?
Attachment #8552410 - Flags: approval-mozilla-b2g34+
You need to log in before you can comment on or make changes to this bug.