1119176 - [gonk-l] Implement bt_os_callouts_t in bluetoothd

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

Bug 1106858 just use dummy functions for bt_os_callouts_t on android lollipop. This bug is created for adding correct implementation for bt_os_callouts_t.

Comment 1

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

In bug 1120471, I'm working on timers in libfdio. We'll need them to fix this bug.

Comment 2

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

Bruce,

In case you'll work on this bug, there is timer support in the libfdio branch at [1]. The code is simple, but mostly untested. Please let me know about bugs.

[1] https://github.com/tdz/platform_system_libfdio/tree/bug-1120471

Comment 3

[Bruce Sun [:brsun]]

Attachment: https://github.com/tdz/platform_system_libfdio/pull/1

I slightly modified the logic to check |periodic| boolean value in libfdio module.

Comment 4

[Bruce Sun [:brsun]]

Attachment: https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/7

Use |add_timer_to_epoll_loop()|, |acquire_wake_lock()|, and |release_wake_lock()| to implement |bt_os_callouts_t| in bluetoothd module.

Comment 5

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

(In reply to Bruce Sun [:brsun] from comment #3)
> Created attachment 8548830 [details] [review]
> https://github.com/tdz/platform_system_libfdio/pull/1
> 
> I slightly modified the logic to check |periodic| boolean value in libfdio
> module.

Makes sense. Thanks for fixing this bug. I'll integrate the fix directly into the original patch and give you credit in the commit message.

Comment 6

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

Comment on attachment 8548831 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/7

I r- because of the problem with CLOCK_REALTIME. This other stuff is nits and minor additions.

Comment 7

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

Comment on attachment 8548830 [details] [review]
https://github.com/tdz/platform_system_libfdio/pull/1

I merged the change into the original patch. Thanks again.

Comment 8

[Bruce Sun [:brsun]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> Comment on attachment 8548831 [details] [review]
> https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/7
> 
> I r- because of the problem with CLOCK_REALTIME. This other stuff is nits
> and minor additions.

Thank you very much for pointing this out. I will change my patch correspondingly.

Regarding to the return values of |acquire_wake_lock()| and |release_wake_lock()|, I think it would be a little bit hard to distinguish whether these functions success or not. We cannot completely avoid false-positive situations even if we do tricky things to compare the return value with all possible errno values. I would suggest to keep things simple by just ignoring the return values of these two functions. What do you think?

Comment 9

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

Hi

> Regarding to the return values of |acquire_wake_lock()| and
> |release_wake_lock()|, I think it would be a little bit hard to distinguish
> whether these functions success or not. We cannot completely avoid
> false-positive situations even if we do tricky things to compare the return
> value with all possible errno values. I would suggest to keep things simple
> by just ignoring the return values of these two functions. What do you think?

Oh, you're right. I just checked the implementation of these functions at [1] a little closer. It looks like their return values are total garbage and not usable. Either they return a positive error code or a positive 'success' indicator. Very clever, Google, very clever... m(

OK, then let's drop the idea with the warnings.

[1] https://android.googlesource.com/platform/hardware/libhardware_legacy/+/master/power/power.c

Comment 10

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

I updated the timer API in libfdio. This pull request will need an update.

Comment 11

[Bruce Sun [:brsun]]

Attachment: https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/9

Modifications:
 - Reorder the sequence of |#include <hardware_legacy/power.h>|.
 - Remove |read(fd, ...)| from |alarm_event_in()|.
 - Add |alarm_state->cb(alarm_state->data)| in |alarm_event_in()|. (ah...yes, this was missed in the previous PR...sorry about that.)
 - Refine error checking of |alarm_state = malloc()|.
 - Replace |CLOCK_REALTIME_ALARM| with |CLOCK_BOOTTIME_ALARM|.
 - Replace |CLOCK_REALTIME| with |CLOCK_BOOTTIME|.

Comment 12

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

Comment on attachment 8549519 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/9

Looks good. Thanks!

Comment 13

[Bruce Sun [:brsun]]

Thanks, I will set checkin-needed once bug 1120471 has been landed.

Comment 14

[Bruce Sun [:brsun]]

Attachment: https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/12

Request for review again due to interface changes of libfdio on bug 1120471.

Comment 15

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

Comment on attachment 8553479 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/12

Looks good. Thanks for your patience, Bruce.

Comment 16

[Bruce Sun [:brsun]]

Thank you Thomas. :)

Comment 17

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/f0d6ce5f727eca4b10850c610a06a45772689a75

Comment 18

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

Bruce, the Sheriffs asked me about b2g37 approval for bug 1120471. I think we'll need it for this bug as well.

Comment 19

[Bruce Sun [:brsun]]

Comment on attachment 8553479 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/12

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: Bluetooth on Android Lollipop might not work properly.
Testing completed: enable/disable, pair/unpair, connect/disconnect, music playback, file transfer.
Risk to taking this patch (and alternatives if risky): Low. This patch is not big and just to use simple implementation to fulfill the callback of bluedroid on Android Lollipop.
String or UUID changes made by this patch: N/A

Comment 20

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

Oh, I just realized that you asked about uplift approval for bug 1120471, not the sheriffs. Not enough coffee today... :D

Comment 21

[Bruce Sun [:brsun]]

:)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/82088c8005d6916eaf1e94c6fb26ee6efa93da51