Closed Bug 1102703 Opened 10 years ago Closed 10 years ago

[gonk-l] Enable MOZ_B2G_BT/MOZ_B2G_BT_BLUEDROID

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

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

Tracking

(feature-b2g:2.2+)

Status:
RESOLVED FIXED
Milestone:
2.2 S3 (9jan)
Project Flags:
feature-b2g 2.2+

People

(Reporter: m1, Assigned: brsun)

References

Details

(Whiteboard: [ft:Peripherals])

Attachments

(10 files, 44 obsolete files)

bug1102703_enable_bluetooth_android_lollipop.internal.core.commit.v2.patch
17.17 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.internal.a2dp.commit.v2.patch
1.97 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.commit.v2.patch
68.06 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.hal.core.commit.v2.patch
8.57 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.hal.a2dp.commit.v2.patch
1.87 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.commit.v2.patch
20.11 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.daemon.core.commit.v2.patch
14.85 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.daemon.a2dp.commit.v2.patch
4.15 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.commit.v2.patch
28.72 KB, patch
brsun
: review+
Details | Diff | Splinter Review
bug1102703_enable_bluetooth_android_lollipop.configure.commit.v2.patch
1.50 KB, patch
brsun
: review+
Details | Diff | Splinter Review 
L bluedroid is slightly different than KK.  We'll probably need to sprinkle some |#if ANDROID_VERSION >= 21| around the BT files to get them to compile.  The affected files seem to be:
 dom/bluetooth/bluedroid/BluetoothA2dpHALInterface.cpp    
 dom/bluetooth/bluedroid/BluetoothHALInterface.cpp         
 dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
 dom/bluetooth2/bluedroid/BluetoothA2dpHALInterface.cpp    
 dom/bluetooth2/bluedroid/BluetoothHALInterface.cpp         
 dom/bluetooth2/bluedroid/BluetoothHandsfreeHALInterface.cpp
This patch contains some changes we had to make just to compile successfully.
Hi Bruce,
Can you help to check this patch?
Thanks.
Assignee: nobody → brsun
See Also: → 1087195
I failed to compile gecko with attachment 8529244 [details] [diff] [review]. This patch is a workable version that I did a little modification based on it.

This is not the final one. I will continue to work on the patch.
The idea of this patch is to extend all internal interfaces of gecko to fit the latest android/bluez interfaces (ex. adding one extra bluetooth_device_address parameter), and then to switch the used underlying APIs individually on ANDROID_VERSION to make gecko compilable on different AOSP versions.
Attachment #8536411 - Flags: review?(shuang)
(In reply to Bruce Sun [:brsun] from comment #4)
> Created attachment 8536411 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.patch
> 
> The idea of this patch is to extend all internal interfaces of gecko to fit
> the latest android/bluez interfaces (ex. adding one extra
> bluetooth_device_address parameter), and then to switch the used underlying
> APIs individually on ANDROID_VERSION to make gecko compilable on different
> AOSP versions.

Please seprate HAL interface and Daemon interface to two patch sets. Please add an another patch set for enable Bluetooth flag.
Comment on attachment 8536411 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.patch

I will wait for updated patch sets to review again, thanks!
Extend internal interfaces to sync with android lollipop and bluez 5.26.
Attachment #8536411 - Attachment is obsolete: true
Attachment #8536411 - Flags: review?(shuang)
Attachment #8537091 - Flags: review?(shuang)
Porting bluetooth interface of android lollipop.
Attachment #8537093 - Flags: review?(shuang)
Porting bluetooth interface of bluez 5.26.
Attachment #8537094 - Flags: review?(shuang)
Enable MOZ_B2G_BT and MOZ_B2G_BT_BLUEDROID on android lollipop.
Attachment #8537095 - Flags: review?(shuang)
Hmm...Aftering apply patches, now I saw crash from bluedroid.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1068.1810]
0xb23755fe in alarm_service_reschedule () at external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:214
214	        int rc = bt_os_callouts->acquire_wake_lock(WAKE_LOCK_ID);
(gdb) bt
#0  0xb23755fe in alarm_service_reschedule () at external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:214
#1  0xb23753da in gki_adjust_timer_count (ticks=ticks@entry=100) at external/bluetooth/bluedroid/gki/./common/gki_time.c:784
#2  0xb237548a in GKI_start_timer (tnum=<optimized out>, ticks=<optimized out>, ticks@entry=100, is_continuous=is_continuous@entry=true) at external/bluetooth/bluedroid/gki/./common/gki_time.c:254
#3  0xb23a59b2 in btu_start_timer (p_tle=0xb24fcc58 <bnep_cb+2384>, type=type@entry=50, timeout=timeout@entry=2) at external/bluetooth/bluedroid/stack/./btu/btu_task.c:668
#4  0xb2379ffa in BNEP_Init () at external/bluetooth/bluedroid/stack/./bnep/bnep_api.c:50
#5  0xb2319762 in BTE_InitStack () at external/bluetooth/bluedroid/main/bte_init.c:376
#6  0xb23a5d30 in btu_task (param=<optimized out>) at external/bluetooth/bluedroid/stack/./btu/btu_task.c:209
#7  0xb237554a in gki_task_entry (params=2991592712) at external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:263
#8  0xb6ef82e4 in __pthread_start (arg=0xadf15100, arg@entry=<error reading variable: value has been optimized out>) at bionic/libc/bionic/pthread_create.cpp:141
#9  0xb6ef62d4 in __start_thread (fn=<optimized out>, arg=<optimized out>) at bionic/libc/bionic/clone.cpp:41
#10 0x00000000 in ?? ()
(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> Hmm...Aftering apply patches, now I saw crash from bluedroid.
> 
Let's handle all alarm wake up related calls first.
(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> Hmm...Aftering apply patches, now I saw crash from bluedroid.
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1068.1810]
> 0xb23755fe in alarm_service_reschedule () at
> external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:214
> 214	        int rc = bt_os_callouts->acquire_wake_lock(WAKE_LOCK_ID);
> (gdb) bt
> #0  0xb23755fe in alarm_service_reschedule () at
> external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:214
> #1  0xb23753da in gki_adjust_timer_count (ticks=ticks@entry=100) at
> external/bluetooth/bluedroid/gki/./common/gki_time.c:784
> #2  0xb237548a in GKI_start_timer (tnum=<optimized out>, ticks=<optimized
> out>, ticks@entry=100, is_continuous=is_continuous@entry=true) at
> external/bluetooth/bluedroid/gki/./common/gki_time.c:254
> #3  0xb23a59b2 in btu_start_timer (p_tle=0xb24fcc58 <bnep_cb+2384>,
> type=type@entry=50, timeout=timeout@entry=2) at
> external/bluetooth/bluedroid/stack/./btu/btu_task.c:668
> #4  0xb2379ffa in BNEP_Init () at
> external/bluetooth/bluedroid/stack/./bnep/bnep_api.c:50
> #5  0xb2319762 in BTE_InitStack () at
> external/bluetooth/bluedroid/main/bte_init.c:376
> #6  0xb23a5d30 in btu_task (param=<optimized out>) at
> external/bluetooth/bluedroid/stack/./btu/btu_task.c:209
> #7  0xb237554a in gki_task_entry (params=2991592712) at
> external/bluetooth/bluedroid/gki/./ulinux/gki_ulinux.c:263
> #8  0xb6ef82e4 in __pthread_start (arg=0xadf15100, arg@entry=<error reading
> variable: value has been optimized out>) at
> bionic/libc/bionic/pthread_create.cpp:141
> #9  0xb6ef62d4 in __start_thread (fn=<optimized out>, arg=<optimized out>)
> at bionic/libc/bionic/clone.cpp:41
> #10 0x00000000 in ?? ()

This caused by gecko doesn't set callbacks for alarm wake lock which added by HAL version 5.0.
Comment on attachment 8537093 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_02.patch

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

Please add OS alarm wake up related callbacks.
Attachment #8537093 - Flags: review?(shuang) → review-
Comment on attachment 8537091 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_01.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +768,5 @@
> +  {
> +    BluetoothDaemonPDU& pdu = GetPDU();
> +
> +    /* Read address */
> +    // TODO

I'd rather like to see one complete change per patch.
Comment on attachment 8537093 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_02.patch

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

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
@@ +773,5 @@
> +    CONVERT(BTHF_WBS_NONE, HFP_WBS_NONE),
> +    CONVERT(BTHF_WBS_NO, HFP_WBS_NO),
> +    CONVERT(BTHF_WBS_YES, HFP_WBS_YES)
> +  };
> +  if (aIn >= MOZ_ARRAY_LENGTH(sWBSConfig)) {

Put an NS_WARN_IF() around these basic tests to make it log a message.

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +116,5 @@
>      BluetoothHandsfreeNRECState, nsString,
>      BluetoothHandsfreeNRECState, const nsAString&>
>      NRECNotification;
>  
> +#if ANDROID_VERSION >= 21

No #if here because it's not version-specific.

@@ +614,5 @@
> +  } else {
> +    status = BT_STATUS_PARM_INVALID;
> +  }
> +#else
> +

Remove new-line. Here and below.

@@ +666,5 @@
> +
> +  if (NS_SUCCEEDED(Convert(aType, type)) &&
> +      NS_SUCCEEDED(Convert(aBdAddr, bdAddr))) {
> +    status = mInterface->volume_control(type, aVolume, &bdAddr);
> +  } else {

Share such else branches, if possible.
Attachment #8537093 - Flags: review- → review?(shuang)
Comment on attachment 8537094 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_03.patch

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

None of the protocol changes are compatible with protocols versions before L.
Attachment #8537094 - Flags: review-
Comment on attachment 8537095 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_04.patch

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

::: configure.in
@@ +299,5 @@
>          # Disable fmp4 and webrtc before related bugs got fixed
>          MOZ_FMP4=
>          MOZ_WEBRTC=0
> +        MOZ_B2G_BT=1
> +        MOZ_B2G_BT_BLUEDROID=1

Other architectures test this. And you will need BT_DAEMON to build the daemon backend.
Attachment #8537095 - Flags: review?(shuang)
Attachment #8537094 - Flags: review?(shuang)
Attachment #8537094 - Flags: review-
Attachment #8537094 - Flags: feedback-
Attachment #8537091 - Flags: review?(shuang)
Sorry for messing up the review flags.
Hi

You should further split up the patch set into generic+HAL+daemon for each profile. The will make a lot of files, but make rebasing and bisecting a lot easier. We might need to back-port some of this into 2.2 and having these large patches is just painful then.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #20)
> Comment on attachment 8537094 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.step_03.patch
> 
> Review of attachment 8537094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> None of the protocol changes are compatible with protocols versions before L.

Hi Thomas,

What strategy should we take to ensure the consistency between gecko and daemon?
- a. Check ANDROID_VERSION at both side.
- b. Check the version of referred BlueZ interface at both side.
- c. Define a version number in daemon and let gecko check that version number of the corresponding daemon.

(b) and (c) are more intuitive than (a) regarding to the used protocol itself, but both of them are required to maintain extra version numbers within gecko and/or daemon.

What do you think?
Flags: needinfo?(tzimmermann)
feature-b2g: --- → 2.2+
Whiteboard: [ft:Peripherals]
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #18)
> Comment on attachment 8537091 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.step_01.patch
> 
> Review of attachment 8537091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
> @@ +768,5 @@
> > +  {
> > +    BluetoothDaemonPDU& pdu = GetPDU();
> > +
> > +    /* Read address */
> > +    // TODO
> 
> I'd rather like to see one complete change per patch.

If I didn't define AnswerCallInitOp like this, |nsString& aArg1| will be unpacked automatically by |UnpackPDUInitOp|, which is not sync with the protocol if the corresponding parameter has not been added into the pack logic at daemon side.

The idea to split patches like this is to keep all used protocols unchanged in step_01 (attachment 8537091 [details] [diff] [review]), then porting android lollipop in step_02 (attachment 8537093 [details] [diff] [review]) and porting bluez 5.26 in step_03 (attachment 8537094 [details] [diff] [review]). If we can decide what version number should be synced at both gecko side and daemon side for our bluetooth ipc protocol (comment 24), I believe we could handle the consistency problem (comment 20) in step_03 more precisely.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #23)
> Hi
> 
> You should further split up the patch set into generic+HAL+daemon for each
> profile. The will make a lot of files, but make rebasing and bisecting a lot
> easier. We might need to back-port some of this into 2.2 and having these
> large patches is just painful then.

Hi Thomas,

Thanks for your comments, but sorry that I am not quite sure what action item I might need to take regarding to this. Currently most changes are related to HandsFreeProfile only, and our target of L porting is 2.2. Would you please share more information about your ideas?
(In reply to Bruce Sun [:brsun] from comment #26)
> (In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment
> #23)
> > Hi
> > 
> > You should further split up the patch set into generic+HAL+daemon for each
> > profile. The will make a lot of files, but make rebasing and bisecting a lot
> > easier. We might need to back-port some of this into 2.2 and having these
> > large patches is just painful then.
> 
> Hi Thomas,
> 
> Thanks for your comments, but sorry that I am not quite sure what action
> item I might need to take regarding to this. Currently most changes are
> related to HandsFreeProfile only, and our target of L porting is 2.2. Would
> you please share more information about your ideas?

I guess he said patches can be split into three parts: (1) General patches (which are not related to HAL/Daemon) (2) HAL related changes per profile (3) Daemon related changes per profile.
Comment on attachment 8537091 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_01.patch

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

Missing adding callback, energy_info_callback energy_info_cb, and set_wake_alarm_callout,  acquire_wake_lock_callout,  release_wake_lock_callout were needed. Otherwise, bluedroid on L will crash.
Attachment #8537091 - Flags: review?(shuang) → review-
Comment on attachment 8537095 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_04.patch

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

::: configure.in
@@ +299,5 @@
>          # Disable fmp4 and webrtc before related bugs got fixed
>          MOZ_FMP4=
>          MOZ_WEBRTC=0
> +        MOZ_B2G_BT=1
> +        MOZ_B2G_BT_BLUEDROID=1

Please add those below lines for android version 21.

            if test -d "$gonkdir/system/bluetoothd"; then 
                MOZ_B2G_BT_DAEMON=1
            fi
Attachment #8537095 - Flags: review?(shuang) → review-
Hi Bruce

> 
> Thanks for your comments, but sorry that I am not quite sure what action
> item I might need to take regarding to this. Currently most changes are
> related to HandsFreeProfile only, and our target of L porting is 2.2. Would
> you please share more information about your ideas?

2.2 will be branched on Jan 12. Bluetoothd will not be ready by then and will have to be backported. If you can land some L patches before Jan 12, I wouldn't expect 2.2 to have full L support at the beginning. Backporting patches is a lot easier for small patches. But looking at the patch set again, I realize that splitting up these patches might not be as easy as I initially thought.
Flags: needinfo?(tzimmermann)
Comment on attachment 8537094 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_03.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +803,5 @@
>      BluetoothDaemonPDU& pdu = GetPDU();
>  
>      /* Read address */
> +    nsresult rv = UnpackPDU(
> +      pdu, UnpackConversion<BluetoothAddress, nsAString>(aArg1));

JB and KK don't support these address variables, but we can emulate that in the backend. The rough sketch is

 (1) store the device's address in a static variable in the backend for JB and KK when creating a connection in a command
 (2) when you receive a notification, use this variable's value to initialize the runnable.
 (3) when no connection is open, the variable's value should be the invalid address
 (4) update the value in when connection-state notifications are received

You have to be careful with multithreading here: commands are created on the main thread, notifications are received on the I/O thread. I suggest to keep two variables on the I/O thread: |sConnectionAddress| for current connections, |sPendingConnectionAddress| for pending connection attempts. The handling of these is:

 (1) in |Connect| send a task to the I/O thread to update |sPendingConnectionAddress| with the new address before sending the command
 (2a) in |ConnectionStateNtf| copy |sPendingConnectionAddress| to |sConnectionAddress| if the connection has been established
 (2b) or clear |sConnectionAddress| to 'invalid' if the connection was closed
 (3) in all other notifications copy the value |sConnectionAddress| to the notification runnable.
Comment on attachment 8537091 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.step_01.patch

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

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +181,5 @@
>    VoiceRecognition(bthf_vr_state_t aState)
>    {
>      VoiceRecognitionNotification::Dispatch(
>        &BluetoothHandsfreeNotificationHandler::VoiceRecognitionNotification,
> +      aState, (bt_bdaddr_t*)nullptr);

Don't pass nullptr here. If you need an invalid address, you can init a bt_bdaddr_t variable to '000000' instead, and pass its address. It's save to create this variable on the stack.

I expect the HAL backend to be removed soon, so it probably does not make sense to implement address tracking as for the daemon backend.
(In reply to Bruce Sun [:brsun] from comment #24)
> (In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment
> #20)
> > Comment on attachment 8537094 [details] [diff] [review]
> > bug1102703_enable_bluetooth_android_lollipop.step_03.patch
> > 
> > Review of attachment 8537094 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > None of the protocol changes are compatible with protocols versions before L.
> 
> Hi Thomas,
> 
> What strategy should we take to ensure the consistency between gecko and
> daemon?
> - a. Check ANDROID_VERSION at both side.
> - b. Check the version of referred BlueZ interface at both side.
> - c. Define a version number in daemon and let gecko check that version
> number of the corresponding daemon.
> 
> (b) and (c) are more intuitive than (a) regarding to the used protocol
> itself, but both of them are required to maintain extra version numbers
> within gecko and/or daemon.
> 
> What do you think?

Hi Thomas,
Thanks for your guidance. May I have your thoughts as well regarding to syncing the used protocol version between daemon and gecko?
Flags: needinfo?(tzimmermann)
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #32)
> Comment on attachment 8537091 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.step_01.patch
> 
> Review of attachment 8537091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
> @@ +181,5 @@
> >    VoiceRecognition(bthf_vr_state_t aState)
> >    {
> >      VoiceRecognitionNotification::Dispatch(
> >        &BluetoothHandsfreeNotificationHandler::VoiceRecognitionNotification,
> > +      aState, (bt_bdaddr_t*)nullptr);
> 
> Don't pass nullptr here. If you need an invalid address, you can init a
> bt_bdaddr_t variable to '000000' instead, and pass its address. It's save to
> create this variable on the stack.

There is one convert helper function already in BluetoothHALHelpers.h which can convert |(bt_bdaddr_t*)nullptr| to the targeting nsAString with the value |BLUETOOTH_ADDRESS_NONE|. Do we still need to use an extra variable temporarily on the stack for each XXXNotification?

========== code snippet begin ==========
inline nsresult
Convert(const bt_bdaddr_t* aIn, nsAString& aOut)
{
  if (!aIn) {
    aOut.AssignLiteral(BLUETOOTH_ADDRESS_NONE);
    return NS_OK;
  }
  return Convert(*aIn, aOut);
}
========== code snippet end ==========
Hi Bruce

(In reply to Bruce Sun [:brsun] from comment #24)
> (In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment
> #20)
> > Comment on attachment 8537094 [details] [diff] [review]
> > bug1102703_enable_bluetooth_android_lollipop.step_03.patch
> > 
> > Review of attachment 8537094 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > None of the protocol changes are compatible with protocols versions before L.
> 
> Hi Thomas,
> 
> What strategy should we take to ensure the consistency between gecko and
> daemon?
> - a. Check ANDROID_VERSION at both side.
> - b. Check the version of referred BlueZ interface at both side.
> - c. Define a version number in daemon and let gecko check that version
> number of the corresponding daemon.
> 
> (b) and (c) are more intuitive than (a) regarding to the used protocol
> itself, but both of them are required to maintain extra version numbers
> within gecko and/or daemon.
> 
> What do you think?

The current code uses (a) for handling differences between Android versions. That should be fine for L as well. Having separate protocol versions is more correct, but since we don't run the daemon on anything besides Android, this would only add overhead to our development.
Flags: needinfo?(tzimmermann)
Our profile handlers should be independent from the underlying backend or version if somehow possible. All the differences between versions should be abstracted by the backend code.

The daemon is merely a wrapper around Android's Bluedroid interfaces. And all the ugly workarounds for Bluedroid bugs should be located in the daemon as well.
Status: NEW → ASSIGNED
Extend internal interfaces to sync with android lollipop and bluez 5.26 (core)
Attachment #8537091 - Attachment is obsolete: true
Attachment #8541195 - Flags: review?(shuang)
Extend internal interfaces to sync with android lollipop and bluez 5.26 (a2dp)
Attachment #8541196 - Flags: review?(shuang)
Extend internal interfaces to sync with android lollipop and bluez 5.26 (avrcp)
Attachment #8541197 - Flags: review?(shuang)
Extend internal interfaces to sync with android lollipop and bluez 5.26 (handsfree)
Attachment #8541200 - Flags: review?(shuang)
Porting bluetooth interface of android lollipop (core)
Attachment #8537093 - Attachment is obsolete: true
Attachment #8541202 - Flags: review?(shuang)
Porting bluetooth interface of android lollipop (a2dp)
Attachment #8541203 - Flags: review?(shuang)
Porting bluetooth interface of android lollipop (avrcp)
Attachment #8541204 - Flags: review?(shuang)
Porting bluetooth interface of android lollipop (handsfree)
Attachment #8541205 - Flags: review?(shuang)
Porting bluetooth interface of bluez 5.26 (core)
Attachment #8537094 - Attachment is obsolete: true
Attachment #8541207 - Flags: review?(shuang)
Porting bluetooth interface of bluez 5.26 (a2dp)
Attachment #8541208 - Flags: review?(shuang)
Porting bluetooth interface of bluez 5.26 (handsfree)
Attachment #8541209 - Flags: review?(shuang)
Enable MOZ_B2G_BT and MOZ_B2G_BT_BLUEDROID.
Attachment #8537095 - Attachment is obsolete: true
Attachment #8541211 - Flags: review?(shuang)
Fix some compiler warnings.
Attachment #8541212 - Flags: review?(shuang)
See Also: → 1106858
I found that the inherited / cooperated relationship between BluetoothDaemonProtocol and BluetoothDaemonXXXModule are somewhat not so rational:
 - RegisterModule() / UnregisterModule() is supposed to be declared in BluetoothDaemonSetupModule.
 - RegisterModule() / UnregisterModule() is not supposed to be declared repeatedly in all profile-related BluetoothDaemonXXXModule.
 - Send() is not supposed to be declared repeatedly in all existing BluetoothDaemonXXXModule.

Bug 1115337 is created for follow-up. We could discuss how these classes could be inherited / combined with a better way on that bug. Maybe breaking down codes into other classes could be a good start.
Comment on attachment 8541212 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.nit.patch

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

Thanks for taking care of this. I think you can fix the compiler warnings by declaring the classes as MOZ_FINAL. It's the cleaner solution for this problem.

This patch is also unrelated to the bug report. Please open a new bug report.
Comment on attachment 8541195 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.core.patch

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

::: dom/bluetooth/BluetoothCommon.h
@@ +238,5 @@
> +/* Physical transport for GATT connections to remote dual-mode devices */
> +enum BluetoothTransport {
> +  TRANSPORT_AUTO,   /* No preference of physical transport */
> +  TRANSPORT_BR_EDR, /* Prefer BR/EDR transport */
> +  TRANSPORT_LE      /* Prefer LE transport */

Other constants use BREDR and BLE. Can we use these names here?

::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
@@ +177,5 @@
>      LeTestModeNotification;
>  
> +  typedef BluetoothNotificationHALRunnable1<NotificationHandlerWrapper, void,
> +                                            nsAutoPtr<BluetoothActivityEnergyInfo>,
> +                                            const BluetoothActivityEnergyInfo*>

You should use a reference instead of pointers here.
Comment on attachment 8541202 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.patch

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

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.cpp
@@ +213,5 @@
>  
> +#if ANDROID_VERSION >= 21
> +nsresult
> +Convert(const bt_activity_energy_info* aIn,
> +        nsAutoPtr<BluetoothActivityEnergyInfo>& aOut)

References instead of pointers.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
@@ +783,5 @@
> +#if ANDROID_VERSION >= 21
> +nsresult
> +Convert(const bt_activity_energy_info* aIn,
> +        nsAutoPtr<BluetoothActivityEnergyInfo>& aOut);
> +

Cleanup empty newline.

::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
@@ +476,5 @@
>    int status = mInterface->init(&sBluetoothCallbacks);
>  
> +#if ANDROID_VERSION >= 21
> +  if (status == BT_STATUS_SUCCESS) {
> +    status = mInterface->set_os_callouts(&sBluetoothOsCallouts);

Call |mInterface->cleanup| if this function fails.
Comment on attachment 8541207 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.core.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.cpp
@@ +341,5 @@
>      res = nullptr;
>    }
>  
>    nsresult rv = mModule->RegisterModule(BluetoothDaemonA2dpModule::SERVICE_ID,
> +                                        1 /* Max Clients */, 0x00, res);

No comment here. Make '1' a constant if you think this needs some context.

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +1325,5 @@
>    }
>  
>    nsresult rv = mModule->RegisterModule(
> +    BluetoothDaemonHandsfreeModule::SERVICE_ID, MODE_NARROWBAND_SPEECH,
> +    1 /* Max Clients */, res);

Same as above.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +418,5 @@
>  
> +#if ANDROID_VERSION >= 21
> +    nsresult rv = PackPDU(
> +      PackConversion<nsAString, BluetoothAddress>(aBdAddr),
> +      PackConversion<BluetoothTransport, uint8_t>(aTransport), *pdu);

There should be a separate function |PackPDU(BluetoothDaemonPDU&, BlutoothTransport)| that contains this conversion. You can then simply pass |aTransport| as argument here and the conversion will be called automatically.
(In reply to Bruce Sun [:brsun] from comment #50)
> I found that the inherited / cooperated relationship between
> BluetoothDaemonProtocol and BluetoothDaemonXXXModule are somewhat not so
> rational:
>  - RegisterModule() / UnregisterModule() is supposed to be declared in
> BluetoothDaemonSetupModule.
>  - RegisterModule() / UnregisterModule() is not supposed to be declared
> repeatedly in all profile-related BluetoothDaemonXXXModule.
>  - Send() is not supposed to be declared repeatedly in all existing
> BluetoothDaemonXXXModule.
> 
> Bug 1115337 is created for follow-up. We could discuss how these classes
> could be inherited / combined with a better way on that bug. Maybe breaking
> down codes into other classes could be a good start.

Please see my comment in bug 1115337.
Comment on attachment 8541200 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +16,5 @@
>  
>  BluetoothHandsfreeNotificationHandler*
>    BluetoothDaemonHandsfreeModule::sNotificationHandler;
>  
> +nsString BluetoothDaemonHandsfreeModule::sConnectedBluetoothAddress(

This should be a regular field in |BluetoothDaemonHandsfreeModule|. There can only be one currently, but would still be cleaner.
Comment on attachment 8541200 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.patch

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

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +322,3 @@
>  };
>  
> +bt_bdaddr_t BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress = {

Declare as 'static'.

@@ +324,5 @@
> +bt_bdaddr_t BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress = {
> +  {0, 0, 0, 0, 0, 0}
> +};
> +
> +Mutex BluetoothHandsfreeHALCallback::sMutexBluetoothAddress(

All access to |sConnectedBluetoothAddress| seems to happen on a Bluedroid-internal thread. Why do you need this lock?
Comment on attachment 8541209 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +159,5 @@
> +  nsresult rv;
> +#if ANDROID_VERSION >= 21
> +  nsAutoPtr<BluetoothDaemonPDU> pdu(
> +    new BluetoothDaemonPDU(SERVICE_ID, OPCODE_START_VOICE_RECOGNITION,
> +                           6)); // Address

I think it's OK to allocate a PDU that is large enough to hold the payload of any version. The memory overhead is minimal and we'd share some more code. Unused payload memory will not be part of the PDU message.

Here and below.

@@ +1055,5 @@
>  
> +    nsresult rv;
> +    // It's a little weird to parse aArg2(aBdAddr) before parsing
> +    // aArg1(aNumber), but this order is defined in bluez 5.25 anyway.
> +    /* Read address */

Please merge these comments into one; with /* */ for consistency.
Comment on attachment 8541208 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.a2dp.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.cpp
@@ +271,5 @@
> +
> +  nsresult
> +  operator () (nsString& aArg1,
> +               uint32_t aArg2,
> +               uint8_t aArg3) const

Can you put these arguments on a single line?

@@ +315,5 @@
>  {
>    static void (BluetoothDaemonA2dpModule::* const HandleNtf[])(
>      const BluetoothDaemonPDUHeader&, BluetoothDaemonPDU&) = {
>      INIT_ARRAY_AT(0, &BluetoothDaemonA2dpModule::ConnectionStateNtf),
>      INIT_ARRAY_AT(1, &BluetoothDaemonA2dpModule::AudioStateNtf),

The trailing comma is actually a syntax error for some compilers in strict mode. I know it's not always done correctly in the current source code, but the comma should be located on a separate line within the ANDROID_VERSION block.

@@ +317,5 @@
>      const BluetoothDaemonPDUHeader&, BluetoothDaemonPDU&) = {
>      INIT_ARRAY_AT(0, &BluetoothDaemonA2dpModule::ConnectionStateNtf),
>      INIT_ARRAY_AT(1, &BluetoothDaemonA2dpModule::AudioStateNtf),
> +#if ANDROID_VERSION >= 21
> +    INIT_ARRAY_AT(2, &BluetoothDaemonA2dpModule::AudioConfigNtf),

And no trailing comma here.

::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.h
@@ +108,5 @@
> +                                         uint32_t,
> +                                         uint8_t,
> +                                         const nsAString&,
> +                                         uint32_t,
> +                                         uint8_t>

Put parameters on same lines.
Comment on attachment 8541197 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.avrcp.patch

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

::: dom/bluetooth/BluetoothInterface.h
@@ +458,5 @@
>    virtual ~BluetoothAvrcpInterface();
>  };
>  
>  //
> +// Bluetooth AVRCP Controller Interface

This is a completely new module and not required for L support AFAICS. (?) We'd also need support in a profile handler. The situation here is similar to AVRP and A2DP.

Please open a new bug for this feature.
Hi Bruce,

Most of the code looks good. Major points from my side are

  - the (probably) unnecessary use of a mutex, and
  - the handling of avrcp-ctrl.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #51)
> Comment on attachment 8541212 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.nit.patch
> 
> Review of attachment 8541212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking care of this. I think you can fix the compiler warnings by
> declaring the classes as MOZ_FINAL. It's the cleaner solution for this
> problem.
> 
> This patch is also unrelated to the bug report. Please open a new bug report.

bug 1115441 is created for separation.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #52)
> Comment on attachment 8541195 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.internal.core.patch
> 
> Review of attachment 8541195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothCommon.h
> @@ +238,5 @@
> > +/* Physical transport for GATT connections to remote dual-mode devices */
> > +enum BluetoothTransport {
> > +  TRANSPORT_AUTO,   /* No preference of physical transport */
> > +  TRANSPORT_BR_EDR, /* Prefer BR/EDR transport */
> > +  TRANSPORT_LE      /* Prefer LE transport */
> 
> Other constants use BREDR and BLE. Can we use these names here?
> 

Agree. It would be better to sync the naming of these constants. Both BR_EDR or BREDR are just fine with me. But after searching the bluetooth spec, there is no such abbreviation for BLE. What would be the preferred name for LE device (simply LE or prefixing a B like BLE)? Do we need to prefix a B for BR or EDR as well (like BBR or BEDR)?

> ::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
> @@ +177,5 @@
> >      LeTestModeNotification;
> >  
> > +  typedef BluetoothNotificationHALRunnable1<NotificationHandlerWrapper, void,
> > +                                            nsAutoPtr<BluetoothActivityEnergyInfo>,
> > +                                            const BluetoothActivityEnergyInfo*>
> 
> You should use a reference instead of pointers here.

I will check what I can do regarding to this.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #54)
> Comment on attachment 8541207 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.daemon.core.patch
> 
> Review of attachment 8541207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +418,5 @@
> >  
> > +#if ANDROID_VERSION >= 21
> > +    nsresult rv = PackPDU(
> > +      PackConversion<nsAString, BluetoothAddress>(aBdAddr),
> > +      PackConversion<BluetoothTransport, uint8_t>(aTransport), *pdu);
> 
> There should be a separate function |PackPDU(BluetoothDaemonPDU&,
> BlutoothTransport)| that contains this conversion. You can then simply pass
> |aTransport| as argument here and the conversion will be called
> automatically.

From the design of bluez, this transport value can be 1 byte (1 octet in Create Bond command of Bluetooth Core HAL) and 4 bytes (4 octets in Connect Device command, Connect Peripheral command, and Start Service command in Bluetooth GATT HAL). Explicitly using uint8_t or uint32_t seems not avoidable if we want to use BluetoothTransport enum in both Core and GATT cases.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #56)
> Comment on attachment 8541200 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.patch
> 
> Review of attachment 8541200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
> @@ +16,5 @@
> >  
> >  BluetoothHandsfreeNotificationHandler*
> >    BluetoothDaemonHandsfreeModule::sNotificationHandler;
> >  
> > +nsString BluetoothDaemonHandsfreeModule::sConnectedBluetoothAddress(
> 
> This should be a regular field in |BluetoothDaemonHandsfreeModule|. There
> can only be one currently, but would still be cleaner.

Because only one connected device is allowed in current design, probably BluetoothDaemonHandsfreeModule::sConnectedBluetoothAddress is supposed to be a static variable, and it is only needed before ANDROID_VERSION 21. After ANDROID_VERSION 21, this address information can be parsed directly from the PDU.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #57)
> Comment on attachment 8541200 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.patch
> 
> Review of attachment 8541200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
> @@ +322,3 @@
> >  };
> >  
> > +bt_bdaddr_t BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress = {
> 
> Declare as 'static'.
> 

It is already declared as a static variable in the patch.

> @@ +324,5 @@
> > +bt_bdaddr_t BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress = {
> > +  {0, 0, 0, 0, 0, 0}
> > +};
> > +
> > +Mutex BluetoothHandsfreeHALCallback::sMutexBluetoothAddress(
> 
> All access to |sConnectedBluetoothAddress| seems to happen on a
> Bluedroid-internal thread. Why do you need this lock?

I didn't find any guarantee that HAL callback will run on the same thread. The interface or the header files don't contain information about the threading model, and the implementation solely depends on our partners. In order to make sure no race condition happens on this address variable, this mutex would still be required.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #60)
> Comment on attachment 8541197 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.internal.avrcp.patch
> 
> Review of attachment 8541197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothInterface.h
> @@ +458,5 @@
> >    virtual ~BluetoothAvrcpInterface();
> >  };
> >  
> >  //
> > +// Bluetooth AVRCP Controller Interface
> 
> This is a completely new module and not required for L support AFAICS. (?)
> We'd also need support in a profile handler. The situation here is similar
> to AVRP and A2DP.
> 
> Please open a new bug for this feature.

It does make sense. I will remove AVRCP Controller Interface from this bug first. But I would suggest fire that bug on demands (probably not now) once we have concrete idea about how this interface can be integrated into Gecko in the future.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #61)
> Hi Bruce,
> 
> Most of the code looks good. Major points from my side are

Thanks a lot. It's great to have your precious comments just in time. Excepts for those questions that are not yet confirmed, I will change my patches correspondingly.

> 
>   - the (probably) unnecessary use of a mutex, and
Please refer to comment 66.
>   - the handling of avrcp-ctrl.
Please refer to comment 67.
Bug 1115473 is create for implementing bt_os_callouts_t with full functionalities on android lollipop (HAL).
(In reply to Bruce Sun [:brsun] from comment #69)
> Bug 1115473 is create for implementing bt_os_callouts_t with full
> functionalities on android lollipop (HAL).

We also need to handle bt_os_callouts_t in daemon side on android lollipop, but I am not sure whether we can customize the used protocol to acquire / release wake locks between daemon and gecko. I don't have any idea right now.
(In reply to Bruce Sun [:brsun] from comment #63)
> (In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment
> #52)
> > Comment on attachment 8541195 [details] [diff] [review]
> > bug1102703_enable_bluetooth_android_lollipop.internal.core.patch
> > 
> > Review of attachment 8541195 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/BluetoothCommon.h
> > @@ +238,5 @@
> > > +/* Physical transport for GATT connections to remote dual-mode devices */
> > > +enum BluetoothTransport {
> > > +  TRANSPORT_AUTO,   /* No preference of physical transport */
> > > +  TRANSPORT_BR_EDR, /* Prefer BR/EDR transport */
> > > +  TRANSPORT_LE      /* Prefer LE transport */
> > 
> > Other constants use BREDR and BLE. Can we use these names here?
> > 
> 
> Agree. It would be better to sync the naming of these constants. Both BR_EDR
> or BREDR are just fine with me. But after searching the bluetooth spec,
> there is no such abbreviation for BLE. What would be the preferred name for
> LE device (simply LE or prefixing a B like BLE)? Do we need to prefix a B
> for BR or EDR as well (like BBR or BEDR)?
I think LE is suitable, both BLE/LE are fine. Bluetooth specification uses LE.
But don't write BBR/BEDR.
(In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment #59)
> Comment on attachment 8541208 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.daemon.a2dp.patch
> 
> Review of attachment 8541208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +315,5 @@
> >  {
> >    static void (BluetoothDaemonA2dpModule::* const HandleNtf[])(
> >      const BluetoothDaemonPDUHeader&, BluetoothDaemonPDU&) = {
> >      INIT_ARRAY_AT(0, &BluetoothDaemonA2dpModule::ConnectionStateNtf),
> >      INIT_ARRAY_AT(1, &BluetoothDaemonA2dpModule::AudioStateNtf),
> 
> The trailing comma is actually a syntax error for some compilers in strict
> mode. I know it's not always done correctly in the current source code, but
> the comma should be located on a separate line within the ANDROID_VERSION
> block.

I am not sure what kind of problem we will encounter by adding this comma at the tail of an array. Would you mind sharing more information about what compilers suffers from this syntax issue?
Comment on attachment 8541195 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.core.patch

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

Please pass by reference followed by Comment 52.

::: dom/bluetooth/BluetoothCommon.h
@@ +238,5 @@
> +/* Physical transport for GATT connections to remote dual-mode devices */
> +enum BluetoothTransport {
> +  TRANSPORT_AUTO,   /* No preference of physical transport */
> +  TRANSPORT_BR_EDR, /* Prefer BR/EDR transport */
> +  TRANSPORT_LE      /* Prefer LE transport */

See Comment 71
Attachment #8541195 - Flags: review?(shuang) → review-
Comment on attachment 8541196 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.a2dp.patch

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

r=me
Attachment #8541196 - Flags: review?(shuang) → review+
Comment on attachment 8541197 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.avrcp.patch

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

V2.2 Gecko only supports AVRCP Target role. We currentl don't support AVRCP Controller role (Controller role), so please file a bug for this feature. Please move avrcp controller related code from this patchset to another bug.
Attachment #8541197 - Flags: review?(shuang)
Comment on attachment 8541200 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.patch

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

Please add comments to explain why Wakelock is needed.

::: dom/bluetooth/BluetoothCommon.h
@@ +322,5 @@
>    HFP_NETWORK_STATE_AVAILABLE
>  };
>  
> +enum BluetoothHandsfreeWbsConfig {
> +  HFP_WBS_NONE,

Please add comments to describe the behavior if NONE is been selected.
/*  HFP_WBS_NONE stands for neither CVSD nor MSBC codec, but other optional codec.*/

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.h
@@ +110,5 @@
>                                 const nsAString& aNumber,
>                                 BluetoothHandsfreeCallAddressType aType,
>                                 BluetoothHandsfreeResultHandler* aRes);
>  
> +  /* CWide Band Speech */

nit:Configure Wide Band Speech

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +307,5 @@
>    void RunInit()
>    {
>      BluetoothHfpManager* hfpManager = BluetoothHfpManager::Get();
>  
> +    mInterface->Init(hfpManager, 1 /* max handsfree clients */, this);

Add a constant MAX_NUM_HF_CLIENTS.
Attachment #8541200 - Flags: review?(shuang)
Comment on attachment 8541202 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.patch

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

Please fix problems mentioned in Comment 53. Thanks.
Attachment #8541202 - Flags: review?(shuang) → review-
Comment on attachment 8541203 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.a2dp.patch

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

r=me
Attachment #8541203 - Flags: review?(shuang) → review+
Comment on attachment 8541204 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.avrcp.patch

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

V2.2 Gecko only supports AVRCP Target role. We currentl don't support AVRCP Controller role (Controller role), so please file a bug for this feature. Please move avrcp controller related code from this patchset to another bug.
Attachment #8541204 - Flags: review?(shuang)
Comment on attachment 8541205 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.patch

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

r=me

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +173,5 @@
>  
>    static void
>    ConnectionState(bthf_connection_state_t aState, bt_bdaddr_t* aBdAddr)
>    {
> +#if ANDROID_VERSION < 21

It would be better to add comments to explain the reason why we need to do this.
Attachment #8541205 - Flags: review?(shuang) → review+
Comment on attachment 8541207 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.core.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
@@ +394,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +Convert(uint32_t aIn, BluetoothTransport& aOut)

Why do we need to have two version BluetoothTransport converstion?
Attachment #8541207 - Flags: review?(shuang) → review-
(In reply to Shawn Huang [:shawnjohnjr] from comment #81)
> Comment on attachment 8541207 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.daemon.core.patch
> 
> Review of attachment 8541207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +394,5 @@
> >    return NS_OK;
> >  }
> >  
> >  nsresult
> > +Convert(uint32_t aIn, BluetoothTransport& aOut)
> 
> Why do we need to have two version BluetoothTransport converstion?

Please kindly refer to comment 64 for the reason.
(In reply to Bruce Sun [:brsun] from comment #72)
> (In reply to Thomas Zimmermann (on PTO) [:tzimmermann] [:tdz] from comment
> #59)
> > Comment on attachment 8541208 [details] [diff] [review]
> > bug1102703_enable_bluetooth_android_lollipop.daemon.a2dp.patch
> > 
> > Review of attachment 8541208 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +315,5 @@
> > >  {
> > >    static void (BluetoothDaemonA2dpModule::* const HandleNtf[])(
> > >      const BluetoothDaemonPDUHeader&, BluetoothDaemonPDU&) = {
> > >      INIT_ARRAY_AT(0, &BluetoothDaemonA2dpModule::ConnectionStateNtf),
> > >      INIT_ARRAY_AT(1, &BluetoothDaemonA2dpModule::AudioStateNtf),
> > 
> > The trailing comma is actually a syntax error for some compilers in strict
> > mode. I know it's not always done correctly in the current source code, but
> > the comma should be located on a separate line within the ANDROID_VERSION
> > block.
> 
> I am not sure what kind of problem we will encounter by adding this comma at
> the tail of an array. Would you mind sharing more information about what
> compilers suffers from this syntax issue?

For C, this syntax (trailing comma in array initialization) has been defined since C89 / ANSI C [1]. For C++, I cannot find the exact documentation for this, but I think this syntax is also supported from the very beginning as well.

In Gecko, this kind of syntaxes are used in other places without problems (ex. ContentParent [2], nsFrame [3], nsDebugImpl [4]).

I would suggest the coding style that putting one comma in a single line like [5] should be avoided.

[1] http://www.lysator.liu.se/c/ANSI-C-grammar-y.html#initializer

[2] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp?from=ContentParent.cpp&case=true#587
[3] http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?from=nsFrame.cpp#10070
[4] http://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebugImpl.cpp?from=nsDebugImpl.cpp#182

[5] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothAvrcpHALInterface.cpp#270
Comment on attachment 8541207 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.core.patch

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

Please remember add GetConnectionState declaration.
Attachment #8541197 - Attachment is obsolete: true
Attachment #8541204 - Attachment is obsolete: true
Attachment #8541212 - Attachment is obsolete: true
Attachment #8541212 - Flags: review?(shuang)
Modification:
 - Replace TRANSPORT_BR_EDR with TRANSPORT_BREDR
 - Use reference in BluetoothHandsfreeAtResponse::EnergyInfoNotification()
 - Add dummy implementation for BluetoothDaemonInterface::GetConnectionState()
 - Add dummy implementation for BluetoothDaemonInterface::ReadEnergyInfo()
 - Use reference in BluetoothCallback::EnergyInfoNotification()
Attachment #8541195 - Attachment is obsolete: true
Attachment #8541674 - Flags: review?(btian)
Attachment #8541674 - Flags: feedback?(tzimmermann)
Modification:
 - Add comments on enum BluetoothHandsfreeWbsConfig values.
 - Refine comments of BluetoothDaemonHandsfreeModule::ConfigureWbsCmd()
 - Add BluetoothHfpManager::MAX_CLIENTS constant.

Note:
 - BluetoothDaemonHandsfreeModule::sConnectedBluetoothAddress is still a static member variable of BluetoothDaemonHandsfreeModule.
 - BluetoothHandsfreeHALCallback::sMutexBluetoothAddress still remains for preventing potential issues caused by unpredictable threading model of callbacks.
 - Comments for WakeLock is not added because this bug doesn't use any WakeLock. Those comments will be added on bug 1115473.
Attachment #8541200 - Attachment is obsolete: true
Attachment #8541675 - Flags: review?(btian)
Attachment #8541675 - Flags: feedback?(tzimmermann)
Modification
 - Use the reference of bt_activity_energy_info and the reference of BluetoothActivityEnergyInfo in Convert()
 - Clear redundant empty line under the declaration of Convert()
 - Replace TRANSPORT_BR_EDR with TRANSPORT_BREDR in Convert()
 - Pass the referece of bt_activity_energy_info in BluetoothCallback::EnergyInfo()
 - Add mInterface->cleanup() in BluetoothHALInterface::Init()
Attachment #8541202 - Attachment is obsolete: true
Attachment #8541678 - Flags: review?(btian)
Attachment #8541678 - Flags: feedback?(tzimmermann)
Modification:
 - Use the reference of bt_activity_energy_info and the reference of BluetoothActivityEnergyInfo in Convert()
 - Add comments for BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress
 - Pass aMaxHandsfreeClients of BluetoothHandsfreeHALInterface::Init() to mInterface->init()
 -
Attachment #8541205 - Attachment is obsolete: true
Attachment #8541680 - Flags: review?(btian)
Attachment #8541680 - Flags: feedback?(tzimmermann)
Modification:
 - Add BluetoothDaemonA2dpModule::MAX_CLIENTS constant.
 - Add BluetoothDaemonHandsfreeModule::MAX_CLIENTS constant.
 - Replace Convert() between BluetoothTransport and uint8_t/uint32_t with PackPDU() between BluetoothTransport and uint8_t.
 - Add BluetoothDaemonCoreModule::MAX_CLIENTS constant.
 - Add BluetoothDaemonSocketModule::MAX_CLIENTS constant.

Note:
 - We should be careful about the conversion between BluetoothTransport and uint32_t once GATT interfaces are supported in the future.
Attachment #8541207 - Attachment is obsolete: true
Attachment #8541682 - Flags: review?(btian)
Attachment #8541682 - Flags: feedback?(tzimmermann)
Modification:
 - Make arguments on the same line if possible.

Note:
 - Trailing commas in array initialization lists remain unchanged.
Attachment #8541208 - Attachment is obsolete: true
Attachment #8541684 - Flags: review?(btian)
Attachment #8541684 - Flags: feedback?(tzimmermann)
Modification:
 - Add BluetoothDaemonHandsfreeModule::MAX_CLIENTS constant.
 - Extend pre-allocated memory of BluetoothDaemonPDU(...) with the largest amount of data that are supported in the latest protocol.
 - Refine the structure of comments in BluetoothDaemonHandsfreeModule::DialCallInitOp.
 - Add comments for BluetoothDaemonHandsfreeModule::sConnectedBluetoothAddress.
Attachment #8541209 - Attachment is obsolete: true
Attachment #8541686 - Flags: review?(btian)
Attachment #8541686 - Flags: feedback?(tzimmermann)
Comment on attachment 8541674 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.core.v2.patch

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothInterface.h
@@ +605,5 @@
>    virtual void CancelBond(const nsAString& aBdAddr,
>                            BluetoothResultHandler* aRes) = 0;
>  
> +  /* Connection */
> +  virtual void GetConnectionState(const nsAString& aBdAddr,

nit: newline here for consistency.

@@ +630,5 @@
>  
>    virtual void LeTestMode(uint16_t aOpcode, uint8_t* aBuf, uint8_t aLen,
>                            BluetoothResultHandler* aRes) = 0;
>  
> +  /* Energy Info */

Ditto.

::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
@@ +734,5 @@
> +void
> +BluetoothHALInterface::GetConnectionState(const nsAString& aBdAddr,
> +                                          BluetoothResultHandler* aRes)
> +{
> +  // TODO: to be added

'to be implemented' is clearer.

@@ +857,5 @@
> +/* Energy Information */
> +void
> +BluetoothHALInterface::ReadEnergyInfo(BluetoothResultHandler* aRes)
> +{
> +  // TODO: to be added

Ditto.

::: dom/bluetooth/bluedroid/BluetoothHALInterface.h
@@ +64,4 @@
>    void RemoveBond(const nsAString& aBdAddr, BluetoothResultHandler* aRes);
>    void CancelBond(const nsAString& aBdAddr, BluetoothResultHandler* aRes);
>  
> +  /* Connection */

Ditto.
Attachment #8541674 - Flags: review?(btian) → review+
Target Milestone: --- → 2.2 S3 (9jan)
Comment on attachment 8541674 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.core.v2.patch

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

Missed one thing: BluetootServiceBluedroid should implement EnergyInfoNotification as LeTestModeNotification [1][2], right? Is it written in another patch?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.h#212
[2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp#1619
Comment on attachment 8541678 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.v2.patch

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
@@ +340,5 @@
> +               bool aShouldWake,
> +               AlarmCallback aAlarmCallback,
> +               void* aData)
> +  {
> +    // TODO: to be added

'to be implemented' is clearer.

@@ +348,5 @@
> +  static int
> +  AcquireWakeLock(const char* aLockName)
> +  {
> +    // TODO: to be added
> +    return BT_STATUS_FAIL;

Leave comment to explain why return BT_STATUS_FAIL here.

@@ +355,5 @@
> +  static int
> +  ReleaseWakeLock(const char* aLockName)
> +  {
> +    // TODO: to be added
> +    return BT_STATUS_FAIL;

Ditto.

@@ +466,5 @@
> +  static bt_os_callouts_t sBluetoothOsCallouts = {
> +      sizeof(sBluetoothOsCallouts),
> +      BluetoothOsCallout::SetWakeAlarm,
> +      BluetoothOsCallout::AcquireWakeLock,
> +      BluetoothOsCallout::ReleaseWakeLock

nit: 2-space indention
Attachment #8541678 - Flags: review?(btian) → review+
Comment on attachment 8541682 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.core.v2.patch

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.h
@@ +27,5 @@
>      OPCODE_CONNECT = 0x01,
>      OPCODE_DISCONNECT = 0x02
>    };
>  
> +  static const int MAX_CLIENTS;

MAX_NUM_CLIENTS is clearer. Please revise all the following.

@@ +33,4 @@
>    virtual nsresult Send(BluetoothDaemonPDU* aPDU, void* aUserData) = 0;
>  
>    virtual nsresult RegisterModule(uint8_t aId, uint8_t aMode,
> +                                  uint32_t aMaxClients,

|aMaxNumClients|
Attachment #8541682 - Flags: review?(btian) → review+
Comment on attachment 8541684 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.a2dp.v2.patch

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

LGTM
Attachment #8541684 - Flags: review?(btian) → review+
Comment on attachment 8541686 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.v2.patch

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +1353,5 @@
>  
>      /* Read address */
> +#if ANDROID_VERSION >= 21
> +    rv = UnpackPDU(
> +      pdu, UnpackConversion<BluetoothAddress, nsAString>(aArg2));

I found that I didn't handle the order of parameters in UnknownAtInitOp as expected. aArg2 is needed to be parsed first for bluez 5.25 (just like DialCallInitOp). I will update the corresponding patch later.
Comment on attachment 8541675 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.v2.patch

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

Please see my comment below.

::: dom/bluetooth/BluetoothInterface.h
@@ +194,5 @@
>  {
>  public:
>    virtual void Init(
>      BluetoothHandsfreeNotificationHandler* aNotificationHandler,
> +    int aMaxHandsfreeClients,

Revise to |aMaxNumHandsfreeClients| and all other places.

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +419,5 @@
> +  BluetoothHandsfreeResultHandler* aRes)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  // TODO: to be added

'to be implemented'

@@ +678,5 @@
>      if (NS_FAILED(rv)) {
>        return rv;
>      }
> +
> +    if (aArg1 == HFP_CONNECTION_STATE_CONNECTED) {

Leave comment to explain why we need to store device address.

@@ +850,5 @@
>  
>    nsresult
> +  operator () (BluetoothHandsfreeVolumeType& aArg1,
> +               int& aArg2,
> +               nsString& aArg3) const

nit: wrap these two lines into one.

@@ +931,5 @@
> +  { }
> +
> +  nsresult
> +  operator () (char& aArg1,
> +               nsString& aArg2) const

nit: wrap these two lines into one.

@@ +969,5 @@
> +  { }
> +
> +  nsresult
> +  operator () (BluetoothHandsfreeNRECState& aArg1,
> +               nsString& aArg2) const

nit: wrap these two lines into one.

@@ +1007,5 @@
> +  { }
> +
> +  nsresult
> +  operator () (BluetoothHandsfreeCallHoldType& aArg1,
> +               nsString& aArg2) const

nit: wrap these two lines into one.

@@ +1169,5 @@
>    { }
>  
>    nsresult
> +  operator () (nsCString& aArg1,
> +               nsString& aArg2) const

nit: wrap these two lines into one.

@@ +1312,1 @@
>    BluetoothHandsfreeResultHandler* aRes)

nit: wrap these two lines into one.

@@ +1485,1 @@
>    BluetoothHandsfreeResultHandler* aRes)

nit: wrap these two lines into one.

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +166,5 @@
>    static void
>    ConnectionState(bthf_connection_state_t aState, bt_bdaddr_t* aBdAddr)
>    {
> +    {
> +      MutexAutoLock al(sMutexBluetoothAddress);

Rename |al| to |lock| to be self-explanatory and all the following.

@@ +169,5 @@
> +    {
> +      MutexAutoLock al(sMutexBluetoothAddress);
> +      if (aState == BTHF_CONNECTION_STATE_CONNECTED && aBdAddr) {
> +        memcpy(&sConnectedBluetoothAddress,
> +               aBdAddr,

nit: wrap these two lines into one.

@@ +173,5 @@
> +               aBdAddr,
> +               sizeof(sConnectedBluetoothAddress));
> +      } else if (aState == BTHF_CONNECTION_STATE_DISCONNECTED) {
> +        memset(&sConnectedBluetoothAddress,
> +               0,

nit: wrap these two lines into one.

@@ +176,5 @@
> +        memset(&sConnectedBluetoothAddress,
> +               0,
> +               sizeof(sConnectedBluetoothAddress));
> +      }
> +    }

nit: newline here.

@@ +318,3 @@
>    }
> +
> +  static bt_bdaddr_t sConnectedBluetoothAddress;

Leave comment to explain why we need to store device address.

@@ +318,4 @@
>    }
> +
> +  static bt_bdaddr_t sConnectedBluetoothAddress;
> +  static Mutex sMutexBluetoothAddress;

Rename to |sConnectedDeviceAddress| and |sMutexDeviceAddress| to be clearer, and modify all the others.

@@ +703,5 @@
> +  const nsAString& aBdAddr,
> +  BluetoothHandsfreeWbsConfig aConfig,
> +  BluetoothHandsfreeResultHandler* aRes)
> +{
> +  // TODO: to be added

'to be implemented'

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +794,5 @@
>  {
>    NS_ENSURE_TRUE_VOID(sBluetoothHfpInterface);
>  
>    sBluetoothHfpInterface->FormattedAtResponse(
> +    aMessage, mDeviceAddress, new FormattedAtResponseResultHandler());

Add argument |aBdAddress| to functions |SendLine| and |SendResponse| rather than passing |mDeviceAddress|.

@@ +1534,5 @@
> +  sBluetoothHfpInterface->CindResponse(
> +    mService, numActive, numHeld,
> +    callState, mSignal, mRoam, mBattChg,
> +    aBdAddress,
> +    new CindResponseResultHandler());

nit: wrap these two lines into one.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h
@@ +76,5 @@
>  {
>  public:
>    BT_DECL_HFP_MGR_BASE
>  
> +  static const int MAX_CLIENTS;

MAX_NUM_CLIENTS is clearer.
Attachment #8541675 - Flags: review?(btian)
Comment on attachment 8541680 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v2.patch

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +120,5 @@
>      BluetoothHandsfreeNRECState, nsString,
>      BluetoothHandsfreeNRECState, const nsAString&>
>      NRECNotification;
>  
> +    typedef BluetoothNotificationHALRunnable2<

nit: 2-space indention.

@@ +186,5 @@
>                 0,
>                 sizeof(sConnectedBluetoothAddress));
>        }
>      }
> +#endif

nit: newline here

@@ +200,5 @@
>        &BluetoothHandsfreeNotificationHandler::AudioStateNotification,
>        aState, aBdAddr);
>    }
>  
> +#if ANDROID_VERSION >= 21

Suggest to group functions of |ANDROID_VERSION >= 21| as following. To me it's more understandable.

#if ANDROID_VERSION >= 21
  // functions for L
#else
  // functions
#endif

@@ +213,4 @@
>    static void
>    VoiceRecognition(bthf_vr_state_t aState)
>    {
>      MutexAutoLock al(sMutexBluetoothAddress);

Rename to |lock|.

@@ +482,5 @@
> +   * of the connected device. Before android lollipop, this address should be
> +   * maintained by ourselves through ConnectionState(). After android lollipop,
> +   * the address of the connected device is directly carried in every callback,
> +   * and we don't need to maintain this address by ourselves afterward.
> +   */

Suggest to simplify as following:

  /* |sConnectedBluetoothAddress| stores bluetooth device address of the
   * connected device. Before bluez 5.25, we maintain this address by ourselves
   * through ConnectStateNtf(); after bluez 5.25, every callback carries this
   * address directly so we don't have to keep it.
   */
Attachment #8541680 - Flags: review?(btian) → review+
Comment on attachment 8541686 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.v2.patch

Clear r? per comment 97.
Attachment #8541686 - Flags: review?(btian)
Sorry for letting you wait for so long. I'll be back form PTO on Monday and will provide feedback on the patch set.
(In reply to Ben Tian [:btian] from comment #98)
> Comment on attachment 8541675 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.v2.patch
> 
> Review of attachment 8541675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment below.
> 
> ::: dom/bluetooth/BluetoothInterface.h
> @@ +194,5 @@
> >  {
> >  public:
> >    virtual void Init(
> >      BluetoothHandsfreeNotificationHandler* aNotificationHandler,
> > +    int aMaxHandsfreeClients,
> 
> Revise to |aMaxNumHandsfreeClients| and all other places.
> 

I will replace |aMaxHandsfreeClients| with |aMaxNumClients| since |Handsfree| is already contained in the class name.

> ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
> @@ +678,5 @@
> >      if (NS_FAILED(rv)) {
> >        return rv;
> >      }
> > +
> > +    if (aArg1 == HFP_CONNECTION_STATE_CONNECTED) {
> 
> Leave comment to explain why we need to store device address.
> 

Sorry that I didn't describe the usage of this variable in this patch clearly.

Like other modifications in this patch, this variable is used to compensate the expansion of internal interfaces. Since comments of this variable will be overwritten immediately by later patches, I just leave it empty here.

Comments will be added in the corresponding porting patches (daemon.handsfree and hal.handsfree) to describe how this variable is used to handle interface differences.

> ::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
> @@ +318,3 @@
> >    }
> > +
> > +  static bt_bdaddr_t sConnectedBluetoothAddress;
> 
> Leave comment to explain why we need to store device address.
> 

See comments above.

> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +794,5 @@
> >  {
> >    NS_ENSURE_TRUE_VOID(sBluetoothHfpInterface);
> >  
> >    sBluetoothHfpInterface->FormattedAtResponse(
> > +    aMessage, mDeviceAddress, new FormattedAtResponseResultHandler());
> 
> Add argument |aBdAddress| to functions |SendLine| and |SendResponse| rather
> than passing |mDeviceAddress|.
> 

As discussed offline, the modification of using |BluetoothHfpManager::mDeviceAddress| is not so related to L porting. It would be better to handle this on a separated bug.
(In reply to Ben Tian [:btian] from comment #99)
> Comment on attachment 8541680 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v2.patch
> 
> Review of attachment 8541680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed.
> 
> ::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
> @@ +200,5 @@
> >        &BluetoothHandsfreeNotificationHandler::AudioStateNotification,
> >        aState, aBdAddr);
> >    }
> >  
> > +#if ANDROID_VERSION >= 21
> 
> Suggest to group functions of |ANDROID_VERSION >= 21| as following. To me
> it's more understandable.
> 
> #if ANDROID_VERSION >= 21
>   // functions for L
> #else
>   // functions
> #endif
> 

By grouping these functions as suggested, codes will become cleaner and shorter. But it would be a little bit harder to tell the difference of the argument list by simply glancing codes without scrolling hundreds of lines and comparing function-by-function carefully. And probably the way to understand the usage of |sConnectedDeviceAddress| is only through the comments.

If we put these functions in the original way in this patch, it is easier to compare the difference of each callback function on different android version. And it would be more intuitive to understand why |sConnectedDeviceAddress| is added and how it is used to compensate the expansion of the argument list on different android version. Codes themselves could deliver more information than only comments.

Both styles have its pros and cons. What do you think?
Flags: needinfo?(btian)
Got your point. Feel free to keep original '#ifdef-for-each-function' approach. Thanks for explaining!
Flags: needinfo?(btian)
Send internal.core for review again per comment 93

Modification:
 - Add dummy implementation for BluetoothServiceBluedroid::EnergyInfoNotification().
Attachment #8541674 - Attachment is obsolete: true
Attachment #8541674 - Flags: feedback?(tzimmermann)
Attachment #8543819 - Flags: review?(btian)
Attachment #8543819 - Flags: feedback?(tzimmermann)
Modification:
 - Replace |aMaxHandsfreeClients| with |aMaxNumClients| in |BluetoothHandsfreeInterface::Init()|.
 - Replace |BluetoothDaemonHandsfreeModule::sConnectedBluetoothAddress| with |BluetoothDaemonHandsfreeModule::sConnectedDeviceAddress|.
 - Refine comments from |//TODO: to be added| to |// TODO: to be implemented|
 - Merge multiple lines into one line if possible.
 - Replace |aMaxHandsfreeClients| with |aMaxNumClients| in |BluetoothDaemonHandsfreeInterface::Init()|.
 - Replace the name of |MutexAutoLock| from |al| to |lock|.
 - Replace |BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress| with |BluetoothHandsfreeHALCallback::sConnectedDeviceAddress|.
 - Replace |BluetoothHandsfreeHALCallback::sMutexBluetoothAddress| with |BluetoothHandsfreeHALCallback::sMutexDeviceAddress|.
 - Replace |aMaxHandsfreeClients| with |aMaxNumClients| in |BluetoothHandsfreeHALInterface::Init()|.
 - Replace |BluetoothHfpManager::MAX_CLIENTS| with |BluetoothHfpManager::MAX_NUM_CLIENTS|.
Attachment #8541675 - Attachment is obsolete: true
Attachment #8541675 - Flags: feedback?(tzimmermann)
Attachment #8543825 - Flags: review?(btian)
Attachment #8543825 - Flags: feedback?(tzimmermann)
Modification:
 - Correct the parsing order of parameters in |UnknownAtInitOp()|.
 - Refine comments of |BluetoothDaemonHandsfreeModule::sConnectedDeviceAddress|.
Attachment #8541686 - Attachment is obsolete: true
Attachment #8541686 - Flags: feedback?(tzimmermann)
Attachment #8543831 - Flags: review?(btian)
Attachment #8543831 - Flags: feedback?(tzimmermann)
Comment on attachment 8543819 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.core.v3.patch

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

LGTM
Attachment #8543819 - Flags: review?(btian) → review+
Comment on attachment 8543825 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.v3.patch

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

LGTM.
Attachment #8543825 - Flags: review?(btian) → review+
Comment on attachment 8543831 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.v3.patch

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

LGTM.
Attachment #8543831 - Flags: review?(btian) → review+
Comment on attachment 8541678 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.v2.patch

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

f- because of the plain assignment in the Convert function and the missing nullptr check on aEnergyInfo.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.cpp
@@ +220,5 @@
> +  aOut.mStackState = aIn.ctrl_state;
> +  aOut.mTxTime = aIn.tx_time;
> +  aOut.mRxTime = aIn.rx_time;
> +  aOut.mIdleTime = aIn.idle_time;
> +  aOut.mEnergyUsed = aIn.energy_used;

You cannot just assign here, you need to convert these values. And you need to check for overflows in the data types, even for simple built-in types. There are many examples in this file on how to do that. You've already done it correctly in other places.

::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
@@ +320,5 @@
>    }
> +
> +#if ANDROID_VERSION >= 21
> +  static void
> +  EnergyInfo(bt_activity_energy_info *aEnergyInfo)

Nit: please put the * directly after the type's name.

@@ +321,5 @@
> +
> +#if ANDROID_VERSION >= 21
> +  static void
> +  EnergyInfo(bt_activity_energy_info *aEnergyInfo)
> +  {

Some weird BT driver might decide to give us a nullptr. You should add some thing like

if (NS_WARN_IF(!aEnergyInfo)) {
  return;
}

here.
Attachment #8541678 - Flags: feedback?(tzimmermann) → feedback-
Comment on attachment 8541680 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v2.patch

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

f- because of the locking and the missing nullptr check.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
@@ +781,5 @@
>  }
>  
>  #if ANDROID_VERSION >= 21
>  nsresult
>  Convert(const bt_activity_energy_info& aIn, BluetoothActivityEnergyInfo& aOut);

Nit: Add an empty line between this declaration and the next function.

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +11,2 @@
>  #include <mozilla/Mutex.h>
> +#endif

I understand that you want to be careful and not run into race conditions, but there is no evidence that there has ever been more than one thread for Bluedroid's callback-functions. You should check out the Bluedroid backend code from ~9 months ago. We did not have this elaborate wrapping code that we use now, but used Bluedroid directly. We never needed an extra lock for protecting callbacks against each other.

If you cannot find evidence that this lock is required, please remove it.

@@ +179,5 @@
>        MutexAutoLock al(sMutexBluetoothAddress);
>        if (aState == BTHF_CONNECTION_STATE_CONNECTED && aBdAddr) {
>          memcpy(&sConnectedBluetoothAddress,
>                 aBdAddr,
>                 sizeof(sConnectedBluetoothAddress));

You have to check if aBdAddr is nullptr before calling memcpy.

@@ +200,5 @@
>        &BluetoothHandsfreeNotificationHandler::AudioStateNotification,
>        aState, aBdAddr);
>    }
>  
> +#if ANDROID_VERSION >= 21

I don't have a strong opinion, but I suggest a case-by-case evaluation. To me, sometimes one or the other is easier to read.

@@ +336,3 @@
>  
> +#if ANDROID_VERSION >= 21
> +    static void

Weird indention, here and below.

@@ +479,3 @@
>  
> +#if ANDROID_VERSION < 21
> +  /* sConnectedBluetoothAddress is used to store the bluetooth device address

Nit: 'Bluetooth' with capital 'B'.

@@ +479,4 @@
>  
> +#if ANDROID_VERSION < 21
> +  /* sConnectedBluetoothAddress is used to store the bluetooth device address
> +   * of the connected device. Before android lollipop, this address should be

'Lollipop' with capital 'L', here and below.
Attachment #8541680 - Flags: feedback?(tzimmermann) → feedback-
Comment on attachment 8541682 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.core.v2.patch

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

f+ with issue fixed.

::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.cpp
@@ +343,5 @@
>      res = nullptr;
>    }
>  
>    nsresult rv = mModule->RegisterModule(BluetoothDaemonA2dpModule::SERVICE_ID,
> +    BluetoothDaemonA2dpModule::MAX_CLIENTS, 0x00, res);

'0x00' must go first, 'MAX_CLIENT' second.

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.h
@@ +362,5 @@
>                   BluetoothDaemonPDU& aPDU,
>                   void* aUserData);
>  
>    static BluetoothHandsfreeNotificationHandler* sNotificationHandler;
> +

Nit: pointless newline.

@@ +367,1 @@
>    static nsString sConnectedBluetoothAddress;

I'm undecided about this static qualifier. I see that you use this variable in the InitOps and that it's easier this way.

But I think this design is very un-clean and ugly. The correct solution is to make the field a regular class member and pass it as reference to InitOp classes on all versions before L.
Attachment #8541682 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 8541684 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.a2dp.v2.patch

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

Looks good.

::: dom/bluetooth/bluedroid/BluetoothDaemonA2dpInterface.h
@@ +106,5 @@
>      AudioStateNotification;
>  
> +  typedef BluetoothNotificationRunnable3<NotificationHandlerWrapper, void,
> +                                         nsString, uint32_t, uint8_t,
> +                                         const nsAString&, uint32_t, uint8_t>

BTW, You can omit trailing parameter types if they are of the same as the data types in the notification; here this would be ', uint32_t, uint8_t'.
Attachment #8541684 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 8543819 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.core.v3.patch

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

Only f+ if you have a good answer to my question ;)

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1984,5 @@
> +BluetoothDaemonInterface::GetConnectionState(const nsAString& aBdAddr,
> +                                             BluetoothResultHandler* aRes)
> +{
> +  // NO-OP: no corresponding interface of current bluez
> +}

What's the point of this method? Active connections should be maintained in the profile managers.
Attachment #8543819 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 8543825 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.v3.patch

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

f+ because I already complained about the locking and the static field in other reviews. The rest is nits.

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +18,5 @@
>    BluetoothDaemonHandsfreeModule::sNotificationHandler;
>  
> +nsString BluetoothDaemonHandsfreeModule::sConnectedDeviceAddress(
> +  NS_ConvertUTF8toUTF16(BLUETOOTH_ADDRESS_NONE));
> +

As I said, I'm undecided here and like to see this being fixed sooner or later. (Rather sooner)

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +127,5 @@
>  
>    typedef BluetoothNotificationHALRunnable1<
> +    HandsfreeNotificationHandlerWrapper, void,
> +    nsString,
> +    const nsAString&>

Please put these parameters on one line if possible. The file is already large enough. Here and below.
Attachment #8543825 - Flags: feedback?(tzimmermann) → feedback+
Comment on attachment 8543831 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.v3.patch

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

f+ with nits.

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +157,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsAutoPtr<BluetoothDaemonPDU> pdu(
>      new BluetoothDaemonPDU(SERVICE_ID, OPCODE_START_VOICE_RECOGNITION,
> +                           6)); // Address (bluez 5.25)

1) 'BlueZ' with capital letters 'B' and 'Z'. Here and everywhere else.

2) I thought we use 5.26 as reference document?

@@ +174,5 @@
>      return rv;
>    }
>    unused << pdu.forget();
>    return NS_OK;
>  }

Nit: The new-lining in this method looks weird. I suggest to either remove new-lines at lines 162 and 171, or add a new-line below line 163. Same for the method below.
Attachment #8543831 - Flags: feedback?(tzimmermann) → feedback+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #112)
> Comment on attachment 8541680 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v2.patch
> 
> Review of attachment 8541680 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f- because of the locking and the missing nullptr check.
> 
> ::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
> @@ +11,2 @@
> >  #include <mozilla/Mutex.h>
> > +#endif
> 
> I understand that you want to be careful and not run into race conditions,
> but there is no evidence that there has ever been more than one thread for
> Bluedroid's callback-functions. You should check out the Bluedroid backend
> code from ~9 months ago. We did not have this elaborate wrapping code that
> we use now, but used Bluedroid directly. We never needed an extra lock for
> protecting callbacks against each other.
> 
> If you cannot find evidence that this lock is required, please remove it.
> 

Actually, I don't have evidences that we will never have any race conditions regarding to this static variable.

The lock is not necessary before L porting because all necessary data are cached in individual instance of XXXNotification runnables. There are no race conditions no matter how the threading module of the underlying library.

But now |sConnectedDeviceAddress| will be accessed in different callbacks. It is dangerous if we don't have any guarantee (ex. identifying and asserting on the single callback thread) to avoid the race condition.

Since this static variable is only necessary before android lollipop, and android lollipop is expected to become popular after gecko v2.2, this potential risk might be getting lower and lower.

I will remove the lock and update the patch again. Probably we should keep in mind that there might be race-condition bugs while others want to port gecko on any other potential platforms.

> @@ +179,5 @@
> >        MutexAutoLock al(sMutexBluetoothAddress);
> >        if (aState == BTHF_CONNECTION_STATE_CONNECTED && aBdAddr) {
                                                            ^^^^^^^
> >          memcpy(&sConnectedBluetoothAddress,
> >                 aBdAddr,
> >                 sizeof(sConnectedBluetoothAddress));
> 
> You have to check if aBdAddr is nullptr before calling memcpy.
> 

Null value checking on aBdAddr exists in the patch.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #113)
> Comment on attachment 8541682 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.daemon.core.v2.patch
> 
> Review of attachment 8541682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ with issue fixed.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.h
> @@ +367,1 @@
> >    static nsString sConnectedBluetoothAddress;
> 
> I'm undecided about this static qualifier. I see that you use this variable
> in the InitOps and that it's easier this way.
> 
> But I think this design is very un-clean and ugly. The correct solution is
> to make the field a regular class member and pass it as reference to InitOp
> classes on all versions before L.

Understand. I've tried the suggested approach a little bit, but the codes become a little complex after adapting this modification (ex. some InitOps need to have an extra pointer parameter in the constructor for the module or the address, but some InitOps don't.)

Since this static variable is only needed before android lollipop, and android lollipop is supposed to become more and more popular, probably we could use this static variable in order to keep our codes cleaner and more maintainable.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #115)
> Comment on attachment 8543819 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.internal.core.v3.patch
> 
> Review of attachment 8543819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only f+ if you have a good answer to my question ;)
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1984,5 @@
> > +BluetoothDaemonInterface::GetConnectionState(const nsAString& aBdAddr,
> > +                                             BluetoothResultHandler* aRes)
> > +{
> > +  // NO-OP: no corresponding interface of current bluez
> > +}
> 
> What's the point of this method? Active connections should be maintained in
> the profile managers.

|bt_interface_t.get_connection_state| is added since android lollipop. So |BluetoothInterface::GetConnectionState()| and |BluetoothHALInterface::GetConnectionState()| and |BluetoothDaemonInterface::GetConnectionState()| are added correspondingly. But since BlueZ doesn't expose the corresponding interface until now, so the implementation of |BluetoothDaemonInterface::GetConnectionState()| is just empty.

Do we need to remove this function?
Flags: needinfo?(tzimmermann)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #117)
> Comment on attachment 8543831 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.daemon.handsfree.v3.patch
> 
> Review of attachment 8543831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ with nits.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
> @@ +157,5 @@
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> >    nsAutoPtr<BluetoothDaemonPDU> pdu(
> >      new BluetoothDaemonPDU(SERVICE_ID, OPCODE_START_VOICE_RECOGNITION,
> > +                           6)); // Address (bluez 5.25)
> 
> 1) 'BlueZ' with capital letters 'B' and 'Z'. Here and everywhere else.
> 
> 2) I thought we use 5.26 as reference document?
> 

The additional parameter (Bluetooth device address) is added since BlueZ 5.25.

The latest version of BlueZ goes from 5.24 to 5.27 within these two months. It seems BlueZ hasn't finalized its interface yet.

> @@ +174,5 @@
> >      return rv;
> >    }
> >    unused << pdu.forget();
> >    return NS_OK;
> >  }
> 
> Nit: The new-lining in this method looks weird. I suggest to either remove
> new-lines at lines 162 and 171, or add a new-line below line 163. Same for
> the method below.

I will remove only line 171 but keep line 162 (and for other method below as well) to sync the coding style with other unmodified functions.
> 
> |bt_interface_t.get_connection_state| is added since android lollipop. So
> |BluetoothInterface::GetConnectionState()| and
> |BluetoothHALInterface::GetConnectionState()| and
> |BluetoothDaemonInterface::GetConnectionState()| are added correspondingly.
> But since BlueZ doesn't expose the corresponding interface until now, so the
> implementation of |BluetoothDaemonInterface::GetConnectionState()| is just
> empty.
> 
> Do we need to remove this function?

I missed this new interface, sorry about the noise. f+ in that case, of course.
Flags: needinfo?(tzimmermann)
Hi

> Actually, I don't have evidences that we will never have any race conditions
> regarding to this static variable.
> 
> The lock is not necessary before L porting because all necessary data are
> cached in individual instance of XXXNotification runnables. There are no
> race conditions no matter how the threading module of the underlying library.
> 
> But now |sConnectedDeviceAddress| will be accessed in different callbacks.
> It is dangerous if we don't have any guarantee (ex. identifying and
> asserting on the single callback thread) to avoid the race condition.

It's more than data access. If we operate under the assumption that callbacks run on different threads, no order of Bluetooth operations would be guaranteed. That means that we could see callbacks before we even know that we enabled Bluetooth successfully; in the extreme case, we'd see the 'disable callback' before the 'enable callback'. IOW the whole API wouldn't work if there was than one callback thread.

> I will remove the lock and update the patch again.

Thank you. :)
(In reply to Bruce Sun [:brsun] from comment #70)
> (In reply to Bruce Sun [:brsun] from comment #69)
> > Bug 1115473 is create for implementing bt_os_callouts_t with full
> > functionalities on android lollipop (HAL).
> 
> We also need to handle bt_os_callouts_t in daemon side on android lollipop,
> but I am not sure whether we can customize the used protocol to acquire /
> release wake locks between daemon and gecko. I don't have any idea right now.

Do you know the use case for these wake locks? Bluedroid already holds wake locks internally. It looks like they just added a callback interface to let the application implement the low-level calls.

The documentation for wake locks is scarce. What I took away from it was that processes hold a wake lock while they have unread data in one of their file buffers. That means that we don't have much to do here:

 (1) Bluedroid acquires the wake lock it needs for running callbacks
 (2) Gecko doesn't hold wake locks now and we probably don't have to change this
 (3) the daemon might need a wake lock while it process incoming commands

I've not yet seen a problem because we don't implement Option (3), but it's on my list for possible improvements. Options (1) and (2) should be fine already.
Modification:
 - Merge multiple lines into one line if possible.
 - Remove |BluetoothHandsfreeHALCallback::sMutexDeviceAddress|
Attachment #8543825 - Attachment is obsolete: true
Attachment #8544462 - Flags: review?(tzimmermann)
Modification:
 - Use Convert() to convert the value of each member of bt_activity_energy_info into each corresponding member of BluetoothActivityEnergyInfo.
 - Replace |EnergyInfo(bt_activity_energy_info *aEnergyInfo)| with |EnergyInfo(bt_activity_energy_info* aEnergyInfo)|.
 - Add nullptr checking of |aEnergyInfo| in |EnergyInfo()|.
 - Add more comments in |SetWakeAlarm()|, |AcquireWakeLock()|, and |ReleaseWakeLock()|.
 - Fix weird indents of |sBluetoothOsCallouts|.
 - Replace comments |// TODO: to be added| with |// TODO: to be implemented|.
Attachment #8541678 - Attachment is obsolete: true
Attachment #8544466 - Flags: review?(tzimmermann)
Modification:
 - Add one empty line below |Convert(const bt_activity_energy_info...)| declaration.
 - Fix weird indents of |WbsNotification|.
 - Replace |BluetoothHandsfreeHALCallback::sConnectedBluetoothAddress| with |BluetoothHandsfreeHALCallback::sConnectedDeviceAddress|.
 - Add one empty line in |ConnectionState()|.
 - Fix weird indents of |WideBandSpeech()| and |CallHold()|.
 - Refine comments of |BluetoothHandsfreeHALCallback::sConnectedDeviceAddress|.
 - Replace |aMaxHandsfreeClients| with |aMaxNumClients| in |BluetoothHandsfreeHALInterface::Init()|.
 - Replace |// TODO: to be added| with |// TODO: to be implemented|.
Attachment #8541680 - Attachment is obsolete: true
Attachment #8544468 - Flags: review?(tzimmermann)
Comment on attachment 8544468 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v3.patch

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

Looks good.
Attachment #8544468 - Flags: review?(tzimmermann)
Comment on attachment 8544466 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.v3.patch

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

r+ with comments.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.cpp
@@ +214,5 @@
> +#if ANDROID_VERSION >= 21
> +nsresult
> +Convert(const bt_activity_energy_info& aIn, BluetoothActivityEnergyInfo& aOut)
> +{
> +  nsresult rv;

Nit: in C++ variables should be declared when you first use them.

@@ +246,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  return NS_OK;

Nit: don't put new-lines between the blocks. The file is long enough.

::: dom/bluetooth/bluedroid/BluetoothHALHelpers.h
@@ +765,5 @@
>  }
>  #endif // ANDROID_VERSION >= 19
>  
> +inline nsresult
> +Convert(BluetoothTransport aIn, int& aOut)

I think this function needs to be covered by ANDROID_VERSION >= 21 as well.
Attachment #8544466 - Flags: review?(tzimmermann) → review+
Comment on attachment 8544462 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.internal.handsfree.v4.patch

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

This looks good.

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +744,5 @@
> +{
> +public:
> +  VoiceRecognitionInitOp(BluetoothDaemonPDU& aPDU)
> +    : PDUInitOp(aPDU)
> +  { }

One-argument constructors are supposed to be marked with MOZ_EXPLICIT to prevent implicit type conversion. I forgot about this in the previous reviews and won't request it now. Nothing else in the source code uses MOZ_EXPLICIT. But we should keep this rule in mind and be strict about it in the future.

::: dom/bluetooth/bluedroid/BluetoothHandsfreeHALInterface.cpp
@@ +153,5 @@
>  
>    static void
>    ConnectionState(bthf_connection_state_t aState, bt_bdaddr_t* aBdAddr)
>    {
> +    if (aState == BTHF_CONNECTION_STATE_CONNECTED && aBdAddr) {

Sorry that I didn't noticed the nullptr check in the previous review.
Attachment #8544462 - Flags: review?(tzimmermann) → review+
Bruce,

before you land this patch set, please build on a JB and a KK device first. This kind of patches tends to break easily on different platforms.

I also strongly suggest to have the daemon side in a working state before landing, so that you can test the protocol code at least once.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #128)
> Comment on attachment 8544468 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v3.patch
> 
> Review of attachment 8544468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.

May I treat it as a review plus? :)
Comment on attachment 8544468 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.handsfree.v3.patch

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

Oops, looks like I picked the wrong entry. :)
Attachment #8544468 - Flags: review+
Comment on attachment 8545197 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.v4.patch

I've tried all these porting patches with hal/daemon (attachment 8542872 [details] [diff] [review]). It seems we have troubles if we simply fails three callout functions in |bt_os_callouts_t| (ex. bluetooth cannot switch on-off twice, music playback stocks if bluetooth a2dp device is connected.) If we cheat the bluetooth backend with a success return value on alarm and wake_lock requests, we will have less trouble at this stage before a real implementation of |bt_os_callouts_t| has been done.

So I am going to change return values of three callout functions in |bt_os_callouts_t| with a fake success.

Modification:
 - Refine coding style of |Convert(const bt_activity_energy_info...| as comment 129.
 - Move |Convert(BluetoothTransport aIn, int& aOut)| into the scope of |ANDROID_VERSION >= 21|.
 - Change the return value of |SetWakeAlarm()| to |true|.
 - Change the return value of |AcquireWakeLock()| to |BT_STATUS_SUCCESS|.
 - Change the return value of |ReleaseWakeLock()| to |BT_STATUS_SUCCESS|.
Attachment #8545197 - Flags: review?(tzimmermann)
Attachment #8545197 - Flags: review?(shuang)
Attachment #8545197 - Flags: feedback?
Comment on attachment 8545197 [details] [diff] [review]
bug1102703_enable_bluetooth_android_lollipop.hal.core.v4.patch

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

I think that should be OK for now. Be aware that the HAL backend is on it's way out, so you might not want to spend too much time on it. The HAL will be a fallback in 2.2 until our daemon got more testing. Fixing these issue in the daemon is more important. There we can simply pick a wake lock and create a timerfd.

::: dom/bluetooth/bluedroid/BluetoothHALInterface.cpp
@@ +336,5 @@
>  
> +#if ANDROID_VERSION >= 21
> +struct BluetoothOsCallout
> +{
> +  typedef void (*AlarmCallback)(void *aData);

I don't know if we have a style rule regarding function typedefs. For the daemon, I'd prefer to not use a typedef, but write-out the declaration when needed.
Attachment #8545197 - Flags: review?(tzimmermann) → review+

        Attachment #8529244 -
        Attachment is obsolete: true

        Attachment #8531364 -
        Attachment is obsolete: true

        Attachment #8543819 -
        Attachment is obsolete: true

        Attachment #8545721 -
        Flags: review+

    
    

    
  


  

        Attachment #8541196 -
        Attachment is obsolete: true

        Attachment #8545722 -
        Flags: review+

    
    

    
  


  

        Attachment #8544462 -
        Attachment is obsolete: true

        Attachment #8545723 -
        Flags: review+

    
    

    
  


  

        Attachment #8544466 -
        Attachment is obsolete: true

        Attachment #8545724 -
        Flags: review+

    
  

        Attachment #8545197 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8541203 -
        Attachment is obsolete: true

        Attachment #8545725 -
        Flags: review+

    
    

    
  


  

        Attachment #8544468 -
        Attachment is obsolete: true

        Attachment #8545726 -
        Flags: review+

    
    

    
  


  

        Attachment #8541682 -
        Attachment is obsolete: true

        Attachment #8545728 -
        Flags: review+

    
    

    
  


  

        Attachment #8541684 -
        Attachment is obsolete: true

        Attachment #8545729 -
        Flags: review+

    
    

    
  


  

        Attachment #8543831 -
        Attachment is obsolete: true

        Attachment #8545730 -
        Flags: review+

    
    

    
  


  

        Attachment #8541211 -
        Attachment is obsolete: true

        Attachment #8545732 -
        Flags: review+

    
    

    
  
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #136)
> Comment on attachment 8545197 [details] [diff] [review]
> bug1102703_enable_bluetooth_android_lollipop.hal.core.v4.patch
> 
> Review of attachment 8545197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that should be OK for now. Be aware that the HAL backend is on it's
> way out, so you might not want to spend too much time on it. The HAL will be
> a fallback in 2.2 until our daemon got more testing. Fixing these issue in
> the daemon is more important. There we can simply pick a wake lock and
> create a timerfd.
> 

The bug for bt_os_callouts_t implementation in bluetoothd is created as bug 1119176.

    
    

    
  
So sad. It seems there are extra efforts we need to handle after bug 1095488 has been resolved. I'll update my patches later again.

    
    

    
  
Yap! The patch of bug 1095488 might conflict with this bug. We need to rebase again and add avrcp support for lollipop again.

    
  
Blocks: 1115473

    
    

    
  
Please land changes like these on b2g-inbound so we don't end up hitting bustage when this merges from mozilla-central and somebody else has landed patches that conflict. In general, B2G-specific code should land on b-i and cross-platform Gecko code should land on m-i.

And yes, bustage scenarios like these do happen (yesterday actually!)
Flags: needinfo?(shuang)

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #165)
> Please land changes like these on b2g-inbound so we don't end up hitting
> bustage when this merges from mozilla-central and somebody else has landed
> patches that conflict. In general, B2G-specific code should land on b-i and
> cross-platform Gecko code should land on m-i.
> 
> And yes, bustage scenarios like these do happen (yesterday actually!)

Hi Ryan,
I'm sorry I landed patches into m-i. I will be careful next time.
Flags: needinfo?(shuang)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: