From: Luiz Augusto von Dentz <[email protected]>
The accept calback may transit the state to connected on the call itself
since most of the time it is just a matter of selecting the attributes
in case of GATT profiles.
---
src/service.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/service.c b/src/service.c
index f387fc4..20a41d0 100644
--- a/src/service.c
+++ b/src/service.c
@@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
return err;
done:
- change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
+ if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
+ change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
return 0;
}
--
2.7.4
Hi,
On Fri, Sep 9, 2016 at 3:35 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Felipe,
>
> On Thu, Sep 8, 2016 at 6:37 PM, Felipe Ferreri Tonello
> <[email protected]> wrote:
>> Hi Luiz,
>>
>> On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
>>> Hi Felipe,
>>>
>>> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
>>> <[email protected]> wrote:
>>>> Hi Luiz,
>>>>
>>>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>>
>>>>> The accept calback may transit the state to connected on the call itself
>>>>> since most of the time it is just a matter of selecting the attributes
>>>>> in case of GATT profiles.
>>>>> ---
>>>>> src/service.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/service.c b/src/service.c
>>>>> index f387fc4..20a41d0 100644
>>>>> --- a/src/service.c
>>>>> +++ b/src/service.c
>>>>> @@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
>>>>> return err;
>>>>>
>>>>> done:
>>>>> - change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>>>> + if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
>>>>> + change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>>>
>>>> This looks really hacky - I know it was like this before already.
>>>
>>> This is to avoid resetting the connection if the accept callback has
>>> set it already.
>>>
>>>> Why can you change the state in two different places? What if the
>>>> profile->accept doesn't change the state to connected? Will this change
>>>> to connecting and then the connected state will never be set?
>>>
>>> Going from connected to connecting is not valid, thus if the driver
>>> had changed the states there is nothing else to be done.
>>
>> Why do you have this done label if the driver is supposed to call
>> btd_service_connecting_complete() on success?
>>>
>>>> I think btd_service_connecting_complete() should always be called with
>>>> the same err as returned by profile->accept()
>>>
>>> Not sure what difference does this make?
>>
>> My suggestion is to handle the state based on the return value of the
>> driver's callback and not let the driver to change the state itself.
>> It's boilerplate code anyway.
>
> The driver may have to respond this asynchronously so we can't really
> depend on the return alone.
Applied.
--
Luiz Augusto von Dentz
Hi Felipe,
On Thu, Sep 8, 2016 at 6:33 PM, Felipe Ferreri Tonello
<[email protected]> wrote:
> Hi Luiz,
>
> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> If the profile accepts connections it should also be notified when ATT
>> disconnects so it can cleanup properly.
>> ---
>> src/device.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/src/device.c b/src/device.c
>> index 9586022..eda873f 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -4563,6 +4563,18 @@ static void attio_disconnected(gpointer data, gpointer user_data)
>> attio->dcfunc(attio->user_data);
>> }
>>
>> +static void disconnect_gatt_service(gpointer data, gpointer user_data)
>> +{
>> + struct btd_service *service = data;
>> + struct btd_profile *profile = btd_service_get_profile(service);
>> +
>> + /* Ignore if profile cannot accept connections */
>> + if (!profile->accept)
>> + return;
>
> What if the profile has connect and not accept?
GATT drivers can only accept not connect so this specific to avoid
disconnecting BR/EDR profiles if both bearers are connected, we might
as well introduce a flag to indicate the driver is to be using with
GATT only if we want to make it more clear.
>> +
>> + btd_service_disconnect(service);
>> +}
>> +
>> static void att_disconnected_cb(int err, void *user_data)
>> {
>> struct btd_device *device = user_data;
>> @@ -4575,6 +4587,7 @@ static void att_disconnected_cb(int err, void *user_data)
>> DBG("%s (%d)", strerror(err), err);
>>
>> g_slist_foreach(device->attios, attio_disconnected, NULL);
>> + g_slist_foreach(device->services, disconnect_gatt_service, NULL);
>>
>> btd_gatt_client_disconnected(device->client_dbus);
>>
>>
>
> --
> Felipe
--
Luiz Augusto von Dentz
Hi Felipe,
On Thu, Sep 8, 2016 at 6:37 PM, Felipe Ferreri Tonello
<[email protected]> wrote:
> Hi Luiz,
>
> On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
>> Hi Felipe,
>>
>> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
>> <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>>> From: Luiz Augusto von Dentz <[email protected]>
>>>>
>>>> The accept calback may transit the state to connected on the call itself
>>>> since most of the time it is just a matter of selecting the attributes
>>>> in case of GATT profiles.
>>>> ---
>>>> src/service.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/service.c b/src/service.c
>>>> index f387fc4..20a41d0 100644
>>>> --- a/src/service.c
>>>> +++ b/src/service.c
>>>> @@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
>>>> return err;
>>>>
>>>> done:
>>>> - change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>>> + if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
>>>> + change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>>
>>> This looks really hacky - I know it was like this before already.
>>
>> This is to avoid resetting the connection if the accept callback has
>> set it already.
>>
>>> Why can you change the state in two different places? What if the
>>> profile->accept doesn't change the state to connected? Will this change
>>> to connecting and then the connected state will never be set?
>>
>> Going from connected to connecting is not valid, thus if the driver
>> had changed the states there is nothing else to be done.
>
> Why do you have this done label if the driver is supposed to call
> btd_service_connecting_complete() on success?
>>
>>> I think btd_service_connecting_complete() should always be called with
>>> the same err as returned by profile->accept()
>>
>> Not sure what difference does this make?
>
> My suggestion is to handle the state based on the return value of the
> driver's callback and not let the driver to change the state itself.
> It's boilerplate code anyway.
The driver may have to respond this asynchronously so we can't really
depend on the return alone.
--
Luiz Augusto von Dentz
Hi Luiz,
On 08/09/16 16:26, Luiz Augusto von Dentz wrote:
> Hi Felipe,
>
> On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
> <[email protected]> wrote:
>> Hi Luiz,
>>
>> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> The accept calback may transit the state to connected on the call itself
>>> since most of the time it is just a matter of selecting the attributes
>>> in case of GATT profiles.
>>> ---
>>> src/service.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/service.c b/src/service.c
>>> index f387fc4..20a41d0 100644
>>> --- a/src/service.c
>>> +++ b/src/service.c
>>> @@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
>>> return err;
>>>
>>> done:
>>> - change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>> + if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
>>> + change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>>
>> This looks really hacky - I know it was like this before already.
>
> This is to avoid resetting the connection if the accept callback has
> set it already.
>
>> Why can you change the state in two different places? What if the
>> profile->accept doesn't change the state to connected? Will this change
>> to connecting and then the connected state will never be set?
>
> Going from connected to connecting is not valid, thus if the driver
> had changed the states there is nothing else to be done.
Why do you have this done label if the driver is supposed to call
btd_service_connecting_complete() on success?
>
>> I think btd_service_connecting_complete() should always be called with
>> the same err as returned by profile->accept()
>
> Not sure what difference does this make?
My suggestion is to handle the state based on the return value of the
driver's callback and not let the driver to change the state itself.
It's boilerplate code anyway.
>
>>
>>> return 0;
>>> }
>>>
>>>
>>
--
Felipe
Hi Luiz,
On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> If the profile accepts connections it should also be notified when ATT
> disconnects so it can cleanup properly.
> ---
> src/device.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index 9586022..eda873f 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4563,6 +4563,18 @@ static void attio_disconnected(gpointer data, gpointer user_data)
> attio->dcfunc(attio->user_data);
> }
>
> +static void disconnect_gatt_service(gpointer data, gpointer user_data)
> +{
> + struct btd_service *service = data;
> + struct btd_profile *profile = btd_service_get_profile(service);
> +
> + /* Ignore if profile cannot accept connections */
> + if (!profile->accept)
> + return;
What if the profile has connect and not accept?
> +
> + btd_service_disconnect(service);
> +}
> +
> static void att_disconnected_cb(int err, void *user_data)
> {
> struct btd_device *device = user_data;
> @@ -4575,6 +4587,7 @@ static void att_disconnected_cb(int err, void *user_data)
> DBG("%s (%d)", strerror(err), err);
>
> g_slist_foreach(device->attios, attio_disconnected, NULL);
> + g_slist_foreach(device->services, disconnect_gatt_service, NULL);
>
> btd_gatt_client_disconnected(device->client_dbus);
>
>
--
Felipe
Hi Felipe,
On Thu, Sep 8, 2016 at 5:45 PM, Felipe Ferreri Tonello
<[email protected]> wrote:
> Hi Luiz,
>
> On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> The accept calback may transit the state to connected on the call itself
>> since most of the time it is just a matter of selecting the attributes
>> in case of GATT profiles.
>> ---
>> src/service.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/service.c b/src/service.c
>> index f387fc4..20a41d0 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
>> return err;
>>
>> done:
>> - change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>> + if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
>> + change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
>
> This looks really hacky - I know it was like this before already.
This is to avoid resetting the connection if the accept callback has
set it already.
> Why can you change the state in two different places? What if the
> profile->accept doesn't change the state to connected? Will this change
> to connecting and then the connected state will never be set?
Going from connected to connecting is not valid, thus if the driver
had changed the states there is nothing else to be done.
> I think btd_service_connecting_complete() should always be called with
> the same err as returned by profile->accept()
Not sure what difference does this make?
>
>> return 0;
>> }
>>
>>
>
> --
> Felipe
--
Luiz Augusto von Dentz
Hi Luiz,
On 08/09/16 13:38, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> The accept calback may transit the state to connected on the call itself
> since most of the time it is just a matter of selecting the attributes
> in case of GATT profiles.
> ---
> src/service.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/service.c b/src/service.c
> index f387fc4..20a41d0 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -209,7 +209,8 @@ int service_accept(struct btd_service *service)
> return err;
>
> done:
> - change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
> + if (service->state == BTD_SERVICE_STATE_DISCONNECTED)
> + change_state(service, BTD_SERVICE_STATE_CONNECTING, 0);
This looks really hacky - I know it was like this before already.
Why can you change the state in two different places? What if the
profile->accept doesn't change the state to connected? Will this change
to connecting and then the connected state will never be set?
I think btd_service_connecting_complete() should always be called with
the same err as returned by profile->accept()
> return 0;
> }
>
>
--
Felipe
From: Luiz Augusto von Dentz <[email protected]>
This implements the profile disconnect callback using it to cleanup
existing references of bt_gatt_client and gatt_db.
---
profiles/scanparam/scan.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index f8ad810..0ff4a43 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -165,6 +165,15 @@ static void foreach_scan_param_service(struct gatt_db_attribute *attr,
gatt_db_service_foreach_char(scan->attr, handle_characteristic, scan);
}
+static void scan_reset(struct scan *scan)
+{
+ scan->attr = NULL;
+ gatt_db_unref(scan->db);
+ scan->db = NULL;
+ bt_gatt_client_unref(scan->client);
+ scan->client = NULL;
+}
+
static int scan_param_accept(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
@@ -182,11 +191,6 @@ static int scan_param_accept(struct btd_service *service)
return -1;
}
- /* Clean-up any old client/db and acquire the new ones */
- scan->attr = NULL;
- gatt_db_unref(scan->db);
- bt_gatt_client_unref(scan->client);
-
scan->db = gatt_db_ref(db);
scan->client = bt_gatt_client_ref(client);
@@ -196,10 +200,7 @@ static int scan_param_accept(struct btd_service *service)
if (!scan->attr) {
error("Scan Parameters attribute not found");
- gatt_db_unref(scan->db);
- scan->db = NULL;
- bt_gatt_client_unref(scan->client);
- scan->client = NULL;
+ scan_reset(scan);
return -1;
}
@@ -208,6 +209,17 @@ static int scan_param_accept(struct btd_service *service)
return 0;
}
+static int scan_param_disconnect(struct btd_service *service)
+{
+ struct scan *scan = btd_service_get_user_data(service);
+
+ scan_reset(scan);
+
+ btd_service_disconnecting_complete(service, 0);
+
+ return 0;
+}
+
static void scan_param_remove(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
@@ -257,6 +269,7 @@ static struct btd_profile scan_profile = {
.device_probe = scan_param_probe,
.device_remove = scan_param_remove,
.accept = scan_param_accept,
+ .disconnect = scan_param_disconnect,
};
static int scan_param_init(void)
--
2.7.4
From: Luiz Augusto von Dentz <[email protected]>
Instead of storing service context data in a list this make use of
btd_service_set_user_data to store the context data which later can be
retrieved with btd_service_get_user_data.
---
profiles/scanparam/scan.c | 35 ++++++++---------------------------
1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index d12e09e..f8ad810 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -62,8 +62,6 @@ struct scan {
guint refresh_cb_id;
};
-static GSList *devices;
-
static void scan_free(struct scan *scan)
{
bt_gatt_client_unregister_notify(scan->client, scan->refresh_cb_id);
@@ -73,14 +71,6 @@ static void scan_free(struct scan *scan)
g_free(scan);
}
-static int cmp_device(gconstpointer a, gconstpointer b)
-{
- const struct scan *scan = a;
- const struct btd_device *device = b;
-
- return scan->device == device ? 0 : -1;
-}
-
static void write_scan_params(struct scan *scan)
{
uint8_t value[4];
@@ -181,21 +171,17 @@ static int scan_param_accept(struct btd_service *service)
struct gatt_db *db = btd_device_get_gatt_db(device);
struct bt_gatt_client *client = btd_device_get_gatt_client(device);
bt_uuid_t scan_parameters_uuid;
- struct scan *scan;
- GSList *l;
+ struct scan *scan = btd_service_get_user_data(service);
char addr[18];
ba2str(device_get_address(device), addr);
DBG("Scan Parameters Client Driver profile accept (%s)", addr);
- l = g_slist_find_custom(devices, device, cmp_device);
- if (!l) {
+ if (!scan) {
error("Scan Parameters service not handled by profile");
return -1;
}
- scan = l->data;
-
/* Clean-up any old client/db and acquire the new ones */
scan->attr = NULL;
gatt_db_unref(scan->db);
@@ -226,21 +212,17 @@ static void scan_param_remove(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
struct scan *scan;
- GSList *l;
char addr[18];
ba2str(device_get_address(device), addr);
DBG("GAP profile remove (%s)", addr);
- l = g_slist_find_custom(devices, device, cmp_device);
- if (!l) {
+ scan = btd_service_get_user_data(service);
+ if (!scan) {
error("GAP service not handled by profile");
return;
}
- scan = l->data;
-
- devices = g_slist_remove(devices, scan);
scan_free(scan);
}
@@ -248,16 +230,15 @@ static int scan_param_probe(struct btd_service *service)
{
struct btd_device *device = btd_service_get_device(service);
struct scan *scan;
- GSList *l;
char addr[18];
ba2str(device_get_address(device), addr);
DBG("Scan Parameters Client Driver profile probe (%s)", addr);
/* Ignore, if we were probed for this device already */
- l = g_slist_find_custom(devices, device, cmp_device);
- if (l) {
- error("Profile probed twice for the same device!");
+ scan = btd_service_get_user_data(service);
+ if (scan) {
+ error("Profile probed twice for the same service!");
return -1;
}
@@ -266,7 +247,7 @@ static int scan_param_probe(struct btd_service *service)
return -1;
scan->device = btd_device_ref(device);
- devices = g_slist_append(devices, scan);
+ btd_service_set_user_data(service, scan);
return 0;
}
--
2.7.4
From: Luiz Augusto von Dentz <[email protected]>
On accept the profile shall check about existing attribute, etc and once
done call btd_service_connecting_complete updating the service state
properly.
---
profiles/scanparam/scan.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index d3ca762..d12e09e 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -201,7 +201,6 @@ static int scan_param_accept(struct btd_service *service)
gatt_db_unref(scan->db);
bt_gatt_client_unref(scan->client);
-
scan->db = gatt_db_ref(db);
scan->client = bt_gatt_client_ref(client);
@@ -209,6 +208,17 @@ static int scan_param_accept(struct btd_service *service)
gatt_db_foreach_service(db, &scan_parameters_uuid,
foreach_scan_param_service, scan);
+ if (!scan->attr) {
+ error("Scan Parameters attribute not found");
+ gatt_db_unref(scan->db);
+ scan->db = NULL;
+ bt_gatt_client_unref(scan->client);
+ scan->client = NULL;
+ return -1;
+ }
+
+ btd_service_connecting_complete(service, 0);
+
return 0;
}
--
2.7.4
From: Luiz Augusto von Dentz <[email protected]>
If the profile accepts connections it should also be notified when ATT
disconnects so it can cleanup properly.
---
src/device.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/device.c b/src/device.c
index 9586022..eda873f 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4563,6 +4563,18 @@ static void attio_disconnected(gpointer data, gpointer user_data)
attio->dcfunc(attio->user_data);
}
+static void disconnect_gatt_service(gpointer data, gpointer user_data)
+{
+ struct btd_service *service = data;
+ struct btd_profile *profile = btd_service_get_profile(service);
+
+ /* Ignore if profile cannot accept connections */
+ if (!profile->accept)
+ return;
+
+ btd_service_disconnect(service);
+}
+
static void att_disconnected_cb(int err, void *user_data)
{
struct btd_device *device = user_data;
@@ -4575,6 +4587,7 @@ static void att_disconnected_cb(int err, void *user_data)
DBG("%s (%d)", strerror(err), err);
g_slist_foreach(device->attios, attio_disconnected, NULL);
+ g_slist_foreach(device->services, disconnect_gatt_service, NULL);
btd_gatt_client_disconnected(device->client_dbus);
--
2.7.4