Add wbs_callback for bluetooth daemon IPC protocol

RESOLVED FIXED in FxOS-S6 (04Sep)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: shawnjohnjr, Assigned: ben.tian)

Tracking

unspecified
FxOS-S6 (04Sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(feature-b2g:2.2r+, b2g-v2.2r fixed, b2g-master fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

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.
<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)
(Assignee)

Comment 2

4 years ago
(Assignee)

Comment 4

4 years ago
Fix nit.
Attachment #8641544 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8641531 - Flags: review?(tzimmermann)
(Assignee)

Comment 5

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

Move this patch to bug 1189315.
Attachment #8641547 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → btian
(Assignee)

Comment 6

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

Will, please help verify whether this daemon patch works for WBS callback. Thanks.
Attachment #8641531 - Flags: feedback?(wiwang)
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.
Attachment #8641531 - Flags: review?(tzimmermann) → review+
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
Attachment #8641531 - Flags: feedback?(wiwang) → feedback+
(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!
Flags: needinfo?(tzimmermann)
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?
Flags: needinfo?(tzimmermann)
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/"
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?
(Assignee)

Comment 13

4 years ago
Will, any update? What's our plan and next step to do?
Flags: needinfo?(wiwang)
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 :)
Flags: needinfo?(wiwang)
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
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.
(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.
Patches applied

Re: [PATCH v2 0/3] Fix missing HFP WBS callback — Linux Bluetooth
http://www.spinics.net/lists/linux-bluetooth/msg63952.html
Attach patches applied by BlueZ here for reference.
Attachment #8650419 - Attachment is obsolete: true
Attach patches applied by BlueZ here for reference.
Attach patches applied by BlueZ here for reference.
Attach patches applied by BlueZ here for reference.
Hi Thomas,

Could you help to merge the pull request(comment 2) of Ben on Github?
Thanks!
Flags: needinfo?(tzimmermann)
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.
Flags: needinfo?(tzimmermann)
(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!
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2r: --- → affected
Flags: needinfo?(whuang)
Resolution: --- → FIXED
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
ni myself and Josh to discuss further for the gonk repo.
Flags: needinfo?(whuang)
Flags: needinfo?(jocheng)
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
status-b2g-v2.2r: affected → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → FxOS-S6 (04Sep)

Updated

4 years ago
Flags: needinfo?(jocheng)
Thanks Ryan!
Thank you Ryan :)
Flags: needinfo?(whuang)
You need to log in before you can comment on or make changes to this bug.