Closed Bug 1091588 Opened 10 years ago Closed 10 years ago

[Bluetooth] Implement HFP support in daemon backend

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.2+)

Status:
RESOLVED FIXED
Milestone:
2.2 S1 (5dec)
Project Flags:
feature-b2g 2.2+

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(4 files, 8 obsolete files)

[01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support (v2)
24.40 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[02] Bug 1091588: Add Handsfree module for Bluetooth daemon (v2)
41.48 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[04] Bug 1091588: Support Handsfree profile when using Bluetooth daemon (v2)
6.64 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1091588: Add Handsfree interface for Bluetooth daemon (v2)
11.77 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
No description provided.
With the patch set for bug 1087195, I'm able to use a Bluetooth Headset and the daemon. I'd like to wait a bit with the review until bluetoothd has been merged.
Blocks: 1065336
No longer blocks: 1005934
Attachment #8518903 - Attachment is obsolete: true
Attachment #8523835 - Flags: review?(shuang)
Attachment #8523835 - Flags: feedback?(btian)
Attachment #8518904 - Attachment is obsolete: true
Attachment #8523836 - Flags: review?(shuang)
Attachment #8523836 - Flags: feedback?(btian)
Attachment #8518906 - Attachment is obsolete: true
Attachment #8523837 - Flags: review?(shuang)
Attachment #8523837 - Flags: feedback?(btian)
Attachment #8518907 - Attachment is obsolete: true
Attachment #8523839 - Flags: review?(shuang)
Attachment #8523839 - Flags: feedback?(btian)
Hi, while the daemon is being added to the repositories, here are some patches for supporting HFP.
feature-b2g: --- → 2.2+
Comment on attachment 8523835 [details] [diff] [review] [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support Review of attachment 8523835 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp @@ +942,5 @@ > + // We get a pointer to the first character in the PDU, a length > + // of 1 ensures we consume the \0 byte. With 'str' pointing to > + // the string in the PDU, we can copy the actual bytes. > + > + const char* str = reinterpret_cast<const char*>(aPDU.Consume(1)); Why is there a \0 byte at the beginning of PDU?
Comment on attachment 8523836 [details] [diff] [review] [02] Bug 1091588: Add Handsfree module for Bluetooth daemon Review of attachment 8523836 [details] [diff] [review]: ----------------------------------------------------------------- f=me with nit addressed. ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.h @@ +28,5 @@ > + OPCODE_DISCONNECT = 0x02, > + OPCODE_CONNECT_AUDIO = 0x03, > + OPCODE_DISCONNECT_AUDIO = 0x04, > + OPCODE_START_VOICE_RECOGNITION = 0x05, > + OPCODE_STOP_VOICE_RECOGNITION =0x06, nit: space after =
Attachment #8523836 - Flags: feedback?(btian) → feedback+
Comment on attachment 8523837 [details] [diff] [review] [03] Bug 1091588: Add Handsfree interface for Bluetooth daemon Review of attachment 8523837 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8523837 - Flags: feedback?(btian) → feedback+
Comment on attachment 8523835 [details] [diff] [review] [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support Review of attachment 8523835 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp @@ +124,5 @@ > } > aOut = sBondState[aIn]; > return NS_OK; > } > Maybe I missed something? I did not see any Convert function for converting HFP_AT_RESPONSE_OK, HFP_AT_RESPONSE_ERROR, HFP_CALL_STATE_*, etc.
Comment on attachment 8523836 [details] [diff] [review] [02] Bug 1091588: Add Handsfree module for Bluetooth daemon Review of attachment 8523836 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp @@ +58,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_CONNECT, 6)); Please define constant BDADDR_SIZE = 6 for all the functions required bd address as the parameter. @@ +146,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_START_VOICE_RECOGNITION, 0)); I checked hal-ipc-api.txt, it seems the format with remote address. So, it shall be 6 not 0 for payload size, right? Opcode 0x05 - Start Voice Recognition command/response Command parameters: Remote address (6 octets) Response parameters: <none> In case of an error, the error response will be returned. @@ +163,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_STOP_VOICE_RECOGNITION, 0)); Ditto. payload is 6. @@ +181,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_VOLUME_CONTROL, 2)); payload size is 8, right? Opcode 0x07 - Volume Control command/response Command parameters: Volume type (1 octet) Volume (1 octet) Remote address (6 octets)
Comment on attachment 8523839 [details] [diff] [review] [04] Bug 1091588: Support Handsfree profile when using Bluetooth daemon Review of attachment 8523839 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. Thanks. ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp @@ +1369,5 @@ > + BluetoothSetupResultHandler* aRes) MOZ_OVERRIDE; > + > + nsresult UnregisterModule(uint8_t aId, > + BluetoothSetupResultHandler* aRes) MOZ_OVERRIDE; > + Replace |mProtocol->RegisterModuleCmd| with |RegisterModule| in the following since the 2 functions become public. [1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1640 [2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1678 [3] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1786 [4] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1805
Attachment #8523839 - Flags: feedback?(btian) → feedback+
Comment on attachment 8523836 [details] [diff] [review] [02] Bug 1091588: Add Handsfree module for Bluetooth daemon Review of attachment 8523836 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp @@ +58,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_CONNECT, 6)); Sorry, the parameter changes only for lollipop. @@ +146,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_START_VOICE_RECOGNITION, 0)); Oh. Sorry, it seems i looked up the latest hal-ipc-api.txt, and that based on lollipop version which excludes the remote address field. @@ +163,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_STOP_VOICE_RECOGNITION, 0)); Sorry. Lollipop change only. @@ +181,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_VOLUME_CONTROL, 2)); Oh. Sorry, it seems i looked up the latest hal-ipc-api.txt, and that based on lollipop version which excludes the remote address field. @@ +227,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_COPS_RESPONSE, 0)); *Lollipop change: Opcode 0x09 - COPS Response command/response Command parameters: COPS command response (string) Remote address (6 octets) @@ +251,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_CIND_RESPONSE, 7)); Reminder: *Lollipop change, PDU size will be 13. Command parameters: Service (1 octet) Number of active calls (1 octet) Number of held calls (1 octet) Call setup state (1 octet) Signal strength (1 octet) Roaming indicator (1 octet) Battery level (1 octet) Remote address (6 octets) @@ +278,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_FORMATTED_AT_RESPONSE, 0)); *Lollipop change: Opcode 0x0b - Formatted AT Response command/response Command parameters: Pre-formatted AT response (string) Remote address (6 octets) @@ +300,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_AT_RESPONSE, 2)); *Lollipop: Opcode 0x0c - AT Response command/response Command parameters: Response code (1 octet) Error code (1 octet) Remote address (6 octets) @@ +330,5 @@ > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_CLCC_RESPONSE, 0)); > + > + nsresult rv = PackPDU(PackConversion<int, uint8_t>(aIndex), > + aDir, aState, aMode, aMpty, aType, > + PackCString0(NS_ConvertUTF16toUTF8(aNumber)), *pdu); Reminder lollipop changes: Opcode 0x0d - CLCC Response command/response Command parameters: Call index (1 octet) Call direction (1 octet) Call state (1 octet) Call mode (1 octet) Call multiparty type (1 octet) Call number type (1 octet) Call number (string) Remote address (6 octets)
Hi > Oh. Sorry, it seems i looked up the latest hal-ipc-api.txt, and that based > on lollipop version which excludes the remote address field. I currently use version 5.24 [1] of the HAL protocol. It's compatible with JB and KK. I don't have any means for testing compatibility with L, so it seems better to stick to older versions for now. L will be supported as soon as I can build for it. [1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt?id=5.24
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #18) > Hi > > > Oh. Sorry, it seems i looked up the latest hal-ipc-api.txt, and that based > > on lollipop version which excludes the remote address field. > > I currently use version 5.24 [1] of the HAL protocol. It's compatible with > JB and KK. I don't have any means for testing compatibility with L, so it > seems better to stick to older versions for now. L will be supported as soon > as I can build for it. > > [1] > https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api. > txt?id=5.24 Sure. You can ignore all my comments related to L interfaces.
(In reply to Ben Tian [:btian] from comment #11) > Comment on attachment 8523835 [details] [diff] [review] > [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support > > Review of attachment 8523835 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp > @@ +942,5 @@ > > + // We get a pointer to the first character in the PDU, a length > > + // of 1 ensures we consume the \0 byte. With 'str' pointing to > > + // the string in the PDU, we can copy the actual bytes. > > + > > + const char* str = reinterpret_cast<const char*>(aPDU.Consume(1)); > > Why is there a \0 byte at the beginning of PDU? It's not at the beginning. The PDU contains a 0-terminated string. We consume 1 byte here and the number of characters in the string later. At the end, we consumed all string characters and the trailing \0 character. Consuming exactly 1 byte at the beginning will return a pointer to the string's beginning and also verify (within |Consume|) that we haven't already reached the end of the PDU's data.
Comment on attachment 8523835 [details] [diff] [review] [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support Review of attachment 8523835 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h @@ +347,5 @@ > + > +/* This implementation of |PackPDU| packs a 0-terminated C string. > + */ > +inline nsresult > +PackPDU(const PackCString0& aIn, BluetoothDaemonPDU& aPDU) I'm a bit confused here, can't we use nsCString directly for |aIn|? Is it necessary to pack it in another structure?
(In reply to Ben Tian [:btian] from comment #16) > Comment on attachment 8523839 [details] [diff] [review] > [04] Bug 1091588: Support Handsfree profile when using Bluetooth daemon > > Review of attachment 8523839 [details] [diff] [review]: > ----------------------------------------------------------------- > > f=me with comment addressed. Thanks. > > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp > @@ +1369,5 @@ > > + BluetoothSetupResultHandler* aRes) MOZ_OVERRIDE; > > + > > + nsresult UnregisterModule(uint8_t aId, > > + BluetoothSetupResultHandler* aRes) MOZ_OVERRIDE; > > + > > Replace |mProtocol->RegisterModuleCmd| with |RegisterModule| in the > following since the 2 functions become public. |RegisterModuleCmd| is the correct method here. |RegisterModule| implements a virtual method for base classes, so that modules (HFP, A2DP, etc) have a way to register themselves.
(In reply to Shawn Huang [:shawnjohnjr] from comment #21) > Comment on attachment 8523835 [details] [diff] [review] > [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support > > Review of attachment 8523835 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h > @@ +347,5 @@ > > + > > +/* This implementation of |PackPDU| packs a 0-terminated C string. > > + */ > > +inline nsresult > > +PackPDU(const PackCString0& aIn, BluetoothDaemonPDU& aPDU) > > I'm a bit confused here, can't we use nsCString directly for |aIn|? Is it > necessary to pack it in another structure? Using |nsCString| is a bit problematic, because there is no clean handling of strings in the HAL protocol. Sometimes they are 0-terminated, sometimes they are transfered with length and characters. I added this structure to distinguish these cases. |PackCString0| signals \0-terminated strings. We can add helper structures for other cases if we need them. The helper structure is lightweight, so I don't worry about overhead here. The compiler will probably inline the function anyway.
> > > Maybe I missed something? I did not see any Convert function for converting > HFP_AT_RESPONSE_OK, HFP_AT_RESPONSE_ERROR, HFP_CALL_STATE_*, etc. Looks like a bug. :( A pass their types to |PackPDU|, but forgot to implement the corresponding conversion. I guess the compiler picked a compatible function instead. Thanks for spotting this problem.
Comment on attachment 8523835 [details] [diff] [review] [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support Review of attachment 8523835 [details] [diff] [review]: ----------------------------------------------------------------- Only some |Convert| functions need to be added.
Attachment #8523835 - Flags: review?(shuang)
Comment on attachment 8523835 [details] [diff] [review] [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support f=me with |Convert| functions (comment 25) added. Thanks for comment 20 explanation.
Attachment #8523835 - Flags: feedback?(btian) → feedback+
Comment on attachment 8523836 [details] [diff] [review] [02] Bug 1091588: Add Handsfree module for Bluetooth daemon Review of attachment 8523836 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I have only one question about payload size. ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp @@ +278,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_FORMATTED_AT_RESPONSE, 0)); So if PDU command parameters contain string type such as 'Call number (string)' here, payload size will be 0 for optimization? @@ +326,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_CLCC_RESPONSE, 0)); So if PDU command parameters contain string type such as 'Call number (string)' here, payload size will be 0 for optimization? @@ +351,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_PHONE_STATE_CHANGE, 0)); So if PDU command parameters contain string type such as 'Call number (string)' here, payload size will be 0 for optimization?
Attachment #8523836 - Flags: review?(shuang)
Comment on attachment 8523837 [details] [diff] [review] [03] Bug 1091588: Add Handsfree interface for Bluetooth daemon Review of attachment 8523837 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8523837 - Flags: review?(shuang) → review+
Comment on attachment 8523839 [details] [diff] [review] [04] Bug 1091588: Support Handsfree profile when using Bluetooth daemon Review of attachment 8523839 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp @@ +1469,5 @@ > const BluetoothDaemonPDUHeader&, BluetoothDaemonPDU&, void*) = { > INIT_ARRAY_AT(0x00, &BluetoothDaemonProtocol::HandleSetupSvc), > INIT_ARRAY_AT(0x01, &BluetoothDaemonProtocol::HandleCoreSvc), > + INIT_ARRAY_AT(0x02, &BluetoothDaemonProtocol::HandleSocketSvc), > + INIT_ARRAY_AT(0x03, nullptr), Add comment /* HID Host */ @@ +1470,5 @@ > INIT_ARRAY_AT(0x00, &BluetoothDaemonProtocol::HandleSetupSvc), > INIT_ARRAY_AT(0x01, &BluetoothDaemonProtocol::HandleCoreSvc), > + INIT_ARRAY_AT(0x02, &BluetoothDaemonProtocol::HandleSocketSvc), > + INIT_ARRAY_AT(0x03, nullptr), > + INIT_ARRAY_AT(0x04, nullptr), Add comment /* PAN */
Attachment #8523839 - Flags: review?(shuang) → review+
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp > @@ +278,5 @@ > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + nsAutoPtr<BluetoothDaemonPDU> pdu( > > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_FORMATTED_AT_RESPONSE, 0)); > > So if PDU command parameters contain string type such as 'Call number > (string)' here, payload size will be 0 for optimization? It's just the default. In contrast to the daemon, the Gecko implementation can resize the PDU's payload buffers. When I did not know the exact length beforehand, I just put in 0 here and let the PDU's |Write| method allocate the required buffer.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #23) > (In reply to Shawn Huang [:shawnjohnjr] from comment #21) > > Comment on attachment 8523835 [details] [diff] [review] > Using |nsCString| is a bit problematic, because there is no clean handling > of strings in the HAL protocol. Sometimes they are 0-terminated, sometimes > they are transfered with length and characters. I added this structure to > distinguish these cases. > If we want to distinguish these cases (0-terminated or not), could we use 'typedef' instead? But yeah, you're right, the overhead is small.
(In reply to Shawn Huang [:shawnjohnjr] from comment #31) > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #23) > > (In reply to Shawn Huang [:shawnjohnjr] from comment #21) > > > Comment on attachment 8523835 [details] [diff] [review] > > Using |nsCString| is a bit problematic, because there is no clean handling > > of strings in the HAL protocol. Sometimes they are 0-terminated, sometimes > > they are transfered with length and characters. I added this structure to > > distinguish these cases. > > > If we want to distinguish these cases (0-terminated or not), could we use > 'typedef' instead? This won't work because 'typedef' doesn't declare a new type, but only a different name for the original type. > But yeah, you're right, the overhead is small. I think after that whole daemon code has been merged and the bugs have been fixed, we could go through the code and move most of it to the header file. gcc will then inline the small functions and we won't see any overhead besides the necessary reading, writing, and type conversion.
Changes since v1: - added missing conversion functions
Attachment #8523835 - Attachment is obsolete: true
Attachment #8526656 - Flags: review?(shuang)
Changes since v1: - commented on PDU payload memory usage in Cmd methods - pre-allocate string memory when string length is known
Attachment #8523836 - Attachment is obsolete: true
Attachment #8526659 - Flags: review?(shuang)
Changes since v1: - commented on empty entries in service array
Attachment #8523839 - Attachment is obsolete: true
Attachment #8526661 - Flags: review+
Comment on attachment 8523837 [details] [diff] [review] [03] Bug 1091588: Add Handsfree interface for Bluetooth daemon Review of attachment 8523837 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp @@ +973,5 @@ > + res = nullptr; > + } > + > + nsresult rv = mModule->RegisterModule( > + BluetoothDaemonHandsfreeModule::SERVICE_ID, 0x00, res); One thing I noticed during going through HFP HAL protocol, it looks like 0x00 means supporting headset profile only, but we shall enable it at least 0x01. Handsfree profile (narrowband speech by default). In fact, i don't see any easy way to configure it via bluetooth hal api at bluetooth daemon side. But pass 0x01 seems to be more reasonable for current configuration. Android HAL name: "handsfree" (BT_PROFILE_HANDSFREE_ID) Service modes: 0x00 = Headset Profile only mode (default) 0x01 = Handsfree Profile (narrowband speech) 0x02 = Handsfree Profile (narrowband and wideband speech)
(In reply to Shawn Huang [:shawnjohnjr] from comment #36) > Comment on attachment 8523837 [details] [diff] [review] > [03] Bug 1091588: Add Handsfree interface for Bluetooth daemon > > Review of attachment 8523837 [details] [diff] [review]: > Handsfree profile (narrowband speech by default). In fact, i don't see > any easy way to configure it via bluetooth hal api at bluetooth daemon side. I could be wrong here. It can be |init_features| bluetooth hal api can control it.
Comment on attachment 8526656 [details] [diff] [review] [01] Bug 1091588: Add helpers for Bluetooth daemon Handsfree support (v2) Review of attachment 8526656 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8526656 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #36) > Comment on attachment 8523837 [details] [diff] [review] > [03] Bug 1091588: Add Handsfree interface for Bluetooth daemon > > Review of attachment 8523837 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp > @@ +973,5 @@ > > + res = nullptr; > > + } > > + > > + nsresult rv = mModule->RegisterModule( > > + BluetoothDaemonHandsfreeModule::SERVICE_ID, 0x00, res); > > One thing I noticed during going through HFP HAL protocol, it looks like > 0x00 means supporting headset profile only, but we shall enable it at least > 0x01. Handsfree profile (narrowband speech by default). In fact, i don't see > any easy way to configure it via bluetooth hal api at bluetooth daemon side. > But pass 0x01 seems to be more reasonable for current configuration. > > Android HAL name: "handsfree" (BT_PROFILE_HANDSFREE_ID) > Service modes: 0x00 = Headset Profile only mode (default) > 0x01 = Handsfree Profile (narrowband speech) > 0x02 = Handsfree Profile (narrowband and wideband speech) Ok, I'll set it to 0x01. I think these values are only relevant to BlueZ's bluetoothd. Our daemon currently ignores the mode argument as Bluedroid doesn't use is.
Changes since v1: - register with narrow-band speech mode
Attachment #8523837 - Attachment is obsolete: true
Attachment #8527503 - Flags: review+
Blocks: 1103872
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: