ble.disconnect() should not call the connect failure callback

[Domvel]

Should the connect method error callback ignore a wanted disconnect by the method disconnect? Currently it also throws an error "Peripherie disconnected" when the user or me invokes the disconnect method.

Background: I want to retry if a wanted connection was failed. Case: User clicked on a device from the list (result of scan). This will work. But if the app disconnects by the method disconnect, the error callback from connect is also invoked. Case: User clicked on disconnect. (I use the rxjs Observable for retries.)

The disconnect method will confirm the wanted disconnect. or not? Perhaps the connect error callback should distinguish between

• Peripheral Disconnected
• Central Disconnected (or ignore it and use the success callback from disconnect to know it.)

I just think aloud. What you think about this? You can close this issue on any reason if you want. :) Is this possible? Maybe on native code side, there is already the reason / device returned on disconnect. But maybe this does not guarantee a wanted or unwanted disconnect. Idk.

Workarounds (not tested / hypothetically)

• Unsubscribe from the observable on disconnect invoked. (ionic)
• Hold a state of wanted disconnect in a variable and handle it on connect error callback. Reset it on connect.

[don]

The failure function of connect should only be called when the device disconnects, not when the user calls ble.disconnect(). I've merged a bunch of stuff lately so the behavior might have changed, which would be a bug.

[Domvel]

🤔 In my case, the failure callback of connect is invoked when disconnect with the disconnect method. ionic-native/ble 4.7.0 cordova-plugin-ble-central 1.1.9

[don]

@Domvel I need to look into this to see what changed.

[Domvel]

@don Ok thanks. I could also test it in cordova (directly without ionic).

[Domvel]

@don I tested it now with a fresh new ionic project with the same version. And it works like you described. On a wanted disconnect the error callback of connect is not invoked. This is weird. I can't explain it. I should give up before it destroys me. 😄 Maybe a bug in the matrix? ... Reinstalling android platform and node_modules does not help.

[don]

@Domvel this works correctly with 1.1.9 from npm. It's broken with the latest code from master. I need to look into why and fix that before tagging and releasing the next version (#588)

[don]

When the app calls ble.disconnect() the error callback of ble.connect() should not be called since the user request the disconnect. The error callback of ble.connect is only for when the device disconnects. v1.1.9 is ok, the problem is in a non released code. Autoconnect is also broken.

[don]

@Domvel I think this is fixed https://github.com/don/cordova-plugin-ble-central/tree/issue-582 I'll test again tomorrow and merge if things still look good.

[Domvel]

@don Thanks. Yes it works correctly with 1.1.9 from npm. Both project has the same version. But on my main project it does not work correctly. (error called regardless of user request disconnect). And on the new ionic project for demonstration of this issue it works correctly. (error does not called on user request disconnect). I checked the BLE versions. On both projects they are identical. ionic-native 4.7.0 ble (this) 1.1.9

But ok, if you found a problem I can test it again with the new version.

[doug-a-brunner]

@don - unfortunately this re-breaks @ionic-native/ble's transformation of the callbacks from connect into an Observable. It looks like I previously had fixed it with 84a8219. If the error callback never fires, the Observable is left dangling, so either the program will leak resources or get stuck waiting for the Observable to complete.

There seems to be some confusion on this - the documentation presently says

connectFailure: Error callback function, invoked when error occurs or the connection disconnects.

[don]

@doug-a-brunner The changes in 84a8219fba82b97531834c90aacfb22c829aafc9 broke the existing plugin behavior. The connect error callback should only fire when the device disconnects, not when the app requests the disconnection. I need to clarify my documentation.

I need to look into Observables some more. I didn't realize they were supposed to complete. If that's the case, ble.disconnect might need to cancel the observable in the ionic wrapper.

[don]

@doug-a-brunner it turns out that Ionic already cleans up the subscription when ionic-native's ble.disconnect is called. The error callback does not need to be called when the user initiates the disconnect.

In the ionic native wrapper, connect lists disconnect as the clearFunction. The Cordova annotation takes care of all the wiring.

@Cordova({
   observable: true,
   clearFunction: 'disconnect',
   clearWithArgs: true
 })
 connect(deviceId: string): Observable<any> {
   return;
 }

https://github.com/ionic-team/ionic-native/blob/7cbe7a628729ec86730827b91ad4073a6a89fc29/src/%40ionic-native/plugins/ble/index.ts#L283-L290

[doug-a-brunner]

@don - I had that same thought when I read through the ionic-native code at first. However, according to the documentation:

clearFunction is used in conjunction with the observable option and indicates the function to be called when the Observable is disposed.

This matches with what I saw when I read through the inner plumbing of the Cordova decorator: rather than adding to disconnect so that it completes the Observable, clearFunction here causes disconnect to be called when all the subscriptions to the Observable have been disposed. The relevant code is in plugin.ts; the returned function is TeardownLogic in RxJS parlance.

[don]

@doug-a-brunner that's not good news. I'll look through this and talk to Ionic to see if they have ideas. Maybe there's another way to do this in the Ionic wrapper?

[doug-a-brunner]

@don I agree that it doesn't seem right for the Observable to terminate with an error when there wasn't actually an error. Ideally there would be a third callback on connect, for when the connection was terminated at application request, or ionic-native would hook the disconnect callback to also terminate any Observables generated by connect.

It seems like the easiest way out, however, is to change the plugin to conform to the existing documentation: always call the error callback when the connection terminates, and perhaps call it terminateCallback to be more clear about when it runs. We could also specify that terminateCallback is called with "Peripheral disconnected" when the disconnect was not initiated by the central, and "Disconnect completed" when it was, thus simplifying logic for the application designers.

[don]

It seems like the easiest way out, however, is to change the plugin to conform to the existing documentation

You're mis-interpreting my documentation. :) I only want the connect failure callback called when the peripheral terminates the connection. This way I know the connection was terminated. If the callback is always called, then I need more logic to see who the caused the disconnect.

The 3rd callback is a good idea, but would be invasive to implement because that's not how Cordova callbacks work. I'd need to set up a bunch of channels.

[Domvel]

btw. a question about notifications. Is it possible to keep the notification after a reconnect (auto connect). I think not, because after the peripheral resets his power supply, all handles are gone. But maybe the autoConnect feature knows the notifications and re-subscribe to them. But I don't know if this will work with ionic observables. What do you think about this?

And is it possible to reduce / set the timeout time of a disconnect detection? (Also will speed up the reconnection)

[don]

@Domvel you need to startNotification again when you reconnect. I haven't seen anything to adjust the disconnect detection on Android.

[don]

@doug-a-brunner I verified that your description of the cancelFunction behavior is what's happening. The Ionic documentation for the plugin needs to be updated to make this clearer. When I unsubscribe from a subscription the cancelFunction is called. It's very magic, but it looks good in some sample code I converted. The biggest problem is that it's not clear what's happening.

This works well if unsubscribe is used to disconnect the peripheral.

this.connect$ = this.ble.connect(device.id).subscribe(
  peripheral => this.onConnected(peripheral),
  peripheral => this.onDeviceDisconnected(peripheral)
);

Later when navigating away from a page I can call unsubscribe to disconnect.

ionViewWillLeave() {
  // unsubscribing automatically calls disconnect and ignores the promise
  this.connect$.unsubscribe();
}

The bad part about the cancelFunction is that it is also called when there is an error. if the peripheral initiates a disconnect, the plugin calls the the error function of connect. Ionic then calls the error on the subscription. Then Ionic calls the cancelFunction ble.disconnect, which doesn't make any sense because we were already disconnected. The promise from ble.disconnect is ignored so that Ionic app is completely unaware of what happened.

I need to find out how other plugins are handling this. Mapping callbacks to Observables is a mismatch for some of these functions. ble.autoConnect won't work with Observables since it calls the success and failure callbacks many times as the device connects and disconnects. As soon as the first disconnect is called, the Observable completes. If you have ideas how we can implement this, let me know.

[Domvel]

btw. I updated the ionic wrapper here. Idk when it will be released. But you could check it for mistakes.