997578 - [Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TWC_BV_02_I

Status: VERIFIED FIXED

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

Description

[Jocelyn Liu [:jocelyn] [:joliu]]

This case is originally be filed in bug993288.
File a new bug for TC_AG_TWC_BV_02_I only because the cause is not the same.
Please refer to the comment link below for QA message.
https://bugzilla.mozilla.org/show_bug.cgi?id=993288#c2

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Depends on bug993280 because we need to respond OK for AT+CHLD command.
Depends on bug993288 for sending +CCWA to HF.

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Patch: change behavior of endAndAnswer for CHLD=1

We found out that gaia use function endAndAnswer() in calls_handler.js to handle CHLD=1. But it seems this function will only end the active call, and will not answer the incoming call.
We did a quick fix by applying the attached patch, this case will pass after patch applied.

We also found out that this function is also used by the hangup button shown in the attached picture.
Need UX's info to confirm the expected behavior for this button is also the same as BT CHLD=1 case. (end the current active call and answer the incoming call)

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: 2014-04-17-08-01-38.png

Comment 4

[Carrie Wang [New account: [e-mail]]]

(In reply to jocelyn [:jocelyn] from comment #2)
> Created attachment 8407993 [details] [diff] [review]
> Patch: change behavior of endAndAnswer for CHLD=1
> 
> We found out that gaia use function endAndAnswer() in calls_handler.js to
> handle CHLD=1. But it seems this function will only end the active call, and
> will not answer the incoming call.
> We did a quick fix by applying the attached patch, this case will pass after
> patch applied.
> 
> We also found out that this function is also used by the hangup button shown
> in the attached picture.
> Need UX's info to confirm the expected behavior for this button is also the
> same as BT CHLD=1 case. (end the current active call and answer the incoming
> call)

Hey, please check p.37 of the attached spec. It illustrates the behavior of call waiting here. If users tap hang up button, the current call will be hung up and the secondary call will be connected directly.
Thanks!

Comment 5

[Carrie Wang [New account: [e-mail]]]

Attachment: Dialer UX Spec 0.7(1).pdf

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Etienne, 

According to Dialer UX Spec, it seems we should answer the incoming call in endAndAnwser().
Please refer to attachment8407993 [details] [diff] [review] which can fix bluetooth CHLD=1 and hangup button behavior.
Could you help us to fix this bug on gaia side?

Thanks,
Jocelyn

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 997578 - Answer incoming call after handup active call

Hi Etienne,

I have made a pull request at https://github.com/mozilla-b2g/gaia/pull/18510/files to fix the behavior of endAndAnswer().
Also update the calls_handler test for CHLD=1.
Could you help me review my PR?

Thanks,
Jocelyn

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8410025 [details]
Bug 997578 - Answer incoming call after handup active call

Comments on github, this doesn't work for me probably because of a race condition.

Do we know why/when gecko stoped answering the other call like it says in the original comment?

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 997578(v2) - Answer incoming call after handup active call

Hi Etienne,

Thanks for the review!
I have updated the patch.
Please visit https://github.com/mozilla-b2g/gaia/pull/18510/files again.

Hi HsinYi,
Did gecko answer the incoming call directly before?

Thanks,
Jocelyn

Comment 10

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

Hi Etienne,

At the every beginning when gecko started to support call waiting (Bug 735170), gecko provided |the hangup 1st and answer 2nd| scenario if call_1.hangUp() was called. But right after, about 2 months later (Bug 766273), we found an error so we changed the implementation, which becomes "hanging up a call doesn't automatically answer a 2nd incoming call." And this behaviour lasts since bug 766273.

IMO, hanging up a call doesn't necessarily imply answering a subsequent call, so the current behaviour looks reasonable to me. But if providing a fantastic composed function helps gaia, then we could see how we could propose an additional function with capability of "hangUpAndAnswer" in Telephony API. How do you think?

Comment 11

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #10)
> Hi Etienne,
> 
> At the every beginning when gecko started to support call waiting (Bug
> 735170), 

Sorry, this is the right bug number: bug 714968

> gecko provided |the hangup 1st and answer 2nd| scenario if
> call_1.hangUp() was called. But right after, about 2 months later (Bug
> 766273), we found an error so we changed the implementation, which becomes
> "hanging up a call doesn't automatically answer a 2nd incoming call." And
> this behaviour lasts since bug 766273.
> 
> IMO, hanging up a call doesn't necessarily imply answering a subsequent
> call, so the current behaviour looks reasonable to me. But if providing a
> fantastic composed function helps gaia, then we could see how we could
> propose an additional function with capability of "hangUpAndAnswer" in
> Telephony API. How do you think?

Comment 12

[Ben Tian (inactive)]

Etienne & Hsinyi,

Since this is a 1.4+ bug to be fixed by Friday (4/25), I suggest to revise gaia based on Jocelyn's approach first, and open another bug to discuss 1) new RIL API to hangup+answer and 2) conference call case change on m-c.

Let me know if you have other suggestion. Thanks.

Comment 13

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

(In reply to Ben Tian [:btian] from comment #12)
> Etienne & Hsinyi,
> 
> Since this is a 1.4+ bug to be fixed by Friday (4/25), I suggest to revise
> gaia based on Jocelyn's approach first, and open another bug to discuss 1)
> new RIL API to hangup+answer and 2) conference call case change on m-c.
> 
> Let me know if you have other suggestion. Thanks.

Hi Ben,

Your suggestion sounds good to me. The current TelephonyAPI should be enough for the complex composition.

Regarding a super API such as hangUpAndAnswer(), for now, I don't really have a good idea. And there are detailed behaviours to be confirmed, especially considering conference call scenario, e.g. what should happen when conferenceGroup.calls[x].hangUpAndAnswer()? are we going to expose this function to a conference call? ...etc. So I agree moving this discussion to another thread. Thanks!

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8410766 [details]
Bug 997578(v2) - Answer incoming call after handup active call

The code looks perfectly fine, but it doesn't work for me and I've tried on multiple phones.

Which phone/version were you using for testing?

Comment 15

[Etienne Segonzac (:etienne)]

Attachment: With the patch applied

This is the screen I get stuck on...
Pressing the hangup button will close the screen and then a new incoming screen will show up with the call that was previously waiting.

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Etienne,

I was using v1.4 and and it works fine.

The last commit of my local branch using m-c was

commit f0463704888881b8ed1619e8d4b0d851b0e0311b
Merge: 5fe2185 e6f6733
Author: John Hu <[e-mail]>
Date:   Tue Apr 22 09:59:03 2014 +0800

    Merge pull request #18359 from huchengtw-moz/gallery/Bug_997051-master
    
    Bug 997051 - [Gallery] the MAX_IMAGE_SIZE in inline viewer doesn't not u...

However, my dialer app didn't work right now for this version with or without this patch.
(Make a phone call by the other phone to the test device, no callscreen will be shown.)

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

Forgot to mention, I was using nexus4. :)

Comment 18

[Ben Tian (inactive)]

Etienne,

I can verify Jocelyn's patch on nexus-4 as following. Please have a try and let me know for any problem. 

Also what else phone do you have?

--
Flash central-nexus-4-eng build: 20140423-160202, and then reset gaia based on commit:

commit b64555b9ed961923b923239e96f87d6ba83cf951
Merge: 18fd4d4 570baab
Author: Ryan VanderMeulen <[e-mail]>
Date:   Wed Apr 23 18:20:15 2014 -0700

    Merge pull request #18505 from zbraniecki/999195-cleanup-mozl10n-emergency-call
    
    Bug 999195 - Clean up bootstrap mozL10n API use in Emergency Call

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8410766 [details]
Bug 997578(v2) - Answer incoming call after handup active call

I tested on a keon and a buri. In both cases it fails. Logging the number+state for each oncallschanged I get:

E/GeckoConsole(  554): Content JS LOG at app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged: +++ got a callschanged []
E/GeckoConsole(  554): Content JS LOG at app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged: +++ got a callschanged ["888-dialing"]
E/GeckoConsole(  554): Content JS LOG at app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged: +++ got a callschanged ["888-connected","[phone]XXX-incoming"]

// Pressing the end and answer button

E/GeckoConsole(  554): Content JS LOG at app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged: +++ got a callschanged ["[phone]XXX-connecting"]


// It stays stuck in the "connecting" state
// The screen is as in my attachement, pressing the hangup button...

E/GeckoConsole(  554): Content JS LOG at app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged: +++ got a callschanged []


Might be a RIL issue, but in the meantime we're better off without the patch since with the current version pressing end and answer will pass the screen in "incoming mode"  which is weird but at least you can answer the call.

I'll reconsider if this works and is needed for a shipping device, but the nexus4 isn't one.

Comment 20

[Ben Tian (inactive)]

(In reply to Etienne Segonzac (:etienne) from comment #19)
> I tested on a keon and a buri. In both cases it fails. Logging the

Please let me know your gecko and gaia version on Buri. I'll verify with my Buri.

> Might be a RIL issue, but in the meantime we're better off without the patch
> since with the current version pressing end and answer will pass the screen
> in "incoming mode"  which is weird but at least you can answer the call.

We should fix this bug since it doesn't follow UX spec (comment 5) and fails bluetooth certification. Do you suggest any gaia developer who can help us with a better solution?

Comment 21

[Ben Tian (inactive)]

Hsinyi,

Etienne met the following problem while verifying Jocelyn's patch to end and answer incoming call. Do you have any idea why the incoming call became stuck in 'connecting'?

(In reply to Etienne Segonzac (:etienne) from comment #19)
> I tested on a keon and a buri. In both cases it fails. Logging the
> number+state for each oncallschanged I get:
> 
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged []
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged ["888-dialing"]
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged ["888-connected","[phone]XXX-incoming"]
> 
> // Pressing the end and answer button
> 
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged ["[phone]XXX-connecting"]
> 
> 
> // It stays stuck in the "connecting" state
> // The screen is as in my attachement, pressing the hangup button...
> 
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged []
> 
> 
> Might be a RIL issue, but in the meantime we're better off without the patch
> since with the current version pressing end and answer will pass the screen
> in "incoming mode"  which is weird but at least you can answer the call.
> 
> I'll reconsider if this works and is needed for a shipping device, but the
> nexus4 isn't one.

Comment 22

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

(In reply to Ben Tian [:btian] from comment #21)
> Hsinyi,
> 
> Etienne met the following problem while verifying Jocelyn's patch to end and
> answer incoming call. Do you have any idea why the incoming call became
> stuck in 'connecting'?
> 
> (In reply to Etienne Segonzac (:etienne) from comment #19)
> > I tested on a keon and a buri. In both cases it fails. Logging the
> > number+state for each oncallschanged I get:
> > 
> > E/GeckoConsole(  554): Content JS LOG at
> > app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> > +++ got a callschanged []
> > E/GeckoConsole(  554): Content JS LOG at
> > app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> > +++ got a callschanged ["888-dialing"]
> > E/GeckoConsole(  554): Content JS LOG at
> > app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> > +++ got a callschanged ["888-connected","[phone]XXX-incoming"]
> > 
> > // Pressing the end and answer button
> > 
> > E/GeckoConsole(  554): Content JS LOG at
> > app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> > +++ got a callschanged ["[phone]XXX-connecting"]
> > 
> > 
> > // It stays stuck in the "connecting" state
> > // The screen is as in my attachement, pressing the hangup button...
> > 
> > E/GeckoConsole(  554): Content JS LOG at
> > app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> > +++ got a callschanged []
> > 
> > 
> > Might be a RIL issue, but in the meantime we're better off without the patch
> > since with the current version pressing end and answer will pass the screen
> > in "incoming mode"  which is weird but at least you can answer the call.
> > 
> > I'll reconsider if this works and is needed for a shipping device, but the
> > nexus4 isn't one.

Please provide me log with ril debugger enabled. And capture the messages by "adb logcat -b radio -b main". Thanks.

Comment 23

[Ben Tian (inactive)]

ni? Etienne for comment 20.

Comment 24

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

(In reply to Etienne Segonzac (:etienne) from comment #19)
> Comment on attachment 8410766 [details]
> Bug 997578(v2) - Answer incoming call after handup active call
> 
> I tested on a keon and a buri. In both cases it fails. Logging the
> number+state for each oncallschanged I get:
> 
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged []
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged ["888-dialing"]
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged ["888-connected","[phone]XXX-incoming"]
> 
> // Pressing the end and answer button
> 
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged ["[phone]XXX-connecting"]
> 
> 
> // It stays stuck in the "connecting" state
> // The screen is as in my attachement, pressing the hangup button...
> 
> E/GeckoConsole(  554): Content JS LOG at
> app://callscreen.gaiamobile.org/js/calls_handler.js:73 in onCallsChanged:
> +++ got a callschanged []
> 
> 
> Might be a RIL issue, but in the meantime we're better off without the patch
> since with the current version pressing end and answer will pass the screen
> in "incoming mode"  which is weird but at least you can answer the call.
> 
> I'll reconsider if this works and is needed for a shipping device, but the
> nexus4 isn't one.

The original patch causes racing on modem.

Please try:

if (callToEnd && callToAnswer) {
  callToEnd.ondisconnected = function(event) {
    callToAnswer.answer();
  };
  callToEnd.hangUp();
} else if (callToEnd) {
  callToEnd.hangUp(); // hangup the active call
} else if (callToAnswer) {
  callToAnswer.answer(); // answer the incoming call
}

This works on buri.

Comment 25

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #24)
> The original patch causes racing on modem.
> 
> Please try:
> 
> if (callToEnd && callToAnswer) {
>   callToEnd.ondisconnected = function(event) {
>     callToAnswer.answer();
>   };
>   callToEnd.hangUp();
> } else if (callToEnd) {
>   callToEnd.hangUp(); // hangup the active call
> } else if (callToAnswer) {
>   callToAnswer.answer(); // answer the incoming call
> }
> 


Thanks Hsin-Yi for saving us once again!

We should implement this suggestion and update the test to cover the wait for ondisconnected (tip: you can call _disconnect() on the mock call), I'll make sure this gets reviewed quickly :)

Comment 26

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

(In reply to Etienne Segonzac (:etienne) from comment #25)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #24)
> > The original patch causes racing on modem.

Aha, on request by Ben, let me explain more about the "racing." :)

There are two types of underlying request for answering a call.
We use type_1 for answering an incoming call (i.e. there's no other call than this incoming call), and type_2 for a waiting call, i.e. there's already another call.

In the original patch, when gecko receives the two requests from gaia, gecko sends type_2 to modem for answering because the callToEnd isn't released yet (thought it's in process from the perspective of gecko).

However, when modem receives type_2, it's very likely that callToEnd is released then. So from the perspective of modem, at that time, type_2 isn't a valid request because there's no other call.

That's the racing case I mentioned. Hope it's clear!


> > Please try:
> > 
> > if (callToEnd && callToAnswer) {
> >   callToEnd.ondisconnected = function(event) {
> >     callToAnswer.answer();
> >   };
> >   callToEnd.hangUp();
> > } else if (callToEnd) {
> >   callToEnd.hangUp(); // hangup the active call
> > } else if (callToAnswer) {
> >   callToAnswer.answer(); // answer the incoming call
> > }
> > 
> 
> 
> Thanks Hsin-Yi for saving us once again!
> 
> We should implement this suggestion and update the test to cover the wait
> for ondisconnected (tip: you can call _disconnect() on the mock call), I'll
> make sure this gets reviewed quickly :)

Comment 27

[Ben Tian (inactive)]

Attachment: PR - endAndAnswer: Answer incoming call after hanging up the active call

Thanks Hsinyi! Your are our heroine!

Etienne, please help review the revised patch. Thanks.

Comment 28

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

Glad to help out :D

Comment 29

[Etienne Segonzac (:etienne)]

Comment on attachment 8412476 [details]
PR - endAndAnswer: Answer incoming call after hanging up the active call

r=me with the .ondisconnected call moved to the mock.

Thanks all!

Comment 30

[Ben Tian (inactive)]

Comment on attachment 8412476 [details]
PR - endAndAnswer: Answer incoming call after hanging up the active call

(In reply to Etienne Segonzac (:etienne) from comment #29)
> Comment on attachment 8412476 [details]
> PR - endAndAnswer: Answer incoming call after hanging up the active call
Etienne, can you review my revised patch again? Some test cases without .disconnected definition would fail if .disconnected is moved into mock_call.js, so I revise my original patch with some large changes as following:

Changes:
1) Use addEventListener/removeEventListener to avoid override on existing (if any) .ondisconnected event handler.
2) Call _disconnect() in MockCall.hangUp().
3) Revise a conference call test case since 2) fails it.
CAUSE -
  The test case adds 3 calls at once and the 3rd one would be hanged up in [1]. With 2) the 3rd call's state becomes 'disconnected' and [2] doesn't add the 3rd call into handledCalls due to [3]. Finally [4] cannot find the 3rd call in handledCalls to resume the conference call in [5][6].
FIX -
  Merge conference call first and then add the 3rd call. I think the sequence is more reasonable and matches real situation.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L132
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L90
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L126
[4] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L95
[5] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L108
[6] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/calls_handler.js#L216

Comment 31

[Etienne Segonzac (:etienne)]

Comment on attachment 8412476 [details]
PR - endAndAnswer: Answer incoming call after hanging up the active call

all good, thanks!

Comment 32

[Ben Tian (inactive)]

Thanks Etienne. Merged PR into gaia master: https://github.com/mozilla-b2g/gaia/commit/8cfa680804a434ea5370cc9a73ed3a2731541404

Comment 33

[Ben Tian (inactive)]

Since some files are relocated, manually uplift to gaia v1.4: https://github.com/mozilla-b2g/gaia/commit/cf590ecb161e7b9fb4ccc672f950072acad62caa

Comment 34

[Eric Chang [:ericcc] [:echang]]

### Build for Verification 
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-b2g30_v1_4-nexus-4/2014/05/2014-05-04-16-02-01/