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

feat: add linux platform #969

Closed
wants to merge 5 commits into from
Closed

feat: add linux platform #969

wants to merge 5 commits into from

Conversation

tnc1997
Copy link
Collaborator

@tnc1997 tnc1997 commented Aug 16, 2024

#6

@chipweinberger
Copy link
Owner

looks pretty well done.

please test extensively.

@tnc1997
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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

@chipweinberger
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

MTU updates usually happen by calling requestMtu.

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

@chipweinberger
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

tnc1997 commented Aug 16, 2024

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.

@tnc1997 tnc1997 closed this Aug 16, 2024
@tnc1997 tnc1997 deleted the feat/6 branch August 16, 2024 21:22
@tnc1997 tnc1997 mentioned this pull request Aug 16, 2024
@chipweinberger
Copy link
Owner

chipweinberger commented Aug 16, 2024

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
Copy link
Collaborator Author

tnc1997 commented Aug 17, 2024

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
Copy link
Owner

chipweinberger commented Aug 17, 2024

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

@boskokg
Copy link
Contributor

boskokg commented Aug 17, 2024

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 [email protected]

@tnc1997
Copy link
Collaborator Author

tnc1997 commented Aug 17, 2024

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.

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

Successfully merging this pull request may close these issues.

3 participants