Re: [PATCH 2/5] Change CreatePairedDevice to support LE devices
From: Johan Hedberg <hidden>
Date: 2010-12-16 09:24:32
Hi, On Wed, Dec 15, 2010, Claudio Takahasi wrote:
CreatePairedDevice implements now the same behaviour of CreateDevice, triggering Discover All Primary Services when needed. SMP negotiation starts when the link is established. LE capable kernel is required to test this method properly. Limitation: For dual mode devices, Discover All Primary Services is not being executed after SDP search if GATT record is found. --- src/adapter.c | 46 ++++++++++++++++++++++++--- src/device.c | 89 +++++++++++++++++++++++++++------------------------- src/device.h | 7 +++- src/glib-helper.c | 5 ++- src/glib-helper.h | 3 ++ 5 files changed, 98 insertions(+), 52 deletions(-)
Couple of issue here:
quoted hunk ↗ jump to hunk
@@ -1642,6 +1646,8 @@ static DBusMessage *create_paired_device(DBusConnection *conn, struct btd_device *device; const gchar *address, *agent_path, *capability, *sender; uint8_t cap; + device_type_t type; + int err; if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &address, DBUS_TYPE_OBJECT_PATH, &agent_path,@@ -1666,12 +1672,40 @@ static DBusMessage *create_paired_device(DBusConnection *conn, if (cap == IO_CAPABILITY_INVALID) return btd_error_invalid_args(msg); - device = adapter_get_device(conn, adapter, address); - if (!device) - return btd_error_failed(msg, - "Unable to create a new device object"); + device = adapter_find_device(adapter, address); + if (!device) { + struct remote_dev_info *dev, match; + + memset(&match, 0, sizeof(struct remote_dev_info)); + str2ba(address, &match.bdaddr); + match.name_status = NAME_ANY; + + dev = adapter_search_found_devices(adapter, &match); + if (dev && dev->flags) + type = flags2type(dev->flags); + else + type = DEVICE_TYPE_BREDR; + + if (type == DEVICE_TYPE_LE && + !event_is_connectable(dev->evt_type)) + return btd_error_failed(msg, + "Device is not connectable"); + + device = adapter_create_device(conn, adapter, address, type); + if (!device) + return NULL; + } else + type = device_get_type(device); + + if (type != DEVICE_TYPE_LE) + return device_create_bonding(device, conn, msg, + agent_path, cap); - return device_create_bonding(device, conn, msg, agent_path, cap); + err = device_browse_primary(device, conn, msg, BT_IO_SEC_HIGH); + if (err < 0) + return btd_error_failed(msg, strerror(-err)); + + return NULL; }
I don't really like the way this makes the create_paired_device function quite long. Could you maybe refactor the if (!device) branch into a separate function?
quoted hunk ↗ jump to hunk
diff --git a/src/device.h b/src/device.h index 784e931..cafa529 100644 --- a/src/device.h +++ b/src/device.h@@ -24,6 +24,8 @@ #define DEVICE_INTERFACE "org.bluez.Device" +#include "btio.h" +
Includes should be the first thing after the copyright/license comments in the file. However, to keep a clear visibility of potential circular dependencies Marcel has requested this kind of inclusion of an internal header file from within an internal header file to be avoided. Instead make sure you include btio.h from early enough in the respective .c file. You could also reconsider if you really need BtIOSecLevel here. Maybe a "gboolean secure" flag would be enough?
quoted hunk ↗ jump to hunk
--- a/src/glib-helper.h +++ b/src/glib-helper.h@@ -21,6 +21,8 @@ * */ +#include "btio.h" +
Same here. I'm feeling a little bit ambivalent about your additions to glib-helper.c. It never had a clearly defined scope and I had been hoping to get rid of it completely. However now it seems you guys are constantly adding new stuff there. Is it really so that you can't find a more specific location for these functions? Could you describe in one or two sentences the purpose and scope that you think the glib-helper.c functions have? Johan