869306 - [Bluetooth][Certification]HFP PTS TC_AG_ECS_BV_01_I failed (CLCC dir value problem)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Test case : TC_AG_ECS_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, 
	- Using SDP to determine IUT RFCOMM server channel number.
	- ProtocolDescriptorList: IUT reports that its RFCOMM server channel number is 10
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- AT: SPP connect succeeded
	- HCI: Audio Connection enabled
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- MTC: AT+CLCC
	- AT: +CLCC received: +CLCC: 1,0,1,0,0,"0988095164",129
	- AT: +CLCC received: +CLCC: 2,0,0,0,0,"0978500925",129
	- FATAL ERROR (MTC): In +CLCC, call with idx=1 contains a dir value which does not indicate an incoming call (should be 1)

Comment 1

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

Good case shall be:
Test case : TC_AG_ECS_BV_01_I started	
	- MTC: AT+CLCC
	- AT: +CLCC received: +CLCC: 1,1,1,0,0,"0958028192",129
	- AT: +CLCC received: +CLCC: 2,1,0,0,0,"0978500925",129
	- MTC: +CLCC responses correctly indicate one active and one held call in the status field.

Comment 2

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

This test based on gecko m-c and gaia master.

Comment 3

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

Problem is in +CLCC AT command dir value shall be  dir= 0 (outgoing), 1 (incoming). As test resuls showed, dir value is invalid (outgoing), this could be incorrect.

Comment 4

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

I will test with v.1.0.1 later after meta bugs.

Comment 5

[Kai-Chih Hu [:khu]]

May I know if this is a must to pass Bluetooth certification? 
If this is a Bluetooth certification blocker, it should be marked as tef+, IMO. marked it as tef? now.

Comment 6

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

Yes. It's certification bug. And I will update this bug by the end of today.

Comment 7

[Kai-Chih Hu [:khu]]

Cool, thank you, Shawn.

Comment 8

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

There is a case, we cannot easily cover here. If we did not connect headset yet, call state enters into call held, we can hardly know if it's incoming or outgoing call. Since we can only get call status, and call index value.
I think telephony shall be able to provide interface, so we can easily get call direction.

Comment 9

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

Query RIL team, see if we can get call direction directly, instead of doing tricky change for direction. Let Hsin-Yi comment the feasibility to provide extra parameter for call direction.

Comment 10

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to Shawn Huang from comment #9)
> Query RIL team, see if we can get call direction directly, instead of doing
> tricky change for direction. Let Hsin-Yi comment the feasibility to provide
> extra parameter for call direction.

Yes, I will provide a patch for RIL change (add .direction attribute) tomorrow!

Comment 11

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

Thanks. Once RIL change applied, this bug shall be easily fixed.

Comment 12

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: RIL - add 'direction' attribute in nsITelephonyListener

Comment 13

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Created attachment 747248 [details] [diff] [review]
> RIL - add 'direction' attribute in nsITelephonyListener

Shawn, see if this works for you.

Comment 14

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

Hi Hsin-Yi,
As always great job! It works fine! Can you submit for v.1.0.1?

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] | PTO]

Comment on attachment 747248 [details] [diff] [review]
RIL - add 'direction' attribute in nsITelephonyListener

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

Seems boolean is enough to me.

Comment 16

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: RIL - add 'isOutgoing' attribute in nsITelephonyListener (v2)

Comment #15 addressed. Thanks!

Comment 17

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Comment on attachment 747288 [details] [diff] [review]
RIL - add 'isOutgoing' attribute in nsITelephonyListener (v2)

Oops, wrong file.

Comment 18

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: RIL - add 'isOutgoing' attribute in nsITelephonyListener (v2)

Comment addressed. Thanks!

Comment 19

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

Attachment: Patch 1: v1-Bug 869306 Update call direction whenever call state change

This patch simply checks call direction from RIL interface and update mDirection. I tested TC_AG_ECS series which all passed.

Comment 20

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 747302 [details] [diff] [review]
Patch 1: v1-Bug 869306 Update call direction whenever call state change

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

r+ with nits addressed.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1209,5 @@
> +    mCurrentCallArray[aCallIndex].mDirection = false;
> +  } else {
> +    mCurrentCallArray[aCallIndex].mDirection = true;
> +  }
> +

How about making it shorter as following?

mCurrentCallArray[aCallIndex].mDirection = !aIsOutgoing;

::: dom/bluetooth/BluetoothTelephonyListener.cpp
@@ +34,4 @@
>                                      bool aIsOutgoing)
>  {
>    BluetoothHfpManager* hfp = BluetoothHfpManager::Get();
> +  hfp->HandleCallStateChanged(aCallIndex, aCallState, aNumber, aIsOutgoing, true);

nit: 80 chars per line, please. Here and below.

Comment 21

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

Attachment: Patch 1: v2-Bug 869306 Update call direction whenever call state change

Fixed nits that Comment 20 addressed.

Comment 22

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 747312 [details] [diff] [review]
Patch 1: v2-Bug 869306 Update call direction whenever call state change

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

Perfect :D

Comment 23

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

Comment on attachment 747312 [details] [diff] [review]
Patch 1: v2-Bug 869306 Update call direction whenever call state change

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

r=me with nit addressed. Let's land it. :)

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1205,3 @@
>    uint16_t prevCallState = mCurrentCallArray[aCallIndex].mState;
>    mCurrentCallArray[aCallIndex].mState = aCallState;
>  

super-nit: please remove this empty line

Comment 24

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

https://tbpl.mozilla.org/?tree=Try&rev=5999a9e5984e

Comment 25

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

Attachment: Patch 1: v2-Bug 869306 Update call direction when call state changes, r=echou

Comment 26

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

This required to change RIL. It needs to go QC process, patch is ready now.

Comment 27

[Daniel Coloma:dcoloma]

Michael, is this on your radar?

Comment 28

[Michael Vines [:m1] [:evilmachines]]

Yes, I'm sure Anshul is on it already.  Thank you for the earlier heads up this time :)

Comment 29

[Michael Vines [:m1] [:evilmachines]]

Removing NO_UPLIFT.

Comment 30

[Kai-Chih Hu [:khu]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #28)
> Yes, I'm sure Anshul is on it already.  Thank you for the earlier heads up
> this time :)

Thank you, Michael.

Comment 31

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: RIL patch - for v1.0.1

Comment 32

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

Attachment: Patch1:V2-for V1.0.1

Comment 33

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

V1.0.1 version
Try: https://tbpl.mozilla.org/?tree=Try&rev=e3c26dbfb93d

Comment 34

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

birch:

https://hg.mozilla.org/projects/birch/rev/03e1ea0f9735
https://hg.mozilla.org/projects/birch/rev/bd2a6001614f

Comment 35

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: RIL patch - for b2g18

Comment 36

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

Attachment: Patch 1: for b2g18

* Gecko/Bluetooth patch for b2g18

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03e1ea0f9735
https://hg.mozilla.org/mozilla-central/rev/bd2a6001614f

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d19b2b089e1e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/548b7689f8fa

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d5779675facd
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f3abf7afd379

Comment 39

[Mila Davydova]

Unable to verify. Lack of resources. Need Bluetooth PTS tool to verify.
Marking as QARegressExclude.