Closed Bug 1106858 Opened 5 years ago Closed 5 years ago

[gonk-l] Adapt bluetoothd to API level 21

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S4 (23jan)
feature-b2g 2.2+
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: m1, Assigned: brsun)

References

Details

(Whiteboard: [ft:Peripherals])

Attachments

(2 files, 3 obsolete files)

bluetoothd fails to build on L as the create_bond() method has a new transport parameter.
Attached file PR (obsolete) —
Attachment #8531313 - Flags: review?(tzimmermann)
Oh, thanks for testing bluetoothd!

We haven't yet started porting to L because I wanted to get JB an KK landed first. We are implementing the BlueZ protocol [1] at specific versions. For Bluedroid up to KK this would be version 5.24, [2] for L, we haven't picked a version yet.

This means that support for API 21 has to be implemented in Gecko and the protocol. If you absolutely depend on the current patch, I can merge it, but we'll still need to apply the complete fix later.

BTW, is this the only fix required for L?

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt
[2] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt?id=5.24
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> 
> BTW, is this the only fix required for L?

So far, looks like it.  There'll surely be more once bug 1087195 lands.  Up to you if you want to land this I guess, was just trying to give a small nudge to the L port of bluetoothd.   We don't strictly need it right now by any means.
Attachment #8531313 - Flags: review?(tzimmermann)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> Oh, thanks for testing bluetoothd!
> 
> We haven't yet started porting to L because I wanted to get JB an KK landed
> first. We are implementing the BlueZ protocol [1] at specific versions. For
> Bluedroid up to KK this would be version 5.24, [2] for L, we haven't picked
> a version yet.

Hi! Thomas,

For sure, yes, we have started porting to L.
Thanks

--
Keven
Comment on attachment 8531313 [details] [review]
PR

We are doing L porting now. Please have a review. Thanks.
Attachment #8531313 - Flags: review?(tzimmermann)
(In reply to Ken Chang[:ken] from comment #5)
> Comment on attachment 8531313 [details] [review]
> PR
> 
> We are doing L porting now. Please have a review. Thanks.
Is this patch going to work on L? It looks like incomplete.
Comment on attachment 8531313 [details] [review]
PR

r- because I heard that this patch doesn't work. And we'll also need more changes for supporting individual profiles.
Attachment #8531313 - Flags: review?(tzimmermann) → review-
Bruce,

will you do the bluetoothd side of the L port as well? Can you take this bug?
Flags: needinfo?(brsun)
No problem. I will take it after bug 1102703 has been finished.
Flags: needinfo?(brsun)
See Also: → 1102703
A prototype to port bluetoothd on android lollipop.
Attachment #8542872 - Flags: feedback?(tzimmermann)
Attachment #8542872 - Flags: feedback?(shuang)
Attachment #8542872 - Flags: feedback?(btian)
Comment on attachment 8542872 [details] [diff] [review]
bug1106858_bluetooth_daemon_lollipop.prototype.patch

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

Looks mostly good to me. I plan to remove all these pointless wrapper functions at some point and put all code into the I/O functions. This will make the overall implementation a bit more readable.

::: src/bt-core-io.c
@@ +487,5 @@
> +{
> +  /* FIXME: need to be implemented in later patches */
> +  return BT_STATUS_FAIL;
> +};
> +

Nit: please remove this newline.

::: src/bt-core.h
@@ +18,5 @@
>  
>  #include <hardware/bluetooth.h>
>  
> +#if ANDROID_VERSION < 21
> +typedef struct bt_os_callouts_t bt_os_callouts_t;

Please don't do this. If you don't have all necessary types available in a version of Android, declare separate versions of |init_bt_core| for different Android versions instead.

::: src/bt-hf-io.c
@@ +221,5 @@
>    struct pdu_wbuf* wbuf;
>  
>    wbuf = create_pdu_wbuf(1 + /* volume type */
> +                         1 + /* volume */
> +                         6, /* address (bluez 5.25) */

Again, the capital letters are incorrect. Please follow the official writing when you refer to names. Here and everywhere else.

@@ +678,4 @@
>  static bt_status_t
>  opcode_start_voice_recognition(const struct pdu* cmd)
>  {
> +  bt_bdaddr_t bd_addr = { {0, 0, 0, 0, 0, 0} };

If you initialize and declaration at the same time, the statement should go at the end of the function's declare block. C before C99 can be picky about mixing declarations and program statements. Here and everywhere else.

@@ +838,4 @@
>  err_bt_hf_cops_response:
>    cleanup_pdu_wbuf(wbuf);
>  err_create_pdu_wbuf:
> +err_read_bt_bdaddr_t: ATTRIBS(UNUSED)

Instead of using an attribute here, I'd rather like to see this label being put into ANDROID_VERSION. Here and below.
Attachment #8542872 - Flags: feedback?(tzimmermann) → feedback+
Target Milestone: --- → 2.2 S4 (23jan)
Comment on attachment 8542872 [details] [diff] [review]
bug1106858_bluetooth_daemon_lollipop.prototype.patch

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

f=me with following comment and comment 11 addressed.

::: src/bt-av-io.c
@@ +179,4 @@
>  
>  bt_status_t
>  (*register_bt_av(unsigned char mode ATTRIBS(UNUSED),
> +                 unsigned long max_clients ATTRIBS(UNUSED),

Replace all |max_clients| with |max_num_clients| and |max_hf_clients| with |max_num_hf_clients|. Here and everywhere else.

::: src/bt-core-io.c
@@ +840,5 @@
>      return BT_STATUS_PARM_INVALID;
>  
> +#if ANDROID_VERSION >= 21
> +  off = read_pdu_at(cmd, off, "C", &transport);
> +  if (off < 0)

Simplify these two lines to:

  if (read_pdu_at(cmd, off, "C", &transport) < 0)

::: src/bt-core.c
@@ +29,5 @@
>  
> +#if ANDROID_VERSION < 21
> +struct bt_os_callouts_t
> +{
> +  /* A dummy struct definition just for backward compability. */

typo: compatibility
Attachment #8542872 - Flags: feedback?(btian) → feedback+
blocking-b2g: --- → 2.2?
Comment on attachment 8542872 [details] [diff] [review]
bug1106858_bluetooth_daemon_lollipop.prototype.patch

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

Can you actually run this daemon flawlessly? Please actually test this daemon patch before requesting review. :)

::: src/bt-av-io.c
@@ +103,5 @@
> +audio_config_cb(bt_bdaddr_t* bd_addr ATTRIBS(UNUSED),
> +                uint32_t sample_rate ATTRIBS(UNUSED),
> +                uint8_t channel_count ATTRIBS(UNUSED))
> +{
> +  /* nothing to do */

Please open an another bug, to support HFP 1.6 HF role.
And replace that line as '/* TODO: Support HFP 1.6 HF role */'
It would be more specific than saying 'nothing to do'.

::: src/bt-hf-io.c
@@ +348,5 @@
>  static void
> +wbs_cb(bthf_wbs_config_t wbs ATTRIBS(UNUSED),
> +       bt_bdaddr_t* bd_addr ATTRIBS(UNUSED))
> +{
> +  /* nothing to do */

/* TODO: Support HFP 1.6 WBS */
Attachment #8542872 - Flags: feedback?(shuang) → feedback+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Comment on attachment 8542872 [details] [diff] [review]
> bug1106858_bluetooth_daemon_lollipop.prototype.patch
> 
> Review of attachment 8542872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly good to me. I plan to remove all these pointless wrapper
> functions at some point and put all code into the I/O functions. This will
> make the overall implementation a bit more readable.
> 
> ::: src/bt-hf-io.c
> @@ +678,4 @@
> >  static bt_status_t
> >  opcode_start_voice_recognition(const struct pdu* cmd)
> >  {
> > +  bt_bdaddr_t bd_addr = { {0, 0, 0, 0, 0, 0} };
> 
> If you initialize and declaration at the same time, the statement should go
> at the end of the function's declare block. C before C99 can be picky about
> mixing declarations and program statements. Here and everywhere else.
> 

Yes, mixing declarations and program statements are not allowed in C89. But declaration-with-initialization is treated as declaration instead of the program statement that we concern about, so those codes don't suffer from this problem. Mixing declaration and declaration-with-intialization should be ok.

If bd_addr is not initialized like this way, we might have to use memset() on it or to assign a value to each element one-by-one to have a proper initial value.
Hi Bruce

> 
> Yes, mixing declarations and program statements are not allowed in C89. But
> declaration-with-initialization is treated as declaration instead of the
> program statement that we concern about, so those codes don't suffer from
> this problem. Mixing declaration and declaration-with-intialization should
> be ok.

Again, I've seen this fail in the real world if the initialization is not located at the end of the declaration block.

> If bd_addr is not initialized like this way, we might have to use memset()
> on it or to assign a value to each element one-by-one to have a proper
> initial value.

The current initialization is fine. I'm just worried about subtle differences between compilers of different platforms. Especially flatfish's compiler is picky about syntax.

BTW, assertions are not enabled by default. While you debug the patch set, you should set -UNDEBUG in LOCAL_CFLAGS in src/Android.mk. This usually helps a lot with finding bugs early on.
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2+
Whiteboard: [ft:Peripherals]
Status: NEW → ASSIGNED
Porting bluetoothd for Android Lollipop by extending the IPC protocol as BlueZ 5.26.
Attachment #8542872 - Attachment is obsolete: true
Attachment #8545705 - Flags: review?(tzimmermann)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
> Hi Bruce
> 
> > 
> > Yes, mixing declarations and program statements are not allowed in C89. But
> > declaration-with-initialization is treated as declaration instead of the
> > program statement that we concern about, so those codes don't suffer from
> > this problem. Mixing declaration and declaration-with-intialization should
> > be ok.
> 
> Again, I've seen this fail in the real world if the initialization is not
> located at the end of the declaration block.
> 
> > If bd_addr is not initialized like this way, we might have to use memset()
> > on it or to assign a value to each element one-by-one to have a proper
> > initial value.
> 
> The current initialization is fine. I'm just worried about subtle
> differences between compilers of different platforms. Especially flatfish's
> compiler is picky about syntax.
> 

I've tried to compile these codes with the compiler for flatfish platform, and the result is ok without any errors or warnings.

But anyway, I changed the coding style in PR as suggested. All declaration-with-initialization are put at the end of each declaration section.
Blocks: 1119176
Hi Bruce,

I left a number of comments in the pull request. The overall code looks good to me, but I'm concerned about all the TODOs and FIXMEs in the patches. Could you please go through the comments and answer my questions? Thanks.
Flags: needinfo?(brsun)
Hi Thomas,

Thanks for reviewing the patch. Comments are added in PR.

I am going to remove those TODOs by syncing our protocol to BlueZ 5.27. Are you still suggest to use the wording |5.26| in the comments?

The efforts for FIXMEs are not really adapting interfaces between Android Lollipop and BlueZ, bluetoothd needs to handle wake-up-timer and wake-lock inside its own process. And that will be address on bug 1119176.
Flags: needinfo?(brsun) → needinfo?(tzimmermann)
Comment on attachment 8545705 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/3

I think the overall patch looks good. The comments and the version numbers could be cleaned up. I left some notes in the pull request.

Thanks for the hard work, Bruce.
Flags: needinfo?(tzimmermann)
Attachment #8545705 - Flags: review?(tzimmermann) → review+
(In reply to Bruce Sun [:brsun] from comment #19)
> I am going to remove those TODOs by syncing our protocol to BlueZ 5.27

The corresponding command/response of bthf_interface_t.configure_wbs is added in BlueZ 5.27, but the corresponding notification of bthf_wbs_callback is still not included in BlueZ 5.27.

There is still TODO for wbs_cb() once BlueZ adds the corresponding notification.
Hi Thomas,

The used protocol has been synced with BlueZ 5.27. Would you mind having a look on it again?

Modification:
 - Implement |audio_config_cb()| with |OPCODE_AUDIO_CONFIGURATION_NTF| notification.
 - Add |opcode_configure_wbs()| and |bt_hf_configure_wbs()| with |OPCODE_CONFIGURE_WBS| command/response.
 - Remove some comments as suggested on the comment 20.
Attachment #8545705 - Attachment is obsolete: true
Attachment #8547984 - Flags: review?(tzimmermann)
(In reply to Bruce Sun [:brsun] from comment #21)
> (In reply to Bruce Sun [:brsun] from comment #19)
> > I am going to remove those TODOs by syncing our protocol to BlueZ 5.27
> 
> The corresponding command/response of bthf_interface_t.configure_wbs is
> added in BlueZ 5.27, but the corresponding notification of bthf_wbs_callback
> is still not included in BlueZ 5.27.
> 
> There is still TODO for wbs_cb() once BlueZ adds the corresponding
> notification.

I see. We don't use WBS, so this should be OK for now.
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1106858
User impact if declined: Bluetoothd cannot even be compiled on android lollipop.
Testing completed: bluetoothd start-up, pair-unpair, file-transfer, music playback.
Risk to taking this patch (and alternatives if risky): Low. bluetoothd has not been activated by default at this stage.
String or UUID changes made by this patch: N/A
Attachment #8548075 - Flags: review+
Attachment #8548075 - Flags: approval-mozilla-b2g37?
Keywords: checkin-needed
Hi Bhavana,

Kindly help check comment 25 to see if this patch (attachment 8548075 [details] [review]) can be uplifted to v2.2.
Thanks in advance.
Flags: needinfo?(bbajaj)
Master: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/aa3adea9c18ae00d36e597d5890cf14cb7dfb105
v2.2:   https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/3c52bcf2826f5631f4c87c2be9522f134a07686b

Whoops, kinda jumped the gun on the v2.2 uplift there. That said, I think a=NPOTB probably applies here given the whole "has not been activated by default" part so I'm going to leave it in.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: needinfo?(bbajaj)
Attachment #8548075 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #27)
> Whoops, kinda jumped the gun on the v2.2 uplift there. That said, I think
> a=NPOTB probably applies here given the whole "has not been activated by
> default" part so I'm going to leave it in.

Thanks! We actually don't know we can skip the process, because v2.2 has already been branched out. ;)
You need to log in before you can comment on or make changes to this bug.