Refactoring to support different backends

basnijholt / miflora

☘️🌡🌼🥀🏡 Mi Flora Plant sensor Python package

MIT License

363 stars 98 forks source link

Refactoring to support different backends #39

Closed ChristianKuehnel closed 6 years ago

ChristianKuehnel commented 6 years ago

Larger refactoring to be able to introduce additional Bluetooth LE backends (e.g. bluepy).

This change is based on the discussion in #23 I moved everything related to gatttool into a separate module and class. I tried to change as little as possible to the structure and flow of the code.

The solution is not properly tested. This is a first proposal to discuss the general ideas of the new architecture.

Please review the change and let met know if you like the new architecture and if I should proceed with it.

Note: This change will break the API! You now have to set the backend when creating a new instance of MiFloraPoller.

ChristianKuehnel commented 6 years ago

@open-homeautomation @joostwestra @AnderssonPeter @hifiberry @chris-v

Any feedback on this proposal? You were involved in the discussion in #23 ...

ThomDietrich commented 6 years ago

(I'm traveling currently) We should discuss s more state-complete interface.

ChristianKuehnel commented 6 years ago

@ThomDietrich Yes let's discuss when you're back. I'm not going to merge anything anytime soon. I do have some ideas I would like to try...

ChristianKuehnel commented 6 years ago

Hey @ThomDietrich

So now I got the changes in I had in mind. When you're back home, let me know what you think.

What did I do:

The new class BluetoothInterfac is now using a context manager to take care of all the locking/unlocking and connection handling. This should be easier than the connect/disconnect operations.
I added a "check_backend()" function to check if the backend is available on this system.
I wanted to add a "scan for devices" feature, but that seems to be a bit difficult as "hcitool lescan" requires root permissions.
Added a tox and pytest configuration to deal with automated tests.
Added a few tests that helped me to test my changes and debug my problems.

ThomDietrich commented 6 years ago

Hey, I've looked at the latest commit (and the combination of all) and am pretty happy about this PR. That looks like a well thought through design. The introduction of check_backend also satisfies that part in PR #36 (minor detail: "check prerequisites" might have been clearer in it's self-description, in general the descriptions could be longer). The reimplementation looks good. I also thought about the "scan for devices" feature and the root requirement. Is that really an issue? If we catch the non-root error and throw an appropriate exception, the application on top can decide what the next step should be. With the above I think you have reached a good result and we should consider the merge.

Next up for me would be the remaining part of PR #36 and an open discussion to switch over to a line length of 120 and a better indention scheme, to get rid of the unpleasant spots in both the backend and poller classes.

Best! Thomas

ChristianKuehnel commented 6 years ago

Hi Thomas,

thx for the feedback, I just merged the branch.