993288 - [Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TWC_BV_01_I, TC_AG_ECS_BV_03_I

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Attachment: TC_AG_TWC_BV_01_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_TWC_BV_04_I

### Expected
Pass 

### Actual

Comment 1

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

Test case : TC_AG_TWC_BV_01_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

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

Attachment: TC_AG_TWC_BV_02_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_TWC_BV_02_I

### Expected
Pass 

### Actual
Test case : TC_AG_TWC_BV_02_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 :

Comment 3

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

Attachment: TC_AG_TWC_BV_03_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_TWC_BV_03_I

### Expected
Pass 

### Actual
Test case : TC_AG_TWC_BV_03_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 4

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

Root cause for these three cases:
1) TWC_BV_01_I: bluedroid won't send +CCWA if call state is BTHF_CALL_STATE_WAITING.
2) TWC_BV_02_I: calls_handler.js in gaia will not answer the incoming call while handling CHLD=1.
3) TWC_BV_03_I: When handling AT+CHLD=2, we will send +CIEV indicates the intermidiate status (active:2, held:0) which is not a valid status PTS expected.

Comment 5

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

Change bug title.
TC_AG_TWC_BV_02 and 03 have been seperated to new bugs.
Add TC_AG_ECS_BV_03_I since it has the same cause as TC_AG_TWC_BV_01.

Comment 6

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

Attachment: Patch1(v1) Bug 993288: use BTHF_CALL_STATE_INCOMING for both incoming and waiting while calling bluedroid phone_state_change

Although bluedroid defines BTHF_CALL_STATE_WAITING status, it treats this state as an invalid state when handling phone state change.
http://androidxref.com/4.2_r1/xref/external/bluetooth/bluedroid/btif/src/btif_hf.c#1037
Hence, no +CCWA will be sent for these test cases.
Change BTHF_CALL_STATE_WAITING to BTHF_CALL_STATE_INCOMING for phone_state_change usage only.

Comment 7

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

Comment on attachment 8408078 [details] [diff] [review]
Patch1(v1) Bug 993288: use BTHF_CALL_STATE_INCOMING for both incoming and waiting while calling bluedroid phone_state_change

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

This patch makes sense to me and it can solve the issue. However I've got a question: why do we insist converting BTHF_CALL_STATE_INCOMING to BTHF_CALL_STATE_WAITING in function ConvertToBthfCallState()? Based on current Gecko implementation, bluedroid and your patch, BTHF_CALL_STATE_WAITING seems not to be used anywhere.

Comment 8

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

Attachment: Patch1(v2) Bug 993288: Separate BTHF_CALL_STATE_INCOMING and BTHF_CALL_STATE_WAITING for CLCC response only

Hi Eric,

BTHF_CALL_STATE_WAITING is needed for clcc_response().
According to HFP spec, we have to reply different status code for incoming and waiting.

After checking again cind_response() in bluedroid, we should also use BTHF_CALL_STATE_INCOMING only since we should reply callsetup=1 for both incoming and waiting.

I have updated the patch, please review it.

Thanks,
Jocelyn

Comment 9

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

Comment on attachment 8409517 [details] [diff] [review]
Patch1(v2) Bug 993288: Separate BTHF_CALL_STATE_INCOMING and BTHF_CALL_STATE_WAITING for CLCC response only

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

Hm, r+ because I'm ok with the patch and it would fix the problem. However I don't really like to make the signature more complicated just because of a special case, which is CLCC here. Since ConvertToBthfCallState is only called at two different spots, how about doing 1-on-1 converting in this function and move

  if (FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)

into BluetoothHfpManager::SendCLCC()?

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1072,5 @@
> +                BTHF_CALL_STATE_WAITING :
> +                BTHF_CALL_STATE_INCOMING;
> +    } else {
> +      state = BTHF_CALL_STATE_INCOMING;
> +    }  

nit: trailing whitespaces

Comment 10

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

Attachment: Patch1(v3) Bug 993288: Separate BTHF_CALL_STATE_INCOMING and BTHF_CALL_STATE_WAITING for CLCC response only

Hi Eric,

Thanks for your comment.
I agreed it makes sense to move the special case into SendCLCC in order to keep the function signature simple.
Please find the attachment for the updated version.

Thanks,
Jocelyn

Comment 11

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

Comment on attachment 8409541 [details] [diff] [review]
Patch1(v3) Bug 993288: Separate BTHF_CALL_STATE_INCOMING and BTHF_CALL_STATE_WAITING for CLCC response only

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

r=me with nits fixed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +962,5 @@
>    if (mPhoneType == PhoneType::CDMA && aIndex == 1 && aCall.IsActive()) {
>      callState = (mCdmaSecondCall.IsActive()) ? BTHF_CALL_STATE_HELD :
>                                                 BTHF_CALL_STATE_ACTIVE;
>    }
> +  

nit: trailing whitespace

@@ +967,5 @@
> +  if (callState == BTHF_CALL_STATE_INCOMING) {
> +    callState = (FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) ?
> +                BTHF_CALL_STATE_WAITING :
> +                BTHF_CALL_STATE_INCOMING;
> +  }

nit: personally I would do

  if (callState == BTHF_CALL_STATE_INCOMING &&
      FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) {
    callState = BTHF_CALL_STATE_WAITING;
  }

Comment 12

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

Attachment: [Final] Patch1 Bug 993288: Separate BTHF_CALL_STATE_INCOMING and BTHF_CALL_STATE_WAITING for CLCC response only

Comment 13

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

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc482bcf17e0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[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/