### ENV Buri/Bluez [1] https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-aurora-hamachi-eng/2014/03/2014-03-25-16-02-01/ ### STR 1. PTS 5.0 2. TC_AG_TWC_BV_04_I ### Expected Pass ### Actual Inconclusive [Output] Test case : TC_AG_TWC_BV_04_I started - SDP Service record for PTS: 'Handsfree HF' successfully registered - The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, - The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, - Using SDP to determine IUT RFCOMM server channel number. - ProtocolDescriptorList: IUT reports that its RFCOMM server channel number is 10 - AT: SPP connect succeeded - AT: Service Level Connection established - HCI: Audio Connection enabled - AT: post SLC command sequence complete - FATAL ERROR (AT): MTC requests a call join (AT+CHLD=3) when there is not both an active and a held call) - MTC received unexpected EXIT message from AT component - AT: Call terminated - FATAL ERROR (AT): (call = 1) received when (callsetup = 0) - WARNING (AT): +CIEV(callheld=2) received when there is no pending CHLD operation. - HCI: Audio Connection disabled - AT: SPP disconnect succeeded - MTC: Test case ended Final Verdict : Inconclusive

[Note on STR] This test case requires SIM card's conference call function be activated as it verifies conference call functionality. -- The bug cause is that HFP manager doesn't update callheld state correctly when 1 active call and 1 held already exist before SLC is connected. Here are the call state changes HFP manager receives in order: call 1: DISCONNECTED -> HELD call 2: DISCONNECTED -> CONNECTED <=== HFP manager doesn't handle this case correctly.

Comment on attachment 8403205 [details] [diff] [review] [Bluetooth] Fix the incorrect condition for reporting call held status when call state changed. (v0) The patch has verified on related PTS testes. TC_AG_TWC_BV_04_I, Pass TC_AG_TWC_BV_05_I, Pass TC_AG_ECS_BV_01_I, Pass TC_AG_ECS_BV_02_I, Pass TC_AG_ECC_BI_03_I, Pass

Comment on attachment 8403205 [details] [diff] [review] [Bluetooth] Fix the incorrect condition for reporting call held status when call state changed. (v0) Review of attachment 8403205 [details] [diff] [review]: ----------------------------------------------------------------- Give f- since I think the patch is incomplete and the logic is unclear. ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp @@ +1415,4 @@ > > switch (aCallState) { > case nsITelephonyProvider::CALL_STATE_HELD: > if (prevCallState == nsITelephonyProvider::CALL_STATE_CONNECTED) { |switch| statement is clearer to handle two prevCallState cases. @@ +1417,5 @@ > case nsITelephonyProvider::CALL_STATE_HELD: > if (prevCallState == nsITelephonyProvider::CALL_STATE_CONNECTED) { > + // mCurrentCallArray[0] is an invalid (padding) call object, > + // (Length() == 2) means there is only one valid call in call array. > + if (mCurrentCallArray.Length() == 2) { Add |MOZ_ASSERT(mCurrentCallArray.Length() > 1)| before |if (length==2)|. The if statement doesn't handle |length==1| case but |prevCallState==CONNECTED| implies |length>1|. @@ +1422,4 @@ > // A single active call is put on hold (+CIEV, callheld=2) > sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_NOACTIVE; > } else { > // Releases all active calls and accepts the other (+CIEV, callheld=1) I suggest 'put on hold' instead of 'release' since 'release' is more like 'disconnect' to me. @@ +1426,5 @@ > sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > } > SendCommand(RESPONSE_CIEV, CINDType::CALLHELD); > + } else if (prevCallState == nsITelephonyProvider::CALL_STATE_DISCONNECTED) { > + // User wants to answer an incoming call and hold the current call. The comment doesn't explain why prevCallState is DISCONNECTED. Also a outgoing call comes here as well. @@ +1427,5 @@ > } > SendCommand(RESPONSE_CIEV, CINDType::CALLHELD); > + } else if (prevCallState == nsITelephonyProvider::CALL_STATE_DISCONNECTED) { > + // User wants to answer an incoming call and hold the current call. > + // Releases all active calls and accepts the other (+CIEV, callheld=1) Ditto. 'release'. @@ +1487,5 @@ > // Outgoing call > UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend); > UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend); > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)) { > + // User wants to answer an incoming call and hold the current call. The comment mismatches the action here that an incoming/outgoing call becomes a connected call. @@ +1488,5 @@ > UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend); > UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend); > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)) { > + // User wants to answer an incoming call and hold the current call. > + // Releases all active calls and accepts the other (+CIEV, callheld=1) Ditto. 'release' @@ +1490,5 @@ > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)) { > + // User wants to answer an incoming call and hold the current call. > + // Releases all active calls and accepts the other (+CIEV, callheld=1) > + UpdateCIND(CINDType::CALLHELD, CallHeldState::ONHOLD_ACTIVE, aSend); > + } Should handle no call held case in |else|?

(In reply to Ben Tian [:btian] from comment #4) > Comment on attachment 8403205 [details] [diff] [review] > [Bluetooth] Fix the incorrect condition for reporting call held status when > call state changed. (v0) > > Review of attachment 8403205 [details] [diff] [review]: Thank you for your comment. I've uploaded a new patch based on your suggestion. > ----------------------------------------------------------------- > @@ +1490,5 @@ > > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)) { > > + // User wants to answer an incoming call and hold the current call. > > + // Releases all active calls and accepts the other (+CIEV, callheld=1) > > + UpdateCIND(CINDType::CALLHELD, CallHeldState::ONHOLD_ACTIVE, aSend); > > + } > > Should handle no call held case in |else|? I think we don't have to since we would handle it when prevCallState == HELD or prevCallState == CONNECTED.

Comment on attachment 8405957 [details] [diff] [review] [Bluetooth] Fix the incorrect condition for reporting call held status when call state changed. (v2) Review of attachment 8405957 [details] [diff] [review]: ----------------------------------------------------------------- Please revise the logic for call state change CONNECTED -> HELD, and split numberPhoneConnect into numHeld and numActive for clarity. ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp @@ +1433,5 @@ > case nsITelephonyProvider::CALL_STATE_HELD: > + switch (prevCallState) { > + case nsITelephonyProvider::CALL_STATE_CONNECTED: > + // The call state changed from CONNECTED to HELD. If none of calls > + // remain CONNECTED, send ONHOLD_NOACTIVE, otherwise, send ONHOLD_ACTIVE. nit: I prefer to move the comment into parentheses. @@ +1435,5 @@ > + case nsITelephonyProvider::CALL_STATE_CONNECTED: > + // The call state changed from CONNECTED to HELD. If none of calls > + // remain CONNECTED, send ONHOLD_NOACTIVE, otherwise, send ONHOLD_ACTIVE. > + { > + uint32_t numberOfPhoneConnection Two separate numHeld and numActive is clearer. We can add them if needed. @@ +1438,5 @@ > + { > + uint32_t numberOfPhoneConnection > + = GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_HELD) > + + GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_CONNECTED); > + Please write comment here to summarize the following logic. @@ +1441,5 @@ > + + GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_CONNECTED); > + > + if (!aIsConference) { > + // It means there is no active call. > + if(numberOfPhoneConnection == 1) { |if (!numActive)| @@ +1448,5 @@ > + } else { > + // Active call is placed on hold or active/held calls swapped > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + } > + SendCommand("+CIEV: ", CINDType::CALLHELD); Replace "+CIEV: " with CIEV_RESPONSE. Also can we call |UpdateCIND| instead of |SendCommand|? @@ +1452,5 @@ > + SendCommand("+CIEV: ", CINDType::CALLHELD); > + } else { > + // It means there will be no active calls when the conference > + // call transition has finished. > + if(numberOfPhoneConnection == GetNumberOfConCalls()) { |if (numHeld + numActive == GetNumberOfConCalls())| The CIEV is sent at every state change during conference call transition. I think we should only send CIEV at the last one. @@ +1455,5 @@ > + // call transition has finished. > + if(numberOfPhoneConnection == GetNumberOfConCalls()) { > + // An active conference call is put on hold. > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_NOACTIVE; > + SendCommand("+CIEV: ", CINDType::CALLHELD); Ditto. @@ +1458,5 @@ > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_NOACTIVE; > + SendCommand("+CIEV: ", CINDType::CALLHELD); > + // Send CIEV only when the conference call transition has finished. > + } else if (GetNumberOfConCalls(nsITelephonyProvider::CALL_STATE_HELD) > + == GetNumberOfConCalls()) { nit: please align the if condition. @@ +1461,5 @@ > + } else if (GetNumberOfConCalls(nsITelephonyProvider::CALL_STATE_HELD) > + == GetNumberOfConCalls()) { > + // Active calls are placed on hold or active/held calls swapped > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + SendCommand("+CIEV: ", CINDType::CALLHELD); Ditto. @@ +1473,5 @@ > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) { > + // If there is already a CONNECTED call, we should send ONHOLD_ACTIVE > + // to indicate that we have both CONNECTED and HELD calls. > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + SendCommand("+CIEV: ", CINDType::CALLHELD); Ditto.

Thank you for your comment. I've uploaded a new patch based on your suggestion. (In reply to Ben Tian [:btian] from comment #8) > Comment on attachment 8405957 [details] [diff] [review] > [Bluetooth] Fix the incorrect condition for reporting call held status when > call state changed. (v2) > > Review of attachment 8405957 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +1441,5 @@ > > + + GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_CONNECTED); > > + > > + if (!aIsConference) { > > + // It means there is no active call. > > + if(numberOfPhoneConnection == 1) { > > |if (!numActive)| > I use |if (numHeld + numActive == 1)| instead since numActive could temporary be zero when user put all active calls on hold and accepts the other. > @@ +1448,5 @@ > > + } else { > > + // Active call is placed on hold or active/held calls swapped > > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > > + } > > + SendCommand("+CIEV: ", CINDType::CALLHELD); > > Replace "+CIEV: " with CIEV_RESPONSE. Also can we call |UpdateCIND| instead > of |SendCommand|? I think it's OK to call |UpdateCIND| instead of |SendCommand| for ONHOLD_NOACTIVE. However, it wouldn't work on ONHOLDOACTIVE and would cause PTS failure if user swap held calls twice.

Comment on attachment 8406668 [details] [diff] [review] [Bluetooth] Fix the incorrect condition for reporting call held status when call state changed. (v3) Review of attachment 8406668 [details] [diff] [review]: ----------------------------------------------------------------- f=me as the logic looks correct to me. Still I suggest to revise comment as following to make the logic easier to understand. ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp @@ +1446,5 @@ > > switch (aCallState) { > case nsITelephonyProvider::CALL_STATE_HELD: > + switch (prevCallState) { > + case nsITelephonyProvider::CALL_STATE_CONNECTED: { nit: next line for the left parenthesis. @@ +1448,5 @@ > case nsITelephonyProvider::CALL_STATE_HELD: > + switch (prevCallState) { > + case nsITelephonyProvider::CALL_STATE_CONNECTED: { > + // The call state changed from CONNECTED to HELD. If none of calls > + // remain CONNECTED, send ONHOLD_NOACTIVE, otherwise, send ONHOLD_ACTIVE. Remove this part of comment. @@ +1453,5 @@ > + uint32_t numActive = GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_CONNECTED); > + uint32_t numHeld = GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_HELD); > + uint32_t numConCalls = GetNumberOfConCalls(); > + > + // Don't determine the callheld state simply by calculating the /** * An active call becomes a held call. * * If this call is not a conference call, * - callheld state = ONHOLD_NOACTIVE if no active call remains; * - callheld state = ONHOLD_ACTIVE otherwise. * If this call belongs to a conference call and all other members of the conference call have become held calls, * - callheld state = ONHOLD_NOACTIVE if no active call remains; * - callheld state = ONHOLD_ACTIVE otherwise. * * Note number of active calls may be 0 in-between state transition (c1 has become held but c2 has not become active yet), so we regard no active call remains if there is no other active/held call besides this changed call/group of conference call. */ @@ +1463,5 @@ > + // Thus, I use number of 'connected call' to determine whether any > + // call is active after the call state transition has finished. > + // Note: The 'connected call' includes active calls and held calls. > + if (!aIsConference) { > + // It means there is no active call. Remove this part of comment. @@ +1473,5 @@ > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + } > + SendCommand(RESPONSE_CIEV, CINDType::CALLHELD); > + } > + // Send CIEV only when the conference call transition has finished. Remove this part of comment. @@ +1477,5 @@ > + // Send CIEV only when the conference call transition has finished. > + else if (GetNumberOfConCalls(nsITelephonyProvider::CALL_STATE_HELD) > + == numConCalls) { > + // It means there will be no active calls when the conference > + // call transition has finished. Remove this part of comment. @@ +1493,5 @@ > + case nsITelephonyProvider::CALL_STATE_DISCONNECTED: > + // The call state changed from DISCONNECTED to HELD. It could happen > + // when user held a call before Bluetooth got connected. > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) { > + // If there is already a CONNECTED call, we should send ONHOLD_ACTIVE // callheld = ONHOLD_ACTIVE if an active call already exists. @@ +1557,5 @@ > // Outgoing call > UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend); > UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend); > + > + // User may answer an incoming call and wants to hold the current call, /** * A call becomes active because: * - user answers an incoming call, * - user dials a outgoing call and it is answered, or * - SLC is connected when a call is active. */ @@ +1562,5 @@ > + // or starts a outgoing call, or simply got Bluetooth connected when > + // a call is active. > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)) { > + // If there is a call on hold, we should send ONHOLD_ACTIVE, since we > + // have a connected call here. // callheld state = ONHOLD_ACTIVE if a held call already exists.

Thank you for your comment It's very helpful. > ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp > @@ +1446,5 @@ > > > > switch (aCallState) { > > case nsITelephonyProvider::CALL_STATE_HELD: > > + switch (prevCallState) { > > + case nsITelephonyProvider::CALL_STATE_CONNECTED: { > > nit: next line for the left parenthesis. I keep this one unchanged since I want to follow the coding style on MDN [1]. [1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures.

Comment on attachment 8407453 [details] [diff] [review] [Bluetooth] Fix the incorrect condition for reporting call held status when call state changed. (v4) f=btian Review of attachment 8407453 [details] [diff] [review]: ----------------------------------------------------------------- Hi Jamin, Please see my comments below. Thanks. ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp @@ +1472,5 @@ > + if (numActive + numHeld == 1) { > + // A single active call is put on hold. > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_NOACTIVE; > + } else { > + // Active call is placed on hold or active/held calls swapped nit: 'An active call' @@ +1474,5 @@ > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_NOACTIVE; > + } else { > + // Active call is placed on hold or active/held calls swapped > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + } Question: there is a case which would not be handled but actually I don't know if that would happen: One active call + One held call =====> Two held calls @@ +1477,5 @@ > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + } > + SendCommand(RESPONSE_CIEV, CINDType::CALLHELD); > + } else if (GetNumberOfConCalls(nsITelephonyProvider::CALL_STATE_HELD) > + == numConCalls) { nit: alignment @@ +1496,5 @@ > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) { > + // callheld = ONHOLD_ACTIVE if an active call already exists. > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > + SendCommand(RESPONSE_CIEV, CINDType::CALLHELD); > + } We should handle the 'else' case as well. To be more specific, I mean the case of callheld=2 (Call on hold, no active call). @@ +1561,5 @@ > + * A call becomes active because: > + * - user answers an incoming call, > + * - user dials a outgoing call and it is answered, or > + * - SLC is connected when a call is active. > + */ Personally I would rather put these comments before the first case (case nsITelephonyProvider::CALL_STATE_INCOMING) because it doesn't explain the following code but the code above. @@ +1570,3 @@ > break; > // User wants to add a held call to the conversation. > // The original connected call become a conference call here. Please remove the above two lines since they've been moved to downstairs.

Hi Eric, Thank you for your comment. (In reply to Eric Chou [:ericchou] [:echou] from comment #14) > Comment on attachment 8407453 [details] [diff] [review] > [Bluetooth] Fix the incorrect condition for reporting call held status when > call state changed. (v4) f=btian > > Review of attachment 8407453 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +1474,5 @@ > > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_NOACTIVE; > > + } else { > > + // Active call is placed on hold or active/held calls swapped > > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > > + } > > Question: there is a case which would not be handled but actually I don't > know if that would happen: > One active call + One held call =====> Two held calls As far as I know, it wouldn't happen. It could only be accomplished by two steps. 1) Merge two call to an active conference call. 2) Put the conference call on hold. > @@ +1496,5 @@ > > + if (FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) { > > + // callheld = ONHOLD_ACTIVE if an active call already exists. > > + sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE; > > + SendCommand(RESPONSE_CIEV, CINDType::CALLHELD); > > + } > > We should handle the 'else' case as well. To be more specific, I mean the > case of callheld=2 (Call on hold, no active call). You are right. It should sent +CIEV callheld=2 in that case. However, we can't tell whether there is only one held call or not when BT is just connected. It's because the HfpManager didn't handle the HandleCallStateChanged event when BT is disconnected, all call state would be disconnected at default. Fortunately, B2G wouldn't hit this case since the held call would be changed to active call by dialer. I think we should support this case if we figure out a better way to do it.

Comment on attachment 8410171 [details] [diff] [review] [Bluetooth] Fix the incorrect condition for reporting call held status when call state changed. (v5) f=btian Review of attachment 8410171 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks.

Does this need checkin-needed?

Hi Andrew, (In reply to Andrew Overholt [:overholt] from comment #18) > Does this need checkin-needed? Jamin has pushed the patch to try. We'll check the patch into b2g-inbound tomorrow once it gets all green on try. Thanks for reminding.

It looks fine on try server and passed the test case TC_AG_TWC_BV_04_I. https://tbpl.mozilla.org/?tree=Try&rev=66a44d7e9f1c

Verified on Gaia d23e479e8a4ce0bc620acb2d7e2f82801aa4d0ea Gecko https://hg.mozilla.org/releases/mozilla-aurora/rev/36f67ce46855 BuildID 20140428000206 Version 30.0a2