1008561 - [Flame] [MozRIL] DTMF_STOP failure

Status: RESOLVED INVALID

(Firefox OS Graveyard :: RIL, defect)

Description

[:gerard-majax]

Custom built system (available in https://mozilla.app.box.com/files/0/f/1932801249/FlameCustomBuilds).

I turned on RILC debug as this:

> In your B2G build tree, edit hardware/ril/libril/ril.cpp and switch the RILC_LOG define to 1. Then rebuild and push libril.so:
> $ ./build.sh out/target/product/flame/system/lib/libril.so
> $ adb root && adb wait-for-device && adb remount
> $ adb push out/target/product/flame/system/lib/libril.so /system/lib/libril.so

When placing calls, I see this in adb logcat -b radio:

D/RILC    (  345): [0416]> DTMF_STOP 
D/RILC    (  345): [0416]< DTMF_STOP fails by E_GENERIC_FAILURE

I have no idea whether this is important or not.

Comment 1

[:gerard-majax]

DTMF itself seems to be working properly when calling voicemail and pressing touches, although it's not very nice (but this seems to be a known bug).

Comment 2

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

(In reply to Alexandre LISSY :gerard-majax from comment #1)
> DTMF itself seems to be working properly when calling voicemail and pressing
> touches, although it's not very nice (but this seems to be a known bug).

(In reply to Alexandre LISSY :gerard-majax from comment #0)
> Custom built system (available in
> https://mozilla.app.box.com/files/0/f/1932801249/FlameCustomBuilds).
> 
> I turned on RILC debug as this:
> 
> > In your B2G build tree, edit hardware/ril/libril/ril.cpp and switch the RILC_LOG define to 1. Then rebuild and push libril.so:
> > $ ./build.sh out/target/product/flame/system/lib/libril.so
> > $ adb root && adb wait-for-device && adb remount
> > $ adb push out/target/product/flame/system/lib/libril.so /system/lib/libril.so
> 
> When placing calls, I see this in adb logcat -b radio:
> 
> D/RILC    (  345): [0416]> DTMF_STOP 
> D/RILC    (  345): [0416]< DTMF_STOP fails by E_GENERIC_FAILURE
> 
> I have no idea whether this is important or not.

This is the response from rild/modem. I think we need the vendor's help to explain why this command fails.

Comment 3

[shawn ku [:sku]]

If there is no DTMF_START, DTMF_STOP request will get failure.
If there is no active/alerting call(s), modem will also return failure.

From my perspective, this might be some error handling (need to check who issue DTMF_STOP command first) to prevent only DTMF_START but no DTMF_STOP case, however, the error should be okay w/o impacting the usage.

Comment 4

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

(In reply to shawn ku [:sku] from comment #3)
> If there is no DTMF_START, DTMF_STOP request will get failure.
> If there is no active/alerting call(s), modem will also return failure.
> 
> From my perspective, this might be some error handling (need to check who
> issue DTMF_STOP command first) to prevent only DTMF_START but no DTMF_STOP
> case, however, the error should be okay w/o impacting the usage.

Thanks for explanation, Shawn.

Comment 5

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi HsinYi -

So do we still need T2M to check this problem? Do we see the same DTMF_STOP error with QCT RIL?

Thanks!

Comment 6

[Ken Chang[:kenkai]]

It seems that we need to do error handling well for this bug. But may I know if this bug causes any use cases failed first?

Comment 7

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

(In reply to Vance Chen [:vchen][[e-mail]] from comment #5)
> Hi HsinYi -
> 
> So do we still need T2M to check this problem? Do we see the same DTMF_STOP
> error with QCT RIL?
> 
> Thanks!

As Shawn's comment, we'd need to check if Gaia(the caller) sends DTMF_STOP at the right time. Then we could have a better understanding if this is a shortcoming on FxOS side or vendor.

(In reply to Ken Chang[:ken] from comment #6)
> It seems that we need to do error handling well for this bug. But may I know
> if this bug causes any use cases failed first?

Per my understanding, no.

Comment 8

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #5)
> > Hi HsinYi -
> > 
> > So do we still need T2M to check this problem? Do we see the same DTMF_STOP
> > error with QCT RIL?
> > 
> > Thanks!
> 
> As Shawn's comment, we'd need to check if Gaia(the caller) sends DTMF_STOP
> at the right time. Then we could have a better understanding if this is a
> shortcoming on FxOS side or vendor.
> 
> (In reply to Ken Chang[:ken] from comment #6)
> > It seems that we need to do error handling well for this bug. But may I know
> > if this bug causes any use cases failed first?
> 
> Per my understanding, no.

Hi Ken, Hsin-Yi -

Neh I just want to go through all the T2M relevant issues to see if there is anything I can help to push T2M. Glad to know that for this one we don't need T2M's help, at least for now

Thanks

Comment 9

[helloarvin]

(In reply to Alexandre LISSY :gerard-majax from comment #0)
> Custom built system (available in
> https://mozilla.app.box.com/files/0/f/1932801249/FlameCustomBuilds).
> 
> I turned on RILC debug as this:
> 
> > In your B2G build tree, edit hardware/ril/libril/ril.cpp and switch the RILC_LOG define to 1. Then rebuild and push libril.so:
> > $ ./build.sh out/target/product/flame/system/lib/libril.so
> > $ adb root && adb wait-for-device && adb remount
> > $ adb push out/target/product/flame/system/lib/libril.so /system/lib/libril.so
> 
> When placing calls, I see this in adb logcat -b radio:
> 
> D/RILC    (  345): [0416]> DTMF_STOP 
> D/RILC    (  345): [0416]< DTMF_STOP fails by E_GENERIC_FAILURE
> 
> I have no idea whether this is important or not.

Hi Alexandre,

I checked the gaia code and found that the root-cause for this might be:

DtmfTone.prototype.play: function dt_play() {
    var self = this;
    clearTimeout(this.timer);

    // Stop previous tone before dispatching a new one
    navigator.mozTelephony.stopTone(this.serviceId);          ------>root-cause
    navigator.mozTelephony.startTone(this.tone, this.serviceId);

From the comment above 'Stop previous tone before dispatching a new one', this should be a mandatory  fault tolerance method.

Do you think so?

Comment 10

[:gerard-majax]

Thanks for looking through this. I'm not sure to get your point. Do you mean that it's not too bad to leave it like this?

Anyway, better to needinfo? people who wrote this DtmfTone code, since I can't help much :)

Comment 11

[helloarvin]

I'm sorry to reply late for the holiday.

In my opinion the current 'stop --- start' design of DtmfTone playing is a mandatory  fault tolerance method. But I'm not sure if it's not too bad to leave it like this or not.

Maybe etienne can help to explain the issue, thanks.

(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Thanks for looking through this. I'm not sure to get your point. Do you mean
> that it's not too bad to leave it like this?
> 
> Anyway, better to needinfo? people who wrote this DtmfTone code, since I
> can't help much :)

Comment 12

[Etienne Segonzac (:etienne)]

Forwarding to Gabriele.

Comment 13

[Gabriele Svelto [:gsvelto]]

I wrote the original code and the intent was to ensure that all call-paths invoking DtmfTone.play() would get it right and we would under no conditions get overlapping tones. When using long tones a tone will only be stopped when the user stops pressing a key but when using short tones that can also happen via a timer. Since this can lead to race conditions I preferred to play it safe and always stop before starting, just in case. This also ensures that futures changes won't ever get the chance to "get it wrong" so I'd rather keep the code and ignore the (harmless) error.