Hi,
The move itself is pretty simple, but when implementing it, I took a decision
and I would like to hear your opinion about it.
With this patch series, the behaviour is that we never disconnect from
a LE device unless we are told so, i.e. org.bluez.Device.Disconnect(),
is that what is expected from us?
Cheers,
--
Vinicius Costa Gomes (6):
Move Attrib client to the core
Register Attrib interface when loading device from storage
Register Attrib interface after Primary Service discovery
Remove _init and _exit methods from Attrib client
Add support for re-using the attrib channel
Fix disconnecting when primary service discovery is done
Makefile.am | 4 ++--
attrib/client.c | 49 ++++++++++++++++++-------------------------------
attrib/client.h | 6 +++---
attrib/manager.c | 47 -----------------------------------------------
src/adapter.c | 5 +++++
src/device.c | 20 +++++++++++++-------
6 files changed, 41 insertions(+), 90 deletions(-)
--
1.7.4.1
Hi Claudio & Vinicius,
> >> The move itself is pretty simple, but when implementing it, I took a decision
> >> and I would like to hear your opinion about it.
> >
> > There were problems with some useless includes. I will send another version
> > soon. But the question below remains.
> >
> >>
> >> With this patch series, the behaviour is that we never disconnect from
> >> a LE device unless we are told so, i.e. org.bluez.Device.Disconnect(),
> >> is that what is expected from us?
>
> In my opinion, the profiles should define how to manage connections.
> For LE re-connections should be avoided since the remote disables the
> advertising when the connection is established.
> Keep the link up seems to be acceptable for now, but for the final
> solution we need to disconnect based on the registered "watchers" and
> profile inputs. Some profiles such as proximity keeps the devices
> always connected and auto-reconnect, others require user interaction.
Sounds good enough to me, i.e. let's keep "connected until an explicit
disconnect request from the user" the default for now.
Johan
Hi Vinicius,
On Mon, Mar 28, 2011 at 8:33 PM, Vinicius Costa Gomes
<[email protected]> wrote:
> Hi Johan,
>
> On 21:01 Fri 25 Mar, Vinicius Costa Gomes wrote:
>> Hi,
>>
>> The move itself is pretty simple, but when implementing it, I took a decision
>> and I would like to hear your opinion about it.
>
> There were problems with some useless includes. I will send another version
> soon. But the question below remains.
>
>>
>> With this patch series, the behaviour is that we never disconnect from
>> a LE device unless we are told so, i.e. org.bluez.Device.Disconnect(),
>> is that what is expected from us?
In my opinion, the profiles should define how to manage connections.
For LE re-connections should be avoided since the remote disables the
advertising when the connection is established.
Keep the link up seems to be acceptable for now, but for the final
solution we need to disconnect based on the registered "watchers" and
profile inputs. Some profiles such as proximity keeps the devices
always connected and auto-reconnect, others require user interaction.
Cheers,
Claudio.
>>
>>
>> Cheers,
>> --
>>
>> Vinicius Costa Gomes (6):
>> Move Attrib client to the core
>> Register Attrib interface when loading device from storage
>> Register Attrib interface after Primary Service discovery
>> Remove _init and _exit methods from Attrib client
>> Add support for re-using the attrib channel
>> Fix disconnecting when primary service discovery is done
>>
>> Makefile.am | 4 ++--
>> attrib/client.c | 49 ++++++++++++++++++-------------------------------
>> attrib/client.h | 6 +++---
>> attrib/manager.c | 47 -----------------------------------------------
>> src/adapter.c | 5 +++++
>> src/device.c | 20 +++++++++++++-------
>> 6 files changed, 41 insertions(+), 90 deletions(-)
>>
>> --
>> 1.7.4.1
>>
>
> Cheers,
> --
> Vinicius
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Johan,
On 21:01 Fri 25 Mar, Vinicius Costa Gomes wrote:
> Hi,
>
> The move itself is pretty simple, but when implementing it, I took a decision
> and I would like to hear your opinion about it.
There were problems with some useless includes. I will send another version
soon. But the question below remains.
>
> With this patch series, the behaviour is that we never disconnect from
> a LE device unless we are told so, i.e. org.bluez.Device.Disconnect(),
> is that what is expected from us?
>
>
> Cheers,
> --
>
> Vinicius Costa Gomes (6):
> Move Attrib client to the core
> Register Attrib interface when loading device from storage
> Register Attrib interface after Primary Service discovery
> Remove _init and _exit methods from Attrib client
> Add support for re-using the attrib channel
> Fix disconnecting when primary service discovery is done
>
> Makefile.am | 4 ++--
> attrib/client.c | 49 ++++++++++++++++++-------------------------------
> attrib/client.h | 6 +++---
> attrib/manager.c | 47 -----------------------------------------------
> src/adapter.c | 5 +++++
> src/device.c | 20 +++++++++++++-------
> 6 files changed, 41 insertions(+), 90 deletions(-)
>
> --
> 1.7.4.1
>
Cheers,
--
Vinicius
In some cases, when the device is already connected there's no need
to create another GAttrib instance.
This will allow the Attrib client to use the connection already
estabilished, this will be very useful when we support more
LE profiles.
---
attrib/client.c | 6 +++++-
attrib/client.h | 3 ++-
src/adapter.c | 3 ++-
src/device.c | 2 +-
4 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/attrib/client.c b/attrib/client.c
index acd35f9..28e5704 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -1046,7 +1046,8 @@ static void register_primaries(struct gatt_service *gatt, GSList *primaries)
}
int attrib_client_register(DBusConnection *connection,
- struct btd_device *device, int psm, GSList *primaries)
+ struct btd_device *device, int psm,
+ GAttrib *attrib, GSList *primaries)
{
struct btd_adapter *adapter = device_get_adapter(device);
const char *path = device_get_path(device);
@@ -1065,6 +1066,9 @@ int attrib_client_register(DBusConnection *connection,
bacpy(&gatt->dba, &dba);
gatt->psm = psm;
+ if (attrib)
+ gatt->attrib = g_attrib_ref(attrib);
+
register_primaries(gatt, primaries);
gatt_services = g_slist_append(gatt_services, gatt);
diff --git a/attrib/client.h b/attrib/client.h
index 650b0c1..b4a4ecc 100644
--- a/attrib/client.h
+++ b/attrib/client.h
@@ -23,5 +23,6 @@
*/
int attrib_client_register(DBusConnection *connection,
- struct btd_device *device, int psm, GSList *primaries);
+ struct btd_device *device, int psm,
+ GAttrib *attrib, GSList *primaries);
void attrib_client_unregister(struct btd_device *device);
diff --git a/src/adapter.c b/src/adapter.c
index 124252a..0af0877 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -58,6 +58,7 @@
#include "storage.h"
#include "attrib-server.h"
#include "att.h"
+#include "gattrib.h"
#include "attrib/client.h"
/* Flags Descriptions */
@@ -2182,7 +2183,7 @@ static void create_stored_device_from_primary(char *key, char *value,
}
/* FIXME: Need the correct psm */
- attrib_client_register(connection, device, -1, services);
+ attrib_client_register(connection, device, -1, NULL, services);
device_probe_drivers(device, uuids);
diff --git a/src/device.c b/src/device.c
index 511c9d0..fa5764e 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1548,7 +1548,7 @@ static void primary_cb(GSList *services, guint8 status, gpointer user_data)
device_probe_drivers(device, uuids);
/* FIXME: Need the correct psm */
- attrib_client_register(req->conn, device, -1, services);
+ attrib_client_register(req->conn, device, -1, req->attrib, services);
g_slist_free(uuids);
--
1.7.4.1
We shouldn't disconnect when the primary service discovery is done.
Most LE devices leave advertising mode as soon as the connection is
established, so shouldn't assume that we are able to reconnect to
them again.
For that we rely on the fact that GAttrib will close the connection
when its last reference is dropped.
---
src/device.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/device.c b/src/device.c
index fa5764e..d567952 100644
--- a/src/device.c
+++ b/src/device.c
@@ -94,7 +94,6 @@ struct browse_req {
DBusConnection *conn;
DBusMessage *msg;
GAttrib *attrib;
- GIOChannel *io;
struct btd_device *device;
GSList *match_uuids;
GSList *profiles_added;
@@ -166,11 +165,8 @@ static void browse_request_free(struct browse_req *req)
if (req->records)
sdp_list_free(req->records, (sdp_free_func_t) sdp_record_free);
- if (req->io) {
+ if (req->attrib)
g_attrib_unref(req->attrib);
- g_io_channel_unref(req->io);
- g_io_channel_shutdown(req->io, FALSE, NULL);
- }
g_free(req);
}
@@ -1588,6 +1584,8 @@ static void gatt_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
}
req->attrib = g_attrib_new(io);
+ g_io_channel_unref(io);
+
gatt_discover_primary(req->attrib, NULL, primary_cb, req);
}
@@ -1597,6 +1595,7 @@ int device_browse_primary(struct btd_device *device, DBusConnection *conn,
struct btd_adapter *adapter = device->adapter;
struct browse_req *req;
BtIOSecLevel sec_level;
+ GIOChannel *io;
bdaddr_t src;
if (device->browse)
@@ -1609,14 +1608,14 @@ int device_browse_primary(struct btd_device *device, DBusConnection *conn,
sec_level = secure ? BT_IO_SEC_HIGH : BT_IO_SEC_LOW;
- req->io = bt_io_connect(BT_IO_L2CAP, gatt_connect_cb, req, NULL, NULL,
+ io = bt_io_connect(BT_IO_L2CAP, gatt_connect_cb, req, NULL, NULL,
BT_IO_OPT_SOURCE_BDADDR, &src,
BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
BT_IO_OPT_CID, GATT_CID,
BT_IO_OPT_SEC_LEVEL, sec_level,
BT_IO_OPT_INVALID);
- if (req->io == NULL ) {
+ if (io == NULL ) {
browse_request_free(req);
return -EIO;
}
--
1.7.4.1
Now that Attrib client is being moved to the core, there's no
need to have explicit _init and _exit methods.
This also means that we need to pass the DBusConnection explicitly
to the registration method.
---
attrib/client.c | 44 ++++++++++++++------------------------------
attrib/client.h | 5 ++---
src/adapter.c | 2 +-
src/device.c | 2 +-
4 files changed, 18 insertions(+), 35 deletions(-)
diff --git a/attrib/client.c b/attrib/client.c
index 590f33b..acd35f9 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -52,6 +52,7 @@
struct gatt_service {
struct btd_device *dev;
+ DBusConnection *conn;
bdaddr_t sba;
bdaddr_t dba;
char *path;
@@ -108,8 +109,6 @@ struct watcher {
static GSList *gatt_services = NULL;
-static DBusConnection *connection;
-
static void characteristic_free(void *user_data)
{
struct characteristic *chr = user_data;
@@ -138,7 +137,7 @@ static void primary_free(void *user_data)
for (l = prim->watchers; l; l = l->next) {
struct watcher *watcher = l->data;
- g_dbus_remove_watch(connection, watcher->id);
+ g_dbus_remove_watch(prim->gatt->conn, watcher->id);
}
g_slist_foreach(prim->chars, (GFunc) characteristic_free, NULL);
@@ -156,6 +155,7 @@ static void gatt_service_free(void *user_data)
g_attrib_unref(gatt->attrib);
g_free(gatt->path);
btd_device_unref(gatt->dev);
+ dbus_connection_unref(gatt->conn);
g_free(gatt);
}
@@ -249,6 +249,7 @@ static void update_watchers(gpointer data, gpointer user_data)
{
struct watcher *w = data;
struct characteristic *chr = user_data;
+ DBusConnection *conn = w->prim->gatt->conn;
DBusMessage *msg;
msg = dbus_message_new_method_call(w->name, w->path,
@@ -261,7 +262,7 @@ static void update_watchers(gpointer data, gpointer user_data)
&chr->value, chr->vlen, DBUS_TYPE_INVALID);
dbus_message_set_no_reply(msg, TRUE);
- g_dbus_send_message(connection, msg);
+ g_dbus_send_message(conn, msg);
}
static void events_handler(const uint8_t *pdu, uint16_t len,
@@ -337,7 +338,7 @@ static void connect_cb(GIOChannel *chan, GError *gerr, gpointer user_data)
if (gatt->msg) {
DBusMessage *reply = btd_error_failed(gatt->msg,
gerr->message);
- g_dbus_send_message(connection, reply);
+ g_dbus_send_message(gatt->conn, reply);
}
error("%s", gerr->message);
@@ -592,7 +593,7 @@ static void register_characteristics(struct primary *prim)
for (lc = prim->chars; lc; lc = lc->next) {
struct characteristic *chr = lc->data;
- g_dbus_register_interface(connection, chr->path,
+ g_dbus_register_interface(prim->gatt->conn, chr->path,
CHAR_INTERFACE, char_methods,
NULL, NULL, chr, NULL);
DBG("Registered: %s", chr->path);
@@ -936,7 +937,7 @@ static void char_discovered_cb(GSList *characteristics, guint8 status,
g_slist_foreach(prim->chars, update_all_chars, prim);
fail:
- g_dbus_send_message(connection, reply);
+ g_dbus_send_message(gatt->conn, reply);
g_attrib_unref(gatt->attrib);
g_free(current);
}
@@ -1033,7 +1034,7 @@ static void register_primaries(struct gatt_service *gatt, GSList *primaries)
prim->path = g_strdup_printf("%s/service%04x", gatt->path,
att->start);
- g_dbus_register_interface(connection, prim->path,
+ g_dbus_register_interface(gatt->conn, prim->path,
CHAR_INTERFACE, prim_methods,
NULL, NULL, prim, NULL);
DBG("Registered: %s", prim->path);
@@ -1044,7 +1045,8 @@ static void register_primaries(struct gatt_service *gatt, GSList *primaries)
}
}
-int attrib_client_register(struct btd_device *device, int psm, GSList *primaries)
+int attrib_client_register(DBusConnection *connection,
+ struct btd_device *device, int psm, GSList *primaries)
{
struct btd_adapter *adapter = device_get_adapter(device);
const char *path = device_get_path(device);
@@ -1056,6 +1058,7 @@ int attrib_client_register(struct btd_device *device, int psm, GSList *primaries
gatt = g_new0(struct gatt_service, 1);
gatt->dev = btd_device_ref(device);
+ gatt->conn = dbus_connection_ref(connection);
gatt->listen = FALSE;
gatt->path = g_strdup(path);
bacpy(&gatt->sba, &sba);
@@ -1085,31 +1088,12 @@ void attrib_client_unregister(struct btd_device *device)
struct primary *prim = lp->data;
for (lc = prim->chars; lc; lc = lc->next) {
struct characteristic *chr = lc->data;
- g_dbus_unregister_interface(connection, chr->path,
+ g_dbus_unregister_interface(gatt->conn, chr->path,
CHAR_INTERFACE);
}
- g_dbus_unregister_interface(connection, prim->path,
+ g_dbus_unregister_interface(gatt->conn, prim->path,
CHAR_INTERFACE);
}
gatt_service_free(gatt);
}
-
-int attrib_client_init(DBusConnection *conn)
-{
-
- connection = dbus_connection_ref(conn);
-
- /*
- * FIXME: if the adapter supports BLE start scanning. Temporary
- * solution, this approach doesn't allow to control scanning based
- * on the discoverable property.
- */
-
- return 0;
-}
-
-void attrib_client_exit(void)
-{
- dbus_connection_unref(connection);
-}
diff --git a/attrib/client.h b/attrib/client.h
index 2bee84c..650b0c1 100644
--- a/attrib/client.h
+++ b/attrib/client.h
@@ -22,7 +22,6 @@
*
*/
-int attrib_client_init(DBusConnection *conn);
-void attrib_client_exit(void);
-int attrib_client_register(struct btd_device *device, int psm, GSList *primaries);
+int attrib_client_register(DBusConnection *connection,
+ struct btd_device *device, int psm, GSList *primaries);
void attrib_client_unregister(struct btd_device *device);
diff --git a/src/adapter.c b/src/adapter.c
index 56105c7..124252a 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2182,7 +2182,7 @@ static void create_stored_device_from_primary(char *key, char *value,
}
/* FIXME: Need the correct psm */
- attrib_client_register(device, -1, services);
+ attrib_client_register(connection, device, -1, services);
device_probe_drivers(device, uuids);
diff --git a/src/device.c b/src/device.c
index 1d7094b..511c9d0 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1548,7 +1548,7 @@ static void primary_cb(GSList *services, guint8 status, gpointer user_data)
device_probe_drivers(device, uuids);
/* FIXME: Need the correct psm */
- attrib_client_register(device, -1, services);
+ attrib_client_register(req->conn, device, -1, services);
g_slist_free(uuids);
--
1.7.4.1
---
src/device.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/device.c b/src/device.c
index 9a3202f..1d7094b 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1546,6 +1546,10 @@ static void primary_cb(GSList *services, guint8 status, gpointer user_data)
}
device_probe_drivers(device, uuids);
+
+ /* FIXME: Need the correct psm */
+ attrib_client_register(device, -1, services);
+
g_slist_free(uuids);
create_device_reply(device, req);
--
1.7.4.1
---
Makefile.am | 4 ++--
attrib/manager.c | 47 -----------------------------------------------
2 files changed, 2 insertions(+), 49 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 2994bf9..1b92115 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -89,7 +89,8 @@ endif
endif
attrib_sources = attrib/att.h attrib/att.c attrib/gatt.h attrib/gatt.c \
- attrib/gattrib.h attrib/gattrib.c
+ attrib/gattrib.h attrib/gattrib.c attrib/client.h \
+ attrib/client.c
gdbus_sources = gdbus/gdbus.h gdbus/mainloop.c gdbus/watch.c \
gdbus/object.c gdbus/polkit.c
@@ -204,7 +205,6 @@ endif
builtin_modules += attrib
builtin_sources += attrib/main.c \
attrib/manager.h attrib/manager.c \
- attrib/client.h attrib/client.c \
attrib/example.h attrib/example.c
endif
diff --git a/attrib/manager.c b/attrib/manager.c
index a5a7de4..df59a0a 100644
--- a/attrib/manager.c
+++ b/attrib/manager.c
@@ -35,57 +35,14 @@
#include "hcid.h"
#include "manager.h"
-#include "client.h"
#include "example.h"
-#define GATT_UUID "00001801-0000-1000-8000-00805f9b34fb"
-
static DBusConnection *connection;
-static int client_probe(struct btd_device *device, GSList *uuids)
-{
- const sdp_record_t *rec;
- int psm = -1;
-
- rec = btd_device_get_record(device, GATT_UUID);
- if (rec) {
- sdp_list_t *list;
- if (sdp_get_access_protos(rec, &list) < 0)
- return -1;
-
- psm = sdp_get_proto_port(list, L2CAP_UUID);
-
- sdp_list_foreach(list, (sdp_list_func_t) sdp_list_free, NULL);
- sdp_list_free(list, NULL);
-
- if (psm < 0)
- return -1;
- }
-
- return attrib_client_register(device, psm);
-}
-
-static void client_remove(struct btd_device *device)
-{
- attrib_client_unregister(device);
-}
-
-static struct btd_device_driver client_driver = {
- .name = "gatt-client",
- .uuids = BTD_UUIDS(GATT_UUID),
- .probe = client_probe,
- .remove = client_remove,
-};
-
int attrib_manager_init(DBusConnection *conn)
{
connection = dbus_connection_ref(conn);
- attrib_client_init(connection);
-
- btd_register_device_driver(&client_driver);
-
-
if (main_opts.attrib_server)
return server_example_init();
@@ -94,12 +51,8 @@ int attrib_manager_init(DBusConnection *conn)
void attrib_manager_exit(void)
{
- btd_unregister_device_driver(&client_driver);
-
if (main_opts.attrib_server)
server_example_exit();
- attrib_client_exit();
-
dbus_connection_unref(connection);
}
--
1.7.4.1
Now that the GATT client functionality is being moved to the
core we need to register the interface when the device is created.
---
attrib/client.c | 3 +--
attrib/client.h | 2 +-
src/adapter.c | 4 ++++
src/device.c | 3 +++
4 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/attrib/client.c b/attrib/client.c
index 54bdc79..590f33b 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -1044,12 +1044,11 @@ static void register_primaries(struct gatt_service *gatt, GSList *primaries)
}
}
-int attrib_client_register(struct btd_device *device, int psm)
+int attrib_client_register(struct btd_device *device, int psm, GSList *primaries)
{
struct btd_adapter *adapter = device_get_adapter(device);
const char *path = device_get_path(device);
struct gatt_service *gatt;
- GSList *primaries = btd_device_get_primaries(device);
bdaddr_t sba, dba;
adapter_get_address(adapter, &sba);
diff --git a/attrib/client.h b/attrib/client.h
index 50e2b5f..2bee84c 100644
--- a/attrib/client.h
+++ b/attrib/client.h
@@ -24,5 +24,5 @@
int attrib_client_init(DBusConnection *conn);
void attrib_client_exit(void);
-int attrib_client_register(struct btd_device *device, int psm);
+int attrib_client_register(struct btd_device *device, int psm, GSList *primaries);
void attrib_client_unregister(struct btd_device *device);
diff --git a/src/adapter.c b/src/adapter.c
index 691b963..56105c7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -58,6 +58,7 @@
#include "storage.h"
#include "attrib-server.h"
#include "att.h"
+#include "attrib/client.h"
/* Flags Descriptions */
#define EIR_LIM_DISC 0x01 /* LE Limited Discoverable Mode */
@@ -2180,6 +2181,9 @@ static void create_stored_device_from_primary(char *key, char *value,
device_add_primary(device, prim);
}
+ /* FIXME: Need the correct psm */
+ attrib_client_register(device, -1, services);
+
device_probe_drivers(device, uuids);
g_slist_free(services);
diff --git a/src/device.c b/src/device.c
index 771a908..9a3202f 100644
--- a/src/device.c
+++ b/src/device.c
@@ -60,6 +60,7 @@
#include "sdp-xml.h"
#include "storage.h"
#include "btio.h"
+#include "attrib/client.h"
#define DISCONNECT_TIMER 2
#define DISCOVERY_TIMER 2
@@ -1030,6 +1031,8 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
g_slist_free(device->drivers);
device->drivers = NULL;
+ attrib_client_unregister(device);
+
btd_device_unref(device);
}
--
1.7.4.1