Closed Bug 1142408 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.2 S8 (20mar)
blocking-b2g 2.2+
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: shinglyu, Assigned: shawnjohnjr)

References

Details

Attachments

(2 files, 3 obsolete files)

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
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).
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).
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
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
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
I've tested notification related test cases, NFY_BV_02, 04, 05, 08, these test cases are pass.
Attachment #8578041 - Flags: review?(tzimmermann)
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-
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)
(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)
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-
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.
(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.
blocking-b2g: 2.2? → 2.2+
Attachment #8579178 - Attachment description: bug1142408.patch → Bug 1142408 - Add data length parameter for Register Notification Response Command
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+
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?
(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.
https://hg.mozilla.org/mozilla-central/rev/dc2322ecd24a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
Attachment #8579253 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.