[Bluetooth] Implement Bluetooth daemon

RESOLVED FIXED in 2.1 S9 (21Nov)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.1 S9 (21Nov)
All
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+)

Details

Attachments

(1 attachment)

We need a daemon program that conects Gecko and Bluedroid.
'connects'
Created attachment 8506842 [details]
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
Attachment #8506842 - Flags: review?(shuang)
Attachment #8506842 - Flags: review?(mwu)
See Also: → bug 1073548
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

4 years ago
I don't know wakelocks particularly well. We get away with not caring sometimes just because the display wakelock is keeping things on.
Duplicate of this bug: 1065335

Comment 6

4 years ago
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.
(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

4 years ago
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.
Attachment #8506842 - Flags: review?(mwu)
(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  :)
I'm thinking how to review this daemon, I cannot use the review system to add comment. :(
(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.
Blocks: 1095491
Blocks: 1087195, 1087196, 1087198
No longer blocks: 1005934
Attachment #8506842 - Flags: review?(shuang)
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.
Attachment #8506842 - Flags: review?(shuang)
Attachment #8506842 - Flags: review?(shuang)
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
Attachment #8506842 - Flags: review?(shuang)
The latest version builds successfully on API versions 17 (flatfish), 18 (nexus-4), and 19 (flame-kk).
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.
Flags: needinfo?(tzimmermann)
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
Flags: needinfo?(tzimmermann)
Attachment #8506842 - Flags: review?(shuang)
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'
Attachment #8506842 - Flags: review?(shuang)
Attachment #8506842 - Flags: review?(shuang) → review+
Thanks for your patience and dedication.
Great, thanks! I'll open bug reports to get an official repository set up.
Depends on: 1100266
Depends on: 1100270

Updated

4 years ago
feature-b2g: --- → 2.2+
Blocks: 1104620
Thanks everyone. The tree has landed in bug 1100266.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → 2.1 S9 (21Nov)
You need to log in before you can comment on or make changes to this bug.