828837 - [Bluetooth]HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x20080c

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Bluetooth Settings application discovery if HF CoD set to 0x200408.
CoD value seems mismatch what phone expected.

Test case : TC_AG_COD_BV_02_I started
	- SDP Service record for PTS: 'Handsfree HF' successfully registered
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- AT: SPP connect succeeded
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- MTC: IUT was able to connect to HF when CoD set to 0x200404
	- AT: Service Level Connection disabled
	- AT: SPP connect succeeded
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- MTC: IUT was able to connect to HF when CoD set to 0x202404
	- AT: Service Level Connection disabled
	- AT: SPP connect succeeded
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- MTC: IUT was able to connect to HF when CoD set to 0x200408
	- AT: Service Level Connection disabled
	- Guard timer timed out
	- MTC: Test case ended

Comment 1

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

Attachment: CoD was not been recognized as Audio device

Comment 2

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

Attachment: hcidump

Comment 3

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

Attachment: logcat

Comment 4

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

This issue was related to we use BlueZ class_to_icon to identify the remote device. Test case write into COD 0x20080c. It is Major device class Toy, Minor device class: Doll/Action Figure. We shall double check in gecko for any Service Class equals AUDIO class. As HFP spec mentioned
A device implementing the HF role of HFP shall set the "Audio" bit in the Service Class
field.

Comment 5

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

Attachment: HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x20080c. Replace icon if icon is empty.

HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x20080c.
Replace icon if icon is empty.

Comment 6

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

Comment on attachment 702691 [details] [diff] [review]
HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x20080c. Replace icon if icon is empty.

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

Happy to review again after questions to be answered.

::: dom/bluetooth/BluetoothDevice.cpp
@@ +96,5 @@
>  
> +bool
> +BluetoothDevice::HasAudioService(uint32_t codValue)
> +{
> +  return ((codValue & 0xFFE000 & 0x200000) == 0x200000);

Question: Can we replace the expression with ((codValue & 0x200000) == 0x200000)?

@@ +98,5 @@
> +BluetoothDevice::HasAudioService(uint32_t codValue)
> +{
> +  return ((codValue & 0xFFE000 & 0x200000) == 0x200000);
> +}
> +

Question: Since HasAudioService() function is only used in this file, can we make it as a function with file scope rather than a member function in BluetoothDevice?

For example:
static bool
HasAudioService(uint32_t codValue)
{
  ...
}

@@ +118,5 @@
> +      if (HasAudioService(mClass)) {
> +        mIcon = NS_LITERAL_STRING("audio-card");
> +      }
> +    }
> +

Please remove the blank line.

Comment 7

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty

Comment 8

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

Comment on attachment 702720 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty

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

Looks good, r=me.

Comment 9

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

Comment on attachment 702720 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty

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

Please send review request to me again once the new patch is incoming.

::: dom/bluetooth/BluetoothDevice.cpp
@@ +94,5 @@
>    }
>  }
>  
> +static bool
> +HasAudioService(uint32_t codValue)

nit: we usually declare arguments with a starting 'a'

@@ +113,5 @@
>    } else if (name.EqualsLiteral("Address")) {
>      mAddress = value.get_nsString();
>    } else if (name.EqualsLiteral("Class")) {
>      mClass = value.get_uint32_t();
> +    if (mClass && mIcon.IsEmpty()) {

The problem here is that we can't guarantee the property "Icon" has been already processed beforehand. If not, then mIcon.IsEmpty() would be true. Although it would probably not affect the final value of "Icon" of this device object, I would still suggest that we should handle this 'Empty Icon' situation after all properties have been retrieved.

Also, the conditional check could be only |if (mIcon.IsEmpty)| since mClass is uint32_t. Then you may want to combine the two conditional checks to one:
if (mIcon.IsEmpty() && HasAudioService(mClass)) {
mIcon = NS_LITERAL_STRING("audio-card");
}

Comment 10

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty

Revised and check only Icon is missing and Class has Audio Service class.

Comment 11

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

Comment on attachment 704773 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty

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

Several places need to be revised. Please send me another review once it's done.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +988,5 @@
>    return -1;
>  }
>  
>  bool
> +HasAudioService(uint32_t codValue)

nit: we usually make the name of arguments initialized with an 'a'. e.g. aCodValue.

@@ +993,5 @@
> +{
> +  return ((codValue & 0x200000) == 0x200000);
> +}
> +
> +bool ContainsIcon(BluetoothValue& v)

nit: we usually make the name of arguments initialized with an 'a'. Also, please break between the function name and the return type.

@@ +998,5 @@
> +{
> +  const InfallibleTArray<BluetoothNamedValue>& properties =
> +    v.get_ArrayOfBluetoothNamedValue();
> +  uint8_t i;
> +  for (i = 0; i < properties.Length(); i++) {

Since i is not used outside the for-loop, using |int i = 0| should be fine.

@@ +1390,5 @@
>          // where to access it later.
>          v.get_ArrayOfBluetoothNamedValue()
>            .AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Path"),
>                                               path));
> +        bool containsIconField = ContainsIcon(v);

We should use the result of ContainsIcon() to determine if we need to add an "Icon" property, so that we can avoid getting into the for-loop below.

@@ +1394,5 @@
> +        bool containsIconField = ContainsIcon(v);
> +        const InfallibleTArray<BluetoothNamedValue>& properties =
> +          v.get_ArrayOfBluetoothNamedValue();
> +        uint8_t i;
> +        for (i = 0; i < properties.Length(); i++) {

Since i is not used outside the for-loop, using |int i = 0| should be fine.

@@ +1395,5 @@
> +        const InfallibleTArray<BluetoothNamedValue>& properties =
> +          v.get_ArrayOfBluetoothNamedValue();
> +        uint8_t i;
> +        for (i = 0; i < properties.Length(); i++) {
> +          // It is possible that property Icon missed due to CoD of major class 

nit: trailing whitespace

@@ +1396,5 @@
> +          v.get_ArrayOfBluetoothNamedValue();
> +        uint8_t i;
> +        for (i = 0; i < properties.Length(); i++) {
> +          // It is possible that property Icon missed due to CoD of major class 
> +          // is TOY but Service class is "Audio", we need to assign Icon as audio-card

super-nit: "Service class" => "service class"

@@ +1400,5 @@
> +          // is TOY but Service class is "Audio", we need to assign Icon as audio-card
> +          // this is for PTS test TC_AG_COD_BV_02_I
> +          if (properties[i].name().EqualsLiteral("Class")) {
> +            if (!containsIconField &&
> +              HasAudioService(properties[i].value().get_uint32_t())) {

Please make these conditions aligned with each other.

if (!containsIconField &&
    HasAudioService(...)

And, we don't need to check containsIconField here if we've already checked beforehand.

@@ +1403,5 @@
> +            if (!containsIconField &&
> +              HasAudioService(properties[i].value().get_uint32_t())) {
> +              v.get_ArrayOfBluetoothNamedValue()
> +                .AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Icon"),
> +                     NS_LITERAL_STRING("audio-card") ));

nit: This is mis-indented.

@@ +1406,5 @@
> +                .AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Icon"),
> +                     NS_LITERAL_STRING("audio-card") ));
> +              if(!ContainsIcon(v)) {
> +                NS_WARNING("Fail to insert Icon as audio-card");
> +              }

I tend to remove this if-block. If you want to know if AppendElement() succeeded, just check the return value.

Comment 12

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty

Comment 13

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty

Comment 14

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

Comment on attachment 705240 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty

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

Almost done! As usual, please have me review again once the patch is ready. :)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +994,5 @@
> +  return ((aCodValue & 0x200000) == 0x200000);
> +}
> +
> +bool
> +ContainsIcon(BluetoothValue& aValue)

I found that aValue.get_ArrayOfBluetoothNamedValue() is called not only in this function but also in the caller (line 1394). So I would suggest that we should just make |properties| as an argument passing into this function. That should work.

@@ +1392,5 @@
>                                               path));
> +        bool containsIconField = ContainsIcon(v);
> +        const InfallibleTArray<BluetoothNamedValue>& properties =
> +          v.get_ArrayOfBluetoothNamedValue();
> +        if (!containsIconField) {

nit: We don't really need to use an additional variable |containsIconField| since it's only used once. Just checking ContainsIcon(v) would be fine.

@@ +1395,5 @@
> +          v.get_ArrayOfBluetoothNamedValue();
> +        if (!containsIconField) {
> +          for (int i = 0; i < properties.Length(); i++) {
> +            // It is possible that property Icon missed due to CoD of major class
> +            // is TOY but service class is "Audio", we need to assign Icon as audio-card

nit: 80 characters per line, please.

@@ +1396,5 @@
> +        if (!containsIconField) {
> +          for (int i = 0; i < properties.Length(); i++) {
> +            // It is possible that property Icon missed due to CoD of major class
> +            // is TOY but service class is "Audio", we need to assign Icon as audio-card
> +            // this is for PTS test TC_AG_COD_BV_02_I. As HFP spec. defined that

super-nit: '.' should be a ','

@@ +1402,5 @@
> +            if (properties[i].name().EqualsLiteral("Class")) {
> +              if (HasAudioService(properties[i].value().get_uint32_t())) {
> +                v.get_ArrayOfBluetoothNamedValue()
> +                  .AppendElement(BluetoothNamedValue(NS_LITERAL_STRING("Icon"),
> +                                                     NS_LITERAL_STRING("audio-card")));

nit: 80 characters per line, please.

Comment 15

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

Besides, the header of your patch seems not correct. The file name "nsGfxScrollFrame.cpp" is shown however I don't think you made any change on that file. Please check if there is anything wrong.


changeset:   118908:3a7a36e83b31
tag:         tip
user:        shuang <[e-mail]>
date:        Wed Jan 23 12:07:11 2013 +0800
files:       dom/bluetooth/linux/BluetoothDBusService.cpp layout/generic/nsGfxScrollFrame.cpp
description:
Bug 828837- Fix COD equals 0x20080c cause icon empty

Comment 16

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty

Comment 17

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

Comment on attachment 705267 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty

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

Looks good! I believe the next patch would be the final one! Please pick those nits and send me back for review.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +993,5 @@
> +{
> +  return ((aCodValue & 0x200000) == 0x200000);
> +}
> +
> +bool ContainsIcon(const InfallibleTArray<BluetoothNamedValue>& aProperties)

nit: break after |bool|

@@ +1392,5 @@
> +          v.get_ArrayOfBluetoothNamedValue();
> +        if (!ContainsIcon(properties)) {
> +          for (uint8_t i = 0; i < properties.Length(); i++) {
> +            // It is possible that property Icon missed due to CoD of major
> +            // class is TOY but service class is "Audio", we need to assign 

nit: trailing white space

@@ +1393,5 @@
> +        if (!ContainsIcon(properties)) {
> +          for (uint8_t i = 0; i < properties.Length(); i++) {
> +            // It is possible that property Icon missed due to CoD of major
> +            // class is TOY but service class is "Audio", we need to assign 
> +            // Icon as audio-card. This is for PTS test TC_AG_COD_BV_02_I. 

nit: trailing white space

@@ +1403,5 @@
> +                  .AppendElement(
> +                    BluetoothNamedValue(NS_LITERAL_STRING("Icon"),
> +                                        NS_LITERAL_STRING("audio-card")));
> +              }
> +            }

Since the goal has been reached, we should |break| to save some time.

Comment 18

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty

Comment 19

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

Comment on attachment 706244 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty

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

Looks good! Thanks for being patient! :)

You may want to send your patch to try server first. After making sure the patch has passed all the tests, remember to modify your hg header to appropriate format, and then add a keyword 'checkin-needed'.

Comment 20

[Mozilla RelEng Bot]

Try run for 31cafb35ae19 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=31cafb35ae19
Results (out of 19 total builds):
    success: 18
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-31cafb35ae19

Comment 21

[Mozilla RelEng Bot]

Try run for ef0a91aeebe1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ef0a91aeebe1
Results (out of 24 total builds):
    success: 20
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ef0a91aeebe1

Comment 22

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

Attachment: Bug 828837- Fix COD equals 0x20080c cause icon empty, r=echou

Final version

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/da4ddb6f669c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da4ddb6f669c

Comment 25

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

Comment on attachment 708892 [details] [diff] [review]
Bug 828837- Fix COD equals 0x20080c cause icon empty, 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: Bluetooth Certification failure. Gecko Bluetooth would behave incorrect during IOPT test cases.
Risk to taking this patch (and alternatives if risky): Fairly low. We have tested HFP with m-c build for a while.
String or UUID changes made by this patch: No

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/af895e09300b

Comment 27

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

This issue blocked bt certification

Comment 28

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

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

Comment 29

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

blocks BT cert.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/11df62344a39