Fixed race condition with characteristic.value Data() being overwritten when dealing with longer messages + added didConnectPeripheral notification

[nervus]

I sign up to a notification on the GoPro and it just sends it when stuff changes on the device .

I believe the problems stems from the fact that the characteristic value’s lifetime is only valid during the didUpdateValueFor: callback. - since it’s then buffered to use asynchronously the contents of the value Data() is no longer valid once the didUpdateValueFor exits.

Basically imagine this timeline ( written on the phone so sorry for the coder graphics :p )

—duv(1)…duv(2)…duv(3)… etc -> didupdatevalue thread ….. cvus(1) ………………..….. Cvus(2)… cvus(3)….. characteristicValueUpdatedSubject Receive Thread

In this case, by the time you receive the update from cvus(2), the characteristic value pointer has already been filled with the duv(3) data and this is pretty much what I’ve seen before doing the change. The other option was to basically copy the current characteristic value and pass the copy on so it doesn’t get overwritten next time didUpdateValue is called .

Sent from my iPhone

On Aug 11, 2022, at 5:17 AM, Manuel Fernandez @.***> wrote:

 @manolofdez commented on this pull request.

In Sources/Peripheral/Peripheral.swift:

@@ -279,25 +279,25 @@ extension Peripheral.DelegateWrapper: CBPeripheralDelegate { }

 func peripheral(_ cbPeripheral: CBPeripheral, didUpdateValueFor characteristic: CBCharacteristic, error: Error?) {

• Task {
• do {
• let result = CallbackUtils.result(for: (), error: error)
• try await self.context.readCharacteristicValueExecutor.setWorkCompletedForKey(
• characteristic.uuid, result: result
• )
• } catch {
• if !characteristic.isNotifying {
• if characteristic.isNotifying {
• // characteristic.value is Data() and it will get trampled if allowed to run async.
• self.context.characteristicValueUpdatedSubject.send( Characteristic(characteristic) )
• } else { What's triggering the data to change?

Are you setting some value on the device, waiting for a notification on a characteristic and then reading its value (repeating this process until you've read all packets)? If so, I wouldn't expect the values to change between reads, since you control when the next set of data comes in (e.g. you'd read the current data before writing to the characteristic to request for the next set of data).

If instead something else is changing the value of the characteristic you want to read (e.g. a timer on the gopro) and you're getting hit with two simultaneous peripheral(_:didUpdateValueFor:error:), it might be a little trickier to solve. If you have some way of knowing the order of a given packet (e.g. continuation packet), one thing you could do is store the values in a "[continuation packet: data]" map and then sort the keys to read the data in order.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[manolofdez]

I sign up to a notification on the GoPro and it just sends it when stuff changes on the device . I believe the problems stems from the fact that the characteristic value’s lifetime is only valid during the didUpdateValueFor: callback. - since it’s then buffered to use asynchronously the contents of the value Data() is no longer valid once the didUpdateValueFor exits. Basically imagine this timeline ( written on the phone so sorry for the coder graphics :p ) —duv(1)…duv(2)…duv(3)… etc -> didupdatevalue thread ….. cvus(1) ………………..….. Cvus(2)… cvus(3)….. characteristicValueUpdatedSubject Receive Thread In this case, by the time you receive the update from cvus(2), the characteristic value pointer has already been filled with the duv(3) data and this is pretty much what I’ve seen before doing the change. The other option was to basically copy the current characteristic value and pass the copy on so it doesn’t get overwritten next time didUpdateValue is called .

Sorry for the delay, was out of town and didn't have a laptop around. In that case we could try copying the current characteristic value like you suggest. One idea is sending out a struct with the characteristic and the data through the characteristicValueUpdatedSubject. Something like:

public struct CharacteristicUpdatedEventData {
    let characteristic: Characteristic
    let value: Data?
}

class PeripheralContext {
    private(set) lazy var characteristicValueUpdatedSubject = PassthroughSubject<CharacteristicUpdatedEventData, Never>()
...
}

extension Peripheral.DelegateWrapper: CBPeripheralDelegate {
...
    self.context.characteristicValueUpdatedSubject.send(
        CharacteristicUpdatedEventData(characteristic: Characteristic(characteristic), value: characteristic.value)
    )
...
}

[manolofdez]

@manolofdez this is a good fix too. do you want this in a separate PR?

9a1902b

No, it's fine to leave in here.

[nervus]

Btw, removing the else triggers a lot of [peripheral] Received UpdateValue result for characteristic without a continuation

On Nov 20, 2022, at 6:43 PM, Manuel Fernandez @.***> wrote:

@manolofdez commented on this pull request.

In Sources/Peripheral/Peripheral.swift https://github.com/manolofdez/AsyncBluetooth/pull/10#discussion_r1027497353:

@@ -279,25 +279,25 @@ extension Peripheral.DelegateWrapper: CBPeripheralDelegate { }

 func peripheral(_ cbPeripheral: CBPeripheral, didUpdateValueFor characteristic: CBCharacteristic, error: Error?) {

• Task {
• do {
• let result = CallbackUtils.result(for: (), error: error)
• try await self.context.readCharacteristicValueExecutor.setWorkCompletedForKey(
• characteristic.uuid, result: result
• )
• } catch {
• if !characteristic.isNotifying {
• if characteristic.isNotifying {
• // characteristic.value is Data() and it will get trampled if allowed to run async.
• self.context.characteristicValueUpdatedSubject.send( Characteristic(characteristic) )
• } else { Heads up this is just missing removing the else statement so that the read queue continues when you're notifying.

— Reply to this email directly, view it on GitHub https://github.com/manolofdez/AsyncBluetooth/pull/10#pullrequestreview-1187480694, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH7GZLORGBMQ4OSXD5UEHTWJLOW5ANCNFSM54GQFQSQ. You are receiving this because you modified the open/close state.