1142390 - [PTS][Bluedroid][2.2][Flame-KK] HSP - TC_AG_OAC_BV_01_I Failed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Attachment: TC_AG_OAC_BV_01_I-FAIL.zip

### STR
Test case : TC_AG_OAC_BV_01_I started
	- SDP Service record for PTS: 'Headset HS' successfully registered
	- MTC: Creating and closing a service level connection with the IUT in order to create a pairing
	- MTC: TS_spp_disable_tester: SPP already disabled
	- Using SDP to determine IUT RFCOMM server channel number.
	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number is 2
	- ProfileDescriptorList: Headset Profile version retrieved successfully
	- AT: SPP connect succeeded
	- AT: SPP connection enabled
	- AT: Service Level Connection disabled
	- AT: SPP connect succeeded
	- AT: SPP connection enabled
	- MTC: SPP Connection established from IUT
	- AT: RING
	- Unexpected CM from CP_AT_MTC
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: INCONC
TC_AG_OAC_BV_01_I finished

Test case : TC_AG_OAC_BV_01_I started
	- SDP Service record for PTS: 'Headset HS' successfully registered
	- MTC: Creating and closing a service level connection with the IUT in order to create a pairing
	- MTC: TS_spp_disable_tester: SPP already disabled
	- Using SDP to determine IUT RFCOMM server channel number.
	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number is 2
	- ProfileDescriptorList: Headset Profile version retrieved successfully
	- AT: SPP connect succeeded
	- AT: SPP connection enabled
	- AT: Service Level Connection disabled
	- AT: SPP connect succeeded
	- AT: SPP connection enabled
	- MTC: SPP Connection established from IUT
	- HCI: Audio Connection enabled
	- HCI: Audio Connection disabled
	- FATAL ERROR (MTC): The IUT and PTS should, but do not have an Audio Connection
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: FAIL
TC_AG_OAC_BV_01_I finished

### Version
1. v18D
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-b2g37_v2_2-flame-kk-eng/2015/03/2015-03-10-16-25-04/

Comment 1

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

Attachment: TC_AG_OAC_BV_01_I-INCONC.zip

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Eric Chang [:ericcc] [:echang] from comment #0)
> Created attachment 8576450 [details]
> TC_AG_OAC_BV_01_I-FAIL.zip
> 
> ### STR
> Test case : TC_AG_OAC_BV_01_I started
> 	- SDP Service record for PTS: 'Headset HS' successfully registered
> 	- MTC: Creating and closing a service level connection with the IUT in
> order to create a pairing
> 	- MTC: TS_spp_disable_tester: SPP already disabled
> 	- Using SDP to determine IUT RFCOMM server channel number.
> 	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number
> is 2
> 	- ProfileDescriptorList: Headset Profile version retrieved successfully
> 	- AT: SPP connect succeeded
> 	- AT: SPP connection enabled
> 	- AT: Service Level Connection disabled
> 	- AT: SPP connect succeeded
> 	- AT: SPP connection enabled
> 	- MTC: SPP Connection established from IUT
> 	- AT: RING
> 	- Unexpected CM from CP_AT_MTC
> 	- AT: Service Level Connection disabled
> 	- MTC: Test case ended
> 	 -Final Verdict: INCONC
> TC_AG_OAC_BV_01_I finished
> 
> Test case : TC_AG_OAC_BV_01_I started
> 	- SDP Service record for PTS: 'Headset HS' successfully registered
> 	- MTC: Creating and closing a service level connection with the IUT in
> order to create a pairing
> 	- MTC: TS_spp_disable_tester: SPP already disabled
> 	- Using SDP to determine IUT RFCOMM server channel number.
> 	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number
> is 2
> 	- ProfileDescriptorList: Headset Profile version retrieved successfully
> 	- AT: SPP connect succeeded
> 	- AT: SPP connection enabled
> 	- AT: Service Level Connection disabled
> 	- AT: SPP connect succeeded
> 	- AT: SPP connection enabled
> 	- MTC: SPP Connection established from IUT
> 	- HCI: Audio Connection enabled
> 	- HCI: Audio Connection disabled
> 	- FATAL ERROR (MTC): The IUT and PTS should, but do not have an Audio
> Connection
> 	- AT: Service Level Connection disabled
> 	- MTC: Test case ended
> 	 -Final Verdict: FAIL
> TC_AG_OAC_BV_01_I finished
> 
> ### Version
> 1. v18D
> https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-
> b2g37_v2_2-flame-kk-eng/2015/03/2015-03-10-16-25-04/

AG_OAC test case actually make PTS sends AT_CKPD=200 to re-dial last number call, but it looks like RING command sent. It seems to me you're making an incoming call. Can you confirm the steps again?

Comment 3

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

I tried couple of times, PTS tried to redial -> INCONC, I made an outgoing/incoming call first(because the instruction suggests that) -> FAIL.. any idea?

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Eric,
I tried this test case and found extra 'OK' command was sent that leads 'INCONC'.

Test case : TC_AG_OAC_BV_01_I started
	- SDP Service record for PTS: 'Headset HS' successfully registered
	- Using SDP to determine IUT RFCOMM server channel number.
	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number is 2
	- ProfileDescriptorList: Headset Profile version retrieved successfully
	- AT: SPP connect succeeded
	- AT: SPP connection enabled
	- MTC: SPP Connection established from IUT
	- FATAL ERROR (AT): unexpected OK received
	- MTC received unexpected EXIT message from AT component
	- HCI: Audio Connection enabled
	- HCI: Audio Connection disabled
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: INCONC
TC_AG_OAC_BV_01_I finished

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

One extra 'OK' command was sent after "OK command replied AT+CKPD=200" and cause test case INCONC.
http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/049713f3b0ed/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp#l1014

Ben, it seems this "OK" reply added in the beginning. What exactly this explicitly command OK reply to? 

HSP doesn't handle call state indicator.

Comment 6

[Ben Tian (inactive)]

The "OK" response is for HFP BLDN/memory dial [1]. Let me check the flow for HSP.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp#1468

(In reply to Shawn Huang [:shawnjohnjr] from comment #5)
> One extra 'OK' command was sent after "OK command replied AT+CKPD=200" and
> cause test case INCONC.
> http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/049713f3b0ed/dom/
> bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp#l1014
> 
> Ben, it seems this "OK" reply added in the beginning. What exactly this
> explicitly command OK reply to? 
> 
> HSP doesn't handle call state indicator.

Comment 7

[Ben Tian (inactive)]

We may remove the BLDN part in |KeyPressedNotification| since HSP spec doesn't mention it. Our implementation refers to AOSP [2].

[2] http://androidxref.com/4.4.2_r2/xref/packages/apps/Bluetooth/src/com/android/bluetooth/hfp/HeadsetStateMachine.java#1772

(In reply to Ben Tian [:btian] from comment #6)
> The "OK" response is for HFP BLDN/memory dial [1]. Let me check the flow for
> HSP.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/hfp/
> BluetoothHfpManager.cpp#1468
> 
> (In reply to Shawn Huang [:shawnjohnjr] from comment #5)
> > One extra 'OK' command was sent after "OK command replied AT+CKPD=200" and
> > cause test case INCONC.
> > http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/049713f3b0ed/dom/
> > bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp#l1014
> > 
> > Ben, it seems this "OK" reply added in the beginning. What exactly this
> > explicitly command OK reply to? 
> > 
> > HSP doesn't handle call state indicator.

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v0)

The root cause of this bug is that BluetoothHfpManager treats the first AT+CKPD=200 as a 'CHUP' command. Please refer to Bug 860166 for more details.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8582998 [details] [diff] [review]
Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v0)

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

"The root cause of this bug is that BluetoothHfpManager treats the first AT+CKPD=200 as a 'CHUP' command. Please refer to Bug 860166 for more details". Do you mind provide your successful cfa and fail case cfa? I would like to confirm one thing.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1286,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  mFirstCKPD = true;
> +

I checked https://bugzilla.mozilla.org/show_bug.cgi?id=860166#c23, but I still don't understand why mFirstCKPD = true when RFCOMM socket connected. Speaking of 'outgoing audio connection' establishment, i found this is a bit weired. Can you explain why we set true here?

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8582998 [details] [diff] [review]
Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v0)

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1622,5 @@
>       * indicating the call is answered successfully.
>       */
>      NotifyDialer(NS_LITERAL_STRING("ATA"));
>    } else if (hasActiveCall) {
> +    if (!mFirstCKPD) {

why do you move this check move up?

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> Comment on attachment 8582998 [details] [diff] [review]
> Add a flag to identify if the CKPD is the very first AT+CKPD=200 for
> Bluetooth HSP handling. (v0)
> 
> Review of attachment 8582998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> "The root cause of this bug is that BluetoothHfpManager treats the first
> AT+CKPD=200 as a 'CHUP' command. Please refer to Bug 860166 for more
> details". Do you mind provide your successful cfa and fail case cfa? I would
> like to confirm one thing.
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1286,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  mFirstCKPD = true;
> > +
> 
> I checked https://bugzilla.mozilla.org/show_bug.cgi?id=860166#c23, but I
> still don't understand why mFirstCKPD = true when RFCOMM socket connected.
> Speaking of 'outgoing audio connection' establishment, i found this is a bit
> weired. Can you explain why we set true here?

I revisited bug 860166, I think adding mFirstCKPD should resolve the bug somehow.

Comment 12

[Jamin Liu [:jaliu][:Jamin]]

Attachment: HSP_Audio_connection_establishment.png

Bluetooth HSP spec. - Figure 4.3: Audio connection establishment

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> Comment on attachment 8582998 [details] [diff] [review]
> Add a flag to identify if the CKPD is the very first AT+CKPD=200 for
> Bluetooth HSP handling. (v0)
> 
> Review of attachment 8582998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> "The root cause of this bug is that BluetoothHfpManager treats the first
> AT+CKPD=200 as a 'CHUP' command. Please refer to Bug 860166 for more
> details". Do you mind provide your successful cfa and fail case cfa? I would
> like to confirm one thing.
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1286,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  mFirstCKPD = true;
> > +
> 
> I checked https://bugzilla.mozilla.org/show_bug.cgi?id=860166#c23, but I
> still don't understand why mFirstCKPD = true when RFCOMM socket connected.
> Speaking of 'outgoing audio connection' establishment, i found this is a bit
> weired. Can you explain why we set true here?

According to Bluetooth HSP spec 4.2 & 4.3, if the connection establishment is initiated by pressing the “headset button”, the HS shall send the AT+CKPD=200 command to the AG.
As shown in Figure 4.3 #Attachment 8583692 [details], the SCO link connection may be set up prior to receiving the
AT+CKPD=200 command. In this case, we should not treat this 'AT+CKPD=200' as 'CHUP' command.

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> Comment on attachment 8582998 [details] [diff] [review]
> Add a flag to identify if the CKPD is the very first AT+CKPD=200 for
> Bluetooth HSP handling. (v0)
> 
> Review of attachment 8582998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1622,5 @@
> >       * indicating the call is answered successfully.
> >       */
> >      NotifyDialer(NS_LITERAL_STRING("ATA"));
> >    } else if (hasActiveCall) {
> > +    if (!mFirstCKPD) {
> 
> why do you move this check move up?

Thank you for pointing it out. :)
I'll revise it on next patch.

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v1)

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8583694 [details] [diff] [review]
Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v1)

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +227,5 @@
>  
>  void
>  BluetoothHfpManager::Reset()
>  {
> +  mFirstCKPD = true;

This should be |false|, right?

@@ +1637,5 @@
> +       * command from the HS.
> +       * Since FxOS would initiate a SCO connection before receiving the
> +       * AT+CKPD=200, we should ignore the first AT+CKPD=200.
> +       */
> +       // Ignore this key press

nit: merge this one line comment into previous comment section.

Comment 17

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v2)

- revise previous patch based on comment 16.

Comment 18

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8583717 [details] [diff] [review]
Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v2)

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

Hi Jamin,
We'd better to run some test cases to make sure we won't cause any side effect on HFP. Let's run HFP TWC test cases before get this patch landed.

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Request 2.2? due to this blocks certification.

Comment 20

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (final) Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v3), r=shuang

- Add reviewer's name to commit message
- convert to hg format

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

Pushed to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09ac9b7811c7

Comment 22

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #18)
> Hi Jamin,
> We'd better to run some test cases to make sure we won't cause any side
> effect on HFP. Let's run HFP TWC test cases before get this patch landed.

Sure.

I've run these PTS tests with #attachment 8583717 [details] [diff] [review].
The results are fine.
> TC_AG_OAC_BV_01_I - PASS
> TC_AG_TWC_BV_01_I - PASS
> TC_AG_TWC_BV_02_I - PASS
> TC_AG_TWC_BV_03_I - PASS
> TC_AG_TWC_BV_04_I - PASS
> TC_AG_TWC_BV_05_I - PASS

Comment 23

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8584367 [details] [diff] [review]
(final) Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v3), r=shuang

> [Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 1139349 - [BlueDroid][PTS][Meta] Bluetooth PTS test (v2.2 in aurora) 
(FxOS 2.2 can't pass the PTS certification suit 6.0.1.)

> User impact if declined: 
The end product can't pass Bluetooth HSP certification.

> Testing completed: 
- PTS, TC_AG_OAC_BV_01_I
- PTS, TC_AG_TWC_BV_01_I
- PTS, TC_AG_TWC_BV_02_I
- PTS, TC_AG_TWC_BV_03_I
- PTS, TC_AG_TWC_BV_04_I
- PTS, TC_AG_TWC_BV_05_I

> Risk to taking this patch (and alternatives if risky): 
In Gecko, HFP(Hand-free Profile) and HSP(Headset Profile) use the same profile manager. We have to be careful when we try to fix one of these profiles.

> String or UUID changes made by this patch:
no

Comment 24

[Jamin Liu [:jaliu][:Jamin]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5dee4f3e13bb

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2a017c7572db