New notifications observing logic, Peripheral provider and unit tests. #215
Conversation
…e Peripheral for on CBPeripheral
Source/Characteristic.swift
Outdated
|
|
||
| Notification is automaticaly unregistered once this observable is unsubscribed | ||
|
|
||
| - returns: `Observable` emitting `Next` with `Characteristic` when the notification setup is complete. |
There was a problem hiding this comment.
I think it should be change to
- returns: `Observable` emitting `Next` with `Characteristic` when give characteristic has been changed.
|
|
||
| class CharacteristicNotificationManager { | ||
|
|
||
| private unowned let peripheral: CBPeripheral |
There was a problem hiding this comment.
Could you also add unowned to Peripheral.manager? Now CentralManager has strong reference to PeripheralProvider which has strong reference to Peripheral. In that case there is reference cycle, so we need to add unowned to Peripheral.manager
| .do(onDispose: { [weak self] in | ||
| guard let strongSelf = self else { return } | ||
| do { strongSelf.lock.lock(); defer { strongSelf.lock.unlock() } | ||
| strongSelf.uuidToActiveObservableMap.removeValue(forKey: characteristic.uuid) |
There was a problem hiding this comment.
I am just thinking of situation when:
- Thread 1 stops at line 53
- Thread 2 locks at line 42 and taking observable on line 44-45
- Thread 1 removing observable from map at line 55
In this screnario Thread 2 receives observable that is disposed. Let's talk about this when you will be at work
| do { strongSelf.lock.lock(); defer { strongSelf.lock.unlock() } | ||
| strongSelf.uuidToActiveObservableMap.removeValue(forKey: characteristic.uuid) | ||
| } | ||
| _ = strongSelf.setNotifyValue(false, for: characteristic) |
There was a problem hiding this comment.
Not sure if we would not need to wait here for CBPeripheralDelegate peripheral:didUpdateNotificationStateForCharacteristic:error:
There could be scenario like:
- Thread 1 stops on line 57 (
setNotifyValuenot called) - Thread 2 goes to line 62, subscibes for observable and calls
setNotifyValue(true) - Thread 1 is doing
setNotifyValue(false)on line 57
In result we havesetNotifyValueturned to false and existing observable for notification change (which will never receive notification)
Simple solution would be to wrap this line in do lock from line 54. In that case it would be not possible to call setNotifyValue with false after setNotifyValue with true. Question here is, how will CoreBluetooth behave when we will set 2x setNotifyValue one after another
Source/Peripheral+Convenience.swift
Outdated
| Notification is automaticaly unregistered once this observable is unsubscribed | ||
|
|
||
| - parameter characteristic: `Characteristic` for notification setup. | ||
| - returns: `Observable` emitting `Peripheral` when the notification setup is complete. |
Source/Peripheral.swift
Outdated
| Notification is automaticaly unregistered once this observable is unsubscribed | ||
|
|
||
| - parameter characteristic: `Characteristic` for notification setup. | ||
| - returns: `Observable` emitting `Peripheral` when the notification setup is complete. |
Source/PeripheralProvider.swift
Outdated
| peripheralsBox.compareAndSet( | ||
| compare: { peripherals in | ||
| return !peripherals.contains(where: { $0.peripheral == cbPeripheral }) | ||
| }, |
|
@rohitjb agree that we could split doing tests to other PR, but due to big delays in reviewing we wanted to make it in one shoot. It won't happen again in future ;) |
| return .deferred { [weak self] in | ||
| if enabled { | ||
| let counter = self?.uuidToActiveObservablesCountMap[characteristic.uuid] ?? 0 | ||
| self?.uuidToActiveObservablesCountMap[characteristic.uuid] = counter + 1 |
There was a problem hiding this comment.
I think that it should be also guarded by NSLock.
Imagine scenario:
- thread 1 is on line 89, after getting counter value (value==1)
- thread 2 goes to line 58 and decrease counter (value==0)
- thread 1 changes coun map on line 89 (value==2)
Finally we have count==2 and only 1 observable.
Part of #173 and #207
A few changes that update API:
PeripheralProvider- central place constructing and managingPeripheralobjectsCharacteristicNotificationManager- which is used to observe characteristics value update and change notification flags on characteristicsRxError.noElementserror, if not all objects with requested UUIDs were discoveredAdded unit tests to the missing classes.