Closed Bug 993286 Opened 6 years ago Closed 6 years ago

[Bluetooth][Certification][PTS][Bluedroid][1.4] HFP TC_AG_TWC_BV_04_I

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
1.4 S6 (25apr)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: echang, Assigned: ben.tian)

References

Details

Attachments

(4 files, 3 obsolete files)

Attached file TC_AG_TWC_BV_04_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
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, 
	- Using SDP to determine IUT RFCOMM server channel number.
	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number is 3
	- 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
	- HCI: Audio Connection enabled
	- MTC: Audio Connection enabled by tester
	- FATAL ERROR (AT): The response to the following AT command has timed out: AT+CHLD=3
	- MTC received unexpected EXIT message from AT component
	- FATAL ERROR (AT): PTS Error: congestion: TS_AT_block called when OK pending
	- HCI: Audio Connection disabled
	- AT: Call terminated
	- AT: SPP disconnect succeeded
	- MTC: Test case ended
Final Verdict : Inconclusive
Blocks: 986297
Changes:
- Keep mCurrentCallArray and mCallSetupState up-to-date for correct CIND response during SLC connection.
- Enumerate calls after SLC is connected since bluedroid accepts phone state change only when SLC is connected.
- Change |IsConnected| from RFCOMM-connected to SLC-connected.
Assignee: nobody → btian
Attachment #8406694 - Flags: review?(echou)
Require bug 993280's patch to respond OK for AT+CHLD.
Depends on: 993280
Comment on attachment 8406694 [details] [diff] [review]
Patch 1 (v1): Support AT+CHLD=3 on bluedroid

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

Mostly looks good, though I have two questions. Please see my comments below. Thanks!

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1054,5 @@
>  {
> +  /**
> +   * Keep mCurrentCallArray and mCallSetupState up-to-date
> +   * for correct CIND response during SLC connection.
> +   */

Can we reuse mListener->EnumerateCalls() in ProcessAtCind() to avoid updating the call array since startup?

@@ +1076,5 @@
> +      aCallState == nsITelephonyProvider::CALL_STATE_HELD) {
> +    mCallSetupState = nsITelephonyProvider::CALL_STATE_DISCONNECTED;
> +  } else {
> +    mCallSetupState = aCallState;
> +  }

Can we do only |mCallSetupState = aCallState| here and move this part of logic into ConvertToBthfCallState()?
Attachment #8406694 - Flags: review?(echou)
(In reply to Eric Chou [:ericchou] [:echou] from comment #3)
> Can we reuse mListener->EnumerateCalls() in ProcessAtCind() to avoid
> updating the call array since startup?
Update:
In this case we need to call mListener->EnumerateCalls() twice: 1st to get correct CIND, and 2nd to update bluedroid phone state. After call enumeration completes, we require 1) a flag to decide whether to respond CIND or update bluedrdroid phone state, and 2) build flag in BluetoothRilListener to call bluedroid function only.

I'm also thinking to update only necessary phone states (numActive, numHeld, and callsetup state) rather than all calls information when SLC is disconnected.
blocking-b2g: --- → 1.4?
The patch is rebased on the patches of bug 993288 and bug 993289.
Attachment #8406694 - Attachment is obsolete: true
Attachment #8409611 - Flags: review?(echou)
Attachment #8409612 - Flags: review?(echou)
Attachment #8409612 - Attachment description: Patch 1/3 (v2): Support AT+CHLD=3 to merge calls → Patch 2/3 (v2): Support AT+CHLD=3 to merge calls
Attachment #8409613 - Flags: review?(echou)
blocking-b2g: 1.4? → 1.4+
Comment on attachment 8409611 [details] [diff] [review]
Patch 1/3 (v1): Get call setup state from call array

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

r=me with nit addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1007,5 @@
>    NS_ENSURE_TRUE_VOID(sBluetoothHfpInterface);
>  
>    int numActive = GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_CONNECTED);
>    int numHeld = GetNumberOfCalls(nsITelephonyProvider::CALL_STATE_HELD);
> +  bthf_call_state_t bthfCallState = ConvertToBthfCallState(GetCallSetupState());

Maybe we could rename bthfCallState to something like callSetupState? Seems to be more reasonable.
Attachment #8409611 - Flags: review?(echou) → review+
Comment on attachment 8409612 [details] [diff] [review]
Patch 2/3 (v2): Support AT+CHLD=3 to merge calls

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

r=me with question answered.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1123,5 @@
>  
> +  // Return if SLC is disconnected
> +  if (!IsConnected()) {
> +    return;
> +  }

Question: should we move this if-block to the place which is before UpdatePhoneCIND(), so that we can refresh the entire 'call' object which is stored in mCurrentCallArray?
Attachment #8409612 - Flags: review?(echou) → review+
Comment on attachment 8409613 [details] [diff] [review]
Patch 3/3 (v1): Add Call.Set() function

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

Thanks for cleaning this up!
Attachment #8409613 - Flags: review?(echou) → review+
(In reply to Eric Chou [:ericchou] [:echou] from comment #10)
> Question: should we move this if-block to the place which is before
> UpdatePhoneCIND(), so that we can refresh the entire 'call' object which is
> stored in mCurrentCallArray?

Yes we can. But only call states are really required to respond CIND before SLC is connected. To save unnecessary update on other call information (number, in/out, and type), I choose to leave these update when SLC is connected.
Rebase on revised patch 1/3.
Attachment #8409612 - Attachment is obsolete: true
Depends on: 1000961
No longer depends on: 1000961
You need to log in before you can comment on or make changes to this bug.