983038 - [DSDS] Unable to send DTMF tone for SIM 2 voice connection with IVR system.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Enpei]

Attachment: time stamp: 14:39 failed with SIM 2(call 99222).

When dialing IVR system or voicemail system from SIM 2, press keypad to send command via DTMF is not working.


* Build Number  
Gaia      b5d0f9b0cd5dce38b79287a9363226b912929270
Gecko     7312341e00e2785419fc4746e291646299ba3018
BuildID   20140313120331
Version   30.0a1

* Reproduce Steps
1. Dial 99222(testing SIM voicemail number) from SIM 2. 
2. After call is connected, listen to voice instruction and press numeric key.

* Expected Result
IVR system will get DTMF command.

* Actual Result
Nothing happen

* Occurrence rate
100%

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

is this a fugu device?
Etienne, do you mind taking a look? Thanks 

traige: 1.4+, broken feature

Comment 2

[Enpei]

(In reply to Joe Cheng [:jcheng] from comment #1)
> is this a fugu device?
Yes, tested on Fugu.

Comment 3

[Enpei]

Test case is added. https://moztrap.mozilla.org/manage/case/11765/

Comment 4

[Etienne Segonzac (:etienne)]

There isn't any DSDS specific code in gaia for this use case [1], the DTMF API lives directly on the navigator.mozTelephony object.

I suspect this is a lower level issue, we should probably change the component.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/keypad.js#L305

Comment 5

[Gabriele Svelto [:gsvelto]]

Actually the startTone() and stopTone() method got an extra parameter for specifying which SIM call they should be called on in bug 814625, see here:

http://hg.mozilla.org/mozilla-central/diff/48ce546499b8/dom/webidl/Telephony.webidl#l1.30

I think this wasn't communicated to the gaia comms team and I had to actually dig into the code to figure out if startTone/stopTone were made multi-SIM aware or not. I'm taking this as the fix should be fairly straightforward.

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Yes, they are multi-sim aware. Sorry that we didn't communicate that clearly with comms team.

Comment 7

[Gabriele Svelto [:gsvelto]]

Quick update here, I'm modifying the code in the KeypadManager to play the DTMF tones on the SIM card of the current active call; it doesn't make sense to play it on any other SIM anyway.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> Quick update here, I'm modifying the code in the KeypadManager to play the
> DTMF tones on the SIM card of the current active call; it doesn't make sense
> to play it on any other SIM anyway.

Sounds good.
We also need to handle the conference group case, telephony.active will be a TelephonyCallGroup which apparently doesn't have a serviceId property.

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #8)
> We also need to handle the conference group case, telephony.active will be a
> TelephonyCallGroup which apparently doesn't have a serviceId property.

Good point, I hadn't thought of that one. One solution would be to pick the SIM of any one of the calls, like the first one. This should be good enough because AFAIK you cannot do a conference call across two SIMs, or can you? I'm also not sure what the low-level behavior of this actually is: do all the participants hear the DTMF tone?

Comment 10

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Play DTMF tones using active call's SIM card on multi-SIM devices

WIP patch, light testing on a single-SIM device and no unit tests yet but it seems to be working :) I've added an object to represent the DTMF tones as I need to encapsulate the |serviceId| a DTMF tone is being played on so that I can stop it properly. This seemed like the best approach here. For conference calls I'm picking the |serviceId| of the first call's SIM hoping it's representative of the call :)

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Play DTMF tones using active call's SIM card on multi-SIM devices

Fixed stuff that was still broken and completed with unit-tests. The way I handle call groups is a bit clunky but I haven't found a better way. It seems to me that when a conference call is underway CallsHandler.activeCall is null and there's not other field in CallsHandler indicating that we're in a conference so I'm pulling out the |calls| array directly from mozTelephony.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Play DTMF tones using active call's SIM card on multi-SIM devices

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8392351 [details] [diff] [review]
[PATCH WIP] Play DTMF tones using active call's SIM card on multi-SIM devices

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

Yep that's the idea :)

::: apps/communications/dialer/js/keypad.js
@@ +21,5 @@
> + * is mandatory.
> + *
> + * @param {String} tone The tone to be played.
> + * @param {Boolean} short True if this will be a short tone, false otherwise.
> + * @param {Integer} serviceId An object from which to copy the fields.

nit: this comment is a bit convoluted :)

@@ +34,5 @@
> +DtmfTone.prototype = {
> +  /**
> +   * Constructor
> +   */
> +  constructor: DtmfTone,

really needed?

@@ +49,5 @@
> +    navigator.mozTelephony.startTone(this.tone, this.serviceId);
> +
> +    if (this.short) {
> +      this.timer = window.setTimeout(function dt_stopTone() {
> +        navigator.mozTelephony.stopTone(this.serviceId);

```
this.timer = window.setTimeout(function dt_stopTone(serviceId) {
  navigator.mozTelephony.stopTone(serviceId);
}, DtmfTone.kShortToneLength, this.serviceId);
```

but the unit tests will tell you that anyway :)

@@ +353,4 @@
>  
>    _playDtmfTone: function kh_playDtmfTone(key) {
> +    var serviceId = 0;
> +    var activeCall = navigator.mozTelephony.active;

maybe |activeLine| since it's not always a call.
(|activeCall.calls| looks weird :))

@@ +354,5 @@
>    _playDtmfTone: function kh_playDtmfTone(key) {
> +    var serviceId = 0;
> +    var activeCall = navigator.mozTelephony.active;
> +
> +    if (activeCall) {

Lets
```
if (!activeLine) {
  return;
}
```

Comment 14

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Play DTMF tones using active call's SIM card on multi-SIM devices

Updated patch with comments from the feedback taken into account, the PR was also updated.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8392796 [details] [diff] [review]
[PATCH v2] Play DTMF tones using active call's SIM card on multi-SIM devices

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

r=me with nits (on github)

Comment 16

[Gabriele Svelto [:gsvelto]]

Updated the PR with fixes for all nits, now waiting for Travis to turn green:

https://travis-ci.org/mozilla-b2g/gaia/builds/21017918

Comment 17

[Gabriele Svelto [:gsvelto]]

Travis was green except for an unrelated failure, pushed to gaia/master 6e9607f80137781c5fc8e4d7965a6f922b4695bf

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH gaia/v1.3] Play DTMF tones using active call's SIM card on multi-SIM devices r=etienne

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 814625
[User impact] if declined: DTMF tones work only when using the first SIM, they do not work in calls done using the second SIM
[Testing completed]: Tested on a Tarako using both SIM slots and swapping SIMs too for completeness, also covered with unit-tests, the Travis run for the v1.3 patch is here https://travis-ci.org/mozilla-b2g/gaia but contains too many errors that are caused by the age of the branch
[Risk to taking this patch] (and alternatives if risky): DTMF functionality might regress though I was unable to identify any issues with the current patch
[String changes made]: none

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/89b150d366cf

Comment 20

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

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> Actually the startTone() and stopTone() method got an extra parameter for
> specifying which SIM call they should be called on in bug 814625, see here:
> 
> http://hg.mozilla.org/mozilla-central/diff/48ce546499b8/dom/webidl/Telephony.
> webidl#l1.30
> 
> I think this wasn't communicated to the gaia comms team and I had to
> actually dig into the code to figure out if startTone/stopTone were made
> multi-SIM aware or not. I'm taking this as the fix should be fairly
> straightforward.

Sorry about the bad communication for this case. Generally I add some from Comms team into CC list if I know WebAPI is gonna be changed, but I did miss that in bug 814625. I'll be more careful to make sure Comms team is in CC and also take good advantage of the standup meeting to share API changes. Hope the proposal sounds good. :)

Comment 21

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #20)
> Sorry about the bad communication for this case. Generally I add some from
> Comms team into CC list if I know WebAPI is gonna be changed, but I did miss
> that in bug 814625.

No problem, I may have missed those e-mails. We caught this one in time anyway :)

> I'll be more careful to make sure Comms team is in CC
> and also take good advantage of the standup meeting to share API changes.
> Hope the proposal sounds good. :)

Yes, it would also be best to always open Gaia follow-ups when doing WebAPI changes so that these things can't fall through the cracks.

Comment 22

[Gabriele Svelto [:gsvelto]]

Pushed attachment 8392981 [details] [diff] [review] to gaia/v1.3 d5dae5263464dbbcf11c3d7f062ade1148792399

Comment 23

[Gabriele Svelto [:gsvelto]]

Cherry picked to gaia/v1.4 a6f0e98007d220d39e045f5bbf901b7d86f59698

Comment 24

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

(In reply to Gabriele Svelto [:gsvelto] from comment #21)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #20)
> > Sorry about the bad communication for this case. Generally I add some from
> > Comms team into CC list if I know WebAPI is gonna be changed, but I did miss
> > that in bug 814625.
> 
> No problem, I may have missed those e-mails. We caught this one in time
> anyway :)
> 
> > I'll be more careful to make sure Comms team is in CC
> > and also take good advantage of the standup meeting to share API changes.
> > Hope the proposal sounds good. :)
> 
> Yes, it would also be best to always open Gaia follow-ups when doing WebAPI
> changes so that these things can't fall through the cracks.

Actually, we did file a gaia bug for this api change for compatibility. However, since at that time gaia isn't supporting on-the-fly DSDS UI, and since without 'serviceId' is assigned, gecko will just use 'defaultServiceId.' So this issue hasn't been detected (or valid, anyway ;) )

However, like you said, at least we caught it anyway! Thanks for taking care of this bug and raising the communication issue. :)

Comment 25

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #24)
> Actually, we did file a gaia bug for this api change for compatibility.
> However, since at that time gaia isn't supporting on-the-fly DSDS UI, and
> since without 'serviceId' is assigned, gecko will just use
> 'defaultServiceId.' So this issue hasn't been detected (or valid, anyway ;) )

I see, I did miss that one indeed :)

Just another quick question on this. Currently I'm implementing this feature by directing the DTMF tones to whatever SIM card is currently in use. Since we can have only one SIM card in use at the same time AFAIK I don't see in which scenario one would use a different |serviceId| parameter.

Do we have future scenarios in which one can have two calls active at the same time on different SIMs and then can send DTMF tones to the distinct calls?

Comment 26

[Enpei]

Verified on v1.4 Fugu.
Gaia      53edbf08b0a750c31e8c6b2c20f2b1315b1412d1
Gecko     979daba51a33e90b5ba88be7ef3940fa55d3297b
BuildID   20140320100025
Version   30.0a2

Comment 27

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

(In reply to Gabriele Svelto [:gsvelto] from comment #25)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #24)
> > Actually, we did file a gaia bug for this api change for compatibility.
> > However, since at that time gaia isn't supporting on-the-fly DSDS UI, and
> > since without 'serviceId' is assigned, gecko will just use
> > 'defaultServiceId.' So this issue hasn't been detected (or valid, anyway ;) )
> 
> I see, I did miss that one indeed :)
> 
> Just another quick question on this. Currently I'm implementing this feature
> by directing the DTMF tones to whatever SIM card is currently in use. Since
> we can have only one SIM card in use at the same time AFAIK I don't see in
> which scenario one would use a different |serviceId| parameter.
> 

As we support only DSDS scenario now, you are totally right!

> Do we have future scenarios in which one can have two calls active at the
> same time on different SIMs and then can send DTMF tones to the distinct
> calls?

I heard a talk bout this DSDA requirement last week but it seems not a very promising requirement (at least at the time I heard). So I am not sure about if there's a real product plan and schedule. Even I don't know when this is gonna happen, I think distinguishing DSDS and DSDA is a potential issue. I just filed bug 985945 for tracking.