Closed Bug 828837 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
1.0.1 IOT1 (10may)
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(4 files, 7 obsolete files)

55.92 KB, image/png
Details
104.36 KB, text/plain
Details
569.63 KB, text/plain
Details
2.88 KB, patch
Details | Diff | Splinter Review
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
Assignee: nobody → shuang
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.
Summary: [Bluetooth]HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x200408 → [Bluetooth]HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x20080c
HFP PTS test case TC_AG_COD_BV_02_I failed due to COD value=0x20080c.
Replace icon if icon is empty.
Attachment #702691 - Flags: review?(gyeh)
Attachment #702691 - Flags: review?(echou)
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.
Attachment #702691 - Flags: review?(gyeh)
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.
Attachment #702720 - Flags: review?(gyeh) → review+
Attachment #702720 - Flags: review+ → review?(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");
}
Attachment #702720 - Flags: review?(echou)
Attachment #702691 - Attachment is obsolete: true
Attachment #702691 - Flags: review?(echou)
Revised and check only Icon is missing and Class has Audio Service class.
Attachment #704773 - Flags: review?(gyeh)
Attachment #704773 - Flags: review?(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.
Attachment #704773 - Flags: review?(echou) → review-
Attachment #704773 - Attachment is obsolete: true
Attachment #704773 - Flags: review?(gyeh)
Attachment #705235 - Attachment is obsolete: true
Attachment #705235 - Flags: review?(gyeh)
Attachment #705235 - Flags: review?(echou)
Attachment #705240 - Flags: review?(gyeh)
Attachment #705240 - Flags: review?(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.
Attachment #705240 - Flags: review?(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 <shuang@mozilla.com>
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
Attachment #705240 - Attachment is obsolete: true
Attachment #705240 - Flags: review?(gyeh)
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.
Attachment #705267 - Flags: review?(echou)
Attachment #705267 - Attachment is obsolete: true
Attachment #705267 - Flags: review?(gyeh)
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'.
Attachment #706244 - Flags: review?(echou) → review+
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/shuang@mozilla.com-31cafb35ae19
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/shuang@mozilla.com-ef0a91aeebe1
Attachment #708892 - Attachment description: Bug 828837- Fix COD equals 0x20080c cause icon empty → Bug 828837- Fix COD equals 0x20080c cause icon empty, r=echou
https://hg.mozilla.org/mozilla-central/rev/da4ddb6f669c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Attachment #708892 - Flags: approval-mozilla-b2g18?
Attachment #708892 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
This issue blocked bt certification
blocking-b2g: --- → tef?
tef- for now until tef release partner confirms this is blocking BT cert.
blocking-b2g: tef? → -
blocks BT cert.
blocking-b2g: - → tef?
blocking-b2g: tef? → tef+
Target Milestone: B2G C4 (2jan on) → 1.0.1 IOT1 (10may)
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.