Closed Bug 1117609 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

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: Jinghua.Xing, Assigned: bevis)

References

Details

(Whiteboard: [sprd389929])

Attachments

(8 files, 5 obsolete files)

18.83 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.97 KB, patch
bevis
: review+
Details | Diff | Splinter Review
18.84 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.98 KB, patch
bevis
: review+
Details | Diff | Splinter Review
18.70 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.33 KB, patch
bevis
: review+
Details | Diff | Splinter Review
18.71 KB, patch
bevis
: review+
Details | Diff | Splinter Review
3.31 KB, patch
bevis
: review+
Details | Diff | Splinter Review
[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)
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.
Attached patch display.patch (obsolete) — Splinter Review
And I have made a patch for this issue, please help to check this issue. 

Thank you.
Flags: needinfo?(vchen)
Flags: needinfo?(sku)
Flags: needinfo?(frsela)
Flags: needinfo?(21)
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
Component: Gaia::Settings → RIL
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Summary: [FFOS7715 v2.1][STK]27.22.4.1.1/2 DISPLAY TEXT, normal priority, Unpacked 8 bit data for Text String, screen busy case failed → [B2G][STK] Support of "Additional Info on Result" in STK Terminal Response.
(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. :)
Assignee: nobody → btseng
Flags: needinfo?(vchen)
Flags: needinfo?(sku)
Flags: needinfo?(frsela)
Flags: needinfo?(21)
(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.
(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!
Whiteboard: [sprd389929]
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!
Attachment #8543736 - Attachment is obsolete: true
Attachment #8543923 - Flags: review?(echen)
Add corresponding test case for verification.

Hi Edgar,

May I have your review for this test as well?

Thanks!
Attachment #8543925 - Flags: review?(echen)
Blocks: b2g-stk
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 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.
Attachment #8543923 - Flags: review?(echen)
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.
Attachment #8543925 - Flags: review?(echen)
See Also: → 1015833
(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.
Flags: needinfo?(btseng)
(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.
Flags: needinfo?(btseng)
1. Address comment 9.
2. Web IDL change of 
   - add "additionalInformation" in MozStkResponse.
   - Revise comments in MozStkCommandEvent for better understanding.
Attachment #8543923 - Attachment is obsolete: true
Attachment #8547341 - Flags: review?(htsai)
Attachment #8547341 - Flags: review?(echen)
Address comment 9.
Attachment #8543925 - Attachment is obsolete: true
Attachment #8547342 - Flags: review?(echen)
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.
Attachment #8547341 - Flags: review?(htsai) → review+
(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 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. ;)
Attachment #8547341 - Flags: review?(echen) → review+
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
Attachment #8547342 - Flags: review?(echen) → review+
Duplicate of this bug: 1122325
Blocks: Woodduck
blocking-b2g: --- → 2.0M+
update final patch which addresses suggestion in comment 16.
Attachment #8547341 - Attachment is obsolete: true
Attachment #8551103 - Flags: review+
update final patch.
Attachment #8547342 - Attachment is obsolete: true
Attachment #8551104 - Flags: review+
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
Attachment #8551103 - Flags: approval-mozilla-b2g37?
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
Attachment #8551104 - Flags: approval-mozilla-b2g37?
[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
Attachment #8551586 - Flags: review+
Attachment #8551586 - Flags: approval-mozilla-b2g34?
[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
Attachment #8551587 - Flags: review+
Attachment #8551587 - Flags: approval-mozilla-b2g34?
[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
Attachment #8551588 - Flags: review+
Attachment #8551588 - Flags: approval-mozilla-b2g34?
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
Attachment #8551588 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g32?
[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
Attachment #8551589 - Flags: review+
Attachment #8551589 - Flags: approval-mozilla-b2g32?
Blocks: 1122330
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
Attachment #8551588 - Flags: approval-mozilla-b2g32?
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
Attachment #8551589 - Flags: approval-mozilla-b2g32?
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.
Attachment #8551103 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8551104 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8551586 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Attachment #8551587 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
NI for 2.0m uplift.
Thank!
Flags: needinfo?(kli)
You need to log in before you can comment on or make changes to this bug.