791935 - B2G STK: Implement 'MT Call Event', 'Call Connected' and 'Call Disconnected' Envelope commands

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

See TS 11.14 clause 11.1

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Send MT Call envelope when call arrives.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 662460 [details] [diff] [review]
WIP: Send MT Call envelope when call arrives.

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

Hi, Patrick
Thanks for your great work.

::: dom/system/gonk/ril_worker.js
@@ +2198,5 @@
> +    command.deviceId = {
> +      sourceId: STK_DEVICE_ID_ME,
> +      destinationId: STK_DEVICE_ID_NETWORK
> +    };
> +    command.eventList = 00;  // MT Call

In the Part 2 Patch of Bug 790543, there is a const STK_EVENT_TYPE_MT_CALL for this.

@@ +2259,5 @@
>                   (options.locationStatus ? 3 : 0) +
>                   (options.locationInfo ?
>                      (options.locationInfo.cellId > 0xffff ? 11 : 9) :
> +                    0) +
> +                 (options.transactionId ? options.transactionId.length + 2 : 0);

From TS 11.14, clause 11.1.2 
"Transaction identifier: the transaction identifier data object shall contain *one* transaction identifier", so I think the length of the ComprehesionTLV should be 3.

@@ +2342,5 @@
> +    // Transaction Id
> +    if (options.transactionId) {
> +      GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_TRANSACTION_ID |
> +                                 COMPREHENSIONTLV_FLAG_CR);
> +      GsmPDUHelper.writeHexOctet(options.transactionId.length);

GsmPDUHelper.writeHexOctet(1);

@@ +2345,5 @@
> +                                 COMPREHENSIONTLV_FLAG_CR);
> +      GsmPDUHelper.writeHexOctet(options.transactionId.length);
> +      for (let i = 0; i < options.transactionId.length; i++) {
> +        GsmPDUHelper.writeHexOctet(options.transactionId[i]);
> +      }

ditto, there should be only 1 trans-id

@@ +2902,5 @@
>          newCall.isActive = this._isActiveCall(newCall.state);
> +        this.sendStkMTCall({
> +          address: newCall.number,
> +          transactionId: [0]
> +        });

might need to discuss here,
should we do this in gecko?

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope

1. For transaction ID, we could query it with phone number.
2. For cause of "Call disconnected" envelope, we could get it from the error string that is passed to gaia.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Correct typo in previous version.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Sorry for not writing detail description in the first place,
this feature is needed by our partners to run their SIM apps,
the SIM apps need to be notified when these call events happen.

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Making call events to share the same dictionary data structure.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 663258 [details] [diff] [review]
WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope

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

Excellent,
but I think we should provide a better documentation here so others can understand this more easily.

::: dom/icc/interfaces/SimToolKit.idl
@@ +415,5 @@
> +
> +dictionary MozStkCallEvent
> +{
> +  /**
> +   * Remote phone number.

Just discuss with Hsinyi, 
'Remote Party number' may be a better description.

@@ +420,5 @@
> +   */
> +  DOMString number;
> +
> +  /**
> +   * This field available in Call Connected and Call Disconnected events.

This field *is* available.

And maybe you could use those constants directly, say 
'STK_EVENT_TYPE_CALL_CONNECTED', 'STK_EVENT_TYPE_CALL_DISCONNECTED'

@@ +421,5 @@
> +  DOMString number;
> +
> +  /**
> +   * This field available in Call Connected and Call Disconnected events.
> +   * For Call Connected, setting this true means the connection is answered by

setting this *to* true.

@@ +424,5 @@
> +   * This field available in Call Connected and Call Disconnected events.
> +   * For Call Connected, setting this true means the connection is answered by
> +   * remote end, that is, this is an outgoing call. For Call Disconnected,
> +   * setting this true indicates the connection is hung up by remote.
> +   */

I think we can make this comment more simpler, as you already did.

For STK_EVENT_TYPE_CALL_CONNECTED, setting this to true for an outgoing call, false otherwise.

For STK_EVENT_TYPE_CALL_DISCONNECTED, ...

@@ +429,5 @@
> +  boolean isIssuedByRemote;
> +
> +  /**
> +   * This field is available in Call Disconnected event to indicate the cause
> +   * of disconnection, the failure string that is with notifyError

Can you provide more documentation about this 'notifyError' ?

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +200,5 @@
>     * @param eventData
>     *        Data for this event.
>     *        When eventType is STK_EVENT_TYPE_LOCATION_STATUS,
>     *        eventData is MozStkLocationEvent.
> +   *        For call events, say, the eventType is STK_EVENT_TYPE_MT_CALL,

When eventType is 
- STK_EVENT_TYPE_MT_CALL,
- ...

@@ +202,5 @@
>     *        When eventType is STK_EVENT_TYPE_LOCATION_STATUS,
>     *        eventData is MozStkLocationEvent.
> +   *        For call events, say, the eventType is STK_EVENT_TYPE_MT_CALL,
> +   *        STK_EVENT_TYPE_CALL_CONNECTED and STK_EVENT_TYPE_CALL_DISCONNECTED,
> +   *        eventData shell be MozStkCallEvent.

*is*, or at least 'shall be'

Comment 10

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #9)
> Comment on attachment 663258 [details] [diff] [review]
> WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope
> 
> Review of attachment 663258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent,
> but I think we should provide a better documentation here so others can
> understand this more easily.
> 
> ::: dom/icc/interfaces/SimToolKit.idl
> @@ +415,5 @@
> > +
> > +dictionary MozStkCallEvent
> > +{
> > +  /**
> > +   * Remote phone number.
> 
> Just discuss with Hsinyi, 
> 'Remote Party number' may be a better description.
I think we can just use 'Call number' here.

Comment 11

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #9)
> > Comment on attachment 663258 [details] [diff] [review]
> > WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope
> > 
> > Review of attachment 663258 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Excellent,
> > but I think we should provide a better documentation here so others can
> > understand this more easily.
> > 
> > ::: dom/icc/interfaces/SimToolKit.idl
> > @@ +415,5 @@
> > > +
> > > +dictionary MozStkCallEvent
> > > +{
> > > +  /**
> > > +   * Remote phone number.
> > 
> > Just discuss with Hsinyi, 
> > 'Remote Party number' may be a better description.
> I think we can just use 'Call number' here.
ril.h uses 'Remote party number'. It seems better to take yoshi's suggestion here.

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

Haven't found the transaction id and subaddress (How do we get the SETUP message that described in ts 04.08?)

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 663915 [details] [diff] [review]
WIP: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

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

Excellent, pretty close now.
Can you check my comments below, and see if my opinion makes sense?

Thank you.

::: dom/system/gonk/ril_worker.js
@@ +2310,5 @@
> +        command.deviceId = {
> +          sourceId :STK_DEVICE_ID_NETWORK,
> +          destinationId: STK_DEVICE_ID_SIM
> +        };
> +        command.eventList = STK_EVENT_TYPE_MT_CALL;

Since each case inside switch uses this, 
do you think can we move this line before 'switch'? for instance,
command.eventList = command.eventType;

@@ +2311,5 @@
> +          sourceId :STK_DEVICE_ID_NETWORK,
> +          destinationId: STK_DEVICE_ID_SIM
> +        };
> +        command.eventList = STK_EVENT_TYPE_MT_CALL;
> +        command.transactionId = 

trailing white space, and some lines below.

@@ +2335,5 @@
> +        command.eventList = STK_EVENT_TYPE_CALL_DISCONNECTED;
> +        command.transactionId = 
> +          this._getTransactionIdByNumber(command.eventData.number);
> +        command.cause = command.eventData.error;
> +        break;

Also I notice these two cases use some similar code, can we use 'fall through' for these? 
like 
case STK_EVENT_TYPE_CALL_CONNECTED:
case STK_EVENT_TYPE_CALL_DISCONNECTED: // Fall through
...

@@ +5771,5 @@
>    },
> +
> +  /**
> +   * Give a geckoError string, this function translate into cause value
> +   * and write the value into buffer.

My English is not quite well, but these comments seems to have some problems, or the sentence seems not well formatted, for example, *Given*, or *translates*, can you check this again?

Also add @param geckoError and add some comments for it.

@@ +5773,5 @@
> +  /**
> +   * Give a geckoError string, this function translate into cause value
> +   * and write the value into buffer.
> +   */
> +  writeErrorNumber(geckoError) {

I think this utility function is for Cause dat object of Comprehension-TLV.
Can you move this function into ComprehentionTlvHelper?

@@ +5783,5 @@
> +      }
> +    }
> +    cause = (cause == -1) ? ERROR_SUCCESS : cause;
> +    this.writeHexOctet(0x60);
> +    this.writeHexOctet(0x80 | cause);

Although we talk about this, but others might not know what these means, also maybe several months later we also forget this.
Can you add some documentations for this ?
For example, You can check several examples from SMS-related or ICC-related functions in ril_worker.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case for writeErrorNumber

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part1: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Hi, philikon,

The IDL is based on patch of bug 790547, adds support for STK call events.
Would you help to review this for me? Thanks.

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

Hi, philikon,

This patch is the implementation of sending MT call, call connected and call disconnected envelopes.
Would you help to review this for me?
Thanks.

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case for writeCauseTlv

Hi, philikon,

The test case of writeCauseTlv of part 2.
Would you help to review this for me?

Thanks.

Comment 20

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664427 [details] [diff] [review]
Part1: IDL Change for MT Call, Call connected, Call Disconnected Envelope

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

r+ with the nits below addressed. Although keep in mind what I pointed out in bug 790547 comment 27 about keeping the object hierarchies flatter.

::: dom/icc/interfaces/SimToolKit.idl
@@ +432,5 @@
> +
> +  /**
> +   * This field is available in Call Disconnected event to indicate the cause
> +   * of disconnection, the failure string that is with
> +   * nsIRILTelephonyCallback.notifyError() callback. Null if there's no error.

nsIRILTelephonyCallback is an internal interface. It makes no sense to mention it here. (Also, the grammar of that sentence doesn't quite work.) Please write this documentation for somebody who knows what the STK is but doesn't know the implementation details of Gecko.

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +200,5 @@
>     * @param eventData
>     *        Data for this event.
>     *        When eventType is STK_EVENT_TYPE_LOCATION_STATUS,
>     *        eventData is MozStkLocationEvent.
> +   *        When event type is STK_EVENT_TYPE_MT_CALL,

s/event type/eventType/

Comment 21

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664429 [details] [diff] [review]
Part 2: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

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

Looks good apart from the two things mentioned below. Please address them. Also, Yoshi and Hsinyi have written unit tests for their TLV helper code. Please do the same.

::: dom/system/gonk/ril_worker.js
@@ +2287,5 @@
>     * @param eventData
>     */
>    sendStkEventDownload: function sendStkEventDownload(command) {
>      command.tag = BER_EVENT_DOWNLOAD_TAG;
> +    command.eventList = command.eventType;

This is starting to look like a no-op to me. Why is this line necessary, why can't we simply look at `command.eventType` in the rest of the code?

@@ +6501,5 @@
> +   * @param geckoError Error string that is passed to gecko.
> +   */
> +  writeCauseTlv: function writeCauseTlv(geckoError) {
> +    let cause = -1;
> +    for each(let errorNo in Object.keys(RIL_ERROR_TO_GECKO_ERROR)) {

for (errorNo in RIL_ERROR_TO_GECKO_ERROR)

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664429 [details] [diff] [review]
Part 2: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

Bah, for some I reason I wasn't aware of your tests patch. Never mind! r=me with comment 21 addressed.

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664437 [details] [diff] [review]
Test case for writeCauseTlv

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

Great. Can we also get tests for the new envelope command type you added support for?

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> Comment on attachment 664429 [details] [diff] [review]
> Part 2: Add call connected, call disconnected and mt call Envelope command
> to sendStkEventDownlad
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2287,5 @@
> >     * @param eventData
> >     */
> >    sendStkEventDownload: function sendStkEventDownload(command) {
> >      command.tag = BER_EVENT_DOWNLOAD_TAG;
> > +    command.eventList = command.eventType;
> 
> This is starting to look like a no-op to me. Why is this line necessary, why
> can't we simply look at `command.eventType` in the rest of the code?
> 

Hi philikon:
The eventList naming is from STK spec(TS 11.14), actually it will be written as a TLV, and its value part is a list with only *one* event in it.

So in IDL we try to prevent confusion, we simply renamed it to eventType.

That's why we did the renaming here, eventList is to confirm the STK spec, eventType is for IDL and in order to have a better understanding of this value.

How do you think?

Thank you.

Comment 25

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Philipp von Weitershausen [:philikon] from comment #23)
> Comment on attachment 664437 [details] [diff] [review]
> Test case for writeCauseTlv
> 
> Review of attachment 664437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great. Can we also get tests for the new envelope command type you added
> support for?

These envelopes are processed by SIM card and they are not expected to have response values. I think it might not easy to write a test case for them. Instead, unit tests for each TLV are provided to make sure each parts of an envelop are correct.

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Fix nits.

Comment 27

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

Fix previous version with comment 21.

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Hi Jonas,

The interface is based on IDL proposed in bug 790547. Add MT Call, Call connected and Call disconnected envelopes.

Thank you.

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

Rebased on bug 790547.

Comment 30

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #24)
> The eventList naming is from STK spec(TS 11.14), actually it will be written
> as a TLV, and its value part is a list with only *one* event in it.
> 
> So in IDL we try to prevent confusion, we simply renamed it to eventType.
> 
> That's why we did the renaming here, eventList is to confirm the STK spec,
> eventType is for IDL and in order to have a better understanding of this
> value.

Ah, ok that makes sense.

Comment 31

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I just discussed with kk1fff,
his patch depends on patch from Bug 791939,
but Bug 791939 might need to test on the SIM apps first.

In order not to prevent blocking this patch,
I suggested kk1fff can move the util function he will use from Bug 791939 into this patch.

Comment 32

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #31)
> I just discussed with kk1fff,
> his patch depends on patch from Bug 791939,
> but Bug 791939 might need to test on the SIM apps first.
> 
> In order not to prevent blocking this patch,
> I suggested kk1fff can move the util function he will use from Bug 791939
> into this patch.

Patrick, I agree with yoshi's comment here, and sorry for blocking you for a while. Please feel free to move the parts about "getSizeOfLengthOctets()" and "writeLength()" into yours. I'will rebase my patches then. :)

Comment 33

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: IDL Change for MT Call, Call connected, Call Disconnected Envelope

Update patch message.

Comment 34

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Add call connected, call disconnected and mt call Envelope command to sendStkEventDownlad

1. Rebase.
2. Move "getSizeOfLengthOctets()" and "writeLength()" functions from bug 791939.

Comment 35

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Test case

1. Rebase.
2. Merge helper test cases for getSizeOfLengthOctets() and writeLength() from bug 791939.

Comment 36

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13085adbbf5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a95427068a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc03c67a4bb

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13085adbbf5d
https://hg.mozilla.org/mozilla-central/rev/c2a95427068a
https://hg.mozilla.org/mozilla-central/rev/6dc03c67a4bb