bluetooth: Implement TxPower and RSSI of BluetoothAdvertisementData

bluetooth: Implement TxPower and RSSI of BluetoothAdvertisementData

Implements the TxPower member and RSSI member of BluetoothAdvertisementData:

https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothadvertisingdata-txpower

BUG=542756
Committed: https://crrev.com/04751f481d8c83e2f1cc5630100477e51d2e2946
Cr-Commit-Position: refs/heads/master@{#357993}

[ortuno, 2015-10-27 19:50:41]

ortuno@chromium.org changed reviewers:
+ scheib@chromium.org

[ortuno, 2015-10-27 19:50:42]

scheib: PTAL

[ortuno, 2015-10-27 20:34:21]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 20:36:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427653003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427653003/20001

[commit-bot: I haz the power, 2015-10-27 20:47:45]

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

[commit-bot: I haz the power, 2015-10-27 20:47:45]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ortuno, 2015-10-27 21:31:42]

The CQ bit was checked by ortuno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 21:35:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427653003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427653003/40001

[commit-bot: I haz the power, 2015-10-27 22:27:58]

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

[commit-bot: I haz the power, 2015-10-27 22:27:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scheib, 2015-10-29 17:37:26]

https://codereview.chromium.org/1427653003/diff/60001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1427653003/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:280:
ON_CALL(*device, GetInquiryRSSI()).WillByDefault(Return(power));
We should return different numbers, validating the parameters are passed
correctly through the whole stack. Perhaps just parse 2 values to return, or
have one be always computed from the other?

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/advertising-data.html (right):

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/advertising-data.html:53: }, 'Power too
low must result in nulled members.');
Power too high?

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothAdvertisingData.cpp
(right):

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothAdvertisingData.cpp:12:
const int kNegativeUnknownPower = INT8_MIN;
kNegativeUnknownPower seems to be a valid minimum value. So perhaps rename to
kMinimumPower

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h
(right):

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h:49:
const int txPower;
Document the expected bounds of this value, and that 127 is considered an
unknown power.

[ortuno, 2015-10-29 20:21:42]

https://codereview.chromium.org/1427653003/diff/60001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc
(right):

https://codereview.chromium.org/1427653003/diff/60001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:280:
ON_CALL(*device, GetInquiryRSSI()).WillByDefault(Return(power));
On 2015/10/29 17:37:25, scheib wrote:
> We should return different numbers, validating the parameters are passed
> correctly through the whole stack. Perhaps just parse 2 values to return, or
> have one be always computed from the other?

Done.

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/advertising-data.html (right):

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/advertising-data.html:53: }, 'Power too
low must result in nulled members.');
On 2015/10/29 17:37:25, scheib wrote:
> Power too high?

Done.

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothAdvertisingData.cpp
(right):

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothAdvertisingData.cpp:12:
const int kNegativeUnknownPower = INT8_MIN;
On 2015/10/29 17:37:26, scheib wrote:
> kNegativeUnknownPower seems to be a valid minimum value. So perhaps rename to
> kMinimumPower

Done.

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h
(right):

https://codereview.chromium.org/1427653003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h:49:
const int txPower;
On 2015/10/29 17:37:26, scheib wrote:
> Document the expected bounds of this value, and that 127 is considered an
> unknown power.

Done.

[scheib, 2015-10-29 20:59:13]

LGTM with comment fix

https://codereview.chromium.org/1427653003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h
(right):

https://codereview.chromium.org/1427653003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothDevice.h:51: //
since the underlying platform uses int16 rather than int8 blink turns
I don't think the int16 reference makes much sense here. Just document what
valid input values are and what the special number is that represents an invalid
value. Possibly use an int8_t to reduce the comment size (as then there is only
1 special value range, not 3).

[ortuno, 2015-10-29 23:54:41]

Patchset #7 (id:120001) has been deleted

[ortuno, 2015-10-29 23:59:35]

scheib: PTAL. Changes
 - Moved enforcing the range to content/common/BluetoothDevice
 - Renamed PowerAdapter to InternalPowerAdapter and added PowerAdapter for more
reusable tests.
 - Sending int8_t over IPC.

[scheib, 2015-10-30 01:05:24]

https://codereview.chromium.org/1427653003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/advertising-data.html (right):

https://codereview.chromium.org/1427653003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/advertising-data.html:10: // Tests that
we are handling values returned from the underlying platform
InternalPowerAdapter a poor name, name them for setting power values and setting
presence. 

Avoid anything Chromium only, make tests that cover our needs and that can be
implemented by others.

CSSv6 says valid values are -127 to +127 dBm, so +127 should work.

[ortuno, 2015-11-04 21:22:09]

Thanks!

https://codereview.chromium.org/1427653003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/bluetooth/advertising-data.html (right):

https://codereview.chromium.org/1427653003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/bluetooth/advertising-data.html:10: // Tests that
we are handling values returned from the underlying platform
On 2015/10/30 at 01:05:23, scheib wrote:
> InternalPowerAdapter a poor name, name them for setting power values and
setting presence. 
> 
> Avoid anything Chromium only, make tests that cover our needs and that can be
implemented by others.
> 
> CSSv6 says valid values are -127 to +127 dBm, so +127 should work.

Adapter renamed. Added TODO for the Tx Power value.

[ortuno, 2015-11-04 21:38:47]

ortuno@chromium.org changed reviewers:
+ palmer@chromium.org

[ortuno, 2015-11-04 21:38:48]

palmer: PTAL at content/common/bluetooth/bluetooth_message.h

[palmer, 2015-11-04 21:57:20]

Just some minor "making sure I understand" questions.

https://codereview.chromium.org/1427653003/diff/160001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1427653003/diff/160001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device.h:24: int8_t rssi,
Elsewhere in the code (in other code, e.g. device/bluetooth/bluetooth_device.h)
I see rssi declared as int and as int16. What is it really?

https://codereview.chromium.org/1427653003/diff/160001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device.h:39: static int8_t
ValidatePower(int16_t power);
So sometimes you have to take in larger ints, but really they are int8s?

https://codereview.chromium.org/1427653003/diff/160001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1427653003/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:125:
GetPowerValueAdapter(int tx_power, int rssi);
Not int8 or int16?

[ortuno, 2015-11-04 22:45:58]

thanks!

https://codereview.chromium.org/1427653003/diff/160001/content/common/bluetoo...
File content/common/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/1427653003/diff/160001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device.h:24: int8_t rssi,
On 2015/11/04 21:57:19, palmer wrote:
> Elsewhere in the code (in other code, e.g.
device/bluetooth/bluetooth_device.h)
> I see rssi declared as int and as int16. What is it really?

According to the Bluetooth Spec RSSI and Tx Power are in fact int8 values. The
low level controller, BlueZ, stores these as int8 [1], but sends a int16 because
DBUS doesn't support int8 [2]. So in theory we should never see a value greater
than 127 or smaller than -128. We know that these are int8 values but just in
case we have a function to make sure the values passed by bluez are within the
int8 limit.

[1] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/device.c#n265
[2] http://dbus.freedesktop.org/doc/api/html/group__DBusProtocol.html

https://codereview.chromium.org/1427653003/diff/160001/content/common/bluetoo...
content/common/bluetooth/bluetooth_device.h:39: static int8_t
ValidatePower(int16_t power);
On 2015/11/04 21:57:19, palmer wrote:
> So sometimes you have to take in larger ints, but really they are int8s?

As mentioned above. This is just in case. In theory we should never receive
values outise the int8 range.

https://codereview.chromium.org/1427653003/diff/160001/content/shell/browser/...
File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h
(right):

https://codereview.chromium.org/1427653003/diff/160001/content/shell/browser/...
content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:125:
GetPowerValueAdapter(int tx_power, int rssi);
On 2015/11/04 21:57:19, palmer wrote:
> Not int8 or int16?

Done.

[palmer, 2015-11-05 01:27:05]

Thanks for the clues. LGTM.

[ortuno, 2015-11-05 01:32:15]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-11-05 01:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/1427653003/#ps180001
(title: "Change PowerValueAdapter to int8_t")

[commit-bot: I haz the power, 2015-11-05 01:34:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427653003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427653003/180001

[commit-bot: I haz the power, 2015-11-05 03:45:42]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-11-05 03:47:03]

Patchset 9 (id:??) landed as
https://crrev.com/04751f481d8c83e2f1cc5630100477e51d2e2946
Cr-Commit-Position: refs/heads/master@{#357993}