1189292 - Add wbs_callback for bluetooth daemon IPC protocol

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

BlueZ 5.32 is missing hf_wbs_callback notification implementation, after consulting :sjanc, he suggested to add callback to tail, so the opcode shall be Opcode 0x91 in theory for wbs_callback.

Comment 1

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

<sjanc> shawnjohnjr_____: hmm it looks like we miss that callback implementation ;/
<sjanc> shawnjohnjr_____: configure_wbs is implemented, but callback is not
<sjanc> shawnjohnjr_____: would like to send patches for this?:)
<shawnjohnjr_____> sjanc: i'm interested to know if we want to add it back, which opcode we should use?
<shawnjohnjr_____> sjanc: would it be 0x8a? or add it to tail?
<shawnjohnjr_____> sjanc: the last one is Opcode 0x90 - Key Pressed Command notification
<sjanc> shawnjohnjr_____: tail,  and keep in mind that libhal must compile on KK where this is not present,  so libhal part must be #if ANDROID_VERSION >= PLATFORM_VER(5, 0, 0)

Comment 2

[Ben Tian (inactive)]

Attachment: WBS callback (daemon part)

Comment 3

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): WBS notification (gecko part)

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 1 (v2): WBS notification (gecko part)wbs.patch

Fix nit.

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8641547 [details] [diff] [review]
Patch 1 (v2): WBS notification (gecko part)wbs.patch

Move this patch to bug 1189315.

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8641531 [details] [review]
WBS callback (daemon part)

Will, please help verify whether this daemon patch works for WBS callback. Thanks.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8641531 [details] [review]
WBS callback (daemon part)

Thanks, there's just a minor comment on style.

Just like for the Gecko side, please don't land the patch until BlueZ has it in their IPC spec.

Comment 8

[Will Wang [:WillWang]]

Comment on attachment 8641531 [details] [review]
WBS callback (daemon part)

Hi Ben,

Verification is passed in PTS tests[1] and parameter is correctly received by audio manager, Thanks for your patch!

[1]
TC_AG_WBS_BV_01_I
TC_AG_SLC_BV_05_I
TC_AG_SLC_BV_06_I

Comment 9

[Will Wang [:WillWang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> Comment on attachment 8641531 [details] [review]
> WBS callback (daemon part)
> 
> Thanks, there's just a minor comment on style.
> 
> Just like for the Gecko side, please don't land the patch until BlueZ has it
> in their IPC spec.

Hi Thomas,

Since our WBS feature implementation had passed the test, and HFP features of Red-tai need to be landed by end of August,
so we are considering to land the WBS feature to branch 2.2r first, meanwhile, write/submit the bluez patch then land our gecko patches to m-c.

Would you mind telling me any concern if you have?
Thanks!

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Will,

How far is the BlueZ patch? I'd like to get some feedback from the BlueZ devs first, especially on the IPC protocol. If BlueZ has a different IPC in their spec, we are incompatible with the spec and I'd like to avoid that.

Can someone reach out to BlueZ and ask for a confirmation on the IPC at least?

Comment 11

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

Hi Will,
You need to fetch code following [1], and compile kernel yourself to catch up the new BlueZ 5 stack.
Build commands:
source build/envsetup.sh
lunch aosp_hammerhead-userdebug

[1] "https://code.google.com/p/aosp-bluez/"

Comment 12

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

Hi Will,
After looking around the handler, i think this is something we can add for wbs callback:
1. Add wbs related code in hal-msg.h
2. Add wbs handler in hal-handsfree.c
3. Add wbs related call in |at_cmd_bcs| to call |ipc_send_notif| in handsfree.c
4. Add wbs documentation in hal-ipc-api.txt.

I have not yet get answer regarding opcode. But last time, sjanc said "<sjanc> shawnjohnjr_____: tail,  and keep in mind that libhal must compile on KK where this is not present,  so libhal part must be #if ANDROID_VERSION >= PLATFORM_VER(5, 0, 0).


21:31:57 <shawnjohnjr_____> sjanc^: if I need to add android hal ipc for callback 'hf_wbs_callback', i just need to add one extra hal_ipc_handler and call |ipc_send_notif| in |at_cmd_bcs|, right?
21:33:04 <sjanc^> shawnjohnjr_____: yes, but remember to update ipc doumentation as well
21:52:47 <shawnjohnjr_____> sjanc^: confirm again that opcode shall be HAL_EV_HANDSFREE_HSP_WBS 0x91?

Comment 13

[Ben Tian (inactive)]

Will, any update? What's our plan and next step to do?

Comment 14

[Will Wang [:WillWang]]

Attachment: 0001-android-handsfree-Fix-missing-WBS-callback.patch

Attach patch here for reference, I am going to split patch and follow the BlueZ rule to submit.

Patch tested by PTS "TC_AG_ACC_BI_12_I".

IRC log for your info:

<WillWang_> Hi, I had already made a patch which can fix the HFP WBS callback for Android
<sjanc> WillWang_: nice
<WillWang_> sjanc: Could you possibly tell me how to properly send this patch to bluez? thanks a lot :)
<sjanc> WillWang_: when sending it to ML, please split it to  hal part, daemon part and documentation part
<sjanc> so it should be like 3 patches
<sjanc> WillWang_: formatted with git format-patch,  check HACKING file in bluez git
<sjanc> WillWang_: also please prefix then with android in commit message,   I suggest checking git log on those files and keep that similar
<sjanc> ie android/handsfree:  android/hal-handsfree:  etc
<WillWang_> ok, via shawnjohnjr's help, I had prefixed that
<sjanc> WillWang_: in worst case you will have to send version 2 of your patches, so don't worry :)
<WillWang_> sjanc: Thanks for your advice! I am going to follow HACKING and split my patch :)
<WillWang_> Thanks again for your speedy reply :)

Comment 15

[Will Wang [:WillWang]]

Three patches have been sent to BlueZ mailing list[1] to review,
and refined again[2] for compilation on Android KitKat.

These patches follow the coding style and commit style defined by Linux kernel and BlueZ.

[1]
[PATCH 0/3] Fix missing HFP WBS callback — Linux Bluetooth
http://www.spinics.net/lists/linux-bluetooth/msg63788.html

[2]
[PATCH v2 0/3] Fix missing HFP WBS callback — Linux Bluetooth
http://www.spinics.net/lists/linux-bluetooth/msg63839.html

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks a lot, Will! Your work is greatly appreciated. Since the documentation patches landed and the others are on track, it should be safe to continue with our own WBS support as well.

Comment 17

[Will Wang [:WillWang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16)
> Thanks a lot, Will! Your work is greatly appreciated. Since the
> documentation patches landed and the others are on track, it should be safe
> to continue with our own WBS support as well.

Thank you Thomas :)
and I am sending patch of Bug 911635 (which is the last piece of our WBS support) for reviewing,
I'll keep you posted if any.

Comment 18

[Will Wang [:WillWang]]

Patches applied

Re: [PATCH v2 0/3] Fix missing HFP WBS callback — Linux Bluetooth
http://www.spinics.net/lists/linux-bluetooth/msg63952.html

Comment 19

[Will Wang [:WillWang]]

Attachment: [PATCH v2 0/3] Fix missing HFP WBS callback

Attach patches applied by BlueZ here for reference.

Comment 20

[Will Wang [:WillWang]]

Attachment: [PATCH v2 1/3] android/hal-ipc-api: Fix missing HFP WBS callback

Attach patches applied by BlueZ here for reference.

Comment 21

[Will Wang [:WillWang]]

Attachment: [PATCH v2 2/3] android/hal-handsfree: Fix missing HFP WBS callback

Attach patches applied by BlueZ here for reference.

Comment 22

[Will Wang [:WillWang]]

Attachment: [PATCH v2 3/3] android/handsfree: Fix missing HFP WBS callback

Attach patches applied by BlueZ here for reference.

Comment 23

[Will Wang [:WillWang]]

Hi Thomas,

Could you help to merge the pull request(comment 2) of Ben on Github?
Thanks!

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Merged at

https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/fd241573c00112ca6e456b8021fb68df0349f6dd

But usually I prefer to set 'checkin-needed' and let the Sheriffs handle the merge. They monitor the builds and probably want to know what when in if something breaks.

Comment 25

[Will Wang [:WillWang]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> Merged at
> 
> https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/
> fd241573c00112ca6e456b8021fb68df0349f6dd
> 
> But usually I prefer to set 'checkin-needed' and let the Sheriffs handle the
> merge. They monitor the builds and probably want to know what when in if
> something breaks.

Thanks for your suggestion and merge :)

Set resolved fixed since m-c landed.

Set status-b2g-v2.2r as affected and ni EPM Wesley since Github repo "platform_system_bluetoothd" currently have no branch for 2.2r

Hi Wesley,

Could you help to handle this for 2.2r?
Thanks!

Comment 26

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

ni myself and Josh to discuss further for the gonk repo.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.2r: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/55665894465a46a82ab15a73883f66b46ed26f70

And an update to b2g-manifest to actually make use of said new branch :)
v2.2r: https://github.com/mozilla-b2g/b2g-manifest/commit/f2f11231ef413bc1fa2a899a38436650df5bf7fd

Comment 28

[Josh Cheng [:josh]]

Thanks Ryan!

Comment 29

[Will Wang [:WillWang]]

Thank you Ryan :)