Closed Bug 828798 Opened 9 years ago Closed 9 years ago

[Bluetooth] [HFP] No redundant +CIEV command should be sent to remote device

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 IOT1 (10may)
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: shawnjohnjr, Assigned: gyeh)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(3 files, 6 obsolete files)

CIEV was sent incorrectly during conference call test cases.
This test was based on patch from Bug 828160.
Test case as the folllowing:
	- SDP Service record for PTS: 'Handsfree HF' successfully registered
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- AT: SPP connect succeeded
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- AT: RING
	- AT: Received +CLIP: "0988095164",129
	- AT: Incoming call established
	- HCI: Audio Connection enabled
	- AT: Received +CCWA: "0939835820",129
	- AT: WARNING: received redunant +CIEV(call): +CIEV: 2,1
	- AT: Call terminated
	- FATAL ERROR (MTC): The AG should, but does not, have a call in progress
	- HCI: Audio Connection disabled
	- AT: Service Level Connection disabled
	- MTC: Test case ended
Final Verdict : Fail

Frame 1045 cause problem. It shall not be in "Call in progress".


	226	Slave	47	ATA.		Answer mode		16	 00:00:00.032000	2013/1/10 上午 11:45:55.702000	
	229	Master	47	..OK..			Success	18	 00:00:00.062000	2013/1/10 上午 11:45:55.764000	
	234	Master	47	..+CIEV: 2,1..		Call Status indicator's status report		26	 00:00:00.640000	2013/1/10 上午 11:45:56.404000	
	235	Master	47	..+CIEV: 4,0..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/10 上午 11:45:56.404000	
	262	Master	47	..+CIEV: 6,4..		Signal indicator's status report		26	 00:00:01.201000	2013/1/10 上午 11:45:57.605000	
	647	Master	47	..+CCWA: "0939835820",129..		Type: 129		39	 00:00:17.160000	2013/1/10 上午 11:46:14.765000	
	648	Master	47	..+CIEV: 4,1..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/10 上午 11:46:14.765000	
	652	Slave	47	AT+CHLD=2.	Execute "Call hold and multiparty" Command			22	 00:00:00.062000	2013/1/10 上午 11:46:14.827000	
	654	Master	47	..OK..			Success	18	 00:00:00.047000	2013/1/10 上午 11:46:14.874000	
	675	Master	47	..+CIEV: 2,1..		Call Status indicator's status report		26	 00:00:00.858000	2013/1/10 上午 11:46:15.732000	
	676	Master	47	..+CIEV: 4,0..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/10 上午 11:46:15.732000	
	677	Master	47	..+CIEV: 3,1..		Call Held indicator's status report		26	 00:00:00.016000	2013/1/10 上午 11:46:15.748000	
	1,026	Slave	47	AT+CHLD=2.	Execute "Call hold and multiparty" Command			22	 00:00:15.568000	2013/1/10 上午 11:46:31.316000	
	1,030	Master	47	..OK..			Success	18	 00:00:00.078000	2013/1/10 上午 11:46:31.394000	
	1,045	Master	47	..+CIEV: 3,1..		Call Held indicator's status report		26	 00:00:00.640000	2013/1/10 上午 11:46:32.034000	
	1,392	Slave	47	AT+CHLD=1.	Execute "Call hold and multiparty" Command			22	 00:00:15.522000	2013/1/10 上午 11:46:47.556000	
	1,396	Master	47	..OK..			Success	18	 00:00:00.063000	2013/1/10 上午 11:46:47.619000	
	1,407	Master	47	..+CIEV: 2,0..		Call Status indicator's status report		26	 00:00:00.421000	2013/1/10 上午 11:46:48.040000
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86 → ARM
Assignee: nobody → gyeh
Summary: [Bluetooth]HFP PTS test case TC_AG_TWC_BV_03_I fail due to incorrect CIEV AT command issued → [Bluetooth] [HFP] Send redundant +CIEV command to bluetooth headset
Patch is updated.
Attachment #702700 - Attachment is obsolete: true
Attachment #702700 - Flags: review?(echou)
Attachment #702701 - Flags: review?
Attachment #702701 - Flags: review? → review?(echou)
Verify on bt-master branch for early testing. Redundant +CIEV command fixed. But TC_AG_TWC_BV_03_I, TC_AG_TWC_BV_05_I were blocked by AT+CHLD=2 timeout. You can track on Bug 828804. They shared the same root cause.
Attachment #702701 - Attachment is obsolete: true
Attachment #702701 - Flags: review?(echou)
Attachment #703802 - Flags: review?(echou)
	1,154	Slave	46	ATA.		Answer mode		16	 00:00:00.031000	2013/1/18 下午 06:32:06.383000	
	1,156	Master	46	..OK..			Success	19	 00:00:00.000000	2013/1/18 下午 06:32:06.383000	
	1,158	Master	46	..+CIEV: 2,1..		Call Status indicator's status report		26	 00:00:01.030000	2013/1/18 下午 06:32:07.413000	
	1,159	Master	46	..+CIEV: 3,0..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/18 下午 06:32:07.413000	
	1,786	Master	46	..+CCWA: "0939835820",129..		Type: 129		39	 00:00:27.970000	2013/1/18 下午 06:32:35.383000	
	1,787	Master	46	..+CIEV: 3,1..		Call Setup indicator's status report		26	 00:00:00.000000	2013/1/18 下午 06:32:35.383000	
	1,791	Slave	46	AT+CHLD=2.	Execute "Call hold and multiparty" Command			22	 00:00:00.078000	2013/1/18 下午 06:32:35.461000	
	1,793	Master	46	..OK..			Success	19	 00:00:00.016000	2013/1/18 下午 06:32:35.477000	
	1,808	Master	46	..+CIEV: 3,0..		Call Setup indicator's status report		26	 00:00:00.655000	2013/1/18 下午 06:32:36.132000	
	1,809	Master	46	..+CIEV: 4,1..		Call Held indicator's status report		26	 00:00:00.000000	2013/1/18 下午 06:32:36.132000	
	1,861	Slave	46	AT+CHLD=2.	Execute "Call hold and multiparty" Command			22	 00:00:02.247000	2013/1/18 下午 06:32:38.379000	
	1,865	Master	46	..OK..			Success	19	 00:00:00.031000	2013/1/18 下午 06:32:38.410000	
	1,876	Master	46	..+CIEV: 4,1..		Call Held indicator's status report		26	 00:00:00.499000	2013/1/18 下午 06:32:38.909000	
	2,031	Slave	46	AT+CHLD=1.	Execute "Call hold and multiparty" Command			22	 00:00:06.786000	2013/1/18 下午 06:32:45.695000	
	2,033	Master	46	..OK..			Success	19	 00:00:00.078000	2013/1/18 下午 06:32:45.773000	
	2,052	Master	46	..+CIEV: 4,0..		Call Held indicator's status report		26	 00:00:00.749000	2013/1/18 下午 06:32:46.522000	
	2,581	Master	46	..+CIEV: 2,0..		Call Status indicator's status report		26	 00:00:23.712000	2013/1/18 下午 06:33:10.234000
Attachment #703802 - Flags: review?(echou)
When we receive "AT+CHLD=2" from bluetooth headset, we will ask dialer app to hold the current call and accept the other (held or waiting) call.

The expected call status change after executing "AT+CHLD=2" in sequence is:
* Current call is on hold -> another call is connected

However, the call status changes sometimes in reverse order, which is:
* Another call is connected -> current call is on hold

In this case, after receiving the first call state change (and before the second change), two calls in mCurrentCallArray are connected and no call is on hold. But, we shouldn't send out "+CIEV: <Callheld Indicator>, 0" in the expected result of PTS test. Thus, status of all calls are checked to make sure whether to send update or not.
Attachment #703802 - Attachment is obsolete: true
Attachment #704453 - Flags: review?(echou)
Depends on: 828804
Attachment #704453 - Attachment is obsolete: true
Attachment #704453 - Flags: review?(echou)
Attachment #704735 - Flags: review?(echou)
Comment on attachment 704735 [details] [diff] [review]
Patch 1(v2): No redundant +CIEV command to bluetooth headset

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

Looks good, r=me with nits addressed. Because this patch is a little complicated, please make sure the logic is right before checking in. Thanks.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1147,5 @@
> +      UpdateCIND(CINDType::CALLSETUP, CallSetupState::OUTGOING_ALERTING, aSend);
> +
> +      // If there's an ongoing call when the headset is just connected, we have
> +      // to open a sco socket here.
> +      if (!aSend) {

Question: Using variable |aSend| to decide if we're going to create a SCO connection is fine for now, because EnumerateCallState() is the only place calling HandleCallStateChanged() and assigning value 'false' to |aSend|. I was wondering if we could use the connection status of SCO to decide if we have to create a SCO link. That would be more clear than than using |aSend|. Make sense?

@@ +1168,5 @@
> +          UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
> +          UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
> +          break;
> +        case nsIRadioInterfaceLayer::CALL_STATE_HELD:
> +          // Check whether to updatd CINDType::CALLHELD or not

typo: s/updatd/update

@@ +1179,5 @@
> +            uint16_t state = mCurrentCallArray[index].mState;
> +            // If there's another call on hold or other calls exist, no need to
> +            // update CINDType::CALLHELD
> +            if ((state == nsIRadioInterfaceLayer::CALL_STATE_HELD) ||
> +                (state != nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED)) {

Just use "if (state != nsIRadioInterfaceLayer::CALL_STATE_DISCONNECTED)" should be o.k.
Attachment #704735 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/b0db5caaeb0e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 828804
Please nominated for uplift approval with a risk assessment.
Summary: [Bluetooth] [HFP] Send redundant +CIEV command to bluetooth headset → [Bluetooth] [HFP] No redundant +CIEV command should be sent to remote device
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 #): No
User impact if declined: Gecko Bluetooth would send result code CIEV incorrectly during conference call test cases.
Testing completed: mozilla-central
Risk to taking this patch (and alternatives if risky): Fairly low. We have tested HFP with m-c build for a while.
String or UUID changes made by this patch: No
Attachment #729441 - Flags: approval-mozilla-b2g18?
Comment on attachment 729441 [details] [diff] [review]
patch 1: for b2g18, r=echou

Approving low risk HFB patches to bring us closer to spec.
Attachment #729441 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
This bug(and other 9 bugs) block Bluetooth certification. So, these bugs need to be marked as tef+ and landed to v1.0.1 in order to pass Bluetooth certification: 

bug 827255
bug 827212
bug 827266
bug 828175
bug 823346
bug 827230
bug 828798
bug 835740
bug 846647
bug 828160

So, mark these bugs as tef?. These fixes have some dependency and it would be better to have them landed in a specific order. Gina has a good view on this.
blocking-b2g: --- → tef?
I recommend to land these patches in the following order:

01. bug827204
02. bug827255
03. bug823346
04. bug827230
05. bug828798
06. bug827212, patch 1
06. bug827212, patch 2
07. bug827266
08. bug828175
09. bug846647
10. bug825861
11. bug825851
12. bug835740




I'm going to attach patches for b2g18_v1_0_1 for each bug. Please land them in the above order. There should be no conflict and feel free to let me know if I can be any help.
Attached patch b2g18_v1_0_1 patch (obsolete) — Splinter Review
tef- for now until partner confirms this is blocking BT cert.
blocking-b2g: tef? → -
needed for tef BT cert.
blocking-b2g: - → tef?
Seems it is confirmed it blocks BT cer as per bug 868347
blocking-b2g: tef? → tef+
v1.0.1 patch updated.
Attachment #738766 - Attachment is obsolete: true
Ryan, I've updated all patches. Please land them in the following order, thanks.

01. bug827204
02. bug827255
(03. bug823346 has been landed on v1.0.1)
04. bug827230
05. bug828798
06. bug827212, patch 1
06. bug827212, patch 2
07. bug827266
08. bug828175
09. bug846647
10. bug825861
11. bug825851
12. bug835740
Target Milestone: B2G C4 (2jan on) → 1.0.1 IOT1 (10may)
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.