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

support gattlib as an alternative btle library #48

Merged
merged 5 commits into from
Mar 9, 2022
Merged

support gattlib as an alternative btle library #48

merged 5 commits into from
Mar 9, 2022

Conversation

helmutg
Copy link
Contributor

@helmutg helmutg commented Mar 3, 2022

Thank you for writing this library. I found using it with bluepy didn't work well for me:

  • The use of an extra process bluepy-helper reduces performance and reliability.
  • bluepy uses a little strange build system for building its bluepy-helper.
  • bluepy is not included in Debian.

As an alternative, gattlib provides roughly the same functionality. Indeed, eq3bt is written so well that replacing the connection class is a relatively simple matter. As such I propose adding another implementation of it such that it can be used with either bluepy (default) or gattlib. What do you think?

@rytilahti
Copy link
Owner

rytilahti commented Mar 3, 2022

Just a quick note, I think it would make more sense to convert the library to use bleak (which also supports windows and mac) and maybe remove the bluepy support altogether after that.

@helmutg
Copy link
Contributor Author

helmutg commented Mar 3, 2022

As a longer-term goal, supporting bleak definitely makes sense. However now, gattlib is widely available (https://repology.org/project/pygattlib/versions). And given that eq3bt has a good abstraction of the underlying connection object, this additional support has a small maintenance cost.

@rytilahti
Copy link
Owner

Fair enough, I think we can add gattlib support but we need to figure out how to mark that either of the two is required as a dependency. And maybe make it configurable, in case someone prefers to use the gattlib even when bluepy is installed.

@helmutg
Copy link
Contributor Author

helmutg commented Mar 3, 2022

You already made it configurable! The Thermostat constructor takes a connection_cls parameter. That still leaves the issue of expressing the dependency, which I think is mostly unfixable, but maybe I'm wrong.

@rytilahti
Copy link
Owner

Ah, indeed! So the way to make it configurable would work like follows:

  1. Default connection_cls parameter to None in Thermostat
  2. If Thermostat constructor gets None as the class, import the default implementation inside the constructor and use it.
  3. Add a new option to the cli (maybe --backend?) that creates and passes the wanted class to Thermostat: https://github.com/rytilahti/python-eq3bt/blob/master/eq3bt/eq3cli.py#L30
  4. Add the library required for pygatt backend as optional dependency (see https://python-poetry.org/docs/pyproject/#extras)

@helmutg
Copy link
Contributor Author

helmutg commented Mar 6, 2022

I implemented the requested changes. Do you want me to resolve conflicts? If yes, via merge or rebase?

@rytilahti
Copy link
Owner

Looking good, thanks! It doesn't matter whether you do a rebase or merge, I personally usually just do a rebase to avoid merge commits for PRs, and then squash-merge to master when ready.

Fixes: 4e19f27 ("Use poetry, add pre-commit hooks & mass format to modern standards, add CI (#47)")
The default should continue being bluepy, but the import of the
connection class can be avoided when a connection class is passed.
@helmutg
Copy link
Contributor Author

helmutg commented Mar 9, 2022

I rebased the branch on master. master introduced a small bug that is fixed as an additional commit here. Hope that's ok.

@rytilahti
Copy link
Owner

That's fine, the test suite is rather lackluster so I didn't spot that difference between the custom click-datetime and the now-standard one :| Please run pre-commit run -a (or just black, as that seems to be the cause for linter failure) and this is ready to go.

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @helmutg! 🥇

@rytilahti rytilahti merged commit 72d831b into rytilahti:master Mar 9, 2022
@rytilahti rytilahti mentioned this pull request Jul 13, 2022
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.

2 participants