993289 - [Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TCA_BV05_I

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Attachment: TC_AG_TCA_BV05_I.zip

### ENV
Nexus 4/Bluedroid
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-aurora-nexus-4/2014/03/2014-03-25-16-02-01/


### STR
1. PTS 5.0
2. TC_AG_TCA_BV05_I

### Expected
Pass 

### Actual

Comment 1

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

Test case : TC_AG_TCA_BV_05_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, 
	- AT: SPP connect succeeded
	- AT: WARNING: IUT does not report support for AT+CHLD=3 in its +CHLD response, but TSPC_HFP_2_12c is set to TRUE
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- AT: RING
	- AT: Received +CLIP: "0986143883",129
	- HCI: Audio Connection enabled
	- AT: Incoming call established
	- FATAL ERROR (AT): +CIEV(callheld=1) received when there is no second call process
	- MTC received unexpected EXIT message from AT component
	- HCI: Audio Connection disabled
	- AT: SPP disconnect succeeded
	- MTC: Test case ended
Final Verdict : Inconclusive

Comment 2

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

Depends on Bug993288 for +CCWA

Comment 3

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

Attachment: Patch(v1) Bug 993289: Fix the logic while updating call setup state

TC_AG_TCA_BV05:
   1) One active call is ongoing, second one is incoming.
   2) end the active call
   3) end the second call
This case failed because we set and notify HF callsetup=0 in step2.
Attached patch only set callsetup=0 if no other call in the call array is in callsetup. 
Also move callsetup update to HandleCallStateChange() since it should be executed whenever a call state change.

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8408868 [details] [diff] [review]
Patch(v1) Bug 993289: Fix the logic while updating call setup state

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

The patch may result in build break since |UpdatePhoneCIND| still access the removed local variable |callState| to print log. Also I think the callsetup state logic can be simplified as comment below.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ -994,5 @@
>  void
>  BluetoothHfpManager::UpdatePhoneCIND(uint32_t aCallIndex, bool aSend)
>  {
> -  // Update callsetup state
> -  uint16_t callState = mCurrentCallArray[aCallIndex].mState;

The variable callState is used to print log below. Please replace callState below with mCurrentCallArray[aCallIndex].mState.

@@ +1123,5 @@
>      mCurrentCallArray[aCallIndex].mType = BTHF_CALL_ADDRTYPE_INTERNATIONAL;
>    }
>  
> +  // Update callsetup state
> +  switch (aCallState) {

I suggest to simplify the logic as following.

bool IsSetupCall = (callstate == INCOMING ||
                    callstate == ALERTING ||
                    callstate == DIALING);
bool RemainSetupCall = (!FindFirstCall(INCOMING) ||
                        !FindFirstCall(ALERTING) ||
                        !FindFirstCall(DIALING);

if (IsSetupCall) {
  mCallSetupState = callstate;
} else if (!RemainSetupCall) {
  mCallSetupState = DISCONNECTED.
}

@@ +1128,5 @@
> +    // This call is not in call setup
> +    case nsITelephonyProvider::CALL_STATE_CONNECTED:
> +    case nsITelephonyProvider::CALL_STATE_HELD:
> +    case nsITelephonyProvider::CALL_STATE_DISCONNECTED:
> +      if (FindFirstCall(nsITelephonyProvider::CALL_STATE_INCOMING) == 0 &&

nit: !FindFirstCall(nsITelephonyProvider::CALL_STATE_INCOMING) is succinter than FindFirstCall(nsITelephonyProvider::CALL_STATE_INCOMING) == 0.

Comment 5

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #4) 
> @@ +1123,5 @@
> >      mCurrentCallArray[aCallIndex].mType = BTHF_CALL_ADDRTYPE_INTERNATIONAL;
> >    }
> >  
> > +  // Update callsetup state
> > +  switch (aCallState) {
> 
> I suggest to simplify the logic as following.
> 
> bool IsSetupCall = (callstate == INCOMING ||
>                     callstate == ALERTING ||
>                     callstate == DIALING);
 bool RemainSetupCall = (!FindFirstCall(INCOMING) &&
                         !FindFirstCall(ALERTING) &&
                         !FindFirstCall(DIALING);

Correction. Should be && here instead of ||.

Comment 6

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #5)
> bool IsSetupCall = (callstate == INCOMING ||
>                     callstate == ALERTING ||
>                     callstate == DIALING);
  bool RemainSetupCall = (FindFirstCall(INCOMING) ||
                          FindFirstCall(ALERTING) ||
                          FindFirstCall(DIALING);

Oops, this one is the really correct logic.

Comment 7

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

Attachment: Patch(v2) Bug 993289: Fix the logic while updating call setup state

Hi Ben,

Thanks a lot for your comment, I have updated the patch according to your comment.
Please let me know if you have any feedbacks for patch v2.

Thanks,
Jocelyn

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8409548 [details] [diff] [review]
Patch(v2) Bug 993289: Fix the logic while updating call setup state

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

f=me. Thanks Jocelyn.

Comment 9

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8409548 [details] [diff] [review]
Patch(v2) Bug 993289: Fix the logic while updating call setup state

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +997,5 @@
>      sBluetoothHfpInterface->at_response(aResponseCode, 0));
>  }
>  
>  void
>  BluetoothHfpManager::UpdatePhoneCIND(uint32_t aCallIndex, bool aSend)

aSend seems not to be used. Can we remove it?

Comment 10

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

Attachment: [final] Patch1 - Bug 993289: Fix the logic while updating call setup state

try: https://tbpl.mozilla.org/?tree=Try&rev=ceb92f0cb329

Comment 11

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

aSend will be removed by bug993286.

Comment 12

[Ben Tian (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/72ad241c645c

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72ad241c645c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec64a6046a31

Comment 15

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

### Build for verification
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-aurora-nexus-4/2014/04/2014-04-28-00-02-06/