Bluetooth: mac: add connected BLE devices at startup

Bluetooth: mac: add connected BLE devices at startup

This fixes issue where BLE devices are treated as classic ones

BUG=630581

[jracle (use Gerrit), 2016-08-12 15:13:50]

Description was changed from

==========
Bluetooth: mac: add connected BLE devices at startup

This fixes issue where BLE devices are treated as classic ones

BUG=630581
==========

to

==========
Bluetooth: mac: add connected BLE devices at startup

This fixes issue where BLE devices are treated as classic ones

BUG=630581
==========

[jracle (use Gerrit), 2016-08-12 15:13:50]

jracle@logitech.com changed reviewers:
+ fbeaufort@google.com, jlebel@chromium.org

[ortuno, 2016-08-15 23:58:08]

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

[ortuno, 2016-08-15 23:58:09]

Thanks jracle. We probably want to add tests as well. The tests for the adapter
should be in bluetooth_adapter_unittests.cc. Seems like we would need to change
CreateAdapterForTest to take a MockCentralManager. Then you can write a similar
function to InitWithFakeAdapter, say InitWithFakeAdapterWithConnectedDevice(),
that has a MockCentralManager that returns a mock device for
retrieveConnectedPeripheralsWithService.

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:34: @interface CBCentralManager
(PartialAvailability)
This doesn't seem to be a pattern we follow through the code base?

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:542: // IOBluetoothDevice instance is
a Bluetooth Classic device if it has a PNP
Could you add a link to some documentation that states that this is always true
for Classic devices and false for Low Energy Devices?

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:551: // Look for Device Information
Service. BLE devices should advertise it.
nit: Well if it's connected it's not really advertising anything :)

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:558:
retrieveConnectedPeripheralsWithServices:@[ device_information_uuid ]];
Just pass nil. There is no requirement that a device has that service and we
want to add all devices.

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:570:
FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
We probably want to also start a service discovery for this device.

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_device_mac.h:36:
BluetoothLowEnergyDeviceMac(BluetoothAdapterMac* adapter,
I'm refactoring this function in another CL. Hopefully we won't need the default
arguments. Otherwise maybe pass in a const int* and make it default to nullptr.
"-1" is a perfectly valid rssi.

[jracle (use Gerrit), 2016-08-16 19:47:19]

On 2016/08/15 23:58:09, ortuno wrote:
> Thanks jracle. We probably want to add tests as well. The tests for the
adapter
> should be in bluetooth_adapter_unittests.cc. Seems like we would need to
change
> CreateAdapterForTest to take a MockCentralManager. Then you can write a
similar
> function to InitWithFakeAdapter, say InitWithFakeAdapterWithConnectedDevice(),
> that has a MockCentralManager that returns a mock device for
> retrieveConnectedPeripheralsWithService.
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> File device/bluetooth/bluetooth_adapter_mac.mm (right):
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter_mac.mm:34: @interface CBCentralManager
> (PartialAvailability)
> This doesn't seem to be a pattern we follow through the code base?
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter_mac.mm:542: // IOBluetoothDevice instance
is
> a Bluetooth Classic device if it has a PNP
> Could you add a link to some documentation that states that this is always
true
> for Classic devices and false for Low Energy Devices?
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter_mac.mm:551: // Look for Device Information
> Service. BLE devices should advertise it.
> nit: Well if it's connected it's not really advertising anything :)
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter_mac.mm:558:
> retrieveConnectedPeripheralsWithServices:@[ device_information_uuid ]];
> Just pass nil. There is no requirement that a device has that service and we
> want to add all devices.
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter_mac.mm:570:
> FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
> We probably want to also start a service discovery for this device.
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> File device/bluetooth/bluetooth_low_energy_device_mac.h (right):
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_low_energy_device_mac.h:36:
> BluetoothLowEnergyDeviceMac(BluetoothAdapterMac* adapter,
> I'm refactoring this function in another CL. Hopefully we won't need the
default
> arguments. Otherwise maybe pass in a const int* and make it default to
nullptr.
> "-1" is a perfectly valid rssi.

Thanks Gio!

I wasn't expecting to get LGTM cause indeed code misses testing plus a couple of
round trips (see
https://bugs.chromium.org/p/chromium/issues/detail?id=630581#c22).
That's great you could start reviewing and I'll reply to your comments.

Cheers,
Julien

[jracle (use Gerrit), 2016-08-16 20:14:18]

Thanks ortuno for review, here are my comments.

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:34: @interface CBCentralManager
(PartialAvailability)
On 2016/08/15 23:58:09, ortuno wrote:
> This doesn't seem to be a pattern we follow through the code base?

Actually, I think I've got it from a landed CL, but I'm not sure (that's
holidays downside!)..
Anyway it needs at least a couple of comments in code here.. 
Also if I can be pointed to the correct way to handle that'd be awesome!

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:542: // IOBluetoothDevice instance is
a Bluetooth Classic device if it has a PNP
On 2016/08/15 23:58:09, ortuno wrote:
> Could you add a link to some documentation that states that this is always
true
> for Classic devices and false for Low Energy Devices?

I need to cross-check with Logitech guys who pointed me to this logic.
Just did a quick test with a couple of devices.
I'll get back to commenting it when I have such data.

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:551: // Look for Device Information
Service. BLE devices should advertise it.
On 2016/08/15 23:58:09, ortuno wrote:
> nit: Well if it's connected it's not really advertising anything :)

Indeed.. does 'implement' look better? ("Most BLE devices should implement it")

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:558:
retrieveConnectedPeripheralsWithServices:@[ device_information_uuid ]];
On 2016/08/15 23:58:09, ortuno wrote:
> Just pass nil. There is no requirement that a device has that service and we
> want to add all devices.

Agree with you. Documentation says passing nil will return all connected
peripherals, but in practice this doesn't seem to work (at least I gonna
re-check). Hence I'm looking for DIS, which I also think is not compulsory (I'll
cross-check with the specialists here). In practice, most LE devices should
implement it.

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:570:
FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_,
On 2016/08/15 23:58:09, ortuno wrote:
> We probably want to also start a service discovery for this device.

I don't do it here, cause user will have to connect peripheral from API
(chrome.bluetoothLowEnergy or web-bluetooth), and services will be enumerated
upon connection success.

Connected peripherals returned here could have been connected by other apps or
by system. This list is actually a list of physically connected peripherals.
User App will have to check whether LE peripheral is (logically) connected (see
BluetoothLowEnergyDeviceMac::IsGattConnected()) before eventually connecting it
(see BluetoothAdapterMac::CreateGattConnection()). Upon connection, service
discovery will be done. See
BluetoothAdapterMac::DidConnectPeripheral(CBPeripheral* peripheral).

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_low_energy_device_mac.h:36:
BluetoothLowEnergyDeviceMac(BluetoothAdapterMac* adapter,
On 2016/08/15 23:58:09, ortuno wrote:
> I'm refactoring this function in another CL. Hopefully we won't need the
default
> arguments. Otherwise maybe pass in a const int* and make it default to
nullptr.
> "-1" is a perfectly valid rssi.

Great. Sure will modify int to const int*, good point!

[jracle (use Gerrit), 2016-08-17 21:21:16]

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:542: // IOBluetoothDevice instance is
a Bluetooth Classic device if it has a PNP
On 2016/08/16 20:14:17, jracle - OOO Aug. 15 - Aug. 26 wrote:
> On 2016/08/15 23:58:09, ortuno wrote:
> > Could you add a link to some documentation that states that this is always
> true
> > for Classic devices and false for Low Energy Devices?
> 
> I need to cross-check with Logitech guys who pointed me to this logic.
> Just did a quick test with a couple of devices.
> I'll get back to commenting it when I have such data.

While checking for official documentation, here is a link
(https://epxx.co/artigos/bluetooth_gatt.html) that explains what I thought : SDP
is required only for BR-EDR and dual-mode devices. It is not needed for LE
devices, cause ATT protocol replaces it.

Yet, another solution would be to match information from IOBluetoothDevice
(IOKit) and CBPeripheral (CoreBluetooth). This requires redesigning codebase +
matching those entities.

[jlebel, 2016-08-31 12:03:28]

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:34: @interface CBCentralManager
(PartialAvailability)
On 2016/08/16 20:14:17, jracle wrote:
> On 2016/08/15 23:58:09, ortuno wrote:
> > This doesn't seem to be a pattern we follow through the code base?
> 
> Actually, I think I've got it from a landed CL, but I'm not sure (that's
> holidays downside!)..
> Anyway it needs at least a couple of comments in code here.. 
> Also if I can be pointed to the correct way to handle that'd be awesome!

I think you can just #import <CoreBluetooth/CoreBluetooth.h> or did I miss
something?

[jlebel, 2016-08-31 15:03:54]

Can you rebase your patch? Some code has been changed since you did it.

Thanks,

[jracle (use Gerrit), 2016-08-31 15:09:13]

On 2016/08/31 15:03:54, jlebel wrote:
> Can you rebase your patch? Some code has been changed since you did it.
> 
> Thanks,

Hi Jérôme, sure will do. Thanks.

[jlebel, 2016-08-31 17:03:53]

On 2016/08/31 15:09:13, jracle wrote:
> On 2016/08/31 15:03:54, jlebel wrote:
> > Can you rebase your patch? Some code has been changed since you did it.
> > 
> > Thanks,
> 
> Hi Jérôme, sure will do. Thanks.

I understand the issue about the partial method (I've just tried to compile your
patch). I will provide you the right way to do it tomorrow.

[jracle (use Gerrit), 2016-08-31 17:21:09]

On 2016/08/31 17:03:53, jlebel wrote:
> On 2016/08/31 15:09:13, jracle wrote:
> > On 2016/08/31 15:03:54, jlebel wrote:
> > > Can you rebase your patch? Some code has been changed since you did it.
> > > 
> > > Thanks,
> > 
> > Hi Jérôme, sure will do. Thanks.
> 
> I understand the issue about the partial method (I've just tried to compile
your
> patch). I will provide you the right way to do it tomorrow.

Indeed that's a partial method.

I had trouble with ninja and compiler today, so I'll try rebase tomorrow as
well.
I keep you in touch, thanks!

[jracle (use Gerrit), 2016-08-31 17:21:23]

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter_mac.mm:34: @interface CBCentralManager
(PartialAvailability)
On 2016/08/31 12:03:28, jlebel wrote:
> On 2016/08/16 20:14:17, jracle wrote:
> > On 2016/08/15 23:58:09, ortuno wrote:
> > > This doesn't seem to be a pattern we follow through the code base?
> > 
> > Actually, I think I've got it from a landed CL, but I'm not sure (that's
> > holidays downside!)..
> > Anyway it needs at least a couple of comments in code here.. 
> > Also if I can be pointed to the correct way to handle that'd be awesome!
> 
> I think you can just #import <CoreBluetooth/CoreBluetooth.h> or did I miss
> something?

Indeed, as you state in other comment, that's a partial method.

Sorry for the late reply.

[jlebel, 2016-08-31 23:08:42]

On 2016/08/31 17:21:23, jracle wrote:
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> File device/bluetooth/bluetooth_adapter_mac.mm (right):
> 
>
https://codereview.chromium.org/2242833002/diff/1/device/bluetooth/bluetooth_...
> device/bluetooth/bluetooth_adapter_mac.mm:34: @interface CBCentralManager
> (PartialAvailability)
> On 2016/08/31 12:03:28, jlebel wrote:
> > On 2016/08/16 20:14:17, jracle wrote:
> > > On 2016/08/15 23:58:09, ortuno wrote:
> > > > This doesn't seem to be a pattern we follow through the code base?
> > > 
> > > Actually, I think I've got it from a landed CL, but I'm not sure (that's
> > > holidays downside!)..
> > > Anyway it needs at least a couple of comments in code here.. 
> > > Also if I can be pointed to the correct way to handle that'd be awesome!
> > 
> > I think you can just #import <CoreBluetooth/CoreBluetooth.h> or did I miss
> > something?
> 
> Indeed, as you state in other comment, that's a partial method.
> 
> Sorry for the late reply.

In mock_bluetooth_cbperipheral_mac.mm:149, I've used:
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wpartial-availability"
  [_delegate peripheral:self.peripheral didModifyServices:invalidatedServices];
#pragma clang diagnostic pop

https://cs.chromium.org/chromium/src/device/bluetooth/test/mock_bluetooth_cbp...

[jracle (use Gerrit), 2016-09-02 14:23:10]

I rebased the CL wrt latest changes.
Need to work on test and logic.
See my comments regarding timed-out devices.

Thanks guys!

https://codereview.chromium.org/2242833002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:571: IOBluetoothSDPServiceRecord*
pnp_information =
Still need to confirm this logic.

https://codereview.chromium.org/2242833002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:589: #pragma clang diagnostic ignored
"-Wpartial-availability"
Passing nil in following line causes -Wnonnull to show-up at compile-time.
Tryed to ignore it : crashed Chrome at run-time.
Seems that [retrieveConnectedPeripheralsWithServices:nil] crashed (didn't
debug).

https://codereview.chromium.org/2242833002/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2242833002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_low_energy_device_mac.mm:40: UpdateTimestamp();
I did this hack here, but I think we could instead try not removing BLE devices
in adapter if possible.
Maybe we could filter-out only classic devices and update
BluetoothAdapter::RemoveTimedOutDevices() instead.

[jracle (use Gerrit), 2016-09-06 12:15:30]

Added basic unit test (on Mac OS X so far).

[jracle (use Gerrit), 2016-09-07 07:38:32]

The CQ bit was checked by jracle@logitech.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 07:38:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 08:17:00]

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

[commit-bot: I haz the power, 2016-09-07 08:17:01]

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

[jracle (use Gerrit), 2016-09-07 09:33:49]

The CQ bit was checked by jracle@logitech.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 09:34:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 11:18:54]

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

[commit-bot: I haz the power, 2016-09-07 11:18:55]

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

[jlebel, 2017-03-06 21:16:28]

jlebel@chromium.org changed reviewers:
- jlebel@chromium.org