[B2G] [DSDS] system message format of 'icc-stkcommand'

RESOLVED FIXED in 1.3 Sprint 6 - 12/6

Status

Firefox OS
RIL
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hsinyi, Assigned: edgar)

Tracking

({dev-doc-needed})

unspecified
1.3 Sprint 6 - 12/6
x86_64
Linux
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Right now, each RadioInterface sends out a 'icc-stkcommand' message as below
    gSystemMessenger.broadcastMessage("icc-stkcommand", message);

The message doesn't carry information about 'service/clientId' yet and this is a miss in DSDS.

The possible solution I see would be
    gSystemMessenger.broadcastMessage("icc-stkcommand", {iccId: iccId, message: message});

This format also makes the interface b/w system message and Icc WebAPI align. How does this sound?
(Assignee)

Comment 1

5 years ago
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> Right now, each RadioInterface sends out a 'icc-stkcommand' message as below
>     gSystemMessenger.broadcastMessage("icc-stkcommand", message);
> 
> The message doesn't carry information about 'service/clientId' yet and this
> is a miss in DSDS.
> 
> The possible solution I see would be
>     gSystemMessenger.broadcastMessage("icc-stkcommand", {iccId: iccId,
> message: message});
> 
Thanks to point this out. I will take care of gecko part.

With this change, gaia side need to do corresponding change as well.

I am going to file two gaia stk bugs:
1. Having the backward compatibility to avoid breaking single sim behavior.
2. Support multiple sim card.

> This format also makes the interface b/w system message and Icc WebAPI
> align. How does this sound?
(Assignee)

Updated

5 years ago
Assignee: nobody → echen
(Assignee)

Updated

5 years ago
Blocks: 799023
(Assignee)

Updated

5 years ago
Blocks: 942714
(Assignee)

Comment 2

5 years ago
Created attachment 8338250 [details] [diff] [review]
Part 1: Extend stk system message to support multiple sim, v1

We need to check iccId first then send the system message. Because I found ril may receive stk event before iccId is ready when device boot up.

BTW, I will create the gaia bug for backward compatibility once the reviewer is comfortable with the naming. :)
Attachment #8338250 - Flags: feedback?(htsai)
(Reporter)

Comment 3

5 years ago
Comment on attachment 8338250 [details] [diff] [review]
Part 1: Extend stk system message to support multiple sim, v1

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

Looks good!
Attachment #8338250 - Flags: feedback?(htsai) → feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 8338288 [details] [diff] [review]
Part 2: Marionette tests for stk system message, v1
(Assignee)

Updated

5 years ago
Depends on: 943229
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Attachment #8338250 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #8338288 - Flags: review?(htsai)
(Reporter)

Updated

5 years ago
Attachment #8338250 - Flags: review?(htsai) → review+
(Reporter)

Comment 5

5 years ago
Comment on attachment 8338288 [details] [diff] [review]
Part 2: Marionette tests for stk system message, v1

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

r=me with comments addressed. Thank you!

::: dom/icc/tests/marionette/stk_helper.js
@@ +59,5 @@
> +      runNextTest();
> +    }
> +  };
> +
> +  sendEmulatorCommand("stk pdu " + pdu);

Please move this line to the end of sendStkPduToEmulator(), i.e. line#82.

@@ +62,5 @@
> +
> +  sendEmulatorCommand("stk pdu " + pdu);
> +
> +  navigator.mozSetMessageHandler("icc-stkcommand",
> +    function callHandleSTKCommand(message) {

We are fine without function name property here. Let's remove it.
Attachment #8338288 - Flags: review?(htsai) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 8339271 [details] [diff] [review]
Part 2: Marionette tests for stk system message, v2, r=hsinyi

Address review comment #5.
Attachment #8338288 - Attachment is obsolete: true
Attachment #8339271 - Flags: review+
(Assignee)

Comment 8

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> try server: https://tbpl.mozilla.org/?tree=Try&rev=b3a3ed2a34dd

raise ScriptTimeoutException(message=message, status=status, stacktrace=stacktrace)
TEST-UNEXPECTED-FAIL | test_stk_proactive_command.js | ScriptTimeoutException: timed out
.....

Stk related marionette tests are all time out due didn't receive 'icc-stkcommand' but I can pass these tests in my local environment. Have not idea why failed in try server yet.
(Assignee)

Comment 9

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> (In reply to Edgar Chen [:edgar][:echen] from comment #7)
> > try server: https://tbpl.mozilla.org/?tree=Try&rev=b3a3ed2a34dd
> 
> raise ScriptTimeoutException(message=message, status=status,
> stacktrace=stacktrace)
> TEST-UNEXPECTED-FAIL | test_stk_proactive_command.js |
> ScriptTimeoutException: timed out
> .....
> 
> Stk related marionette tests are all time out due didn't receive
> 'icc-stkcommand' but I can pass these tests in my local environment. Have
> not idea why failed in try server yet.

I can reproduce this in my local environment by running all marionette tests.
(Assignee)

Comment 10

4 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> (In reply to Edgar Chen [:edgar][:echen] from comment #8)
> 
> I can reproduce this in my local environment by running all marionette tests.

It seems the failed is related to the implementation of marionette framework and system message, and needs more time to investigate and figure out how to fix. In order not to block Gaia development, I decide to separate test case into a follow-up bug.
(Assignee)

Updated

4 years ago
Blocks: 945576
(Assignee)

Comment 11

4 years ago
Created attachment 8341495 [details] [diff] [review]
Patch, v1, r=hsinyi

Correct patch title
Attachment #8338250 - Attachment is obsolete: true
Attachment #8339271 - Attachment is obsolete: true
Attachment #8341495 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/84012aceec07
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
(Assignee)

Updated

4 years ago
Blocks: 946165
(Assignee)

Updated

4 years ago
Blocks: 934404
(Assignee)

Updated

4 years ago
Blocks: 951586

Updated

4 years ago
Blocks: 999370
You need to log in before you can comment on or make changes to this bug.