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

Design Issues with Flutter Blue #264

Closed
chipweinberger opened this issue Apr 29, 2023 · 7 comments
Closed

Design Issues with Flutter Blue #264

chipweinberger opened this issue Apr 29, 2023 · 7 comments

Comments

@chipweinberger
Copy link
Owner

chipweinberger commented Apr 29, 2023

I've been working on the Flutter Blue Plus codebase for a couple days now, and as I'm looking at the issues, it's clear that there are some major design issues with Flutter Blue.

Design Issue 1: Timing

Flutter Blue has some timing problems. See commits like this for reference: a85378b

Before this commit we would:

  1. invoke a platform method
  2. Wait for the first result

After this commit:

  1. start waiting for the result
  2. invoke a platform method

This fixes a big problem, missing results. But causes a new one, returning wrong results. If multiple callers call read at the same time, they'll all do step 1, and then start waiting on step 2. But which result will they get? Typically, the wrong one. They'll both just return the first result. The design of FlutterBlue with a shared channel for returning results means there is no simple fix for this.

  Future<void> write(List<int> value, {bool withoutResponse = false}) async {

   ...

      var responseStream = FlutterBluePlus.instance._methodStream
        .where((m) => m.method == "WriteCharacteristicResponse")
        .map((m) => m.arguments)
        .map((buffer) => protos.WriteCharacteristicResponse.fromBuffer(buffer))
        .where((p) =>
            (p.request.remoteId == request.remoteId) &&
            (p.request.characteristicUuid == request.characteristicUuid) &&
            (p.request.serviceUuid == request.serviceUuid));

      // Start listening now to ensure we don't miss the response
      Future<protos.WriteCharacteristicResponse> futureResponse = responseStream.first;

      await FlutterBluePlus.instance._channel
        .invokeMethod('writeCharacteristic', request.writeToBuffer());

      // wait for response, so that we can check for success
      protos.WriteCharacteristicResponse response = await futureResponse;
      if (!response.success) {
        throw Exception('Failed to write the characteristic');
      }

      return Future.value();
}

Design Issue 2: Error Handling

Overall, Flutter Blue was not designed with error handling front-of-mind.

This is clear with issues like #242, where an exception is missed because Flutter Blue code was just not written correctly. This bug has been fixed, but it makes it clear that Error Handling was not a major consideration, or strongly tested during development. I do not have faith in the error handling abilities of Flutter Blue.

Design Issue 3: Inconsistency

This is harder to write down, but there is lots of general sloppiness & inconsistency in the code. Duplicate variables in the Dart API. Inconsistency between returning streams or variables directly. Try/catch clauses that just log in native code. Code which was needlessly hard to read. etc.

It does not inspire confidence =)

future

These issues can of course be addressed -- It would just take quite a bit of development. Perhaps these can be addressed in the future.

flutter reactive blue

Do to these issues, and after reading the flutter_reactive_blue codebase, I am going to be switching my efforts to that repository.

  1. they do not have the timing issue, by using a task queue to handle requests in-order
  2. all functions have error handling considered
  3. the code itself is consistent, though it is overly complicated
  4. It is used in a major commercial app with high reliability - PhillipsHue
  5. it's newer. ~2020, as opposed to flutter blue which came out in ~2017

Flutter Reactive BLE takes error handling very seriously:

Dart:

flutterReactiveBle.scanForDevices(withServices: [serviceId], scanMode: ScanMode.lowLatency).listen((device) {
      //code for handling results
    }, onError: () {
      //code for handling error
    });

Swift:

    func disableCharacteristicNotifications(name: String, args: NotifyNoMoreCharacteristicRequest, completion: @escaping PlatformMethodCompletionHandler) {
        guard let central = central
        else {
            completion(.failure(PluginError.notInitialized.asFlutterError))
            return
        }

        guard let characteristic = QualifiedCharacteristicIDFactory().make(from: args.characteristic)
        else {
            completion(.failure(PluginError.invalidMethodCall(method: name, details: "characteristic, service, and peripheral IDs are required").asFlutterError))
            return
        }

        do {
            try central.turnNotifications(.off, for: characteristic, completion: { _, error in
                if let error = error {
                    completion(.failure(PluginError.unknown(error).asFlutterError))
                } else {
                    completion(.success(nil))
                }
            })
        } catch {
            completion(.failure(PluginError.unknown(error).asFlutterError))
        }
    }

Covering my own ass

If flutter_reactive_blue does not work out long term, there is of course still a chance I will return to this codebase. I'll give myself that out =) Though given these issues I would also consider writing my own ble driver instead / significantly rewriting Flutter Blue.

@chipweinberger chipweinberger changed the title Major Design Issues with Flutter Blue Design Issues with Flutter Blue Apr 29, 2023
@chipweinberger
Copy link
Owner Author

chipweinberger commented Apr 29, 2023

I've updated this recommendation on the readme.

"Flutter Blue is simple way to add BLE to any app, however flutter_reactive_ble is generally best for apps that need the highest reliability. See #264"

Hopefully I'm not stepping on peoples feed with this recommendation. Just trying to help people write the best apps they can!

Edit: I fixed the timing issue and removed the warning / recommendation #266

@chipweinberger
Copy link
Owner Author

chipweinberger commented May 1, 2023

So I am running into issues with flutter_reactive_ble as well 😩

PhilipsHue/flutter_reactive_ble#728
PhilipsHue/flutter_reactive_ble#385

So...I am going to start tackling these design issues in flutter_blue_plus.

I've thought of a simple way to fix Design Issue 1: Timing: just wrap read & write in a mutex so that simultaneous calls to read & write to the same characteristic are serialized. This is not ideal for perf - queueing reads & writes on the iOS/Android side would be faster, as opposed to on the dart side, but I imagine it will still be fast enough to hit maximum BLE bandwidth. And more importantly, it doesn't require a big rewrite.

Design Issue 2: Error Handling & Design Issue 3: Inconsistency, I'll just tackle as-needed.

@chipweinberger
Copy link
Owner Author

Fixed Design Issue 1: 27a2a44

@Sajonji
Copy link

Sajonji commented May 2, 2023

Great to see progress on this library again !

@chipweinberger: do you have any idea if and, in case of yes, when you can update the pub dev packages with your changes ?

@stargazing-dino
Copy link

Our team has experienced challenges with both flutter_blue(_plus) and flutter_reactive_ble. While flutter_blue_plus offers a more understandable codebase, its capabilities are limited and it has been poorly kept up with in the past. On the other hand, flutter_reactive_ble is reliable for developing real-time and robust applications. I really like their API and the stream-based implementations were easy to use. However, some long-standing issues have remained unaddressed, which is understandable considering it's an open-source project.

Recently, I've been exploring flutter_btleplug as a potential final Bluetooth solution for our projects. Although still heavily under development, the necessary tooling is already in place, and the parent project btleplug has a large user base and a good maintainer.

Just some random thoughts I had lol sorry

@chipweinberger
Copy link
Owner Author

chipweinberger commented May 2, 2023

@Sajonji I am still waiting on access to pub.dev. I don't plan to update it right away. I want to integrate & test the changes in my app fully. Feel free to pull master and let me know if you have issues. I have none so far.

@Nolence I totally agree. flutter_reactive_ble has way too many classes & unnecessary abstraction. The Swift code is incomprehensible. flutter_btleplug looks interesting. But needing a rust toolchain is way too much complexity IMO. Debugging and maintenance of a rust codebase would be pretty annoying. But hopefully it "just works".

I like the simplicity of flutter_blue, which is why I just plan to make improvements as needed.

Our team has experienced challenges with both flutter_blue(_plus) and flutter_reactive_ble.

Anything specific?

@stargazing-dino
Copy link

stargazing-dino commented May 8, 2023

Yeah, our issues have mostly come from connecting and disconnecting reliably (repeatedly) with flutter_blue(_plus). Another issue we ran into was auto fragmentation of packets. In flutter_blue that's mostly taken care of I think. In flutter_reactive_ble I don't think that happens based on some tests I ran on an older iphone and android device. Flutter_reactive_ble also doesn't have support for manually messing with a characteristic's descriptors which we would like for some odd pieces of hardware we have.

The Swift code is incomprehensible.

I think their code is a little funky too but tbh I think I prefer it compared to the Obj-C in flutter_blue which is really tough to read for me personally haha - just my personal preference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants