1117609 - [B2G][STK] Support of "Additional Info on Result" in STK Terminal Response.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[jinghua.xing]

[Testing Steps     ]:USAT case 27.22.4.1.1 DISPLAY TEXT SEQ 1.2	DISPLAY TEXT normal priority, Unpacked 8 bit data for Text String, screen busy
1. set the ME to a display mode other than the normal stand-by mode

[Expected Result  ]:TERMINAL RESPONSE:DISPLAY TEXT 1.2.1 Terminal currently unable to process command - screen busy
TR返0x20 0x01（ME currently unable to process; Screen is busy）

[Test Result      ]:返回0x20 0x00（No specfic cause can be given）

Comment 1

[jinghua.xing]

See also bug1015833. 

That issue only fix the MMI for this case. The response for this issue is 
12-30 01:40:51.330   129   453 I Gecko   : RIL Worker: [0] Received chrome message {"resultCode":32,"additionalInformation":1,"command":{"commandNumber":1,"typeOfCommand":33,"commandQualifier":128,"rilMessageType":"stkcommand","options":{"text":"Toolkit Test 1","userClear":true},"rilMessageClientId":0},"rilMessageClientId":0,"rilMessageToken":22,"rilMessageType":"sendStkTerminalResponse"}

We have added an 'additionalInformation' property in the response for this case in gaia. But in the function sendStkTerminalResponse() in ril_worker.js, I can't find where to write the value of this property into the parcel which will be sended to the modem.

Comment 2

[jinghua.xing]

Attachment: display.patch

And I have made a patch for this issue, please help to check this issue. 

Thank you.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Update title properly.

According to |12.12 Result| in 3GPP TS 11.14 SAT for SIM [1], there is an field called |Additional information on result| which allows ME to provide more specific info of the reult:
"
For the general result "Command performed successfully", some proactive commands require additional
information in the command result. This is defined in the subclauses below. For the general results '20', '21', '26','34', '35', '37', '38' and '39' and '3A', it is mandatory for the ME to provide a specific cause value as additional information, as defined in the subclauses below. For the other general results, the ME may optionally supply additional information. If additional information is not supplied, then the length of the value part of the data object need only contain the general result.
"

It seems that we haven't support this in current ril_worker implementation. [2]

[1] http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/ts_101267v081800p.pdf
[2] http://hg.mozilla.org/mozilla-central/file/636498d041b5/dom/system/gonk/ril_worker.js#l2927

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to jinghua.xing from comment #2)
> Created attachment 8543736 [details] [diff] [review]
> display.patch
> 
> And I have made a patch for this issue, please help to check this issue. 
> 
> Thank you.

Thanks for providing the patch.
We will merge it into our implementation. :)

Comment 5

[jinghua.xing]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)

> We will merge it into our implementation. :)

I am sorry about I found something may be wrong in my patch. According to |12.12 Result| in 3GPP TS 11.14, the length of additional information maybe larger than 1 , as

Byte(s)            Description                        Length
1                  Result tag                           1
2 to (Y-1)+2       Length (X)                           X
(Y-1)+3            General result                       1
(Y-1)+4 to         Additional information on result    X-1
(Y-1)+X+2

So we can't simply use GsmPDUHelper.writeHexOctet(response.additionalInformation) to write it.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to jinghua.xing from comment #5)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> 
> > We will merge it into our implementation. :)
> 
> I am sorry about I found something may be wrong in my patch. According to
> |12.12 Result| in 3GPP TS 11.14, the length of additional information maybe
> larger than 1 , as
> 
> Byte(s)            Description                        Length
> 1                  Result tag                           1
> 2 to (Y-1)+2       Length (X)                           X
> (Y-1)+3            General result                       1
> (Y-1)+4 to         Additional information on result    X-1
> (Y-1)+X+2
> 
> So we can't simply use
> GsmPDUHelper.writeHexOctet(response.additionalInformation) to write it.

I'll take this into account when providing the patch. :)
Thanks!

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen

This patch is to append |additionalInformation| if available into the terminal response.

Note: 
1. The length of |additionalInformation| is varied according the command type and the result code. 
   (See |12.12 Result| in 3GPP TS 11.14 SAT for SIM for detailed information.)
2. In bug 1015833, the |additionalInformation| was provided in a numeric value in gaia.
3. To be backward compatible to bug 1015833 and to be more flexible for various length of |additionalInformation|, we accept both a single numeric value or a octet array. We should see if we could lock down this in bug 1114938 in the future.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v1: Add Test case to verify additional information inside TR. r=echen

Add corresponding test case for verification.

Hi Edgar,

May I have your review for this test as well?

Thanks!

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

After further study of the possible |additional info| data to be delivered in TR in 3GPP standard,
the only case that contains additional info with length > 1 is SEND SS.
(See |12.12 Result| in 3GPP TS 11.14 SAT for SIM for detailed information.)

However, in current RIL design in both FFOS & AOSP, the proactive command of SEND SS expected to be handled directly in modem side. TR should not be sent above RIL.
In Application side, only an alert has to be displayed to inform user of this operation.

Hence, we could keep the data type of the |additional information| as a single numeric value.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8543923 [details] [diff] [review]
Part 1 v1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen

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

Please comment #9 and off-line discussion, let's use numeric value for additional information.
And please remember to update MozStkResponse in MozStkCommandEvent.webidl, https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozStkCommandEvent.webidl#555-603, though we didn't really use it in currently design.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8543925 [details] [diff] [review]
Part 2 v1: Add Test case to verify additional information inside TR. r=echen

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

Per comment #9 and off-line discussion, let's use numeric value for additional information.

Comment 12

[Wei Gao (Spreadtrum)]

(In reply to (PTO on 1/9) Bevis Tseng[:bevistseng][:btseng] from comment #9)
> 
> Hence, we could keep the data type of the |additional information| as a
> single numeric value.

Hi Bevis

From comment9, you say "we could keep the data type of the |additional information| as a single numeric value", but your patch seems to support the length of additionalInformation >1.
So I can't understand, is there anything I miss?

BTW, if I add additionalInformation for SEND SS from gaia in sprd only, do your patch supports it?
Thanks.

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Wei Gao (Spreadtrum) from comment #12)
> (In reply to (PTO on 1/9) Bevis Tseng[:bevistseng][:btseng] from comment #9)
> > 
> > Hence, we could keep the data type of the |additional information| as a
> > single numeric value.
> 
> Hi Bevis
> 
> From comment9, you say "we could keep the data type of the |additional
> information| as a single numeric value", but your patch seems to support the
> length of additionalInformation >1.
> So I can't understand, is there anything I miss?

> BTW, if I add additionalInformation for SEND SS from gaia in sprd only, do
> your patch supports it?
> Thanks.

Per discussion offline with reviewer, our conclusion is that
1. For SEND SS and other STK proactive commands like SEND_SMS, SEND_USSD which are more suitable to be notified with RIL_UNSOL_STK_EVENT_NOTIFY instead of RIL_UNSOL_STK_PROACTIVE_COMMAND, we will follow the same flow as AOSP to just display some information to the user without sending the TR.
2. According to the 3GPP TS 11.14 and TS 31.111, the only case that requires more than 1 octet of additional information is |SEND SS|.
3. For the better Web API design, we decide not to expose hard-to-understand information to application or some back-door(an opaque array) to the malicious apps.
4. Due to the reasons above, we provide the additional information as a single numeric value.

For better integration by sprd in the upcoming releases, we would suggest sprd to follow the same design as AOSP to have these TR handled in modem instead.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1 v2: Add Support of "Additional Info on Result" in STK Terminal Response.

1. Address comment 9.
2. Web IDL change of 
   - add "additionalInformation" in MozStkResponse.
   - Revise comments in MozStkCommandEvent for better understanding.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2 v2: Add Test case to verify additional information inside TR.

Address comment 9.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8547341 [details] [diff] [review]
Part 1 v2: Add Support of "Additional Info on Result" in STK Terminal Response.

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

r=me with comment addressed, thank you Bevis!

::: dom/webidl/MozIccManager.webidl
@@ +149,5 @@
>    /** Bearer independent protocol error */
>    const unsigned short STK_RESULT_BIP_ERROR                         = 0x3a;
>  
>    /**
> +   * Additional information on result.

Let's make the comment clearer a bit. Say that this is the additional information on the result for ME problem. 
@ TS 11.14, clause 12.12.2.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Hsin-Yi Tsai (OOO Jan. 8 ~ Jan. 13) [:hsinyi] from comment #16)
> Let's make the comment clearer a bit. Say that this is the additional
> information on the result for ME problem. 
> @ TS 11.14, clause 12.12.2.

Will do. Thanks!

Comment 18

[Edgar Chen [:edgar]]

Comment on attachment 8547341 [details] [diff] [review]
Part 1 v2: Add Support of "Additional Info on Result" in STK Terminal Response.

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

::: dom/webidl/MozStkCommandEvent.webidl
@@ +89,5 @@
>     * there is a conflict of priority level of alerting such as incoming calls
>     * or a low battery warning. In that situation, the resolution is left to
>     * the terminal. If the command is rejected in spite of the high priority,
>     * the terminal shall inform the ICC with resultCode is
> +   * MozIccManager.STK_RESULT_TERMINAL_CRNTLY_UNABLE_TO_PROCESS in MozStkResponse.

Thanks for doing this. ;)

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8547342 [details] [diff] [review]
Part 2 v2: Add Test case to verify additional information inside TR.

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

Looks good, thank you.

::: dom/system/gonk/tests/test_ril_worker_stk.js
@@ +227,5 @@
>    do_test(true);
>    // Test "No" response
>    do_test(false);
> +
> +  run_next_test();

Nice catch!!!

@@ +242,5 @@
> +    let pduHelper = context.GsmPDUHelper;
> +
> +    buf.sendParcel = function() {
> +      // Type
> +      do_check_eq(this.readInt32(), REQUEST_STK_SEND_TERMINAL_RESPONSE);

Just FYI, do_check_eq() is deprecated since gecko 32 [1]. And bug 1120805 is filed for removing those deprecated function used in RIL xpcshell tests.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#XPCShell_test_utility_functions

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=689af6b7a27b

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

update final patch which addresses suggestion in comment 16.

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Add Test case to verify additional information inside TR. r=echen

update final patch.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8551103 [details] [diff] [review]
Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

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 #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8551104 [details] [diff] [review]
Part 2: Add Test case to verify additional information inside TR. r=echen

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 #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.1] Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.1] Part 2: Add Test case to verify additional information inside TR. r=echen

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.0] Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8551588 [details] [diff] [review]
[v2.0] Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

Bug caused by (feature/regressing bug #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.0] Part 2: Add Test case to verify additional information inside TR. r=echen

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: The test case was passed in bug 1015833 with QC RIL enabled. This fix is for the same test case with MOZ-RIL enabled.
Testing completed: Yes. Test case is included.
Risk to taking this patch (and alternatives if risky): NO.
String or UUID changes made by this patch:N/A

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.0m] Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [v2.0m] Part 2: Add Test case to verify additional information inside TR. r=echen

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8551588 [details] [diff] [review]
[v2.0] Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

no impact for v2.0

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8551589 [details] [diff] [review]
[v2.0] Part 2: Add Test case to verify additional information inside TR. r=echen

no impact for v2.0

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a558364e1021
https://hg.mozilla.org/integration/b2g-inbound/rev/ee6a7811ff8c

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a558364e1021
https://hg.mozilla.org/mozilla-central/rev/ee6a7811ff8c

Comment 37

[bhavana bajaj [:bajaj]]

Comment on attachment 8551103 [details] [diff] [review]
Part 1: Add Support of "Additional Info on Result" in STK Terminal Response. r=echen,htsai

cc :mvines so he is aware of these changes.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/533e3459926a
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0676c4319211

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0ab5c40a0654
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/804697da948c

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

NI for 2.0m uplift.
Thank!

Comment 40

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/6f4b49203de5
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/54e16865026b

Comment 41

[Vincent Liu[:vliu]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/0ab5c40a0654