Hook up RSSI and host transmit power Bluetooth device properties for ChromeOS.

Hook up RSSI and host transmit power Bluetooth device properties for ChromeOS.

BUG=365966
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269676

[Tim Song, 2014-05-08 23:28:13]

Please take a look.

[keybuk, 2014-05-09 00:35:25]

https://codereview.chromium.org/273953002/diff/20001/DEPS
File DEPS (right):

https://codereview.chromium.org/273953002/diff/20001/DEPS#newcode472
DEPS:472: "@5b855c05cbbd9d4dcd7430fa58e73a296d038047",
DEPS rolls are always done in separate commits

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.h (right):

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:81: dbus::Property<int16> inquiry_rssi;
Properties must match the D-Bus name, keep this as rssi

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:91: dbus::Property<int16> tx_power;
This is called connection_tx_power in your cros_system_api change. Properties
must match their D-Bus name.

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:95: dbus::Property<int16> tx_power_max;
ditto

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_device_client.h (right):

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_device_client.h:205: void UpdateInquiryRSSI(const
dbus::ObjectPath& object_path, int16 rssi);
Nak on name change

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:340: property_name ==
properties->tx_power.name())
add ordinary rssi while you're in here?

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:232: :
properties->inquiry_rssi.value();
Do we really want to conflate these properties into one?

Good arguments were given upstream for not doing that ... shouldn't we follow
suit?

If not, the Inquiry RSSI is likely to be better for BR/EDR devices (the
connection one is relative to golden range) - while the Connection RSSI is
better for LE devices

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:564: StartConnectionMonitor();
NAK, doing a connection monitor for every single paired device is expensive with
multiple round-trips to the controller

This should be something enabled on a per-device basis

[Tim Song, 2014-05-09 02:55:28]

https://codereview.chromium.org/273953002/diff/20001/DEPS
File DEPS (right):

https://codereview.chromium.org/273953002/diff/20001/DEPS#newcode472
DEPS:472: "@5b855c05cbbd9d4dcd7430fa58e73a296d038047",
On 2014/05/09 00:35:25, keybuk wrote:
> DEPS rolls are always done in separate commits

Done.

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
File chromeos/dbus/bluetooth_device_client.h (right):

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:81: dbus::Property<int16> inquiry_rssi;
On 2014/05/09 00:35:25, keybuk wrote:
> Properties must match the D-Bus name, keep this as rssi

Done.

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:91: dbus::Property<int16> tx_power;
On 2014/05/09 00:35:25, keybuk wrote:
> This is called connection_tx_power in your cros_system_api change. Properties
> must match their D-Bus name.

Done.

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/bluetooth_...
chromeos/dbus/bluetooth_device_client.h:95: dbus::Property<int16> tx_power_max;
On 2014/05/09 00:35:25, keybuk wrote:
> ditto

Done.

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/fake_bluet...
File chromeos/dbus/fake_bluetooth_device_client.h (right):

https://codereview.chromium.org/273953002/diff/20001/chromeos/dbus/fake_bluet...
chromeos/dbus/fake_bluetooth_device_client.h:205: void UpdateInquiryRSSI(const
dbus::ObjectPath& object_path, int16 rssi);
On 2014/05/09 00:35:25, keybuk wrote:
> Nak on name change

Done.

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.cc:340: property_name ==
properties->tx_power.name())
On 2014/05/09 00:35:25, keybuk wrote:
> add ordinary rssi while you're in here?

Done.

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:232: :
properties->inquiry_rssi.value();
On 2014/05/09 00:35:25, keybuk wrote:
> Do we really want to conflate these properties into one?
> 
> Good arguments were given upstream for not doing that ... shouldn't we follow
> suit?
> 
> If not, the Inquiry RSSI is likely to be better for BR/EDR devices (the
> connection one is relative to golden range) - while the Connection RSSI is
> better for LE devices

It does make sense to have a connection RSSI and an inquiry RSSI as separate
values.

The GetRSSI() function in the BluetoothDevice interface says that it should
return the inquiry RSSI if not connected and connection RSSI if connected. I
think we should split this in a separate CL as it touches the extension API.

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:564: StartConnectionMonitor();
On 2014/05/09 00:35:25, keybuk wrote:
> NAK, doing a connection monitor for every single paired device is expensive
with
> multiple round-trips to the controller
> 
> This should be something enabled on a per-device basis

In that case, should we add an API in chrome.bluetooth, and I guess the
BluetoothDevice interface, to enable connection monitoring?

How would this work for other platforms, like Mac, which doesn't have system
supported polling? Should we do the polling in Chrome to make the interface
consistent?

[keybuk, 2014-05-09 18:22:07]

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:232: :
properties->inquiry_rssi.value();
On 2014/05/09 02:55:29, Tim Song wrote:

> The GetRSSI() function in the BluetoothDevice interface says that it should
> return the inquiry RSSI if not connected and connection RSSI if connected. I
> think we should split this in a separate CL as it touches the extension API.

That separate CL will be much easier if you only store connection RSSI in this
function (matching the mac implementation)

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:564: StartConnectionMonitor();
On 2014/05/09 02:55:29, Tim Song wrote:
> In that case, should we add an API in chrome.bluetooth, and I guess the
> BluetoothDevice interface, to enable connection monitoring?
> 
> How would this work for other platforms, like Mac, which doesn't have system
> supported polling? Should we do the polling in Chrome to make the interface
> consistent?

Yes.

[keybuk, 2014-05-09 18:27:06]

https://codereview.chromium.org/273953002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:231: return IsConnected() ?
properties->connection_rssi.value() : kUnknownPower;
Shouldn't this be "is a connection monitor running" ?

[Tim Song, 2014-05-09 19:01:35]

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:232: :
properties->inquiry_rssi.value();
On 2014/05/09 18:22:07, keybuk wrote:
> On 2014/05/09 02:55:29, Tim Song wrote:
> 
> > The GetRSSI() function in the BluetoothDevice interface says that it should
> > return the inquiry RSSI if not connected and connection RSSI if connected. I
> > think we should split this in a separate CL as it touches the extension API.
> 
> That separate CL will be much easier if you only store connection RSSI in this
> function (matching the mac implementation)

Done.

https://codereview.chromium.org/273953002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:564: StartConnectionMonitor();
On 2014/05/09 18:22:07, keybuk wrote:
> On 2014/05/09 02:55:29, Tim Song wrote:
> > In that case, should we add an API in chrome.bluetooth, and I guess the
> > BluetoothDevice interface, to enable connection monitoring?
> > 
> > How would this work for other platforms, like Mac, which doesn't have system
> > supported polling? Should we do the polling in Chrome to make the interface
> > consistent?
> 
> Yes.

Okay, I'll add this API function in the other CL.

https://codereview.chromium.org/273953002/diff/40001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/40001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:231: return IsConnected() ?
properties->connection_rssi.value() : kUnknownPower;
On 2014/05/09 18:27:06, keybuk wrote:
> Shouldn't this be "is a connection monitor running" ?

Done. I added a boolean member variable tracking if the connection monitor has
been turned on.

[keybuk, 2014-05-09 19:06:38]

https://codereview.chromium.org/273953002/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:247: return IsConnected() ?
properties->connection_tx_power.value()
shouldn't this also be connection monitor?

https://codereview.chromium.org/273953002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:257: return IsConnected() ?
properties->connection_tx_power_max.value()
ditto

[Tim Song, 2014-05-09 20:08:01]

https://codereview.chromium.org/273953002/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/273953002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:247: return IsConnected() ?
properties->connection_tx_power.value()
On 2014/05/09 19:06:38, keybuk wrote:
> shouldn't this also be connection monitor?

Done.

https://codereview.chromium.org/273953002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_device_chromeos.cc:257: return IsConnected() ?
properties->connection_tx_power_max.value()
On 2014/05/09 19:06:38, keybuk wrote:
> ditto

The maximum TX power doesn't need to be tracked, and it is cached when the
connection is first opened.

[keybuk, 2014-05-09 20:08:44]

lgtm

[Tim Song, 2014-05-09 22:19:23]

Scott, I updated this CL to start the connection monitor for connections opened
with chrome.bluetooth.connect.

Please take another look.

[keybuk, 2014-05-09 22:37:20]

https://codereview.chromium.org/273953002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/bluetooth/bluetooth_api.cc (right):

https://codereview.chromium.org/273953002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth/bluetooth_api.cc:461:
base::Bind(&BluetoothConnectFunction::OnSuccessCallback, this, device),
You can't pass device pointers around like this, it's only yours until your
method returns, then you can't have it anymore

Pass the address and call GetDevice again

https://codereview.chromium.org/273953002/diff/100001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/273953002/diff/100001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:417: 
NAK... we've been burned by these Start/Stop APIs before

Model your API on the Discovery Session API - have StartConnectionMonitor return
a scoped or ref-counted object that has a method on it to stop it again

[Tim Song, 2014-05-09 23:10:42]

https://codereview.chromium.org/273953002/diff/100001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/273953002/diff/100001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:417: 
On 2014/05/09 22:37:21, keybuk wrote:
> NAK... we've been burned by these Start/Stop APIs before
> 
> Model your API on the Discovery Session API - have StartConnectionMonitor
return
> a scoped or ref-counted object that has a method on it to stop it again

Can I just have StartConnectionMonitor for now (no StopConnectionMonitor) and
make a session based approach with the actual API?

[keybuk, 2014-05-10 02:15:06]

lgtm

[Tim Song, 2014-05-10 04:32:24]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2014-05-10 04:32:25]

The CQ bit was unchecked by tengs@chromium.org

[Tim Song, 2014-05-10 04:32:40]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-05-10 04:35:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tengs@chromium.org/273953002/170001

[commit-bot: I haz the power, 2014-05-10 06:18:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-10 07:14:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 07:14:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Tim Song, 2014-05-10 07:32:39]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-05-10 07:33:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tengs@chromium.org/273953002/140001

[commit-bot: I haz the power, 2014-05-10 09:02:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-10 10:00:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 10:00:54]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Tim Song, 2014-05-10 22:23:36]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-05-10 22:24:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tengs@chromium.org/273953002/190001

[commit-bot: I haz the power, 2014-05-11 01:26:22]

Change committed as 269676