Closed Bug 791268 Opened 8 years ago Closed 8 years ago

[b2g-bluetooth] No attempt made to enable/disable firmware if bt_is_enabled returns -1

Categories

(Firefox OS Graveyard :: General, defect, P3)

All
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: bent.mozilla, Assigned: qdot)

References

Details

(Whiteboard: [LOE:S])

Attachments

(1 file, 1 obsolete file)

See http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/gonk/BluetoothGonkService.cpp#118

Looks like we won't ever try to reenable the firmware if that ever returns -1. Not sure if that's possible or likely but seems like we should figure out something better.
I'm not exactly sure how firmware loading would intermittently fail, but it seems like something we should fix without opaquely requiring a full phone reboot.
blocking-basecamp: --- → ?
Full reboot to fix if BT firmware doesn't load properly seems bad.
Assignee: nobody → kyle
blocking-basecamp: ? → +
Priority: -- → P3
Whiteboard: [LOE:S]
Comment on attachment 676261 [details] [diff] [review]
Patch 1 (v1) - Make dylib symbol binding actually retry.

Review of attachment 676261 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good, but it seems that this patch only solve part of the problem? I am not pretty sure if it actually solved the problem described in the title, "No attempt made to enable/disable firmware if bt_is_enabled returns -1". We still won't retry enabling Bluetooth after this patch lands, right? 

Sorry if I misunderstood anything. I'm going to cancel the review request and please have me review again once my above question is answered. Please feel free to leave comment. Thanks.

::: dom/bluetooth/gonk/BluetoothGonkService.cpp
@@ +47,1 @@
>    {

Nit: Please help to put brace to the end of previous line

@@ +47,2 @@
>    {
>      return sBluedroidFunctions.initialized;

Nit: How about just return true here?

@@ +70,4 @@
>      NS_ERROR("Failed to attach bt_is_enabled function");
>      return false;
>    }
> +  

Nit: Unnecessary blanks
Attachment #676261 - Flags: review?(echou)
Ah, yeah, I got caught up in one way we could do that and ignored the original one. Thanks for catching that. :)

So, checking inside bluedroid, when calling bt_is_enabled it does a test on the bluez exposed hci socket. I'm going to /assume/ that we don't need bluetoothd up for that to register true, which means that if we block on enable, then call is_bluetooth_enabled() right after, if we get back -1, we're ok to just disable and pass back an error. The user can then retry as needed. I think that'll fix this up.

I also did a bit of cleanup. Removed some unused functions, staticed some file local functions.
Attachment #676261 - Attachment is obsolete: true
Attachment #677178 - Flags: review?(echou)
Comment on attachment 677178 [details] [diff] [review]
Patch 1 (v2) - Make dylib symbol binding retry, let bluetooth firmware shutdown still work even on error

Review of attachment 677178 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

::: dom/bluetooth/gonk/BluetoothGonkService.cpp
@@ +98,5 @@
> +    if (sBluedroidFunctions.bt_is_enabled() < 0) {
> +      // if isEnabled < 0, this means we brought up the firmware, but something
> +      // went wrong with bluetoothd. Post a warning message, but try to proceed
> +      // with firmware unloading if that was requested, so we can retry later.
> +      NS_WARNING("Bluetooth firmware up, but cannot connect to HCI socket! Check bluetoothd and try stopping/starting bluetooth again.");

Nit: needs to wrap
Attachment #677178 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/c65dde5a84b1
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.