[Bluetooth][PTS][2.2] AVRCP- TC_TG_NFY_BV_02_C failed

RESOLVED FIXED in Firefox 39, Firefox OS v2.2

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shinglyu, Assigned: shawnjohnjr)

Tracking

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8576484 [details]
_2015_03_11_17_32_44.zip

The test runs forever until timed out

##STR

Test case : TC_TG_NFY_BV_02_C started
	- MTC: SDP Service record was successfully registered.
	- A2DP SNK SDP Service record successfully registered
	- A2DP SNKsuccessfully registered
	- MTC INCONC: IUT failed to successfully respond to AVDTP CONNECT.
	- AVDTP: Exiting AVDTP1 PTC
	- AVCTP: Exiting AVCTP1 PTC
	- CM_PTC_EXIT
	- MTC: Test case ended
Final Verdict:INCONC
TC_TG_NFY_BV_02_C finished

## Version
* V18D
* 2014/03/12's pvt build for 2.2
* adb shell setprop ro.moz.bluetooth.backend bluetoothd
(Assignee)

Comment 1

3 years ago
This looks like you have not yet paired before. Due to the test case needs to establish connection from PTS machine (incoming connection). So I would suggest you can run one HFP AG-AG_ATA_BV_02_I to make sure you paired with PTS machine, then you can cancel AG-AG_ATA_BV_02_I test case and start to test AVRCP test cases.

PTS cannot handle this situation if you haven't pair with phone (if you don't pair the incoming connection will be blocked by the phone).
(Assignee)

Comment 2

3 years ago
This looks like you have not yet paired before. Due to the test case needs to establish connection from PTS machine (incoming connection). So I would suggest you can run one HFP AG-AG_ATA_BV_02_I to make sure you paired with PTS machine, then you can cancel AG-AG_ATA_BV_02_I test case and start to test AVRCP test cases.

PTS cannot handle this situation if you haven't pair with phone (if you don't pair the incoming connection will be blocked by the phone).
(Assignee)

Comment 3

3 years ago
I will take this bug.

03-16 15:47:44.046 E/bt-avp  ( 1643): opcode 0
03-16 15:47:44.046 D/bt-btif ( 1643): btif_av_state_opening_handler event:BTA_AV_META_MSG_EVT flags 0
03-16 15:47:44.046 E/bluetoothd( 1643): PDU overflow

If fails when getting register notification with event track changed. This is a regression.
Assignee: nobody → shuang
(Assignee)

Comment 4

3 years ago
PDU overflow

#0  read_pdu_at_va (ap=..., fmt=<optimized out>, offset=3, pdu=0xb6e8c010) at system/bluetoothd/src/bt-proto.c:140
#1  read_pdu_at (pdu=pdu@entry=0xb6e8c010, offset=<optimized out>, fmt=0xb6f7ce24 "m") at system/bluetoothd/src/bt-proto.c:157
#2  0xb6f7ae58 in read_btrc_uid_t (pdu=pdu@entry=0xb6e8c010, off=<optimized out>, p_val=p_val@entry=0xbe9974f0 "\n") at system/bluetoothd/src/bt-proto.c:445
#3  0xb6f7ae9a in read_btrc_register_notification_t (pdu=pdu@entry=0xb6e8c010, off=<optimized out>, event_id=BTRC_EVT_TRACK_CHANGE, param=param@entry=0xbe9974f0) at system/bluetoothd/src/bt-proto.c:465
#4  0xb6f7b69c in opcode_register_notification_rsp (cmd=0xb6e8c010) at system/bluetoothd/src/bt-rc-io.c:676
#5  0xb6f7aabc in handle_pdu (handler=<optimized out>, cmd=<optimized out>, value=<optimized out>, field=0xb6f7cfad "opcode") at system/bluetoothd/src/bt-proto.c:50
#6  handle_pdu_by_opcode (cmd=<optimized out>, handler=<optimized out>) at system/bluetoothd/src/bt-proto.c:64
(Assignee)

Comment 5

3 years ago
PDU overflow, offset+len: 11, pdu->len: 10

According to spec, command parameters:
  Event (1 octet)
  Type (1 octet)
  Data length (1 octet)
  Data (variable)

It looks like one parameter 'Data length' is missing :(
See:
http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/18619f8f6c5c/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp#l268
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1142411
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1142413
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1142416
(Assignee)

Comment 9

3 years ago
Created attachment 8578041 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

I've tested notification related test cases, NFY_BV_02, 04, 05, 08, these test cases are pass.
Attachment #8578041 - Flags: review?(tzimmermann)
(Assignee)

Comment 10

3 years ago
2.2? for PTS certification requirement.
blocking-b2g: --- → 2.2?
Comment on attachment 8578041 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

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

Thanks for finding and fixing this bug. I set the patch to r- because it could introduce similar problems for other parameters. See below for an idea on how to handle the issue.

::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp
@@ +257,5 @@
>    BluetoothAvrcpResultHandler* aRes)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  uint8_t len = 8; // UID size

The value of |len| depends on the values of |aEvent| and |aType|. Please add a method to compute the size of the data. The best place is probably something like |size_t BluetoothAvrcpEventParamPair::GetLength|.

The code in the local method would then look like this

>  ...
>  BluetoothAvrcpEventParamPair data(aEvent, aParam);
> 
>  PackPDU(aEvent, aType, data->GetLength(), data, *pdu);
>  ...

The code for packing is at [1].

[1] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#1139
Attachment #8578041 - Flags: review?(tzimmermann) → review-
(Reporter)

Comment 12

3 years ago
Hi Shawn,

Sorry I was on sick leave yesterday. Seems like Teri have already helped you run the PTS again. Do you still need a fresh log right now?
Flags: needinfo?(shuang)
(Assignee)

Comment 13

3 years ago
(In reply to Shing Lyu [:slyu] from comment #12)
> Hi Shawn,
> 
> Sorry I was on sick leave yesterday. Seems like Teri have already helped you
> run the PTS again. Do you still need a fresh log right now?

Nope :) Thanks for all the help. I made a stupid mistake to test in-gecko HAL interface instead of bluetooth daemon last week so this is why I did not notice that we have these Notification series bugs.
Thanks a lot!
Flags: needinfo?(shuang)
(Assignee)

Updated

3 years ago
Attachment #8578041 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8578573 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command
Attachment #8578573 - Flags: review?(tzimmermann)
Comment on attachment 8578573 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +86,5 @@
>    { }
>  
> +  size_t GetLength()
> +  {
> +    return sizeof(mParam);

This won't work. The structure [1] is quite large and the actual size depends on the value of |mEvent|. I suggest to handle each case separately in a switch statement. You can probably use the packing code [2] as template.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothCommon.h#443
[2] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#1139
Attachment #8578573 - Flags: review?(tzimmermann) → review-
(Assignee)

Comment 16

3 years ago
Hmm, a bit strange, I tested this patch, I got the correct result. I tested notification of TRACK_CHANGE, or PLAYSTATUS_CHANGED, track numbers and play status both correct.
(Assignee)

Comment 17

3 years ago
Maybe it's because |len| actually been ignored?
https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/v2.2/src/bt-proto.c#L458
(In reply to Shawn Huang [:shawnjohnjr] from comment #17)
> Maybe it's because |len| actually been ignored?
> https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/v2.2/src/bt-
> proto.c#L458

Lol :D

I'd still prefer to fix this correctly.

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+
(Assignee)

Updated

3 years ago
Attachment #8578573 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8579178 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command
(Assignee)

Updated

3 years ago
Attachment #8579178 - Attachment description: bug1142408.patch → Bug 1142408 - Add data length parameter for Register Notification Response Command
(Assignee)

Updated

3 years ago
Attachment #8579178 - Flags: review?(tzimmermann)
Comment on attachment 8579178 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

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

One of the cases seems to return an incorrect value; r+ with this fixed. Thanks a lot!

::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp
@@ +263,5 @@
>                             1 + // Event
>                             1 + // Type
>                             1 + // Data length
>                             256)); // Maximum data length
> +  BluetoothAvrcpEventParamPair data(aEvent, aParam);

Nit: could you add an empty line before this one and remove the one on line 270.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +105,5 @@
> +      case AVRCP_EVENT_PLAY_POS_CHANGED:
> +        size = sizeof(mParam.mSongPos);
> +        break;
> +      case AVRCP_EVENT_APP_SETTINGS_CHANGED:
> +        size = sizeof(mParam.mIds) + sizeof(mParam.mValues) + sizeof(mParam.mNumAttr);

We are packing pairs of ids and values. So this should be '(sizeof(mParam.mIds[0]) + sizeof(mParam.mValues[0])) * mNumAttr'.
Attachment #8579178 - Flags: review?(tzimmermann) → review+
(Assignee)

Updated

3 years ago
Attachment #8579178 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8579253 [details] [diff] [review]
(v2.2) Bug 1142408 - Add data length parameter for Register Notification Response Command, r=tzimmermann
(Assignee)

Updated

3 years ago
Blocks: 1142417
(Assignee)

Comment 22

3 years ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbee172c2389
(Assignee)

Comment 23

3 years ago
Comment on attachment 8579253 [details] [diff] [review]
(v2.2) Bug 1142408 - Add data length parameter for Register Notification Response Command, r=tzimmermann

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 #): daemon feature
User impact if declined: Can not receive notifcation/fail to pass certification
Testing completed: Pass PTS test case
Risk to taking this patch (and alternatives if risky):Add missing parameter 
String or UUID changes made by this patch: None
Attachment #8579253 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 24

3 years ago
(In reply to Shawn Huang [:shawnjohnjr] from comment #22)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbee172c2389
Try server seems to be green.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/dc2322ecd24a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dc2322ecd24a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)

Updated

3 years ago
Attachment #8579253 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/744630454912
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → wontfix
You need to log in before you can comment on or make changes to this bug.