flutter_reactive_ble icon indicating copy to clipboard operation
flutter_reactive_ble copied to clipboard

Cancel subscription when a disconnect event has been thrown

Open sjoerd0301 opened this issue 1 year ago • 3 comments

How to replicate:

[Android only]

  • connect to device
  • enable notifications
  • power device off without disconnecting. (or walk out of range)
  • wait for disconnect event from plugin
  • power on device (or walk in range when you walked out of range in step 3)
  • scan and connect to device
  • enable notifications -> This should give an error the first time after disconnect. (EstablishedConnectionUpdate failure device disconnected MAC="...." etc)
  • cancel subscription
  • enable notifications again -> works

Somehow the EstablishedconnectionUpdate gets lost/cached/whatever when a device disconnects. This PR cancels the characteristic notificationstream on the android native side.

Possible fix for #384

sjoerd0301 avatar Jul 07 '22 10:07 sjoerd0301

@remonh87 @Taym95 @werediver?

sjoerd0301 avatar Jul 27 '22 12:07 sjoerd0301

@sjoerd0301 Thank you for the update. Do you believe you tested your changes sufficiently? If yes, this looks good to me.

CC @Taym95 @remonh87

werediver avatar Aug 03 '22 08:08 werediver

@sjoerd0301 Thank you for the update. Do you believe you tested your changes sufficiently? If yes, this looks good to me.

CC @Taym95 @remonh87

I believe I've tested this enough. I would like a reply from @remonh87 as I'm not sure what kind of impact this might have somewhere else in the plugin. As far as I can see there is none, because everything keeps working, but you can never be too sure

sjoerd0301 avatar Aug 05 '22 08:08 sjoerd0301

Hi all

Any chances this gets merged? We currently have issues when reconnecting to a device that was out of range. Subscribing to characteristics sometimes throws CharacteristicValueUpdateError.unknown errors. Issue #609 describes our case quite well. Unfortunately there is issues with the proposed solution as well. I found that delays are considered only on the initial connect but not on any subsequent reconnects unless it is set to something above 500 ms. I don't really like this workaround... It is somehow arbitrary and it might work on one mobile but not on another. This problem is especially nasty as we see it in release builds only.

Anyway, I just stumbled upon this MR which seems to solve all our issues. We can even get rid of delays. Obviously I'm very interested in a merge. Is there anything I can do here to speed up the process?

Many thanks for your efforts!

rfuerst87 avatar Dec 13 '22 15:12 rfuerst87

@Taym95 @werediver @remonh87 @sjoerd0301 I've tried to copy your changes to the library but seems to work same as the old way, maybe I'm doing something wrong with the dependencies, but I've imported flutter_reactive_ble inside my project and renamed and added to pubspec.yaml of the project the new path, but to mantain the chnages I also copied the reactive_ble_mobile and adjusted its pubspec.yaml and then inside of its foder i copied the reactive_ble_platform_interface , cuold you help me to reproduce your case?

meuhle avatar Jun 28 '23 10:06 meuhle

@meuhle you can directly point your dependencies in pubspec.yaml to @sjoerd0301's fork:

flutter_reactive_ble:
  git:
    url: https://github.com/sjoerd0301/flutter_reactive_ble.git
    ref: bug/cancel_char_notif_on_disconnect
    path: packages/flutter_reactive_ble

additionally you will need to override dependencies, as otherwise wrong dependencies get pulled in:

dependency_overrides:
  reactive_ble_mobile:
    git:
      url: https://github.com/sjoerd0301/flutter_reactive_ble.git
      ref: bug/cancel_char_notif_on_disconnect
      path: packages/reactive_ble_mobile
  reactive_ble_platform_interface:
    git:
      url: https://github.com/sjoerd0301/flutter_reactive_ble.git
      ref: bug/cancel_char_notif_on_disconnect
      path: packages/reactive_ble_platform_interface

By the way: Is there any updates on this MR? We're still relying on a custom fork on this repo just because of this one issue. Is there anything that that speaks against merging? @Taym95 @werediver @remonh87

rfuerst87 avatar Jun 29 '23 08:06 rfuerst87

@rfuerst87 thank you, I've added the fork dependencies to my pubspec.yaml, but I don't see any improvement. I'm connecting to the device the first time and get the notification from the charateristic but after the disconnection and reconnection the notification is not receiving. If i reload the app or switch off and on the device the notification restart, do you have any suggestion?

this is how I handle the disconnection from the device is therte anything wrong? deviceConnectionController.close(); g.flutterReactiveBle.deinitialize();

meuhle avatar Jun 29 '23 08:06 meuhle

For some reason I didn't receive any notifications for this repo and I was not working with ble last year. I looked into the PR and it seems good to me, good catch!

remonh87 avatar Jul 01 '23 08:07 remonh87

Welcome back, @remonh87

farr64 avatar Jul 01 '23 17:07 farr64

@sjoerd0301 lets rebase and merge it!

Taym95 avatar Jul 06 '23 18:07 Taym95

I will close this one and clone it in #769

Taym95 avatar Jul 10 '23 21:07 Taym95