Closed Bug 881194 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)

People

(Reporter: shawnjohnjr, Assigned: jaliu)

References

Details

Attachments

(3 files, 11 obsolete files)

2.67 KB, patch
echou
: review+
Details | Diff | Splinter Review
3.91 KB, patch
echou
: review+
Details | Diff | Splinter Review
7.82 KB, patch
echou
: review+
Details | Diff | Splinter Review
Supports CHLD=3, merge all conversation.
Assignee: nobody → btian
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Depends on: 772765
Sorry for setting the wrong flag. Should be 'depend on', not 'blocks'.
No longer blocks: 887680, 887686, 887764
Depends on: 887680, 887686, 887764
Depends on: 917221
Assignee: btian → jaliu
Flags: needinfo?
Attachment #828491 - Flags: feedback?(echou)
Attachment #828491 - Flags: feedback?(btian)
Flags: needinfo?
Comment on attachment 828491 [details] [diff] [review]
[HFP] Support AT+CHLD=3  patch (draft)

Looks good to me. Thanks.
Attachment #828491 - Flags: feedback?(btian) → feedback+
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.
Attachment #828491 - Flags: feedback?(echou) → feedback+
Attachment #831394 - Flags: feedback?(echou)
Attachment #831394 - Flags: feedback?(btian)
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.
Attachment #831394 - Flags: feedback?(echou)
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.
Remove the modification in AnswerWaitingCall() which is a CDMA-specific function.
Attachment #831394 - Attachment is obsolete: true
Attachment #831394 - Flags: feedback?(btian)
Attachment #832104 - Flags: feedback?(echou)
Attachment #832104 - Flags: feedback?(btian)
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?
Attachment #832104 - Flags: feedback?(btian)
Hsinyi, one question about call state change:
  Does a held call necessarily become active when the original active call is held/disconnected?
Flags: needinfo?(htsai)
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.
(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.
Flags: needinfo?(htsai)
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]
(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.
(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.
Attachment #832104 - Attachment is obsolete: true
Attachment #832104 - Flags: feedback?(echou)
Attachment #832759 - Flags: feedback?(echou)
Attachment #832759 - Flags: feedback?(btian)
Attachment #832761 - Flags: feedback?(echou)
Attachment #832761 - Flags: feedback?(btian)
Status: NEW → ASSIGNED
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.
Attachment #832759 - Flags: feedback?(echou) → feedback-
Attachment #832759 - Attachment is obsolete: true
Attachment #832759 - Flags: feedback?(btian)
Attachment #8339755 - Flags: review?(echou)
Attachment #8339755 - Attachment is obsolete: true
Attachment #8339755 - Flags: review?(echou)
Attachment #8339764 - Flags: review?(echou)
Rebase.
Attachment #832761 - Attachment is obsolete: true
Attachment #832761 - Flags: feedback?(echou)
Attachment #832761 - Flags: feedback?(btian)
Attachment #8339765 - Flags: review?(echou)
rebase
Attachment #828491 - Attachment is obsolete: true
Attachment #8339770 - Flags: review?(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.
Attachment #8339764 - Flags: review?(echou) → review-
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?
Attachment #8339765 - Flags: review?(echou) → review+
Attachment #8339764 - Attachment is obsolete: true
Attachment #8340275 - Flags: review?(echou)
Attachment #8339765 - Attachment is obsolete: true
Attachment #8340276 - Flags: review?(echou)
(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.
Attachment #8340276 - Attachment is obsolete: true
Attachment #8340276 - Flags: review?(echou)
Attachment #8340284 - Flags: review?(echou)
Attachment #8339770 - Flags: review?(echou) → review+
Just added a space to this patch.
Nothing else changed.
Attachment #8340284 - Attachment is obsolete: true
Attachment #8340284 - Flags: review?(echou)
Attachment #8340950 - Flags: review?(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.
Attachment #8340275 - Flags: review?(echou) → review+
Attachment #8340950 - Flags: review?(echou) → review+
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.
Attachment #8341561 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/b03d8885420a
https://hg.mozilla.org/mozilla-central/rev/b3fc73a2fb98
https://hg.mozilla.org/mozilla-central/rev/aec25ec7bf38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Duplicate of this bug: 996424
You need to log in before you can comment on or make changes to this bug.