The default bug view has changed. See this FAQ.

B2G Bluetooth: Turn bluetooth adapter on/off

RESOLVED FIXED in mozilla13

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: qdot, Assigned: ericchou)

Tracking

Trunk
mozilla13
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Allow the system bluetooth adapter to be turned on/off via the BT DOM API.
(Reporter)

Updated

5 years ago
Blocks: 711601
OS: Linux → Gonk
(Reporter)

Updated

5 years ago
Assignee: nobody → kyle
(Reporter)

Comment 1

5 years ago
This should be fairly trivial, most of the code work will be happening in adding the navigator.bluetooth stuff.

All of the code to turn the bluetooth on/off in android, as well as to get status of the adapter, is in glue/gonk/system/bluetooth/bluetooth.c, which compiles to /system/lib/libbluedroid.so. We can either link against this and call its function, or just integrate what it does into our code directly. It's mostly just echoing 0/1's to a node on sysfs (/sys/class/rfkill/rfkill?/type where ? is the adapter index, usually 0 for a phone). 

The samsung phones wrap this in an undocumented daemon called btld. Assuming btld doesn't do something interesting with the broadcom adapter driver, we may be able to just ignore it.
(Reporter)

Comment 2

5 years ago
Unless we want to go the rilproxy route (we don't), we'll also need to change the permissions in the init.rc for the kernel, as it has the following lines:

    chown bluetooth bluetooth /sys/class/rfkill/rfkill0/state
    chown bluetooth bluetooth /sys/class/rfkill/rfkill1/state
    chown bluetooth bluetooth /sys/class/rfkill/rfkill0/type
    chown bluetooth bluetooth /sys/class/rfkill/rfkill1/type

Since we'll want to be able to change these from b2g, and b2g is running as root, we should be able to just comment this out (leaving the perms as root) and be ok.
(Reporter)

Comment 3

5 years ago
Ok, strike that. btld is broadcom's proprietary bluetooth stack, which works outside of bluez. So we don't have all the access we want from libbluetooth alone. Researching further.
(Reporter)

Comment 4

5 years ago
And reverse it. Cyanogenmod seems to successfully use brcm_patchrom_plus as the same service that btld provides. Just need to switch it out in the init.rc.
Hardware: x86_64 → All
(Reporter)

Comment 5

5 years ago
Managed to get bluetooth firmware loaded and bluetoothd running on SGS2, using cyanogenmod brcm_patchram_plus loading methods. This requires some patches to brcm_patchram_plus due to the fact that the phone is on the BCM4330 firmware, which doesn't send a 2 byte reply that's expected by older versions of brcm_patchram_plus. There's two places these patches exist: the ics updates of system/bluetooth, and cyanogenmod's versions of system/bluetooth. For the moment, I'm going to try repointing us at a fork of cyanogenmod's system/bluetooth, since moving the ICS stuff would require both a fork and some bluez header reverts.
(Reporter)

Comment 6

5 years ago
Looks like even though firmware is loading, we can't actually do anything with the chip yet. "hcitool scan" always times out, and "hciconfig -a" drops out when trying to do things like getting the local BT device name. So, there's some extra setting we're missing.
(Reporter)

Comment 7

5 years ago
I'm pretty sure we're using the wrong kernel version for B2G. We're using the first drop of the Samsung open source kernel from august, there've been 2 since then. In the first drop, the bcm4330 driver didn't exist yet so they're using bcm4329, which could explain our connection issues. Cyanogenmod uses a newer kernel with bcm4330 driver built in, and works fine.

This is now being tracked at

https://github.com/andreasgal/GT9100-kernel/issues/1
(Reporter)

Comment 8

5 years ago
Ran bluetoothd through logwrapper in init.rc, results are not happy:

I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Bluetooth deamon 4.69
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() parsing main.conf
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() discovto=120
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() pairto=0
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() pageto=8192
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() name=%m
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() class=0x40020C
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() discov_interval=0
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() deviceid=android:generic:1.5
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:parse_config() default_link_policy=7
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Starting SDP server
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Got Unix socket fd '14' from environment
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Adding device id record for 000a:0000
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/plugin.c:plugin_init() Loading builtin plugins
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/plugin.c:add_plugin() Loading hciops plugin
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/plugin.c:plugin_init() Loading plugins /system/lib/bluez-plugin
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/plugin.c:add_plugin() Loading audio plugin
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/plugin.c:add_plugin() Loading input plugin
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: HCI dev 0 registered
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/plugins/hciops.c:init_device() child 2726 forked
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/adapter.c:btd_adapter_ref() 0x10d50: ref=1
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: HCI dev 0 up
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Starting security manager 0
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/adapter.c:btd_adapter_set_class() Changing Major/Minor class to 0x00020c
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/adapter.c:adapter_start() Stopping Inquiry at adapter startup
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/adapter.c:btd_adapter_ref() 0x10d50: ref=2
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/adapter.c:btd_adapter_ref() 0x10d50: ref=3
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10001
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000019-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110a-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110d-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10002
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000017-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110c-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110e-0000-1000-8000-00805f9
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: ioctl(HCIUNBLOCKADDR): Invalid argument (22)
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Adapter /org/bluez/2725/hci0 has been enabled
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Failed to open RFKILL control device
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/main.c:main() Entering main loop
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/plugins/hciops.c:child_exit() child 2726 exited
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: external/bluetooth/bluez/src/adapter.c:adapter_update_tx_power() inquiry respone tx power level is 0
I//system/bin/bluetoothd( 2724): bluetoothd[2725]: Inquiry Failed with status 0x12
No longer blocks: 711601
Blocks: 727618
(Assignee)

Updated

5 years ago
Assignee: kyle → echou
(Assignee)

Comment 9

5 years ago
The bluetooth firmware loading issue has been already fixed, see https://github.com/andreasgal/B2G/issues/124.

Kyle has done most work in Gecko, see https://bugzilla.mozilla.org/show_bug.cgi?id=711601, and I'm going to complete the rest implementations for this bug, making Bluetooth can be turned on/off via BT DOM API correctly.
(Assignee)

Comment 10

5 years ago
Created attachment 599546 [details] [diff] [review]
Including async bt on/off and firing event to JS
Attachment #599546 - Flags: review?(kyle)
(Reporter)

Updated

5 years ago
Blocks: 729470
(Reporter)

Comment 11

5 years ago
Comment on attachment 599546 [details] [diff] [review]
Including async bt on/off and firing event to JS

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

Notes from reviewing alongside bent:

- nsRefPtr-ize mAdapter object in task, fix reference counting during pass
- change to lazy idle thread
- fix spacing/tabs
- make sure tabwidth is 4 not 40 in emacs formatting
- remove MozBluetooth Adapter renaming changes
Attachment #599546 - Flags: review?(kyle) → review-
(Reporter)

Comment 12

5 years ago
Also:

- Move macros in BluetoothAdapterMacros back into BluetoothAdapter file.
(Assignee)

Comment 13

5 years ago
Created attachment 599923 [details] [diff] [review]
patch for the items listed by Kyle
Attachment #599546 - Attachment is obsolete: true
Attachment #599923 - Flags: review?
(Assignee)

Comment 14

5 years ago
Created attachment 599925 [details] [diff] [review]
Patch 1: including async bt on/off and event firing.
Attachment #599923 - Attachment is obsolete: true
Attachment #599923 - Flags: review?
Attachment #599925 - Flags: review?(kyle)
(Assignee)

Comment 15

5 years ago
Created attachment 599931 [details] [diff] [review]
Patch 1: including async bt on/off and event firing.
Attachment #599925 - Attachment is obsolete: true
Attachment #599925 - Flags: review?(kyle)
Attachment #599931 - Flags: review?(kyle)
(Reporter)

Comment 16

5 years ago
Comment on attachment 599931 [details] [diff] [review]
Patch 1: including async bt on/off and event firing.

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

Looks good. r=me after changes below are made.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +31,5 @@
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(BluetoothAdapter)
> +  NS_INTERFACE_MAP_ENTRY(nsIDOMBluetoothAdapter)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMBluetoothAdapter)

This line can be killed, since we'll pick up nsISupports in the inheritance chain.

@@ +57,5 @@
>  NS_IMETHODIMP
>  BluetoothAdapter::SetPower(bool aPower)
>  {
>    mPower = aPower;
> +

We should check to make sure mPower != aPower before we do anything, otherwise we'll keep reloading firmware.

::: dom/bluetooth/BluetoothToggleTask.h
@@ +1,1 @@
> +#ifndef mozilla_dom_bluetooth_bluetoothtoggletask_h__

This file should be moved into the top of BluetoothAdapter.cpp. We'll just have the code in there for both platforms, and that's the only place it'll be used. Not to mention, planning ahead for the dlopen fix, we'll be calling bt_is_enabled from there so it'd be nice to have all of that in the same place.

@@ +6,5 @@
> +#if defined(MOZ_WIDGET_GONK)
> +#include <bluedroid/bluetooth.h>
> +#endif
> +
> +typedef mozilla::dom::bluetooth::BluetoothAdapter BtAdapter;

This stuff should be under the m::d::bt namespace, so we shouldn't need this. You can also just wrap it in BEGIN/END_BLUETOOTH_NAMESPACE which is in the BluetoothCommon.h file. Note that even after moving this code to BluetoothAdapter.cpp, it'll still need to be wrapped, since that file just uses USING_BLUETOOTH_NAMESPACE. Might actually be worth it to switch out the USING for the BEGIN/END there too.
Attachment #599931 - Flags: review?(kyle) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 599960 [details] [diff] [review]
Patch 1: including async bt on/off and event firing. r=Kyle
Attachment #599931 - Attachment is obsolete: true
Attachment #599960 - Flags: review?(kyle)
(Reporter)

Comment 18

5 years ago
Comment on attachment 599960 [details] [diff] [review]
Patch 1: including async bt on/off and event firing. r=Kyle

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

Looks good, time to land. r=me

I'll take care of the landing since you don't have rights yet.

::: dom/bluetooth/BluetoothToggleTask.h
@@ +1,1 @@
> +#ifndef mozilla_dom_bluetooth_bluetoothtoggletask_h__

We don't need this file in the patch anymore. :)

Don't worry about it though, I'll take care of it when I land it.
Attachment #599960 - Flags: review?(kyle) → review+
(Reporter)

Comment 19

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec8b8731343b
Comment on attachment 599960 [details] [diff] [review]
Patch 1: including async bt on/off and event firing. r=Kyle

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +15,5 @@
> +#if defined(MOZ_WIDGET_GONK)
> +#include <bluedroid/bluetooth.h>
> +#endif
> +
> +#define POWERED_EVENT_NAME NS_LITERAL_STRING("powered")

Since you have to use this constant three times (in header, in InitEvent, and NS_IMPL_EVENT_HANDLER) I would suggest changing this as follows:

Header:

  #define POWERED_EVENT_NAME "powered"
 
Cpp:

  event->InitEvent(NS_LITERAL_STRING(POWERED_EVENT_NAME), ...)
  ...
  NS_IMPL_EVENT_HANDLER(BluetoothAdapter, POWERED_EVENT_NAME)

@@ +17,5 @@
> +#endif
> +
> +#define POWERED_EVENT_NAME NS_LITERAL_STRING("powered")
> +
> +BEGIN_BLUETOOTH_NAMESPACE

Nit: This is not necessary. In fact, I'd recommend using the anonymous namespace here.

@@ +27,5 @@
> +      : mResult(result)
> +    {
> +      MOZ_ASSERT(!NS_IsMainThread()); // This should be running on the worker thread
> +
> +      mAdapterPtr.swap(adapterPtr);

Nit: Since this is tricky a comment saying why we do this could be helpful.

@@ +32,5 @@
> +    }
> +
> +    NS_IMETHOD Run() {
> +      MOZ_ASSERT(NS_IsMainThread()); // This method is supposed to run on the main thread!
> +      mAdapterPtr->FirePowered();

Here's the race! Need to null out mAdapterPtr here before returning (while you're guaranteed to be on the main thread).

The background thread maintains a (threadsafe) ref on this runnable while it is dispatching to the main thread, so if this runnable runs on main thread before background thread finishes cleaning up then the background thread could end up with the last reference to this runnable. When it releases the last ref it will destroy this runnable and call release on mAdapterPtr if it is non-null. That will crash in the CC.

Very subtle. The only way I know about this is because I've committed this exact bug about 500 times now, and had to debug it every time :-/

@@ +38,5 @@
> +      return NS_OK;
> +    }
> +
> +  private:
> +    bool mResult;

This is unused and we should figure out what to do here. Simple warning? JS console message? Fire some error event at the page? If we don't care then we should remove this.

@@ +47,5 @@
> +{
> +  public:
> +    ToggleBtTask(bool onOff, BluetoothAdapter* adapterPtr)
> +      : mOnOff(onOff),
> +      mAdapterPtr(adapterPtr)

Nit: Indentation is weird here.

@@ +70,5 @@
> +#endif
> +
> +      // Create a result thread and pass it to Main Thread, 
> +      nsCOMPtr<nsIRunnable> resultRunnable = new ToggleBtResultTask(result, mAdapterPtr);
> +      NS_DispatchToMainThread(resultRunnable);

This can (theoretically) fail. I'd make this:

  if (NS_FAILED(NS_DispatchToMainThread(...))) {
    NS_WARNING("Failed to dispatch to main thread!");
  }

@@ +87,2 @@
>  
>  USING_BLUETOOTH_NAMESPACE

Nit: I'd move this to the top of the file, then you don't have to add the 'mozilla::dom::bluetooth::' in the line above.

@@ +91,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(BluetoothAdapter,
> +    nsDOMEventTargetHelper)
> +NS_CYCLE_COLLECTION_TRAVERSE_EVENT_HANDLER(powered)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

Nit: indent on these macros got screwed up somewhere along the way.

@@ +114,5 @@
>  NS_IMETHODIMP
>  BluetoothAdapter::GetPower(bool* aPower)
>  {
> +#if defined(MOZ_WIDGET_GONK)  
> +  *aPower = bt_is_enabled();

Eric thinks that this is a blocking call, and this is a synchronous getter that happens on the main thread. Can we just always return the mPower value (i.e. the last thing we set to)? Otherwise we need to reassess here.

@@ +142,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> r = new ToggleBtTask(mPower, this);
> +
> +  mToggleBtThread->Dispatch(r, 0);

This can fail, so you should change ToggleBluetoothAsync to return nsresult, then make sure you use the result in SetPower so the web page will know if something goes wrong.

Also, don't use '0' here, use NS_DISPATCH_NORMAL (from nsIEventTarget.idl)

@@ +153,5 @@
> +  nsresult rv = event->InitEvent(POWERED_EVENT_NAME, false, false);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool dummy;
> +  rv = DispatchEvent(event, &dummy);

This event will be untrusted by default. Need to call SetIsTrusted on it before dispatching.

::: dom/bluetooth/BluetoothAdapter.h
@@ +13,5 @@
>  
>  BEGIN_BLUETOOTH_NAMESPACE
>  
>  class BluetoothAdapter : public nsIDOMBluetoothAdapter
> +                        ,public nsDOMEventTargetHelper

Nit: space between , and public, then line up. Or put , on previous line.

@@ +22,5 @@
>  
> +  NS_FORWARD_NSIDOMEVENTTARGET(nsDOMEventTargetHelper::)
> +
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(BluetoothAdapter,
> +                                           nsDOMEventTargetHelper)

Ok, since you've made this an event target you have to change the flags in nsDOMClasInfo.cpp. Specifically, use nsEventTargetSH and EVENTTARGET_SCRIPTABLE_FLAGS (rather than nsDOMGenericSH and DOM_DEFAULT_SCRIPTABLE_FLAGS).

@@ +34,5 @@
> +
> +  NS_DECL_EVENT_HANDLER(powered)
> +
> +private:
> +  nsCOMPtr<nsIEventTarget> mToggleBtThread;

Since you're using nsIEventTarget here you should forward declare it above (before BEGIN_BLUETOOTH_NAMESPACE)
Blocks: 729991
Er, oops. That review comment was meant for bug 729991. Let's move there.
No longer blocks: 729991
Depends on: 729991
https://hg.mozilla.org/mozilla-central/rev/ec8b8731343b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.