Hi Johan,
On Thu, Dec 16, 2010 at 6:24 AM, Johan Hedberg <johan.hedberg@xxxxxxxxx> wrote:
> 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:
>
>> @@ -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?
ok. We will try. Maybe it will be possible to create a common function
to move shared code between CreateDevice and CreatePairedDevice.
>
>> 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?
For now a boolean is enough, we can change it to boolean.
>
>> --- 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
>
Currently, the purpose are service search functions and UUIDs utility functions.
glib-helper was originally created to implement some utility functions
to manage connections and sdp search abstractions.
Connection functions were moved/removed when btio was created. I have
two suggestions to try cleanup the code:
1. keep only sdp functions that use GLib types and rename the file to
gsdp or other convenient name
2. Or create a btd_device_search/cancel functions(moving them to
device.c) and try to remove glib-helper from the source tree, in the
worst case keep only functions to manipulate UUIDs
The bt_discover_primary can be moved to gatt.c if we split the
discover cancel function.
bt_discover_services() can be removed, there isn't reference in code.
Which approach do you prefer? Any other suggestion?
Regards,
Claudio
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
