WebTelephony: implement navigator.mozTelephony.sendTones()

RESOLVED FIXED in 2.2 S3 (9jan)

Status

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: jhammink, Assigned: albert)

Tracking

({dev-doc-complete})

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

7 years ago
from https://github.com/jonallengriffin/B2G
pulled and merged approx 11:59 a.m. p.s.t 1/31/2012
Samsung Galaxy S2 with working SIM card installed.

DTMF tones don't work correctly on IVR menu

Before test:  Made control call to testcall.com on my working android mobile to verify that it actually DOES recognize DTMF.  It does.

To repro:
1. Dial a number such as that of testcall.com (804) 222-1111
2. Verify also that IVR can verify the DTMF Tones from the B2G phone dialer.

NOTE: DTMF tones are not recognized by testcall IVR.
Bug 674726 never implemented navigator.mozTelephony.sendTones():

NS_IMETHODIMP
Telephony::SendTones(const nsAString& aTones, PRUint32 aToneDuration,
                     PRUint32 aIntervalDuration)
{
  NS_NOTYETIMPLEMENTED("Implement me!");
  return NS_ERROR_NOT_IMPLEMENTED;
}

It should be plumbed through from nsIRadioInterfaceLayer onwards.
Blocks: webtelephony
No longer blocks: b2g-telephony
Whiteboard: [good first bug][lang=c++][mentor=philikon]
Summary: B2G Telephony - DTMF tones not working correctly e.g. on IVR menu → WebTelephony: DTMF tones not working correctly e.g. on IVR menu
Summary: WebTelephony: DTMF tones not working correctly e.g. on IVR menu → WebTelephony: support DTMF tones
I'd like to take this bug.
Assignee: nobody → swu
Posted patch Support sendTones() V1 (obsolete) — Splinter Review
To make DTMF tone during a call working, it also requires Gaia implementation at dialer.js.
It can be implemented by either:
1. Calling startTone() at keyDown and stopTone() at keyUp.
2. Calling sendTones() at keyDown, passing only one DTMF tone.

This first patch supports sendTones() by sending one tone with fixed duration.

The current ril_worker.js already implemented sending one tone by sendTone(), as it seems RIL supports only sending one tone with fixed duration(by REQUEST_DTMF).
I'm thinking how to send multiple tones with a requested duration, maybe to handle it in RIL layer by REQUEST_DTMF_START and REQUEST_DTMF_STOP.

Any comments are welcome.  Thank you very much!  :)
Comment on attachment 596273 [details] [diff] [review]
Support sendTones() V1

Nice first patch!

># HG changeset patch
># Parent d71dab82fff4325584406ae0ffac3ba89a73f13e
># User Shian-Yow Wu <swu@mozilla.com>
>First patch to support navigator.mozTelephony.sendTones()

Please follow the mozilla-central commit msg convention:

  Bug NNN - bug summary. r=reviewer

(See https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities)

>diff --git a/dom/system/b2g/nsIRadioInterfaceLayer.idl b/dom/system/b2g/nsIRadioInterfaceLayer.idl
>--- a/dom/system/b2g/nsIRadioInterfaceLayer.idl
>+++ b/dom/system/b2g/nsIRadioInterfaceLayer.idl
>@@ -153,16 +153,17 @@ interface nsIRadioInterfaceLayer : nsISu
>   /**
>    * Functionality for making and managing phone calls.
>    */
>   void dial(in DOMString number);
>   void hangUp(in unsigned long callIndex);
> 
>   void startTone(in DOMString dtmfChar);
>   void stopTone();
>+  void sendTone(in DOMString dtmfChar);

When changing an interface, you also need to give it a new UUID. (firebot can generate one for you.) r- from me for this.

>diff --git a/dom/telephony/Telephony.cpp b/dom/telephony/Telephony.cpp
>--- a/dom/telephony/Telephony.cpp
>+++ b/dom/telephony/Telephony.cpp
>@@ -353,18 +353,29 @@ Telephony::StopTone()
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> Telephony::SendTones(const nsAString& aTones, PRUint32 aToneDuration,
>                      PRUint32 aIntervalDuration)
> {
>-  NS_NOTYETIMPLEMENTED("Implement me!");
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  if (aTones.IsEmpty()) {
>+    NS_WARNING("Empty tone string will be ignored");
>+    return NS_OK;
>+  }
>+
>+  if (aTones.Length() > 1) {
>+    return NS_ERROR_INVALID_ARG;
>+  }

The method and parameter name is plural ("tones" rather than "tone"), which leads me to think that multiple tones should be supported. But I wonder for what use case...

It seems "sendTones()" method as proposed in the WebTelephony API isn't actually very useful in reality. A "real" phone starts sending the phone when you touch the button and stops the tone when you let go of the button. So the length of the tone actually depends on how long you were holding the button. This doesn't seem currently implementable with the "sendTones()" API. We might want to consider exposing the start/stop tone functionality instead. Asking bent for some feedback on this.
Attachment #596273 - Flags: review-
Attachment #596273 - Flags: feedback?(bent.mozilla)
(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> We might want to consider exposing the start/stop tone functionality
> instead. Asking bent for some feedback on this.

So, uh, I forgot that there was actually navigator.mozTelephony.startTone()/stopTone(), which renders this point moot. Still I wonder about the point of sendTones(), hoping bent can clarify.
Summary: WebTelephony: support DTMF tones → WebTelephony: implement navigator.mozTelephony.sendTones()
The idea was to be able to do something like this:

  sendTones("*2,12345,2");

This lets people program a series of dial tones that need to be sent when calling automated numbers or something (not sure if you can still do this on smart phones, but you used to be able to with most programmable land-line handsets).

This would send the tones for "*" with some standard duration, pause very briefly, then send "2", then pause for some standard number of seconds (the ","). Then it would send "1", "2", "3", "4", "5", pause again, and then send "2".

It's entirely possible for the page to do this on its own, of course, with timeouts and startTone/stopTone. Seems to me that there is value in having a built-in way to do this, but I'm not sure how much value really. We could punt and remove this for the time being if others disagree.
Thanks for review comments from philikon and clarification from bent.  :)

There is one RIL command "REQUEST_CDMA_BURST_DTMF" should be what we need.  

/**
 * RIL_REQUEST_CDMA_BURST_DTMF
 *
 * Send DTMF string
 *
 * "data" is const char **
 * ((const char **)data)[0] is a DTMF string
 * ((const char **)data)[1] is the DTMF ON length in milliseconds, or 0 to use
 *                          default
 * ((const char **)data)[2] is the DTMF OFF length in milliseconds, or 0 to use
 *                          default
 *
 * "response" is NULL
 *
 * Valid errors:
 *  SUCCESS
 *  RADIO_NOT_AVAILABLE
 *  GENERIC_FAILURE
 *
 */

However, RIL is proprietary and provided as binary blob by device vendors, it still depends on whether the device implemented this command.  On SGS2, this command is supported, but seems it always ignores the tone & interval duration parameters and uses 500ms for each by default, it means sending a DTMF string "1234567890" would take about 10 seconds.  Not sure about the result on other devices.

Implementing by this command provides a working but not perfect result at least on SGS2 phone.  I think we can implement sendTones() this way for at this moment, and leave the built-in support status to be platform dependent issue.  Will do it this way if no other comments.
BTW, this did get implemented, in bug 709565. I think it got stomped when we moved telephony back to C++.
Currently startTone()/stopTone()/sendTone() were implemented in ril_worker.js, and telephony.cpp implemented startTone()/stopTone().

This bug requires implementation of sendTones().  So we need another implementation of sendTones() in ril_worker.js and telephony.cpp.
Try run for cbaf30163c8d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cbaf30163c8d
Results (out of 209 total builds):
    success: 170
    warnings: 23
    failure: 14
    other: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/swu@mozilla.com-cbaf30163c8d
(In reply to ben turner [:bent] from comment #6)
> The idea was to be able to do something like this:
> 
>   sendTones("*2,12345,2");
> 
> This lets people program a series of dial tones that need to be sent when
> calling automated numbers or something (not sure if you can still do this on
> smart phones, but you used to be able to with most programmable land-line
> handsets).
> 
> This would send the tones for "*" with some standard duration, pause very
> briefly, then send "2", then pause for some standard number of seconds (the
> ","). Then it would send "1", "2", "3", "4", "5", pause again, and then send
> "2".

Where are the lengths of tones and pauses defined, and the valid characters for the `sendTones()` string argument? It seems to me the draft proposal is lacking some details here (and sadly not just here.)

> It's entirely possible for the page to do this on its own, of course, with
> timeouts and startTone/stopTone. Seems to me that there is value in having a
> built-in way to do this, but I'm not sure how much value really. We could
> punt and remove this for the time being if others disagree.

I'm not sure I even understand the use case for this. Where in mobile phone functionality does this come up?

(In reply to Shian-Yow Wu from comment #9)
> This bug requires implementation of sendTones().  So we need another
> implementation of sendTones() in ril_worker.js and telephony.cpp.

I'm not sure about the "need" here. I think it makes more sense to punt on `sendTones()` for the time being. Dialer apps can use `startTone()` and `stopTone()` to implement DTMF tones quite easily.

Unless I hear objections, I'm going to close this bug as WONTFIX.
The use case is wanting to have a series of tones that can be programed ahead of time and then replayed during a phone call. For example, say I was to call my bank's automated telephone system. Every time the prompt is the same ("press 1 for english", then a pause, then "press 4 for account balance", pause, "enter the last 4 digits of your social security number", pause, etc.). Rather than doing this manually all the time some phones have the ability to program this in and execute it automatically. Then I would just call the bank and press some key to have the prompts automatically answered. Make sense?

We can do this later or not at all, sure. As I said, it's possible for the dialer to do this already.
(In reply to ben turner [:bent] from comment #12)
> Rather than doing this manually all the time some phones have
> the ability to program this in and execute it automatically.

I did not know this.

> Then I would
> just call the bank and press some key to have the prompts automatically
> answered. Make sense?

Yes.

> We can do this later or not at all, sure. As I said, it's possible for the
> dialer to do this already.

Right. If I were to prioritize this, it would be pretty low on my list, IMHO. I'd rather take error events (bug 717462). I'm resolving this as WONTFIX for now. We can always reopen if we decide to come back to it.

Shian-Yow, thanks for the patch, it's very nice, but given the above discussion, I don't think we want to go forward with it. As you noted, the biggest missing piece for DTMF tones at this point is in Gaia and I see you've already filed https://github.com/andreasgal/gaia/issues/406 for that. If you would like to work on something else RIL/Telephony-related (e.g. bug 717462), you're more than welcome to. At least now you know your way around the code :)
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
It would be nice if implementing this could be reconsidered, as I think there are some cases in which the use of startTone/stopTone is not the best option or even not an option at all.
For example in bug 911055, we need to send a series of DTMF tones of certain duration (300 ms). The problem is that you have to use setTimeout to accurately call stopTone at an specific time so the tone has the right duration, and also to wait certain time before calling the next startTone. This can lead to inaccurate tone timing, for example when the application process goes to background, setTimeout callback will not be called in less than 1 second, so you will not be able to send tones shorter than a second using startTone/setTimeout/stopTone.

HsinYi, do you think it makes sense to reopen this bug?
Flags: needinfo?(htsai)
Yes, as I mentioned in bug 911055, reopening this bug makes sense to me. Thanks.
Flags: needinfo?(htsai)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: swu → nobody
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Whiteboard: [good first bug][lang=c++][mentor=philikon] → [good first bug][lang=c++]
Version: Trunk → unspecified
Removing Good First Bug status; happy to re-add it if the expected outcome is clarified.
Whiteboard: [good first bug][lang=c++] → [lang=c++]
Assignee: nobody → david.garciaparedes
One question about the signature of the method.
The draft [1] says startTone and sendTones should take a ToneOptions parameter

dictionary ToneOptions {
    unsigned long duration;
    unsigned long gap;
    DOMString     serviceId;
};

But startTone is not implemented that way. It takes a tone and a serviceId.
Should I do something similar with sendTones? Something like:

sendTones(tones, duration, gap, serviceId);

What do you think Hsin-Yi?

[1] http://www.w3.org/2012/sysapps/telephony/#idl-def-ToneOptions
Flags: needinfo?(htsai)
(In reply to David Garcia [:davidg] from comment #17)
> One question about the signature of the method.
> The draft [1] says startTone and sendTones should take a ToneOptions
> parameter
> 
> dictionary ToneOptions {
>     unsigned long duration;
>     unsigned long gap;
>     DOMString     serviceId;
> };
> 
> But startTone is not implemented that way. It takes a tone and a serviceId.
> Should I do something similar with sendTones? Something like:
> 
> sendTones(tones, duration, gap, serviceId);
> 
> What do you think Hsin-Yi?
> 
> [1] http://www.w3.org/2012/sysapps/telephony/#idl-def-ToneOptions

Hi David,

Thanks for taking this bug :)

Per previous discussion, I thought Dialer would send one tone at a time to gecko but not a series. According to the latest comment of bug 911055, I am wondering if we need to have an API for sending a series of tones.
Have you seen any problem if sending tones one by one? Could you please elaborate more if I miss something? Thank you!
Flags: needinfo?(htsai)
Anshul,

Our partner is trying to make this happen in v2.2. I am looping you in cc so that you won't miss ril interface change. :)
Anshul,

Our partner is trying to make this happen in v2.2. I am looping you in cc so that you won't miss ril interface change. :)
Hi Hsinyi,

NP. Thanks for having a look :)

The problem is that if you want to be precise on the duration of the tones, and the time gap between the tones, it is pretty hard to do it in GAIA.
So lets say you want to play a series of tones in gaia, each tone played for 300ms and 500ms between each tone, you could do something like:

telephony.startTone('1');
setTimeout(function() {
    telephony.stopTone();
    setTimeout(function() {
        // Keep playing the rest of the tones
    }, 500);
}, 300);

That is what I did for bug 911055. The problem is that setTimeout is not precise, and if the app goes to background (that was our problem), then the minimum setTimeout is 1000ms, so the code above doesn't work properly. We couldn't figure out a way to solve this problem in a clean way (maybe there is a better way, I don't know).

Then I checked the draft and saw the sendTones method was defined, and it did exactly what we needed.

By sending a single tone, do you mean something like sendTone instead of sendTones? The problem is that you still need to deal with the time gap between tones in GAIA, having the same problem I said above.
I personally prefer the way it is defined in the draft, because if you want to send a single tone, you still can call sendTones with just 1 character. 

Thanks!
(In reply to David Garcia [:davidg] from comment #21)
> Hi Hsinyi,
> 
> NP. Thanks for having a look :)
> 
> The problem is that if you want to be precise on the duration of the tones,
> and the time gap between the tones, it is pretty hard to do it in GAIA.
> So lets say you want to play a series of tones in gaia, each tone played for
> 300ms and 500ms between each tone, you could do something like:
> 
> telephony.startTone('1');
> setTimeout(function() {
>     telephony.stopTone();
>     setTimeout(function() {
>         // Keep playing the rest of the tones
>     }, 500);
> }, 300);
> 
> That is what I did for bug 911055. The problem is that setTimeout is not
> precise, and if the app goes to background (that was our problem), then the
> minimum setTimeout is 1000ms, so the code above doesn't work properly. We
> couldn't figure out a way to solve this problem in a clean way (maybe there
> is a better way, I don't know).
> 
> Then I checked the draft and saw the sendTones method was defined, and it
> did exactly what we needed.
> 
> By sending a single tone, do you mean something like sendTone instead of
> sendTones? The problem is that you still need to deal with the time gap
> between tones in GAIA, having the same problem I said above.
> I personally prefer the way it is defined in the draft, because if you want
> to send a single tone, you still can call sendTones with just 1 character. 
> 
> Thanks!

Your explanation makes sense to me. Let's do this way as you proposed. :)

Comment 23

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #20)
> Anshul,
> 
> Our partner is trying to make this happen in v2.2. I am looping you in cc so
> that you won't miss ril interface change. :)

Thank you Hsin-Yi. As always very much appreciate it.
Posted file patch to add sendTones (obsolete) —
This patch adds sendTones method to mozTelephony.

I'm aware that the way I hardcoded default values for duration and gap in dom/telephony/Telephony.cpp, is probably not the best way to do it.
Where do you think is better to put this default values?
Attachment #8501111 - Flags: review?(htsai)
Comment on attachment 8501111 [details]
patch to add sendTones

Thanks for the patch, David, though the format is wrong. Please do follow [1] and [2] to submit a nice-orgnized patch, and ask for review again.

Quick comments for the patch, and please have them addressed in the next revision:
1) We should return Promise for sendTones
2) Define the default values for "gap" and "duration" in Telephony.webidl. [3] is an example. How are the default values determined, any reference or standard?
3) Use "let" instead of "var" in Telephony js code. 

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
[3] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/AudioBufferSourceNode.webidl?from=AudioBufferSourceNode.webidl&case=true#24
Attachment #8501111 - Flags: review?(htsai)
Whiteboard: [lang=c++]
Posted patch WIP (obsolete) — Splinter Review
Attachment #8501111 - Attachment is obsolete: true
One question Hsin-Yi.
I have seen that when a method returns a promise it is defined the promise type. For example:

Promise<(TelephonyCall or DOMRequest)> dial(DOMString number, optional unsigned long serviceId);

In the case of the sendTones method, what do you think it make more sense? It should be a DOMRequest?

Thanks a lot!
Flags: needinfo?(htsai)
Another question :)

If a define the defaults in the WebIdl I still need to do this?

  if (aDuration.WasPassed()) {
    duration = aDuration.Value();
  } else {
    duration = 300;
  }

or maybe I can just do:

duration = aDuration.Value()

?

Thanks again
(In reply to David Garcia [:davidg] from comment #27)
> One question Hsin-Yi.
> I have seen that when a method returns a promise it is defined the promise
> type. For example:
> 
> Promise<(TelephonyCall or DOMRequest)> dial(DOMString number, optional
> unsigned long serviceId);
> 
> In the case of the sendTones method, what do you think it make more sense?
> It should be a DOMRequest?
> 
> Thanks a lot!

Promise is used for asynchronous operation. In this case, when sendTones succeeds, we don't need to return a specific data, so |Promise<void>| makes sense.

(In reply to David Garcia [:davidg] from comment #28)
> Another question :)
> 
> If a define the defaults in the WebIdl I still need to do this?
> 
>   if (aDuration.WasPassed()) {
>     duration = aDuration.Value();
>   } else {
>     duration = 300;
>   }
> 
> or maybe I can just do:
> 
> duration = aDuration.Value()
> 
> ?
> 
> Thanks again

I believe you don't need either .WasPassed() or .Value(). Use "aDuration" directly. 
(I haven't tried it yet ;) )
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #29)
> (In reply to David Garcia [:davidg] from comment #27)
> > One question Hsin-Yi.
> > I have seen that when a method returns a promise it is defined the promise
> > type. For example:
> > 
> > Promise<(TelephonyCall or DOMRequest)> dial(DOMString number, optional
> > unsigned long serviceId);
> > 
> > In the case of the sendTones method, what do you think it make more sense?
> > It should be a DOMRequest?
> > 
> > Thanks a lot!
> 
> Promise is used for asynchronous operation. In this case, when sendTones
> succeeds, we don't need to return a specific data, so |Promise<void>| makes
> sense.
> 

One thing deserves more discussion: when is Promise supposed to reject? My opinion is if any of a tone sending fails, the promise should reject. Makes sense?



> (In reply to David Garcia [:davidg] from comment #28)
> > Another question :)
> > 
> > If a define the defaults in the WebIdl I still need to do this?
> > 
> >   if (aDuration.WasPassed()) {
> >     duration = aDuration.Value();
> >   } else {
> >     duration = 300;
> >   }
> > 
> > or maybe I can just do:
> > 
> > duration = aDuration.Value()
> > 
> > ?
> > 
> > Thanks again
> 
> I believe you don't need either .WasPassed() or .Value(). Use "aDuration"
> directly. 
> (I haven't tried it yet ;) )
Posted patch WIP (obsolete) — Splinter Review
Attachment #8501623 - Attachment is obsolete: true

Comment 32

4 years ago
Hsin-Yi, this bug is precisely the reason why I was thinking that the pause character as being discussed in bug 911055 should be handled by gecko so Gaia doesn't need to worry about it. What if there is more than one pause characters in the dial string (definitely possible); how is gaia going to know that the previous sendTones is finished before processing the second pause character?
Flags: needinfo?(htsai)
Hi Anshul, since sendTones returns a promise, api user can simply deal with multiple pause characters by a chain of "then" handlers. The cases you mentioned are covered too. 

If we want to support "dial waiting" in the future, the api could support that, too. But if we parse the string in dial(), then the api isn't flexible enough to "dial waiting." In addition, that makes dial() much vague. For example, how do we express "a call was successfully made but the following dtmf tones weren't sent successfully" via the api itself? Do my concerns make sense to you?
Flags: needinfo?(htsai)
Assignee: david.garciaparedes → alberto.crespellperez
(Assignee)

Comment 34

4 years ago
Posted patch Patch (obsolete) — Splinter Review
Changes from comment 25
Attachment #596273 - Attachment is obsolete: true
Attachment #8525359 - Attachment is obsolete: true
Attachment #8537150 - Flags: review?(htsai)
Wesley, Wilfred, we need someone to review the patch if Hsin-Yi is busy, we want to land this in master before Jan 12th (so it can be part of 2.2 branch) and part of my team will be in Holidays the next two weeks.
Could please help me to speed up the review? Thanks a lot
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(whuang)
(In reply to Maria Angeles Oteo (:oteo) from comment #35)
> Wesley, Wilfred, we need someone to review the patch if Hsin-Yi is busy, we
> want to land this in master before Jan 12th (so it can be part of 2.2
> branch) and part of my team will be in Holidays the next two weeks.
> Could please help me to speed up the review? Thanks a lot

I'll do the review today.
Flags: needinfo?(wmathanaraj)
Flags: needinfo?(whuang)
Comment on attachment 8537150 [details] [diff] [review]
Patch

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

Thanks for the patch, Albert.
I still have some questions about the API design. Please see my comments below, thank you.

::: dom/telephony/Telephony.cpp
@@ +388,5 @@
> +  }
> +
> +  if (aDTMFChars.IsEmpty()) {
> +    NS_WARNING("Empty tone string will be ignored");
> +    return nullptr;

We should |return promise.reject();|.

Please do:
promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
return promise.forget();

@@ +393,5 @@
> +  }
> +
> +  if (!IsValidServiceId(serviceId)) {
> +    aRv.Throw(NS_ERROR_INVALID_ARG);
> +    return nullptr;

ditto.

::: dom/telephony/gonk/TelephonyService.js
@@ +893,5 @@
>        this._sendToRilWorker(aClientId, "hangUp", { callIndex: aCallIndex });
>      }
>    },
>  
> +  sendTones: function(aClientId, aDtmfChars, duration, gap, aCallback) {

s/duration/aDuration/  and s/gap/aGap/

@@ +894,5 @@
>      }
>    },
>  
> +  sendTones: function(aClientId, aDtmfChars, duration, gap, aCallback) {
> +    var self = this;

Please use "let" instead of "var" in Telephony gecko code. And in this case, we usually use |.bind()| in gecko.

@@ +899,5 @@
> +
> +    function playSingleTone(tone, cb) {
> +      self._sendToRilWorker(aClientId, "startTone",
> +                            { dtmfChar: tone });
> +      setTimeout(function() {

Please just use Ci.nsITimer (contractid: mozilla.org/timer;1), instead of Timer.jsm.

You may refer to [1].
[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#171

@@ +902,5 @@
> +                            { dtmfChar: tone });
> +      setTimeout(function() {
> +        self._sendToRilWorker(aClientId, "stopTone");
> +        setTimeout(function() {
> +          cb(); 

nit: no trailing white space

@@ +903,5 @@
> +      setTimeout(function() {
> +        self._sendToRilWorker(aClientId, "stopTone");
> +        setTimeout(function() {
> +          cb(); 
> +        }, gap);

The implementation here is different from my understanding of the API design.
My understanding is, "gap" is used to indicate the "pause" duration between each series of tones, but not between each tone. For example, for the dialing string "00000,123,45", gap is the indication of the pause duration b/w the sub-strings "123" and "45." However, this is not what your code tries to achieve. Could you explain? Thank you.

@@ +913,5 @@
> +        playSingleTone(tones[0], function() {
> +          playTones(tones.substr(1));
> +        });
> +      } else {
> +        aCallback.notifySuccess();

In addition to "notifySuccess", we should also take care of "error" cases which will call "notifyError()"

::: dom/telephony/ipc/TelephonyIPCService.cpp
@@ +290,5 @@
> +                               uint32_t aDuration, uint32_t aGap,
> +                               nsITelephonyCallback *aCallback)
> +{
> +  return SendRequest(nullptr, aCallback,
> +            SendTonesRequest(aClientId, nsString(aDtmfChars), aDuration, aGap));

nit: align "SendTonesRequest" with "nullptr", and wrap a line at 80th character

::: dom/webidl/Telephony.webidl
@@ +28,5 @@
>    [Throws]
>    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
> +  Promise<void> sendTones(DOMString tone, optional unsigned long duration = 300, optional unsigned long gap = 500, optional unsigned long serviceId);

s/tone/tones/

Could you point me out how the default values are determined here? Thank you.
Attachment #8537150 - Flags: review?(htsai)
Flags: needinfo?(alberto.crespellperez)
(Assignee)

Comment 38

4 years ago
Posted patch Patch V2 (obsolete) — Splinter Review
Changes from comment 37
Attachment #8537150 - Attachment is obsolete: true
Flags: needinfo?(alberto.crespellperez)
Attachment #8540670 - Flags: review?(htsai)
(Assignee)

Comment 39

4 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #37)
> Comment on attachment 8537150 [details] [diff] [review]
> Patch
> 
> Review of attachment 8537150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +893,5 @@
> >        this._sendToRilWorker(aClientId, "hangUp", { callIndex: aCallIndex });
> >      }
> >    },
> >  
> > +  sendTones: function(aClientId, aDtmfChars, duration, gap, aCallback) {
> 
> s/duration/aDuration/  and s/gap/aGap/
> 

As talked offline aGap is not exposed to gaia, now is a constant. And I've added pauseDuration.

> 
> @@ +903,5 @@
> > +      setTimeout(function() {
> > +        self._sendToRilWorker(aClientId, "stopTone");
> > +        setTimeout(function() {
> > +          cb(); 
> > +        }, gap);
> 
> The implementation here is different from my understanding of the API design.
> My understanding is, "gap" is used to indicate the "pause" duration between
> each series of tones, but not between each tone. For example, for the
> dialing string "00000,123,45", gap is the indication of the pause duration
> b/w the sub-strings "123" and "45." However, this is not what your code
> tries to achieve. Could you explain? Thank you.
> 

There are three parameters involved.
- toneDuration: amount of time that a digit sound is being generated
- gap: interval of silence inserted between each successive DTMF character
- pauseDuration: silent interval when ',' char is present

I noticed that I was sending the ',' pause char and it was wrong. Now the ',' char makes a pause and then the following char is processed.

>
> ::: dom/webidl/Telephony.webidl
> @@ +28,5 @@
> >    [Throws]
> >    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
> >  
> >    [Throws]
> > +  Promise<void> sendTones(DOMString tone, optional unsigned long duration = 300, optional unsigned long gap = 500, optional unsigned long serviceId);
> 
> s/tone/tones/
> 
> Could you point me out how the default values are determined here? Thank you.

I used those values because David used them. I tried to find the standard values but no luck, only found that 3GPP specifies that pauses should be of 3000ms. However, after having a look to AOSP I see that android uses 2000ms. Now I use 2000ms because 3000ms seems very long.
Comment on attachment 8540670 [details] [diff] [review]
Patch V2

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

Hi Albert, 
thanks for the revision!

Two main concerns:
1) this patch still misses the error handling, please see [comment 1] inline for details
2) regarding ',' parsing, seems there's mis-communication b/w us over irc. please see [comment 2]

Thank you very much.

::: dom/telephony/gonk/TelephonyService.js
@@ +10,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/systemlibs.js");
> +Cu.import("resource://gre/modules/Timer.jsm");

We don't need this.

@@ +47,5 @@
>  const DIAL_ERROR_BAD_NUMBER = RIL.GECKO_CALL_ERROR_BAD_NUMBER;
>  
>  const DEFAULT_EMERGENCY_NUMBERS = ["112", "911"];
>  
> +const TONES_GAP_DURATION = 300;

Per http://www.etsi.org/deliver/etsi_ts/101200_101299/10123502/01.01.01_60/ts_10123502v010101p.pdf, the minimum tones gap duration is 65ms. Let's use 70 for safety.

@@ +900,5 @@
> +                      aCallback) {
> +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +
> +    let playSingleTone = (function (tone, cb) {
> +      this.startTone(aClientId, tone);

[comment 1]
We need to consider error cases, notifyError, as I mentioned in my last review.

startTone could fail. Once it fails, we need to reject the promise. Please take care of the error handling. That means, you need to wait until the startTone callback comes from [1], then to either proceed to the next dtmf char or notifyError(), based on the callback result.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#6127

@@ +909,5 @@
> +        }, TONES_GAP_DURATION, Ci.nsITimer.TYPE_ONE_SHOT);
> +      }.bind(this), aToneDuration, Ci.nsITimer.TYPE_ONE_SHOT);
> +    }).bind(this);
> +
> +    let playTones = function (tones) {

nit: no space between "function" and "("

@@ +911,5 @@
> +    }).bind(this);
> +
> +    let playTones = function (tones) {
> +      if (tones.length) {
> +        if (tones[0] == ',') {

[comment 2]
Hmm, I don't think we need to check ',' in gecko. I am sorry if I didn't express myself clearly enough when we chatted on IRC yesterday. We don't check ',' in gecko, but we should instead do that in gaia. That means, gaia application should NOT treat ',' being part of aDtmfChars.

The use case in mind looks like: 
For the dialing string "00000,123,,45", gaia will do:
telephony.dial("00000")
.then (() => waitForCallConnected();)
.then (() => telephony.sendTones("123",70,3000))
.then (() => telephony.sendTones("45",70, 6000))  // 6000 due to "2" pause characters

So, considering [comment 1] and [comment 2], the whole concept might look like below (the code might not be 100% correct but I'd like to show what I meant by code):

 sendTones: function(aClientId, aDtmfChars, aToneDuration, aPauseDuration, aCallback) {
     let tones = aDtmfChars;
     let playTone = (tone) => {
        this.startTone(aClientId, tone, () => {
           if (!response.success) {
             aCallback.notifyError(response.errorMsg);
             return;
           }
 
         timer.initWithCallback(() => {
            this.stopTone();
            timer.initWithCallback(() => {
              if (tones.length === 1) {
                aCallback.notifySuccess();
              } else {
                tones = tones.substr(1);
                playTone(tones[0]);
              }
            }, TONES_GAP_DURATION, Ci.nsITimer.TYPE_ONE_SHOT);
          }, aToneDuration, Ci.nsITimer.TYPE_ONE_SHOT);
        });
      };
  
      timer.initWithCallback(() => {
        playTone(tones[0]);
      }, aPauseDuration, Ci.nsITimer.TYPE_ONE_SHOT);
    },

::: dom/webidl/Telephony.webidl
@@ +28,5 @@
>    [Throws]
>    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
> +  Promise<void> sendTones(DOMString tones, optional unsigned long toneDuration = 500, optional unsigned long pauseDuration = 2000, optional unsigned long serviceId);

Per http://www.etsi.org/deliver/etsi_ts/101200_101299/10123502/01.01.01_60/ts_10123502v010101p.pdf, minimum tone duration is 65ms. Let's use 70 ms as the default value of toneDuration.

Agree that 3000ms seems long ;) but let's keep what spec suggests for API. If UI requires a shorter value, Gaia could always easily adjust that. Thank you.

In addition, please also comment on the "unit" of duration.
Attachment #8540670 - Flags: review?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #40)
> Comment on attachment 8540670 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 8540670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Albert, 
> thanks for the revision!
> 
> Two main concerns:
> 1) this patch still misses the error handling, please see [comment 1] inline
> for details
> 2) regarding ',' parsing, seems there's mis-communication b/w us over irc.
> please see [comment 2]
> 
> Thank you very much.
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +10,5 @@
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource://gre/modules/Promise.jsm");
> >  Cu.import("resource://gre/modules/systemlibs.js");
> > +Cu.import("resource://gre/modules/Timer.jsm");
> 
> We don't need this.
> 
> @@ +47,5 @@
> >  const DIAL_ERROR_BAD_NUMBER = RIL.GECKO_CALL_ERROR_BAD_NUMBER;
> >  
> >  const DEFAULT_EMERGENCY_NUMBERS = ["112", "911"];
> >  
> > +const TONES_GAP_DURATION = 300;
> 
> Per
> http://www.etsi.org/deliver/etsi_ts/101200_101299/10123502/01.01.01_60/
> ts_10123502v010101p.pdf, the minimum tones gap duration is 65ms. Let's use
> 70 for safety.
> 
> @@ +900,5 @@
> > +                      aCallback) {
> > +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +
> > +    let playSingleTone = (function (tone, cb) {
> > +      this.startTone(aClientId, tone);
> 
> [comment 1]
> We need to consider error cases, notifyError, as I mentioned in my last
> review.
> 
> startTone could fail. Once it fails, we need to reject the promise. Please
> take care of the error handling. That means, you need to wait until the
> startTone callback comes from [1], then to either proceed to the next dtmf
> char or notifyError(), based on the callback result.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js?from=ril_worker.js&case=true#6127

To be more clear, we need:
1) 
RilObject.prototype[REQUEST_DTMF_START] = function REQUEST_DTMF_START(length, options) {
  options.success = (options.rilRequestError === 0);
  if (!options.success) {
    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
  }
  this.sendChromeMessage(options);
};

2) to replace |Buf.newParcel(REQUEST_DTMF_START);| with |Buf.newParcel(REQUEST_DTMF_START, options);|
(Assignee)

Comment 42

4 years ago
Posted patch Patch V3 (obsolete) — Splinter Review
Changes from comment 40
Attachment #8540670 - Attachment is obsolete: true
Attachment #8541267 - Flags: review?(htsai)
Comment on attachment 8541267 [details] [diff] [review]
Patch V3

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

Looks good. I also played with this and patch on bug 911055. Everything works well, thank you.

r=me with comments addressed.

::: dom/telephony/gonk/TelephonyService.js
@@ +904,5 @@
> +        if (!response.success) {
> +          aCallback.notifyError(response.errorMsg);
> +          return;
> +        }
> + 

nit: no trailing white space

@@ +921,5 @@
> +    };
> +
> +    timer.initWithCallback(() => {
> +      playTone(tones[0]);
> +    }, aPauseDuration, Ci.nsITimer.TYPE_ONE_SHOT);  

nit: no trailing white space

::: dom/webidl/Telephony.webidl
@@ +28,5 @@
>    [Throws]
>    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
> +  Promise<void> sendTones(DOMString tones, optional unsigned long pauseDuration = 3000 /* ms */, optional unsigned long toneDuration = 70 /* ms */, optional unsigned long serviceId);

Thanks for adding "ms"! But let's try to not have comments in an API function to have better readability.

Let's have below written in front of the function:
/**
  * Send a series of DTMF tones.
  *
  * @param tones
  *    DTMF chars.
  * @param pauseDuraton (ms) [optional]
  *    Time to wait before sending tones. Default value is 3000 ms.
  * @param toneDuration (ms) [optional]
  *    Duration of each tone. Default value is 70 ms.
  * @param serviceId [optional]
  *    Default value is as user setting dom.telephony.defaultServiceId.
  */
Attachment #8541267 - Flags: review?(htsai) → review+
(Assignee)

Comment 44

4 years ago
Posted patch Patch V4 (obsolete) — Splinter Review
Fixed nits from comment 43
Attachment #8541267 - Attachment is obsolete: true
Attachment #8541827 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
can we get a try run here ?
Flags: needinfo?(alberto.crespellperez)
Keywords: checkin-needed
Antonio, could you please run the try after your holiday period? Thanks in advance.
Flags: needinfo?(amac.bug)
No need to wait for my holidays to end. There you go: https://hg.mozilla.org/try/pushloghtml?changeset=821425cb0bc6
Flags: needinfo?(amac.bug)
Keeping the NI to me so I remember to check the try results.
Flags: needinfo?(amac.bug)
And of course, the link wasn't exactly helpful. Correct link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=821425cb0bc6

The try looks good to me. There are some failures but appear to be previously reported ones (I retriggered them just in case anyway, but flagged them already).

Re-requesting checkin.
Flags: needinfo?(amac.bug)
Keywords: checkin-needed
Needs rebasing against bug 1077075. Also, please use ./mach update-uuids for making UUID changes to make sure that any inheriting interfaces are also updated. In this case, nsIGonkTelephonyService will also be automatically updated if you do.
Keywords: checkin-needed
(Assignee)

Comment 51

4 years ago
Posted patch Patch V4Splinter Review
Rebased and updated uuids
Attachment #8541827 - Attachment is obsolete: true
Flags: needinfo?(alberto.crespellperez)
Attachment #8545059 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/841495120f82
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
I've documented this method, at https://developer.mozilla.org/en-US/docs/Web/API/Telephony.sendTones. Could someone please give it a technical review?
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #55)
> I've documented this method, at
> https://developer.mozilla.org/en-US/docs/Web/API/Telephony.sendTones. Could
> someone please give it a technical review?

I could help. But I am busy on other stuff so it might take some time.
anyway, I flag myself for tracking.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #56)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #55)
> > I've documented this method, at
> > https://developer.mozilla.org/en-US/docs/Web/API/Telephony.sendTones. Could
> > someone please give it a technical review?
> 
> I could help. But I am busy on other stuff so it might take some time.
> anyway, I flag myself for tracking.

Thank you - I really appreciate it! It isn't super urgent, so whenever you have some time is good.
Mdn review done & some errors were corrected.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #58)
> Mdn review done & some errors were corrected.

cool, thanks!
You need to log in before you can comment on or make changes to this bug.