875719 - [Bluetooth]Bluetooth SCO does not disconnect after immediately receiving AT+CHUP while dialing an outgoing call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P1)

Description

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

TC_AG_TCA_BV_04_I.
STR:
1. Make an outgoing call via Bluetooth (using ATDxxxxxxxxx)
2. BT headset immediately sends AT+CHUP to cancel call

Problem:
After call cancel, Bluetooth SCO does not disconnect.

Comment 1

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

It looks like mCurrentCallIndex is 0
Call disconnected: call state: 10
Call disconnected: call aCallIndex:  1
Call disconnected: call mCurrentCallIndex:  0
Call disconnected: call index:  1

mCurrentCallIndex will be updated, only call state changed to CALL_STATE_CONNECTED.

Comment 2

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

It looks like the logic is dangerous. If outgoing call was been performed (BT headset connected), and if call state does not go into connected, SCO connection could still exist. It's not a good thing if SCO connection remains, it means power consumption problem might raise.

How to recover this problem:
If the next incoming/outgoing call with BT headset connected, and call state changed to connected, after call hang up, SCO connection will be disconnected again.

Comment 3

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

How to recover this problem:
If the next incoming/outgoing call with BT headset connected, and call state changed to connected, after call hang up, SCO connection will be disconnected again.

Comment 4

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

Hello Gina,
Can we simply remove this check? mCurrentCallIndex is not been assigned.
----->if (aCallIndex == mCurrentCallIndex) {
        // Find the first non-disconnected call (like connected, held),
        // and update mCurrentCallIndex
----->}

It seems checking mCurrentCallIndex is not a good idea here. Also, another bug Bug 869902 is also related to mCurrentCallIndex.

Comment 5

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

Hi Shawn, thanks for reporting. I found this issue last week and forgot to file a new bug :(

I'm afraid that we can't remove the check here. Think about the following scenario. There are two calls: one is active and the other is on hold. Then, receiving "AT+CHLD=1" from headset, then we should hang up the first call and then answer the second call. For this case, we shouldn't disconnect SCO, right?

I think I got a solution. Let me update here after testing.

Comment 6

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

Attachment: Patch 1(v1): Set mCurrentCallIndex when making an outgoing call

What do you think about this? Can you help to run the test case again with this patch? Thanks.

Comment 7

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

Comment on attachment 753819 [details] [diff] [review]
Patch 1(v1): Set mCurrentCallIndex when making an outgoing call

I have re-run TCA series test cases. All pass.

Comment 8

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

Transfer to Gina, because she has a better solution.

Comment 9

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

Thanks for your help, Shawn. Let me ask Eric to review my patch.

Comment 10

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

Attachment: Patch 1(v2): Set mActiveCallIndex whenever set up a SCO socket

This patch modified the following parts:
- Rename |mCurrentCallArray| to |mCallArray|
- Rename |mCurrentCallIndex| to |mActiveCallIndex|
- Put data members in BluetoothHfpManager in alphabetical order
- Assign new value to |mActiveCallIndex| whenever set up a SCO socket successfully

Comment 11

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

I will do PTS scanning again for Attachment #754314 [details] [diff] and expect be done two hours later.

Comment 12

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

Discussed with Eric. He's going to provide a new patch for this issue.

Comment 13

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

Comment on attachment 754314 [details] [diff] [review]
Patch 1(v2): Set mActiveCallIndex whenever set up a SCO socket

>From a074cd87403f6cc505ce01dab7ce69e1c9af68f1 Mon Sep 17 00:00:00 2001
>From: Gina Yeh <[e-mail]>
>Date: Mon, 27 May 2013 10:57:32 +0800
>Subject: [PATCH] Bug 875719 - Patch 1(v2): Bluetooth SCO does not disconnect
> after immediately receiving AT+CHUP while dialing an
> outgoing call
>
>---
> dom/bluetooth/BluetoothHfpManager.cpp |   63 +++++++++++++++++----------------
> dom/bluetooth/BluetoothHfpManager.h   |   13 ++++---
> 2 files changed, 41 insertions(+), 35 deletions(-)
>
>diff --git a/dom/bluetooth/BluetoothHfpManager.cpp b/dom/bluetooth/BluetoothHfpManager.cpp
>index 11ecef1..55a5fa9 100644
>--- a/dom/bluetooth/BluetoothHfpManager.cpp
>+++ b/dom/bluetooth/BluetoothHfpManager.cpp
>@@ -371,14 +371,14 @@ BluetoothHfpManager::BluetoothHfpManager()
> void
> BluetoothHfpManager::ResetCallArray()
> {
>   LOG("[H] %s", __FUNCTION__);
>-  mCurrentCallIndex = 0;
>-  mCurrentCallArray.Clear();
>-  // Append a call object at the beginning of mCurrentCallArray since call
>+  mActiveCallIndex = 0;
>+  mCallArray.Clear();
>+  // Append a call object at the beginning of mCallArray since call
>   // index from RIL starts at 1.
>   Call call;
>-  mCurrentCallArray.AppendElement(call);
>+  mCallArray.AppendElement(call);
> }
> 
> void
> BluetoothHfpManager::Reset()
>@@ -974,9 +974,9 @@ BluetoothHfpManager::ReceiveSocketData(BluetoothSocket* aSocket,
>         // Bluetooth HSP spec 4.5
>         // There are two ways to release SCO: sending CHUP to dialer or closing
>         // SCO socket directly. We notify dialer only if there is at least one
>         // active call.
>-        if (mCurrentCallArray.Length() > 1) {
>+        if (mCallArray.Length() > 1) {
>           NotifyDialer(NS_LITERAL_STRING("CHUP"));
>         } else {
>           DisconnectSco();
>         }
>@@ -1186,11 +1186,11 @@ BluetoothHfpManager::SendCommand(const char* aCommand, uint8_t aValue)
>       }
>     }
>   } else if (!strcmp(aCommand, "+CLCC: ")) {
>     bool rv = true;
>-    uint32_t callNumbers = mCurrentCallArray.Length();
>+    uint32_t callNumbers = mCallArray.Length();
>     for (uint32_t i = 1; i < callNumbers; i++) {
>-      Call& call = mCurrentCallArray[i];
>+      Call& call = mCallArray[i];
>       if (call.mState == nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
>         continue;
>       }
> 
>@@ -1213,9 +1213,9 @@ BluetoothHfpManager::SendCommand(const char* aCommand, uint8_t aValue)
>         case nsITelephonyProvider::CALL_STATE_ALERTING:
>           message.AppendInt(3);
>           break;
>         case nsITelephonyProvider::CALL_STATE_INCOMING:
>-          message.AppendInt((i == mCurrentCallIndex) ? 4 : 5);
>+          message.AppendInt((i == mActiveCallIndex) ? 4 : 5);
>           break;
>         default:
>           NS_WARNING("Not handling call status for CLCC");
>           break;
>@@ -1259,26 +1259,26 @@ BluetoothHfpManager::HandleCallStateChanged(uint32_t aCallIndex,
>     // Normal case. No need to print out warnings.
>     return;
>   }
> 
>-  while (aCallIndex >= mCurrentCallArray.Length()) {
>+  while (aCallIndex >= mCallArray.Length()) {
>     Call call;
>-    mCurrentCallArray.AppendElement(call);
>+    mCallArray.AppendElement(call);
>   }
> 
>-  uint16_t prevCallState = mCurrentCallArray[aCallIndex].mState;
>-  mCurrentCallArray[aCallIndex].mState = aCallState;
>-  mCurrentCallArray[aCallIndex].mDirection = !aIsOutgoing;
>+  uint16_t prevCallState = mCallArray[aCallIndex].mState;
>+  mCallArray[aCallIndex].mState = aCallState;
>+  mCallArray[aCallIndex].mDirection = !aIsOutgoing;
> 
>   // Same logic as implementation in ril_worker.js
>   if (aNumber.Length() && aNumber[0] == '+') {
>-    mCurrentCallArray[aCallIndex].mType = TOA_INTERNATIONAL;
>+    mCallArray[aCallIndex].mType = TOA_INTERNATIONAL;
>   }
>-  mCurrentCallArray[aCallIndex].mNumber = aNumber;
>+  mCallArray[aCallIndex].mNumber = aNumber;
> 
>   nsRefPtr<nsRunnable> sendRingTask;
>   nsString address;
>-  uint32_t callArrayLength = mCurrentCallArray.Length();
>+  uint32_t callArrayLength = mCallArray.Length();
>   uint32_t index = 1;
> 
>   switch (aCallState) {
>     case nsITelephonyProvider::CALL_STATE_HELD:
>@@ -1286,14 +1286,14 @@ BluetoothHfpManager::HandleCallStateChanged(uint32_t aCallIndex,
>       SendCommand("+CIEV: ", CINDType::CALLHELD);
>       break;
>     case nsITelephonyProvider::CALL_STATE_INCOMING:
> 
>-      if (mCurrentCallIndex) {
>+      if (mActiveCallIndex) {
>         if (mCCWA) {
>           nsAutoCString ccwaMsg("+CCWA: \"");
>           ccwaMsg.Append(NS_ConvertUTF16toUTF8(aNumber));
>           ccwaMsg.AppendLiteral("\",");
>-          ccwaMsg.AppendInt(mCurrentCallArray[aCallIndex].mType);
>+          ccwaMsg.AppendInt(mCallArray[aCallIndex].mType);
>           SendLine(ccwaMsg.get());
>         }
>         UpdateCIND(CINDType::CALLSETUP, CallSetupState::INCOMING, aSend);
>       } else {
>@@ -1308,9 +1308,9 @@ BluetoothHfpManager::HandleCallStateChanged(uint32_t aCallIndex,
> 
>         MessageLoop::current()->PostDelayedTask(
>           FROM_HERE,
>           new SendRingIndicatorTask(number,
>-                                    mCurrentCallArray[aCallIndex].mType),
>+                                    mCallArray[aCallIndex].mType),
>           sRingInterval);
>       }
>       break;
>     case nsITelephonyProvider::CALL_STATE_DIALING:
>@@ -1319,39 +1319,44 @@ BluetoothHfpManager::HandleCallStateChanged(uint32_t aCallIndex,
>         mBLDNProcessed = true;
>       }
> 
>       UpdateCIND(CINDType::CALLSETUP, CallSetupState::OUTGOING, aSend);
>-      ConnectSco();
>+      if (ConnectSco()) {
>+        mActiveCallIndex = aCallIndex;
>+      }
>       break;
>     case nsITelephonyProvider::CALL_STATE_ALERTING:
>       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.
>-      ConnectSco();
>+      if (ConnectSco()) {
>+        mActiveCallIndex = aCallIndex;
>+      }
>       break;
>     case nsITelephonyProvider::CALL_STATE_CONNECTED:
>-      mCurrentCallIndex = aCallIndex;
>       switch (prevCallState) {
>         case nsITelephonyProvider::CALL_STATE_INCOMING:
>         case nsITelephonyProvider::CALL_STATE_DISCONNECTED:
>           // Incoming call, no break
>           sStopSendingRingFlag = true;
>-          ConnectSco();
>+          if (ConnectSco()) {
>+            mActiveCallIndex = aCallIndex;
>+          }
>         case nsITelephonyProvider::CALL_STATE_ALERTING:
>           // Outgoing call
>           UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
>           UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
>           break;
>         case nsITelephonyProvider::CALL_STATE_HELD:
>           // Check whether to update CINDType::CALLHELD or not
>           while (index < callArrayLength) {
>-            if (index == mCurrentCallIndex) {
>+            if (index == mActiveCallIndex) {
>               index++;
>               continue;
>             }
> 
>-            uint16_t state = mCurrentCallArray[index].mState;
>+            uint16_t state = mCallArray[index].mState;
>             // If there's another call on hold or other calls exist, no need to
>             // update CINDType::CALLHELD
>             if (state != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
>               break;
>@@ -1391,21 +1396,19 @@ BluetoothHfpManager::HandleCallStateChanged(uint32_t aCallIndex,
>         default:
>           NS_WARNING("Not handling state changed");
>       }
> 
>-      if (aCallIndex == mCurrentCallIndex) {
>+      if (aCallIndex == mActiveCallIndex) {
>         // Find the first non-disconnected call (like connected, held),
>-        // and update mCurrentCallIndex
>         while (index < callArrayLength) {
>-          if (mCurrentCallArray[index].mState !=
>+          if (mCallArray[index].mState !=
>               nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
>-            mCurrentCallIndex = index;
>             break;
>           }
>           index++;
>         }
> 
>-        // There is no call, close Sco and clear mCurrentCallArray
>+        // There is no call, close Sco and clear mCallArray
>         if (index == callArrayLength) {
>           DisconnectSco();
>           ResetCallArray();
>         }
>diff --git a/dom/bluetooth/BluetoothHfpManager.h b/dom/bluetooth/BluetoothHfpManager.h
>index 31e22e9..f544e6d 100644
>--- a/dom/bluetooth/BluetoothHfpManager.h
>+++ b/dom/bluetooth/BluetoothHfpManager.h
>@@ -122,24 +122,27 @@ private:
>   void OnScoDisconnect();
> 
>   int mCurrentVgs;
>   int mCurrentVgm;
>-  uint32_t mCurrentCallIndex;
>+  int mNetworkSelectionMode;
>+
>+  bool mBLDNProcessed;
>   bool mCCWA;
>   bool mCLIP;
>   bool mCMEE;
>   bool mCMER;
>   bool mFirstCKPD;
>-  int mNetworkSelectionMode;
>-  bool mReceiveVgsFlag;
>-  bool mBLDNProcessed;
>   bool mIsHandsfree;
>   bool mNeedsUpdatingSdpRecords;
>+  bool mReceiveVgsFlag;
>+
>   nsString mDeviceAddress;
>   nsString mMsisdn;
>   nsString mOperatorName;
> 
>-  nsTArray<Call> mCurrentCallArray;
>+  uint32_t mActiveCallIndex;
>+  nsTArray<Call> mCallArray;
>+
>   nsAutoPtr<BluetoothTelephonyListener> mListener;
>   nsRefPtr<BluetoothReplyRunnable> mRunnable;
>   nsRefPtr<BluetoothReplyRunnable> mScoRunnable;
> 
>-- 
>1.7.9.5
>

Comment 14

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

Attachment: patch 1: v1: simplify and correct the mechanism of handling call state change event

* Remove variable mCurrentCallIndex and replace with using function FindFirstCall(aState).
* Remove unused variable index and callArrayLength

Comment 15

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

Attachment: patch 1: v1: simplify and correct the mechanism of handling call state change event

* Update since we would fail when running TWC_BV_03. Please see comments inline.

Comment 16

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

Comment on attachment 754374 [details] [diff] [review]
patch 1: v1: simplify and correct the mechanism of handling call state change event

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

After discussion, cancel this review request and let me check new patch.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1334,5 @@
>            UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
>            UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
>            break;
>          case nsIRadioInterfaceLayer::CALL_STATE_HELD:
> +          if (!FindFirstCall(nsIRadioInterfaceLayer::CALL_STATE_HELD)) {

The logic here is different from original.

Original: UpdateCIND if all calls are disconnected
New: UpdateCIND if all calls are not on held

Comment 17

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

Comment on attachment 754398 [details] [diff] [review]
patch 1: v1: simplify and correct the mechanism of handling call state change event

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

Looks great! r+ with question answered. We finally get rid of |mCurrentCallIndex|. :)

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1213,5 @@
>      }
>    }
>  }
>  
> +int

Can we use uint32_t for compatibility with call index?

Comment 18

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

Attachment: patch 1: final: simplify and correct the mechanism of handling call state change event

* nit picked

Comment 19

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

https://hg.mozilla.org/projects/birch/rev/0b13a2559365

Comment 20

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

The patch not only fix this issue but also bug 869902. Since bug 869902 has already been set as leo+, we should at least make this leo+ as well. Furthermore, since this is a certification-blocking issue, I would like to nominate as a tef+.

Comment 21

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

Attachment: patch 1: final: for b2g18

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b13a2559365

Comment 23

[Wayne Chang [:wchang]]

ni?jhford - can you help to uplift this as the blocking status has changed?

Thanks.

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Ryan, looks like a Gecko patch, mind taking a look at this?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4a8e06236ea9
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/be5c2ee11d02