Closed Bug 1003591 Opened 7 years ago Closed 6 years ago

[MADAI][Bluetooth] Incoming a2dp connection failed

Categories

(Firefox OS Graveyard :: Bluetooth, defect, P3)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rapbong, Assigned: yrliou)

References

Details

(Whiteboard: [caf priority: p1][CR 714912][LibGLA, Mantis][POVB])

Attachments

(6 files)

User Agent: Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.1; Trident/4.0; CNS_UA; AD_LOGON=4C47452E4E4554; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; InfoPath.3; Tablet PC 2.0; .NET4.0E; CNS_UA; AD_LOGON=4C47452E4E4554)

Steps to reproduce:

1. Connected to the bluetooth headset
2. Disconnected the bluetooth headset
3. Try to connection by the bluetooth headset side


Actual results:

HFP connection was succeeded but A2DP connection has failed.


Expected results:

HFP & A2DP connection should success when Incoming connection request.
the assertion log was occurred.
Could you check this case?

556	01-01	12:51:05.309	V	217	558	AudioFlinger:		 setParameters(io 21, keyvalue closing=true, calling pid 217
557	01-01	12:51:05.309	E	221	2969	bt-btif		 : bta_av_rc_create ACP handle exist for shdl:0
558	01-01	12:51:05.309	V	217	558	AudioFlinger:		 ThreadBase::setParameters() closing=true
559	01-01	12:51:05.309	E	221	844	BTIF_AV		 : ### ASSERT : external/bluetooth/bluedroid/main/../btif/src/btif_av.c line 370 Callback is NULL (0) ###

Thank you for your support.
In the BluetoothA2dpManager.cpp

btav_callbacks_t callback function defines the below element.

static btav_callbacks_t sBtA2dpCallbacks = {
  sizeof(sBtA2dpCallbacks),
  A2dpConnectionStateCallback,
  A2dpAudioStateCallback
};

But, bluedroid uses connection_priority_cb.
(bt_av.h)
/** BT-AV callback structure. */
typedef struct {
    /** set to sizeof(btav_callbacks_t) */
    size_t      size;
    btav_connection_state_callback  connection_state_cb;
    btav_audio_state_callback audio_state_cb;
    btav_connection_priority_callback connection_priority_cb;
    btav_audio_focus_request_callback audio_focus_request_cb;
} btav_callbacks_t;

(btif_av.c)
        case BTA_AV_RC_OPEN_EVT:
            /* Check if connection allowed with this device */
            BTIF_TRACE_DEBUG0("Check A2dp priority of device");
            if (idle_rc_event != 0)
            {
                BTIF_TRACE_DEBUG0("Processing another RC Event ");
                return FALSE;
            }
            idle_rc_event = event;
            memcpy(&idle_rc_data, ((tBTA_AV*)p_data), sizeof(tBTA_AV));
            if (event == BTA_AV_RC_OPEN_EVT )
            {
                bdcpy(btif_av_cb.peer_bda.address, ((tBTA_AV*)p_data)->rc_open.peer_addr);
            }
            else
            {
                bdcpy(btif_av_cb.peer_bda.address, ((tBTA_AV*)p_data)->pend.bd_addr);
            }
            HAL_CBACK(bt_av_callbacks, connection_priority_cb, &(btif_av_cb.peer_bda));
            break;

Could you check this case?
Thanks.
blocking-b2g: --- → 1.4?
OS: All → Gonk (Firefox OS)
Priority: -- → P3
Hardware: All → ARM
Which branch of hardware/libhardware do you fetch from codeaurora? The assertion failure results from extra callback functions added by qcom [1][2] that mismatches the version used by B2G. However the assertion failure is not necessarily relevant to the bug symptom.

Can you capture sniffer/hcidump log as [3] to help us investigate the symptom? Also is the occurrence rate 100%?

[1] https://www.codeaurora.org/cgit/external/gigabyte/platform/hardware/libhardware/commit/include/hardware/bt_av.h?h=caf/kk_3.5_rb4.1&id=03b487823f473bbed527282cfb5e19bb3c13eb0a
[2] https://www.codeaurora.org/cgit/external/gigabyte/platform/hardware/libhardware/commit/include/hardware/bt_av.h?h=caf/kk_3.5_rb4.1&id=d59af8ede363c8c39f624a4b581785ffbe2d14f7
[3] https://wiki.mozilla.org/B2G/Bluetooth-bluedroid#Debugging:
Flags: needinfo?(rapbong)
Flags: needinfo?(rapbong)
Whiteboard: [LibGLA, Mantis]
Attached file btsnoop_hci.log
Attached file a2dp_conn_fail_hci.log
I attached snoop, and hci log.

I requested the used branch name.
If I receive the branch information then I will update the comment.

Thanks.
QA,

Please help with reproducing on Open C.
Keywords: qawanted
(In reply to Preeti Raghunath(:Preeti) from comment #8)
> QA,
> 
> Please help with reproducing on Open C.

Testing on OpenC is useless, because it uses JellyBean + bluez bluetooth stack. Bug reporter uses Kitkat + bluedroid bluetooth stack (probably CAF modified version, not AOSP version). Our reference design is all based on bluedroid (AOSP version).
Keywords: qawanted
Eric,

Can we please check if this is reproducible in house?
Flags: needinfo?(echou)
Hi Preeti,

We couldn't reproduce it right now since we don't have MADAI on hand.
Also, we need hardware/libhardware branch that was used as Comment4 mentioned.
If we get the branch, we can try to build a similar environment to try to reproduce the bug on nexus5.

Hi ILBEOM,
Any updates about the branch name?

Thanks,
Jocelyn
Flags: needinfo?(echou) → needinfo?(rapbong)
Assignee: nobody → joliu
Blocking this is this is basic STR for a shipping device and we progressing via logs etc to debug this since we don't have a device in hand.
blocking-b2g: 1.4? → 1.4+
I'm sorry that update is so late.
I received the answer by the project team.

The branch name is the confidential information.
So I requested this case to the Qualcomm.

Thank you for your support.
Flags: needinfo?(rapbong)
(In reply to ILBEOM KIM from comment #13)
> I'm sorry that update is so late.
> I received the answer by the project team.
> 
> The branch name is the confidential information.
> So I requested this case to the Qualcomm.
> 
> Thank you for your support.

Since it might be related to proprietary bluedroid modification based on Comment 13. So i suggest to have more concrete information for us to debug further, if it's really a gecko bug. Please feel free to re-nom 1.4+ if needed.
blocking-b2g: 1.4+ → ---
I had processed in the Qualcomm case and received the answer.

the connection_priority_cb already was added in the bluedroid, because MADAI uses the latest Blueroid stack.

But Gecko didn't handle the connection_priority_cb, So callback function has the null pointer.

I attach the Qualcomm answer to below.

------------------------------------------------------------------------------------------------------
Based on the functionality of connection_priority_cb, it just performed that connection is allowed from framework in checking mStateMachine when BTA_AV_PENDING_EVT/BTA_AV_RC_OPEN_EVT happened and then go ahead and connect by calling allow_connection in bta_av.c 
I believe that the connection priority function should be handled in jni and framework side because the stack code of MADIA is coming from latest bluedroid stack almost. Otherwise, the code should be regressive. 


Thanks.
blocking-b2g: --- → 1.4?
AFAIK connection_priority_cb is not part of AOSP HAL, Bruce can you help to provide suggestion?
There is no branch name to reproduce the issue. Also, the bluedroid bluetooth stack is probably CAF modified version. Although QC reported the stack code is coming from latest bluedroid stack "almost", it doesn't indicate connection_priority_cb is part of AOSP HAL as Comment 16 mentioned. Once again, our reference design is all based on the "official AOSP version". Please provide indication that the stack is AOSP version then re-nom if needed. Thanks.
blocking-b2g: 1.4? → ---
(In reply to ILBEOM KIM from comment #15)
> I had processed in the Qualcomm case and received the answer.
> 
> the connection_priority_cb already was added in the bluedroid, because MADAI
> uses the latest Blueroid stack.
> 
> But Gecko didn't handle the connection_priority_cb, So callback function has
> the null pointer.
> 
> I attach the Qualcomm answer to below.
> 
> -----------------------------------------------------------------------------
> -------------------------
> Based on the functionality of connection_priority_cb, it just performed that
> connection is allowed from framework in checking mStateMachine when
> BTA_AV_PENDING_EVT/BTA_AV_RC_OPEN_EVT happened and then go ahead and connect
> by calling allow_connection in bta_av.c 
> I believe that the connection priority function should be handled in jni and
> framework side because the stack code of MADIA is coming from latest
> bluedroid stack almost. Otherwise, the code should be regressive. 
> 
> 
> Thanks.

Thanks for sharing this information. I think there are two things we can discuss here:
(1). Does Mozilla want to remain Android HAL neutral? If we can accept non-neutral HAL changes we can have changes like on Bug 1004896 to introduce flags into gecko. If we don't want to have this kind of special flag to check bluedroid implementation, bluedroid btif_av shall have internal flag to enable/disable checking whether priority required to be considered. Although for me, it's like a bug, see (2).

(2). This device connection priority mechanism is for Android only. Default priority for devices that we try to auto-connect to and and allow incoming connections for the profile. Android bluedroid in JB/Kitkat seems does not check this part.
Because Android let users allow an incoming hfp/a2dp connection from Bluetooth Settings menu. If the user unselected a2dp profile (priority=0), bluedroid stack shall block the incoming a2dp connection. It's clear that bluedroid will auto-accept this a2dp connection even if a2dp profile had been un-selected with patch in Comment 18. However, b2g Settings application does not have this UX design. Eventually, we will receive connection_priority_cb callback and we have to directly call |allow_connection()| to accept incoming connection anyway inside gecko. Although this is not necessary since we don't have connection priority for Settings app.

So if we can accept things to allow gecko has a specific CAF bluedroid flag to choose the different HAL interface, we can do (2) stuff.
I hope this can help decision making and please correct me if there is anything wrong.
Thanks.
Dear Mozilla Engineer.

I agree your opinion.
The connection priority check is used in the android scenario.
So FFOS will always try to connect when receive the connection_priority_cb.

For resolved this case, we need to use (2) method because MADAI uses the bluedroid modified by qualcomm.

Could you release the flag patch?
Attachment #8419885 - Attachment description: gonk_misc.patch → Add CAF bluedroid flag and expose env variables from gonk-misc
Attachment #8419885 - Attachment description: Add CAF bluedroid flag and expose env variables from gonk-misc → Add CAF bluedroid flag and expose env variables from gonk-misc (Patch 1)
You need to add flag in your BoardConfig.mk with Patch 1, BOARD_HAVE_BLUETOOTH_BLUEDROID_CAF := true
Attachment #8419886 - Attachment description: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 2) → Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 2)
Please help understand if CAF is looking at this?
Flags: needinfo?(ikumar)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25)
> Created attachment 8419948 [details] [diff] [review]
> Bug 1003591 - Allow a2dp incoming connection (Patch 3)

oh...you probably need to replace allowConnection to allow_connection, because I tested CAF on JB branch.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #26)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25)
> > Created attachment 8419948 [details] [diff] [review]
> > Bug 1003591 - Allow a2dp incoming connection (Patch 3)
> 
> oh...you probably need to replace allowConnection to allow_connection,
> because I tested CAF on JB branch.

I will check the patch and update to result.

Thanks.
(In reply to ILBEOM KIM from comment #27)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #26)
> > (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25)
> > > Created attachment 8419948 [details] [diff] [review]
> > > Bug 1003591 - Allow a2dp incoming connection (Patch 3)
> > 
> > oh...you probably need to replace allowConnection to allow_connection,
> > because I tested CAF on JB branch.
> 
> I will check the patch and update to result.
> 
> Thanks.

Please make sure you delete objdir-gecko to rebuild gecko.
Comment on attachment 8419886 [details] [diff] [review]
Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 2)

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

It seems like we cannot assign nullptr here.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +495,1 @@
>  

+#ifdef MOZ_B2G_BT_BLUEDROID_CAF
+static void
+WakeStateChangedCallback(bt_state_t state)
+{
+}
+#endif

@@ +630,5 @@
>    RemoteDevicePropertiesCallback,
>    DeviceFoundCallback,
>    DiscoveryStateChangedCallback,
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +  nullptr,

WakeStateChangedCallback,
This issue has been fixed in CAF for LF builds, see:[1a][1b]
If you sync to AU86+ [2] then you shouldn't see this issue.
So, the changes in this bug are not needed.

Thanks Preeti for checking.

[1a] https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/commit/?h=LNX.LF.3.5.2.2&id=b11effcb128e22dc3090aed9afdf7afa2a270005
[1b] https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/bluedroid/commit/?h=b2g_kk_3.5&id=4494d0952636c490b9bfd8c71105be8f9edc13ba
[2] https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tag/?id=AU_LINUX_GECKO_B2G_KK_3.5.01.04.00.113.086
Flags: needinfo?(shuang)
Flags: needinfo?(rapbong)
Flags: needinfo?(ikumar)
(In reply to Inder from comment #30)
> This issue has been fixed in CAF for LF builds, see:[1a][1b]
> If you sync to AU86+ [2] then you shouldn't see this issue.
> So, the changes in this bug are not needed.
But in btif_av.c, Does connection_priority_cb still need to be added?
Can we have a flag for btif_av.h and btif_av.c?
Flags: needinfo?(shuang)
Greg -- can you please comment.
Flags: needinfo?(ikumar) → needinfo?(ggrisco)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #31)
> (In reply to Inder from comment #30)
> > This issue has been fixed in CAF for LF builds, see:[1a][1b]
> > If you sync to AU86+ [2] then you shouldn't see this issue.
> > So, the changes in this bug are not needed.
> But in btif_av.c, Does connection_priority_cb still need to be added?
> Can we have a flag for btif_av.h and btif_av.c?

Yeah, this doesn't cause a compilation error because, I guess, the new callback was added to the end of the list.  But if it's causing runtime issue then I think we need to conditionally compile it like we did with the other bluetooth.h callbacks.  I'll take a look.
Flags: needinfo?(ggrisco)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #28)
> (In reply to ILBEOM KIM from comment #27)
> > (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #26)
> > > (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25)
> > > > Created attachment 8419948 [details] [diff] [review]
> > > > Bug 1003591 - Allow a2dp incoming connection (Patch 3)
> > > 
> > > oh...you probably need to replace allowConnection to allow_connection,
> > > because I tested CAF on JB branch.
> > 
> > I will check the patch and update to result.
> > 
> > Thanks.
> 
> Please make sure you delete objdir-gecko to rebuild gecko.

Dear Mozilla Engineer.

I tested the patch, and modified some codes.

1. I added the below macro in the configure.in
AC_SUBST(MOZ_B2G_BT_BLUEDROID_CAF)
If remove the above macro, then "if CONFIG['MOZ_B2G_BT_BLUEDROID_CAF']:" condition isn't adapted.

2.  if test -n "${BOARD_HAVE_BLUETOOTH_BLUEDROID_CAF}"; then
    ===>  if test -n "${BOARD_HAVE_BLUEDROID_CAF}"; then

I tested the patch, and a2dp connection failed wasn't reproduced.

Thank you for your support.
Flags: needinfo?(rapbong)
Comment on attachment 8419886 [details] [diff] [review]
Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 2)

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

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +630,5 @@
>    RemoteDevicePropertiesCallback,
>    DeviceFoundCallback,
>    DiscoveryStateChangedCallback,
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +  nullptr,

Simply assign the fields by value, like this

static struct s {
  .field = FieldFunc
};

All unassigned fields will be set to zero automatically. As long as you're not going to use them, you won't need these preprocessor defines and related build support.
Yeah we could add a Q_BLUETOOTH flag here.  This is not a high priority bug though, based on the current flags so we are unlikely to prioritize it.
Flags: needinfo?(mvines)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #37)
> Yeah we could add a Q_BLUETOOTH flag here.  This is not a high priority bug
> though, based on the current flags so we are unlikely to prioritize it.
I wonder CAF has additional patch on Gecko for this bug. Otherwise, a2dp incoming connection shall always fail. Honestly, I really hope you can add Q_BLUETOOTH for btav_connection_priority_callback. Without the flag, we have to introduce CAF flag ourself. It really doesn't make sense to fail CAF build.
CAF flags or any vendor specific compile time flags are an automatic r-. Please assign fields by value as mentioned in comment 35. If you have any future need for this and can't find a good solution, let me know.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #35)
> Simply assign the fields by value, like this
> 
> static struct s {
>   .field = FieldFunc
> };
> 

Unfortunately designated initializers aren't supported in c++, but the equivalent can still be done. Just without the nice syntax.
Bhargav is checking comment 38 over here.  If this is causing issues we will change the CAF HAL for B2G to avoid the need for a Gecko CAF flag
Flags: needinfo?(bhargavg1)
(In reply to Michael Wu [:mwu] from comment #39)
> CAF flags or any vendor specific compile time flags are an automatic r-.
> Please assign fields by value as mentioned in comment 35. If you have any
> future need for this and can't find a good solution, let me know.
Thanks mwu. It's clear that we will talk to :m1 if there is any non-AOSP interface. :)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #41)
> Bhargav is checking comment 38 over here.  If this is causing issues we will
> change the CAF HAL for B2G to avoid the need for a Gecko CAF flag

Shawn, our internal test teams dont see any issue while testing the STR mentioned in comment 0

Let me loop back to you on the changes needed for CAF
Flags: needinfo?(bhargavg1)
Flags: needinfo?(bhargavg1)
Whiteboard: [LibGLA, Mantis] → [CR 714912][LibGLA, Mantis]
Whiteboard: [CR 714912][LibGLA, Mantis] → [caf priority: p2][CR 714912][LibGLA, Mantis]
Moving ni to Vasanth who is working on this feature
Flags: needinfo?(bhargavg1) → needinfo?(vasanth)
Sorry for the confusion. 
I see this issue is reproducible from our side as well. 
Based on that what we need to do?

Shawn,
I guess you are adding support for A2dpConnectionPriorityCallback with a Gecko CAF flag?
That should fix this issue right?
Then why do we need Q_BLUETOOTH flag? 
If needed where to add it?
Flags: needinfo?(shuang)
Flags: needinfo?(vasanth)
(In reply to vasanth from comment #45)
> I guess you are adding support for A2dpConnectionPriorityCallback with a
> Gecko CAF flag?
> That should fix this issue right?
> Then why do we need Q_BLUETOOTH flag? 
> If needed where to add it?

A2dpConnectionPriorityCallback should be wrapped with a Q_BLUETOOTH flag so that adding a CAF flag to Gecko can be avoided.
Per Comment 46 answer, cancel ni flag.
Flags: needinfo?(shuang)
Whiteboard: [caf priority: p2][CR 714912][LibGLA, Mantis] → [caf priority: p1][CR 714912][LibGLA, Mantis]
Whiteboard: [caf priority: p1][CR 714912][LibGLA, Mantis] → [caf priority: p1][CR 714912][LibGLA, Mantis][POVB]
Any target date for this bug? Which branch will be fixed this issue?
Flags: needinfo?(bhuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #48)
> Any target date for this bug? Which branch will be fixed this issue?

for v2.0 the change would land @https://www.codeaurora.org/cgit/external/gigabyte/platform/external/bluetooth/bluedroid/log/?h=caf/b2g_kk_3.5

Change is merged internally it should show up externally in a day/two
(In reply to Shawn Huang [:shawnjohnjr] from comment #48)
> Any target date for this bug? Which branch will be fixed this issue?

Change is available at [1]  for v2.0

[1] https://www.codeaurora.org/cgit/external/gigabyte/platform/external/bluetooth/bluedroid/commit/?h=caf/b2g_kk_3.5&id=5a71145c850dda353b7d93805f53a732f15febdf
Fix is available at [1]  for v2.0

for v2.1 would be available [2], wait for the changes to land externally

[1] https://www.codeaurora.org/cgit/external/gigabyte/platform/external/bluetooth/bluedroid/commit/?h=caf/b2g_kk_3.5&id=5a71145c850dda353b7d93805f53a732f15febdf 

[2] https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/bluedroid/log/?h=LNX.LF.3.5.1_RB1.2
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
How long usually will this patch set be merged take?
Flags: needinfo?(bhargavg1)
(In reply to Shawn Huang [:shawnjohnjr] from comment #52)
> How long usually will this patch set be merged take?

v2.1 Changes at, 

platform/external/bluetooth/bluedroid - https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/bluedroid/commit/?h=LNX.LF.3.5.1_RB1.2&id=949f774c77d25185d664ab4e7d8abf7fc78fe94a

platform/hardware/libhardware - https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/commit/?h=LNX.LF.3.5.1_RB1.2&id=dbbce4d60675b973aa6d40a07994c578e2067082
Flags: needinfo?(bhargavg1)
You need to log in before you can comment on or make changes to this bug.