See TS 11.14 clause 11.1

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?

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.

Correct typo in previous version.

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.

Making call events to share the same dictionary data structure.

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'

(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.

(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.

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

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.

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.

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.

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

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 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 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 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?

(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.

(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.

Fix nits.

Fix previous version with comment 21.

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

Rebased on bug 790547.

(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.

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.

(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. :)

Update patch message.

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

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

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

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