Closed Bug 879635 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 file, 2 obsolete files)

HFP 1.6 requires AG support indicators activation and deactivation stated in 4.34.
The patch implements indicator activation and deactivation required by HFP 1.6.
Attachment #759012 - Flags: review?(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'.
Revise variable names and remove some warning logs.
Attachment #759012 - Attachment is obsolete: true
Attachment #759012 - Flags: review?(echou)
Attachment #759651 - Flags: review?(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.
Attachment #759651 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/1f7fdff2c4f0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
QA Contact: wachen
You need to log in before you can comment on or make changes to this bug.