[gonk-l] Implement bt_os_callouts_t in bluetoothd

RESOLVED FIXED in 2.2 S4 (23jan)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
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
Assignee

Comment 3

5 years ago
I slightly modified the logic to check |periodic| boolean value in libfdio module.
Attachment #8548830 - Flags: review?(tzimmermann)
Assignee

Comment 4

5 years ago
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

Updated

5 years ago
Assignee: nobody → brsun
Assignee

Comment 8

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

Comment 11

5 years ago
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)
Assignee

Comment 13

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

Updated

5 years ago
feature-b2g: --- → 2.2+
Status: NEW → ASSIGNED

Updated

5 years ago
Target Milestone: --- → 2.2 S4 (23jan)
Assignee

Comment 14

5 years ago
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+
Assignee

Comment 16

5 years ago
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)
Assignee

Comment 19

4 years ago
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
Assignee

Comment 21

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