Skip to content

support connect/disconnect, support multiple callbacks #39

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.

Sign up for GitHub

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

Open
fhempy wants to merge 3 commits into
base: master
Choose a base branch
from
Open

support connect/disconnect, support multiple callbacks #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75e34a5
support multiple hci interfaces
fhempy Oct 6, 2020
43e6c5f
remove get adapters
fhempy Oct 8, 2020
75115f9
add disconnect()
fhempy Oct 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 56 additions & 15 deletions eq3bt/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
"""
import logging
import codecs
import dbus
import re

from bluepy import btle

Expand All @@ -15,46 +17,83 @@
class BTLEConnection(btle.DefaultDelegate):
"""Representation of a BTLE Connection."""

def __init__(self, mac):
def __init__(self, mac, keep_connected=False):
"""Initialize the connection."""
btle.DefaultDelegate.__init__(self)

self._ifaces = self.get_hci_ifaces()
self._iface_idx = 0

self._conn = None
self._mac = mac
self._callbacks = {}
self._keep_connected = keep_connected

def next_iface(self):
self._nr_conn_errors += 1
self._iface_idx = (self._iface_idx + 1) % len(self._ifaces)
if self._nr_conn_errors >= len(self._ifaces)*2:
return False
return True

def get_hci_ifaces(self):
iface_list = []
bus = dbus.SystemBus()
manager = dbus.Interface(bus.get_object("org.bluez", "/"), "org.freedesktop.DBus.ObjectManager")
objects = manager.GetManagedObjects()
for path, interfaces in objects.items():
adapter = interfaces.get("org.bluez.Adapter1")
if adapter is None:
continue
iface_list.append(re.search(r'\d+$', path)[0])
return iface_list

@rytilahti rytilahti Oct 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This would introduce hard dependency for dbus (which may or may not be available on all environments), so it's a no go. Could you please create a separate PR for adding optional support for multiple interfaces?

Also, are you sure bluepy does not provide an interface for enumerating over existing adapters? That would be the preferred way over manually scraping bluez responses.

@fhempy fhempy Oct 6, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Unfortunately bluepy doesn't provide such a functionality, that was the first place I searched for. I could find this one from bluez which is the functionality I implemented:
https://github.com/pauloborges/bluez/blob/master/test/test-adapter

Another possible solution would be to remove this one from eq3bt and add an additional parameter "iface".

@rytilahti rytilahti Oct 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hmm, I think the best solution is to refactor the connection handling (so that connect can be done separately outside the __enter__) and expose iface parameter for it as you propose. The eq3bt cli tool could also optionally expose --interface option, and the downstream users can use whatever they want (like dbus) to locate available interfaces.

@fhempy fhempy Oct 6, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Just to be sure that we are on the same page:

  • add connect(iface) method
  • add iface parameter to Thermostat
  • remove dbus and get adapters completely

@rytilahti rytilahti Oct 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sounds good to me!

  • connect(iface) will be useful for your use case, where you want to fall back to other interfaces if the connection fails
  • Adding optional iface to Thermostat allows forcing to use specific controller (e.g., via cli) during the initialization phase, in case someone wants to do that.

The real fix for obtaining interfaces should be done inside bluepy, but I think the library is not very actively maintained, notwithstanding the problem with dbus availability (iirc there was also some problems getting that library easily pip-installable due to some compiling needed during the setup process). Last time I did some dbus interfacing, I used python-dbus-next which is pure python only library.


def __enter__(self):
"""
Context manager __enter__ for connecting the device
:rtype: btle.Peripheral
:return:
"""
self._conn = btle.Peripheral()
self._conn.withDelegate(self)
_LOGGER.debug("Trying to connect to %s", self._mac)
try:
self._conn.connect(self._mac)
except btle.BTLEException as ex:
_LOGGER.debug("Unable to connect to the device %s, retrying: %s", self._mac, ex)
try:
self._conn.connect(self._mac)
except Exception as ex2:
_LOGGER.debug("Second connection try to %s failed: %s", self._mac, ex2)
raise
conn_state = self._conn.getState()
except:
self._conn = None

@rytilahti rytilahti Oct 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This will lose the reference to the potentially already existing _conn object, is there something else that should be done in cases where getState errors on already existing connection?

Anycase, please don't use bare excepts but catch only exceptions you are expecting.

@fhempy fhempy Oct 6, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, I can change that to catch only the errors which indicate that the connection was already lost and therefore a disconnect() call isn't needed.


if self._conn is None or conn_state != "conn":
self._conn = btle.Peripheral()
self._conn.withDelegate(self)
self._nr_conn_errors = 0
_LOGGER.debug("Trying to connect to %s", self._mac)
while True:
# try to connect with all ifaces
try:
self._conn.connect(self._mac, iface=self._ifaces[self._iface_idx])
break
except btle.BTLEException as ex:
_LOGGER.debug("Unable to connect to the device %s using iface %s, retrying: %s", self._mac, self._ifaces[self._iface_idx], ex)
try:
self._conn.connect(self._mac, iface=self._ifaces[self._iface_idx])
break
except Exception as ex2:
_LOGGER.debug("Second connection try to %s using ifaces %s failed: %s", self._mac, self._ifaces[self._iface_idx], ex2)
if self.next_iface() is False:
# tried all ifaces, raise exception
raise

@rytilahti rytilahti Oct 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This structure feels very error-prone, I think the correct way would be refactoring the connection code away from __enter__ (it's not really exposed anywhere), and create separate connect() method that does the connection using the given iface.

@fhempy fhempy Oct 6, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That makes sense. I'll refactor and add a connect() method with iface as parameter.


_LOGGER.debug("Connected to %s", self._mac)
return self

def __exit__(self, exc_type, exc_val, exc_tb):
if self._conn:
if self._conn and self._keep_connected is False:
self._conn.disconnect()
self._conn = None

def handleNotification(self, handle, data):
"""Handle Callback from a Bluetooth (GATT) request."""
_LOGGER.debug("Got notification from %s: %s", handle, codecs.encode(data, 'hex'))
if handle in self._callbacks:
self._callbacks[handle](data)
for callback in self._callbacks[handle]:
callback(data)

@property
def mac(self):
Expand All @@ -63,7 +102,9 @@ def mac(self):

def set_callback(self, handle, function):
"""Set the callback for a Notification handle. It will be called with the parameter data, which is binary."""
self._callbacks[handle] = function
if handle not in self._callbacks:
self._callbacks[handle] = []
self._callbacks[handle].append(function)

def make_request(self, handle, value, timeout=DEFAULT_TIMEOUT, with_response=True):
"""Write a GATT Command without callback - not utf-8."""
Expand Down
4 changes: 2 additions & 2 deletions eq3bt/eq3btsmart.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class TemperatureException(Exception):
class Thermostat:
"""Representation of a EQ3 Bluetooth Smart thermostat."""

def __init__(self, _mac, connection_cls=BTLEConnection):
def __init__(self, _mac, connection_cls=BTLEConnection, keep_connection=False):
"""Initialize the thermostat."""

self._target_temperature = Mode.Unknown
Expand All @@ -94,7 +94,7 @@ def __init__(self, _mac, connection_cls=BTLEConnection):
self._firmware_version = None
self._device_serial = None

self._conn = connection_cls(_mac)
self._conn = connection_cls(_mac, keep_connection)
self._conn.set_callback(PROP_NTFY_HANDLE, self.handle_notification)

def __str__(self):
Expand Down