Hi Arman,
On Tue, Jan 6, 2015 at 8:09 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
> Hi Luiz,
>
>> On Tue, Jan 6, 2015 at 1:44 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>> Hi Luiz,
>>
>>> On Tue, Jan 6, 2015 at 12:45 PM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>>> Hi Arman,
>>>
>>> On Tue, Jan 6, 2015 at 4:58 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>>>> Hi Luiz,
>>>>
>>>>> On Tue, Jan 6, 2015 at 5:09 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>>>>> Hi Arman,
>>>>>
>>>>> On Tue, Jan 6, 2015 at 12:03 AM, Arman Uguray <armansito@xxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
>>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
