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

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 2

[Andrew Overholt [:overholt]]

Full reboot to fix if BT firmware doesn't load properly seems bad.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Make dylib symbol binding actually retry.

Comment 4

[Eric Chou [:ericchou] [:echou]]

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

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Make dylib symbol binding retry, let bluetooth firmware shutdown still work even on error

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.

Comment 6

[Eric Chou [:ericchou] [: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

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c65dde5a84b1

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c65dde5a84b1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1ad4fc08462d