Closed Bug 951565 Opened 10 years ago Closed 10 years ago

[b2g-bluetooth][HFP] Override bluedroid config file with B2G supported features

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: ben.tian, Assigned: tzimmermann)

References

Details

Attachments

(2 files, 4 obsolete files)

Per bug 951530, we have to modify manifest to fetch b2g bluedroid branch instead of codeaurora one.
:mwu, can you help review my base-jb.xml change for b2g bluedroid branch adoption?
Attachment #8349274 - Flags: review?(mwu)
Attachment #8349274 - Attachment mime type: text/plain → text/html
This looks fine, but I'm a bit concerned about how bug 951530 was implemented. I think that stuff should actually be defined in a header file or build file.
I've revised bug 951530 patch to modify only 2 definitions. Do you have concern modifying definition in btif_hf.c?
https://github.com/mozilla-b2g/platform_external_bluetooth_bluedroid/pull/2/files
Flags: needinfo?(mwu)
Nomming for 1.3.  Important.
blocking-b2g: --- → 1.3?
triage: 1.3+ as this will block PTS cert.
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
(In reply to Ben Tian [:btian] from comment #3)
> I've revised bug 951530 patch to modify only 2 definitions. Do you have
> concern modifying definition in btif_hf.c?
> https://github.com/mozilla-b2g/platform_external_bluetooth_bluedroid/pull/2/
> files

The code provides ways for you to modify those definitions without directly modifying the source files. See https://android.googlesource.com/device/generic/common/+/android-4.3.1_r1/bluetooth/bdroid_buildcfg.h . I think we should create our own override header and include it by default in external/bluetooth/bluedroid.
Flags: needinfo?(mwu)
> The code provides ways for you to modify those definitions without directly
> modifying the source files. See
> https://android.googlesource.com/device/generic/common/+/android-4.3.1_r1/
> bluetooth/bdroid_buildcfg.h . I think we should create our own override
> header and include it by default in external/bluetooth/bluedroid.
You are right. I didn't notice the external config file. I'll modify bdroid_buildcfg.h instead and provide another patch. Thanks for the advice.
Summary: [b2g-bluetooth] Replace codeaurora bluedroid branch with b2g one. → [b2g-bluetooth][HFP] Override bluedroid config file with B2G supported features
:mwu, per our last discussion we choose to add a common bluedroid header beside the device one. Still I have 2 questions:
1) Which folder do you suggest to put the common bluedroid header in? My first idea is in gecko/dom/bluetooth but wonder if there's more suitable folder.
2) The method still requires Android.mk change in bluedroid folder. To keep bluedroid folder clean, what do you think if we let all JB device bluedroid headers include the common one?
Flags: needinfo?(mwu)
(In reply to Ben Tian [:btian] from comment #8)
> :mwu, per our last discussion we choose to add a common bluedroid header
> beside the device one. Still I have 2 questions:
> 1) Which folder do you suggest to put the common bluedroid header in? My
> first idea is in gecko/dom/bluetooth but wonder if there's more suitable
> folder.

Things typically go into gonk-misc, but gecko/dom/bluetooth might actually be better. The only tricky part is that you have to make sure you point to the right gecko directory, as the correct gecko to use may be different than (REPO_ROOT)/gecko .

> 2) The method still requires Android.mk change in bluedroid folder. To keep
> bluedroid folder clean, what do you think if we let all JB device bluedroid
> headers include the common one?

I think the Android.mk change will end up easier and more foolproof for downstream users to take advantage of. On the other the hand, I do agree that having device bluedroid headers explicitly include the header is cleaner. We already patch many repos though. If devices can't pass certification without including this header, I'm also ok with letting device bluedroid headers include this.
Flags: needinfo?(mwu)
Depends on: 957949
:mwu,

After discussion we decide to put common header under gecko bluetooth folder, and let device header include it by adding gecko bluetooth folder into $(BOARD_BLUETOOTH_BDROID_BUILDCFG_INCLUDE_DIR).

This patch modifies device header and bug 957949 creates common header under gecko. Please let us know for any concern. Thanks.
Attachment #8349274 - Attachment is obsolete: true
Attachment #8349274 - Flags: review?(mwu)
Attachment #8357642 - Attachment mime type: text/plain → text/html
Comment on attachment 8357642 [details]
link to https://github.com/mozilla-b2g/device-mako/pull/15

m1, are you ok with this approach to adding B2G specific bluedroid customizations? This requires device bluedroid configs to explicitly include our b2g specific configuration.
Attachment #8357642 - Flags: feedback?(mvines)
I'd prefer b2g_bdroid_buildcfg.h to be licensed as Apache2 to be aligned with the rest of bluedroid.

Should we throw this into gonk-misc/ too?  Makes it less "hidden" than nestled deep within the gecko source, less likely for somebody to miss it as they port.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #12)
> I'd prefer b2g_bdroid_buildcfg.h to be licensed as Apache2 to be aligned
> with the rest of bluedroid.
OK. I'll revise it.

> Should we throw this into gonk-misc/ too?  Makes it less "hidden" than
> nestled deep within the gecko source, less likely for somebody to miss it as
> they port.
The reason we put it under gecko is to make code changes atomic. Since gonk-misc/ is kept in another repository, we want to ensure each 'gecko change + bluedroid config' is applied simultaneously to avoid temporary errors. Also this saves additional gonk-misc/ reviewing for each bluedroid config modification.

Let me know for any further concern. Thanks.
:m1, please provide your feedback or let me know for any concern.
Flags: needinfo?(mvines)
Comment on attachment 8357642 [details]
link to https://github.com/mozilla-b2g/device-mako/pull/15

Look fine to me with the A2 license change
Attachment #8357642 - Flags: feedback?(mvines) → feedback+
Flags: needinfo?(mvines)
PM triaged this bug and it be a 1.3 blocker.
Comment on attachment 8357642 [details]
link to https://github.com/mozilla-b2g/device-mako/pull/15

:mwu, m1 gives f+ in comment 15. Please help review my patch, thanks.
Attachment #8357642 - Flags: review?(mwu)
Hi Michael,

Could you review this patch? Thanks.
Flags: needinfo?(mwu)
Attachment #8357642 - Flags: review?(mwu) → review+
Flags: needinfo?(mwu)
This patch can be landed only when Bug 951565 (gecko patch for new header file) is landed to each branch. And ask 1.3? on Bug 951565 already.
https://github.com/mozilla-b2g/device-mako/commit/59877c64bb3665441547f9e7a9a990394393bffc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This is POVB, right? (Nothing to be uplifted?)
Depends on: 965173
I've reverted this to hopefully un-bust Nexus 4 builds.

b2g-4.3_r2.1: 55a5f7def3310ba245102a97270daf1c052c058d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 1.3 C2/1.4 S2(17jan) → ---
My guess here is that GECKO_PATH isn't defined, so we just end up including the wrong directory. Make sure you don't have a GECKO_PATH setup locally when you test.
Note that this bug should not block shipped device on V1.3 because these device used V1.3 + BlueZ not Bluedroid.
I'll take this bug to get it finished for 1.3.
Attached file Github pull request for build scripts (obsolete) —
This fixes the Nexus 4 build for me when GECKO_PATH is not defined.
Attachment #8368488 - Flags: review?(mwu)
Assignee: btian → tzimmermann
That doesn't really work because our partners do not use our build wrapper scripts. Things are generally not allowed to rely on behavior provided by the build scripts or anything else provided by the B2G repo.
Ok. What other options do we have? Define this in the mako's makefile?
gonk-misc/Android.mk has a similar problem, but it simply defaults to gecko when GECKO_PATH isn't defined.
Attached file Github pull request for device_mako (obsolete) —
This works for me.
Attachment #8357642 - Attachment is obsolete: true
Attachment #8368488 - Attachment is obsolete: true
Attachment #8368488 - Flags: review?(mwu)
Attachment #8368513 - Flags: review?(mwu)
Attachment #8368517 - Flags: review?(mwu)
Attachment #8368517 - Flags: review?(mwu) → review+
Attachment #8368513 - Flags: review?(mwu)
Attachment #8368513 - Attachment is obsolete: true
Please merge, if this is ok. Thanks.
Keywords: checkin-needed
Here goes! :)

b2g-4.3_r2.1: 78d17f0c117f0c66dd55ee8d5c5dde8ccc93ecba
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Was this supposed to affect v1.2? Because it apparently did.
https://tbpl.mozilla.org/php/getParsedLog.php?id=33911848&tree=Mozilla-B2g26-v1.2
Status: RESOLVED → REOPENED
Flags: needinfo?(tzimmermann)
Resolution: FIXED → ---
No it wasn't supposed to break 1.2.
Flags: needinfo?(tzimmermann)
Can we simply use the latest working revision of device-mako on v1.2? v1.2 does not even contain Bluedroid code.
Attachment #8369357 - Flags: review?(mwu)
Attachment #8369357 - Flags: review?(mwu) → review+
Thank you.

@sheriffs: attachment 8369357 [details] needs check-in
Keywords: checkin-needed
https://github.com/mozilla-b2g/b2g-manifest/commit/8a083086a5eff34d61d425dee6232260d63ab89a
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Working again, thanks :)
You need to log in before you can comment on or make changes to this bug.