2015-01-06 02:03:28

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 0/8] Implmenet doc/gatt-api.txt for client

*v1: Picking up remaining patches from before the holidays:
- Rebased on top of Luiz's modifications.
- Fixed small bugs that appeared after the merge.
- Addressed some of the initial comments.
- Added StartNotify/StopNotify. I left these methods as they are.
I'm planning to address the issue with potentially missed
notifications in a GattProfile1 API.

Arman Uguray (8):
core: gatt: Expose charac. extended properties.
core: gatt: Implement GattCharacteristic1.StartNotify
core: gatt: Implement GattCharacteristic1.StopNotify
shared/gatt-db: Add truncate argument to write
core: gatt: Issue long write for reliable-write
core: gatt: Reference count chrcs and descs.
core: gatt: Handle Service Changed.
doc/gatt-api.txt: Update error names

android/gatt.c | 17 +-
doc/gatt-api.txt | 12 +-
src/device.c | 4 +-
src/gatt-client.c | 609 ++++++++++++++++++++++++++++++++++++++++-------
src/shared/gatt-db.c | 18 +-
src/shared/gatt-db.h | 1 +
src/shared/gatt-server.c | 6 +-
tools/btgatt-server.c | 8 +-
unit/test-gatt.c | 6 +-
9 files changed, 569 insertions(+), 112 deletions(-)

--
2.2.0.rc0.207.ga3a616c



2015-01-07 12:41:49

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

Hi Arman,

On Tue, Jan 6, 2015 at 8:09 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>> On Tue, Jan 6, 2015 at 1:44 PM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>>> On Tue, Jan 6, 2015 at 12:45 PM, Luiz Augusto von Dentz <[email protected]> wrote:
>>> Hi Arman,
>>>
>>> On Tue, Jan 6, 2015 at 4:58 PM, Arman Uguray <[email protected]> wrote:
>>>> Hi Luiz,
>>>>
>>>>> On Tue, Jan 6, 2015 at 5:09 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>>>>> Hi Arman,
>>>>>
>>>>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>>>>>> This patch adds the "truncate" argument to gatt_db_attribute_write. This
>>>>>> allows users to specify if the cached value should be truncated when the
>>>>>> write value length is shorter than what is stored in the database. This
>>>>>> allows client code to correctly store a remote attribute value when the
>>>>>> database is used as a client-role cache.
>>>>>> ---
>>>>>> android/gatt.c | 17 +++++++++--------
>>>>>> src/gatt-client.c | 6 +++---
>>>>>> src/shared/gatt-db.c | 18 ++++++++++++++++--
>>>>>> src/shared/gatt-db.h | 1 +
>>>>>> src/shared/gatt-server.c | 6 ++++--
>>>>>> tools/btgatt-server.c | 8 ++++----
>>>>>> unit/test-gatt.c | 6 +++---
>>>>>> 7 files changed, 40 insertions(+), 22 deletions(-)
>>>>>>
>>>>>> diff --git a/android/gatt.c b/android/gatt.c
>>>>>> index 3a35bd6..7f765e0 100644
>>>>>> --- a/android/gatt.c
>>>>>> +++ b/android/gatt.c
>>>>>> @@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>>> return;
>>>>>>
>>>>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
>>>>>> - write_confirm, NULL);
>>>>>> + false, write_confirm, NULL);
>>>>>> }
>>>>>>
>>>>>> static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>>> @@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>>> /* Signature OK, proceed with write */
>>>>>> bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
>>>>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>>>>> - &dev->bdaddr, write_confirm, NULL);
>>>>>> + &dev->bdaddr, false,
>>>>>> + write_confirm, NULL);
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> @@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>>> }
>>>>>>
>>>>>> if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>>>>> - &dev->bdaddr, attribute_write_cb,
>>>>>> - data)) {
>>>>>> + &dev->bdaddr, false,
>>>>>> + attribute_write_cb, data)) {
>>>>>> queue_remove(dev->pending_requests, data);
>>>>>> free(data);
>>>>>> return ATT_ECODE_UNLIKELY;
>>>>>> @@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>>> data->length = vlen;
>>>>>>
>>>>>> if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
>>>>>> - &dev->bdaddr, attribute_write_cb,
>>>>>> - data)) {
>>>>>> + &dev->bdaddr, false,
>>>>>> + attribute_write_cb, data)) {
>>>>>> queue_remove(dev->pending_requests, data);
>>>>>> g_free(data->value);
>>>>>> free(data);
>>>>>> @@ -6805,7 +6806,7 @@ static void register_gap_service(void)
>>>>>> value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
>>>>>> gatt_db_attribute_write(gap_srvc_data.appear, 0,
>>>>>> (void *) &value, sizeof(value),
>>>>>> - ATT_OP_WRITE_REQ, NULL,
>>>>>> + ATT_OP_WRITE_REQ, NULL, false,
>>>>>> write_confirm, NULL);
>>>>>> }
>>>>>>
>>>>>> @@ -6822,7 +6823,7 @@ static void register_gap_service(void)
>>>>>> value = PERIPHERAL_PRIVACY_DISABLE;
>>>>>> gatt_db_attribute_write(gap_srvc_data.priv, 0,
>>>>>> &value, sizeof(value),
>>>>>> - ATT_OP_WRITE_REQ, NULL,
>>>>>> + ATT_OP_WRITE_REQ, NULL, false,
>>>>>> write_confirm, NULL);
>>>>>> }
>>>>>>
>>>>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>>>>>> index 1edbafc..79e1d26 100644
>>>>>> --- a/src/gatt-client.c
>>>>>> +++ b/src/gatt-client.c
>>>>>> @@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> - gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
>>>>>> + gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
>>>>>> write_descriptor_cb, desc);
>>>>>>
>>>>>> /*
>>>>>> @@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
>>>>>> }
>>>>>>
>>>>>> gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
>>>>>> - write_characteristic_cb, chrc);
>>>>>> + true, write_characteristic_cb, chrc);
>>>>>
>>>>>
>>>>> I think it would be better to have some means to truncate the value
>>>>> separately, e.g. gatt_db_attribute_truncate.
>>>>>
>>>>
>>>> Would this just truncate the value to the specified length? Or would
>>>> we want a "set_value" function that works like write but truncates it
>>>> if needed?
>>>
>>> Maybe a reset would be enough so once we start read operation we reset
>>> the current value, how about it?
>>>
>>
>> Sounds good to me.
>>
>
> Sorry to break the flow, but it looks like I don't fully understand
> what you meant by "once we start read operation we reset the current
> value". Do you mean we call gatt_db_attribute_truncate after we call
> gatt_db_attribute_write? Or we do something in gatt_db_attribute_read?

I was thinking in gatt_db_attribute_reset followed by
gatt_db_attribute_write, obviously we should only reset for the first
time if the value cannot be read all at once.

>>>>>> /*
>>>>>> * If the value length is exactly MTU-1, then we may not have read the
>>>>>> @@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
>>>>>> * signal so that we propagate the notification/indication to
>>>>>> * applications.
>>>>>> */
>>>>>> - gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>>>>>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
>>>>>> write_characteristic_cb, chrc);
>>>>>> }
>>>>>>
>>>>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>>>>> index 74c3e70..87e77d1 100644
>>>>>> --- a/src/shared/gatt-db.c
>>>>>> +++ b/src/shared/gatt-db.c
>>>>>> @@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
>>>>>> return false;
>>>>>> }
>>>>>>
>>>>>> +static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
>>>>>> + uint16_t offset,
>>>>>> + bool truncate)
>>>>>> +{
>>>>>> + unsigned diff;
>>>>>> +
>>>>>> + if (!attrib->value || offset >= attrib->value_len)
>>>>>> + return true;
>>>>>> +
>>>>>> + diff = attrib->value_len - offset;
>>>>>> +
>>>>>> + return truncate ? len != diff : len > diff;
>>>>>> +}
>>>>>> +
>>>>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>>>> const uint8_t *value, size_t len,
>>>>>> uint8_t opcode, bdaddr_t *bdaddr,
>>>>>> + bool truncate,
>>>>>> gatt_db_attribute_write_t func,
>>>>>> void *user_data)
>>>>>> {
>>>>>> @@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>>>> }
>>>>>>
>>>>>> /* For values stored in db allocate on demand */
>>>>>> - if (!attrib->value || offset >= attrib->value_len ||
>>>>>> - len > (unsigned) (attrib->value_len - offset)) {
>>>>>> + if (should_resize_value(attrib, len, offset, truncate)) {
>>>>>> void *buf;
>>>>>>
>>>>>> buf = realloc(attrib->value, len + offset);
>>>>>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>>>>>> index f188944..2467852 100644
>>>>>> --- a/src/shared/gatt-db.h
>>>>>> +++ b/src/shared/gatt-db.h
>>>>>> @@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>>>>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>>>> const uint8_t *value, size_t len,
>>>>>> uint8_t opcode, bdaddr_t *bdaddr,
>>>>>> + bool truncate,
>>>>>> gatt_db_attribute_write_t func,
>>>>>> void *user_data);
>>>>>>
>>>>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>>>>> index 00f36fd..2af7cf1 100644
>>>>>> --- a/src/shared/gatt-server.c
>>>>>> +++ b/src/shared/gatt-server.c
>>>>>> @@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
>>>>>> server->pending_write_op = op;
>>>>>>
>>>>>> if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
>>>>>> - NULL, write_complete_cb, op))
>>>>>> + NULL, false,
>>>>>> + write_complete_cb, op))
>>>>>> return;
>>>>>>
>>>>>> if (op)
>>>>>> @@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>>> status = gatt_db_attribute_write(attr, next->offset,
>>>>>> next->value, next->length,
>>>>>> BT_ATT_OP_EXEC_WRITE_REQ, NULL,
>>>>>> - exec_write_complete_cb, server);
>>>>>> + false, exec_write_complete_cb,
>>>>>> + server);
>>>>>>
>>>>>> prep_write_data_destroy(next);
>>>>>>
>>>>>> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
>>>>>> index f6ad8c3..393a071 100644
>>>>>> --- a/tools/btgatt-server.c
>>>>>> +++ b/tools/btgatt-server.c
>>>>>> @@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
>>>>>> gatt_db_attribute_write(tmp, 0, (void *) &appearance,
>>>>>> sizeof(appearance),
>>>>>> BT_ATT_OP_WRITE_REQ,
>>>>>> - NULL, confirm_write,
>>>>>> - NULL);
>>>>>> + NULL, true,
>>>>>> + confirm_write, NULL);
>>>>>>
>>>>>> gatt_db_service_set_active(service, true);
>>>>>> }
>>>>>> @@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
>>>>>> NULL, NULL, server);
>>>>>> gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
>>>>>> BT_ATT_OP_WRITE_REQ,
>>>>>> - NULL, confirm_write,
>>>>>> - NULL);
>>>>>> + NULL, true,
>>>>>> + confirm_write, NULL);
>>>>>>
>>>>>> /* HR Control Point Characteristic */
>>>>>> bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
>>>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>>>> index 169fbd3..7a5081d 100644
>>>>>> --- a/unit/test-gatt.c
>>>>>> +++ b/unit/test-gatt.c
>>>>>> @@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,
>>>>>>
>>>>>> g_assert(attrib != NULL);
>>>>>>
>>>>>> - gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
>>>>>> - NULL);
>>>>>> + gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
>>>>>> + att_write_cb, NULL);
>>>>>>
>>>>>> return attrib;
>>>>>> }
>>>>>> @@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
>>>>>> desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
>>>>>> NULL, NULL);
>>>>>>
>>>>>> - gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
>>>>>> + gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
>>>>>> att_write_cb, NULL);
>>>>>>
>>>>>> return desc_att;
>>>>>> --
>>>>>> 2.2.0.rc0.207.ga3a616c
>>>>>>
>>>>>> --
>>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Luiz Augusto von Dentz
>>>>
>>>> Cheers,
>>>> Arman
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> Cheers,
>> Arman
>
> Thanks,
> Arman



--
Luiz Augusto von Dentz

2015-01-07 04:03:19

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 6/8] core: gatt: Reference count chrcs and descs.

Hi Luiz,

> On Tue, Jan 6, 2015 at 5:17 AM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>> Many of the GATT D-Bus API methods are asynchronous and their callbacks
>> depend on characteristic and descriptor instances to be alive when they
>> execute. This patch makes characteristics and descriptors reference
>> counted so that an interim disconnect or "Service Changed" won't
>> immediately free up those instances.
>
> IMO this logic needs to be reverted, the pending operations should be
> stored in the respective struct not the other way around since that
> way you can cancel an outstanding operation if the object is to be
> freed.
>

That makes sense, I initially did this because shared/gatt-client
didn't support canceling GATT procedures that worked over multiple ATT
protocol PDUs but I went ahead and added this support to
bt_gatt_client. I'll send some patches for this tomorrow.

>> ---
>> src/gatt-client.c | 179 ++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 125 insertions(+), 54 deletions(-)
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index 6897f30..c4f3582 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -83,6 +83,8 @@ struct characteristic {
>> bt_uuid_t uuid;
>> char *path;
>>
>> + int ref_count;
>> +
>> bool in_read;
>> bool in_write;
>>
>> @@ -99,10 +101,49 @@ struct descriptor {
>> bt_uuid_t uuid;
>> char *path;
>>
>> + int ref_count;
>> +
>> bool in_read;
>> bool in_write;
>> };
>>
>> +static struct characteristic *characteristic_ref(struct characteristic *chrc)
>> +{
>> + __sync_fetch_and_add(&chrc->ref_count, 1);
>> +
>> + return chrc;
>> +}
>> +
>> +static void characteristic_unref(void *data)
>> +{
>> + struct characteristic *chrc = data;
>> +
>> + if (__sync_sub_and_fetch(&chrc->ref_count, 1))
>> + return;
>> +
>> + queue_destroy(chrc->descs, NULL); /* List should be empty here */
>> + g_free(chrc->path);
>> + free(chrc);
>> +}
>> +
>> +static struct descriptor *descriptor_ref(struct descriptor *desc)
>> +{
>> + __sync_fetch_and_add(&desc->ref_count, 1);
>> +
>> + return desc;
>> +}
>> +
>> +static void descriptor_unref(void *data)
>> +{
>> + struct descriptor *desc = data;
>> +
>> + if (__sync_sub_and_fetch(&desc->ref_count, 1))
>> + return;
>> +
>> + g_free(desc->path);
>> + free(desc);
>> +}
>> +
>> static bool uuid_cmp(const bt_uuid_t *uuid, uint16_t u16)
>> {
>> bt_uuid_t uuid16;
>> @@ -220,6 +261,7 @@ static bool parse_value_arg(DBusMessage *msg, uint8_t **value,
>> }
>>
>> typedef bool (*async_dbus_op_complete_t)(void *data);
>> +typedef void (*async_dbus_op_destroy_t)(void *data);
>>
>> struct async_dbus_op {
>> int ref_count;
>> @@ -227,12 +269,16 @@ struct async_dbus_op {
>> void *data;
>> uint16_t offset;
>> async_dbus_op_complete_t complete;
>> + async_dbus_op_destroy_t destroy;
>> };
>>
>> static void async_dbus_op_free(void *data)
>> {
>> struct async_dbus_op *op = data;
>>
>> + if (op->destroy)
>> + op->destroy(op->data);
>> +
>> if (op->msg)
>> dbus_message_unref(op->msg);
>>
>> @@ -389,7 +435,8 @@ static DBusMessage *descriptor_read_value(DBusConnection *conn,
>> return btd_error_failed(msg, "Failed to initialize request");
>>
>> op->msg = dbus_message_ref(msg);
>> - op->data = desc;
>> + op->data = descriptor_ref(desc);
>> + op->destroy = descriptor_unref;
>>
>> if (bt_gatt_client_read_value(gatt, desc->handle, desc_read_cb,
>> async_dbus_op_ref(op),
>> @@ -444,7 +491,8 @@ static bool start_long_write(DBusMessage *msg, uint16_t handle,
>> struct bt_gatt_client *gatt,
>> bool reliable, const uint8_t *value,
>> size_t value_len, void *data,
>> - async_dbus_op_complete_t complete)
>> + async_dbus_op_complete_t complete,
>> + async_dbus_op_destroy_t destroy)
>> {
>> struct async_dbus_op *op;
>>
>> @@ -455,6 +503,7 @@ static bool start_long_write(DBusMessage *msg, uint16_t handle,
>> op->msg = dbus_message_ref(msg);
>> op->data = data;
>> op->complete = complete;
>> + op->destroy = destroy;
>>
>> if (bt_gatt_client_write_long_value(gatt, reliable, handle,
>> 0, value, value_len,
>> @@ -471,7 +520,8 @@ static bool start_write_request(DBusMessage *msg, uint16_t handle,
>> struct bt_gatt_client *gatt,
>> const uint8_t *value, size_t value_len,
>> void *data,
>> - async_dbus_op_complete_t complete)
>> + async_dbus_op_complete_t complete,
>> + async_dbus_op_destroy_t destroy)
>> {
>> struct async_dbus_op *op;
>>
>> @@ -482,6 +532,7 @@ static bool start_write_request(DBusMessage *msg, uint16_t handle,
>> op->msg = dbus_message_ref(msg);
>> op->data = data;
>> op->complete = complete;
>> + op->destroy = destroy;
>>
>> if (bt_gatt_client_write_value(gatt, handle, value, value_len,
>> write_cb, op,
>> @@ -536,11 +587,13 @@ static DBusMessage *descriptor_write_value(DBusConnection *conn,
>> if (value_len <= (unsigned) bt_gatt_client_get_mtu(gatt) - 3)
>> result = start_write_request(msg, desc->handle, gatt, value,
>> value_len, desc,
>> - desc_write_complete);
>> + desc_write_complete,
>> + descriptor_unref);
>> else
>> result = start_long_write(msg, desc->handle, gatt, false, value,
>> value_len, desc,
>> - desc_write_complete);
>> + desc_write_complete,
>> + descriptor_unref);
>>
>> if (!result)
>> return btd_error_failed(msg, "Failed to initiate write");
>> @@ -571,14 +624,6 @@ static const GDBusMethodTable descriptor_methods[] = {
>> { }
>> };
>>
>> -static void descriptor_free(void *data)
>> -{
>> - struct descriptor *desc = data;
>> -
>> - g_free(desc->path);
>> - free(desc);
>> -}
>> -
>> static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,
>> struct characteristic *chrc)
>> {
>> @@ -600,10 +645,11 @@ static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,
>> GATT_DESCRIPTOR_IFACE,
>> descriptor_methods, NULL,
>> descriptor_properties,
>> - desc, descriptor_free)) {
>> + descriptor_ref(desc),
>> + descriptor_unref)) {
>> error("Unable to register GATT descriptor with handle 0x%04x",
>> desc->handle);
>> - descriptor_free(desc);
>> + descriptor_unref(desc);
>>
>> return NULL;
>> }
>> @@ -622,6 +668,8 @@ static void unregister_descriptor(void *data)
>>
>> DBG("Removing GATT descriptor: %s", desc->path);
>>
>> + desc->chrc = NULL;
>> +
>> g_dbus_unregister_interface(btd_get_dbus_connection(), desc->path,
>> GATT_DESCRIPTOR_IFACE);
>> }
>> @@ -803,7 +851,8 @@ static DBusMessage *characteristic_read_value(DBusConnection *conn,
>> return btd_error_failed(msg, "Failed to initialize request");
>>
>> op->msg = dbus_message_ref(msg);
>> - op->data = chrc;
>> + op->data = characteristic_ref(chrc);
>> + op->destroy = characteristic_unref;
>>
>> if (bt_gatt_client_read_value(gatt, chrc->value_handle, chrc_read_cb,
>> async_dbus_op_ref(op),
>> @@ -877,13 +926,16 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
>>
>> if (value_len <= (unsigned) mtu - 3)
>> result = start_write_request(msg, chrc->value_handle,
>> - gatt, value,
>> - value_len, chrc,
>> - chrc_write_complete);
>> + gatt, value, value_len,
>> + characteristic_ref(chrc),
>> + chrc_write_complete,
>> + characteristic_unref);
>> else
>> result = start_long_write(msg, chrc->value_handle, gatt,
>> - false, value, value_len, chrc,
>> - chrc_write_complete);
>> + false, value, value_len,
>> + characteristic_ref(chrc),
>> + chrc_write_complete,
>> + characteristic_unref);
>>
>> if (result)
>> goto done_async;
>> @@ -1016,12 +1068,33 @@ static bool match_notify_sender(const void *a, const void *b)
>> return strcmp(client->owner, sender) == 0;
>> }
>>
>> +struct register_notify_op {
>> + int ref_count;
>> + struct notify_client *client;
>> + struct characteristic *chrc;
>> +};
>> +
>> +static void register_notify_op_unref(void *data)
>> +{
>> + struct register_notify_op *op = data;
>> +
>> + DBG("Released register_notify_op");
>> +
>> + if (__sync_sub_and_fetch(&op->ref_count, 1))
>> + return;
>> +
>> + notify_client_unref(op->client);
>> + characteristic_unref(op->chrc);
>> +
>> + free(op);
>> +}
>> +
>> static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> uint16_t length, void *user_data)
>> {
>> struct async_dbus_op *op = user_data;
>> - struct notify_client *client = op->data;
>> - struct characteristic *chrc = client->chrc;
>> + struct register_notify_op *notify_op = op->data;
>> + struct characteristic *chrc = notify_op->chrc;
>>
>> /*
>> * Even if the value didn't change, we want to send a PropertiesChanged
>> @@ -1036,26 +1109,24 @@ static void register_notify_cb(unsigned int id, uint16_t att_ecode,
>> void *user_data)
>> {
>> struct async_dbus_op *op = user_data;
>> - struct notify_client *client = op->data;
>> - struct characteristic *chrc = client->chrc;
>> + struct register_notify_op *notify_op = op->data;
>> + struct notify_client *client = notify_op->client;
>> + struct characteristic *chrc = notify_op->chrc;
>> DBusMessage *reply;
>>
>> - /* Make sure that an interim disconnect did not remove the client */
>> - if (!queue_find(chrc->notify_clients, NULL, client)) {
>> + /*
>> + * Make sure that an interim disconnect or "Service Changed" did not
>> + * remove the client
>> + */
>> + if (!chrc->service || !queue_find(chrc->notify_clients, NULL, client)) {
>> bt_gatt_client_unregister_notify(chrc->service->client->gatt,
>> id);
>> - notify_client_unref(client);
>>
>> reply = btd_error_failed(op->msg,
>> "Characteristic not available");
>> goto done;
>> }
>>
>> - /*
>> - * Drop the reference count that we added when registering the callback.
>> - */
>> - notify_client_unref(client);
>> -
>> if (!id) {
>> queue_remove(chrc->notify_clients, client);
>> notify_client_free(client);
>> @@ -1094,6 +1165,7 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>> const char *sender = dbus_message_get_sender(msg);
>> struct async_dbus_op *op;
>> struct notify_client *client;
>> + struct register_notify_op *notify_op;
>>
>> if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
>> chrc->props & BT_GATT_CHRC_PROP_INDICATE))
>> @@ -1110,20 +1182,28 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>> if (!client)
>> return btd_error_failed(msg, "Failed allocate notify session");
>>
>> + notify_op = new0(struct register_notify_op, 1);
>> + if (!notify_op) {
>> + notify_client_unref(client);
>> + return btd_error_failed(msg, "Failed to initialize request");
>> + }
>> +
>> + notify_op->ref_count = 1;
>> + notify_op->client = client;
>> + notify_op->chrc = characteristic_ref(chrc);
>> +
>> op = new0(struct async_dbus_op, 1);
>> if (!op) {
>> - notify_client_unref(client);
>> + register_notify_op_unref(notify_op);
>> return btd_error_failed(msg, "Failed to initialize request");
>> }
>>
>> - /*
>> - * Add to the ref count so that a disconnect during register won't free
>> - * the client instance.
>> - */
>> - op->data = notify_client_ref(client);
>> + op->data = notify_op;
>> op->msg = dbus_message_ref(msg);
>> + op->destroy = register_notify_op_unref;
>>
>> - queue_push_tail(chrc->notify_clients, client);
>> + /* The characteristic owns a reference to the client */
>> + queue_push_tail(chrc->notify_clients, notify_client_ref(client));
>>
>> if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
>> register_notify_cb, notify_cb,
>> @@ -1133,9 +1213,6 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>> queue_remove(chrc->notify_clients, client);
>> async_dbus_op_free(op);
>>
>> - /* Directly free the client */
>> - notify_client_free(client);
>> -
>> return btd_error_failed(msg, "Failed to register notify session");
>> }
>>
>> @@ -1220,15 +1297,6 @@ static const GDBusMethodTable characteristic_methods[] = {
>> { }
>> };
>>
>> -static void characteristic_free(void *data)
>> -{
>> - struct characteristic *chrc = data;
>> -
>> - queue_destroy(chrc->descs, NULL); /* List should be empty here */
>> - g_free(chrc->path);
>> - free(chrc);
>> -}
>> -
>> static struct characteristic *characteristic_create(
>> struct gatt_db_attribute *attr,
>> struct service *service)
>> @@ -1269,10 +1337,11 @@ static struct characteristic *characteristic_create(
>> GATT_CHARACTERISTIC_IFACE,
>> characteristic_methods, NULL,
>> characteristic_properties,
>> - chrc, characteristic_free)) {
>> + characteristic_ref(chrc),
>> + characteristic_unref)) {
>> error("Unable to register GATT characteristic with handle "
>> "0x%04x", chrc->handle);
>> - characteristic_free(chrc);
>> + characteristic_unref(chrc);
>>
>> return NULL;
>> }
>> @@ -1291,6 +1360,8 @@ static void unregister_characteristic(void *data)
>> queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>>
>> + chrc->service = NULL;
>> +
>> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>> GATT_CHARACTERISTIC_IFACE);
>> }
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> 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
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Arman

2015-01-06 22:09:39

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

Hi Luiz,

> On Tue, Jan 6, 2015 at 1:44 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>> On Tue, Jan 6, 2015 at 12:45 PM, Luiz Augusto von Dentz <[email protected]> wrote:
>> Hi Arman,
>>
>> On Tue, Jan 6, 2015 at 4:58 PM, Arman Uguray <[email protected]> wrote:
>>> Hi Luiz,
>>>
>>>> On Tue, Jan 6, 2015 at 5:09 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>>>> Hi Arman,
>>>>
>>>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>>>>> This patch adds the "truncate" argument to gatt_db_attribute_write. This
>>>>> allows users to specify if the cached value should be truncated when the
>>>>> write value length is shorter than what is stored in the database. This
>>>>> allows client code to correctly store a remote attribute value when the
>>>>> database is used as a client-role cache.
>>>>> ---
>>>>> android/gatt.c | 17 +++++++++--------
>>>>> src/gatt-client.c | 6 +++---
>>>>> src/shared/gatt-db.c | 18 ++++++++++++++++--
>>>>> src/shared/gatt-db.h | 1 +
>>>>> src/shared/gatt-server.c | 6 ++++--
>>>>> tools/btgatt-server.c | 8 ++++----
>>>>> unit/test-gatt.c | 6 +++---
>>>>> 7 files changed, 40 insertions(+), 22 deletions(-)
>>>>>
>>>>> diff --git a/android/gatt.c b/android/gatt.c
>>>>> index 3a35bd6..7f765e0 100644
>>>>> --- a/android/gatt.c
>>>>> +++ b/android/gatt.c
>>>>> @@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>> return;
>>>>>
>>>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
>>>>> - write_confirm, NULL);
>>>>> + false, write_confirm, NULL);
>>>>> }
>>>>>
>>>>> static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>> @@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>> /* Signature OK, proceed with write */
>>>>> bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
>>>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>>>> - &dev->bdaddr, write_confirm, NULL);
>>>>> + &dev->bdaddr, false,
>>>>> + write_confirm, NULL);
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>> }
>>>>>
>>>>> if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>>>> - &dev->bdaddr, attribute_write_cb,
>>>>> - data)) {
>>>>> + &dev->bdaddr, false,
>>>>> + attribute_write_cb, data)) {
>>>>> queue_remove(dev->pending_requests, data);
>>>>> free(data);
>>>>> return ATT_ECODE_UNLIKELY;
>>>>> @@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
>>>>> data->length = vlen;
>>>>>
>>>>> if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
>>>>> - &dev->bdaddr, attribute_write_cb,
>>>>> - data)) {
>>>>> + &dev->bdaddr, false,
>>>>> + attribute_write_cb, data)) {
>>>>> queue_remove(dev->pending_requests, data);
>>>>> g_free(data->value);
>>>>> free(data);
>>>>> @@ -6805,7 +6806,7 @@ static void register_gap_service(void)
>>>>> value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
>>>>> gatt_db_attribute_write(gap_srvc_data.appear, 0,
>>>>> (void *) &value, sizeof(value),
>>>>> - ATT_OP_WRITE_REQ, NULL,
>>>>> + ATT_OP_WRITE_REQ, NULL, false,
>>>>> write_confirm, NULL);
>>>>> }
>>>>>
>>>>> @@ -6822,7 +6823,7 @@ static void register_gap_service(void)
>>>>> value = PERIPHERAL_PRIVACY_DISABLE;
>>>>> gatt_db_attribute_write(gap_srvc_data.priv, 0,
>>>>> &value, sizeof(value),
>>>>> - ATT_OP_WRITE_REQ, NULL,
>>>>> + ATT_OP_WRITE_REQ, NULL, false,
>>>>> write_confirm, NULL);
>>>>> }
>>>>>
>>>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>>>>> index 1edbafc..79e1d26 100644
>>>>> --- a/src/gatt-client.c
>>>>> +++ b/src/gatt-client.c
>>>>> @@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
>>>>> return;
>>>>> }
>>>>>
>>>>> - gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
>>>>> + gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
>>>>> write_descriptor_cb, desc);
>>>>>
>>>>> /*
>>>>> @@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
>>>>> }
>>>>>
>>>>> gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
>>>>> - write_characteristic_cb, chrc);
>>>>> + true, write_characteristic_cb, chrc);
>>>>
>>>>
>>>> I think it would be better to have some means to truncate the value
>>>> separately, e.g. gatt_db_attribute_truncate.
>>>>
>>>
>>> Would this just truncate the value to the specified length? Or would
>>> we want a "set_value" function that works like write but truncates it
>>> if needed?
>>
>> Maybe a reset would be enough so once we start read operation we reset
>> the current value, how about it?
>>
>
> Sounds good to me.
>

Sorry to break the flow, but it looks like I don't fully understand
what you meant by "once we start read operation we reset the current
value". Do you mean we call gatt_db_attribute_truncate after we call
gatt_db_attribute_write? Or we do something in gatt_db_attribute_read?

>>>>> /*
>>>>> * If the value length is exactly MTU-1, then we may not have read the
>>>>> @@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
>>>>> * signal so that we propagate the notification/indication to
>>>>> * applications.
>>>>> */
>>>>> - gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>>>>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
>>>>> write_characteristic_cb, chrc);
>>>>> }
>>>>>
>>>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>>>> index 74c3e70..87e77d1 100644
>>>>> --- a/src/shared/gatt-db.c
>>>>> +++ b/src/shared/gatt-db.c
>>>>> @@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
>>>>> return false;
>>>>> }
>>>>>
>>>>> +static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
>>>>> + uint16_t offset,
>>>>> + bool truncate)
>>>>> +{
>>>>> + unsigned diff;
>>>>> +
>>>>> + if (!attrib->value || offset >= attrib->value_len)
>>>>> + return true;
>>>>> +
>>>>> + diff = attrib->value_len - offset;
>>>>> +
>>>>> + return truncate ? len != diff : len > diff;
>>>>> +}
>>>>> +
>>>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>>> const uint8_t *value, size_t len,
>>>>> uint8_t opcode, bdaddr_t *bdaddr,
>>>>> + bool truncate,
>>>>> gatt_db_attribute_write_t func,
>>>>> void *user_data)
>>>>> {
>>>>> @@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>>> }
>>>>>
>>>>> /* For values stored in db allocate on demand */
>>>>> - if (!attrib->value || offset >= attrib->value_len ||
>>>>> - len > (unsigned) (attrib->value_len - offset)) {
>>>>> + if (should_resize_value(attrib, len, offset, truncate)) {
>>>>> void *buf;
>>>>>
>>>>> buf = realloc(attrib->value, len + offset);
>>>>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>>>>> index f188944..2467852 100644
>>>>> --- a/src/shared/gatt-db.h
>>>>> +++ b/src/shared/gatt-db.h
>>>>> @@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>>>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>>> const uint8_t *value, size_t len,
>>>>> uint8_t opcode, bdaddr_t *bdaddr,
>>>>> + bool truncate,
>>>>> gatt_db_attribute_write_t func,
>>>>> void *user_data);
>>>>>
>>>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>>>> index 00f36fd..2af7cf1 100644
>>>>> --- a/src/shared/gatt-server.c
>>>>> +++ b/src/shared/gatt-server.c
>>>>> @@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
>>>>> server->pending_write_op = op;
>>>>>
>>>>> if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
>>>>> - NULL, write_complete_cb, op))
>>>>> + NULL, false,
>>>>> + write_complete_cb, op))
>>>>> return;
>>>>>
>>>>> if (op)
>>>>> @@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>> status = gatt_db_attribute_write(attr, next->offset,
>>>>> next->value, next->length,
>>>>> BT_ATT_OP_EXEC_WRITE_REQ, NULL,
>>>>> - exec_write_complete_cb, server);
>>>>> + false, exec_write_complete_cb,
>>>>> + server);
>>>>>
>>>>> prep_write_data_destroy(next);
>>>>>
>>>>> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
>>>>> index f6ad8c3..393a071 100644
>>>>> --- a/tools/btgatt-server.c
>>>>> +++ b/tools/btgatt-server.c
>>>>> @@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
>>>>> gatt_db_attribute_write(tmp, 0, (void *) &appearance,
>>>>> sizeof(appearance),
>>>>> BT_ATT_OP_WRITE_REQ,
>>>>> - NULL, confirm_write,
>>>>> - NULL);
>>>>> + NULL, true,
>>>>> + confirm_write, NULL);
>>>>>
>>>>> gatt_db_service_set_active(service, true);
>>>>> }
>>>>> @@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
>>>>> NULL, NULL, server);
>>>>> gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
>>>>> BT_ATT_OP_WRITE_REQ,
>>>>> - NULL, confirm_write,
>>>>> - NULL);
>>>>> + NULL, true,
>>>>> + confirm_write, NULL);
>>>>>
>>>>> /* HR Control Point Characteristic */
>>>>> bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
>>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>>> index 169fbd3..7a5081d 100644
>>>>> --- a/unit/test-gatt.c
>>>>> +++ b/unit/test-gatt.c
>>>>> @@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,
>>>>>
>>>>> g_assert(attrib != NULL);
>>>>>
>>>>> - gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
>>>>> - NULL);
>>>>> + gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
>>>>> + att_write_cb, NULL);
>>>>>
>>>>> return attrib;
>>>>> }
>>>>> @@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
>>>>> desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
>>>>> NULL, NULL);
>>>>>
>>>>> - gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
>>>>> + gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
>>>>> att_write_cb, NULL);
>>>>>
>>>>> return desc_att;
>>>>> --
>>>>> 2.2.0.rc0.207.ga3a616c
>>>>>
>>>>> --
>>>>> 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
>>>>
>>>>
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>>
>>> Cheers,
>>> Arman
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Cheers,
> Arman

Thanks,
Arman

2015-01-06 21:49:51

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 2/8] core: gatt: Implement GattCharacteristic1.StartNotify

Hi Luiz,

> On Tue, Jan 6, 2015 at 12:54 PM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Tue, Jan 6, 2015 at 4:56 PM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>>> On Tue, Jan 6, 2015 at 4:33 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>>> Hi Arman,
>>>
>>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>>>> This patch implements the StartNotify method of
>>>> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
>>>> session to the D-Bus sender which internally registers a unique
>>>> notify handler with the bt_gatt_client. Each received notification
>>>> or indication causes a PropertiesChanged signal to be emitted for the
>>>> "Value" property.
>>>>
>>>> The notify handler gets automatically unregistered when sender
>>>> disconnects from D-Bus.
>>>
>>> I recall we discussed removing not implementing StartNofify in favor
>>> of an API where the application can register for notification rather
>>> than enabling it on every connection, furthermore CCC is currently
>>> being exposed via GattDescriptor1 which might conflict with this.
>>>
>>
>> I'm not sure about removing StartNotify/StopNotify just yet. My
>> intention is to first get everything specified in doc/gatt-api.txt
>> implemented and see how it works (since the API is experimental, I
>> think this is OK). And once we have a GattProfile1 API we can remove
>> these. I think that StartNotify/StopNotify addresses most use cases
>> for now, and I want to at least support those platforms that have
>> already developed against doc/gatt-api.txt (ChromiumOS, Tizen, etc)
>> behind the experimental flag.
>
> Well I considerer enabling notifications across connection too
> important to be ignored, in fact this is the very reason CCC is
> persistent so the client configure it once and can start receiving
> notification since the beginning of the connection, with StartNotify
> we cannot do that because the objects are destroyed on disconnect.
>

Oh, I agree that this doesn't address that particular case, I just
wanted to keep the methods though since the API is experimental.
Anyway, I suppose I'll go ahead and remove StartNotify/StopNotify from
the API doc and work on a proposal for GattProfile1. I'll place the
new interfaces inside doc/gatt-api.txt.

>> GattDescriptor1 won't be an issue since GattDescriptor1.WriteValue
>> will fail with Error.NotSupported if called on a CCC descriptor.
>
> I guess we should hide it then, or you do expect anyone to read it?
>

I think it's better to export it and prevent writing to it than hiding
it, since it won't hurt anybody. Some applications may be interested
in reading it, though they should probably use
GattCharacteristic1.Notifying instead. I'm not super opinionated on
this but I prefer exporting it so that developers can clearly see that
this descriptor exists on the other end.

>>
>>>> ---
>>>> src/gatt-client.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 246 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>>>> index 8bbb03d..5c030ee 100644
>>>> --- a/src/gatt-client.c
>>>> +++ b/src/gatt-client.c
>>>> @@ -87,6 +87,9 @@ struct characteristic {
>>>> bool in_write;
>>>>
>>>> struct queue *descs;
>>>> +
>>>> + bool notifying;
>>>> + struct queue *notify_clients;
>>>> };
>>>>
>>>> struct descriptor {
>>>> @@ -230,7 +233,9 @@ static void async_dbus_op_free(void *data)
>>>> {
>>>> struct async_dbus_op *op = data;
>>>>
>>>> - dbus_message_unref(op->msg);
>>>> + if (op->msg)
>>>> + dbus_message_unref(op->msg);
>>>> +
>>>> free(op);
>>>> }
>>>>
>>>> @@ -674,12 +679,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
>>>> static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
>>>> DBusMessageIter *iter, void *data)
>>>> {
>>>> - dbus_bool_t notifying = FALSE;
>>>> -
>>>> - /*
>>>> - * TODO: Return the correct value here once StartNotify and StopNotify
>>>> - * methods are implemented.
>>>> - */
>>>> + struct characteristic *chrc = data;
>>>> + dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>>>>
>>>> dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
>>>>
>>>> @@ -895,11 +896,239 @@ done_async:
>>>> return NULL;
>>>> }
>>>>
>>>> +struct notify_client {
>>>> + struct characteristic *chrc;
>>>> + int ref_count;
>>>> + char *owner;
>>>> + guint watch;
>>>> + unsigned int notify_id;
>>>> +};
>>>> +
>>>> +static void notify_client_free(struct notify_client *client)
>>>> +{
>>>> + DBG("owner %s", client->owner);
>>>> +
>>>> + g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
>>>> + free(client->owner);
>>>> + free(client);
>>>> +}
>>>> +
>>>> +static void notify_client_unref(void *data)
>>>> +{
>>>> + struct notify_client *client = data;
>>>> +
>>>> + DBG("owner %s", client->owner);
>>>> +
>>>> + if (__sync_sub_and_fetch(&client->ref_count, 1))
>>>> + return;
>>>> +
>>>> + notify_client_free(client);
>>>> +}
>>>> +
>>>> +static struct notify_client *notify_client_ref(struct notify_client *client)
>>>> +{
>>>> + DBG("owner %s", client->owner);
>>>> +
>>>> + __sync_fetch_and_add(&client->ref_count, 1);
>>>> +
>>>> + return client;
>>>> +}
>>>> +
>>>> +static bool match_notifying(const void *a, const void *b)
>>>> +{
>>>> + const struct notify_client *client = a;
>>>> +
>>>> + return !!client->notify_id;
>>>> +}
>>>> +
>>>> +static void update_notifying(struct characteristic *chrc)
>>>> +{
>>>> + if (!chrc->notifying)
>>>> + return;
>>>> +
>>>> + if (queue_find(chrc->notify_clients, match_notifying, NULL))
>>>> + return;
>>>> +
>>>> + chrc->notifying = false;
>>>> +
>>>> + g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
>>>> + GATT_CHARACTERISTIC_IFACE,
>>>> + "Notifying");
>>>> +}
>>>> +
>>>> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
>>>> +{
>>>> + struct notify_client *client = user_data;
>>>> + struct characteristic *chrc = client->chrc;
>>>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>>>> +
>>>> + DBG("owner %s", client->owner);
>>>> +
>>>> + queue_remove(chrc->notify_clients, client);
>>>> + bt_gatt_client_unregister_notify(gatt, client->notify_id);
>>>> +
>>>> + update_notifying(chrc);
>>>> +
>>>> + notify_client_unref(client);
>>>> +}
>>>> +
>>>> +static struct notify_client *notify_client_create(struct characteristic *chrc,
>>>> + const char *owner)
>>>> +{
>>>> + struct notify_client *client;
>>>> +
>>>> + client = new0(struct notify_client, 1);
>>>> + if (!client)
>>>> + return NULL;
>>>> +
>>>> + client->chrc = chrc;
>>>> + client->owner = strdup(owner);
>>>> + if (!client->owner) {
>>>> + free(client);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
>>>> + owner, notify_client_disconnect,
>>>> + client, NULL);
>>>> + if (!client->watch) {
>>>> + free(client->owner);
>>>> + free(client);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + return notify_client_ref(client);
>>>> +}
>>>> +
>>>> +static bool match_notify_sender(const void *a, const void *b)
>>>> +{
>>>> + const struct notify_client *client = a;
>>>> + const char *sender = b;
>>>> +
>>>> + return strcmp(client->owner, sender) == 0;
>>>> +}
>>>> +
>>>> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
>>>> + uint16_t length, void *user_data)
>>>> +{
>>>> + struct async_dbus_op *op = user_data;
>>>> + struct notify_client *client = op->data;
>>>> + struct characteristic *chrc = client->chrc;
>>>> +
>>>> + /*
>>>> + * Even if the value didn't change, we want to send a PropertiesChanged
>>>> + * signal so that we propagate the notification/indication to
>>>> + * applications.
>>>> + */
>>>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>>>> + write_characteristic_cb, chrc);
>>>> +}
>>>> +
>>>> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
>>>> + void *user_data)
>>>> +{
>>>> + struct async_dbus_op *op = user_data;
>>>> + struct notify_client *client = op->data;
>>>> + struct characteristic *chrc = client->chrc;
>>>> + DBusMessage *reply;
>>>> +
>>>> + /* Make sure that an interim disconnect did not remove the client */
>>>> + if (!queue_find(chrc->notify_clients, NULL, client)) {
>>>> + bt_gatt_client_unregister_notify(chrc->service->client->gatt,
>>>> + id);
>>>> + notify_client_unref(client);
>>>> +
>>>> + reply = btd_error_failed(op->msg,
>>>> + "Characteristic not available");
>>>> + goto done;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Drop the reference count that we added when registering the callback.
>>>> + */
>>>> + notify_client_unref(client);
>>>> +
>>>> + if (!id) {
>>>> + queue_remove(chrc->notify_clients, client);
>>>> + notify_client_free(client);
>>>> +
>>>> + reply = create_gatt_dbus_error(op->msg, att_ecode);
>>>> +
>>>> + goto done;
>>>> + }
>>>> +
>>>> + client->notify_id = id;
>>>> +
>>>> + if (!chrc->notifying) {
>>>> + chrc->notifying = true;
>>>> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
>>>> + chrc->path, GATT_CHARACTERISTIC_IFACE,
>>>> + "Notifying");
>>>> + }
>>>> +
>>>> + reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
>>>> +
>>>> +done:
>>>> + if (reply)
>>>> + g_dbus_send_message(btd_get_dbus_connection(), reply);
>>>> + else
>>>> + error("Failed to construct D-Bus message reply");
>>>> +
>>>> + dbus_message_unref(op->msg);
>>>> + op->msg = NULL;
>>>> +}
>>>> +
>>>> static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>>>> DBusMessage *msg, void *user_data)
>>>> {
>>>> - /* TODO: Implement */
>>>> - return btd_error_failed(msg, "Not implemented");
>>>> + struct characteristic *chrc = user_data;
>>>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>>>> + const char *sender = dbus_message_get_sender(msg);
>>>> + struct async_dbus_op *op;
>>>> + struct notify_client *client;
>>>> +
>>>> + if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
>>>> + chrc->props & BT_GATT_CHRC_PROP_INDICATE))
>>>> + return btd_error_not_supported(msg);
>>>> +
>>>> + /* Each client can only have one active notify session. */
>>>> + client = queue_find(chrc->notify_clients, match_notify_sender, sender);
>>>> + if (client)
>>>> + return client->notify_id ?
>>>> + btd_error_failed(msg, "Already notifying") :
>>>> + btd_error_in_progress(msg);
>>>> +
>>>> + client = notify_client_create(chrc, sender);
>>>> + if (!client)
>>>> + return btd_error_failed(msg, "Failed allocate notify session");
>>>> +
>>>> + op = new0(struct async_dbus_op, 1);
>>>> + if (!op) {
>>>> + notify_client_unref(client);
>>>> + return btd_error_failed(msg, "Failed to initialize request");
>>>> + }
>>>> +
>>>> + /*
>>>> + * Add to the ref count so that a disconnect during register won't free
>>>> + * the client instance.
>>>> + */
>>>> + op->data = notify_client_ref(client);
>>>> + op->msg = dbus_message_ref(msg);
>>>> +
>>>> + queue_push_tail(chrc->notify_clients, client);
>>>> +
>>>> + if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
>>>> + register_notify_cb, notify_cb,
>>>> + op, async_dbus_op_free))
>>>> + return NULL;
>>>> +
>>>> + queue_remove(chrc->notify_clients, client);
>>>> + async_dbus_op_free(op);
>>>> +
>>>> + /* Directly free the client */
>>>> + notify_client_free(client);
>>>> +
>>>> + return btd_error_failed(msg, "Failed to register notify session");
>>>> }
>>>>
>>>> static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
>>>> @@ -992,6 +1221,13 @@ static struct characteristic *characteristic_create(
>>>> return NULL;
>>>> }
>>>>
>>>> + chrc->notify_clients = queue_new();
>>>> + if (!chrc->notify_clients) {
>>>> + queue_destroy(chrc->descs, NULL);
>>>> + free(chrc);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> chrc->service = service;
>>>>
>>>> gatt_db_attribute_get_char_data(attr, &chrc->handle,
>>>> @@ -1027,6 +1263,7 @@ static void unregister_characteristic(void *data)
>>>>
>>>> DBG("Removing GATT characteristic: %s", chrc->path);
>>>>
>>>> + queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>>>> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>>>>
>>>> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>>>> --
>>>> 2.2.0.rc0.207.ga3a616c
>>>>
>>>> --
>>>> 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
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz

Arman

2015-01-06 21:44:40

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

Hi Luiz,

> On Tue, Jan 6, 2015 at 12:45 PM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Tue, Jan 6, 2015 at 4:58 PM, Arman Uguray <[email protected]> wrote:
>> Hi Luiz,
>>
>>> On Tue, Jan 6, 2015 at 5:09 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>>> Hi Arman,
>>>
>>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>>>> This patch adds the "truncate" argument to gatt_db_attribute_write. This
>>>> allows users to specify if the cached value should be truncated when the
>>>> write value length is shorter than what is stored in the database. This
>>>> allows client code to correctly store a remote attribute value when the
>>>> database is used as a client-role cache.
>>>> ---
>>>> android/gatt.c | 17 +++++++++--------
>>>> src/gatt-client.c | 6 +++---
>>>> src/shared/gatt-db.c | 18 ++++++++++++++++--
>>>> src/shared/gatt-db.h | 1 +
>>>> src/shared/gatt-server.c | 6 ++++--
>>>> tools/btgatt-server.c | 8 ++++----
>>>> unit/test-gatt.c | 6 +++---
>>>> 7 files changed, 40 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/android/gatt.c b/android/gatt.c
>>>> index 3a35bd6..7f765e0 100644
>>>> --- a/android/gatt.c
>>>> +++ b/android/gatt.c
>>>> @@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>> return;
>>>>
>>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
>>>> - write_confirm, NULL);
>>>> + false, write_confirm, NULL);
>>>> }
>>>>
>>>> static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>> @@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>>> /* Signature OK, proceed with write */
>>>> bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
>>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>>> - &dev->bdaddr, write_confirm, NULL);
>>>> + &dev->bdaddr, false,
>>>> + write_confirm, NULL);
>>>> }
>>>> }
>>>>
>>>> @@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
>>>> }
>>>>
>>>> if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>>> - &dev->bdaddr, attribute_write_cb,
>>>> - data)) {
>>>> + &dev->bdaddr, false,
>>>> + attribute_write_cb, data)) {
>>>> queue_remove(dev->pending_requests, data);
>>>> free(data);
>>>> return ATT_ECODE_UNLIKELY;
>>>> @@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
>>>> data->length = vlen;
>>>>
>>>> if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
>>>> - &dev->bdaddr, attribute_write_cb,
>>>> - data)) {
>>>> + &dev->bdaddr, false,
>>>> + attribute_write_cb, data)) {
>>>> queue_remove(dev->pending_requests, data);
>>>> g_free(data->value);
>>>> free(data);
>>>> @@ -6805,7 +6806,7 @@ static void register_gap_service(void)
>>>> value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
>>>> gatt_db_attribute_write(gap_srvc_data.appear, 0,
>>>> (void *) &value, sizeof(value),
>>>> - ATT_OP_WRITE_REQ, NULL,
>>>> + ATT_OP_WRITE_REQ, NULL, false,
>>>> write_confirm, NULL);
>>>> }
>>>>
>>>> @@ -6822,7 +6823,7 @@ static void register_gap_service(void)
>>>> value = PERIPHERAL_PRIVACY_DISABLE;
>>>> gatt_db_attribute_write(gap_srvc_data.priv, 0,
>>>> &value, sizeof(value),
>>>> - ATT_OP_WRITE_REQ, NULL,
>>>> + ATT_OP_WRITE_REQ, NULL, false,
>>>> write_confirm, NULL);
>>>> }
>>>>
>>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>>>> index 1edbafc..79e1d26 100644
>>>> --- a/src/gatt-client.c
>>>> +++ b/src/gatt-client.c
>>>> @@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
>>>> return;
>>>> }
>>>>
>>>> - gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
>>>> + gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
>>>> write_descriptor_cb, desc);
>>>>
>>>> /*
>>>> @@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
>>>> }
>>>>
>>>> gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
>>>> - write_characteristic_cb, chrc);
>>>> + true, write_characteristic_cb, chrc);
>>>
>>>
>>> I think it would be better to have some means to truncate the value
>>> separately, e.g. gatt_db_attribute_truncate.
>>>
>>
>> Would this just truncate the value to the specified length? Or would
>> we want a "set_value" function that works like write but truncates it
>> if needed?
>
> Maybe a reset would be enough so once we start read operation we reset
> the current value, how about it?
>

Sounds good to me.

>>>> /*
>>>> * If the value length is exactly MTU-1, then we may not have read the
>>>> @@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
>>>> * signal so that we propagate the notification/indication to
>>>> * applications.
>>>> */
>>>> - gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>>>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
>>>> write_characteristic_cb, chrc);
>>>> }
>>>>
>>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>>> index 74c3e70..87e77d1 100644
>>>> --- a/src/shared/gatt-db.c
>>>> +++ b/src/shared/gatt-db.c
>>>> @@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
>>>> return false;
>>>> }
>>>>
>>>> +static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
>>>> + uint16_t offset,
>>>> + bool truncate)
>>>> +{
>>>> + unsigned diff;
>>>> +
>>>> + if (!attrib->value || offset >= attrib->value_len)
>>>> + return true;
>>>> +
>>>> + diff = attrib->value_len - offset;
>>>> +
>>>> + return truncate ? len != diff : len > diff;
>>>> +}
>>>> +
>>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>> const uint8_t *value, size_t len,
>>>> uint8_t opcode, bdaddr_t *bdaddr,
>>>> + bool truncate,
>>>> gatt_db_attribute_write_t func,
>>>> void *user_data)
>>>> {
>>>> @@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>> }
>>>>
>>>> /* For values stored in db allocate on demand */
>>>> - if (!attrib->value || offset >= attrib->value_len ||
>>>> - len > (unsigned) (attrib->value_len - offset)) {
>>>> + if (should_resize_value(attrib, len, offset, truncate)) {
>>>> void *buf;
>>>>
>>>> buf = realloc(attrib->value, len + offset);
>>>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>>>> index f188944..2467852 100644
>>>> --- a/src/shared/gatt-db.h
>>>> +++ b/src/shared/gatt-db.h
>>>> @@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>>> const uint8_t *value, size_t len,
>>>> uint8_t opcode, bdaddr_t *bdaddr,
>>>> + bool truncate,
>>>> gatt_db_attribute_write_t func,
>>>> void *user_data);
>>>>
>>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>>> index 00f36fd..2af7cf1 100644
>>>> --- a/src/shared/gatt-server.c
>>>> +++ b/src/shared/gatt-server.c
>>>> @@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
>>>> server->pending_write_op = op;
>>>>
>>>> if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
>>>> - NULL, write_complete_cb, op))
>>>> + NULL, false,
>>>> + write_complete_cb, op))
>>>> return;
>>>>
>>>> if (op)
>>>> @@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>> status = gatt_db_attribute_write(attr, next->offset,
>>>> next->value, next->length,
>>>> BT_ATT_OP_EXEC_WRITE_REQ, NULL,
>>>> - exec_write_complete_cb, server);
>>>> + false, exec_write_complete_cb,
>>>> + server);
>>>>
>>>> prep_write_data_destroy(next);
>>>>
>>>> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
>>>> index f6ad8c3..393a071 100644
>>>> --- a/tools/btgatt-server.c
>>>> +++ b/tools/btgatt-server.c
>>>> @@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
>>>> gatt_db_attribute_write(tmp, 0, (void *) &appearance,
>>>> sizeof(appearance),
>>>> BT_ATT_OP_WRITE_REQ,
>>>> - NULL, confirm_write,
>>>> - NULL);
>>>> + NULL, true,
>>>> + confirm_write, NULL);
>>>>
>>>> gatt_db_service_set_active(service, true);
>>>> }
>>>> @@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
>>>> NULL, NULL, server);
>>>> gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
>>>> BT_ATT_OP_WRITE_REQ,
>>>> - NULL, confirm_write,
>>>> - NULL);
>>>> + NULL, true,
>>>> + confirm_write, NULL);
>>>>
>>>> /* HR Control Point Characteristic */
>>>> bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>>> index 169fbd3..7a5081d 100644
>>>> --- a/unit/test-gatt.c
>>>> +++ b/unit/test-gatt.c
>>>> @@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,
>>>>
>>>> g_assert(attrib != NULL);
>>>>
>>>> - gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
>>>> - NULL);
>>>> + gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
>>>> + att_write_cb, NULL);
>>>>
>>>> return attrib;
>>>> }
>>>> @@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
>>>> desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
>>>> NULL, NULL);
>>>>
>>>> - gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
>>>> + gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
>>>> att_write_cb, NULL);
>>>>
>>>> return desc_att;
>>>> --
>>>> 2.2.0.rc0.207.ga3a616c
>>>>
>>>> --
>>>> 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
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> Cheers,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz

Cheers,
Arman

2015-01-06 20:54:42

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 2/8] core: gatt: Implement GattCharacteristic1.StartNotify

Hi Arman,

On Tue, Jan 6, 2015 at 4:56 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>> On Tue, Jan 6, 2015 at 4:33 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>> Hi Arman,
>>
>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>>> This patch implements the StartNotify method of
>>> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
>>> session to the D-Bus sender which internally registers a unique
>>> notify handler with the bt_gatt_client. Each received notification
>>> or indication causes a PropertiesChanged signal to be emitted for the
>>> "Value" property.
>>>
>>> The notify handler gets automatically unregistered when sender
>>> disconnects from D-Bus.
>>
>> I recall we discussed removing not implementing StartNofify in favor
>> of an API where the application can register for notification rather
>> than enabling it on every connection, furthermore CCC is currently
>> being exposed via GattDescriptor1 which might conflict with this.
>>
>
> I'm not sure about removing StartNotify/StopNotify just yet. My
> intention is to first get everything specified in doc/gatt-api.txt
> implemented and see how it works (since the API is experimental, I
> think this is OK). And once we have a GattProfile1 API we can remove
> these. I think that StartNotify/StopNotify addresses most use cases
> for now, and I want to at least support those platforms that have
> already developed against doc/gatt-api.txt (ChromiumOS, Tizen, etc)
> behind the experimental flag.

Well I considerer enabling notifications across connection too
important to be ignored, in fact this is the very reason CCC is
persistent so the client configure it once and can start receiving
notification since the beginning of the connection, with StartNotify
we cannot do that because the objects are destroyed on disconnect.

> GattDescriptor1 won't be an issue since GattDescriptor1.WriteValue
> will fail with Error.NotSupported if called on a CCC descriptor.

I guess we should hide it then, or you do expect anyone to read it?

>
>>> ---
>>> src/gatt-client.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 246 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>>> index 8bbb03d..5c030ee 100644
>>> --- a/src/gatt-client.c
>>> +++ b/src/gatt-client.c
>>> @@ -87,6 +87,9 @@ struct characteristic {
>>> bool in_write;
>>>
>>> struct queue *descs;
>>> +
>>> + bool notifying;
>>> + struct queue *notify_clients;
>>> };
>>>
>>> struct descriptor {
>>> @@ -230,7 +233,9 @@ static void async_dbus_op_free(void *data)
>>> {
>>> struct async_dbus_op *op = data;
>>>
>>> - dbus_message_unref(op->msg);
>>> + if (op->msg)
>>> + dbus_message_unref(op->msg);
>>> +
>>> free(op);
>>> }
>>>
>>> @@ -674,12 +679,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
>>> static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
>>> DBusMessageIter *iter, void *data)
>>> {
>>> - dbus_bool_t notifying = FALSE;
>>> -
>>> - /*
>>> - * TODO: Return the correct value here once StartNotify and StopNotify
>>> - * methods are implemented.
>>> - */
>>> + struct characteristic *chrc = data;
>>> + dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>>>
>>> dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
>>>
>>> @@ -895,11 +896,239 @@ done_async:
>>> return NULL;
>>> }
>>>
>>> +struct notify_client {
>>> + struct characteristic *chrc;
>>> + int ref_count;
>>> + char *owner;
>>> + guint watch;
>>> + unsigned int notify_id;
>>> +};
>>> +
>>> +static void notify_client_free(struct notify_client *client)
>>> +{
>>> + DBG("owner %s", client->owner);
>>> +
>>> + g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
>>> + free(client->owner);
>>> + free(client);
>>> +}
>>> +
>>> +static void notify_client_unref(void *data)
>>> +{
>>> + struct notify_client *client = data;
>>> +
>>> + DBG("owner %s", client->owner);
>>> +
>>> + if (__sync_sub_and_fetch(&client->ref_count, 1))
>>> + return;
>>> +
>>> + notify_client_free(client);
>>> +}
>>> +
>>> +static struct notify_client *notify_client_ref(struct notify_client *client)
>>> +{
>>> + DBG("owner %s", client->owner);
>>> +
>>> + __sync_fetch_and_add(&client->ref_count, 1);
>>> +
>>> + return client;
>>> +}
>>> +
>>> +static bool match_notifying(const void *a, const void *b)
>>> +{
>>> + const struct notify_client *client = a;
>>> +
>>> + return !!client->notify_id;
>>> +}
>>> +
>>> +static void update_notifying(struct characteristic *chrc)
>>> +{
>>> + if (!chrc->notifying)
>>> + return;
>>> +
>>> + if (queue_find(chrc->notify_clients, match_notifying, NULL))
>>> + return;
>>> +
>>> + chrc->notifying = false;
>>> +
>>> + g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
>>> + GATT_CHARACTERISTIC_IFACE,
>>> + "Notifying");
>>> +}
>>> +
>>> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
>>> +{
>>> + struct notify_client *client = user_data;
>>> + struct characteristic *chrc = client->chrc;
>>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>>> +
>>> + DBG("owner %s", client->owner);
>>> +
>>> + queue_remove(chrc->notify_clients, client);
>>> + bt_gatt_client_unregister_notify(gatt, client->notify_id);
>>> +
>>> + update_notifying(chrc);
>>> +
>>> + notify_client_unref(client);
>>> +}
>>> +
>>> +static struct notify_client *notify_client_create(struct characteristic *chrc,
>>> + const char *owner)
>>> +{
>>> + struct notify_client *client;
>>> +
>>> + client = new0(struct notify_client, 1);
>>> + if (!client)
>>> + return NULL;
>>> +
>>> + client->chrc = chrc;
>>> + client->owner = strdup(owner);
>>> + if (!client->owner) {
>>> + free(client);
>>> + return NULL;
>>> + }
>>> +
>>> + client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
>>> + owner, notify_client_disconnect,
>>> + client, NULL);
>>> + if (!client->watch) {
>>> + free(client->owner);
>>> + free(client);
>>> + return NULL;
>>> + }
>>> +
>>> + return notify_client_ref(client);
>>> +}
>>> +
>>> +static bool match_notify_sender(const void *a, const void *b)
>>> +{
>>> + const struct notify_client *client = a;
>>> + const char *sender = b;
>>> +
>>> + return strcmp(client->owner, sender) == 0;
>>> +}
>>> +
>>> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
>>> + uint16_t length, void *user_data)
>>> +{
>>> + struct async_dbus_op *op = user_data;
>>> + struct notify_client *client = op->data;
>>> + struct characteristic *chrc = client->chrc;
>>> +
>>> + /*
>>> + * Even if the value didn't change, we want to send a PropertiesChanged
>>> + * signal so that we propagate the notification/indication to
>>> + * applications.
>>> + */
>>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>>> + write_characteristic_cb, chrc);
>>> +}
>>> +
>>> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
>>> + void *user_data)
>>> +{
>>> + struct async_dbus_op *op = user_data;
>>> + struct notify_client *client = op->data;
>>> + struct characteristic *chrc = client->chrc;
>>> + DBusMessage *reply;
>>> +
>>> + /* Make sure that an interim disconnect did not remove the client */
>>> + if (!queue_find(chrc->notify_clients, NULL, client)) {
>>> + bt_gatt_client_unregister_notify(chrc->service->client->gatt,
>>> + id);
>>> + notify_client_unref(client);
>>> +
>>> + reply = btd_error_failed(op->msg,
>>> + "Characteristic not available");
>>> + goto done;
>>> + }
>>> +
>>> + /*
>>> + * Drop the reference count that we added when registering the callback.
>>> + */
>>> + notify_client_unref(client);
>>> +
>>> + if (!id) {
>>> + queue_remove(chrc->notify_clients, client);
>>> + notify_client_free(client);
>>> +
>>> + reply = create_gatt_dbus_error(op->msg, att_ecode);
>>> +
>>> + goto done;
>>> + }
>>> +
>>> + client->notify_id = id;
>>> +
>>> + if (!chrc->notifying) {
>>> + chrc->notifying = true;
>>> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
>>> + chrc->path, GATT_CHARACTERISTIC_IFACE,
>>> + "Notifying");
>>> + }
>>> +
>>> + reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
>>> +
>>> +done:
>>> + if (reply)
>>> + g_dbus_send_message(btd_get_dbus_connection(), reply);
>>> + else
>>> + error("Failed to construct D-Bus message reply");
>>> +
>>> + dbus_message_unref(op->msg);
>>> + op->msg = NULL;
>>> +}
>>> +
>>> static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>>> DBusMessage *msg, void *user_data)
>>> {
>>> - /* TODO: Implement */
>>> - return btd_error_failed(msg, "Not implemented");
>>> + struct characteristic *chrc = user_data;
>>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>>> + const char *sender = dbus_message_get_sender(msg);
>>> + struct async_dbus_op *op;
>>> + struct notify_client *client;
>>> +
>>> + if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
>>> + chrc->props & BT_GATT_CHRC_PROP_INDICATE))
>>> + return btd_error_not_supported(msg);
>>> +
>>> + /* Each client can only have one active notify session. */
>>> + client = queue_find(chrc->notify_clients, match_notify_sender, sender);
>>> + if (client)
>>> + return client->notify_id ?
>>> + btd_error_failed(msg, "Already notifying") :
>>> + btd_error_in_progress(msg);
>>> +
>>> + client = notify_client_create(chrc, sender);
>>> + if (!client)
>>> + return btd_error_failed(msg, "Failed allocate notify session");
>>> +
>>> + op = new0(struct async_dbus_op, 1);
>>> + if (!op) {
>>> + notify_client_unref(client);
>>> + return btd_error_failed(msg, "Failed to initialize request");
>>> + }
>>> +
>>> + /*
>>> + * Add to the ref count so that a disconnect during register won't free
>>> + * the client instance.
>>> + */
>>> + op->data = notify_client_ref(client);
>>> + op->msg = dbus_message_ref(msg);
>>> +
>>> + queue_push_tail(chrc->notify_clients, client);
>>> +
>>> + if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
>>> + register_notify_cb, notify_cb,
>>> + op, async_dbus_op_free))
>>> + return NULL;
>>> +
>>> + queue_remove(chrc->notify_clients, client);
>>> + async_dbus_op_free(op);
>>> +
>>> + /* Directly free the client */
>>> + notify_client_free(client);
>>> +
>>> + return btd_error_failed(msg, "Failed to register notify session");
>>> }
>>>
>>> static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
>>> @@ -992,6 +1221,13 @@ static struct characteristic *characteristic_create(
>>> return NULL;
>>> }
>>>
>>> + chrc->notify_clients = queue_new();
>>> + if (!chrc->notify_clients) {
>>> + queue_destroy(chrc->descs, NULL);
>>> + free(chrc);
>>> + return NULL;
>>> + }
>>> +
>>> chrc->service = service;
>>>
>>> gatt_db_attribute_get_char_data(attr, &chrc->handle,
>>> @@ -1027,6 +1263,7 @@ static void unregister_characteristic(void *data)
>>>
>>> DBG("Removing GATT characteristic: %s", chrc->path);
>>>
>>> + queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>>> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>>>
>>> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>>> --
>>> 2.2.0.rc0.207.ga3a616c
>>>
>>> --
>>> 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
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Cheers,
> Arman



--
Luiz Augusto von Dentz

2015-01-06 20:45:22

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

Hi Arman,

On Tue, Jan 6, 2015 at 4:58 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>> On Tue, Jan 6, 2015 at 5:09 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>> Hi Arman,
>>
>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>>> This patch adds the "truncate" argument to gatt_db_attribute_write. This
>>> allows users to specify if the cached value should be truncated when the
>>> write value length is shorter than what is stored in the database. This
>>> allows client code to correctly store a remote attribute value when the
>>> database is used as a client-role cache.
>>> ---
>>> android/gatt.c | 17 +++++++++--------
>>> src/gatt-client.c | 6 +++---
>>> src/shared/gatt-db.c | 18 ++++++++++++++++--
>>> src/shared/gatt-db.h | 1 +
>>> src/shared/gatt-server.c | 6 ++++--
>>> tools/btgatt-server.c | 8 ++++----
>>> unit/test-gatt.c | 6 +++---
>>> 7 files changed, 40 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/android/gatt.c b/android/gatt.c
>>> index 3a35bd6..7f765e0 100644
>>> --- a/android/gatt.c
>>> +++ b/android/gatt.c
>>> @@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>> return;
>>>
>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
>>> - write_confirm, NULL);
>>> + false, write_confirm, NULL);
>>> }
>>>
>>> static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>> @@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>>> /* Signature OK, proceed with write */
>>> bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
>>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>> - &dev->bdaddr, write_confirm, NULL);
>>> + &dev->bdaddr, false,
>>> + write_confirm, NULL);
>>> }
>>> }
>>>
>>> @@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
>>> }
>>>
>>> if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>>> - &dev->bdaddr, attribute_write_cb,
>>> - data)) {
>>> + &dev->bdaddr, false,
>>> + attribute_write_cb, data)) {
>>> queue_remove(dev->pending_requests, data);
>>> free(data);
>>> return ATT_ECODE_UNLIKELY;
>>> @@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
>>> data->length = vlen;
>>>
>>> if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
>>> - &dev->bdaddr, attribute_write_cb,
>>> - data)) {
>>> + &dev->bdaddr, false,
>>> + attribute_write_cb, data)) {
>>> queue_remove(dev->pending_requests, data);
>>> g_free(data->value);
>>> free(data);
>>> @@ -6805,7 +6806,7 @@ static void register_gap_service(void)
>>> value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
>>> gatt_db_attribute_write(gap_srvc_data.appear, 0,
>>> (void *) &value, sizeof(value),
>>> - ATT_OP_WRITE_REQ, NULL,
>>> + ATT_OP_WRITE_REQ, NULL, false,
>>> write_confirm, NULL);
>>> }
>>>
>>> @@ -6822,7 +6823,7 @@ static void register_gap_service(void)
>>> value = PERIPHERAL_PRIVACY_DISABLE;
>>> gatt_db_attribute_write(gap_srvc_data.priv, 0,
>>> &value, sizeof(value),
>>> - ATT_OP_WRITE_REQ, NULL,
>>> + ATT_OP_WRITE_REQ, NULL, false,
>>> write_confirm, NULL);
>>> }
>>>
>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>>> index 1edbafc..79e1d26 100644
>>> --- a/src/gatt-client.c
>>> +++ b/src/gatt-client.c
>>> @@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
>>> return;
>>> }
>>>
>>> - gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
>>> + gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
>>> write_descriptor_cb, desc);
>>>
>>> /*
>>> @@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
>>> }
>>>
>>> gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
>>> - write_characteristic_cb, chrc);
>>> + true, write_characteristic_cb, chrc);
>>
>>
>> I think it would be better to have some means to truncate the value
>> separately, e.g. gatt_db_attribute_truncate.
>>
>
> Would this just truncate the value to the specified length? Or would
> we want a "set_value" function that works like write but truncates it
> if needed?

Maybe a reset would be enough so once we start read operation we reset
the current value, how about it?

>>> /*
>>> * If the value length is exactly MTU-1, then we may not have read the
>>> @@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
>>> * signal so that we propagate the notification/indication to
>>> * applications.
>>> */
>>> - gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
>>> write_characteristic_cb, chrc);
>>> }
>>>
>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>> index 74c3e70..87e77d1 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
>>> return false;
>>> }
>>>
>>> +static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
>>> + uint16_t offset,
>>> + bool truncate)
>>> +{
>>> + unsigned diff;
>>> +
>>> + if (!attrib->value || offset >= attrib->value_len)
>>> + return true;
>>> +
>>> + diff = attrib->value_len - offset;
>>> +
>>> + return truncate ? len != diff : len > diff;
>>> +}
>>> +
>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> const uint8_t *value, size_t len,
>>> uint8_t opcode, bdaddr_t *bdaddr,
>>> + bool truncate,
>>> gatt_db_attribute_write_t func,
>>> void *user_data)
>>> {
>>> @@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> }
>>>
>>> /* For values stored in db allocate on demand */
>>> - if (!attrib->value || offset >= attrib->value_len ||
>>> - len > (unsigned) (attrib->value_len - offset)) {
>>> + if (should_resize_value(attrib, len, offset, truncate)) {
>>> void *buf;
>>>
>>> buf = realloc(attrib->value, len + offset);
>>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>>> index f188944..2467852 100644
>>> --- a/src/shared/gatt-db.h
>>> +++ b/src/shared/gatt-db.h
>>> @@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> const uint8_t *value, size_t len,
>>> uint8_t opcode, bdaddr_t *bdaddr,
>>> + bool truncate,
>>> gatt_db_attribute_write_t func,
>>> void *user_data);
>>>
>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>> index 00f36fd..2af7cf1 100644
>>> --- a/src/shared/gatt-server.c
>>> +++ b/src/shared/gatt-server.c
>>> @@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
>>> server->pending_write_op = op;
>>>
>>> if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
>>> - NULL, write_complete_cb, op))
>>> + NULL, false,
>>> + write_complete_cb, op))
>>> return;
>>>
>>> if (op)
>>> @@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>> status = gatt_db_attribute_write(attr, next->offset,
>>> next->value, next->length,
>>> BT_ATT_OP_EXEC_WRITE_REQ, NULL,
>>> - exec_write_complete_cb, server);
>>> + false, exec_write_complete_cb,
>>> + server);
>>>
>>> prep_write_data_destroy(next);
>>>
>>> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
>>> index f6ad8c3..393a071 100644
>>> --- a/tools/btgatt-server.c
>>> +++ b/tools/btgatt-server.c
>>> @@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
>>> gatt_db_attribute_write(tmp, 0, (void *) &appearance,
>>> sizeof(appearance),
>>> BT_ATT_OP_WRITE_REQ,
>>> - NULL, confirm_write,
>>> - NULL);
>>> + NULL, true,
>>> + confirm_write, NULL);
>>>
>>> gatt_db_service_set_active(service, true);
>>> }
>>> @@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
>>> NULL, NULL, server);
>>> gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
>>> BT_ATT_OP_WRITE_REQ,
>>> - NULL, confirm_write,
>>> - NULL);
>>> + NULL, true,
>>> + confirm_write, NULL);
>>>
>>> /* HR Control Point Characteristic */
>>> bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>> index 169fbd3..7a5081d 100644
>>> --- a/unit/test-gatt.c
>>> +++ b/unit/test-gatt.c
>>> @@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,
>>>
>>> g_assert(attrib != NULL);
>>>
>>> - gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
>>> - NULL);
>>> + gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
>>> + att_write_cb, NULL);
>>>
>>> return attrib;
>>> }
>>> @@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
>>> desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
>>> NULL, NULL);
>>>
>>> - gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
>>> + gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
>>> att_write_cb, NULL);
>>>
>>> return desc_att;
>>> --
>>> 2.2.0.rc0.207.ga3a616c
>>>
>>> --
>>> 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
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> Cheers,
> Arman



--
Luiz Augusto von Dentz

2015-01-06 18:58:28

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

Hi Luiz,

> On Tue, Jan 6, 2015 at 5:09 AM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>> This patch adds the "truncate" argument to gatt_db_attribute_write. This
>> allows users to specify if the cached value should be truncated when the
>> write value length is shorter than what is stored in the database. This
>> allows client code to correctly store a remote attribute value when the
>> database is used as a client-role cache.
>> ---
>> android/gatt.c | 17 +++++++++--------
>> src/gatt-client.c | 6 +++---
>> src/shared/gatt-db.c | 18 ++++++++++++++++--
>> src/shared/gatt-db.h | 1 +
>> src/shared/gatt-server.c | 6 ++++--
>> tools/btgatt-server.c | 8 ++++----
>> unit/test-gatt.c | 6 +++---
>> 7 files changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index 3a35bd6..7f765e0 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>> return;
>>
>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
>> - write_confirm, NULL);
>> + false, write_confirm, NULL);
>> }
>>
>> static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>> @@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>> /* Signature OK, proceed with write */
>> bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
>> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>> - &dev->bdaddr, write_confirm, NULL);
>> + &dev->bdaddr, false,
>> + write_confirm, NULL);
>> }
>> }
>>
>> @@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
>> }
>>
>> if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
>> - &dev->bdaddr, attribute_write_cb,
>> - data)) {
>> + &dev->bdaddr, false,
>> + attribute_write_cb, data)) {
>> queue_remove(dev->pending_requests, data);
>> free(data);
>> return ATT_ECODE_UNLIKELY;
>> @@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
>> data->length = vlen;
>>
>> if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
>> - &dev->bdaddr, attribute_write_cb,
>> - data)) {
>> + &dev->bdaddr, false,
>> + attribute_write_cb, data)) {
>> queue_remove(dev->pending_requests, data);
>> g_free(data->value);
>> free(data);
>> @@ -6805,7 +6806,7 @@ static void register_gap_service(void)
>> value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
>> gatt_db_attribute_write(gap_srvc_data.appear, 0,
>> (void *) &value, sizeof(value),
>> - ATT_OP_WRITE_REQ, NULL,
>> + ATT_OP_WRITE_REQ, NULL, false,
>> write_confirm, NULL);
>> }
>>
>> @@ -6822,7 +6823,7 @@ static void register_gap_service(void)
>> value = PERIPHERAL_PRIVACY_DISABLE;
>> gatt_db_attribute_write(gap_srvc_data.priv, 0,
>> &value, sizeof(value),
>> - ATT_OP_WRITE_REQ, NULL,
>> + ATT_OP_WRITE_REQ, NULL, false,
>> write_confirm, NULL);
>> }
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index 1edbafc..79e1d26 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
>> return;
>> }
>>
>> - gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
>> + gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
>> write_descriptor_cb, desc);
>>
>> /*
>> @@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
>> }
>>
>> gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
>> - write_characteristic_cb, chrc);
>> + true, write_characteristic_cb, chrc);
>
>
> I think it would be better to have some means to truncate the value
> separately, e.g. gatt_db_attribute_truncate.
>

Would this just truncate the value to the specified length? Or would
we want a "set_value" function that works like write but truncates it
if needed?

>> /*
>> * If the value length is exactly MTU-1, then we may not have read the
>> @@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> * signal so that we propagate the notification/indication to
>> * applications.
>> */
>> - gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
>> write_characteristic_cb, chrc);
>> }
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 74c3e70..87e77d1 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
>> return false;
>> }
>>
>> +static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
>> + uint16_t offset,
>> + bool truncate)
>> +{
>> + unsigned diff;
>> +
>> + if (!attrib->value || offset >= attrib->value_len)
>> + return true;
>> +
>> + diff = attrib->value_len - offset;
>> +
>> + return truncate ? len != diff : len > diff;
>> +}
>> +
>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> const uint8_t *value, size_t len,
>> uint8_t opcode, bdaddr_t *bdaddr,
>> + bool truncate,
>> gatt_db_attribute_write_t func,
>> void *user_data)
>> {
>> @@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> }
>>
>> /* For values stored in db allocate on demand */
>> - if (!attrib->value || offset >= attrib->value_len ||
>> - len > (unsigned) (attrib->value_len - offset)) {
>> + if (should_resize_value(attrib, len, offset, truncate)) {
>> void *buf;
>>
>> buf = realloc(attrib->value, len + offset);
>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>> index f188944..2467852 100644
>> --- a/src/shared/gatt-db.h
>> +++ b/src/shared/gatt-db.h
>> @@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> const uint8_t *value, size_t len,
>> uint8_t opcode, bdaddr_t *bdaddr,
>> + bool truncate,
>> gatt_db_attribute_write_t func,
>> void *user_data);
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 00f36fd..2af7cf1 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
>> server->pending_write_op = op;
>>
>> if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
>> - NULL, write_complete_cb, op))
>> + NULL, false,
>> + write_complete_cb, op))
>> return;
>>
>> if (op)
>> @@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>> status = gatt_db_attribute_write(attr, next->offset,
>> next->value, next->length,
>> BT_ATT_OP_EXEC_WRITE_REQ, NULL,
>> - exec_write_complete_cb, server);
>> + false, exec_write_complete_cb,
>> + server);
>>
>> prep_write_data_destroy(next);
>>
>> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
>> index f6ad8c3..393a071 100644
>> --- a/tools/btgatt-server.c
>> +++ b/tools/btgatt-server.c
>> @@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
>> gatt_db_attribute_write(tmp, 0, (void *) &appearance,
>> sizeof(appearance),
>> BT_ATT_OP_WRITE_REQ,
>> - NULL, confirm_write,
>> - NULL);
>> + NULL, true,
>> + confirm_write, NULL);
>>
>> gatt_db_service_set_active(service, true);
>> }
>> @@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
>> NULL, NULL, server);
>> gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
>> BT_ATT_OP_WRITE_REQ,
>> - NULL, confirm_write,
>> - NULL);
>> + NULL, true,
>> + confirm_write, NULL);
>>
>> /* HR Control Point Characteristic */
>> bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>> index 169fbd3..7a5081d 100644
>> --- a/unit/test-gatt.c
>> +++ b/unit/test-gatt.c
>> @@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,
>>
>> g_assert(attrib != NULL);
>>
>> - gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
>> - NULL);
>> + gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
>> + att_write_cb, NULL);
>>
>> return attrib;
>> }
>> @@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
>> desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
>> NULL, NULL);
>>
>> - gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
>> + gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
>> att_write_cb, NULL);
>>
>> return desc_att;
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> 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
>
>
>
> --
> Luiz Augusto von Dentz

Cheers,
Arman

2015-01-06 18:56:48

by Arman Uguray

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 2/8] core: gatt: Implement GattCharacteristic1.StartNotify

Hi Luiz,

> On Tue, Jan 6, 2015 at 4:33 AM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
>> This patch implements the StartNotify method of
>> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
>> session to the D-Bus sender which internally registers a unique
>> notify handler with the bt_gatt_client. Each received notification
>> or indication causes a PropertiesChanged signal to be emitted for the
>> "Value" property.
>>
>> The notify handler gets automatically unregistered when sender
>> disconnects from D-Bus.
>
> I recall we discussed removing not implementing StartNofify in favor
> of an API where the application can register for notification rather
> than enabling it on every connection, furthermore CCC is currently
> being exposed via GattDescriptor1 which might conflict with this.
>

I'm not sure about removing StartNotify/StopNotify just yet. My
intention is to first get everything specified in doc/gatt-api.txt
implemented and see how it works (since the API is experimental, I
think this is OK). And once we have a GattProfile1 API we can remove
these. I think that StartNotify/StopNotify addresses most use cases
for now, and I want to at least support those platforms that have
already developed against doc/gatt-api.txt (ChromiumOS, Tizen, etc)
behind the experimental flag.

GattDescriptor1 won't be an issue since GattDescriptor1.WriteValue
will fail with Error.NotSupported if called on a CCC descriptor.

>> ---
>> src/gatt-client.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 246 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index 8bbb03d..5c030ee 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -87,6 +87,9 @@ struct characteristic {
>> bool in_write;
>>
>> struct queue *descs;
>> +
>> + bool notifying;
>> + struct queue *notify_clients;
>> };
>>
>> struct descriptor {
>> @@ -230,7 +233,9 @@ static void async_dbus_op_free(void *data)
>> {
>> struct async_dbus_op *op = data;
>>
>> - dbus_message_unref(op->msg);
>> + if (op->msg)
>> + dbus_message_unref(op->msg);
>> +
>> free(op);
>> }
>>
>> @@ -674,12 +679,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
>> static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
>> DBusMessageIter *iter, void *data)
>> {
>> - dbus_bool_t notifying = FALSE;
>> -
>> - /*
>> - * TODO: Return the correct value here once StartNotify and StopNotify
>> - * methods are implemented.
>> - */
>> + struct characteristic *chrc = data;
>> + dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>>
>> dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
>>
>> @@ -895,11 +896,239 @@ done_async:
>> return NULL;
>> }
>>
>> +struct notify_client {
>> + struct characteristic *chrc;
>> + int ref_count;
>> + char *owner;
>> + guint watch;
>> + unsigned int notify_id;
>> +};
>> +
>> +static void notify_client_free(struct notify_client *client)
>> +{
>> + DBG("owner %s", client->owner);
>> +
>> + g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
>> + free(client->owner);
>> + free(client);
>> +}
>> +
>> +static void notify_client_unref(void *data)
>> +{
>> + struct notify_client *client = data;
>> +
>> + DBG("owner %s", client->owner);
>> +
>> + if (__sync_sub_and_fetch(&client->ref_count, 1))
>> + return;
>> +
>> + notify_client_free(client);
>> +}
>> +
>> +static struct notify_client *notify_client_ref(struct notify_client *client)
>> +{
>> + DBG("owner %s", client->owner);
>> +
>> + __sync_fetch_and_add(&client->ref_count, 1);
>> +
>> + return client;
>> +}
>> +
>> +static bool match_notifying(const void *a, const void *b)
>> +{
>> + const struct notify_client *client = a;
>> +
>> + return !!client->notify_id;
>> +}
>> +
>> +static void update_notifying(struct characteristic *chrc)
>> +{
>> + if (!chrc->notifying)
>> + return;
>> +
>> + if (queue_find(chrc->notify_clients, match_notifying, NULL))
>> + return;
>> +
>> + chrc->notifying = false;
>> +
>> + g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
>> + GATT_CHARACTERISTIC_IFACE,
>> + "Notifying");
>> +}
>> +
>> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
>> +{
>> + struct notify_client *client = user_data;
>> + struct characteristic *chrc = client->chrc;
>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>> +
>> + DBG("owner %s", client->owner);
>> +
>> + queue_remove(chrc->notify_clients, client);
>> + bt_gatt_client_unregister_notify(gatt, client->notify_id);
>> +
>> + update_notifying(chrc);
>> +
>> + notify_client_unref(client);
>> +}
>> +
>> +static struct notify_client *notify_client_create(struct characteristic *chrc,
>> + const char *owner)
>> +{
>> + struct notify_client *client;
>> +
>> + client = new0(struct notify_client, 1);
>> + if (!client)
>> + return NULL;
>> +
>> + client->chrc = chrc;
>> + client->owner = strdup(owner);
>> + if (!client->owner) {
>> + free(client);
>> + return NULL;
>> + }
>> +
>> + client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
>> + owner, notify_client_disconnect,
>> + client, NULL);
>> + if (!client->watch) {
>> + free(client->owner);
>> + free(client);
>> + return NULL;
>> + }
>> +
>> + return notify_client_ref(client);
>> +}
>> +
>> +static bool match_notify_sender(const void *a, const void *b)
>> +{
>> + const struct notify_client *client = a;
>> + const char *sender = b;
>> +
>> + return strcmp(client->owner, sender) == 0;
>> +}
>> +
>> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> + uint16_t length, void *user_data)
>> +{
>> + struct async_dbus_op *op = user_data;
>> + struct notify_client *client = op->data;
>> + struct characteristic *chrc = client->chrc;
>> +
>> + /*
>> + * Even if the value didn't change, we want to send a PropertiesChanged
>> + * signal so that we propagate the notification/indication to
>> + * applications.
>> + */
>> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
>> + write_characteristic_cb, chrc);
>> +}
>> +
>> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
>> + void *user_data)
>> +{
>> + struct async_dbus_op *op = user_data;
>> + struct notify_client *client = op->data;
>> + struct characteristic *chrc = client->chrc;
>> + DBusMessage *reply;
>> +
>> + /* Make sure that an interim disconnect did not remove the client */
>> + if (!queue_find(chrc->notify_clients, NULL, client)) {
>> + bt_gatt_client_unregister_notify(chrc->service->client->gatt,
>> + id);
>> + notify_client_unref(client);
>> +
>> + reply = btd_error_failed(op->msg,
>> + "Characteristic not available");
>> + goto done;
>> + }
>> +
>> + /*
>> + * Drop the reference count that we added when registering the callback.
>> + */
>> + notify_client_unref(client);
>> +
>> + if (!id) {
>> + queue_remove(chrc->notify_clients, client);
>> + notify_client_free(client);
>> +
>> + reply = create_gatt_dbus_error(op->msg, att_ecode);
>> +
>> + goto done;
>> + }
>> +
>> + client->notify_id = id;
>> +
>> + if (!chrc->notifying) {
>> + chrc->notifying = true;
>> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> + chrc->path, GATT_CHARACTERISTIC_IFACE,
>> + "Notifying");
>> + }
>> +
>> + reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
>> +
>> +done:
>> + if (reply)
>> + g_dbus_send_message(btd_get_dbus_connection(), reply);
>> + else
>> + error("Failed to construct D-Bus message reply");
>> +
>> + dbus_message_unref(op->msg);
>> + op->msg = NULL;
>> +}
>> +
>> static DBusMessage *characteristic_start_notify(DBusConnection *conn,
>> DBusMessage *msg, void *user_data)
>> {
>> - /* TODO: Implement */
>> - return btd_error_failed(msg, "Not implemented");
>> + struct characteristic *chrc = user_data;
>> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
>> + const char *sender = dbus_message_get_sender(msg);
>> + struct async_dbus_op *op;
>> + struct notify_client *client;
>> +
>> + if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
>> + chrc->props & BT_GATT_CHRC_PROP_INDICATE))
>> + return btd_error_not_supported(msg);
>> +
>> + /* Each client can only have one active notify session. */
>> + client = queue_find(chrc->notify_clients, match_notify_sender, sender);
>> + if (client)
>> + return client->notify_id ?
>> + btd_error_failed(msg, "Already notifying") :
>> + btd_error_in_progress(msg);
>> +
>> + client = notify_client_create(chrc, sender);
>> + if (!client)
>> + return btd_error_failed(msg, "Failed allocate notify session");
>> +
>> + op = new0(struct async_dbus_op, 1);
>> + if (!op) {
>> + notify_client_unref(client);
>> + return btd_error_failed(msg, "Failed to initialize request");
>> + }
>> +
>> + /*
>> + * Add to the ref count so that a disconnect during register won't free
>> + * the client instance.
>> + */
>> + op->data = notify_client_ref(client);
>> + op->msg = dbus_message_ref(msg);
>> +
>> + queue_push_tail(chrc->notify_clients, client);
>> +
>> + if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
>> + register_notify_cb, notify_cb,
>> + op, async_dbus_op_free))
>> + return NULL;
>> +
>> + queue_remove(chrc->notify_clients, client);
>> + async_dbus_op_free(op);
>> +
>> + /* Directly free the client */
>> + notify_client_free(client);
>> +
>> + return btd_error_failed(msg, "Failed to register notify session");
>> }
>>
>> static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
>> @@ -992,6 +1221,13 @@ static struct characteristic *characteristic_create(
>> return NULL;
>> }
>>
>> + chrc->notify_clients = queue_new();
>> + if (!chrc->notify_clients) {
>> + queue_destroy(chrc->descs, NULL);
>> + free(chrc);
>> + return NULL;
>> + }
>> +
>> chrc->service = service;
>>
>> gatt_db_attribute_get_char_data(attr, &chrc->handle,
>> @@ -1027,6 +1263,7 @@ static void unregister_characteristic(void *data)
>>
>> DBG("Removing GATT characteristic: %s", chrc->path);
>>
>> + queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
>> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>>
>> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> 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
>
>
>
> --
> Luiz Augusto von Dentz

Cheers,
Arman

2015-01-06 13:17:32

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 6/8] core: gatt: Reference count chrcs and descs.

Hi Arman,

On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
> Many of the GATT D-Bus API methods are asynchronous and their callbacks
> depend on characteristic and descriptor instances to be alive when they
> execute. This patch makes characteristics and descriptors reference
> counted so that an interim disconnect or "Service Changed" won't
> immediately free up those instances.

IMO this logic needs to be reverted, the pending operations should be
stored in the respective struct not the other way around since that
way you can cancel an outstanding operation if the object is to be
freed.

> ---
> src/gatt-client.c | 179 ++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 125 insertions(+), 54 deletions(-)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 6897f30..c4f3582 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -83,6 +83,8 @@ struct characteristic {
> bt_uuid_t uuid;
> char *path;
>
> + int ref_count;
> +
> bool in_read;
> bool in_write;
>
> @@ -99,10 +101,49 @@ struct descriptor {
> bt_uuid_t uuid;
> char *path;
>
> + int ref_count;
> +
> bool in_read;
> bool in_write;
> };
>
> +static struct characteristic *characteristic_ref(struct characteristic *chrc)
> +{
> + __sync_fetch_and_add(&chrc->ref_count, 1);
> +
> + return chrc;
> +}
> +
> +static void characteristic_unref(void *data)
> +{
> + struct characteristic *chrc = data;
> +
> + if (__sync_sub_and_fetch(&chrc->ref_count, 1))
> + return;
> +
> + queue_destroy(chrc->descs, NULL); /* List should be empty here */
> + g_free(chrc->path);
> + free(chrc);
> +}
> +
> +static struct descriptor *descriptor_ref(struct descriptor *desc)
> +{
> + __sync_fetch_and_add(&desc->ref_count, 1);
> +
> + return desc;
> +}
> +
> +static void descriptor_unref(void *data)
> +{
> + struct descriptor *desc = data;
> +
> + if (__sync_sub_and_fetch(&desc->ref_count, 1))
> + return;
> +
> + g_free(desc->path);
> + free(desc);
> +}
> +
> static bool uuid_cmp(const bt_uuid_t *uuid, uint16_t u16)
> {
> bt_uuid_t uuid16;
> @@ -220,6 +261,7 @@ static bool parse_value_arg(DBusMessage *msg, uint8_t **value,
> }
>
> typedef bool (*async_dbus_op_complete_t)(void *data);
> +typedef void (*async_dbus_op_destroy_t)(void *data);
>
> struct async_dbus_op {
> int ref_count;
> @@ -227,12 +269,16 @@ struct async_dbus_op {
> void *data;
> uint16_t offset;
> async_dbus_op_complete_t complete;
> + async_dbus_op_destroy_t destroy;
> };
>
> static void async_dbus_op_free(void *data)
> {
> struct async_dbus_op *op = data;
>
> + if (op->destroy)
> + op->destroy(op->data);
> +
> if (op->msg)
> dbus_message_unref(op->msg);
>
> @@ -389,7 +435,8 @@ static DBusMessage *descriptor_read_value(DBusConnection *conn,
> return btd_error_failed(msg, "Failed to initialize request");
>
> op->msg = dbus_message_ref(msg);
> - op->data = desc;
> + op->data = descriptor_ref(desc);
> + op->destroy = descriptor_unref;
>
> if (bt_gatt_client_read_value(gatt, desc->handle, desc_read_cb,
> async_dbus_op_ref(op),
> @@ -444,7 +491,8 @@ static bool start_long_write(DBusMessage *msg, uint16_t handle,
> struct bt_gatt_client *gatt,
> bool reliable, const uint8_t *value,
> size_t value_len, void *data,
> - async_dbus_op_complete_t complete)
> + async_dbus_op_complete_t complete,
> + async_dbus_op_destroy_t destroy)
> {
> struct async_dbus_op *op;
>
> @@ -455,6 +503,7 @@ static bool start_long_write(DBusMessage *msg, uint16_t handle,
> op->msg = dbus_message_ref(msg);
> op->data = data;
> op->complete = complete;
> + op->destroy = destroy;
>
> if (bt_gatt_client_write_long_value(gatt, reliable, handle,
> 0, value, value_len,
> @@ -471,7 +520,8 @@ static bool start_write_request(DBusMessage *msg, uint16_t handle,
> struct bt_gatt_client *gatt,
> const uint8_t *value, size_t value_len,
> void *data,
> - async_dbus_op_complete_t complete)
> + async_dbus_op_complete_t complete,
> + async_dbus_op_destroy_t destroy)
> {
> struct async_dbus_op *op;
>
> @@ -482,6 +532,7 @@ static bool start_write_request(DBusMessage *msg, uint16_t handle,
> op->msg = dbus_message_ref(msg);
> op->data = data;
> op->complete = complete;
> + op->destroy = destroy;
>
> if (bt_gatt_client_write_value(gatt, handle, value, value_len,
> write_cb, op,
> @@ -536,11 +587,13 @@ static DBusMessage *descriptor_write_value(DBusConnection *conn,
> if (value_len <= (unsigned) bt_gatt_client_get_mtu(gatt) - 3)
> result = start_write_request(msg, desc->handle, gatt, value,
> value_len, desc,
> - desc_write_complete);
> + desc_write_complete,
> + descriptor_unref);
> else
> result = start_long_write(msg, desc->handle, gatt, false, value,
> value_len, desc,
> - desc_write_complete);
> + desc_write_complete,
> + descriptor_unref);
>
> if (!result)
> return btd_error_failed(msg, "Failed to initiate write");
> @@ -571,14 +624,6 @@ static const GDBusMethodTable descriptor_methods[] = {
> { }
> };
>
> -static void descriptor_free(void *data)
> -{
> - struct descriptor *desc = data;
> -
> - g_free(desc->path);
> - free(desc);
> -}
> -
> static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,
> struct characteristic *chrc)
> {
> @@ -600,10 +645,11 @@ static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,
> GATT_DESCRIPTOR_IFACE,
> descriptor_methods, NULL,
> descriptor_properties,
> - desc, descriptor_free)) {
> + descriptor_ref(desc),
> + descriptor_unref)) {
> error("Unable to register GATT descriptor with handle 0x%04x",
> desc->handle);
> - descriptor_free(desc);
> + descriptor_unref(desc);
>
> return NULL;
> }
> @@ -622,6 +668,8 @@ static void unregister_descriptor(void *data)
>
> DBG("Removing GATT descriptor: %s", desc->path);
>
> + desc->chrc = NULL;
> +
> g_dbus_unregister_interface(btd_get_dbus_connection(), desc->path,
> GATT_DESCRIPTOR_IFACE);
> }
> @@ -803,7 +851,8 @@ static DBusMessage *characteristic_read_value(DBusConnection *conn,
> return btd_error_failed(msg, "Failed to initialize request");
>
> op->msg = dbus_message_ref(msg);
> - op->data = chrc;
> + op->data = characteristic_ref(chrc);
> + op->destroy = characteristic_unref;
>
> if (bt_gatt_client_read_value(gatt, chrc->value_handle, chrc_read_cb,
> async_dbus_op_ref(op),
> @@ -877,13 +926,16 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
>
> if (value_len <= (unsigned) mtu - 3)
> result = start_write_request(msg, chrc->value_handle,
> - gatt, value,
> - value_len, chrc,
> - chrc_write_complete);
> + gatt, value, value_len,
> + characteristic_ref(chrc),
> + chrc_write_complete,
> + characteristic_unref);
> else
> result = start_long_write(msg, chrc->value_handle, gatt,
> - false, value, value_len, chrc,
> - chrc_write_complete);
> + false, value, value_len,
> + characteristic_ref(chrc),
> + chrc_write_complete,
> + characteristic_unref);
>
> if (result)
> goto done_async;
> @@ -1016,12 +1068,33 @@ static bool match_notify_sender(const void *a, const void *b)
> return strcmp(client->owner, sender) == 0;
> }
>
> +struct register_notify_op {
> + int ref_count;
> + struct notify_client *client;
> + struct characteristic *chrc;
> +};
> +
> +static void register_notify_op_unref(void *data)
> +{
> + struct register_notify_op *op = data;
> +
> + DBG("Released register_notify_op");
> +
> + if (__sync_sub_and_fetch(&op->ref_count, 1))
> + return;
> +
> + notify_client_unref(op->client);
> + characteristic_unref(op->chrc);
> +
> + free(op);
> +}
> +
> static void notify_cb(uint16_t value_handle, const uint8_t *value,
> uint16_t length, void *user_data)
> {
> struct async_dbus_op *op = user_data;
> - struct notify_client *client = op->data;
> - struct characteristic *chrc = client->chrc;
> + struct register_notify_op *notify_op = op->data;
> + struct characteristic *chrc = notify_op->chrc;
>
> /*
> * Even if the value didn't change, we want to send a PropertiesChanged
> @@ -1036,26 +1109,24 @@ static void register_notify_cb(unsigned int id, uint16_t att_ecode,
> void *user_data)
> {
> struct async_dbus_op *op = user_data;
> - struct notify_client *client = op->data;
> - struct characteristic *chrc = client->chrc;
> + struct register_notify_op *notify_op = op->data;
> + struct notify_client *client = notify_op->client;
> + struct characteristic *chrc = notify_op->chrc;
> DBusMessage *reply;
>
> - /* Make sure that an interim disconnect did not remove the client */
> - if (!queue_find(chrc->notify_clients, NULL, client)) {
> + /*
> + * Make sure that an interim disconnect or "Service Changed" did not
> + * remove the client
> + */
> + if (!chrc->service || !queue_find(chrc->notify_clients, NULL, client)) {
> bt_gatt_client_unregister_notify(chrc->service->client->gatt,
> id);
> - notify_client_unref(client);
>
> reply = btd_error_failed(op->msg,
> "Characteristic not available");
> goto done;
> }
>
> - /*
> - * Drop the reference count that we added when registering the callback.
> - */
> - notify_client_unref(client);
> -
> if (!id) {
> queue_remove(chrc->notify_clients, client);
> notify_client_free(client);
> @@ -1094,6 +1165,7 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
> const char *sender = dbus_message_get_sender(msg);
> struct async_dbus_op *op;
> struct notify_client *client;
> + struct register_notify_op *notify_op;
>
> if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
> chrc->props & BT_GATT_CHRC_PROP_INDICATE))
> @@ -1110,20 +1182,28 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
> if (!client)
> return btd_error_failed(msg, "Failed allocate notify session");
>
> + notify_op = new0(struct register_notify_op, 1);
> + if (!notify_op) {
> + notify_client_unref(client);
> + return btd_error_failed(msg, "Failed to initialize request");
> + }
> +
> + notify_op->ref_count = 1;
> + notify_op->client = client;
> + notify_op->chrc = characteristic_ref(chrc);
> +
> op = new0(struct async_dbus_op, 1);
> if (!op) {
> - notify_client_unref(client);
> + register_notify_op_unref(notify_op);
> return btd_error_failed(msg, "Failed to initialize request");
> }
>
> - /*
> - * Add to the ref count so that a disconnect during register won't free
> - * the client instance.
> - */
> - op->data = notify_client_ref(client);
> + op->data = notify_op;
> op->msg = dbus_message_ref(msg);
> + op->destroy = register_notify_op_unref;
>
> - queue_push_tail(chrc->notify_clients, client);
> + /* The characteristic owns a reference to the client */
> + queue_push_tail(chrc->notify_clients, notify_client_ref(client));
>
> if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
> register_notify_cb, notify_cb,
> @@ -1133,9 +1213,6 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
> queue_remove(chrc->notify_clients, client);
> async_dbus_op_free(op);
>
> - /* Directly free the client */
> - notify_client_free(client);
> -
> return btd_error_failed(msg, "Failed to register notify session");
> }
>
> @@ -1220,15 +1297,6 @@ static const GDBusMethodTable characteristic_methods[] = {
> { }
> };
>
> -static void characteristic_free(void *data)
> -{
> - struct characteristic *chrc = data;
> -
> - queue_destroy(chrc->descs, NULL); /* List should be empty here */
> - g_free(chrc->path);
> - free(chrc);
> -}
> -
> static struct characteristic *characteristic_create(
> struct gatt_db_attribute *attr,
> struct service *service)
> @@ -1269,10 +1337,11 @@ static struct characteristic *characteristic_create(
> GATT_CHARACTERISTIC_IFACE,
> characteristic_methods, NULL,
> characteristic_properties,
> - chrc, characteristic_free)) {
> + characteristic_ref(chrc),
> + characteristic_unref)) {
> error("Unable to register GATT characteristic with handle "
> "0x%04x", chrc->handle);
> - characteristic_free(chrc);
> + characteristic_unref(chrc);
>
> return NULL;
> }
> @@ -1291,6 +1360,8 @@ static void unregister_characteristic(void *data)
> queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>
> + chrc->service = NULL;
> +
> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> GATT_CHARACTERISTIC_IFACE);
> }
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> 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



--
Luiz Augusto von Dentz

2015-01-06 13:09:22

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

Hi Arman,

On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
> This patch adds the "truncate" argument to gatt_db_attribute_write. This
> allows users to specify if the cached value should be truncated when the
> write value length is shorter than what is stored in the database. This
> allows client code to correctly store a remote attribute value when the
> database is used as a client-role cache.
> ---
> android/gatt.c | 17 +++++++++--------
> src/gatt-client.c | 6 +++---
> src/shared/gatt-db.c | 18 ++++++++++++++++--
> src/shared/gatt-db.h | 1 +
> src/shared/gatt-server.c | 6 ++++--
> tools/btgatt-server.c | 8 ++++----
> unit/test-gatt.c | 6 +++---
> 7 files changed, 40 insertions(+), 22 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 3a35bd6..7f765e0 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> return;
>
> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
> - write_confirm, NULL);
> + false, write_confirm, NULL);
> }
>
> static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> @@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> /* Signature OK, proceed with write */
> bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
> gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
> - &dev->bdaddr, write_confirm, NULL);
> + &dev->bdaddr, false,
> + write_confirm, NULL);
> }
> }
>
> @@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
> }
>
> if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
> - &dev->bdaddr, attribute_write_cb,
> - data)) {
> + &dev->bdaddr, false,
> + attribute_write_cb, data)) {
> queue_remove(dev->pending_requests, data);
> free(data);
> return ATT_ECODE_UNLIKELY;
> @@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
> data->length = vlen;
>
> if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
> - &dev->bdaddr, attribute_write_cb,
> - data)) {
> + &dev->bdaddr, false,
> + attribute_write_cb, data)) {
> queue_remove(dev->pending_requests, data);
> g_free(data->value);
> free(data);
> @@ -6805,7 +6806,7 @@ static void register_gap_service(void)
> value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
> gatt_db_attribute_write(gap_srvc_data.appear, 0,
> (void *) &value, sizeof(value),
> - ATT_OP_WRITE_REQ, NULL,
> + ATT_OP_WRITE_REQ, NULL, false,
> write_confirm, NULL);
> }
>
> @@ -6822,7 +6823,7 @@ static void register_gap_service(void)
> value = PERIPHERAL_PRIVACY_DISABLE;
> gatt_db_attribute_write(gap_srvc_data.priv, 0,
> &value, sizeof(value),
> - ATT_OP_WRITE_REQ, NULL,
> + ATT_OP_WRITE_REQ, NULL, false,
> write_confirm, NULL);
> }
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 1edbafc..79e1d26 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
> return;
> }
>
> - gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
> + gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
> write_descriptor_cb, desc);
>
> /*
> @@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
> }
>
> gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
> - write_characteristic_cb, chrc);
> + true, write_characteristic_cb, chrc);


I think it would be better to have some means to truncate the value
separately, e.g. gatt_db_attribute_truncate.

> /*
> * If the value length is exactly MTU-1, then we may not have read the
> @@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
> * signal so that we propagate the notification/indication to
> * applications.
> */
> - gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
> write_characteristic_cb, chrc);
> }
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 74c3e70..87e77d1 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
> return false;
> }
>
> +static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
> + uint16_t offset,
> + bool truncate)
> +{
> + unsigned diff;
> +
> + if (!attrib->value || offset >= attrib->value_len)
> + return true;
> +
> + diff = attrib->value_len - offset;
> +
> + return truncate ? len != diff : len > diff;
> +}
> +
> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> const uint8_t *value, size_t len,
> uint8_t opcode, bdaddr_t *bdaddr,
> + bool truncate,
> gatt_db_attribute_write_t func,
> void *user_data)
> {
> @@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> }
>
> /* For values stored in db allocate on demand */
> - if (!attrib->value || offset >= attrib->value_len ||
> - len > (unsigned) (attrib->value_len - offset)) {
> + if (should_resize_value(attrib, len, offset, truncate)) {
> void *buf;
>
> buf = realloc(attrib->value, len + offset);
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index f188944..2467852 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> const uint8_t *value, size_t len,
> uint8_t opcode, bdaddr_t *bdaddr,
> + bool truncate,
> gatt_db_attribute_write_t func,
> void *user_data);
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 00f36fd..2af7cf1 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
> server->pending_write_op = op;
>
> if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
> - NULL, write_complete_cb, op))
> + NULL, false,
> + write_complete_cb, op))
> return;
>
> if (op)
> @@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
> status = gatt_db_attribute_write(attr, next->offset,
> next->value, next->length,
> BT_ATT_OP_EXEC_WRITE_REQ, NULL,
> - exec_write_complete_cb, server);
> + false, exec_write_complete_cb,
> + server);
>
> prep_write_data_destroy(next);
>
> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
> index f6ad8c3..393a071 100644
> --- a/tools/btgatt-server.c
> +++ b/tools/btgatt-server.c
> @@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
> gatt_db_attribute_write(tmp, 0, (void *) &appearance,
> sizeof(appearance),
> BT_ATT_OP_WRITE_REQ,
> - NULL, confirm_write,
> - NULL);
> + NULL, true,
> + confirm_write, NULL);
>
> gatt_db_service_set_active(service, true);
> }
> @@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
> NULL, NULL, server);
> gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
> BT_ATT_OP_WRITE_REQ,
> - NULL, confirm_write,
> - NULL);
> + NULL, true,
> + confirm_write, NULL);
>
> /* HR Control Point Characteristic */
> bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 169fbd3..7a5081d 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,
>
> g_assert(attrib != NULL);
>
> - gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
> - NULL);
> + gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
> + att_write_cb, NULL);
>
> return attrib;
> }
> @@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
> desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
> NULL, NULL);
>
> - gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
> + gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
> att_write_cb, NULL);
>
> return desc_att;
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> 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



--
Luiz Augusto von Dentz

2015-01-06 12:33:47

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v1 2/8] core: gatt: Implement GattCharacteristic1.StartNotify

Hi Arman,

On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <[email protected]> wrote:
> This patch implements the StartNotify method of
> org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
> session to the D-Bus sender which internally registers a unique
> notify handler with the bt_gatt_client. Each received notification
> or indication causes a PropertiesChanged signal to be emitted for the
> "Value" property.
>
> The notify handler gets automatically unregistered when sender
> disconnects from D-Bus.

I recall we discussed removing not implementing StartNofify in favor
of an API where the application can register for notification rather
than enabling it on every connection, furthermore CCC is currently
being exposed via GattDescriptor1 which might conflict with this.

> ---
> src/gatt-client.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 246 insertions(+), 9 deletions(-)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 8bbb03d..5c030ee 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -87,6 +87,9 @@ struct characteristic {
> bool in_write;
>
> struct queue *descs;
> +
> + bool notifying;
> + struct queue *notify_clients;
> };
>
> struct descriptor {
> @@ -230,7 +233,9 @@ static void async_dbus_op_free(void *data)
> {
> struct async_dbus_op *op = data;
>
> - dbus_message_unref(op->msg);
> + if (op->msg)
> + dbus_message_unref(op->msg);
> +
> free(op);
> }
>
> @@ -674,12 +679,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
> static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
> DBusMessageIter *iter, void *data)
> {
> - dbus_bool_t notifying = FALSE;
> -
> - /*
> - * TODO: Return the correct value here once StartNotify and StopNotify
> - * methods are implemented.
> - */
> + struct characteristic *chrc = data;
> + dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;
>
> dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);
>
> @@ -895,11 +896,239 @@ done_async:
> return NULL;
> }
>
> +struct notify_client {
> + struct characteristic *chrc;
> + int ref_count;
> + char *owner;
> + guint watch;
> + unsigned int notify_id;
> +};
> +
> +static void notify_client_free(struct notify_client *client)
> +{
> + DBG("owner %s", client->owner);
> +
> + g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
> + free(client->owner);
> + free(client);
> +}
> +
> +static void notify_client_unref(void *data)
> +{
> + struct notify_client *client = data;
> +
> + DBG("owner %s", client->owner);
> +
> + if (__sync_sub_and_fetch(&client->ref_count, 1))
> + return;
> +
> + notify_client_free(client);
> +}
> +
> +static struct notify_client *notify_client_ref(struct notify_client *client)
> +{
> + DBG("owner %s", client->owner);
> +
> + __sync_fetch_and_add(&client->ref_count, 1);
> +
> + return client;
> +}
> +
> +static bool match_notifying(const void *a, const void *b)
> +{
> + const struct notify_client *client = a;
> +
> + return !!client->notify_id;
> +}
> +
> +static void update_notifying(struct characteristic *chrc)
> +{
> + if (!chrc->notifying)
> + return;
> +
> + if (queue_find(chrc->notify_clients, match_notifying, NULL))
> + return;
> +
> + chrc->notifying = false;
> +
> + g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
> + GATT_CHARACTERISTIC_IFACE,
> + "Notifying");
> +}
> +
> +static void notify_client_disconnect(DBusConnection *conn, void *user_data)
> +{
> + struct notify_client *client = user_data;
> + struct characteristic *chrc = client->chrc;
> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
> +
> + DBG("owner %s", client->owner);
> +
> + queue_remove(chrc->notify_clients, client);
> + bt_gatt_client_unregister_notify(gatt, client->notify_id);
> +
> + update_notifying(chrc);
> +
> + notify_client_unref(client);
> +}
> +
> +static struct notify_client *notify_client_create(struct characteristic *chrc,
> + const char *owner)
> +{
> + struct notify_client *client;
> +
> + client = new0(struct notify_client, 1);
> + if (!client)
> + return NULL;
> +
> + client->chrc = chrc;
> + client->owner = strdup(owner);
> + if (!client->owner) {
> + free(client);
> + return NULL;
> + }
> +
> + client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
> + owner, notify_client_disconnect,
> + client, NULL);
> + if (!client->watch) {
> + free(client->owner);
> + free(client);
> + return NULL;
> + }
> +
> + return notify_client_ref(client);
> +}
> +
> +static bool match_notify_sender(const void *a, const void *b)
> +{
> + const struct notify_client *client = a;
> + const char *sender = b;
> +
> + return strcmp(client->owner, sender) == 0;
> +}
> +
> +static void notify_cb(uint16_t value_handle, const uint8_t *value,
> + uint16_t length, void *user_data)
> +{
> + struct async_dbus_op *op = user_data;
> + struct notify_client *client = op->data;
> + struct characteristic *chrc = client->chrc;
> +
> + /*
> + * Even if the value didn't change, we want to send a PropertiesChanged
> + * signal so that we propagate the notification/indication to
> + * applications.
> + */
> + gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
> + write_characteristic_cb, chrc);
> +}
> +
> +static void register_notify_cb(unsigned int id, uint16_t att_ecode,
> + void *user_data)
> +{
> + struct async_dbus_op *op = user_data;
> + struct notify_client *client = op->data;
> + struct characteristic *chrc = client->chrc;
> + DBusMessage *reply;
> +
> + /* Make sure that an interim disconnect did not remove the client */
> + if (!queue_find(chrc->notify_clients, NULL, client)) {
> + bt_gatt_client_unregister_notify(chrc->service->client->gatt,
> + id);
> + notify_client_unref(client);
> +
> + reply = btd_error_failed(op->msg,
> + "Characteristic not available");
> + goto done;
> + }
> +
> + /*
> + * Drop the reference count that we added when registering the callback.
> + */
> + notify_client_unref(client);
> +
> + if (!id) {
> + queue_remove(chrc->notify_clients, client);
> + notify_client_free(client);
> +
> + reply = create_gatt_dbus_error(op->msg, att_ecode);
> +
> + goto done;
> + }
> +
> + client->notify_id = id;
> +
> + if (!chrc->notifying) {
> + chrc->notifying = true;
> + g_dbus_emit_property_changed(btd_get_dbus_connection(),
> + chrc->path, GATT_CHARACTERISTIC_IFACE,
> + "Notifying");
> + }
> +
> + reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
> +
> +done:
> + if (reply)
> + g_dbus_send_message(btd_get_dbus_connection(), reply);
> + else
> + error("Failed to construct D-Bus message reply");
> +
> + dbus_message_unref(op->msg);
> + op->msg = NULL;
> +}
> +
> static DBusMessage *characteristic_start_notify(DBusConnection *conn,
> DBusMessage *msg, void *user_data)
> {
> - /* TODO: Implement */
> - return btd_error_failed(msg, "Not implemented");
> + struct characteristic *chrc = user_data;
> + struct bt_gatt_client *gatt = chrc->service->client->gatt;
> + const char *sender = dbus_message_get_sender(msg);
> + struct async_dbus_op *op;
> + struct notify_client *client;
> +
> + if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
> + chrc->props & BT_GATT_CHRC_PROP_INDICATE))
> + return btd_error_not_supported(msg);
> +
> + /* Each client can only have one active notify session. */
> + client = queue_find(chrc->notify_clients, match_notify_sender, sender);
> + if (client)
> + return client->notify_id ?
> + btd_error_failed(msg, "Already notifying") :
> + btd_error_in_progress(msg);
> +
> + client = notify_client_create(chrc, sender);
> + if (!client)
> + return btd_error_failed(msg, "Failed allocate notify session");
> +
> + op = new0(struct async_dbus_op, 1);
> + if (!op) {
> + notify_client_unref(client);
> + return btd_error_failed(msg, "Failed to initialize request");
> + }
> +
> + /*
> + * Add to the ref count so that a disconnect during register won't free
> + * the client instance.
> + */
> + op->data = notify_client_ref(client);
> + op->msg = dbus_message_ref(msg);
> +
> + queue_push_tail(chrc->notify_clients, client);
> +
> + if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
> + register_notify_cb, notify_cb,
> + op, async_dbus_op_free))
> + return NULL;
> +
> + queue_remove(chrc->notify_clients, client);
> + async_dbus_op_free(op);
> +
> + /* Directly free the client */
> + notify_client_free(client);
> +
> + return btd_error_failed(msg, "Failed to register notify session");
> }
>
> static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
> @@ -992,6 +1221,13 @@ static struct characteristic *characteristic_create(
> return NULL;
> }
>
> + chrc->notify_clients = queue_new();
> + if (!chrc->notify_clients) {
> + queue_destroy(chrc->descs, NULL);
> + free(chrc);
> + return NULL;
> + }
> +
> chrc->service = service;
>
> gatt_db_attribute_get_char_data(attr, &chrc->handle,
> @@ -1027,6 +1263,7 @@ static void unregister_characteristic(void *data)
>
> DBG("Removing GATT characteristic: %s", chrc->path);
>
> + queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>
> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> 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



--
Luiz Augusto von Dentz

2015-01-06 02:03:36

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 8/8] doc/gatt-api.txt: Update error names

Updated possible error names that can be returned from
ReadValue/WriteValue methods to match those currently returned from the
experimental implementation.
---
doc/gatt-api.txt | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index f7125a2..bfeaf6d 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -75,9 +75,8 @@ Methods array{byte} ReadValue()

Possible Errors: org.bluez.Error.Failed
org.bluez.Error.InProgress
- org.bluez.Error.ReadNotPermitted
+ org.bluez.Error.NotPermitted
org.bluez.Error.NotAuthorized
- org.bluez.Error.NotPaired
org.bluez.Error.NotSupported

void WriteValue(array{byte} value)
@@ -87,10 +86,9 @@ Methods array{byte} ReadValue()

Possible Errors: org.bluez.Error.Failed
org.bluez.Error.InProgress
- org.bluez.Error.WriteNotPermitted
+ org.bluez.Error.NotPermitted
org.bluez.Error.InvalidValueLength
org.bluez.Error.NotAuthorized
- org.bluez.Error.NotPaired
org.bluez.Error.NotSupported

void StartNotify()
@@ -175,9 +173,8 @@ Methods array{byte} ReadValue()

Possible Errors: org.bluez.Error.Failed
org.bluez.Error.InProgress
- org.bluez.Error.ReadNotPermitted
+ org.bluez.Error.NotPermitted
org.bluez.Error.NotAuthorized
- org.bluez.Error.NotPaired
org.bluez.Error.NotSupported

void WriteValue(array{byte} value)
@@ -187,10 +184,9 @@ Methods array{byte} ReadValue()

Possible Errors: org.bluez.Error.Failed
org.bluez.Error.InProgress
- org.bluez.Error.WriteNotPermitted
+ org.bluez.Error.NotPermitted
org.bluez.Error.InvalidValueLength
org.bluez.Error.NotAuthorized
- org.bluez.Error.NotPaired
org.bluez.Error.NotSupported

Properties string UUID [read-only]
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:34

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 6/8] core: gatt: Reference count chrcs and descs.

Many of the GATT D-Bus API methods are asynchronous and their callbacks
depend on characteristic and descriptor instances to be alive when they
execute. This patch makes characteristics and descriptors reference
counted so that an interim disconnect or "Service Changed" won't
immediately free up those instances.
---
src/gatt-client.c | 179 ++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 125 insertions(+), 54 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 6897f30..c4f3582 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -83,6 +83,8 @@ struct characteristic {
bt_uuid_t uuid;
char *path;

+ int ref_count;
+
bool in_read;
bool in_write;

@@ -99,10 +101,49 @@ struct descriptor {
bt_uuid_t uuid;
char *path;

+ int ref_count;
+
bool in_read;
bool in_write;
};

+static struct characteristic *characteristic_ref(struct characteristic *chrc)
+{
+ __sync_fetch_and_add(&chrc->ref_count, 1);
+
+ return chrc;
+}
+
+static void characteristic_unref(void *data)
+{
+ struct characteristic *chrc = data;
+
+ if (__sync_sub_and_fetch(&chrc->ref_count, 1))
+ return;
+
+ queue_destroy(chrc->descs, NULL); /* List should be empty here */
+ g_free(chrc->path);
+ free(chrc);
+}
+
+static struct descriptor *descriptor_ref(struct descriptor *desc)
+{
+ __sync_fetch_and_add(&desc->ref_count, 1);
+
+ return desc;
+}
+
+static void descriptor_unref(void *data)
+{
+ struct descriptor *desc = data;
+
+ if (__sync_sub_and_fetch(&desc->ref_count, 1))
+ return;
+
+ g_free(desc->path);
+ free(desc);
+}
+
static bool uuid_cmp(const bt_uuid_t *uuid, uint16_t u16)
{
bt_uuid_t uuid16;
@@ -220,6 +261,7 @@ static bool parse_value_arg(DBusMessage *msg, uint8_t **value,
}

typedef bool (*async_dbus_op_complete_t)(void *data);
+typedef void (*async_dbus_op_destroy_t)(void *data);

struct async_dbus_op {
int ref_count;
@@ -227,12 +269,16 @@ struct async_dbus_op {
void *data;
uint16_t offset;
async_dbus_op_complete_t complete;
+ async_dbus_op_destroy_t destroy;
};

static void async_dbus_op_free(void *data)
{
struct async_dbus_op *op = data;

+ if (op->destroy)
+ op->destroy(op->data);
+
if (op->msg)
dbus_message_unref(op->msg);

@@ -389,7 +435,8 @@ static DBusMessage *descriptor_read_value(DBusConnection *conn,
return btd_error_failed(msg, "Failed to initialize request");

op->msg = dbus_message_ref(msg);
- op->data = desc;
+ op->data = descriptor_ref(desc);
+ op->destroy = descriptor_unref;

if (bt_gatt_client_read_value(gatt, desc->handle, desc_read_cb,
async_dbus_op_ref(op),
@@ -444,7 +491,8 @@ static bool start_long_write(DBusMessage *msg, uint16_t handle,
struct bt_gatt_client *gatt,
bool reliable, const uint8_t *value,
size_t value_len, void *data,
- async_dbus_op_complete_t complete)
+ async_dbus_op_complete_t complete,
+ async_dbus_op_destroy_t destroy)
{
struct async_dbus_op *op;

@@ -455,6 +503,7 @@ static bool start_long_write(DBusMessage *msg, uint16_t handle,
op->msg = dbus_message_ref(msg);
op->data = data;
op->complete = complete;
+ op->destroy = destroy;

if (bt_gatt_client_write_long_value(gatt, reliable, handle,
0, value, value_len,
@@ -471,7 +520,8 @@ static bool start_write_request(DBusMessage *msg, uint16_t handle,
struct bt_gatt_client *gatt,
const uint8_t *value, size_t value_len,
void *data,
- async_dbus_op_complete_t complete)
+ async_dbus_op_complete_t complete,
+ async_dbus_op_destroy_t destroy)
{
struct async_dbus_op *op;

@@ -482,6 +532,7 @@ static bool start_write_request(DBusMessage *msg, uint16_t handle,
op->msg = dbus_message_ref(msg);
op->data = data;
op->complete = complete;
+ op->destroy = destroy;

if (bt_gatt_client_write_value(gatt, handle, value, value_len,
write_cb, op,
@@ -536,11 +587,13 @@ static DBusMessage *descriptor_write_value(DBusConnection *conn,
if (value_len <= (unsigned) bt_gatt_client_get_mtu(gatt) - 3)
result = start_write_request(msg, desc->handle, gatt, value,
value_len, desc,
- desc_write_complete);
+ desc_write_complete,
+ descriptor_unref);
else
result = start_long_write(msg, desc->handle, gatt, false, value,
value_len, desc,
- desc_write_complete);
+ desc_write_complete,
+ descriptor_unref);

if (!result)
return btd_error_failed(msg, "Failed to initiate write");
@@ -571,14 +624,6 @@ static const GDBusMethodTable descriptor_methods[] = {
{ }
};

-static void descriptor_free(void *data)
-{
- struct descriptor *desc = data;
-
- g_free(desc->path);
- free(desc);
-}
-
static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,
struct characteristic *chrc)
{
@@ -600,10 +645,11 @@ static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,
GATT_DESCRIPTOR_IFACE,
descriptor_methods, NULL,
descriptor_properties,
- desc, descriptor_free)) {
+ descriptor_ref(desc),
+ descriptor_unref)) {
error("Unable to register GATT descriptor with handle 0x%04x",
desc->handle);
- descriptor_free(desc);
+ descriptor_unref(desc);

return NULL;
}
@@ -622,6 +668,8 @@ static void unregister_descriptor(void *data)

DBG("Removing GATT descriptor: %s", desc->path);

+ desc->chrc = NULL;
+
g_dbus_unregister_interface(btd_get_dbus_connection(), desc->path,
GATT_DESCRIPTOR_IFACE);
}
@@ -803,7 +851,8 @@ static DBusMessage *characteristic_read_value(DBusConnection *conn,
return btd_error_failed(msg, "Failed to initialize request");

op->msg = dbus_message_ref(msg);
- op->data = chrc;
+ op->data = characteristic_ref(chrc);
+ op->destroy = characteristic_unref;

if (bt_gatt_client_read_value(gatt, chrc->value_handle, chrc_read_cb,
async_dbus_op_ref(op),
@@ -877,13 +926,16 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,

if (value_len <= (unsigned) mtu - 3)
result = start_write_request(msg, chrc->value_handle,
- gatt, value,
- value_len, chrc,
- chrc_write_complete);
+ gatt, value, value_len,
+ characteristic_ref(chrc),
+ chrc_write_complete,
+ characteristic_unref);
else
result = start_long_write(msg, chrc->value_handle, gatt,
- false, value, value_len, chrc,
- chrc_write_complete);
+ false, value, value_len,
+ characteristic_ref(chrc),
+ chrc_write_complete,
+ characteristic_unref);

if (result)
goto done_async;
@@ -1016,12 +1068,33 @@ static bool match_notify_sender(const void *a, const void *b)
return strcmp(client->owner, sender) == 0;
}

+struct register_notify_op {
+ int ref_count;
+ struct notify_client *client;
+ struct characteristic *chrc;
+};
+
+static void register_notify_op_unref(void *data)
+{
+ struct register_notify_op *op = data;
+
+ DBG("Released register_notify_op");
+
+ if (__sync_sub_and_fetch(&op->ref_count, 1))
+ return;
+
+ notify_client_unref(op->client);
+ characteristic_unref(op->chrc);
+
+ free(op);
+}
+
static void notify_cb(uint16_t value_handle, const uint8_t *value,
uint16_t length, void *user_data)
{
struct async_dbus_op *op = user_data;
- struct notify_client *client = op->data;
- struct characteristic *chrc = client->chrc;
+ struct register_notify_op *notify_op = op->data;
+ struct characteristic *chrc = notify_op->chrc;

/*
* Even if the value didn't change, we want to send a PropertiesChanged
@@ -1036,26 +1109,24 @@ static void register_notify_cb(unsigned int id, uint16_t att_ecode,
void *user_data)
{
struct async_dbus_op *op = user_data;
- struct notify_client *client = op->data;
- struct characteristic *chrc = client->chrc;
+ struct register_notify_op *notify_op = op->data;
+ struct notify_client *client = notify_op->client;
+ struct characteristic *chrc = notify_op->chrc;
DBusMessage *reply;

- /* Make sure that an interim disconnect did not remove the client */
- if (!queue_find(chrc->notify_clients, NULL, client)) {
+ /*
+ * Make sure that an interim disconnect or "Service Changed" did not
+ * remove the client
+ */
+ if (!chrc->service || !queue_find(chrc->notify_clients, NULL, client)) {
bt_gatt_client_unregister_notify(chrc->service->client->gatt,
id);
- notify_client_unref(client);

reply = btd_error_failed(op->msg,
"Characteristic not available");
goto done;
}

- /*
- * Drop the reference count that we added when registering the callback.
- */
- notify_client_unref(client);
-
if (!id) {
queue_remove(chrc->notify_clients, client);
notify_client_free(client);
@@ -1094,6 +1165,7 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
const char *sender = dbus_message_get_sender(msg);
struct async_dbus_op *op;
struct notify_client *client;
+ struct register_notify_op *notify_op;

if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
chrc->props & BT_GATT_CHRC_PROP_INDICATE))
@@ -1110,20 +1182,28 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
if (!client)
return btd_error_failed(msg, "Failed allocate notify session");

+ notify_op = new0(struct register_notify_op, 1);
+ if (!notify_op) {
+ notify_client_unref(client);
+ return btd_error_failed(msg, "Failed to initialize request");
+ }
+
+ notify_op->ref_count = 1;
+ notify_op->client = client;
+ notify_op->chrc = characteristic_ref(chrc);
+
op = new0(struct async_dbus_op, 1);
if (!op) {
- notify_client_unref(client);
+ register_notify_op_unref(notify_op);
return btd_error_failed(msg, "Failed to initialize request");
}

- /*
- * Add to the ref count so that a disconnect during register won't free
- * the client instance.
- */
- op->data = notify_client_ref(client);
+ op->data = notify_op;
op->msg = dbus_message_ref(msg);
+ op->destroy = register_notify_op_unref;

- queue_push_tail(chrc->notify_clients, client);
+ /* The characteristic owns a reference to the client */
+ queue_push_tail(chrc->notify_clients, notify_client_ref(client));

if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
register_notify_cb, notify_cb,
@@ -1133,9 +1213,6 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
queue_remove(chrc->notify_clients, client);
async_dbus_op_free(op);

- /* Directly free the client */
- notify_client_free(client);
-
return btd_error_failed(msg, "Failed to register notify session");
}

@@ -1220,15 +1297,6 @@ static const GDBusMethodTable characteristic_methods[] = {
{ }
};

-static void characteristic_free(void *data)
-{
- struct characteristic *chrc = data;
-
- queue_destroy(chrc->descs, NULL); /* List should be empty here */
- g_free(chrc->path);
- free(chrc);
-}
-
static struct characteristic *characteristic_create(
struct gatt_db_attribute *attr,
struct service *service)
@@ -1269,10 +1337,11 @@ static struct characteristic *characteristic_create(
GATT_CHARACTERISTIC_IFACE,
characteristic_methods, NULL,
characteristic_properties,
- chrc, characteristic_free)) {
+ characteristic_ref(chrc),
+ characteristic_unref)) {
error("Unable to register GATT characteristic with handle "
"0x%04x", chrc->handle);
- characteristic_free(chrc);
+ characteristic_unref(chrc);

return NULL;
}
@@ -1291,6 +1360,8 @@ static void unregister_characteristic(void *data)
queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);

+ chrc->service = NULL;
+
g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
GATT_CHARACTERISTIC_IFACE);
}
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:35

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 7/8] core: gatt: Handle Service Changed.

This patch adds handling for Service Changed events. All exported
objects that match attributes within the changed handle range are
unregistered and new ones get exported based on the newly discovered
services.
---
src/device.c | 4 ++--
src/gatt-client.c | 35 +++++++++++++++++++++++++++++++----
2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/src/device.c b/src/device.c
index 0abed9b..346b68d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2703,7 +2703,7 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
if (l)
service_accept(l->data);

- store_services(device);
+ store_device_info(device);

btd_gatt_client_service_added(device->client_dbus, attr);
}
@@ -2775,7 +2775,7 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,

g_free(prim);

- store_services(device);
+ store_device_info(device);

btd_gatt_client_service_removed(device->client_dbus, attr);
}
diff --git a/src/gatt-client.c b/src/gatt-client.c
index c4f3582..cbafdef 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1521,6 +1521,11 @@ static void unregister_service(void *data)

static void notify_chrcs(struct service *service)
{
+
+ if (service->chrcs_ready ||
+ !queue_isempty(service->pending_ext_props))
+ return;
+
service->chrcs_ready = true;

g_dbus_emit_property_changed(btd_get_dbus_connection(), service->path,
@@ -1577,8 +1582,7 @@ static void read_ext_props_cb(bool success, uint8_t att_ecode,

queue_remove(service->pending_ext_props, chrc);

- if (queue_isempty(service->pending_ext_props))
- notify_chrcs(service);
+ notify_chrcs(service);
}

static void read_ext_props(void *data, void *user_data)
@@ -1760,13 +1764,36 @@ void btd_gatt_client_ready(struct btd_gatt_client *client)
void btd_gatt_client_service_added(struct btd_gatt_client *client,
struct gatt_db_attribute *attrib)
{
- /* TODO */
+ if (!client)
+ return;
+
+ export_service(attrib, client);
+}
+
+static bool match_service_handle(const void *a, const void *b)
+{
+ const struct service *service = a;
+ uint16_t start_handle = PTR_TO_UINT(b);
+
+ return service->start_handle == start_handle;
}

void btd_gatt_client_service_removed(struct btd_gatt_client *client,
struct gatt_db_attribute *attrib)
{
- /* TODO */
+ uint16_t start_handle, end_handle;
+
+ if (!client || !attrib)
+ return;
+
+ gatt_db_attribute_get_service_handles(attrib, &start_handle,
+ &end_handle);
+
+ DBG("GATT Services Removed - start: 0x%04x, end: 0x%04x", start_handle,
+ end_handle);
+ queue_remove_all(client->services, match_service_handle,
+ UINT_TO_PTR(start_handle),
+ unregister_service);
}

void btd_gatt_client_disconnected(struct btd_gatt_client *client)
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:32

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 4/8] shared/gatt-db: Add truncate argument to write

This patch adds the "truncate" argument to gatt_db_attribute_write. This
allows users to specify if the cached value should be truncated when the
write value length is shorter than what is stored in the database. This
allows client code to correctly store a remote attribute value when the
database is used as a client-role cache.
---
android/gatt.c | 17 +++++++++--------
src/gatt-client.c | 6 +++---
src/shared/gatt-db.c | 18 ++++++++++++++++--
src/shared/gatt-db.h | 1 +
src/shared/gatt-server.c | 6 ++++--
tools/btgatt-server.c | 8 ++++----
unit/test-gatt.c | 6 +++---
7 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 3a35bd6..7f765e0 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6349,7 +6349,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
return;

gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0], &dev->bdaddr,
- write_confirm, NULL);
+ false, write_confirm, NULL);
}

static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
@@ -6421,7 +6421,8 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
/* Signature OK, proceed with write */
bt_update_sign_counter(&dev->bdaddr, REMOTE_CSRK, r_sign_cnt);
gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
- &dev->bdaddr, write_confirm, NULL);
+ &dev->bdaddr, false,
+ write_confirm, NULL);
}
}

@@ -6481,8 +6482,8 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
}

if (!gatt_db_attribute_write(attrib, 0, value, vlen, cmd[0],
- &dev->bdaddr, attribute_write_cb,
- data)) {
+ &dev->bdaddr, false,
+ attribute_write_cb, data)) {
queue_remove(dev->pending_requests, data);
free(data);
return ATT_ECODE_UNLIKELY;
@@ -6541,8 +6542,8 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
data->length = vlen;

if (!gatt_db_attribute_write(attrib, offset, value, vlen, cmd[0],
- &dev->bdaddr, attribute_write_cb,
- data)) {
+ &dev->bdaddr, false,
+ attribute_write_cb, data)) {
queue_remove(dev->pending_requests, data);
g_free(data->value);
free(data);
@@ -6805,7 +6806,7 @@ static void register_gap_service(void)
value = cpu_to_le16(APPEARANCE_GENERIC_PHONE);
gatt_db_attribute_write(gap_srvc_data.appear, 0,
(void *) &value, sizeof(value),
- ATT_OP_WRITE_REQ, NULL,
+ ATT_OP_WRITE_REQ, NULL, false,
write_confirm, NULL);
}

@@ -6822,7 +6823,7 @@ static void register_gap_service(void)
value = PERIPHERAL_PRIVACY_DISABLE;
gatt_db_attribute_write(gap_srvc_data.priv, 0,
&value, sizeof(value),
- ATT_OP_WRITE_REQ, NULL,
+ ATT_OP_WRITE_REQ, NULL, false,
write_confirm, NULL);
}

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 1edbafc..79e1d26 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -349,7 +349,7 @@ static void desc_read_cb(bool success, uint8_t att_ecode,
return;
}

- gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL,
+ gatt_db_attribute_write(desc->attr, 0, value, length, 0, NULL, true,
write_descriptor_cb, desc);

/*
@@ -764,7 +764,7 @@ static void chrc_read_cb(bool success, uint8_t att_ecode, const uint8_t *value,
}

gatt_db_attribute_write(chrc->attr, 0, value, length, op->offset, NULL,
- write_characteristic_cb, chrc);
+ true, write_characteristic_cb, chrc);

/*
* If the value length is exactly MTU-1, then we may not have read the
@@ -1020,7 +1020,7 @@ static void notify_cb(uint16_t value_handle, const uint8_t *value,
* signal so that we propagate the notification/indication to
* applications.
*/
- gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
+ gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL, true,
write_characteristic_cb, chrc);
}

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 74c3e70..87e77d1 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1440,9 +1440,24 @@ static bool write_timeout(void *user_data)
return false;
}

+static bool should_resize_value(struct gatt_db_attribute *attrib, size_t len,
+ uint16_t offset,
+ bool truncate)
+{
+ unsigned diff;
+
+ if (!attrib->value || offset >= attrib->value_len)
+ return true;
+
+ diff = attrib->value_len - offset;
+
+ return truncate ? len != diff : len > diff;
+}
+
bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
const uint8_t *value, size_t len,
uint8_t opcode, bdaddr_t *bdaddr,
+ bool truncate,
gatt_db_attribute_write_t func,
void *user_data)
{
@@ -1471,8 +1486,7 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
}

/* For values stored in db allocate on demand */
- if (!attrib->value || offset >= attrib->value_len ||
- len > (unsigned) (attrib->value_len - offset)) {
+ if (should_resize_value(attrib, len, offset, truncate)) {
void *buf;

buf = realloc(attrib->value, len + offset);
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index f188944..2467852 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -193,6 +193,7 @@ typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
const uint8_t *value, size_t len,
uint8_t opcode, bdaddr_t *bdaddr,
+ bool truncate,
gatt_db_attribute_write_t func,
void *user_data);

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 00f36fd..2af7cf1 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -723,7 +723,8 @@ static void write_cb(uint8_t opcode, const void *pdu,
server->pending_write_op = op;

if (gatt_db_attribute_write(attr, 0, pdu + 2, length - 2, opcode,
- NULL, write_complete_cb, op))
+ NULL, false,
+ write_complete_cb, op))
return;

if (op)
@@ -1008,7 +1009,8 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
status = gatt_db_attribute_write(attr, next->offset,
next->value, next->length,
BT_ATT_OP_EXEC_WRITE_REQ, NULL,
- exec_write_complete_cb, server);
+ false, exec_write_complete_cb,
+ server);

prep_write_data_destroy(next);

diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
index f6ad8c3..393a071 100644
--- a/tools/btgatt-server.c
+++ b/tools/btgatt-server.c
@@ -446,8 +446,8 @@ static void populate_gap_service(struct server *server)
gatt_db_attribute_write(tmp, 0, (void *) &appearance,
sizeof(appearance),
BT_ATT_OP_WRITE_REQ,
- NULL, confirm_write,
- NULL);
+ NULL, true,
+ confirm_write, NULL);

gatt_db_service_set_active(service, true);
}
@@ -514,8 +514,8 @@ static void populate_hr_service(struct server *server)
NULL, NULL, server);
gatt_db_attribute_write(body, 0, (void *) &body_loc, sizeof(body_loc),
BT_ATT_OP_WRITE_REQ,
- NULL, confirm_write,
- NULL);
+ NULL, true,
+ confirm_write, NULL);

/* HR Control Point Characteristic */
bt_uuid16_create(&uuid, UUID_HEART_RATE_CTRL);
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 169fbd3..7a5081d 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -654,8 +654,8 @@ static struct gatt_db_attribute *add_char_with_value(struct gatt_db *db,

g_assert(attrib != NULL);

- gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, att_write_cb,
- NULL);
+ gatt_db_attribute_write(attrib, 0, value, len, 0x00, NULL, true,
+ att_write_cb, NULL);

return attrib;
}
@@ -670,7 +670,7 @@ add_desc_with_value(struct gatt_db_attribute *att, bt_uuid_t *uuid,
desc_att = gatt_db_service_add_descriptor(att, uuid, att_perms, NULL,
NULL, NULL);

- gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL,
+ gatt_db_attribute_write(desc_att, 0, value, len, 0x00, NULL, true,
att_write_cb, NULL);

return desc_att;
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:33

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 5/8] core: gatt: Issue long write for reliable-write

If the characteristic has the "reliable-write" extended property,
GattCharacteristic1.WriteValue will now start a reliable long-write
procedure.
---
src/gatt-client.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 79e1d26..6897f30 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -859,6 +859,14 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
* - If value is larger than MTU - 3: long-write
* * "write-without-response" property set -> write command.
*/
+ if ((chrc->ext_props & BT_GATT_CHRC_EXT_PROP_RELIABLE_WRITE) &&
+ start_long_write(msg, chrc->value_handle, gatt, true,
+ value, value_len,
+ characteristic_ref(chrc),
+ chrc_write_complete,
+ characteristic_unref))
+ goto done_async;
+
if (chrc->props & BT_GATT_CHRC_PROP_WRITE) {
uint16_t mtu;
bool result;
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:30

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 2/8] core: gatt: Implement GattCharacteristic1.StartNotify

This patch implements the StartNotify method of
org.bluez.GattCharacteristic1. Each call to StartNotify assigns a
session to the D-Bus sender which internally registers a unique
notify handler with the bt_gatt_client. Each received notification
or indication causes a PropertiesChanged signal to be emitted for the
"Value" property.

The notify handler gets automatically unregistered when sender
disconnects from D-Bus.
---
src/gatt-client.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 246 insertions(+), 9 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 8bbb03d..5c030ee 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -87,6 +87,9 @@ struct characteristic {
bool in_write;

struct queue *descs;
+
+ bool notifying;
+ struct queue *notify_clients;
};

struct descriptor {
@@ -230,7 +233,9 @@ static void async_dbus_op_free(void *data)
{
struct async_dbus_op *op = data;

- dbus_message_unref(op->msg);
+ if (op->msg)
+ dbus_message_unref(op->msg);
+
free(op);
}

@@ -674,12 +679,8 @@ static gboolean characteristic_value_exists(const GDBusPropertyTable *property,
static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
DBusMessageIter *iter, void *data)
{
- dbus_bool_t notifying = FALSE;
-
- /*
- * TODO: Return the correct value here once StartNotify and StopNotify
- * methods are implemented.
- */
+ struct characteristic *chrc = data;
+ dbus_bool_t notifying = chrc->notifying ? TRUE : FALSE;

dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &notifying);

@@ -895,11 +896,239 @@ done_async:
return NULL;
}

+struct notify_client {
+ struct characteristic *chrc;
+ int ref_count;
+ char *owner;
+ guint watch;
+ unsigned int notify_id;
+};
+
+static void notify_client_free(struct notify_client *client)
+{
+ DBG("owner %s", client->owner);
+
+ g_dbus_remove_watch(btd_get_dbus_connection(), client->watch);
+ free(client->owner);
+ free(client);
+}
+
+static void notify_client_unref(void *data)
+{
+ struct notify_client *client = data;
+
+ DBG("owner %s", client->owner);
+
+ if (__sync_sub_and_fetch(&client->ref_count, 1))
+ return;
+
+ notify_client_free(client);
+}
+
+static struct notify_client *notify_client_ref(struct notify_client *client)
+{
+ DBG("owner %s", client->owner);
+
+ __sync_fetch_and_add(&client->ref_count, 1);
+
+ return client;
+}
+
+static bool match_notifying(const void *a, const void *b)
+{
+ const struct notify_client *client = a;
+
+ return !!client->notify_id;
+}
+
+static void update_notifying(struct characteristic *chrc)
+{
+ if (!chrc->notifying)
+ return;
+
+ if (queue_find(chrc->notify_clients, match_notifying, NULL))
+ return;
+
+ chrc->notifying = false;
+
+ g_dbus_emit_property_changed(btd_get_dbus_connection(), chrc->path,
+ GATT_CHARACTERISTIC_IFACE,
+ "Notifying");
+}
+
+static void notify_client_disconnect(DBusConnection *conn, void *user_data)
+{
+ struct notify_client *client = user_data;
+ struct characteristic *chrc = client->chrc;
+ struct bt_gatt_client *gatt = chrc->service->client->gatt;
+
+ DBG("owner %s", client->owner);
+
+ queue_remove(chrc->notify_clients, client);
+ bt_gatt_client_unregister_notify(gatt, client->notify_id);
+
+ update_notifying(chrc);
+
+ notify_client_unref(client);
+}
+
+static struct notify_client *notify_client_create(struct characteristic *chrc,
+ const char *owner)
+{
+ struct notify_client *client;
+
+ client = new0(struct notify_client, 1);
+ if (!client)
+ return NULL;
+
+ client->chrc = chrc;
+ client->owner = strdup(owner);
+ if (!client->owner) {
+ free(client);
+ return NULL;
+ }
+
+ client->watch = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
+ owner, notify_client_disconnect,
+ client, NULL);
+ if (!client->watch) {
+ free(client->owner);
+ free(client);
+ return NULL;
+ }
+
+ return notify_client_ref(client);
+}
+
+static bool match_notify_sender(const void *a, const void *b)
+{
+ const struct notify_client *client = a;
+ const char *sender = b;
+
+ return strcmp(client->owner, sender) == 0;
+}
+
+static void notify_cb(uint16_t value_handle, const uint8_t *value,
+ uint16_t length, void *user_data)
+{
+ struct async_dbus_op *op = user_data;
+ struct notify_client *client = op->data;
+ struct characteristic *chrc = client->chrc;
+
+ /*
+ * Even if the value didn't change, we want to send a PropertiesChanged
+ * signal so that we propagate the notification/indication to
+ * applications.
+ */
+ gatt_db_attribute_write(chrc->attr, 0, value, length, 0, NULL,
+ write_characteristic_cb, chrc);
+}
+
+static void register_notify_cb(unsigned int id, uint16_t att_ecode,
+ void *user_data)
+{
+ struct async_dbus_op *op = user_data;
+ struct notify_client *client = op->data;
+ struct characteristic *chrc = client->chrc;
+ DBusMessage *reply;
+
+ /* Make sure that an interim disconnect did not remove the client */
+ if (!queue_find(chrc->notify_clients, NULL, client)) {
+ bt_gatt_client_unregister_notify(chrc->service->client->gatt,
+ id);
+ notify_client_unref(client);
+
+ reply = btd_error_failed(op->msg,
+ "Characteristic not available");
+ goto done;
+ }
+
+ /*
+ * Drop the reference count that we added when registering the callback.
+ */
+ notify_client_unref(client);
+
+ if (!id) {
+ queue_remove(chrc->notify_clients, client);
+ notify_client_free(client);
+
+ reply = create_gatt_dbus_error(op->msg, att_ecode);
+
+ goto done;
+ }
+
+ client->notify_id = id;
+
+ if (!chrc->notifying) {
+ chrc->notifying = true;
+ g_dbus_emit_property_changed(btd_get_dbus_connection(),
+ chrc->path, GATT_CHARACTERISTIC_IFACE,
+ "Notifying");
+ }
+
+ reply = g_dbus_create_reply(op->msg, DBUS_TYPE_INVALID);
+
+done:
+ if (reply)
+ g_dbus_send_message(btd_get_dbus_connection(), reply);
+ else
+ error("Failed to construct D-Bus message reply");
+
+ dbus_message_unref(op->msg);
+ op->msg = NULL;
+}
+
static DBusMessage *characteristic_start_notify(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
- /* TODO: Implement */
- return btd_error_failed(msg, "Not implemented");
+ struct characteristic *chrc = user_data;
+ struct bt_gatt_client *gatt = chrc->service->client->gatt;
+ const char *sender = dbus_message_get_sender(msg);
+ struct async_dbus_op *op;
+ struct notify_client *client;
+
+ if (!(chrc->props & BT_GATT_CHRC_PROP_NOTIFY ||
+ chrc->props & BT_GATT_CHRC_PROP_INDICATE))
+ return btd_error_not_supported(msg);
+
+ /* Each client can only have one active notify session. */
+ client = queue_find(chrc->notify_clients, match_notify_sender, sender);
+ if (client)
+ return client->notify_id ?
+ btd_error_failed(msg, "Already notifying") :
+ btd_error_in_progress(msg);
+
+ client = notify_client_create(chrc, sender);
+ if (!client)
+ return btd_error_failed(msg, "Failed allocate notify session");
+
+ op = new0(struct async_dbus_op, 1);
+ if (!op) {
+ notify_client_unref(client);
+ return btd_error_failed(msg, "Failed to initialize request");
+ }
+
+ /*
+ * Add to the ref count so that a disconnect during register won't free
+ * the client instance.
+ */
+ op->data = notify_client_ref(client);
+ op->msg = dbus_message_ref(msg);
+
+ queue_push_tail(chrc->notify_clients, client);
+
+ if (bt_gatt_client_register_notify(gatt, chrc->value_handle,
+ register_notify_cb, notify_cb,
+ op, async_dbus_op_free))
+ return NULL;
+
+ queue_remove(chrc->notify_clients, client);
+ async_dbus_op_free(op);
+
+ /* Directly free the client */
+ notify_client_free(client);
+
+ return btd_error_failed(msg, "Failed to register notify session");
}

static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
@@ -992,6 +1221,13 @@ static struct characteristic *characteristic_create(
return NULL;
}

+ chrc->notify_clients = queue_new();
+ if (!chrc->notify_clients) {
+ queue_destroy(chrc->descs, NULL);
+ free(chrc);
+ return NULL;
+ }
+
chrc->service = service;

gatt_db_attribute_get_char_data(attr, &chrc->handle,
@@ -1027,6 +1263,7 @@ static void unregister_characteristic(void *data)

DBG("Removing GATT characteristic: %s", chrc->path);

+ queue_remove_all(chrc->notify_clients, NULL, NULL, notify_client_unref);
queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);

g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:31

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 3/8] core: gatt: Implement GattCharacteristic1.StopNotify

This patch implements the StopNotify method of
org.bluez.GattCharacteristic1.
---
src/gatt-client.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 5c030ee..1edbafc 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1134,8 +1134,25 @@ static DBusMessage *characteristic_start_notify(DBusConnection *conn,
static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
- /* TODO: Implement */
- return btd_error_failed(msg, "Not implemented");
+ struct characteristic *chrc = user_data;
+ struct bt_gatt_client *gatt = chrc->service->client->gatt;
+ const char *sender = dbus_message_get_sender(msg);
+ struct notify_client *client;
+
+ if (!chrc->notifying)
+ return btd_error_failed(msg, "Not notifying");
+
+ client = queue_remove_if(chrc->notify_clients, match_notify_sender,
+ (void *) sender);
+ if (!client)
+ return btd_error_failed(msg, "No notify session started");
+
+ bt_gatt_client_unregister_notify(gatt, client->notify_id);
+ update_notifying(chrc);
+
+ notify_client_unref(client);
+
+ return dbus_message_new_method_return(msg);
}

static void append_desc_path(void *data, void *user_data)
--
2.2.0.rc0.207.ga3a616c


2015-01-06 02:03:29

by Arman Uguray

[permalink] [raw]
Subject: [PATCH BlueZ v1 1/8] core: gatt: Expose charac. extended properties.

This patch reads the extended properties for characteristics that have
the necessary descriptor. Updating the "Characteristics" property is
delayed for services until the extended properties have been read for
all of their characteristics that have them.
---
src/gatt-client.c | 155 +++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 119 insertions(+), 36 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 47faf86..8bbb03d 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -42,6 +42,10 @@
#include "gatt-client.h"
#include "dbus-common.h"

+#ifndef NELEM
+#define NELEM(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
#define GATT_SERVICE_IFACE "org.bluez.GattService1"
#define GATT_CHARACTERISTIC_IFACE "org.bluez.GattCharacteristic1"
#define GATT_DESCRIPTOR_IFACE "org.bluez.GattDescriptor1"
@@ -64,6 +68,8 @@ struct service {
char *path;
struct queue *chrcs;
bool chrcs_ready;
+ struct queue *pending_ext_props;
+ guint idle_id;
};

struct characteristic {
@@ -72,6 +78,8 @@ struct characteristic {
uint16_t handle;
uint16_t value_handle;
uint8_t props;
+ uint16_t ext_props;
+ uint16_t ext_props_handle;
bt_uuid_t uuid;
char *path;

@@ -92,6 +100,15 @@ struct descriptor {
bool in_write;
};

+static bool uuid_cmp(const bt_uuid_t *uuid, uint16_t u16)
+{
+ bt_uuid_t uuid16;
+
+ bt_uuid16_create(&uuid16, u16);
+
+ return bt_uuid_cmp(uuid, &uuid16) == 0;
+}
+
static gboolean descriptor_get_uuid(const GDBusPropertyTable *property,
DBusMessageIter *iter, void *data)
{
@@ -492,7 +509,6 @@ static DBusMessage *descriptor_write_value(DBusConnection *conn,
uint8_t *value = NULL;
size_t value_len = 0;
bool result;
- bt_uuid_t uuid;

if (desc->in_write)
return btd_error_in_progress(msg);
@@ -505,8 +521,7 @@ static DBusMessage *descriptor_write_value(DBusConnection *conn,
* descriptors. We achieve this through the StartNotify and StopNotify
* methods on GattCharacteristic1.
*/
- bt_uuid16_create(&uuid, GATT_CLIENT_CHARAC_CFG_UUID);
- if (bt_uuid_cmp(&desc->uuid, &uuid))
+ if (uuid_cmp(&desc->uuid, GATT_CLIENT_CHARAC_CFG_UUID))
return btd_error_not_permitted(msg, "Write not permitted");

/*
@@ -590,6 +605,9 @@ static struct descriptor *descriptor_create(struct gatt_db_attribute *attr,

DBG("Exported GATT characteristic descriptor: %s", desc->path);

+ if (uuid_cmp(&desc->uuid, GATT_CHARAC_EXT_PROPER_UUID))
+ chrc->ext_props_handle = desc->handle;
+
return desc;
}

@@ -668,10 +686,12 @@ static gboolean characteristic_get_notifying(const GDBusPropertyTable *property,
return TRUE;
}

-static struct {
+struct chrc_prop_data {
uint8_t prop;
char *str;
-} properties[] = {
+};
+
+struct chrc_prop_data chrc_props[] = {
/* Default Properties */
{ BT_GATT_CHRC_PROP_BROADCAST, "broadcast" },
{ BT_GATT_CHRC_PROP_READ, "read" },
@@ -680,7 +700,13 @@ static struct {
{ BT_GATT_CHRC_PROP_NOTIFY, "notify" },
{ BT_GATT_CHRC_PROP_INDICATE, "indicate" },
{ BT_GATT_CHRC_PROP_AUTH, "authenticated-signed-writes" },
- { },
+ { BT_GATT_CHRC_PROP_EXT_PROP, "extended-properties" }
+};
+
+struct chrc_prop_data chrc_ext_props[] = {
+ /* Extended Properties */
+ { BT_GATT_CHRC_EXT_PROP_RELIABLE_WRITE, "reliable-write" },
+ { BT_GATT_CHRC_EXT_PROP_WRITABLE_AUX, "writable-auxiliaries" }
};

static gboolean characteristic_get_flags(const GDBusPropertyTable *property,
@@ -688,20 +714,21 @@ static gboolean characteristic_get_flags(const GDBusPropertyTable *property,
{
struct characteristic *chrc = data;
DBusMessageIter array;
- int i;
+ unsigned i;

dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "s", &array);

- for (i = 0; properties[i].str; i++) {
- if (chrc->props & properties[i].prop)
+ for (i = 0; i < NELEM(chrc_props); i++) {
+ if (chrc->props & chrc_props[i].prop)
dbus_message_iter_append_basic(&array, DBUS_TYPE_STRING,
- &properties[i].str);
+ &chrc_props[i].str);
}

- /*
- * TODO: Handle extended properties if the descriptor is
- * present.
- */
+ for (i = 0; i < NELEM(chrc_ext_props); i++) {
+ if (chrc->ext_props & chrc_ext_props[i].prop)
+ dbus_message_iter_append_basic(&array, DBUS_TYPE_STRING,
+ &chrc_ext_props[i].str);
+ }

dbus_message_iter_close_container(iter, &array);

@@ -1085,6 +1112,7 @@ static void service_free(void *data)
struct service *service = data;

queue_destroy(service->chrcs, NULL); /* List should be empty here */
+ queue_destroy(service->pending_ext_props, NULL);
g_free(service->path);
free(service);
}
@@ -1106,6 +1134,13 @@ static struct service *service_create(struct gatt_db_attribute *attr,
return NULL;
}

+ service->pending_ext_props = queue_new();
+ if (!service->pending_ext_props) {
+ queue_destroy(service->chrcs, NULL);
+ free(service);
+ return NULL;
+ }
+
service->client = client;

gatt_db_attribute_get_service_data(attr, &service->start_handle,
@@ -1142,12 +1177,24 @@ static void unregister_service(void *data)

DBG("Removing GATT service: %s", service->path);

+ if (service->idle_id)
+ g_source_remove(service->idle_id);
+
queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);

g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
GATT_SERVICE_IFACE);
}

+static void notify_chrcs(struct service *service)
+{
+ service->chrcs_ready = true;
+
+ g_dbus_emit_property_changed(btd_get_dbus_connection(), service->path,
+ GATT_SERVICE_IFACE,
+ "Characteristics");
+}
+
struct export_data {
void *root;
bool failed;
@@ -1171,6 +1218,46 @@ static void export_desc(struct gatt_db_attribute *attr, void *user_data)
queue_push_tail(charac->descs, desc);
}

+static void read_ext_props_cb(bool success, uint8_t att_ecode,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
+{
+ struct characteristic *chrc = user_data;
+ struct service *service = chrc->service;
+
+ if (!success) {
+ error("Failed to obtain extended properties - error: 0x%02x",
+ att_ecode);
+ return;
+ }
+
+ if (!value || length != 2) {
+ error("Malformed extended properties value");
+ return;
+ }
+
+ chrc->ext_props = get_le16(value);
+ if (chrc->ext_props)
+ g_dbus_emit_property_changed(btd_get_dbus_connection(),
+ service->path,
+ GATT_SERVICE_IFACE, "Flags");
+
+ queue_remove(service->pending_ext_props, chrc);
+
+ if (queue_isempty(service->pending_ext_props))
+ notify_chrcs(service);
+}
+
+static void read_ext_props(void *data, void *user_data)
+{
+ struct characteristic *chrc = data;
+
+ bt_gatt_client_read_value(chrc->service->client->gatt,
+ chrc->ext_props_handle,
+ read_ext_props_cb,
+ chrc, NULL);
+}
+
static bool create_descriptors(struct gatt_db_attribute *attr,
struct characteristic *charac)
{
@@ -1204,6 +1291,9 @@ static void export_char(struct gatt_db_attribute *attr, void *user_data)

queue_push_tail(service->chrcs, charac);

+ if (charac->ext_props_handle)
+ queue_push_tail(service->pending_ext_props, charac);
+
return;

fail:
@@ -1220,28 +1310,20 @@ static bool create_characteristics(struct gatt_db_attribute *attr,

gatt_db_service_foreach_char(attr, export_char, &data);

- return !data.failed;
-}
-
-static void notify_chrcs(void *data, void *user_data)
-{
- struct service *service = data;
+ if (data.failed)
+ return false;

- service->chrcs_ready = true;
+ /* Obtain extended properties */
+ queue_foreach(service->pending_ext_props, read_ext_props, NULL);

- g_dbus_emit_property_changed(btd_get_dbus_connection(), service->path,
- GATT_SERVICE_IFACE,
- "Characteristics");
+ return true;
}

static gboolean set_chrcs_ready(gpointer user_data)
{
- struct btd_gatt_client *client = user_data;
-
- if (!client->gatt)
- return FALSE;
+ struct service *service = user_data;

- queue_foreach(client->services, notify_chrcs, NULL);
+ notify_chrcs(service);

return FALSE;
}
@@ -1265,6 +1347,14 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
}

queue_push_tail(client->services, service);
+
+ /*
+ * Asynchronously update the "Characteristics" property of the service.
+ * If there are any pending reads to obtain the value of the "Extended
+ * Properties" descriptor then wait until they are complete.
+ */
+ if (!service->chrcs_ready && queue_isempty(service->pending_ext_props))
+ service->idle_id = g_idle_add(set_chrcs_ready, service);
}

static void create_services(struct btd_gatt_client *client)
@@ -1272,13 +1362,6 @@ static void create_services(struct btd_gatt_client *client)
DBG("Exporting objects for GATT services: %s", client->devaddr);

gatt_db_foreach_service(client->db, NULL, export_service, client);
-
- /*
- * Asynchronously update the "Characteristics" property of each service.
- * We do this so that users have a way to know that all characteristics
- * of a service have been exported.
- */
- g_idle_add(set_chrcs_ready, client);
}

struct btd_gatt_client *btd_gatt_client_new(struct btd_device *device)
--
2.2.0.rc0.207.ga3a616c