Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2282763004:
bluetooth: mac: Improve classic device discovery and update (Closed)
Descriptionbluetooth: 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
Committed: https://crrev.com/154da849a1f7106c4efb3b514d88eac45a39afaa
Cr-Commit-Position: refs/heads/master@{#421069}
Patch Set 1 #Patch Set 2 : Clean up #
Total comments: 2
Patch Set 3 : Add comment and fix comment #
Total comments: 2
Patch Set 4 : Store a TimeDelta instead of int #
Total comments: 14
Patch Set 5 : Override getlastupdate and make constexpr #
Total comments: 4
Patch Set 6 : Remove timer #Patch Set 7 : Clean up #Patch Set 8 : Clean up #
Total comments: 2
Patch Set 9 : Address moar comments #
Messages
Total messages: 38 (22 generated)
Description was changed from
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is seen. Which allows us to implement
the solution for 2.
2. Now that we notifie whenever we see the 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.
BUG=618650,638715
==========
to
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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.
BUG=618650,638715
==========
Description was changed from
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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.
BUG=618650,638715
==========
to
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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
==========
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL. This is my attempt to fix device discovery. I looked into writing tests but there is not a single test for this part of the code D: The only way this affects Web Bluetooth is because we would show devices that people can't interact with (even with this fix they would only be able to get the basic device information, we still don't support GATT over BT Classic). Do you think it's worth writing tests for this? We could just disable the classic part for Web Bluetooth and leave the bugs for Chrome Apps owners.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
scheib@chromium.org changed reviewers: + scheib@chromium.org
LGTM https://codereview.chromium.org/2282763004/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:37: const int kCleanCacheTimer = 1; Comment explaining why we're using 1. And include the time unit (second).
Description was changed from
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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
==========
to
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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
==========
cc jlebel
Description was changed from
==========
bluetooth: 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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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
==========
to
==========
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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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
==========
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2282763004/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:37: const int kCleanCacheTimer = 1; On 2016/08/29 at 02:42:32, scheib wrote: > Comment explaining why we're using 1. And include the time unit (second). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2282763004/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:39: const int kCleanCacheTimer = 1; I think you can use a global constexpr TimeDelta now.
https://codereview.chromium.org/2282763004/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:39: const int kCleanCacheTimer = 1; On 2016/09/13 at 20:58:05, Jeffrey Yasskin wrote: > I think you can use a global constexpr TimeDelta now. Argh, I always forget that we should use TimeDelta instead of ints. Done.
https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:577: if ([device isPaired]) { Could you comment why this is needed, with any radar or other bugs that have been filed about it? https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_classic_device_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_classic_device_mac.mm:260: // Even when a device is discovered during an inquiry procedure this returns "Even when a device is discovered during an inquiry procedure" makes it sound like the function always returns nil. When does it actually return non-nil? Maybe "If the device was just discovered, this returns nil, even if the discovery was during an inquiry procedure."? https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_classic_device_mac.mm:265: return base::Time::FromDoubleT([inquiry_update timeIntervalSince1970]); Should you update the last_update_time_ in this case? https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:39: const base::TimeDelta kCleanCacheDelay = base::TimeDelta::FromSeconds(1); s/const/constexpr/ to make it clear to readers that this is a static rather than a dynamic initialization. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:54: clean_cache_timer_( I'm nervous about periodic tasks that call into MacOS's bluetooth stack, after https://crbug.com/482141. If you've already thought about jank and concluded this won't cause any, could you comment how you know that? Otherwise, please think about it. :)
https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:577: if ([device isPaired]) { On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > Could you comment why this is needed, with any radar or other bugs that have been filed about it? Done. For some reason pairedDevices returns an unknown device (that the machine has never paired with) that also returns false for isPaired. Since the function is called AddPairedDevices I figured that the non-paired device should be filtered out. I filed an issue with apple 28379050. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_classic_device_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_classic_device_mac.mm:260: // Even when a device is discovered during an inquiry procedure this returns On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > "Even when a device is discovered during an inquiry procedure" makes it sound like the function always returns nil. When does it actually return non-nil? Maybe "If the device was just discovered, this returns nil, even if the discovery was during an inquiry procedure."? I've actually never seen this function return a non-nil value. I tried with just classic devices around me, paired devices, connected devices. All of them return nil O.O I opened radar 28379445 for it. Maybe just: Sometimes getLastInquiryUpdate returns nil so we implement our own... Or maybe I should just remove the use of the function and just add a comment explaining that getLastInquiryUpdate returns nil unpredictably so we just use our own variable to keep track of the update time. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_classic_device_mac.mm:265: return base::Time::FromDoubleT([inquiry_update timeIntervalSince1970]); On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > Should you update the last_update_time_ in this case? See comment above. I think we should just remove the use of getLastInquiryUpdate. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:39: const base::TimeDelta kCleanCacheDelay = base::TimeDelta::FromSeconds(1); On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > s/const/constexpr/ to make it clear to readers that this is a static rather than a dynamic initialization. Done. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:54: clean_cache_timer_( On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > I'm nervous about periodic tasks that call into MacOS's bluetooth stack, after https://crbug.com/482141. If you've already thought about jank and concluded this won't cause any, could you comment how you know that? Otherwise, please think about it. :) I chose 1 second in case cleaning the cache took too long. I'm also not that worried about it because this only happens during an active scan. The other one happened ALL the time. We could add some tracers to make sure it doesn't take too long. That said maybe I'm trying to force the classic API to be too similar to the LE API, that is, maybe there is no need to be notified of each time we see a classic device. We could just clean the cache before a client starts a discovery that would allow new discovery sessions to find old devices and avoid the timer. WDYT?
jyasskin: ping?
https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_classic_device_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_classic_device_mac.mm:260: // Even when a device is discovered during an inquiry procedure this returns On 2016/09/20 04:31:33, ortuno wrote: > On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > > "Even when a device is discovered during an inquiry procedure" makes it sound > like the function always returns nil. When does it actually return non-nil? > Maybe "If the device was just discovered, this returns nil, even if the > discovery was during an inquiry procedure."? > > I've actually never seen this function return a non-nil value. I tried with just > classic devices around me, paired devices, connected devices. All of them return > nil O.O I opened radar 28379445 for it. Maybe just: Sometimes > getLastInquiryUpdate returns nil so we implement our own... > > Or maybe I should just remove the use of the function and just add a comment > explaining that getLastInquiryUpdate returns nil unpredictably so we just use > our own variable to keep track of the update time. Removing the use sounds reasonable if you've never seen it return a useful answer. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:54: clean_cache_timer_( On 2016/09/20 04:31:33, ortuno wrote: > On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > > I'm nervous about periodic tasks that call into MacOS's bluetooth stack, after > https://crbug.com/482141. If you've already thought about jank and concluded > this won't cause any, could you comment how you know that? Otherwise, please > think about it. :) > > I chose 1 second in case cleaning the cache took too long. I'm also not that > worried about it because this only happens during an active scan. The other one > happened ALL the time. We could add some tracers to make sure it doesn't take > too long. Having a tracer sounds like a good idea. Just around the whole CleanFoundDevicesCache() body? Or ignore this if you take the timer out as you say below. > That said maybe I'm trying to force the classic API to be too similar to the LE > API, that is, maybe there is no need to be notified of each time we see a > classic device. We could just clean the cache before a client starts a discovery > that would allow new discovery sessions to find old devices and avoid the timer. > WDYT? Yeah, Classic inquiry is done by the inquirer sending a broadcast and listening for responses, rather than LE's style of listening for spontaneous broadcasts. It probably makes sense to only discover each Classic device once per inquiry. https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:519: // Return the timestamp for when this device was last seen. Mention what the default return value is.
Description was changed from
==========
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. Add a timer that cleans up the cache every second. This means we will
be notified every time the device is 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
==========
to
==========
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
==========
https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_classic_device_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_classic_device_mac.mm:260: // Even when a device is discovered during an inquiry procedure this returns On 2016/09/23 at 22:25:21, Jeffrey Yasskin wrote: > On 2016/09/20 04:31:33, ortuno wrote: > > On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > > > "Even when a device is discovered during an inquiry procedure" makes it sound > > like the function always returns nil. When does it actually return non-nil? > > Maybe "If the device was just discovered, this returns nil, even if the > > discovery was during an inquiry procedure."? > > > > I've actually never seen this function return a non-nil value. I tried with just > > classic devices around me, paired devices, connected devices. All of them return > > nil O.O I opened radar 28379445 for it. Maybe just: Sometimes > > getLastInquiryUpdate returns nil so we implement our own... > > > > Or maybe I should just remove the use of the function and just add a comment > > explaining that getLastInquiryUpdate returns nil unpredictably so we just use > > our own variable to keep track of the update time. > > Removing the use sounds reasonable if you've never seen it return a useful answer. Done. https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_discovery_manager_mac.mm (right): https://codereview.chromium.org/2282763004/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_discovery_manager_mac.mm:54: clean_cache_timer_( On 2016/09/23 at 22:25:21, Jeffrey Yasskin wrote: > On 2016/09/20 04:31:33, ortuno wrote: > > On 2016/09/14 at 20:35:57, Jeffrey Yasskin wrote: > > > I'm nervous about periodic tasks that call into MacOS's bluetooth stack, after > > https://crbug.com/482141. If you've already thought about jank and concluded > > this won't cause any, could you comment how you know that? Otherwise, please > > think about it. :) > > > > I chose 1 second in case cleaning the cache took too long. I'm also not that > > worried about it because this only happens during an active scan. The other one > > happened ALL the time. We could add some tracers to make sure it doesn't take > > too long. > > Having a tracer sounds like a good idea. Just around the whole CleanFoundDevicesCache() body? Or ignore this if you take the timer out as you say below. > > > That said maybe I'm trying to force the classic API to be too similar to the LE > > API, that is, maybe there is no need to be notified of each time we see a > > classic device. We could just clean the cache before a client starts a discovery > > that would allow new discovery sessions to find old devices and avoid the timer. > > WDYT? > > Yeah, Classic inquiry is done by the inquirer sending a broadcast and listening for responses, rather than LE's style of listening for spontaneous broadcasts. It probably makes sense to only discover each Classic device once per inquiry. Removed timer. https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:519: // Return the timestamp for when this device was last seen. On 2016/09/23 at 22:25:21, Jeffrey Yasskin wrote: > Mention what the default return value is. What do you mean? The default value of base::Time() or the base implementation value i.e. last_update_time_? I did the former. Let me know if you meant the other one.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:519: // Return the timestamp for when this device was last seen. On 2016/09/26 08:45:29, ortuno wrote: > On 2016/09/23 at 22:25:21, Jeffrey Yasskin wrote: > > Mention what the default return value is. > > What do you mean? The default value of base::Time() or the base implementation > value i.e. last_update_time_? > > I did the former. Let me know if you meant the other one. The base implementation value; probably something like "returns the time of the last call to UpdateTimestamp(), or base::Time() if it hasn't been called yet." https://codereview.chromium.org/2282763004/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2282763004/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:577: // pairedDevices returns an unknown device that is not paired. s/returns/includes/, I think, since it also returns paired devices.
The CQ bit was checked by ortuno@chromium.org
Thanks! https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/2282763004/diff/80001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:519: // Return the timestamp for when this device was last seen. On 2016/09/26 at 18:35:59, Jeffrey Yasskin wrote: > On 2016/09/26 08:45:29, ortuno wrote: > > On 2016/09/23 at 22:25:21, Jeffrey Yasskin wrote: > > > Mention what the default return value is. > > > > What do you mean? The default value of base::Time() or the base implementation > > value i.e. last_update_time_? > > > > I did the former. Let me know if you meant the other one. > > The base implementation value; probably something like "returns the time of the last call to UpdateTimestamp(), or base::Time() if it hasn't been called yet." Done. Thanks for the suggestion :) https://codereview.chromium.org/2282763004/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/2282763004/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:577: // pairedDevices returns an unknown device that is not paired. On 2016/09/26 at 18:35:59, Jeffrey Yasskin wrote: > s/returns/includes/, I think, since it also returns paired devices. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/2282763004/#ps160001 (title: "Address moar comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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
Committed: https://crrev.com/154da849a1f7106c4efb3b514d88eac45a39afaa
Cr-Commit-Position: refs/heads/master@{#421069}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/154da849a1f7106c4efb3b514d88eac45a39afaa Cr-Commit-Position: refs/heads/master@{#421069} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
