basnijholt / miflora

☘️🌑🌼πŸ₯€πŸ‘ Mi Flora Plant sensor Python package
MIT License
366 stars 98 forks source link

Refactoring to use bluepy instead of gatttool #23

Closed ChristianKuehnel closed 6 years ago

ChristianKuehnel commented 7 years ago

As discusses in issue https://github.com/open-homeautomation/miflora/issues/22

A proposal for using bluepy instead of gatttool. This will probably sill require some work before being ready for production use.

The demo.py is working for my setup.

Things to be done/checked:

ChristianKuehnel commented 7 years ago

@open-homeautomation : After sleeping on it, I think I found a proper control flow for the class (see latest commits). I also cleaned up the code to make it easier to read and understand.

The only thing that I did not implement is the locking feature as I'm not sure that this class needs to be thread safe. You need to convince me, that we will actually need this feature...

I'm not happy with how the user gets the data from the miflora.poller. Basically I would expect to read the measurements via some properties and not a mix of functions. But that would require an incompatible change of the public interface...

joostwestra commented 7 years ago

Does going to bluepy not make the ability to run this across platforms worse? I personally would like the same script to be compatible with Linux and Windows if possible.

ghost commented 7 years ago

Unfortunately I don't know any cross-platform Bluetooth library. Is there one?

BTW: I will be a away for some time and won't find the time to test this pull request. That does not mean, I'm ignoring it. It will just take some time...

joostwestra commented 7 years ago

http://christopherpeplin.com/2015/10/22/pygatt-python-bluetooth-low-energy-ble/ "It currently supports any BLE adapter compatible with BlueZ in Linux, and any BGAPI-compatible adapter on any platform."

ChristianKuehnel commented 7 years ago

I added some unit tests and also implemented a locking feature to be thread safe.

From looking at the code in home assistant's sensor.miflora, it looks like we really need that locks to account for concurrent access to the poller and to the bluetooth hardware.

@joostwestra The API of https://github.com/peplin/pygatt does not look much different from bluepy. Did you try it out? Does it work on Windows?

joostwestra commented 7 years ago

@ChristianKuehnel I ordered a BLED112 so that I can test it under Windows. (Could not find that many BGAPI compatible dongles.)

AnderssonPeter commented 7 years ago

@joostwestra i have been able to get this to work under windows without using a BLED112 dongle, i just wrote a program that emulated gatttool you can find it here, its not a complete port of gatttool, i only ported exactly the parts needed for miflora integration.

My solution should work with most bluetooth adapters, but its limited to Windows 10, might work on 8 and 8.1, but sadly it won't work on windows 7 as the api's i use didn't exist back then.

hifiberry commented 7 years ago

@AnderssonPeter Cool! @ChristianKuehnel I guess, BluePy isn't the right way to move forward as it is Linux only. As Peter created a gatttool replacement for Windows, I would like to stay with Gatttool as long as there is no real multi-platform library library.

Did somebody look into pygatt?

AnderssonPeter commented 7 years ago

@hifiberry One minor change has to be done manually as os.setsid dosen't exist on Windows. So when running under windows change from

            with Popen(cmd,
                       shell=True,
                       stdout=PIPE,
                       preexec_fn=os.setsid) as process:

to

            with Popen(cmd,
                       shell=True,
                       stdout=PIPE) as process:

Im no python programmer otherwise i would create a pull request with a better solution that is cross platform compatible. (i have described this issue in #28.)

ChristianKuehnel commented 7 years ago

I'll have a look at pygatt over Easter. When I have something with it on Linux, I'll let you know.

ChristianKuehnel commented 7 years ago

pygatt supports who backends:

  1. The BlueGigaAPI backend works with special bluetooth adapters from BlueGiga that support the serial protocol. This does not work the the usual adapters you can buy everywhere.
  2. The GATTTool Backend is basically the same thing that is implemented here already.

@open-homeautomation: now it's up for you to decide in which direction you want to go:

  1. bluepy: Having a nice API, with good error handling and fast responses but no Windows support
  2. pygatt BlueGigaAPI: Only works with special bluetooth adapters, but works on Windows
  3. pygatt GATTTool: also based on gatttool, but you could get rid of the parsing/command line handling as this is done by the library already,
  4. current sollution

For 3. and 4. you can get Windows support with the emulator from @AnderssonPeter

joostwestra commented 7 years ago

@ChristianKuehnel Looks good, and would cover most of our needs. However if I run hcitool or gattool I do not get any results (could be user error). Tried to debug it with visual studio but having problem it in a debug session.

AnderssonPeter commented 7 years ago

@joostwestra are you using my tools? what exit code does it give you? 0 = Success, 1 = Error.

joostwestra commented 7 years ago

@AnderssonPeter Yes I am trying to.

After some reference fixing I am able to get this far: "Enumerating devices Done The program '[5588] hcitool.exe' has exited with code 0 (0x0)."

:"DeviceWatcher_Added" never triggers.

"DeviceWatcher_EnumerationCompleted" triggers very fast.

AnderssonPeter commented 7 years ago

@joostwestra try to add the device in windows bluetooth gui first and see if that helps.

joostwestra commented 7 years ago

@AnderssonPeter I could see the device I was testing with in the UI, but can not pair. Took a brand new device -> paired in the UI. And looks to trigger the event now in Visual studio. Thank you.

ChristianKuehnel commented 7 years ago

OK, I did not really see a comment in which direction we should take this topic. I still see the same options: https://github.com/open-homeautomation/miflora/pull/23#issuecomment-294335304

In addition we could do something like:

  1. Support different Bluetooth backend on different platforms and let the user decide what he wants. But I'm not sure if that's work the extra complexity and effort.
Chris-V commented 7 years ago

What the status on this?

gatttool is being phased out in the latest BlueZ releases. Other tools are recommended, namely btgatt-client and btmgmt.

I'm running Arch on a Pi 3 and the HCI tools are no longer part of the bluez-utils package. They are still available in the AUR legacy package, but this package is not ported to arm. I believe other distros will face a similar issue when the newer BlueZ packages get released to them.

That said, the latest release of bluepy seems to be compatible with those tools (according to the doc, did not try it). On the other hand, pygatt has a pending pull request to support a dbus backend: https://github.com/peplin/pygatt/pull/95 (last activity on Apr 22).

Although my knowledge of those libraries is pretty limited, I could give a hand here.

ghost commented 7 years ago

This hasn't merged as it was a very complex pull request. If you want to implement a new backend, I would recommend to simply implement the functions "read_ble" and "write_ble" based on another bluetooth library. Having pluggable backends is in general a good thing as it might improve support for different operating systems as long as there is no real multi-platform bluetooth library.

Chris-V commented 7 years ago

Thank you for the pointers. I will have a look at this in the coming days. No promises on a working implementation though.