Closed Bug 1054218 Opened 10 years ago Closed 10 years ago

[flame-kk]Build break in BluetoothInterface.cpp:334:3: sorry, unimplemented: non-trivial designated initializers not supported

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: shawnjohnjr, Unassigned)

References

Details

Attachments

(11 obsolete files) 

We have seen build break on flame-kk, because it uses CAF repos, and the HAL is different from AOSP Bluetooth, this is why we can build pass on Nexus4 but fail on flame-kk. When migrating to flame KK, we switch flame to bluedroid stack. And flame-kk has their own HAL interface.

BluetoothInterface.o
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp: In function 'nsresult mozilla::dom::bluetooth::Convert(bt_property_type_t, mozilla::dom::bluetooth::BluetoothPropertyType&)':
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:334:3: sorry, unimplemented: non-trivial designated initializers not supported
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Summary: Build break in BluetoothInterface.cpp:334:3: sorry, unimplemented: non-trivial designated initializers not supported → [flame-kk]Build break in BluetoothInterface.cpp:334:3: sorry, unimplemented: non-trivial designated initializers not supported
Attachment #8473603 - Attachment is obsolete: true
Attachment #8473603 - Flags: review?(echou)
Attachment #8473603 - Flags: feedback?(tzimmermann)
Attachment #8473615 - Flags: review?(azakai) → review?(kli)
Attachment #8473612 - Attachment description: Bug 1054218 - Expose bluedroid stack variants → Bug 1054218 - Expose bluedroid stack variants (gonk-misc)
Attachment #8473615 - Attachment description: Bug 1054218 - Expose bluedroid stack CAF variant → Bug 1054218 - Expose bluedroid stack CAF variant (device-flame-kk)
Attachment #8473607 - Flags: feedback?(tzimmermann) → feedback+
Attachment #8473607 - Attachment is obsolete: true
Attachment #8473607 - Flags: review?(echou)
Comment on attachment 8473615 [details] [review]
Bug 1054218 - Expose bluedroid stack CAF variant (device-flame-kk)

Looks good.
Attachment #8473615 - Flags: review?(kli) → review+
Comment on attachment 8473612 [details] [review]
Bug 1054218 - Expose bluedroid stack variants (gonk-misc)

Looks good, with an assumption of BOARD_HAVE_BLUETOOTH_BLUEDROID_CAF is also good for configure.in.
Attachment #8473612 - Flags: review?(kli) → review+
Comment on attachment 8473607 [details] [diff] [review]
Bug 1054218 - Add vendor specific flag for customized bluetooth HAL

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

::: configure.in
@@ +252,5 @@
>              MOZ_B2G_BT=1
>              MOZ_B2G_BT_BLUEDROID=1
>          fi
> +        if test -n "${BOARD_HAVE_BLUETOOTH_BLUEDROID_CAF}"; then
> +          MOZ_B2G_BT_BLUEDROID="CAF"

super-nit: 4 whitespace indentation, please.
Attachment #8473607 - Flags: review?(echou) → review+
Comment on attachment 8473676 [details] [diff] [review]
[01] Bug 1054218: Add support for CAF Bluedroid interface

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

https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/tree/include/hardware/bluetooth.h?id=AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.062
In KK, CAF adds their internal flag (Q_BLUETOOTH).

See https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/tree/include/hardware/bluetooth.h?id=AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.062#n391
See https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/tree/include/hardware/bluetooth.h?id=AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.062#n305

Before we introduced backend neutral interface, we can build gecko successfully, so I guess Q_BLUETOOTH is not defined. But I haven't tried to build yet.
Have you tried to build on flame-kk?
I built it locally, it seems like Comment 11 is correct.
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp: In member function 'void mozilla::dom::bluetooth::BluetoothInterface::Init(mozilla::dom::bluetooth::BluetoothNotificationHandler*, mozilla::dom::bluetooth::BluetoothResultHandler*)':
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:3072:3: error: 'bt_callbacks_t' has no non-static data member named 'wake_state_changed_cb'
Comment on attachment 8473676 [details] [diff] [review]
[01] Bug 1054218: Add support for CAF Bluedroid interface

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +2900,5 @@
>        &BluetoothNotificationHandler::DiscoveryStateChangedNotification,
>        aState);
>    }
>  
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF

CAF already added build flag Q_BLUETOOTH, so we don't need to add this.

@@ +2912,5 @@
>    static void
>    PinRequest(bt_bdaddr_t* aRemoteBdAddress,
> +             bt_bdname_t* aRemoteBdName, uint32_t aRemoteClass
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +             , uint8_t aSecure

CAF already added build flag Q_BLUETOOTH, so we don't need to add this.

@@ +3058,5 @@
>      .adapter_properties_cb = BluetoothCallback::AdapterProperties,
>      .remote_device_properties_cb = BluetoothCallback::RemoteDeviceProperties,
>      .device_found_cb = BluetoothCallback::DeviceFound,
>      .discovery_state_changed_cb= BluetoothCallback::DiscoveryStateChanged,
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF

Ditto.
Attachment #8473676 - Flags: review?(shuang)
Comment on attachment 8473647 [details] [diff] [review]
Bug 1054218 - Add vendor specific flag for customized bluetooth HAL, r=echou

For configure.in
Attachment #8473647 - Flags: review?(mh+mozilla)
Changes since v1:

  - build for CAF if Q_BLUETOOTH is defined
Attachment #8473676 - Attachment is obsolete: true
Attachment #8473843 - Attachment is obsolete: true
Attachment #8474405 - Flags: review?(shuang)
> Have you tried to build on flame-kk?

I used Flame's JB branch. Stupid me didn't expect yet more differences within CAF's repositories.
Comment on attachment 8474405 [details] [diff] [review]
[01] Bug 1054218: Add support for CAF Bluedroid interface (v2)

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +15,5 @@
>  
> +/* CAF KK has the flag Q_BLUETOOTH set. We have to build for
> + * CAF in this case.
> + */
> +#ifdef Q_BLUETOOTH

Q_BLUETOOTH is used inside bluedroid, I think gecko cannot resolve it. Also, CAF KK undefined Q_BLUETOOTH for b2g to disable their Android new features which changes interface.
I think we can use Android version + MOZ_B2G_BT_BLUEDROID_CAF to resolve this problem if we want to support bluedroid for JB and KK. What do you think?
Attachment #8474405 - Flags: review?(shuang)
Comment on attachment 8473647 [details] [diff] [review]
Bug 1054218 - Add vendor specific flag for customized bluetooth HAL, r=echou

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

::: dom/bluetooth/moz.build
@@ +72,5 @@
>  
>              DEFINES['MOZ_B2G_BT_BLUEDROID'] = True
> +
> +            if CONFIG['MOZ_B2G_BT_BLUEDROID'] == 'CAF':
> +              DEFINES['MOZ_B2G_BT_BLUEDROID_CAF'] = True

how about dom/bluetooth2?
Attachment #8473647 - Flags: review?(mh+mozilla) → review+
> I think we can use Android version + MOZ_B2G_BT_BLUEDROID_CAF to resolve
> this problem if we want to support bluedroid for JB and KK. What do you
> think?

Sure, should work.
Attachment #8474405 - Attachment is obsolete: true
This bug probably unnecessary at this point.

The fix on CAF side is to simply move the BT_PROPERTY_REMOTE_TRUST_VALUE enum after BT_PROPERTY_REMOTE_VERSION_INFO.   I've made this change here already and it'll be making it's way out to the CAF gits soon.
Comment on attachment 8473615 [details] [review]
Bug 1054218 - Expose bluedroid stack CAF variant (device-flame-kk)

In cases like this please work with us directly so that we can fix the CAF headers to be a proper superset of AOSP headers.  They should be in the first place, the fact that it was not in this case is just a silly bug.
Attachment #8473615 - Flags: feedback-
(In reply to Michael Vines [:m1] [:evilmachines] from comment #24)
> Comment on attachment 8473615 [details] [review]
> Bug 1054218 - Expose bluedroid stack CAF variant (device-flame-kk)
> 
> In cases like this please work with us directly so that we can fix the CAF
> headers to be a proper superset of AOSP headers.  They should be in the
> first place, the fact that it was not in this case is just a silly bug.
Great, would you please also fix headers for b2g-jb branch? We need to work on both JB/KK.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25)
> Great, would you please also fix headers for b2g-jb branch? We need to work
> on both JB/KK.

You haven't killed the JB Flame build yet?  Why not?
We got some RIL problems on flame-kk, we don't want to be blocked since we're working on moving bluedroid to separate process. Also, due to bug 1003591, we need to have CAF flag. We thought CAF doesn't want to change the interface.
K, I'm bringing to JB as well.  It'll take a couple days for this to get out to CAF gits, just FYI.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #28)
> K, I'm bringing to JB as well.  It'll take a couple days for this to get out
> to CAF gits, just FYI.
Thank you. Note that for JB not only BT_PROPERTY_REMOTE_TRUST_VALUE, but also non-AOSP callbacks.
For jb you can use the patches here as a work around until your flame-kk build is working.  flame-jb has no future (from our PoV) so there's no point spending time on that build.
Based on Comment 24, bug 1003591 #c39, we will not have vendor specific compile time flags.


https://bugzilla.mozilla.org/show_bug.cgi?id=1003591#c39
Michael Wu [:mwu]  2014-08-19 17:16:33 PDT
CAF flags or any vendor specific compile time flags are an automatic r-. Please assign fields by value as mentioned in comment 35. If you have any future need for this and can't find a good solution, let me know.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #23)
> This bug probably unnecessary at this point.
> 
> The fix on CAF side is to simply move the BT_PROPERTY_REMOTE_TRUST_VALUE
> enum after BT_PROPERTY_REMOTE_VERSION_INFO.   I've made this change here
> already and it'll be making it's way out to the CAF gits soon.

It that really necessary?

We already see different headers for JB, JB on Flatfish, KK, and KK from CAF. This would just be another variant we'd have to support. Not something I'm looking forward to.

What I really want is a preprocessor define that tells my when we build against CAF headers; ideally a version number.
Changes since v2:

  - define Q_BLUETOOTH before including CAF headers

This produces working builds with Nexus 4 and Flame-kk.
Attachment #8475736 - Flags: review?(shuang)
Comment on attachment 8475736 [details] [diff] [review]
[01] Bug 1054218: Add support for CAF Bluedroid interface (v3)

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

If CAF flag can be used, I only have one concern for this patch.

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
>  #define mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
>  
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF

Sigh, I'm not sure we can use MOZ_B2G_BT_BLUEDROID_CAF flag anymore, since mwu strongly disagree introducing any vendor specific flag. If CAF can clean up non-AOSP interface, we don't have this problem anymore.

@@ +8,5 @@
>  #define mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
>  
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +/* Bluetooth headers of CAF KK require Q_BLUETOOTH */
> +#define Q_BLUETOOTH 1

If we have Q_BLUETOOTH enabled in header while compiling, what about external/bluetooth/bluedroid? I mean this will make headers and implementation different. I grep Q_BLUETOOTH under external/bluetooth/bluedroid, several places uses this flag. I wonder this might cause problem?

https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/bluedroid/tree/btif/src/btif_dm.c?h=LNX.LA.2.7.3&id=AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.062#n872

#ifdef Q_BLUETOOTH
    HAL_CBACK(bt_hal_cbacks, pin_request_cb,
                     &bd_addr, &bd_name, cod, secure);
#else
    HAL_CBACK(bt_hal_cbacks, pin_request_cb,
                     &bd_addr, &bd_name, cod);
#endif
Attachment #8475736 - Flags: review?(shuang)
Comment on attachment 8475736 [details] [diff] [review]
[01] Bug 1054218: Add support for CAF Bluedroid interface (v3)

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +3072,5 @@
>      .le_test_mode_cb = BluetoothCallback::LeTestMode
> +#if defined MOZ_B2G_BT_BLUEDROID_CAF && ANDROID_VERSION >= 19
> +    ,
> +    /* CAF KK has its callbacks at the end of the structure */
> +    .wake_state_changed_cb = BluetoothCallback::WakeStateChanged

It looks like even without this callback, CAF KK BT is working correctly.

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +7,5 @@
>  #ifndef mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
>  #define mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
>  
> +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> +/* Bluetooth headers of CAF KK require Q_BLUETOOTH */

CAF KK doesn't need Q_BLUETOOTH (I mean even without wakeup callback, bluetooth on/off can work correctly). So you don't need  #define Q_BLUETOOTH 1
> >  
> >  #ifndef mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
> >  #define mozilla_dom_bluetooth_bluedroid_bluetoothinterface_h__
> >  
> > +#ifdef MOZ_B2G_BT_BLUEDROID_CAF
> 
> Sigh, I'm not sure we can use MOZ_B2G_BT_BLUEDROID_CAF flag anymore, since
> mwu strongly disagree introducing any vendor specific flag. If CAF can clean
> up non-AOSP interface, we don't have this problem anymore.

But how do we know which interface to use?

If we only have a Bluedroid binary driver, we need Gecko to be binary compatible to it. We don't know whether the binary expects AOSP or CAF interfaces. So when we pull from CAF repositories, I assume that the binary driver has been built against CAF and uses CAF interfaces. Otherwise it's regular AOSP.

I don't know how to solve this case without an additional flag.
Shall we define Q_BLUETOOTH when building against CAF headers?
Flags: needinfo?(mvines)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #37)
> Shall we define Q_BLUETOOTH when building against CAF headers?

I don't think you should, until you decide to start using the extra features that are supplied (which would be good for b2g one day). The intent is for Q_BLUETOOTH to wrap the non-AOSP features for a given AOSP branch, others could do the same for their divergent headers.
Flags: needinfo?(mvines)
Thanks for the info. Personally I wouldn't mind supporting additional features of various BT branches. We only need to make sure that we're compatible with everyone else's branches.
Comment on attachment 8475736 [details] [diff] [review]
[01] Bug 1054218: Add support for CAF Bluedroid interface (v3)

Obsolete, since we'll only use AOSP features.
Attachment #8475736 - Attachment is obsolete: true
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #39)
> Thanks for the info. Personally I wouldn't mind supporting additional
> features of various BT branches. We only need to make sure that we're
> compatible with everyone else's branches.

Yep!  If/when we start pulling in non-AOSP BT features (I'm all for that too BTW, gets us closer to what current Android products already have), using a specific flag in Gecko for each distinct feature seems way nicer than something like Q_BLUETOOTH.  We could refactor stuff on the CAF side to support that as needed on our non-release branches.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: