Michael Vines [:m1] [:evilmachines] Reporter
	Description • 9 years ago

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

	Stephany Wilkes
	Comment 1 • 9 years ago

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

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 2 • 9 years ago

Michael,

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 3 • 9 years ago

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.

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 4 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 5 • 9 years ago

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.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 6 • 9 years ago

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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 7 • 9 years ago

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

Redirect review to mwu

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 8 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 9 • 9 years ago

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

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 10 • 9 years ago

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.

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 11 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 12 • 9 years ago

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

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 13 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 14 • 9 years ago

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

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 15 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 16 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 17 • 9 years ago

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

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 18 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 19 • 9 years ago

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?

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 20 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 21 • 9 years ago

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

	Michael Vines [:m1] [:evilmachines] Reporter
	Comment 22 • 9 years ago

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.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 23 • 9 years ago

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

	Michael Wu [:mwu]
	Comment 24 • 9 years ago

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.

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 26 • 9 years ago

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

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 27 • 9 years ago

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.

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 28 • 9 years ago

Thanks Shawn!

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

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 9 years ago

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

	Thomas Zimmermann [:tzimmermann] [:tdz] Assignee
	Comment 30 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 9 years ago

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

	Pi Wei Cheng [:piwei] (inactive)
	Comment 32 • 9 years ago

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