rytilahti / python-eq3bt

Python library and command-line tool for eQ-3 Smart Bluetooth thermostats
MIT License
115 stars 36 forks source link

support gattlib as an alternative btle library #48

Closed helmutg closed 2 years ago

helmutg commented 2 years ago

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

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

rytilahti commented 2 years ago

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.

helmutg commented 2 years ago

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

rytilahti commented 2 years ago

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.