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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P1)

Description

[Michael Vines [:m1] [:evilmachines]]

+++ 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.

Comment 1

[Stephany Wilkes]

Triage is blocking because this is part of the CAF 2.2 meta bug.

Comment 2

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

Michael,

meanwhile you can set the property 'ro.moz.bluetooth.backend' to 'bluetoothd' to enable the daemon.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 4

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

Attachment: Github pull request

This will activate bluetoothd for all our images. Dogfooders don't get this patch and will continue to use Bluedroid.

Comment 5

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 6

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

(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 7

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

Comment on attachment 8569115 [details] [review]
Github pull request

Redirect review to mwu

Comment 8

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

(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.

Comment 9

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 10

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

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.

Comment 11

[Michael Vines [:m1] [:evilmachines]]

(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).

Comment 12

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 13

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

This might be your lucky day. https://bugzilla.mozilla.org/show_bug.cgi?id=1130288#c66

Comment 14

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 15

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

(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.

Comment 16

[Michael Vines [:m1] [:evilmachines]]

(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.

Comment 17

[Michael Vines [:m1] [:evilmachines]]

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

Comment 18

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

You can have both and switch at runtime. That's what attachment 8569115 [details] [review] does.

Comment 19

[Michael Vines [:m1] [:evilmachines]]

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?

Comment 20

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

There's only *one* BT stack running at a time. We still need the old code until testing is completed.

Comment 21

[Michael Vines [:m1] [:evilmachines]]

(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 22

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 23

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

Michael,
Could you help to review this patch? Could you give some advices?

Comment 24

[Michael Wu [: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.

Comment 25

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

Attachment: [01] Bug 1134821: Detect default Bluetooth backend

Comment 26

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

(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.

Comment 27

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

https://developer.mozilla.org/en-US/Firefox_OS/Phone_guide/Flame/Updating_your_Flame#Up-to-date_%28use_these_unless_you_have_a_good_reason_not_to%29

v180d_nightly is released on MDN now.

Comment 28

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

Thanks Shawn!

https://hg.mozilla.org/integration/b2g-inbound/rev/c854fe96a8a1

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=c854fe96a8a1

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c854fe96a8a1

Comment 30

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

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/174bd1b729f5

Comment 32

[Pi Wei Cheng [:piwei] (inactive)]

Not sure what this bug is doing so don't know how to verify it.