chipweinberger / flutter_blue_plus

Flutter plugin for connecting and communicationg with Bluetooth Low Energy devices, on Android, iOS, macOS
Other
768 stars 465 forks source link

feat: add linux platform #969

Closed tnc1997 closed 2 months ago

tnc1997 commented 2 months ago

6

chipweinberger commented 2 months ago

looks pretty well done.

please test extensively.

tnc1997 commented 2 months ago

Thank you very much for your feedback, I have updated the example and tested using this.

chipweinberger commented 2 months ago

a few things worth checking

  1. canceling an in-progress connection attempt, does it work? what happens (i.e. calling disconnect(queue:false))
  2. mtu updates. how do we know the mtu?
  3. i dont see you calling OnConnectionStateChanged if the device loses connection unexpectedly
  4. I dont see support for continuous_divisor
  5. I dont see autoconnect support
  6. BmScanAdvertisement connectable: true, is this really impossible on linux?
  7. how does adapterState work? I dont see OnAdapterStateChanged called anywhere
  8. We don't need a try/catch for every individual method call. Just a single one around all of it, just like on iOS & Android. The function name, and Platform.linux is added in the dart layer above, e.g. BluetoothCharacteristic.write
  9. setLogLevel support should be a no-op if you can't add support on the platform side. Not an exception.
  10. setOptions should be a no-op
  11. how does bonding work on linux? add documentation please
  12. bluez: ^0.8.0 please use the latest bluez: 0.8.2. And I think we should probably pin it to a specific version. i.e. remove ^, this way users don't need to separately specify their BlueZ version when filing GitHub issues.

That's all I can think of for now. It's overall good code.

You might want to open a separate PR to update the example app with more functionality so you can test it:

  1. optional autoconnect support
  2. disconnect(queue:false) support, i.e. a show cancel button as the CircularProgressIndicator is spinning during connection

Keep in mind, I don't want a "half complete" implementation merged, because it will cause a lot of new Github issues. It needs to be a full implementation.

Federated Plugin

I've avoided it for awhile, but now that you are opening PRs for multiple platforms, I think we should switch to Federated.

  1. you can open a PR to switch this repo to federated
  2. you can maintain the Linux & Web packages yourself.
  3. I will "endorse" your packages.

Thoughts?

chipweinberger commented 2 months ago

The more I think about it, I think you should create a new package: flutter_blue_gold or something.

Backwards compatible with flutter_blue_plus and adds support for all platforms.

tnc1997 commented 2 months ago

A few things worth checking

  1. I will double check this using the example and test the outcome.
  2. From what I gather the mtu is not returned as part of the advertisement data in the platform interface.
  3. Good catch, I will investigate if this is possible, and add support if so.
  4. After doing a cursory search of the BlueZ repository this does not yield any results.
  5. After doing a cursory search of the BlueZ repository this does not yield any results.
  6. Good catch, I will investigate if this is possible, and add support if so.
  7. Should we listen to adaptor state changes throughout the entire lifetime of the plugin from start to finish?
  8. I will refactor the try/catch blocks into a single try/catch block accordingly.
  9. I will refactor setLogLevel to remove the exception.
  10. I will refactor setOptions to remove the exception.
  11. From what I gather, bonding and pairing are slightly different concepts, but I may be incorrect.
  12. Should we not make the dependency as loose as possible to allow for maximum compatibility with consumers?

I don't want a "half complete" implementation merged

I have put time and effort into these implementations and believe that they offer much of the compatible functionality, it is unfortunate that you feel that they are "half complete". If you think that there are areas of functionality missing that could be implemented, then I would like to work with you to get those missing pieces of functionality implemented.

chipweinberger commented 2 months ago

Really appreciate all your work Thomas.

I have put time and effort into these implementations and believe that they offer much of the compatible functionality, it is unfortunate that you feel that they are "half complete". If you think that there are areas of functionality missing that could be implemented, then I would like to work with you to get those missing pieces of functionality implemented.

Oh sorry, I do not mean to say yours is half complete. It's quite good. I just mean to say it needs to have all the edge cases considered before we merge :)

tnc1997 commented 2 months ago

I think we should switch to Federated.

I agree that a federated architecture makes more sense with multiple platforms.

I am more than happy to attempt to convert the package to a federated architecture but I may need your assistance.

chipweinberger commented 2 months ago

From what I gather the mtu is not returned as part of the advertisement data in the platform interface.

MTU updates usually happen by calling requestMtu. iOS is special in that they dont allow calling requestMtu and call it silently. But I would think linux allows it.

tnc1997 commented 2 months ago

You can maintain the Linux & Web packages yourself

This is something that I would be more than happy to do, including handling any issues arising from these platforms.

Something along the lines of flutter_blus_plus_linux and flutter_blue_plus_web respectively for example?

tnc1997 commented 2 months ago

MTU updates usually happen by calling requestMtu.

Ah, that makes sense, apologies for my naivety, I will endeavour to implement that method.

chipweinberger commented 2 months ago

RE: Federated

The more I think about this, these are big changes, especially federated.

I think you should consider a new package, flutter_blue_universal

You can use federated if you want.

  1. It will be backwards compatible with flutter_blue_plus and adds support for all platforms.
  2. I'll link to your package in my readme.
  3. we'll see how it goes, in terms of adoption, bug reports, etc
  4. maybe eventually people will migrate to your package, or maybe eventually I'll merge your changes and we can continue from this repo

There is a big problem, in that @boskokg may be deceased. I have not heard from him in almost a year, and have not been able to contact him. Without him, we cannot add more maintainers on GitHub. This is why we should probably switch to a new github repo anyway.

chipweinberger commented 2 months ago

For your additional questions:

  1. autoConnect is special on android, but on iOS it is just handled in the Dart layer. It just means we always try to reconnnect, forever. I think we'll handle in dart layer as well for linux. (iOS 17 did add proper autoconnect support, but I don't know exactly what it does)

  2. yes, listen to adapter state the whole life cycle

  3. continuous_divisor is just handled in FBP code. It's own own concept, to tackle some perf issues.

tnc1997 commented 2 months ago

I think you should consider a new package, flutter_blue_universal

I'll link to your package in my readme

Would it potentially be better to link to one of the other existing cross-platform packages, instead of creating another new cross-platform package? I agree that moving to a federated architecture would be a significant change, but we could version it as a breaking change of flutter_blue_plus to avoid creating another new Bluetooth package.

There is a big problem, in that @boskokg may be deceased

I am sorry to hear this and I am not sure what the process may be to transfer ownership of packages in this scenario.

chipweinberger commented 2 months ago

Would it potentially be better to link to one of the other existing cross-platform packages,

I've looked, and they all have a very different user facing API. So there is demand for a cross platform package with the FBP api.

That said, I'm going to add links to them anyway. The best I know of are: https://pub.dev/packages/bluetooth_low_energy, https://pub.dev/packages/quick_blue, https://pub.dev/packages/universal_ble

transfer ownership

I do have control of the pub.dev package, just not the Github repo.

tnc1997 commented 2 months ago

So there is demand for a cross platform package with the FBP api

I think that it would potentially make more sense for cross-platform support to be added to the flutter_blue_plus package directly, instead of creating a second package with an identical API that is cross-platform, but I understand if you would rather keep the flutter_blue_plus package as Android/iOS/macOS only using its current architecture.

chipweinberger commented 2 months ago

Yes, to be honest, I'm about to take a very long vacation and potentially switch careers.

So I don't have the bandwidth to handle these extra platforms. I plan to maintain ios/android, but I don't see myself doing big active development on this package.

I strongly recommend a new package, and I'll link to it as a successor, when it is ready.

chipweinberger commented 2 months ago

To say a few more words,

flutter_blue has long been the most popular BLE library for flutter.

I've moved it along a lot in this past year, with flutter_blue_plus

But it's ready for another person to take it to the next level: all platforms.

a new package makes most sense IMO.

flutter_blue_plus will be in maintenance mode for the foreseeable future.

tnc1997 commented 2 months ago

I would be happy maintaining packages for currently unimplemented platforms, but am not comfortable maintaining an entire flutter_blue_plus fork with a duplicate API, however thank you for reviewing these changes and enjoy your vacation! Feel free to reach out if you ever decide to adopt a federated architecture and maybe we can work together.

chipweinberger commented 2 months ago

what is your plan for this code, out of curiosity?

yes, maintaining so many platforms is a lot of work 😅

so federated makes sense.

FBP is still the most popular, well tested, feature rich, so I do hope someone takes it forward to other platforms.

tnc1997 commented 2 months ago

what is your plan for this code, out of curiosity?

I was hoping to use flutter_blue_plus in a cross-platform application that I am currently developing.

so I do hope someone takes it forward to other platforms.

I would be happy to help federate the current package but have no interest publishing a separate federated clone.

Federating the current package would allow third-parties (such as myself) to create non-endorsed (or endorsed if you choose) implementations for other platforms without additional maintenance or support overhead from yourself.

chipweinberger commented 2 months ago

if you federate the current package, ill probably merge it, assuming its not that hard to do.

boskokg commented 2 months ago

Hello. Just to say that I changed the company and I am not using ble for a long time. If you want, I can add contributors or switch ownership to someone else. Sorry for the late reply. Please contact me at boskokg@gmail.com

tnc1997 commented 2 months ago

if you federate the current package, ill probably merge it, assuming its not that hard to do

Thank you very much for agreeing to this (difficulty permitting).

I will attempt to federate the current package, using the existing method channel as the default implementation.