881194 - [Bluetooth][HFP]Adds a held call to the conversation (Supports CHLD=3, merge all conversation)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Supports CHLD=3, merge all conversation.

Comment 1

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

Sorry for setting the wrong flag. Should be 'depend on', not 'blocks'.

Comment 2

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

Attachment: [HFP] Support AT+CHLD=3 patch (draft)

Comment 3

[Ben Tian (inactive)]

Comment on attachment 828491 [details] [diff] [review]
[HFP] Support AT+CHLD=3  patch (draft)

Looks good to me. Thanks.

Comment 4

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

Comment on attachment 828491 [details] [diff] [review]
[HFP] Support AT+CHLD=3  patch (draft)

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

LGTM. I think you still need another patch to notify HF about the held call being merged.

Comment 5

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

Attachment: [HFP] Send +CIEV callheld properly (draft)

Comment 6

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

Comment on attachment 831394 [details] [diff] [review]
[HFP] Send +CIEV callheld properly (draft)

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

Hi Jamin, please update the patch according to my comments. I'll review the logic tomorrow. Sorry for the late reply.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1364,5 @@
>    uint16_t prevCallState = mCurrentCallArray[aCallIndex].mState;
>    mCurrentCallArray[aCallIndex].mState = aCallState;
>    mCurrentCallArray[aCallIndex].mDirection = !aIsOutgoing;
>  
> +  uint16_t prevCallIsConference = mCurrentCallArray[aCallIndex].mIsConference;

Why prevCallIsConference is not a bool?

@@ +1438,5 @@
>            // Outgoing call
>            UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
>            UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
>            break;
> +

nit: we don't usually leave a blank line between cases in switch statement.

@@ +1444,5 @@
> +          if (aIsConference) {
> +            UpdateCIND(CINDType::CALLHELD, CallHeldState::NO_CALLHELD, aSend);
> +          }
> +          break;
> +

Ditto.

Comment 7

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

I've tested these PTS test cases with Attachment #831394 [details] [diff].
 - 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_05_I, pass

It can't pass TC_AG_TWC_BV_04_I since we haven't handled AT+CHLD=3 command in Gaia layer.
I'll test TC_AG_TWC_BV_04 when it's ready.

Comment 8

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

Attachment: [HFP] Send +CIEV callheld properly (draft-2)

Remove the modification in AnswerWaitingCall() which is a CDMA-specific function.

Comment 9

[Ben Tian (inactive)]

Comment on attachment 832104 [details] [diff] [review]
[HFP] Send +CIEV callheld properly (draft-2)

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

Please see my comments below. Also I have two questions:
1) Are all conference call cases covered? Please enumerate possible conference call cases systematically for 1).
2) Is it valid to assume a held call becomes active when the active call becomes held/disconnected? I think we should confirm with ril team.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1364,5 @@
>    uint16_t prevCallState = mCurrentCallArray[aCallIndex].mState;
>    mCurrentCallArray[aCallIndex].mState = aCallState;
>    mCurrentCallArray[aCallIndex].mDirection = !aIsOutgoing;
>  
> +  uint16_t prevCallIsConference = mCurrentCallArray[aCallIndex].mIsConference;

Variable type should be bool instead of uint16_t.

@@ +1439,5 @@
>            UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
>            UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
>            break;
> +
> +        case nsITelephonyProvider::CALL_STATE_CONNECTED:

Please write comment to explain why there's CONNECTED -> CONNECTED call state change.

@@ +1447,5 @@
> +          break;
> +
> +        case nsITelephonyProvider::CALL_STATE_HELD:
> +          if (!FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)
> +            && aIsConference && !prevCallIsConference) {

nit: insert 2 more spaces to align with the if-condition.

@@ +1482,4 @@
>        if (!FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)) {
>          UpdateCIND(CINDType::CALLHELD, CallHeldState::NO_CALLHELD, aSend);
>        } else if (!FindFirstCall(nsITelephonyProvider::CALL_STATE_CONNECTED)) {
>          UpdateCIND(CINDType::CALLHELD, CallHeldState::ONHOLD_NOACTIVE, aSend);

Do we still need to send this CIEV?

Comment 10

[Ben Tian (inactive)]

Hsinyi, one question about call state change:
  Does a held call necessarily become active when the original active call is held/disconnected?

Comment 11

[Ben Tian (inactive)]

Jamin, I suggest you split into 2 patches: extra CIEV fix and conference call change. It's clearer to understand the purpose of each line change.

Comment 12

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

(In reply to Ben Tian [:btian] from comment #10)
> Hsinyi, one question about call state change:
>   Does a held call necessarily become active when the original active call
> is held/disconnected?

Spec doesn't elaborate on this case. I think this heavily depends on modem/platform/UX behaviour, should not be necessary.

Just playing with an Android phone, the held call stays on-hold even after the original active call is released.

Comment 13

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

I enumerate conference call operating cases for 3~4 callers. (I believe we can follw the same rule for more callers)

Notation Definition:
  Merge: Add a held call to the conversation.
  Swap: Swap the Connected and Held call
  Hand up: Hand up all the connected call
  C  = Connected call
  H  = Held call (including waiting call)
  C' = Connected call with conference call flag
  H' = Held call with conference call flag
  #  = What would Hfp manager do with Attachment #832104 [details] [diff]

Merge (1C, 1H)
  Case 1:
  H -> C'   (2C', 0H), send
  C -> C'   (2C', 0H), send
  # Send callheld=0 in HandleCallStateChanged(), case CALL_STATE_CONNECTED: [HELD & CONNECTED]

  Case 2:
  C -> C'   (1C', 1H), send
  H -> C'   (2C', 0H), send
  # Send callheld=0 in HandleCallStateChanged(), case CALL_STATE_CONNECTED: [HELD & CONNECTED]

Merge (2C', 1H)
  H -> C'   (3C', 0H), send callheld=0
  # Send callheld=0 in HandleCallStateChanged(), case CALL_STATE_CONNECTED: [HELD]

Merge (1C, 2H')
  Case 1:
  C  -> C'   (1C', 2H'), send
  H' -> C'   (2C', 1H')
  H' -> C'   (3C', 0H),
  # Send callheld=0 in HandleCallStateChanged(), case CALL_STATE_CONNECTED: [CONNECTED]

  Case 2:
  H' -> C'   (1C'1C, 1H')
  H' -> C'   (2C'1C, 0H),
  C  -> C'   (3C',   0H), send
  # Send callheld=0 in HandleCallStateChanged(), case CALL_STATE_CONNECTED: [CONNECTED]

  Case 3: (won't happen now?)
  H' -> C'   (1C'1C, 1H')
  C  -> C'   (2C',   1H'), send
  H' -> C'   (3C', 0H),
  # Send callheld=0 in HandleCallStateChanged(), case CALL_STATE_CONNECTED: [CONNECTED]

Swap (2C', 1H)
  Case 1:
  H  -> C    (2C'1C, 0H)
  C' -> H'   (1C'1C, 1H'), send
  C' -> H'   (1C,    2H'), send
  # Send callheld=1 in HandleCallStateChanged(), case CALL_STATE_HELD: 

  Case 2:
  C' -> H'   (1C', 1H'1H), send
  C' -> H'   (0C,  2H'1H), send
  H  -> C    (1C,  2H')
  # Send callheld=1 in HandleCallStateChanged(), case CALL_STATE_HELD: 

  Case 3: (won't happen now?)
  C' -> H'   (1C',   1H'1H), send
  H  -> C    (1C'1C, 1H')
  C' -> H'   (1C,    2H'),   send
  # Send callheld=1 in HandleCallStateChanged(), case CALL_STATE_HELD: 

Swap (1C, 2H')
  Case 1:
  H' -> C'   (1C'1C, 1H')
  H' -> C'   (2C'1C, 0H)
  C  -> H    (2C',   1H), send
  # Send callheld=1 in HandleCallStateChanged(), case CALL_STATE_HELD: 

  Case 2:
  C  -> H    (0C,  2H'1H), send
  H' -> C'   (1C', 1H'1H)
  H' -> C'   (2C', 1H)
  # Send callheld=1 in HandleCallStateChanged(), case CALL_STATE_HELD: 

  Case 3: (won't happen now?)
  H' -> C'   (1C'1C, 1H')
  C  -> H    (1C',   1H'1H), send
  H' -> C'   (2C',   1H)
  # Send callheld=1 in HandleCallStateChanged(), case CALL_STATE_HELD: 

Hang up (2C', 1H)
  C' -> D    (1C', 1H), 
  C' -> D    (0C', 1H), send
  H  -> C    (1C,  0H)
  # Send callheld=2, callheld=0 in HandleCallStateChanged(), case CALL_STATE_DISCONNECTED: [CONNECTED]

Hang up (1C, 2H')
  C  -> D    (0C,  2H'), send
  H' -> C'   (1C', 1H')
  H' -> C'   (2C', 0H)
  # Send callheld=2, callheld=0 in HandleCallStateChanged(), case CALL_STATE_DISCONNECTED: [CONNECTED]

Comment 14

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

(In reply to Ben Tian [:btian] from comment #11)
> Jamin, I suggest you split into 2 patches: extra CIEV fix and conference
> call change. It's clearer to understand the purpose of each line change.

Ben,
Got it. I'll upload two patches for +CIEV to replace Attachment #832104 [details] [diff].
Thank you.

Comment 15

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #12)
> (In reply to Ben Tian [:btian] from comment #10)
> > Hsinyi, one question about call state change:
> >   Does a held call necessarily become active when the original active call
> > is held/disconnected?
> 
> Spec doesn't elaborate on this case. I think this heavily depends on
> modem/platform/UX behaviour, should not be necessary.
> 
> Just playing with an Android phone, the held call stays on-hold even after
> the original active call is released.

Since the UX behavior may change in the future, I think I should refine the code for "Hang up" operation for different behavior. I'll upload a new patch later. 
Thanks.

Comment 16

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

Attachment: [HFP] Send +CIEV callheld properly. (part 1)

Comment 17

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

Attachment: [HFP] Send +CIEV callheld for conference call operating . (part 2)

Comment 18

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

Comment on attachment 832759 [details] [diff] [review]
[HFP] Send +CIEV callheld properly. (part 1)

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

Hi Jamin, you may need to rebase onto the latest codebase since the code tree has been modified for a while.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1373,5 @@
>    nsString address;
>  
>    switch (aCallState) {
>      case nsITelephonyProvider::CALL_STATE_HELD:
> +      if (prevCallState == nsITelephonyProvider::CALL_STATE_CONNECTED) {

This will be wrong in case 1C => 1H.

According to Page 72 of HFP spec 1.6, "... or a single active call is put on hold, the AG shall issue a +CIEV unsolicited result code with the callheld indicator value of '2'." However callheld=1 would be sent with this patch applied.

Comment 19

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

Attachment: [HFP] Send +CIEV callheld properly. (part 1, v2)

Comment 20

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

Attachment: [HFP] Send +CIEV callheld properly. (part 1, v2)

Comment 21

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

Attachment: [HFP] Send +CIEV callheld for conference call operating . (part 2, v2)

Rebase.

Comment 22

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

Attachment: [HFP] Support AT+CHLD=3.

rebase

Comment 23

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

Comment on attachment 8339764 [details] [diff] [review]
[HFP] Send +CIEV callheld properly. (part 1, v2)

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

::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
@@ +1382,4 @@
>          sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE;
> +        SendCommand("+CIEV: ", CINDType::CALLHELD);
> +      } else if (mCurrentCallArray.Length() == 1) {
> +        // A single active call is put on hold, send +CIEV, callheld=2

This won't work in the case you mentioned here. If there is only one active call and is going to be put on hold, the first if-statement check will be passed and ONHOLD_ACTIVE will be still sent.

Comment 24

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

Comment on attachment 8339765 [details] [diff] [review]
[HFP] Send +CIEV callheld for conference call operating . (part 2, v2)

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

r=me with nits addressed.

::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
@@ +1448,5 @@
>            UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
>            UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
>            break;
>  
> +        // User want to add a held call to the conversation.

typo: want's'

@@ +1460,2 @@
>          case nsITelephonyProvider::CALL_STATE_HELD:
> +          // if (!FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)

These comments are meaningless. Please remove them.

@@ +1471,1 @@
>                UpdateCIND(CINDType::CALLHELD, CallHeldState::NO_CALLHELD, aSend);

There are same operations in above two if-blocks. How about combining to one?

Comment 25

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

Attachment: [HFP] Send +CIEV callheld properly. (part 1, v3)

Comment 26

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

Attachment: [HFP] Send +CIEV callheld for conference call operating . (part 2, v3)

Comment 27

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #24)
> Comment on attachment 8339765 [details] [diff] [review]
> [HFP] Send +CIEV callheld for conference call operating . (part 2, v2)
> 
> Review of attachment 8339765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed.
> 
> ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
> @@ +1448,5 @@
> >            UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
> >            UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
> >            break;
> >  
> > +        // User want to add a held call to the conversation.
> 
> typo: want's'
> 
> @@ +1460,2 @@
> >          case nsITelephonyProvider::CALL_STATE_HELD:
> > +          // if (!FindFirstCall(nsITelephonyProvider::CALL_STATE_HELD)
> 
> These comments are meaningless. Please remove them.
> 
> @@ +1471,1 @@
> >                UpdateCIND(CINDType::CALLHELD, CallHeldState::NO_CALLHELD, aSend);
> 
> There are same operations in above two if-blocks. How about combining to one?
I've tried to combine these two conditions in to one if-blocks.
However, it becomes difficult to read and hard to add comment for each cases.
I will upload a new patch if I find a better way to handle this.
Thank you.

Comment 28

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

Attachment: [HFP] Send +CIEV callheld for conference call operating . (part 2, v4)

Comment 29

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

Attachment: [HFP] Send +CIEV callheld for conference call operating . (part 2, v5)

Just added a space to this patch.
Nothing else changed.

Comment 30

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

Comment on attachment 8340275 [details] [diff] [review]
[HFP] Send +CIEV callheld properly. (part 1, v3)

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

r=me with nits addressed.

::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
@@ +1444,5 @@
>            // Outgoing call
>            UpdateCIND(CINDType::CALL, CallState::IN_PROGRESS, aSend);
>            UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, aSend);
>            break;
> +

We usually don't leave an extra line before each 'case'.

@@ +1453,5 @@
> +              CallHeldState::ONHOLD_NOACTIVE) {
> +              UpdateCIND(CINDType::CALLHELD, CallHeldState::NO_CALLHELD, aSend);
> +          }
> +          break;
> +

Ditto.

Comment 31

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

Hi Jamin,

All patches got r+. Please remember to run all 3-way calling test cases on PTS before you request for checking in.

Thanks.

Comment 32

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

Attachment: [final] [HFP] Send +CIEV callheld for conference call operating . (part 2, v7)

Comment 33

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

It looks good on try server.
https://tbpl.mozilla.org/?tree=Try&rev=a17f512df494

Comment 34

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

The related Gaia patch of Bug 917221 has been merged on mater.
https://github.com/mozilla-b2g/gaia/commit/c3ee8d3f54c468b5e5576416dabde0b4b55a7dbb

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b03d8885420a
https://hg.mozilla.org/integration/b2g-inbound/rev/b3fc73a2fb98
https://hg.mozilla.org/integration/b2g-inbound/rev/aec25ec7bf38

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b03d8885420a
https://hg.mozilla.org/mozilla-central/rev/b3fc73a2fb98
https://hg.mozilla.org/mozilla-central/rev/aec25ec7bf38