Closed Bug 1119176 Opened 6 years ago Closed 6 years ago

[gonk-l] Implement bt_os_callouts_t in bluetoothd

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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: brsun, Assigned: brsun)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 1106858
In bug 1120471, I'm working on timers in libfdio. We'll need them to fix this bug.
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
I slightly modified the logic to check |periodic| boolean value in libfdio module.
Attachment #8548830 - Flags: review?(tzimmermann)
Use |add_timer_to_epoll_loop()|, |acquire_wake_lock()|, and |release_wake_lock()| to implement |bt_os_callouts_t| in bluetoothd module.
Attachment #8548831 - Flags: review?(tzimmermann)
(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 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.
Attachment #8548831 - Flags: review?(tzimmermann) → review-
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.
Attachment #8548830 - Flags: review?(tzimmermann)
Assignee: nobody → 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?
Flags: needinfo?(tzimmermann)
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
Flags: needinfo?(tzimmermann)
I updated the timer API in libfdio. This pull request will need an update.
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|.
Attachment #8548830 - Attachment is obsolete: true
Attachment #8548831 - Attachment is obsolete: true
Attachment #8549519 - Flags: review?(tzimmermann)
Thanks, I will set checkin-needed once bug 1120471 has been landed.
feature-b2g: --- → 2.2+
Status: NEW → ASSIGNED
Target Milestone: --- → 2.2 S4 (23jan)
Request for review again due to interface changes of libfdio on bug 1120471.
Attachment #8549519 - Attachment is obsolete: true
Attachment #8553479 - Flags: review?(tzimmermann)
Comment on attachment 8553479 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/12

Looks good. Thanks for your patience, Bruce.
Attachment #8553479 - Flags: review?(tzimmermann) → review+
Thank you Thomas. :)
Keywords: checkin-needed
Bruce, the Sheriffs asked me about b2g37 approval for bug 1120471. I think we'll need it for this bug as well.
Flags: needinfo?(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
Flags: needinfo?(brsun)
Attachment #8553479 - Flags: approval-mozilla-b2g37?
Oh, I just realized that you asked about uplift approval for bug 1120471, not the sheriffs. Not enough coffee today... :D
:)
Attachment #8553479 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.