1161888 - [Bluetooth][PTS][2.2][L][FC] TC_AG_ACR_BV_01_I failed

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Teri Wen [:twen]]

Attachment: TC_AG_ACR_BV_01_I_2015_05_05_14_44_14.zip

##STR
Test case : TC_AG_ACR_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
	- MTC: SPP Connection established from IUT
	- AT: RING
	- MTC: MTC: Call is connected
	- HCI: Audio Connection enabled
	- MTC: Call disabled by tester
	- MTC: Disable call/audio connection successfully
	- FATAL ERROR (MTC): The IUT and PTS have an Audio Connection, when they should not
	- HCI: Audio Connection disabled
	- AT: Service Level Connection disabled
	- MTC: Test case ended
Final Verdict:FAIL
TC_AG_ACR_BV_01_I finished

## Version
Build Number: 20150428041949

Comment 1

[Teri Wen [:twen]]

#Updated Build Version
Gaia-Rev        9f6b1b9082662ba2c14168fc66bb02b4df3141e5
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e79c19bf19bf
Build-ID        20150428002500
Version         37.0
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150428.041949
FW-Date         Tue Apr 28 04:20:07 EDT 2015
Bootloader      HHZ12d

Comment 2

[Will Wang [:WillWang]]

Attachment: 0506-hsp-ACR-htc-pass.zip

Attach PASS log for reference

Comment 3

[Will Wang [:WillWang]]

Attachment: hsp-ACR-fail-logcat.log

Root cause found:

Because of [1], NotifyDialer(NS_LITERAL_STRING("CHUP")) in the |BluetoothHfpManager| was not executed when getting AT+CKPD=200, so the "Audio Connection" was not released as Verdict description mentioned[2]


[1] 05-06 19:59:10.314 13370 13370 I GeckoBluetooth: KeyPressedNotification: HSP: mFirstCKPD is true
[2] FATAL ERROR (MTC): The IUT and PTS have an Audio Connection, when they should not

Comment 4

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD.patch

This PTS failure was due to side effect of bug 1142390, which used a first CKPD flag to solve a PTS HSP TC_AG_OAC_BV_01_I issue.

The side effect will cause a key pressed event (which aim to disconnect audio connection) was ignored, so PTS failed [1].

Per offline discussion with Jamin, so we:
1. remove first CKPD flag => to solve this bug 1161888(TC_AG_ACR_BV_01_I) ,and
2. do not set another |mDialingRequestProcessed| flag to avoid the redundant HFP_AT_RESPONSE_OK [2] => to solve bug 1142390 (TC_AG_OAC_BV_01_I)

After examination, the attached patch can pass following related PTS test cases:
HFP: 
TC_AG_OCL_BV_01_I, TC_AG_OCL_BV_02_I (re-dial related [3])

HSP: 
TC_AG_IAC_BV_01_I
TC_AG_OAC_BV_01_I
TC_AG_ACR_BV_01_I
TC_AG_ACR_BV_02_I
TC_AG_ACT_BV_01_I
TC_AG_ACT_BV_02_I
(all our current HSP test cases)


Hi Shawn, 
May you help to review this patch?
thanks!


[1] FATAL ERROR (MTC): The IUT and PTS have an Audio Connection, when they should not
[2] Bug 1142390 comment 4 ~ comment 7
[3] please refer to HFP spec v1.6, section 4.20 "Last Number Re-Dial from the HF"

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8604084 [details] [diff] [review]
0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD.patch

Jamin, please help feedback on Will's patch. Also we can resolve bug 1161992 after landing this patch, right?

Comment 7

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

Comment on attachment 8604084 [details] [diff] [review]
0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD.patch

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

Thank Will for fixing the bug.
Good catch.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1684,3 @@
>         */
>        NotifyDialer(NS_LITERAL_STRING("CHUP"));
>      }

We might consider to move this statement "mFirstCKPD = false;" to the end of function.
Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=860166#c23.

This this case, we can handle "4.6.1 Audio Connection Transfer from AG to HS" even if SCO link is established before KeyPressedNotification() does.

However, I don't have a strong opinion on this though.
Since the priority objective is to pass the PTS set, please feel free to land this patch without mFirstCKPD.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h
@@ +213,3 @@
>    int mCurrentVgm;
>    bool mReceiveVgsFlag;
>    bool mDialingRequestProcessed;

Since this variable wouldn't be set even when a dialing request is made by HSP.
Please leave a comment or revise the naming to let people know it's only used by HFP.
Thank you.

Comment 8

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

Hi Will,
Please revise the patch first based on feedback and will take a look soon.

Comment 9

[Will Wang [:WillWang]]

(In reply to Jamin Liu [:jaliu] from comment #7)
> Comment on attachment 8604084 [details] [diff] [review]
> 0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD.patch
> 
> Review of attachment 8604084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank Will for fixing the bug.
> Good catch.
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1684,3 @@
> >         */
> >        NotifyDialer(NS_LITERAL_STRING("CHUP"));
> >      }
> 
> We might consider to move this statement "mFirstCKPD = false;" to the end of
> function.
> Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=860166#c23.
> 
> This this case, we can handle "4.6.1 Audio Connection Transfer from AG to
> HS" even if SCO link is established before KeyPressedNotification() does.
> 
Thank you for pointing out the possibility!
To confirm the timing is identical with expectation,
I made several PTS HSP TC_AG_ACT_BV_01_I tests for "4.6.1 Audio Connection Transfer from AG to HS",
all logs show that FxOS behavior can meet the spec:
|KeyPressedNotification| first(line 834), then SCO connecting(line 838) and connected(line 839). [1]

> However, I don't have a strong opinion on this though.
> Since the priority objective is to pass the PTS set, please feel free to
> land this patch without mFirstCKPD.
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h
> @@ +213,3 @@
> >    int mCurrentVgm;
> >    bool mReceiveVgsFlag;
> >    bool mDialingRequestProcessed;
> 
> Since this variable wouldn't be set even when a dialing request is made by
> HSP.
> Please leave a comment or revise the naming to let people know it's only
> used by HFP.
> Thank you.
OK, I will add comment and made another patch,
thanks for your feedback :)



[1]

 813 05-09 19:21:09.230 I/GeckoBluetooth(29670): ConnectionStateNotification: state 2
 814 05-09 19:21:09.230 I/GeckoBluetooth(29670): ConnectionStateNotification: state 3
 834 05-09 19:21:09.390 I/GeckoBluetooth(29670): KeyPressedNotification: ConnectSco
 838 05-09 19:21:09.480 I/GeckoBluetooth(29670): AudioStateNotification: state 1    
 839 05-09 19:21:09.480 I/GeckoBluetooth(29670): AudioStateNotification: state 2

Comment 10

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD-v2.patch

Hi Shawn

Could you help to review this patch?
Thanks!

Comment 11

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

Comment on attachment 8605044 [details] [diff] [review]
0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD-v2.patch

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h
@@ +212,4 @@
>    int mCurrentVgs;
>    int mCurrentVgm;
>    bool mReceiveVgsFlag;
> +  bool mDialingRequestProcessed; // This flag is for HFP only, not for HSP.

nit:Please move comments above.

Comment 12

[Will Wang [:WillWang]]

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

Comment 13

[Will Wang [:WillWang]]

Attachment: 0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD-v3.patch

This patch(v3) is basically identical with [1] which is r+, except for a slight difference in comment format.

====

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1142390
User impact if declined: Bluetooth certification(PTS test) will fail.
Testing completed: Several related PTS tests are tested and PASS.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None


[1] 8605044: 0001-Bug-1161888-remove-first-CKPD-flag-and-do-not-set-mD-v2.patch

Comment 14

[Pulsebot]

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

Comment 15

[Josh Cheng [:josh]]

Will,
My bad, please also raise PR to central. Thanks

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 17

[Josh Cheng [:josh]]

Hi Teri,
Could you help to verify the patch on mc?
Thanks!

Comment 18

[Teri Wen [:twen]]

Verified on V3.0. All HSP test cases passed. 

HSP: 
TC_AG_IAC_BV_01_I
TC_AG_OAC_BV_01_I
TC_AG_ACR_BV_01_I
TC_AG_ACR_BV_02_I
TC_AG_ACT_BV_01_I
TC_AG_ACT_BV_02_I

Gaia-Rev        afea16de7a76c3b6d15c35fb4c37bac71c8ddc6a
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/35918b0441b4
Build-ID        20150517160201
Version         41.0a1
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150517.194053
FW-Date         Sun May 17 19:41:09 EDT 2015
Bootloader      HHZ12f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/fe3cb392cc58

Comment 20

[Teri Wen [:twen]]

Verified HSP test cases again on today's latest V2.2.

TC_AG_OAC_BV_01_I   ERROR
TC_AG_ACR_BV_01_I   FAIL


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
	- Warning: unknown AT command: ERROR
	- FATAL ERROR (MTC): Audio Connection established from IUT timed out
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: INCONC
TC_AG_OAC_BV_01_I finished


Test case : TC_AG_ACR_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: RING
	- MTC: MTC: Call is connected
	- HCI: Audio Connection enabled
	- MTC: Call disabled by tester
	- MTC: Disable call/audio connection successfully
	- FATAL ERROR (MTC): The IUT and PTS have an Audio Connection, when they should not
	- HCI: Audio Connection disabled
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: FAIL
TC_AG_ACR_BV_01_I finished

Comment 21

[Teri Wen [:twen]]

All other cases passed on V2.2.

##Build info
Gaia-Rev        5212c658a651e04d6d84dfc1bce06b499c0d0d96
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9e9e0d0f45f0
Build-ID        20150518162501
Version         37.0
Device-Name     hammerhead
FW-Release      5.1
FW-Incremental  eng.cltbld.20150518.211334
FW-Date         Mon May 18 21:13:53 EDT 2015
Bootloader      HHZ12f

Comment 22

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

Will try to verify tomorrow, as the patch might not yet merged into PVT build. ni myself.

Comment 23

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

Test case : TC_AG_ACR_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: RING
	- MTC: MTC: Call is connected
	- HCI: Audio Connection enabled
	- MTC: Call disabled by tester
	- MTC: Disable call/audio connection successfully
	- HCI: Audio Connection disabled
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: PASS
TC_AG_ACR_BV_01_I finished

Comment 24

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

Build ID               20150520162503
Gaia Revision          bc42fbc12d622bffd7e8afcb8d56f8a1d9773c60
Gaia Date              2015-05-20 22:32:56
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8663598512f7
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150520.193900
Firmware Date          Wed May 20 19:39:15 EDT 2015
Bootloader             HHZ12f

Comment 25

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

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
	- MTC: Audio Connection enabled by IUT
	- HCI: Audio Connection disabled
	- MTC: Audio Connection disabled by IUT
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: PASS
TC_AG_OAC_BV_01_I finished