Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Help]: Bug: ERROR_GATT_WRITE_REQUEST_BUSY (201) on Version 1.35.0 on setCharacteristicNotification #1102

Closed
1 task done
codercengiz opened this issue Jan 16, 2025 · 27 comments
Labels
help Questions, help, observations, or possible bugs

Comments

@codercengiz
Copy link

Requirements

  • I've looked at the README 'Common Problems' section

Have you checked this problem on the example app?

No

FlutterBluePlus Version

1.35.0

Flutter Version

3.27.2

What OS?

Android

OS Version

Android 14

Bluetooth Module

esp32

What is your problem?

I was using version 1.34.5 without any issues, but after upgrading to version 1.35.0, I encountered a bug.

Here are the details:

  • I downgraded back to 1.34.5 and tested it twice to confirm. The issue does not occur in the previous version, but it consistently happens in version 1.35.0.
  • The issue arises after connecting and performing MTU negotiation. When attempting to subscribe to a characteristic, I get the following error:
I/flutter (25160): [BluetoothConnectionPlus] error: | 10:23:15.431 | Got exception when connecting to device:
I/flutter (25160): PlatformException(setNotifyValue, gatt.writeDescriptor() returned 201 : ERROR_GATT_WRITE_REQUEST_BUSY, null, null)

Could you please look into this? Let me know if you need further details or additional testing.

Logs

I/flutter (25160): [BluetoothConnectionPlus] info: | 10:23:14.323 | State Connecting Device
D/BluetoothGatt(25160): discoverServices() - device:
D/BluetoothGatt(25160): onConnectionUpdated() - Device=XX:XX:XX:XX:95:BA interval=6 latency=0 timeout=500 status=0
D/BluetoothGatt(25160): onSearchComplete() = Device=XX:XX:XX:XX:95:BA Status=0
D/BluetoothGatt(25160): onConnectionUpdated() - Device=XX:XX:XX:XX:95:BA interval=39 latency=0 timeout=500 status=0
D/BluetoothGatt(25160): configureMTU() - device: XX:XX:XX:XX:95:BA mtu: 517
D/BluetoothGatt(25160): onConfigureMTU() - Device=XX:XX:XX:XX:95:BA mtu=517 status=0
D/BluetoothGatt(25160): setCharacteristicNotification() - uuid: xxxxxxxxxxxxxx enable: true
I/flutter (25160): [BluetoothConnectionPlus] info: | 10:23:15.413 | Subscribing to characteristic DEVICE_EVENT
D/BluetoothGatt(25160): setCharacteristicNotification() - uuid: xxxxxxxxxxx enable: true
W/BluetoothGatt(25160): writeDescriptor() - prior command is not finished, mClient= 5
I/flutter (25160): [BluetoothConnectionPlus] error: | 10:23:15.431 | Got exception when connecting to Device:
I/flutter (25160): PlatformException(setNotifyValue, gatt.writeDescriptor() returned 201 : ERROR_GATT_WRITE_REQUEST_BUSY, null, null)
@codercengiz codercengiz added the help Questions, help, observations, or possible bugs label Jan 16, 2025
@sumaro
Copy link

sumaro commented Jan 16, 2025

I also encountered this problem

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 16, 2025

@chipweinberger am I right in thinking that in theory the mutex should prevent more than one operation at a time?

It looks like this issue may be caused by the previous operation not completing before the next operations starts.

@chipweinberger
Copy link
Owner

yes this is the role of the mutex. not sure why 1.35.0 would regress this

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 16, 2025

@codercengiz would I be able to get you to confirm if you still experience this issue with the fix/1102 branch?

dependency_overrides:
  flutter_blue_plus_android:
    git:
      url: https://github.com/chipweinberger/flutter_blue_plus.git
      ref: fix/1102
      path: packages/flutter_blue_plus_android

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 16, 2025

@tnc1997 , why not just keep it how we had it in 1.34.5? And why was this changed? We need to wait on all platforms, not just Android. If you dont wait on iOS, then we no longer detect failure anymore.

You can see we waited for the descriptor to be written.

  Future<bool> setNotifyValue(bool notify, {int timeout = 15, bool forceIndications = false}) async {
    // check connected
    if (device.isDisconnected) {
      throw FlutterBluePlusException(
          ErrorPlatform.fbp, "setNotifyValue", FbpErrorCode.deviceIsDisconnected.index, "device is not connected");
    }

    // check
    if (Platform.isMacOS || Platform.isIOS) {
      assert(forceIndications == false, "iOS & macOS do not support forcing indications");
    }

    // Only allow a single ble operation to be underway at a time
    _Mutex mtx = _MutexFactory.getMutexForKey("global");
    await mtx.take();

    try {
      var request = BmSetNotifyValueRequest(
        remoteId: remoteId,
        serviceUuid: serviceUuid,
        characteristicUuid: characteristicUuid,
        forceIndications: forceIndications,
        enable: notify,
        primaryServiceUuid: primaryServiceUuid,
      );

      // Notifications & Indications are configured by writing to the
      // Client Characteristic Configuration Descriptor (CCCD)
      Stream<BmDescriptorData> responseStream = FlutterBluePlus._methodStream.stream
          .where((m) => m.method == "OnDescriptorWritten")
          .map((m) => m.arguments)
          .map((args) => BmDescriptorData.fromMap(args))
          .where((p) => p.remoteId == request.remoteId)
          .where((p) => p.serviceUuid == request.serviceUuid)
          .where((p) => p.characteristicUuid == request.characteristicUuid)
          .where((p) => p.descriptorUuid == cccdUuid)
          .where((p) => p.primaryServiceUuid == request.primaryServiceUuid);

      // Start listening now, before invokeMethod, to ensure we don't miss the response
      Future<BmDescriptorData> futureResponse = responseStream.first;

      // invoke
      bool hasCCCD = await FlutterBluePlus._invokeMethod('setNotifyValue', request.toMap());

      // wait for CCCD descriptor to be written?
      if (hasCCCD) {
        BmDescriptorData response = await futureResponse
            .fbpEnsureAdapterIsOn("setNotifyValue")
            .fbpEnsureDeviceIsConnected(device, "setNotifyValue")
            .fbpTimeout(timeout, "setNotifyValue");

        // failed?
        if (!response.success) {
          throw FlutterBluePlusException(_nativeError, "setNotifyValue", response.errorCode, response.errorString);
        }
      }
    } finally {
      mtx.give();
    }

    return true;
  }

@codercengiz
Copy link
Author

@codercengiz would I be able to get you to confirm if you still experience this issue with the fix/1102 branch?

dependency_overrides:
flutter_blue_plus_android:
git:
url: https://github.com/chipweinberger/flutter_blue_plus.git
ref: fix/1102
path: packages/flutter_blue_plus_android

I tested with override plugin (flutter_blue_plus_android)
and it seems that the issue is fixed. Thanks :)

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 16, 2025

We need to wait on all platforms, not just Android. If you dont wait on iOS, then we no longer detect failure anymore.

I believe that we only need to wait on Android and iOS/macOS therefore this is internal platform specific behaviour.

Also reviewing the iOS/macOS implementation it only appears to return true which is subtly different from Android.

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 16, 2025

I tested with override plugin (flutter_blue_plus_android) and it seems that the issue is fixed. Thanks :)

Thank you for confirming that change fixes this issue, we will publish some patched versions to pub.dev.

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 16, 2025

I believe that we only need to wait on Android and iOS/macOS therefore this is internal platform specific behaviour.

For the other platforms (just web I think), they just need to return "false", then we wont wait.

Yes we could refactor it. And we probably should at some point. But let's get all the bugs worked out first.

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 16, 2025

Btw, this fix has an issue.

if (hasCCCD == true) {
      await response.timeout(
        Duration(
          seconds: 15,
        ),
      );
    }

It should early returns if the device is disconnected / adapter turned off, and the timeout should be configurable like this:

        BmDescriptorData response = await futureResponse
            .fbpEnsureAdapterIsOn("setNotifyValue")
            .fbpEnsureDeviceIsConnected(device, "setNotifyValue")
            .fbpTimeout(timeout, "setNotifyValue");

Also, response.timeout, returns a TimeoutException, whereas before it was a FbpException.

(my goal with FBP was to always return well defined FbpExceptions with nice error messages, but I never finished wrapping the PlatformExceptions).

e.g. FlutterBluePlusException | requestMtu | fbp-code: 1 | Timed out after 15

IMO, lets just update the platform interface to keep it how it was. Seems simpler.

So that all platforms are more unified in their behavior.

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 16, 2025

IMO, lets just update the platform interface to keep it how it was.

Of course, that sounds good to me. If you can update the platform interface how you would like it, then I would be more than happy to update the Linux and Web packages after.

@chipweinberger
Copy link
Owner

I think just this: 5e59a71

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 17, 2025

Looks good to me, don't forget to bump the major version of the platform interface package, as that is a breaking change.

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 17, 2025

as that is a breaking change

Now would be a good opportunity to review the platform interface for any other breaking changes that you may like to make so that they could all be combined together into a single major release.

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 17, 2025

Now would be a good opportunity to review the platform interface for any other breaking changes that you may like to make so that they could all be combined together into a single major release.

What is the reason to not break the platform interface? We own nearly all the platform code too.

If someone else starts making platform implementations, we can have that conversation later. But for now, we can break whenever we want with zero consequnce. Literally no one will know / care haha.

I don't see this situation changing anytime soon. There are almost no other platforms left to implement. TBD on Windows.

But Windows should not prevent us from designing the platform interface as we see fit. We are the main users of it. Normal users will never touch it.

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 17, 2025

What is the reason to not break the platform interface?

This guide from the Flutter team has more information on platform interface breaking changes.

As long as the major version is bumped each time then you can make as many breaking changes as you like.

It is just that each breaking change that is made requires updating each federated implementation package appropriately.

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 17, 2025

even without a major version bump, it affects no one except you and me,

we are the only platform implementers right now (possibly ever).

I do appreciate the link though.

but seems like worrying about a problem that doesnt yet exist / probably never will IMO.

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 17, 2025

I do appreciate the link though.

No problem, you're welcome!

but seems like worrying about a problem that doesnt yet exist

I would strongly advocate bumping the major version when any breaking changes are made as per semantic versioning.

More information regarding the pub.dev best practice on package versioning can be found here for reference.

If you choose not to follow semantic versioning then I would suggest documenting this in the README.

@chipweinberger
Copy link
Owner

If you choose not to follow semantic versioning then I would suggest documenting this in the README.

Okay

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 17, 2025

Added.

Image

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 17, 2025

Also, the 1.X.X style is a badge of honor for me. You can clearly see how much work I've put into it. 35 versions!

When you go to 2.0.0, it gets wiped.

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 17, 2025

Also, the 1.X.X style is a badge of honor for me.

Of course, as the lead maintainer, you are entitled to version each of the packages however you would like to.

The reason for my strong advocacy for semantic versioning, is that most packages across different languages follow this established convention, including Flutter packages.

@chipweinberger
Copy link
Owner

Let's use this versioning.

tnc1997#1 (comment)

@tnc1997
Copy link
Collaborator

tnc1997 commented Jan 18, 2025

Let's use this versioning.

As I previously advocated, many developers are familiar with and expect Flutter packages to use semantic versioning, because this is the documented standard, but ultimately you can of course version the flutter_blue_plus packages however you like.

That describes the format of a version number, and the exact API behavioral differences when you increment to a later version number. Pub requires versions to be formatted that way, and to play well with the pub community, your package should follow the semantics. You should assume that the packages you depend on also follow it. (And if you find out they don't, let their authors know!)

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 18, 2025

however you like.

Yes let's just try 1 version number to start. It's easy to switch to semantic versioning later if we want.

But it would be annoying to switch back to 1 version number after semantic versioning.

@chipweinberger
Copy link
Owner

chipweinberger commented Jan 18, 2025

But I'm also open to you doing whatever you want.

Feel free to use semantic versioning for the other packages.

im flexible

@chipweinberger
Copy link
Owner

update to 1.35.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help Questions, help, observations, or possible bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants