1084257 - [Bluetooth] Implement Bluetooth daemon

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

We need a daemon program that conects Gecko and Bluedroid.

Comment 1

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

'connects'

Comment 2

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

Attachment: Github tree for bluetoothd

Hi,

Please review the first version of the Bluetooth daemon.

The daemon connects Gecko with Bluedroid and currently supports Bluetooth Core and Socket profiles. More profiles will be added later. The protocol between Gecko and bluetoothd is specified in [1]. The implementation uses libfdio [2] for its main loop and handling of file descriptors.

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt?id=5.24
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1031122

Comment 3

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

Michael,

do you know some guidelines for handling wake locks? Bluetoothd doesn't care about them ATM and I haven't seen any problems resulting from this. But I'm afraid that the system might suspend between reading a PDU from the socket and calling the respective Bluedroid interface.

Comment 4

[Michael Wu [:mwu]]

I don't know wakelocks particularly well. We get away with not caring sometimes just because the display wakelock is keeping things on.

Comment 6

[Michael Wu [:mwu]]

Is there any particular parts you'd like me to take a close look at? I don't know enough about bluetooth to do a very good review of the whole thing.

Comment 7

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

(In reply to Michael Wu [:mwu] from comment #6)
> Is there any particular parts you'd like me to take a close look at? I don't
> know enough about bluetooth to do a very good review of the whole thing.

No, nothing specific. I was just hoping you could provide general feedback or spot potential bugs. bt-proto.c, bt-pdubuf.c and bt-io.c might be most relevant files here.

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 8506842 [details]
Github tree for bluetoothd

Skimmed over a bit of a code. Seems reasonable overall for C code, though I think error handling in here is a bit painful since you're not using C++. In general, C++ is supported here and you're not writing a library, so it doesn't seem necessary to suffer.

In Android.mk, LOCAL_MODULE_TAGS should *not* be eng. Use optional and make sure bluetoothd is in PRODUCT_PACKAGES in the relevant device configurations. eng tagged packages are not installed in shipping builds.

Storing the source in src/ seems a bit silly for the size of this.

Comment 9

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

(In reply to Michael Wu [:mwu] from comment #8)
> Comment on attachment 8506842 [details]
> Github tree for bluetoothd
> 
> Skimmed over a bit of a code. Seems reasonable overall for C code, though I
> think error handling in here is a bit painful since you're not using C++. In
> general, C++ is supported here and you're not writing a library, so it
> doesn't seem necessary to suffer.
> 
> In Android.mk, LOCAL_MODULE_TAGS should *not* be eng. Use optional and make
> sure bluetoothd is in PRODUCT_PACKAGES in the relevant device
> configurations. eng tagged packages are not installed in shipping builds.
> 
See: https://github.com/mozilla-b2g/device-flame/blob/kitkat/flame.mk#L35  :)

Comment 10

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

I'm thinking how to review this daemon, I cannot use the review system to add comment. :(

Comment 11

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> I'm thinking how to review this daemon, I cannot use the review system to
> add comment. :(
Oh! Sorry, I need to add comments per commit. It looks fine to me now.

Comment 12

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

Comment on attachment 8506842 [details]
Github tree for bluetoothd

Updated Github tree:

  - fixed assertions
  - removed invalid error labels in |handle_pdu|
  - fixed returned value in |handle_pdu|
  - fixed warnings about unused arguments
  - fixed includes in bt-proto.h and service.c
  - replaced numeric length values by sizeof() constants in PDU reading/writing
  - renamed 'off' to 'offset' in bt-proto functions
  - call |uninit_core| in |uninit_core_io|
  - clarified comments in bt-io.c
  - |memset| on iovec in |io_state_in|
  - constant for |listen|'s backlog parameter
  - constant and comment for minimum property length

Still missing:

  - declare bluetoothd as optional: I'll add this change in the final patch to ease current development.

Comment 13

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

Comment on attachment 8506842 [details]
Github tree for bluetoothd

Updated Github tree:

  - commented on allocation sizes of payload buffer 
  - check (ANDROID_VERSION >= 18) for |le_test_mode| and BT_PROPERTY_REMOTE_INFO
  - replace '1' by 'sizeof(uint8_t)' in |append_bt_property_t_array|
  - removed unused variable 'tail' in |build_ancillary_data|
  - cleaned up and documented padding bytes for tail-buffer alignment in bt-sock-io.c
  - mark all unused arguments as UNUSED
  - built successfully on Flatfish

Comment 14

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

The latest version builds successfully on API versions 17 (flatfish), 18 (nexus-4), and 19 (flame-kk).

Comment 15

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

Comment on attachment 8506842 [details]
Github tree for bluetoothd

So all the Bluetooth service functions (enable/disable/pair/discovery/set name) can work properly? If things work normally, we can move on.

Comment 16

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

Hi

> So all the Bluetooth service functions (enable/disable/pair/discovery/set
> name) can work properly? If things work normally, we can move on.

Yes. I always do some basic test, like on/off, pairing, file transfer. Everything works so far.

The last missing thing from the reviews is the 'optional' tag in the Android.mk

Comment 17

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

Comment on attachment 8506842 [details]
Github tree for bluetoothd

Updated Github tree:

  - build with '-Wall -Werror' by default
  - use UNUSED attribute for fd in |io_fd_event_| functions
  - set LOCAL_MODULE_TAGS to 'optional'

Comment 18

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

Thanks for your patience and dedication.

Comment 19

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

Great, thanks! I'll open bug reports to get an official repository set up.

Comment 20

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

Thanks everyone. The tree has landed in bug 1100266.