Closed Bug 1142390 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S9 (3apr)
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: ericcc, Assigned: jaliu)

References

Details

Attachments

(4 files, 3 obsolete files)

TC_AG_OAC_BV_01_I-FAIL.zip
134.36 KB, application/zip
Details
TC_AG_OAC_BV_01_I-INCONC.zip
42.02 KB, application/zip
Details
HSP_Audio_connection_establishment.png
32.78 KB, image/png
Details
(final) Add a flag to identify if the CKPD is the very first AT+CKPD=200 for Bluetooth HSP handling. (v3), r=shuang
3.80 KB, patch
bajaj
: approval-mozilla-b2g37+
Details | Diff | Splinter Review 
### 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/
(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?
Flags: needinfo?(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?
Flags: needinfo?(echang)
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
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.
Flags: needinfo?(btian)
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.
Flags: needinfo?(btian)
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.
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.
Attachment #8582998 - Flags: feedback?(shuang)
Assignee: shuang → jaliu
Status: NEW → ASSIGNED
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 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?
Attachment #8582998 - Flags: feedback?(shuang) → feedback-
(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.
Bluetooth HSP spec. - Figure 4.3: Audio connection establishment
(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.
(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 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.
Attachment #8583694 - Flags: review?(shuang)
- revise previous patch based on comment 16.
Attachment #8583694 - Attachment is obsolete: true
Attachment #8583717 - Flags: review?(shuang)
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.
Attachment #8583717 - Flags: review?(shuang) → review+
Request 2.2? due to this blocks certification.
blocking-b2g: --- → 2.2?
- Add reviewer's name to commit message
- convert to hg format
Attachment #8583717 - Attachment is obsolete: true
(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 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
Attachment #8584367 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/5dee4f3e13bb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S9 (3apr)
blocking-b2g: 2.2? → 2.2+
Attachment #8584367 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
See Also: → 1161889
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: