879635 - [Bluetooth] [Hfp] Implement HFP 1.6 indicators activation and deactivation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

HFP 1.6 requires AG support indicators activation and deactivation stated in 4.34.

Comment 1

[Ben Tian (inactive)]

Attachment: HFP 1.6 indicator activation/deactivation

The patch implements indicator activation and deactivation required by HFP 1.6.

Comment 2

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

Comment on attachment 759012 [details] [diff] [review]
HFP 1.6 indicator activation/deactivation

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

Looks good. Please have me review again after revision. Thanks.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +112,5 @@
>  typedef struct {
>    const char* name;
>    const char* range;
>    int value;
> +  bool active;

'activated' seems to be more accurate than 'active'

@@ +355,5 @@
>    return false;
>  }
>  
> +static bool
> +IsMandatoryINDType(const int type) {

'C'INDType?

@@ +1000,5 @@
>      }
> +  } else if (msg.Find("AT+BIA=") != -1) {
> +    ParseAtCommand(msg, 7, atCommandValues);
> +
> +    if (atCommandValues.IsEmpty()) {

Don't worry if it's empty. The for-loop below will handle it.

@@ +1007,5 @@
> +    }
> +
> +    for (uint8_t i = 0; i < atCommandValues.Length(); i++) {
> +      // Map indicator type
> +      uint8_t type = i + 1;

I would recommend using 'indicator' or 'indicatorType' as the variable name.

@@ +1028,5 @@
> +          sCINDItems[type].active = 0;
> +        } else if (!atCommandValues[i].EqualsLiteral("")) {
> +          SendLine("ERROR");
> +        }
> +      } else if (!atCommandValues[i].EqualsLiteral("0")) {

well, it's good to check if the mandatory indicator is assigned with 0. However, since it needs an additional check and there is no obvious different between activate and deactivate a mandatory feature, let's just use 'else' instead of 'else if'.

Comment 3

[Ben Tian (inactive)]

Attachment: HFP 1.6 indicator activation/deactivation v2

Revise variable names and remove some warning logs.

Comment 4

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

Comment on attachment 759651 [details] [diff] [review]
HFP 1.6 indicator activation/deactivation v2

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

r=me with nits addressed. Please use PTS to ensure HFP 1.6 can be passed before handing in your final patch. Thank you.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +355,5 @@
>    return false;
>  }
>  
> +static bool
> +IsMandatoryIndicator(const int indicatorType) {

Using |CINDType| as variable type seems more reasonable than int.

@@ +1020,5 @@
> +          sCINDItems[indicatorType].activated = 1;
> +        } else if (atCommandValues[i].EqualsLiteral("0")) {
> +          sCINDItems[indicatorType].activated = 0;
> +        } else if (!atCommandValues[i].EqualsLiteral("")) {
> +          SendLine("ERROR");

Remember to return after sending ERROR, otherwise OK will be sent as well.

Comment 5

[Ben Tian (inactive)]

Attachment: [final] HFP 1.6 indicator activation/deactivation, r=echou

Comment 6

[Ben Tian (inactive)]

try server link: https://tbpl.mozilla.org/?tree=Try&rev=816a2f2a49ab

Comment 7

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

https://hg.mozilla.org/projects/birch/rev/1f7fdff2c4f0

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f7fdff2c4f0