Revert "Switch Bluetooth to Bluedroid until FOTA update is ready"

RESOLVED FIXED in Firefox 39

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: m1, Assigned: tzimmermann)

Tracking

({verifyme})

unspecified
2.2 S8 (20mar)
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1132388 +++

Bug 1132388 should be reverted on v2.2 as soon as possible, it moves us backwards.
blocking-b2g: --- → 2.2?
Triage is blocking because this is part of the CAF 2.2 meta bug.
blocking-b2g: 2.2? → 2.2+
Michael,

meanwhile you can set the property 'ro.moz.bluetooth.backend' to 'bluetoothd' to enable the daemon.
Would you rather that we define the backend property in our Gonk regardless of this bug rather than relying on the Gecko default?  I'm leary of doing that as its then another deviation at the Gonk level between our build.  In the meantime we've just backed out the bug here.
Whiteboard: [CR 798549]
Whiteboard: [CR 798549] → [caf priority: p2][CR 798549]
Posted file Github pull request (obsolete) —
This will activate bluetoothd for all our images. Dogfooders don't get this patch and will continue to use Bluedroid.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Attachment #8569115 - Flags: review?(shuang)
Comment on attachment 8569115 [details] [review]
Github pull request

If we make the change here then all devices need to do the same.  Are you sure this is what you want?  Reverting the change in Gecko would be much less error prone from a device integration perspective.
Attachment #8569115 - Flags: feedback-
(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> Comment on attachment 8569115 [details] [review]
> Github pull request
> 
> If we make the change here then all devices need to do the same.  Are you
> sure this is what you want?  Reverting the change in Gecko would be much
> less error prone from a device integration perspective.

But reverting the patch can create tons of problems if users only update gecko/gaia from v2.0 to v2.2.
Comment on attachment 8569115 [details] [review]
Github pull request

Redirect review to mwu
Attachment #8569115 - Flags: review?(mwu)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> Comment on attachment 8569115 [details] [review]
> Github pull request
> 
> If we make the change here then all devices need to do the same.  Are you
> sure this is what you want?  Reverting the change in Gecko would be much
> less error prone from a device integration perspective.

I think I don't understand your concerns. What do you mean by 'all devices need to do the same?' If I'm not mistaken, the patch will affect all devices that support bluetoothd, (i.e., JB and later). Older devices run BlueZ.

I'm trying to solve the issue, so that we get most users to bluetoothd, without breaking dogfooders' devices. Our dogfooders won't get the required updates for init.bluetooth.rc. So no matter what we do, they'll have to stay on Gecko-based Bluedroid in the near future; or lose Bluetooth functionality.

But with the current setup, everyone uses Gecko's implementation, so we don't get much testing for bluetoothd. The patch is an attempt to enable bluetoothd for some users without breaking dogfooders' Bluetooth.

Do that make sense? I'm open for all ideas, but not running bluetoothd won't help anyone.
Flags: needinfo?(mvines)
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> 
> But reverting the patch can create tons of problems if users only update
> gecko/gaia from v2.0 to v2.2.

They can flash the full image or have a complete OTA update, how hard is that?

But does the in-Gecko Bluedroid integration even work with L, or is it <= KK only.
I've been told that we'll need a new base image to distribute the init.*.rc changes. And that won't happen for legal reasons or whatever.

In-Gecko Bluedroid is still available on L.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> I think I don't understand your concerns. What do you mean by 'all devices
> need to do the same?'

https://github.com/mozilla-b2g/platform_build/ isn't used by real devices, it's just reference code.  So this becomes one more thing that needs to be done while porting b2g to a new device.

Easy enough, we can to that here.  But the real problem is dogfooders not dogfooding bluetoothd, that this patch condones.

> I'm trying to solve the issue, so that we get most users to bluetoothd,
> without breaking dogfooders' devices. Our dogfooders won't get the required
> updates for init.bluetoothd.rc
...
> So no matter what we do, they'll have to stay
> on Gecko-based Bluedroid in the near future; or lose Bluetooth functionality.

Ask them to reflash their boot.img.  It's a one time thing, and then your dogfooders can help test bluetoothd as well (although bluetoothd crashes will not be uploaded to Mozilla like Gecko crashes, which isn't great but oh well).
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> I've been told that we'll need a new base image to distribute the init.*.rc
> changes. And that won't happen for legal reasons or whatever.

You could write a script that repacks the user's boot image with the .rc file updates easily so they can do it themselves.
Flags: needinfo?(mvines)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
>
> In-Gecko Bluedroid is still available on L.

This will remain a supported configuration for v2.2?  Using in-gecko bluedroid is starting to look tempting for lower memory configurations since I see bluetoothd taking 2.5MB of USS.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #14)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> >
> > In-Gecko Bluedroid is still available on L.
> 
> This will remain a supported configuration for v2.2?  Using in-gecko
> bluedroid is starting to look tempting for lower memory configurations since
> I see bluetoothd taking 2.5MB of USS.

It will be the 'fall-back' for v2.2; use it at your own risk, we're moving to bluetoothd. The memory is almost entirely consumed by Bluedroid, bluetoothd doesn't allocate much memory besides a few bytes here and there.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
>
> It will be the 'fall-back' for v2.2; use it at your own risk, we're moving
> to bluetoothd. The memory is almost entirely consumed by Bluedroid,
> bluetoothd doesn't allocate much memory besides a few bytes here and there.

I see no increase in the main b2g process memory when the 'fall-back' is active, so it looks like bluedroid is still getting loaded in b2g even if bluetoothd is in use.
Shouldn't MOZ_B2G_BT_BLUEDROID be disabled when MOZ_B2G_BT_DAEMON is enabled?  At least for non-dev builds that only use bluedroidd and no 'fall-back' is needed?   I tried this quickly and Gecko fails to build in some BT files.

Ref: https://github.com/mozilla/gecko-dev/blob/1f351bd61/configure.in#L301-L304
Flags: needinfo?(tzimmermann)
You can have both and switch at runtime. That's what attachment 8569115 [details] [review] does.
Flags: needinfo?(tzimmermann)
Sure, but we would never want that in a commercial build and this appears to be costing about 3MB RAM for the duplicated bluedroid stacks.  Looks like we need a new Gecko configure flag to disable the in-gecko bluedroid stack for non-dev builds.  Do you agree?
There's only *one* BT stack running at a time. We still need the old code until testing is completed.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> There's only *one* BT stack running at a time. 

Right.  At first I wasn't seeing the expected decrease in b2g memory usage when BT was turned on/off using the in-gecko implementation, but I just wasn't looking carefully enough.  I see it behaving as I would expect now, so *phew*.
Comment on attachment 8569115 [details] [review]
Github pull request

I've made a similar change on our end to always select bluedroidd regardless of the in-gecko default, to insulate us from these kind of unannounced changes, so I'm firmly meh on this patch now.
Attachment #8569115 - Flags: feedback-
No longer blocks: CAF-v2.2-metabug
Whiteboard: [caf priority: p2][CR 798549]
Michael,
Could you help to review this patch? Could you give some advices?
Flags: needinfo?(mwu)
I've discussed the PR with tzimmermann on IRC. I think we can detect init.bluetooth.rc and /system/bin/bluetoothd at runtime to determine which backend to use. ro.moz.bluetooth.backend can be used to override this decision, but gecko should tend towards the right thing by default.
Flags: needinfo?(mwu)
Attachment #8569115 - Flags: review?(mwu)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> Created attachment 8575295 [details] [diff] [review]
> [01] Bug 1134821: Detect default Bluetooth backend

With this patch, we'll use bluetoothd when it's available, but dogfooders should stay on in-Gecko Bluedroid until they have the new device image.
https://hg.mozilla.org/mozilla-central/rev/c854fe96a8a1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
Comment on attachment 8575295 [details] [diff] [review]
[01] Bug 1134821: Detect default Bluetooth backend

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 1132388

User impact if declined: 

No bluetoothd in v2.2. We had to disable it temporarily in bug 1132388 until we found a solution to not break dogfooders' devices. This should work now.

Testing completed: 

I tested this locally and it has already been committed to m-c.

Risk to taking this patch (and alternatives if risky): 

Small

String or UUID changes made by this patch:

None
Attachment #8575295 - Flags: approval-mozilla-b2g37?
Keywords: verifyme
Attachment #8575295 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Not sure what this bug is doing so don't know how to verify it.
QA Whiteboard: QAExclude
Flags: needinfo?(jmercado)
Flags: needinfo?(jmercado)
You need to log in before you can comment on or make changes to this bug.