791939 - B2G STK: Implement 'Call Control' Envelope command

Status: RESOLVED WONTFIX

(Core :: DOM: Device Interfaces, defect)

Description

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

See TS 11.14 clause 9.1

Comment 1

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

Attachment: WIP1: send Envelope command (call control)

Need to keep working on the response of the envelope command.

Comment 2

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

Attachment: WIP part1: send Envelope command (call control)

Need to keep working on the response of the envelope command.

Comment 3

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

Comment on attachment 662482 [details] [diff] [review]
WIP part1: send Envelope command (call control)

Sorry about a typo...

Comment 4

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

Attachment: WIP part1: send Envelope command (call control)

Need to keep working on the response of the envelope command.

Thanks!

Comment 5

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

Comment on attachment 662484 [details] [diff] [review]
WIP part1: send Envelope command (call control)

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

Great, thanks for your quick patch.
This is what I have in mind in the first place.

Also there are small nits should be addressed.

::: dom/system/gonk/ril_worker.js
@@ +1754,1 @@
>    },

Since this function is dial,
to make this function more clear,
I think we should not call sendStlCallControl here

@@ +2232,5 @@
> +    * Send STK Envelope(Call Control) command.
> +    * @param dialingNumber
> +    */
> +    sendStkCallControl: function sendStkCallControl(dialingNumber) {
> +      command.tag = BER_CALL_CONTROL_TAG;

where does command come from?

@@ +2270,5 @@
>                   (options.locationInfo ?
>                      (options.locationInfo.cellId > 0xffff ? 11 : 9) :
> +                    0) +
> +                 (options.dialingNumber ?
> +                    ((options.dialingNumber.length + 1) / 2) + 3 : 0);

seems wrong here.

@@ +2349,5 @@
>          GsmPDUHelper.writeHexOctet(loc.cellId & 0xff);
>        }
>      }
>  
> +    if (options.diallingNumber) {

dialling or dialing?
Since we already used 'Dialling' in other places, 
do you think it's better if we should make it more consistent naming?

Comment 6

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

Attachment: WIP part1 (v2): send Envelope command (call control)

Comment 5 addressed.

Comment 7

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #5)
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1754,1 @@
> >    },
> 
> Since this function is dial,
> to make this function more clear,
> I think we should not call sendStlCallControl here
> 
Thanks for the feedback!

I renamed the function 'requestSimCallControl()' and added some comments to make it clearer.

> @@ +2232,5 @@
> > +    * Send STK Envelope(Call Control) command.
> > +    * @param dialingNumber
> > +    */
> > +    sendStkCallControl: function sendStkCallControl(dialingNumber) {
> > +      command.tag = BER_CALL_CONTROL_TAG;
> 
> where does command come from?

The constant had been added in ril_const.js in the patch according to the spec.

> 
> @@ +2270,5 @@
> >                   (options.locationInfo ?
> >                      (options.locationInfo.cellId > 0xffff ? 11 : 9) :
> > +                    0) +
> > +                 (options.dialingNumber ?
> > +                    ((options.dialingNumber.length + 1) / 2) + 3 : 0);
> 
> seems wrong here.
> 

Thanks for pointing this out.
I addressed this.

> @@ +2349,5 @@
> >          GsmPDUHelper.writeHexOctet(loc.cellId & 0xff);
> >        }
> >      }
> >  
> > +    if (options.diallingNumber) {
> 
> dialling or dialing?
> Since we already used 'Dialling' in other places, 
> do you think it's better if we should make it more consistent naming?

Hmmm... 'dialing' is used in the part of 'telephony' stuff, though 'dialling number' is adopted in the PDU-related part. But okay, I use 'diallingNumber' to conform to the PUD usage.

Comment 8

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> Created attachment 662808 [details] [diff] [review]
> WIP part1 (v2): send Envelope command (call control)
> 
> Comment 5 addressed.

The patch bases on those of Bug 792335.

Comment 9

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

Attachment: WIP - Patch to check ICC supports "Call Control"

Comment 10

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

Attachment: WIP - Patch to check ICC supports "Call Control"

Forgot qref

Comment 11

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

Attachment: WIP (v3): send Envelope command (call control)

WIP: request ICC call control by sending envelope (call control) command if ICC supports this feature. Need to base on attachment  662912 [details] [diff] [review].

Keep working on processing the response of the envelope command.

Comment 12

[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,
When the SIM supports 'Call Control' service, each time when user dials a telephony call, ME needs to send this 'Call Control Envelope Command' to ICC, and ICC can do following actions:
1. Allowed,
2. Denied
3. Allowed, but with the dialling number modified by ICC.

Comment 13

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

Attachment: WIP (v4): send Envelope command (call control)

Merged attachment 662912 [details] [diff] [review] and rebased on patches of bug 736706.

Comment 14

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

Attachment: Write Length of Comprehension-TLV util

Comment 15

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

Attachment: WIP (v5): send Envelope command (call control)

WIP v5: revised 'writeHexOctet' for locationInfo & merged 'comprehensionTlvHelper.writeLength' in attachment 663891 [details] [diff] [review]

Comment 16

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

Attachment: Patch: testcase

verify "ComprehensionTlvHelper.getSizeOfLengthOctets" and "ComprehensionTlvHelper.writeLength"

Comment 17

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

Comment on attachment 664021 [details] [diff] [review]
WIP (v5): send Envelope command (call control)

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

Thanks for your hard work. :)

Comment 18

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

Attachment: Patch part1: send Envelope command (call control)

Patch: request ICC call control by sending icc envelope (call control) command.

This patch bases on Bug 790547 for using "ComprehensionTlvHelper.writeLocationInfoTlv()".
This also bases on Bug Bug 736706 to refactor the respons of "REQUEST_STK_SEND_ENVELOPE_WITH_STATUS".

Comment 19

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

Attachment: Patch part2: testcase

The testcase verifies "ComprehensionTlvHelper.getSizeOfLengthOctets" and "ComprehensionTlvHelper.writeLength."

This patch is rebased on Bug 790547.

Comment 20

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

Comment on attachment 664413 [details] [diff] [review]
Patch part1: send Envelope command (call control)

This patch tries to enable ICC call control. If the ICC supports this service and the new dialling call isn't an emergency one, before REQUEST_DIAL, we shall query the service to check whether the call is allowed. If the call is denied by the ICC, we will notify an error of call barring.

This patch bases on Bug 790547 for using "ComprehensionTlvHelper.writeLocationInfoTlv()", and on Bug 736706 to refactor the respons of "REQUEST_STK_SEND_ENVELOPE_WITH_STATUS".

Yoshi's comments have been addressed as well.

Comment 21

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664413 [details] [diff] [review]
Patch part1: send Envelope command (call control)

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

Please be sure to run the telephony Marionette tests. This *shouldn't* break them, but you never know.

The code looks pretty good, so r=me, but I would like to see my questions/comments addressed.

::: dom/system/gonk/ril_worker.js
@@ +1251,5 @@
>      });
>    },
>  
> +  updateICCServiceTable: function updateICCServiceTable() {
> +    this.iccServices = {};

Should this perhaps live under `this.iccInfo` so it gets cleared / re-initialized correctly. E.g. this.iccInfo.serviceTable or something?

@@ +1818,5 @@
>     */
>    dial: function dial(options) {
>      let dial_request_type = REQUEST_DIAL;
> +
> +    // Determine the dial request type.

Can we not move this into _dial()? I think it's only used for that case anyway.

@@ +1845,5 @@
> +    // An emergency call, instead of being controlled by the ICC, needs to be
> +    // placed directly.
> +    let iccCallControlEnabled = !(this.voiceRegistrationState.emergencyCallsOnly ||
> +                                  options.isDialEmergency ||
> +                                  this._isEmergencyNumber(options.number));

The double negative (negating the expression here and the `else` below) is a bit hard to read. Why not:

  // Place a call directly if the ICC doesn't support call control or if we're
  // dealing with an emergency call.
  if (!this.iccServices.callControl ||
      this.voiceRegistrationState.emergencyCallsOnly ||
      options.isDialEmergency ||
      this._isEmergencyNumber(options.number)) {
    this._dial(options);
    return;
  }
  this.requestIccCallControl(options);

@@ +3203,5 @@
> +      this.sendDOMMessage(options);
> +      return;
> +    }
> +
> +    Buf.readStringDelimiter(length);

It seems really odd to have this here when there's a few early returns higher up.

Also, where does `length` come from? It doesn't seem to be defined in this function. Seems like this line is somewhat misplaced. And that you didn't test this code very well.

Lastly, what should happen when neither of the `if` blocks are entered? Remember that in the DOM we have already created a call object for this call, so we should definitely make sure the call gets terminated. In fact, above where you notify the error, we should also make sure to send a DISCONNECTED event in addition to the error (probably before the error.)

@@ +4458,5 @@
> +        this.acknowledgeSMS(false, PDU_FCS_UNSPECIFIED);
> +        break;
> +      case BER_CALL_CONTROL_TAG:
> +        this._dial(REQUEST_DIAL, options);
> +        break;

So this is to ensure that if the STK envelope stuff fails, we still place the call the conventional way? If so, why do we do that while indicating acknowledging failure for the SMS?

Would be good to explain in a comment.

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 664414 [details] [diff] [review]
Patch part2: testcase

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

I like tests.

Comment 23

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

Attachment: Patch part1: send Envelope command (call control) (v2) r=philikon

Comment 21 addressed:
1) remove "updateICCServiceTable()" in v1, and directly use "isICCServiceAvailable()" to check "CALL_CONTRL" service
2) refactor dial() and _dial()
3) correct the error handling for response of "REQUEST_STK_SEND_ENVELOPE_WITH_STATUS"

Comment 24

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

(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> Comment on attachment 664413 [details] [diff] [review]
> Patch part1: send Envelope command (call control)
> 
> Review of attachment 664413 [details] [diff] [review]:
> -----------------------------------------------------------------

Philipp, thanks for the comments.
My responses are inline.

> 
> Please be sure to run the telephony Marionette tests. This *shouldn't* break
> them, but you never know.
>

I ran the tests and them passed.

> The code looks pretty good, so r=me, but I would like to see my
> questions/comments addressed.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1251,5 @@
> >      });
> >    },
> >  
> > +  updateICCServiceTable: function updateICCServiceTable() {
> > +    this.iccServices = {};
> 
> Should this perhaps live under `this.iccInfo` so it gets cleared /
> re-initialized correctly. E.g. this.iccInfo.serviceTable or something?

I removed this since there has been 'this.iccInfo.sst' and an update mechanism already. 
> 
> @@ +1818,5 @@
> >     */
> >    dial: function dial(options) {
> >      let dial_request_type = REQUEST_DIAL;
> > +
> > +    // Determine the dial request type.
> 
> Can we not move this into _dial()? I think it's only used for that case
> anyway.
> 

I refactored dial() and _dial() as you suggested. 

> @@ +1845,5 @@
> > +    // An emergency call, instead of being controlled by the ICC, needs to be
> > +    // placed directly.
> > +    let iccCallControlEnabled = !(this.voiceRegistrationState.emergencyCallsOnly ||
> > +                                  options.isDialEmergency ||
> > +                                  this._isEmergencyNumber(options.number));
> 
> The double negative (negating the expression here and the `else` below) is a
> bit hard to read. Why not:
> 
>   // Place a call directly if the ICC doesn't support call control or if
> we're
>   // dealing with an emergency call.
>   if (!this.iccServices.callControl ||
>       this.voiceRegistrationState.emergencyCallsOnly ||
>       options.isDialEmergency ||
>       this._isEmergencyNumber(options.number)) {
>     this._dial(options);
>     return;
>   }
>   this.requestIccCallControl(options);
> 

Sure! It is easier to read now. Thanks!

> @@ +3203,5 @@
> > +      this.sendDOMMessage(options);
> > +      return;
> > +    }
> > +
> > +    Buf.readStringDelimiter(length);
> 
> It seems really odd to have this here when there's a few early returns
> higher up.
> 
> Also, where does `length` come from? It doesn't seem to be defined in this
> function. Seems like this line is somewhat misplaced. And that you didn't
> test this code very well.
> 

Sorry that I didn't detect this in my manual test. 

> Lastly, what should happen when neither of the `if` blocks are entered?
> Remember that in the DOM we have already created a call object for this
> call, so we should definitely make sure the call gets terminated. In fact,
> above where you notify the error, we should also make sure to send a
> DISCONNECTED event in addition to the error (probably before the error.)
> 

I enhanced _processStkEnvelopeCallControl() to cover other 'else' situation, notifying call error 'CALL_FAIL_ERROR_UNSPECIFIED'. 

The call error mechanism itself (in TelephonyCall.cpp [1]) handles the disconnected event. That's why I don't deal with this in ril_worker.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyCall.cpp#131

> @@ +4458,5 @@
> > +        this.acknowledgeSMS(false, PDU_FCS_UNSPECIFIED);
> > +        break;
> > +      case BER_CALL_CONTROL_TAG:
> > +        this._dial(REQUEST_DIAL, options);
> > +        break;
> 
> So this is to ensure that if the STK envelope stuff fails, we still place
> the call the conventional way? If so, why do we do that while indicating
> acknowledging failure for the SMS?
> 
> Would be good to explain in a comment.

After discussing with yoshi and vicamo, it should notify call error here, not _dial(). I corrected the error handling in the patch v2. Thanks!

Comment 25

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

Remark: as I mentioned in bug 736706, Otoro doesn't support "RIL_REQUEST_STK_SEND_ENVELOPE_WITH_STATUS". We shall detect whether ril supports this or not. If not, we need to use "RIL_REQUEST_STK_SEND_ENVELOPE_COMMAND" instead. But please note, some error situations may not be distinguished as in fine-grained as spec says though. 

I filed bug 794404 for this.

Comment 26

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

Attachment: Patch part1: send Envelope command (call control) (v3)

v3: revised '_processStkEnvelopeCallControl()'

Comment 27

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

Comment on attachment 665261 [details] [diff] [review]
Patch part1: send Envelope command (call control) (v3)

Hi Yoshi,

I was missing something that spec says when processing responses of the envelope command. This patch corrects that part. Would you please give me some feedback, especially regarding the PDU processing? Thank you.

Comment 28

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

Attachment: Patch part1: send Envelope command (call control) (v4)

v4: revised _processStkEnvelopeCallControl as explained in comment 27.

Comment 29

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

Comment on attachment 665282 [details] [diff] [review]
Patch part1: send Envelope command (call control) (v4)

Hi Philipp,

All your comments in comment 21 have been addressed. Patch v4 mainly corrects "_processStkEnvelopeCallControl()" as I explained in comment 27. Since I added new logic there, would you please review this again? Thank you?

Comment 30

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

Comment on attachment 665282 [details] [diff] [review]
Patch part1: send Envelope command (call control) (v4)

need to address some feedback from yoshi

Comment 31

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

Attachment: Patch part1: send Envelope command (call control) (v5)

v5:
1) revised _processStkEnvelopeCallControl() as explained in comment 27
2) used some constants, like TLV_DEVICE_ID_SIZE, defined in bug 790547

Comment 32

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665366 [details] [diff] [review]
Patch part1: send Envelope command (call control) (v5)

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

::: dom/system/gonk/ril_worker.js
@@ +3222,5 @@
> +      options.rilMessageType = "callError";
> +      options.error =
> +        RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[CALL_FAIL_CALL_BARRED];
> +      this.sendDOMMessage(options);
> +    } else {

Seems like you can collapse the `else if` and `else` part and do

  } else {
    options.callIndex = -1;
    options.rilMessageType = "callError";
    if ((sw1 == ICC_STATUS_SAT_BUSY) && (sw2 == 0x00)) {
      // The ICC does not allow this call. Notify error of call barring.
      options.error = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[CALL_FAIL_CALL_BARRED];
    } else {
      options.error = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[CALL_FAIL_ERROR_UNSPECIFIED];
    }
    this.sendDOMMessage(options);
  }

@@ +3245,5 @@
> +
> +    let success = false;
> +    if (((sw1 == ICC_STATUS_NORMAL_ENDING) && (sw2 == 0x00))
> +        || (sw1 == ICC_STATUS_NORMAL_ENDING_WITH_EXTRA)) {
> +      success = true;

let success = ((sw1 == ICC_STATUS_NORMAL_ENDING) && (sw2 == 0x00))
               || (sw1 == ICC_STATUS_NORMAL_ENDING_WITH_EXTRA));

Comment 33

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

Attachment: Patch part1: send Envelope command (call control) (v6) r=philikon

Comment 32 addressed & rebased

Comment 34

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

Attachment: Patch part1: send Envelope command (call control) (v6) r=philikon

Comment 32 addressed & rebased.

Comment 35

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

Attachment: Patch part1: send Envelope command (call control) (v7) r=philikon

getSizeOfLengthOctets() and writeLength() and the testcase have been moved to bug 791935.

Comment 36

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

Hi, Hsinyi
Since otoro doesn't support ENVELOPE_WITH_STATUS,
so if we use SEND_ENVELOPE_COMMAND instead, we won't get sw1/sw2 in response.
Will that cause any problem?

Comment 37

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

Comment on attachment 666568 [details] [diff] [review]
Patch part1: send Envelope command (call control) (v7) r=philikon

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

::: dom/system/gonk/ril_worker.js
@@ +3336,5 @@
> +    let sw1 = Buf.readUint32();
> +    let sw2 = Buf.readUint32();
> +    if (((sw1 == ICC_STATUS_NORMAL_ENDING) && (sw2 == 0x00))
> +        || (sw1 == ICC_STATUS_NORMAL_ENDING_WITH_EXTRA)) {
> +      let length = Buf.readUint32();

Here I think we also need to check the length of the incoming parcel first, i.e. the length from RIL[REQUEST_SEND_ENVELOPE_COMMAND].

Comment 38

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #36)
> Hi, Hsinyi
> Since otoro doesn't support ENVELOPE_WITH_STATUS,
> so if we use SEND_ENVELOPE_COMMAND instead, we won't get sw1/sw2 in response.
> Will that cause any problem?
Since the spec mentions sw1 and sw2, I used ENVELOPE_WITH_STATUS here.
I'll provide a patch using SEND_ENVELOPE_COMMAND to see if we can still make this service work.

Comment 39

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

change to blocking-basecamp ? since otoro doesn't support this.

Comment 40

[Andrew Overholt [:overholt]]

blocking- since Otoro doesn't support this.

Comment 41

[Dietrich Ayala (:dietrich)]

This is blocking 1.0.1 RIL compliance bug. Does it need to block that release?

Comment 42

[Michael Vines [:m1] [:evilmachines]]

This feature will be covered by partners for 1.0.1 so NPOTB

Comment 43

[David Scravaglieri [:scravag]]

Non blocking then

Comment 44

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

Mark this as WONTFIX as it is blocked by another WONTFIX bug 795199.