Closed Bug 1146355 Opened 9 years ago Closed 9 years ago

Run Bluetooth APIs v1 and v2 with same backend code

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S10 (17apr)
Tracking Status
firefox40 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

(Depends on 1 open bug)

Details

Attachments

(18 files, 24 obsolete files)

18.54 KB, patch
yrliou
: review+
Details | Diff | Splinter Review
6.46 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
14.09 KB, patch
yrliou
: review+
Details | Diff | Splinter Review
6.61 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
17.76 KB, patch
brsun
: review+
Details | Diff | Splinter Review
2.19 KB, patch
yrliou
: review+
Details | Diff | Splinter Review
4.32 KB, patch
yrliou
: review+
Details | Diff | Splinter Review
4.72 KB, patch
brsun
: review+
Details | Diff | Splinter Review
253.01 KB, patch
Details | Diff | Splinter Review
30.32 KB, patch
Details | Diff | Splinter Review
2.58 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
4.10 KB, patch
Details | Diff | Splinter Review
12.35 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
12.03 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
77.50 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
42.87 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
958.63 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
684 bytes, patch
tzimmermann
: review+
Details | Diff | Splinter Review
The backend code in bluetooth1/ and bluetooth2/ is diverging (GATT, LE, fixes). We should merge both implementations and run either API on top of the result.
Attached patch Activate Bluetooth v2 (obsolete) — Splinter Review
Hello Shawn, Ben, Jocelyn, Bruce,

As recently discussed on IRC by some of us, I'd like to run the Bluetooth APIs v1 and v2 on top of the same backend code.

Both Bluetooth implementations currently have different feature sets (mainly GATT vs LE). V1 currently goes through QA and we have fixes that haven't made it into v2. We'll certainly run into old bugs when we switch to v2. Porting code between v1 and v2 also adds overhead to the development and is not easily possible in some cases.

The patch set intents to solve these problems and will allow a smooth transition from v1 to v2 during the next months. It

  1) moves generic v1 code into subdirectory dom/bluetooth/bluetooth1,
  2) moves generic v2 code into subdirectory dom/bluetooth/bluetooth2,
  3) moves GATT support into the shared Bluedroid backend,
  4) adds support for both versions to Bluedroid backend interfaces,
  5) adds support for both versions to Bluedroid backend managers, and
  6) adds support for both versions to the build scripts.

The actual patches are trivial. I usually only had to add some code from v2 into the backend code of v1. I copied the implementation of v2 and GATT as it is. So if you work on patches for either, you typically only have to rename some filenames in the patch files. The patches will still apply.

The only major difference is in |BluetoothServiceBluedroid|, which differs significantly between v1 and v2. In such cases I put both implementations into the same file and separated them by ifdef.

Once this patch set has been merged, we can merge shared code into single implementations and cleanup most the remaining differences in the backend code. First candidates are BluetoothCommon.{cpp,h}, BluetoothUtils.{cpp,h} and BluetoothInterface.{cpp,h}. When v2 is ready, we'll only have minimum of code left to switch.

I hope you're OK with this plan. Any comments are welcome. (adding ni?)
Flags: needinfo?(shuang)
Flags: needinfo?(joliu)
Flags: needinfo?(btian)
Flags: needinfo?(brsun)
We have general discussion on this topic. Jocelyn will help to provide the consideration and comments.
Flags: needinfo?(shuang)
Hi Thomas,

First of all, thanks for the effort for letting bluetooth1 and bluetooth2 share the same backend. ;)

IMHO, it will save porting efforts when we need to modify the backend.
As for generic code, the workflow looks the same to me for bluetooth1/bluetooth2 except for the path.
(ie. Bluetooth2 still won't be built on try server and can use HAL either by default or by manually switch to bluedroid. And we don't need to modify v1's generic code when modify v2's.)

And I have some considerations about this change, please see my comments below.

1) timeline
Based on APIv2 switch plan, I would recommend to do this change either right before we start running PTS test for v2(4/13) or after our tests are done(that would be around the date that we plan to switch to v2, mid of June).
I suppose you would prefer 4/13?

2) readability
Compare to current code base, I think our codes will be harder to read in those points that we need to use #ifdef.
Some refinement might enhance the readability though I think it's not as high priority as our current tasks.

However, since our development on v2 will be GATT implementation recently, it's probably OK to me since we usually focus on generic code, and v1 doesn't have gatt support in the backend.

3) stability
Since we already done the first round test for APIv2, introduce this change between second and first round test might somehow decrease the stability.
Though I think it's probably OK if we review those copies very carefully.

4) GATT support
I think we should at least have the skeleton for GATT in daemon, so we won't crash or have troubles when using daemon with v2.
The actual support can be happened later.
Then we will still need to use HAL during development before we have GATT support in daemon.

5) ongoing work on v2
There are several ongoing work on v2, since the schedule is already tight, I was hoping if we can save the effort for rebasing on this change?
For example, Bug 1140952 and Bug 1146792.

Thanks,
Jocelyn
Flags: needinfo?(joliu)
Oh cool! What a massive amount of feedback. Thank you!

> (ie. Bluetooth2 still won't be built on try server and can use HAL either by

I nice side effect of sharing the backend code is that parts of v2 will be covered by v1 tests. :)


> 1) timeline
> Based on APIv2 switch plan, I would recommend to do this change either right
> before we start running PTS test for v2(4/13) or after our tests are
> done(that would be around the date that we plan to switch to v2, mid of
> June).
> I suppose you would prefer 4/13?

I'd like to land this ASAP, so I'd prefer 4/13. In the schedule, there is a 'buffer week' (04/06 to 04/10). Can we target this week for landing? This would

 - give us two weeks for the review,
 - maybe let me prepare v1 code base to reduce the amount of ifdefs, and
 - be right before the second test phase.

04/06 is a holiday for me, but if we'd land it on 04/07, we'd have the rest of the week for spotting regressions.


> 2) readability
> Compare to current code base, I think our codes will be harder to read in
> those points that we need to use #ifdef.
> Some refinement might enhance the readability though I think it's not as
> high priority as our current tasks.

I agree about the readability of ifdefs. I've been careful to only keep them in the backend code, so that at least the implementations won't be affected. My plan is to sort out as many of them as possible until v2 is ready in June.

For possible refinements in v1, please see point 1: we'd still have two weeks for this. There are some simple things (type names) that can be changed in v1 to reduce the amount of required ifdefs.


> 3) stability
> Since we already done the first round test for APIv2, introduce this change
> between second and first round test might somehow decrease the stability.
> Though I think it's probably OK if we review those copies very carefully.

This patch set won't go into v2.2, so the current release won't be affected.

I also tried hard to not introduce any changes to the functionality of the code; just moving code around. I hope this will reduce the amount to regressions to a minimum.


> 4) GATT support
> I think we should at least have the skeleton for GATT in daemon, so we won't
> crash or have troubles when using daemon with v2.
> The actual support can be happened later.
> Then we will still need to use HAL during development before we have GATT
> support in daemon.

Nothing will change for GATT. Even with the patch set applied, v2 will still use HAL by default. The code is in BluetoothInterface.cpp, which is currently specific for each implementation. We can keep it like this until we have GATT support in the daemon.

In general, I agree that adding some GATT support to the daemon would be nice. I recently wrote some initial patches, but haven't looks at it since.


> 5) ongoing work on v2
> There are several ongoing work on v2, since the schedule is already tight, I
> was hoping if we can save the effort for rebasing on this change?
> For example, Bug 1140952 and Bug 1146792.

These bugs already have patches. Can you land them before 4/13?

Best regards
Thomas
Hi Thomas,

Please see my comments below.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> Oh cool! What a massive amount of feedback. Thank you!
> 
> > (ie. Bluetooth2 still won't be built on try server and can use HAL either by
> 
> I nice side effect of sharing the backend code is that parts of v2 will be
> covered by v1 tests. :)
> 
> 
> > 1) timeline
> > Based on APIv2 switch plan, I would recommend to do this change either right
> > before we start running PTS test for v2(4/13) or after our tests are
> > done(that would be around the date that we plan to switch to v2, mid of
> > June).
> > I suppose you would prefer 4/13?
> 
> I'd like to land this ASAP, so I'd prefer 4/13. In the schedule, there is a
> 'buffer week' (04/06 to 04/10). Can we target this week for landing? This
> would
> 
>  - give us two weeks for the review,
>  - maybe let me prepare v1 code base to reduce the amount of ifdefs, and
>  - be right before the second test phase.
> 
> 04/06 is a holiday for me, but if we'd land it on 04/07, we'd have the rest
> of the week for spotting regressions.
> 
> 

Just had an offline sync-up with other members, we all agree it should be OK to target on that week.
In the mean time, we will try our best to finish APIv2 bugs which need to be fixed and all ongoing GATT bugs before second round test before 4/7.
If we couldn't make it for GATT bugs since we also have holidays on 4/3 and 4/6, we will rebase them based on this change.

> > 2) readability
> > Compare to current code base, I think our codes will be harder to read in
> > those points that we need to use #ifdef.
> > Some refinement might enhance the readability though I think it's not as
> > high priority as our current tasks.
> 
> I agree about the readability of ifdefs. I've been careful to only keep them
> in the backend code, so that at least the implementations won't be affected.
> My plan is to sort out as many of them as possible until v2 is ready in June.
> 
> For possible refinements in v1, please see point 1: we'd still have two
> weeks for this. There are some simple things (type names) that can be
> changed in v1 to reduce the amount of required ifdefs.
> 
> 

Sounds good to me, thanks for the effort!

> > 3) stability
> > Since we already done the first round test for APIv2, introduce this change
> > between second and first round test might somehow decrease the stability.
> > Though I think it's probably OK if we review those copies very carefully.
> 
> This patch set won't go into v2.2, so the current release won't be affected.
> 
> I also tried hard to not introduce any changes to the functionality of the
> code; just moving code around. I hope this will reduce the amount to
> regressions to a minimum.
> 
> 

Yes, I think it should be OK if we doing this very carefully.

> > 4) GATT support
> > I think we should at least have the skeleton for GATT in daemon, so we won't
> > crash or have troubles when using daemon with v2.
> > The actual support can be happened later.
> > Then we will still need to use HAL during development before we have GATT
> > support in daemon.
> 
> Nothing will change for GATT. Even with the patch set applied, v2 will still
> use HAL by default. The code is in BluetoothInterface.cpp, which is
> currently specific for each implementation. We can keep it like this until
> we have GATT support in the daemon.
> 

Oh, I see.
So the v2 will still run HAL by default without any manual switch, then I have no concerns on this.

> In general, I agree that adding some GATT support to the daemon would be
> nice. I recently wrote some initial patches, but haven't looks at it since.
> 
> 

Sounds good to me. ;)

> > 5) ongoing work on v2
> > There are several ongoing work on v2, since the schedule is already tight, I
> > was hoping if we can save the effort for rebasing on this change?
> > For example, Bug 1140952 and Bug 1146792.
> 
> These bugs already have patches. Can you land them before 4/13?
> 

As I state in 1), we will try our best!
But is it 4/7 or 4/13?

> Best regards
> Thomas

Thanks,
Jocelyn
Flags: needinfo?(joliu)
Fix some typo, sorry.
(In reply to jocelyn [:jocelyn] from comment #21)
> Hi Thomas,
> 
> Please see my comments below.
> 
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> > Oh cool! What a massive amount of feedback. Thank you!
> > 
> > > (ie. Bluetooth2 still won't be built on try server and can use HAL either by
> > 
> > I nice side effect of sharing the backend code is that parts of v2 will be
> > covered by v1 tests. :)
> > 
> > 
> > > 1) timeline
> > > Based on APIv2 switch plan, I would recommend to do this change either right
> > > before we start running PTS test for v2(4/13) or after our tests are
> > > done(that would be around the date that we plan to switch to v2, mid of
> > > June).
> > > I suppose you would prefer 4/13?
> > 
> > I'd like to land this ASAP, so I'd prefer 4/13. In the schedule, there is a
> > 'buffer week' (04/06 to 04/10). Can we target this week for landing? This
> > would
> > 
> >  - give us two weeks for the review,
> >  - maybe let me prepare v1 code base to reduce the amount of ifdefs, and
> >  - be right before the second test phase.
> > 
> > 04/06 is a holiday for me, but if we'd land it on 04/07, we'd have the rest
> > of the week for spotting regressions.
> > 
> > 
> 
> Just had an offline sync-up with other members, we all agree it should be OK
> to target on that week.
> In the mean time, we will try our best to finish APIv2 bugs which need to be
> fixed and all ongoing GATT bugs before second round test before 4/7.
> If we couldn't make it for GATT bugs since we also have holidays on 4/3 and
> 4/6, we will rebase them based on this change.
> 

I meant, APIv2 bugs which need to be fixed before second round test and all ongoing GATT bugs.
> As I state in 1), we will try our best!
> But is it 4/7 or 4/13?

Right, 4/7. Basically before this patch lands.
Depends on: 1146923
Attachment #8581736 - Attachment is obsolete: true
Attachment #8583871 - Flags: review?(joliu)
Attachment #8581738 - Attachment is obsolete: true
Attachment #8583873 - Flags: review?(joliu)
Attachment #8581740 - Attachment is obsolete: true
Attachment #8583875 - Flags: review?(joliu)
Attachment #8581742 - Attachment is obsolete: true
Attachment #8583876 - Flags: review?(joliu)
Attachment #8581743 - Attachment is obsolete: true
Attachment #8583878 - Flags: review?(joliu)
Hey,

let's get this reviewed. Feel free to switch review request as you prefer. There's still a BlueZ patch required, which I'll upload soon.
Flags: needinfo?(btian)
Flags: needinfo?(brsun)
Comment on attachment 8583859 [details] [diff] [review]
[02] Bug 1146355: Prepare build system to support Bluetooth APIs v1 and v2

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

::: dom/base/moz.build
@@ -397,5 @@
>  
>  LOCAL_INCLUDES += [
>      '../battery',
>      '../bluetooth',
> -    '../bluetooth/bluetooth1',

Why does this line exist?
(In reply to Shawn Huang [:shawnjohnjr] from comment #38)
> Comment on attachment 8583859 [details] [diff] [review]
> [02] Bug 1146355: Prepare build system to support Bluetooth APIs v1 and v2
> 
> Review of attachment 8583859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/moz.build
> @@ -397,5 @@
> >  
> >  LOCAL_INCLUDES += [
> >      '../battery',
> >      '../bluetooth',
> > -    '../bluetooth/bluetooth1',
> 
> Why does this line exist?

Oh, sorry. Patch 2 depends on Patch 1, i forgot to check Patch 1.
Comment on attachment 8583863 [details] [diff] [review]
[04] Bug 1146355: Add Bluetooth v2 to Bluetooth build scripts

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

It looks good to me.
Attachment #8583863 - Flags: review?(shuang) → review+
Before we land all these patches, we need to build both w/o flag MOZ_B2G_BT_API_V2, or even remove MOZ_B2G_BT flag.
(In reply to Shawn Huang [:shawnjohnjr] from comment #41)
> Before we land all these patches, we need to build both w/o flag
> MOZ_B2G_BT_API_V2, or even remove MOZ_B2G_BT flag.

True. I usually build without MOZ_B2G_BT_API_V2, then apply attachment 8581748 [details] [diff] [review] and build again; this time with MOZ_B2G_BT_API_V2 set. Typically this shows up some places where the backend code differs between both APIs. I usually also do some basic tests. I guess the patch set also has to be rebased before landing it.
Attached patch Bluedroid diffSplinter Review
A diff between the two Bluedroid backends.
Attached patch BlueZ diffSplinter Review
A diff between the two BlueZ backends
Comment on attachment 8583865 [details] [diff] [review]
[05] Bug 1146355: Update Bluetooth backend interface for bluetooth2

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

LGTM
Attachment #8583865 - Flags: review?(brsun) → review+
Comment on attachment 8583866 [details] [diff] [review]
[06] Bug 1146355: Prepare Bluetooth backend helpers to handle v1 and v2 implementations

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

LGTM
Attachment #8583866 - Flags: review?(brsun) → review+
Thanks Shawn! Thanks Bruce!
Comment on attachment 8583857 [details] [diff] [review]
[01] Bug 1146355: Move Bluetooth legacy interface of into sub-directory bluetooth1/

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

LGTM.
Attachment #8583857 - Flags: review?(joliu) → review+
Comment on attachment 8583860 [details] [diff] [review]
[03] Bug 1146355: Import Bluetooth v2 implementation

LGTM.
Attachment #8583860 - Flags: review?(joliu) → review+
Comment on attachment 8583869 [details] [diff] [review]
[07] Bug 1146355: Added GATT support to Bluedroid backend

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

I suppose these lines are under 'MOZ_B2G_BT_BLUEDROID' check?
Just want to double confirm with you since it's hard to tell from this patch.

r=me if they are under bluedroid check.
Attachment #8583869 - Flags: review?(joliu) → review+
(In reply to jocelyn [:jocelyn] from comment #51)
> Comment on attachment 8583869 [details] [diff] [review]
> [07] Bug 1146355: Added GATT support to Bluedroid backend
> 
> Review of attachment 8583869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I suppose these lines are under 'MOZ_B2G_BT_BLUEDROID' check?
> Just want to double confirm with you since it's hard to tell from this patch.
> 
> r=me if they are under bluedroid check.

Yes, everything under bluedroid/ is protected by this token. And everything under bluez/ is protected by MOZ_B2G_BT_BLUEZ. It's handled in the moz.build file. So without Bluedroid, these files won't ever by built.
'ever be build.'
Comment on attachment 8583875 [details] [diff] [review]
[10] Bug 1146355: Prepare Bluetooth A2DP manager

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

LGTM.
Attachment #8583875 - Flags: review?(joliu) → review+
Comment on attachment 8583876 [details] [diff] [review]
[11] Bug 1146355: Prepare Bluetooth OPP manager

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

Looks like the difference is introduced since we didn't port Bug 1118177 to bluetooth2.
What's your plan on further combining this kind of difference?
We're going to review them in the cleanup later?
Let me know if you need my help to check on them or implement the cleanup patch.

It looks good to me if we don't need to combine this kind of difference in this round.
Attachment #8583876 - Flags: review?(joliu) → review+
Comment on attachment 8583879 [details] [diff] [review]
[13] Bug 1146355: Update Bluedroid configuration for Bluetooth LE support

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

It will enable LE when running on v1 code?
Could we leave this for v2 only?
Attachment #8583879 - Flags: review?(joliu)
Comment on attachment 8583871 [details] [diff] [review]
[08] Bug 1146355: Support Bluetooth v1 and v2 in backend interfaces

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

LGTM.
Attachment #8583871 - Flags: review?(joliu) → review+
Attachment #8584436 - Flags: review?(joliu) → review?(brsun)
Attachment #8583878 - Flags: review?(joliu) → review?(brsun)
Hi Bruce,

Could you help with bluez and HFP patch?

Thanks,
Jocelyn
Comment on attachment 8583878 [details] [diff] [review]
[12] Bug 1146355: Prepare Bluetooth HFP manager

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

LGTM
Attachment #8583878 - Flags: review?(brsun) → review+
Comment on attachment 8584436 [details] [diff] [review]
[14] Bug 1146355: Prepare BlueZ for Bluetooth v2

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

LGTM
Attachment #8584436 - Flags: review?(brsun) → review+
Hi

> Looks like the difference is introduced since we didn't port Bug 1118177 to
> bluetooth2.
> What's your plan on further combining this kind of difference?
> We're going to review them in the cleanup later?

Yes. Currently, I only went through the code and isolated differences by ifdef; maybe except for a few places where I know which was the correct version. My plan is to go through the differing code fragments after this patch set has been landed. We can sort them out one by one and be sure to not miss anything.
(In reply to jocelyn [:jocelyn] from comment #56)
> Comment on attachment 8583879 [details] [diff] [review]
> [13] Bug 1146355: Update Bluedroid configuration for Bluetooth LE support
> 
> Review of attachment 8583879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It will enable LE when running on v1 code?
> Could we leave this for v2 only?

Sure. I'll need to check how to do this though.
Changes since v1:

  - reordered patch set
Attachment #8584436 - Attachment is obsolete: true
Attachment #8588981 - Flags: review+
Changes since v1:

  - handle bug 1148311
Attachment #8583875 - Attachment is obsolete: true
Attachment #8589024 - Flags: review+
Only removes remaining files of bluetooth2's back-end code.
Attachment #8589026 - Flags: review?(joliu)
Attachment #8589027 - Flags: review?(joliu)
Attachment #8581748 - Attachment is obsolete: true
Attachment #8583879 - Attachment is obsolete: true
Attachment #8589027 - Flags: review?(joliu) → review+
Comment on attachment 8589026 [details] [diff] [review]
[14] Bug 1146355: Remove original directory of Bluetooth v2

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

LGTM.
Attachment #8589026 - Flags: review?(joliu) → review+
Comment on attachment 8583873 [details] [diff] [review]
[09] Bug 1146335: Prepare |BluetoothServiceBluedroid|

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

LGTM.

Let's further clean it up later.

Thanks,
Jocelyn
Attachment #8583873 - Flags: review?(joliu) → review+
Attachment #8581746 - Attachment is obsolete: true
Changes since v1:

  - cherry-picked changes from bug 1132343 and bug 1140951
Attachment #8583866 - Attachment is obsolete: true
Attachment #8590259 - Flags: review+
Changes since v1:

  - cherry-picked changes from bug 1132343 and bug 1140951
Attachment #8583871 - Attachment is obsolete: true
Attachment #8590260 - Flags: review+
Changes since v1:

  - cherry-picked changes from bug 1132343 and bug 1140951
Attachment #8583873 - Attachment is obsolete: true
Attachment #8590261 - Flags: review+
Changes since v2:

  - cherry-picked changes from bug 1132343 and bug 1140951
Attachment #8588981 - Attachment is obsolete: true
Attachment #8590262 - Flags: review+
Changes since v1:

  - cherry-picked changes from bug 1132343 and bug 1140951
Attachment #8589026 - Attachment is obsolete: true
Attachment #8590265 - Flags: review+
Changes since v1:

  - rebased onto m-c
Attachment #8589027 - Attachment is obsolete: true
Attachment #8590266 - Flags: review+
Depends on: 1154232
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: