827212 - [Bluetooth][Hfp] Support feature: Enhanced call status indications

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

To support "Enhanced call status indications", we need to be able to handle AT command: AT+CLCC, which shall be used to query list of current calls in AG.

Please see 4.31.1 "Query List of Current Calls in AG" of HFP 1.5 for more detail.

Comment 1

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

Attachment: Patch 1(v1): Support feature 'Enhanced call status indications'

Create a new class "Call" consist of call information. These call information are maintained in mCurrentCallArray and will be returned whenever receiving AT+CLCC command

Comment 2

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

Attachment: Patch 2(v1): Move functions from public to private in BluetoothHfpManager

Some functions in BluetoothHfpManager should be private functions, such as SendLine(), SetupCIND(), etc.

Comment 3

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

Comment on attachment 698569 [details] [diff] [review]
Patch 2(v1): Move functions from public to private in BluetoothHfpManager

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

r=me with nits addressed

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +209,5 @@
>      return NS_OK;
>    }
>  };
>  
> +NS_IMPL_ISUPPORTS1(BluetoothHfpManager::GetVolumeTask, nsISettingsServiceCallback);

Nit: 80 characters per line

@@ +256,5 @@
>        return;
>      }
>  
> +    const char* kHfpCrlf = "\xd\xa";
> +    nsAutoCString RingMsg(kHfpCrlf);

Nit: The first letter of local variables should be uncapitalized, here and below.

Comment 4

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

Attachment: Patch 2: Move functions from public to private in BluetoothHfpManager, r=echou

Final patch of Patch 2.

Comment 5

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

Comment on attachment 698562 [details] [diff] [review]
Patch 1(v1): Support feature 'Enhanced call status indications'

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

ok, still some places to be modified, happy to review once you're ready.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +117,5 @@
> +
> +    int state;
> +    bool direction; // true: incoming call; false: outgoing call
> +    nsString number;
> +    int type;

Nit: please make these member variables initialized with "m"

@@ +315,5 @@
>    sCINDItems[CINDType::CALLSETUP].value = CallSetupState::NO_CALLSETUP;
>    sCINDItems[CINDType::CALLHELD].value = CallHeldState::NO_CALLHELD;
>  
> +  Call call;
> +  mCurrentCallArray.AppendElement(call);

Please explain shortly about why we have to append one Call object at here. (index starts with 1)

@@ +957,5 @@
> +      case nsIRadioInterfaceLayer::CALL_STATE_INCOMING:
> +        if (i == mCurrentCallIndex)
> +          message.AppendInt(4);
> +        else
> +          message.AppendInt(5);

Not-even-a-bug:

I would write this in one line, like:

   message.AppendInt((i == mCurrentCallIndex) ? 4 : 5);

It's only my suggestion, though. You make the decision. :)

@@ +1094,5 @@
>  
>            // Find the first non-disconnected call (like connected, held),
>            // and update mCurrentCallIndex
> +          int c = 1;
> +          for (int length = (int)mCurrentCallArray.Length(); c < length; c++) {

Not-even-a-nit: I don't write a for-loop this way very often. In this case, I may declare another variable 'length' outside to avoid calling Length() several times. It may be like:

int c = 1;
int length = mCurrentCallArray.Length();

for (; c < length; c++) {
  ...
}

if (c == length) {
 ...
}

     or

while (c < length) {
  ...
  c++;
}

if (c == length) {
 ...
}

::: dom/bluetooth/BluetoothHfpManager.h
@@ +23,5 @@
> + * of these constants is the same as TOA_INTERNATIONAL/TOA_UNKNOWN defined in
> + * ril_consts.js
> + */
> +#define TOA_UNKNOWN 0x81
> +#define TOA_INTERNATIONAL 0x91

I prefer not moving this part to header since those are only used in BluetoothHfpManager.cpp. Make sense?

Comment 6

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

Attachment: Patch 1(v2): Support feature 'Enhanced call status indications'

New patch with nit addressed:

- Make data members in class Call starting with 'm'
- Leave const definition of TOA_UNKNOWN/TOA_INTERNATIONAL in BluetoothHfpManager.cpp
- Replace a for loop with a while loop for the case of DISCONNECTED in SetupCIND()
- Add comment for the first call object in call array
- Add function ResetCallArray(), which is called whenever there's no call
- Change the type of argument aNumber of SetupCIND() from const char* to nsAString&
- Merge function SendCLCC into function SendCommand

Comment 7

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

Attachment: Patch 2: Move functions from public to private in BluetoothHfpManager, r=echou

Comment 8

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

Attachment: Patch 1(v2): Support feature 'Enhanced call status indications'

Comment 9

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

Comment on attachment 702203 [details] [diff] [review]
Patch 1(v2): Support feature 'Enhanced call status indications'

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

Looks good!

Comment 10

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

Attachment: Patch 1: Support feature: Enhanced call status indications, r=echou

try:
https://tbpl.mozilla.org/?tree=Try&rev=ab6734df5ecf
https://tbpl.mozilla.org/?tree=Try&rev=7f05ebd3ec60

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/30e1cf104d2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf4b30c71f5

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30e1cf104d2c
https://hg.mozilla.org/mozilla-central/rev/5bf4b30c71f5

Comment 14

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

The patches here create an array of call object and the array is used in Bug 846647. Since bug 846647 is tracking-b2g18+, can we mark this tracking-b2g18+, too?

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Yes, marking tracking since it's blocking other tracking work.

Comment 16

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

Attachment: patch 1: for b2g18, r=echou

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: no
Testing completed: mozilla-central
Risk to taking this patch (and alternatives if risky): Very low. Just re-organized the public/private method in class BluetoothHfpManager.
String or UUID changes made by this patch: no

Comment 17

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

Attachment: patch 2: for b2g18, r=echou

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: would miss a Bluetooth HFP feature which is neccesary for certification
Testing completed: mozilla-central
Risk to taking this patch (and alternatives if risky): bug 846647 and bug 828798 fixed a logic problem caused by this patch, so I'll uplift these two bugs as well. No other risk.
String or UUID changes made by this patch: no

Comment 18

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #17)
> Created attachment 728910 [details] [diff] [review]
> patch 2: for b2g18, r=echou
> 
> 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: would miss a Bluetooth HFP feature which is
> neccesary for certification
> Testing completed: mozilla-central
> Risk to taking this patch (and alternatives if risky): bug 846647 and bug
> 828798 fixed a logic problem caused by this patch, so I'll uplift these two
> bugs as well. No other risk.
> String or UUID changes made by this patch: no

I've sent uplift request for bug 828798, and we should uplift bug 846647 after this bug and bug 828798 are both merged into b2g18.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b9a0fc5024a2
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ceae69d43d5c

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 728910 [details] [diff] [review]
patch 2: for b2g18, r=echou

Approving low risk HFB patches to bring us closer to spec.

Comment 21

[Kai-Chih Hu [:khu]]

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.

Comment 22

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

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.

Comment 23

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

Attachment: b2g18_v1_0_1 patch, Patch 1

Comment 24

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

Attachment: b2g18_v1_0_1 patch, Patch 2

Comment 25

[Joe Cheng [:jcheng] (please needinfo)]

tef- for now until tef release partner confirms this is blocking BT cert.

Comment 26

[Daniel Coloma:dcoloma]

Seems it is confirmed it blocks BT cer as per bug 868347

Comment 27

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

Attachment: b2g18_v1_0_1 patch, Patch 1

v1.0.1 patch updated.

Comment 28

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

Attachment: b2g18_v1_0_1 patch, Patch 2

v1.0.1 patch updated.

Comment 29

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

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5e80e070e7e3
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/147bf42d3b21