From 154da849a1f7106c4efb3b514d88eac45a39afaa Mon Sep 17 00:00:00 2001 From: ortuno Date: Mon, 26 Sep 2016 18:27:23 -0700 Subject: [PATCH] bluetooth: mac: Improve classic device discovery and update There are a couple of problems with the current implementation of classic device discovery: 1. IOBluetoothDeviceInquiry's cache is never cleaned which means that only the first discovery session will find the device. (Since the device is still in the cache future discovery session will not be notified of the device being found.) We could send notifications for all devices in the cache but then we could be notifying of devices that are no longer around. 2. IOBluetoothDevice getLastInquiryUpdate returns nil even for devices that have just been discovered during an inquiry procedure. This causes us to immediately remove a device that we just discovered. 3. IOBluetoothDevice pairedDevices sometimes returns devices that are not paired which causes us to add a non paired device. Solutions: 1. Clean cache every time StartDiscovery is called. This means new sessions will be notified of previously seen. Which allows us to implement the solution for 2. 2. Now that we notify whenever we see a device, we can update our cross platform last_update_time_. Then when we are removing outdated devices we can check last_update_time_ to see if the device should be removed. 3. Check [IOBluetoothDevice isPaired] before adding a device. Also changes some VLOG(1)s to VLOG(3) since they were spamming the logs. BUG=618650,638715 Review-Url: https://codereview.chromium.org/2282763004 Cr-Commit-Position: refs/heads/master@{#421069} --- device/bluetooth/bluetooth_adapter.cc | 8 +++++--- device/bluetooth/bluetooth_adapter_mac.mm | 17 ++++++++++++++--- device/bluetooth/bluetooth_classic_device_mac.h | 2 +- .../bluetooth/bluetooth_classic_device_mac.mm | 9 ++++++--- device/bluetooth/bluetooth_device.cc | 4 ++++ device/bluetooth/bluetooth_device.h | 11 ++++++----- .../bluetooth_discovery_manager_mac.mm | 6 +++++- ...luetooth_low_energy_discovery_manager_mac.mm | 2 +- 8 files changed, 42 insertions(+), 17 deletions(-) diff --git a/device/bluetooth/bluetooth_adapter.cc b/device/bluetooth/bluetooth_adapter.cc index 878d3b4c978820..1701b808345407 100644 --- a/device/bluetooth/bluetooth_adapter.cc +++ b/device/bluetooth/bluetooth_adapter.cc @@ -122,13 +122,13 @@ const BluetoothDevice* BluetoothAdapter::GetDevice( std::string canonicalized_address = BluetoothDevice::CanonicalizeAddress(address); if (canonicalized_address.empty()) - return NULL; + return nullptr; DevicesMap::const_iterator iter = devices_.find(canonicalized_address); if (iter != devices_.end()) return iter->second; - return NULL; + return nullptr; } void BluetoothAdapter::AddPairingDelegate( @@ -383,7 +383,7 @@ void BluetoothAdapter::RemoveTimedOutDevices() { bool device_expired = (base::Time::NowFromSystemTime() - last_update_time) > timeoutSec; - VLOG(1) << "device: " << device->GetAddress() + VLOG(3) << "device: " << device->GetAddress() << ", last_update: " << last_update_time << ", exp: " << device_expired; @@ -391,6 +391,8 @@ void BluetoothAdapter::RemoveTimedOutDevices() { ++it; continue; } + + VLOG(1) << "Removing device: " << device->GetAddress(); DevicesMap::iterator next = it; next++; std::unique_ptr removed_device = diff --git a/device/bluetooth/bluetooth_adapter_mac.mm b/device/bluetooth/bluetooth_adapter_mac.mm index c485f99bc40c7e..3bc426e6a45440 100644 --- a/device/bluetooth/bluetooth_adapter_mac.mm +++ b/device/bluetooth/bluetooth_adapter_mac.mm @@ -469,12 +469,19 @@ std::string device_address = BluetoothClassicDeviceMac::GetDeviceAddress(device); + BluetoothDevice* device_classic = GetDevice(device_address); + // Only notify observers once per device. - if (devices_.count(device_address)) + if (device_classic != nullptr) { + VLOG(3) << "Updating classic device: " << device_classic->GetAddress(); + device_classic->UpdateTimestamp(); return; + } - BluetoothDevice* device_classic = new BluetoothClassicDeviceMac(this, device); + device_classic = new BluetoothClassicDeviceMac(this, device); devices_.set(device_address, base::WrapUnique(device_classic)); + VLOG(1) << "Adding new classic device: " << device_classic->GetAddress(); + FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_, DeviceAdded(this, device_classic)); } @@ -567,7 +574,11 @@ void BluetoothAdapterMac::AddPairedDevices() { // Add any new paired devices. for (IOBluetoothDevice* device in [IOBluetoothDevice pairedDevices]) { - ClassicDeviceAdded(device); + // pairedDevices sometimes includes unknown devices that are not paired. + // Radar issue with id 2282763004 has been filed about it. + if ([device isPaired]) { + ClassicDeviceAdded(device); + } } } diff --git a/device/bluetooth/bluetooth_classic_device_mac.h b/device/bluetooth/bluetooth_classic_device_mac.h index dbbe2a5204b88a..8a9069ef15b474 100644 --- a/device/bluetooth/bluetooth_classic_device_mac.h +++ b/device/bluetooth/bluetooth_classic_device_mac.h @@ -73,7 +73,7 @@ class BluetoothClassicDeviceMac : public BluetoothDeviceMac { const GattConnectionCallback& callback, const ConnectErrorCallback& error_callback) override; - base::Time GetLastUpdateTime() const; + base::Time GetLastUpdateTime() const override; // Returns the Bluetooth address for the |device|. The returned address has a // normalized format (see below). diff --git a/device/bluetooth/bluetooth_classic_device_mac.mm b/device/bluetooth/bluetooth_classic_device_mac.mm index cc2fafb6e2632a..da2866715b52fe 100644 --- a/device/bluetooth/bluetooth_classic_device_mac.mm +++ b/device/bluetooth/bluetooth_classic_device_mac.mm @@ -62,7 +62,9 @@ BluetoothUUID ExtractUuid(IOBluetoothSDPDataElement* service_class_data) { BluetoothClassicDeviceMac::BluetoothClassicDeviceMac( BluetoothAdapterMac* adapter, IOBluetoothDevice* device) - : BluetoothDeviceMac(adapter), device_([device retain]) {} + : BluetoothDeviceMac(adapter), device_([device retain]) { + UpdateTimestamp(); +} BluetoothClassicDeviceMac::~BluetoothClassicDeviceMac() { } @@ -255,8 +257,9 @@ BluetoothUUID ExtractUuid(IOBluetoothSDPDataElement* service_class_data) { } base::Time BluetoothClassicDeviceMac::GetLastUpdateTime() const { - return base::Time::FromDoubleT( - [[device_ getLastInquiryUpdate] timeIntervalSince1970]); + // getLastInquiryUpdate returns nil unpredictably so just use the + // cross platform implementation of last update time. + return last_update_time_; } int BluetoothClassicDeviceMac::GetHostTransmitPower( diff --git a/device/bluetooth/bluetooth_device.cc b/device/bluetooth/bluetooth_device.cc index bdb764dae82b23..6060816375e588 100644 --- a/device/bluetooth/bluetooth_device.cc +++ b/device/bluetooth/bluetooth_device.cc @@ -479,6 +479,10 @@ void BluetoothDevice::UpdateTimestamp() { last_update_time_ = base::Time::NowFromSystemTime(); } +base::Time BluetoothDevice::GetLastUpdateTime() const { + return last_update_time_; +} + // static int8_t BluetoothDevice::ClampPower(int power) { if (power < INT8_MIN) { diff --git a/device/bluetooth/bluetooth_device.h b/device/bluetooth/bluetooth_device.h index 23565c9b7a8049..b2dfdedce2c004 100644 --- a/device/bluetooth/bluetooth_device.h +++ b/device/bluetooth/bluetooth_device.h @@ -517,8 +517,12 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice { // empty string. static std::string CanonicalizeAddress(const std::string& address); - // Return the timestamp for when this device was last seen. - base::Time GetLastUpdateTime() const { return last_update_time_; } + // Update the last time this device was seen. + void UpdateTimestamp(); + + // Returns the time of the last call to UpdateTimestamp(), or base::Time() if + // it hasn't been called yet. + virtual base::Time GetLastUpdateTime() const; // Called by BluetoothAdapter when a new Advertisement is seen for this // device. This replaces previously seen Advertisement Data. @@ -581,9 +585,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice { BluetoothDevice(BluetoothAdapter* adapter); - // Update the last time this device was seen. - void UpdateTimestamp(); - // Implements platform specific operations to initiate a GATT connection. // Subclasses must also call DidConnectGatt, DidFailToConnectGatt, or // DidDisconnectGatt immediately or asynchronously as the connection state diff --git a/device/bluetooth/bluetooth_discovery_manager_mac.mm b/device/bluetooth/bluetooth_discovery_manager_mac.mm index a2a024d777b29c..ebeb01dc0904dc 100644 --- a/device/bluetooth/bluetooth_discovery_manager_mac.mm +++ b/device/bluetooth/bluetooth_discovery_manager_mac.mm @@ -31,7 +31,7 @@ - (id)initWithManager:(device::BluetoothDiscoveryManagerMacClassic*)manager; namespace device { -// ementation of BluetoothDiscoveryManagerMac for Bluetooth classic device +// Implementation of BluetoothDiscoveryManagerMac for Bluetooth classic device // discovery, using the IOBluetooth framework. class BluetoothDiscoveryManagerMacClassic : public BluetoothDiscoveryManagerMac { @@ -58,6 +58,10 @@ bool StartDiscovery() override { DVLOG(1) << "Discovery requested"; should_do_discovery_ = true; + // Clean the cache so that new discovery sessions find previously + // discovered devices as well. + [inquiry_ clearFoundDevices]; + if (inquiry_running_) { DVLOG(1) << "Device inquiry already running"; return true; diff --git a/device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm b/device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm index eeb1ace3beb199..f4f13ee9922fed 100644 --- a/device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm +++ b/device/bluetooth/bluetooth_low_energy_discovery_manager_mac.mm @@ -93,7 +93,7 @@ CBPeripheral* peripheral, NSDictionary* advertisementData, int rssi) { - VLOG(1) << "DiscoveredPeripheral"; + VLOG(3) << "DiscoveredPeripheral"; observer_->LowEnergyDeviceUpdated(peripheral, advertisementData, rssi); }