1004896 - Add CAF bluedroid flag

Status: RESOLVED INCOMPLETE

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Enable bluedroid as default bluetooth stack

Comment 1

[howie [:howie]]

added target milestone, feel free to update it, thanks

Comment 2

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

Attachment: Bug 1004896 - Add bluedroid library bluetooth.default (Patch 1)

Comment 3

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

Attachment: Bug 1004896 - Use bluedroid as the default bluetooth stack (b2g-manifest)(Patch 4)

Comment 4

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

Attachment: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 3)

Comment 5

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

Attachment: Bug 1004896 - Expose bluedroid stack variants (Patch 4)

Comment 6

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

Comment on attachment 8419198 [details] [review]
Bug 1004896 - Add bluedroid library bluetooth.default (Patch 1)

Using BOARD_HAVE_BLUETOOTH_QCOM seems not a good flag, since it's possible to use qc chipset but uses bluedroid library from caf.

Comment 7

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

Attachment: Bug 1004896 - Expose bluedroid stack variants (gonk-misc) (Patch 4)

Comment 8

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

Attachment: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 1)

Comment 9

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

Attachment: Bug 1004896 - Add bluedroid library bluetooth.default (BoardConfig) (Patch 3)

Comment 10

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

Given the fact that bluedroid stack and HAL header files are different from AOSP version, I try to add one flag to resolve this interface conflict for flame, since we took most of code from CAF. Auto patching these HAL files is not a good idea like Bug 980899 did before.
Another way to fix this is to get AOSP bluedroid stack, and change bluetooth HAL header files to AOSP version under libhardware on CAF b2g branch, but I'm not sure this is feasible to do that.
And since HAL changes can be unexpected such as bug 1000457, i'm afraid we need to handle it from gecko?

Comment 11

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

Comment on attachment 8419221 [details] [diff] [review]
Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 1)

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

Hi

I don't have a flame so i cannot test the change. There doesn't seem to be a flag in the BT headers that tells us whether we're using CAF. If that exists, I'd prefer it over defining the BT variant explicitly.

::: configure.in
@@ +251,4 @@
>              MOZ_B2G_BT=1
>              MOZ_B2G_BT_BLUEDROID=1
>          fi
> +        if test -n "${BOARD_HAVE_BLUEDROID_CAF}"; then

Shouldn't this be called BOARD_HAVE_BLUETOOTH_BLUEDROID_CAF?

@@ +7466,4 @@
>  AC_SUBST(MOZ_B2G_BT)
>  AC_SUBST(MOZ_B2G_BT_BLUEZ)
>  AC_SUBST(MOZ_B2G_BT_BLUEDROID)
> +AC_SUBST(MOZ_B2G_BT_BLUEDROID_VARIANT)

Maybe you could drop MOZ_B2G_BT_BLUEDROID_VARIANT and store the variant in MOZ_B2G_BT_BLUEDROID, say '2' means 'CAF'?

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +495,5 @@
>  
>  static void
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +PinRequestCallback(bt_bdaddr_t* aRemoteBdAddress,
> +                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass, uint8_t secure)

I suggest to only put the final argument into ifdefs.

Comment 12

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Comment on attachment 8419221 [details] [diff] [review]
> Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko)
> (Patch 1)
> 
> Review of attachment 8419221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi
> 
> I don't have a flame so i cannot test the change. There doesn't seem to be a
> flag in the BT headers that tells us whether we're using CAF. If that
> exists, I'd prefer it over defining the BT variant explicitly.
It does not exist so I can only add it from BoardConfig.mk
> 
> ::: configure.in
> @@ +251,4 @@
> >              MOZ_B2G_BT=1
> >              MOZ_B2G_BT_BLUEDROID=1
> >          fi
> > +        if test -n "${BOARD_HAVE_BLUEDROID_CAF}"; then
> 
> Shouldn't this be called BOARD_HAVE_BLUETOOTH_BLUEDROID_CAF?
It should, but I feel the name is too long.
> 
> @@ +7466,4 @@
> >  AC_SUBST(MOZ_B2G_BT)
> >  AC_SUBST(MOZ_B2G_BT_BLUEZ)
> >  AC_SUBST(MOZ_B2G_BT_BLUEDROID)
> > +AC_SUBST(MOZ_B2G_BT_BLUEDROID_VARIANT)
> 
> Maybe you could drop MOZ_B2G_BT_BLUEDROID_VARIANT and store the variant in
> MOZ_B2G_BT_BLUEDROID, say '2' means 'CAF'?
That shall be fine, can reduce extra flag.
> 
> ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +495,5 @@
> >  
> >  static void
> > +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> > +PinRequestCallback(bt_bdaddr_t* aRemoteBdAddress,
> > +                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass, uint8_t secure)
> 
> I suggest to only put the final argument into ifdefs.
You mean like this?
PinRequestCallback(bt_bdaddr_t* aRemoteBdAddress,
-                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass)
+                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass, 
+#ifdef MOZ_B2G_BT_BLUEDROID_CAF
+                   uint8_t secure
+#endif
+)

Comment 13

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

Attachment: Bug 1004896 - Add bluedroid library bluetooth.default (BoardConfig) (Patch 3)

Comment 14

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

Hi

> > I suggest to only put the final argument into ifdefs.
> You mean like this?
> PinRequestCallback(bt_bdaddr_t* aRemoteBdAddress,
> -                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass)
> +                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass, 
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +                   uint8_t secure
> +#endif
> +)

Yes. Looks a bit awkward, but in my experience makes later maintenance less error-prone.

Comment 15

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

Attachment: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 1)

Comment 16

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

Comment on attachment 8419313 [details] [diff] [review]
Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 1)

 static void
 PinRequestCallback(bt_bdaddr_t* aRemoteBdAddress,
-                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass)
+                   bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass
+#ifdef MOZ_B2G_BT_BLUEDROID_CAF
+                   , uint8_t secure
+#endif
+                  )

I need to move ',' to the next line :(

Comment 17

[Michael Wu [:mwu]]

I would prefer to use bluedroid on the Flame when there is

Comment 18

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

(In reply to Michael Wu [:mwu] from comment #17)
> I would prefer to use bluedroid on the Flame when there is
when there is?

Comment 19

[Michael Wu [:mwu]]

(accidentally submitted the previous comment)

I would prefer to use bluedroid on the Flame when running on a KK base image. Since 1.3/JB is shipping with bluez, it would be more useful to leave a JB flame manifest with bluez.

Comment 20

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

(In reply to Michael Wu [:mwu] from comment #19)
> (accidentally submitted the previous comment)
> 
> I would prefer to use bluedroid on the Flame when running on a KK base
> image. Since 1.3/JB is shipping with bluez, it would be more useful to leave
> a JB flame manifest with bluez.
I got it, let's do it on flame-kk. We have seen there is a requirement to add flag to check whether the current bluedroid stack is from CAF or not. Such as bug 1000457, 1003591, and even to avoid auto-patch in Bug 980899. We got some problems with HAL compatibility from CAF. I wonder if you can help to review this patch (Bug 1004896 - Expose bluedroid stack variants (gonk-misc) (Patch 4))? Given that the fact partners may use bluedroid from CAF instead of AOSP.

Comment 21

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #20)
> (In reply to Michael Wu [:mwu] from comment #19)
> > (accidentally submitted the previous comment)
> > 
> > I would prefer to use bluedroid on the Flame when running on a KK base
> > image. Since 1.3/JB is shipping with bluez, it would be more useful to leave
> > a JB flame manifest with bluez.
> I got it, let's do it on flame-kk. We have seen there is a requirement to
> add flag to check whether the current bluedroid stack is from CAF or not.
I mean adding CAF bluedroid flag is for v1.4.

Comment 22

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8419313 [details] [diff] [review]
Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 1)

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

I'm ok with the change, not familiar with build flag related implementation, though. :)

Comment 23

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

We don't need this change.
See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1003591#c30

Comment 25

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

Summary:
flame-kk changed to bluedroid, and CAF has proprietary changes which really need CAF bluetooth flag.
It seems we can all agree this is necessary now. We need to deal with these compiling errors.

Comment 26

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

Attachment: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 1)

Comment 27

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

Rebase for the flame-kk build.

Comment 28

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

Attachment: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL, r=echou