Closed Bug 1004896 Opened 6 years ago Closed 6 years ago

Add CAF bluedroid flag

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED INCOMPLETE
2.0 S1 (9may)

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Whiteboard: [ETA:5/8][p=2])

Attachments

(4 files, 7 obsolete files)

52 bytes, text/x-github-pull-request
Details | Review
49 bytes, text/x-github-pull-request
Details | Review
51 bytes, text/x-github-pull-request
Details | Review
2.55 KB, patch
Details | Diff | Splinter Review
Enable bluedroid as default bluetooth stack
Whiteboard: [ETA:5/8] → [ETA:5/8][p=2]
added target milestone, feel free to update it, thanks
Target Milestone: --- → 2.0 S1 (9may)
Attachment #8419198 - Attachment description: Bug 1004896 - Add bluedroid library bluetooth.default → Bug 1004896 - Add bluedroid library bluetooth.default (Part 1)
Attachment #8419199 - Attachment description: Bug 1004896 - Use bluedroid as the default bluetooth stack → Bug 1004896 - Use bluedroid as the default bluetooth stack (Part 2)
Attachment #8419201 - Attachment description: Bug 1004896 - AddFlagForNonAOSPbluedroid.patch → Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 3)
Attachment #8419198 - Attachment description: Bug 1004896 - Add bluedroid library bluetooth.default (Part 1) → Bug 1004896 - Add bluedroid library bluetooth.default (Patch 1)
Attachment #8419199 - Attachment description: Bug 1004896 - Use bluedroid as the default bluetooth stack (Part 2) → Bug 1004896 - Use bluedroid as the default bluetooth stack (Patch 2)
Attachment #8419206 - Attachment description: Bug 1004896 - Expose bluedroid stack variants → Bug 1004896 - Expose bluedroid stack variants (Patch 4)
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.
Attachment #8419198 - Attachment is obsolete: true
Attachment #8419201 - Attachment is obsolete: true
Attachment #8419201 - Flags: feedback?(echou)
Attachment #8419199 - Attachment description: Bug 1004896 - Use bluedroid as the default bluetooth stack (Patch 2) → Bug 1004896 - Use bluedroid as the default bluetooth stack, change manifest(Patch 4)
Attachment #8419221 - Attachment description: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 3) → Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 1)
Attachment #8419223 - Attachment description: Bug 1004896 - Add bluedroid library bluetooth.default (Patch 1) → Bug 1004896 - Add bluedroid library bluetooth.default (Patch 3)
Attachment #8419221 - Attachment description: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 1) → Bug 1004896 - Add vendor specific flag for customized bluetooth HAL-gecko (Patch 1)
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?
Attachment #8419223 - Attachment description: Bug 1004896 - Add bluedroid library bluetooth.default (Patch 3) → Bug 1004896 - Add bluedroid library bluetooth.default (Patch 3) (BoardConfig)
Attachment #8419220 - Attachment description: Bug 1004896 - Expose bluedroid stack variants (Patch 4) → Bug 1004896 - Expose bluedroid stack variants (gonk-misc) (Patch 4)
Attachment #8419223 - Attachment description: Bug 1004896 - Add bluedroid library bluetooth.default (Patch 3) (BoardConfig) → Bug 1004896 - Add bluedroid library bluetooth.default (BoardConfig) (Patch 3)
Attachment #8419199 - Attachment description: Bug 1004896 - Use bluedroid as the default bluetooth stack, change manifest(Patch 4) → Bug 1004896 - Use bluedroid as the default bluetooth stack (b2g-manifest)(Patch 4)
Attachment #8419221 - Attachment description: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL-gecko (Patch 1) → Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Gecko) (Patch 1)
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.
Attachment #8419221 - Flags: feedback?(tzimmermann) → feedback+
(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
+)
Attachment #8419223 - Attachment is obsolete: true
Attachment #8419223 - Flags: review?(mwu)
Attachment #8419221 - Attachment is obsolete: true
Attachment #8419221 - Flags: review?(echou)
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 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 :(
Attachment #8419313 - Flags: feedback?(tzimmermann)
Attachment #8419313 - Flags: feedback?(tzimmermann) → feedback+
Summary: [Flame]Enable bluedroid as default bluetooth stack → [Flame]Enable bluedroid as default bluetooth stack and add CAF bluedroid flag
I would prefer to use bluedroid on the Flame when there is
(In reply to Michael Wu [:mwu] from comment #17)
> I would prefer to use bluedroid on the Flame when there is
when there is?
(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.
Flags: needinfo?(mwu)
Summary: [Flame]Enable bluedroid as default bluetooth stack and add CAF bluedroid flag → Add CAF bluedroid flag
(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.
Flags: needinfo?(mwu)
(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 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. :)
Attachment #8419313 - Flags: review?(echou) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
Duplicate of this bug: 1048788
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.
Attachment #8473596 - Attachment description: Bug 1004896 - Add vendor specific flag for customized bluetooth HAL → Bug 1004896 - Add vendor specific flag for customized bluetooth HAL (Patch 1)
Rebase for the flame-kk build.
You need to log in before you can comment on or make changes to this bug.