Hi Lukasz,
On Fri, Nov 20, 2015 at 10:15 AM, Łukasz Rymanowski
<lukasz.rymanowski@xxxxxxxxxxx> wrote:
> Hi Luiz,
>
> On Thu, Nov 19, 2015 at 12:01 AM, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
>>
>> Hi Lukasz,
>>
>> On Tue, Nov 17, 2015 at 11:32 PM, Łukasz Rymanowski
>> <lukasz.rymanowski@xxxxxxxxxxx> wrote:
>> > Hi Luiz, Marcin,
>> >
>> > On Tue, Nov 17, 2015 at 1:49 PM, Luiz Augusto von Dentz
>> > <luiz.dentz@xxxxxxxxx> wrote:
>> >> Hi Marcin,
>> >>
>> >> On Tue, Nov 17, 2015 at 2:12 PM, Marcin Kraglak
>> >> <marcin.kraglak@xxxxxxxxx> wrote:
>> >>> Hi Lukasz,
>> >>>
>> >>> On 16 November 2015 at 22:08, Łukasz Rymanowski
>> >>> <lukasz.rymanowski@xxxxxxxxxxx> wrote:
>> >>>>
>> >>>> With current implementation application layer has no idea when
>> >>>> long write session ends as it gets write callback with execute
>> >>>> write for each prepare write data.
>> >>>> With this patch, after data are completly received by gatt-server it
>> >>>> starts to call write callback in proper order with opcode
>> >>>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic
>> >>>> value
>> >>>> has been received and data can be marked as valid.
>> >>>> ---
>> >>>> src/shared/gatt-server.c | 78
>> >>>> ++++++++++++++++++++++++++++++++++++++++++++----
>> >>>> 1 file changed, 73 insertions(+), 5 deletions(-)
>> >>>>
>> >>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> >>>> index 329f53f..931d619 100644
>> >>>> --- a/src/shared/gatt-server.c
>> >>>> +++ b/src/shared/gatt-server.c
>> >>>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>> >>>> struct queue *prep_queue;
>> >>>> unsigned int max_prep_queue_len;
>> >>>>
>> >>>> + struct queue *exec_queue;
>> >>>> +
>> >>>> struct async_read_op *pending_read_op;
>> >>>> struct async_write_op *pending_write_op;
>> >>>>
>> >>>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct
>> >>>> bt_gatt_server *server)
>> >>>> server->pending_write_op->server = NULL;
>> >>>>
>> >>>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>> >>>> + queue_destroy(server->exec_queue, NULL);
>> >>>>
>> >>>> gatt_db_unref(server->db);
>> >>>> bt_att_unref(server->att);
>> >>>> @@ -1155,10 +1158,61 @@ error:
>> >>>>
>> >>>> }
>> >>>>
>> >>>> +static void exec_next_write(struct bt_gatt_server *server);
>> >>>> +
>> >>>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr,
>> >>>> int err,
>> >>>> + void
>> >>>> *user_data)
>> >>>> +{
>> >>>> + struct bt_gatt_server *server = user_data;
>> >>>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>> >>>> +
>> >>>> + if (err) {
>> >>>> + queue_remove_all(server->exec_queue, NULL, NULL,
>> >>>> NULL);
>> >>>> + bt_att_send_error_rsp(server->att,
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> +
>> >>>> handle, err);
>> >>>> + }
>> >>>> +
>> >>>> + exec_next_write(server);
>> >>>> +}
>> >>>> +
>> >>>> +static void exec_next_write(struct bt_gatt_server *server)
>> >>>> +{
>> >>>> + uint16_t *handle;
>> >>>> + struct gatt_db_attribute *attr;
>> >>>> + bool status;
>> >>>> + int err;
>> >>>> +
>> >>>> + handle = queue_pop_head(server->exec_queue);
>> >>>> + if (!handle) {
>> >>>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP,
>> >>>> NULL, 0,
>> >>>> + NULL, NULL,
>> >>>> NULL);
>> >>>> + }
>> >>>> +
>> >>>> + attr = gatt_db_get_attribute(server->db,
>> >>>> PTR_TO_UINT(handle));
>> >>>> + if (!attr) {
>> >>>> + err = BT_ATT_ERROR_UNLIKELY;
>> >>>> + goto error;
>> >>>> + }
>> >>>> +
>> >>>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>> >>>> +
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> + server->att,
>> >>>> +
>> >>>> exec_write_complete_cb, server);
>> >>>> + if (status)
>> >>>> + return;
>> >>>> +
>> >>>> + err = BT_ATT_ERROR_UNLIKELY;
>> >>>> +
>> >>>> +error:
>> >>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> >>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> + PTR_TO_UINT(handle),
>> >>>> err);
>> >>>> +}
>> >>>> +
>> >>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> >>>> uint16_t ehandle, int
>> >>>> err);
>> >>>>
>> >>>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr,
>> >>>> int err,
>> >>>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr,
>> >>>> int err,
>> >>>> void
>> >>>> *user_data)
>> >>>> {
>> >>>> struct bt_gatt_server *server = user_data;
>> >>>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct
>> >>>> gatt_db_attribute *attr, int err,
>> >>>> exec_next_prep_write(server, handle, err);
>> >>>> }
>> >>>>
>> >>>> +static bool match_attribute_handle(const void *data, const void
>> >>>> *match_data)
>> >>>> +{
>> >>>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>> >>>> +}
>> >>>> +
>> >>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> >>>> uint16_t ehandle, int
>> >>>> err)
>> >>>> {
>> >>>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct
>> >>>> bt_gatt_server *server,
>> >>>> if (err)
>> >>> Shouldn't you remove all elements of server->exec_queue here? Looking
>> >>> at the next patch, all attributes in exec_queue shall be notified that
>> >>> exec write is canceled too.
>> >>> And just open question: with introducing two types of write requests
>> >>> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
>> >>> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
>> >>> prepare writes immediatelly to specific service implementation instead
>> >>> of storing all values in prepare writes in server.
>> >>
>> >> Even if we have some flush logic trigger by execute we cannot really
>> >> dispatch writes immediately to the services because it can fail with
>> >> some errors that are not allowed by the testing spec, yes there is a
>> >> specific test checking the e.g. invalid offset is only receive as a
>> >> response to execute. It looks like this is because the value could be
>> >> changed in between prepare and execute, which IMO is something
>> >> horribly broken with prepare + execute since it is intended for
>> >> reliable writes but it can produce all sort of errors which is
>> >> impossible to guess without reading the value written after it has
>> >> been executed in case anything has change in between the commands.
>> >>
>> >
>> > I see Marcins point and agree. According to spec if handle is valid,
>> > permisions are ok and there
>> > is enought place in the queue, Prepare Write Response shall be sent
>> > back. No matter if application
>> > likes data or not. Error on value validation we will get during
>> > execute writes. So in teory we could indeed
>> > avoid prep_queue and send data directly to application when handle is
>> > ok and permisions are fine.
>>
>> I thought the data reaching the application would mean it shall now
>> apply, but perhaps Android interface does not care what the
>> application will do with it, anyway this is something I would not like
>> to have in D-Bus since we don't expose prepare + execute write there.
>>
>>
>> > Actually this is how we do it now in Android part.
>> > But on the other side, that is not big deal, so if you Luiz saw any
>> > issues with that, we can stay with server
>> > handling queuing prep writes.
>>
>> Im afraid Android got this wrong, if do dispatch to the application it
>> means they have to be queued there so you no longer can guarantee the
>> sequence of execute e.g.
>>
>> prep handle1 -> app1
>> prep handle2 -> app2
>> prep handle2 -> app2
>> prep handle1 -> app1
>>
>> exec -> app1, app2, app1 ? (opps, it already execute the first and the
>> second write)
>>
>
> Indeed that is the issue.
>
> Now, lets take other example.
>
> prep handle1 -> app1 (offset 0)
> prep handle2 -> app1 (offset 20 - means that first prepare was only part of
> the value)
>
> Exec->app1 ?( opps, what to do with only part of the value)
If you are talking about D-Bus experimental interface, yes that does
not support offset for WriteValue yet but I think we might need to
introduce for long writes, actually Web Bluetooth also don't care
about offset:
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothgattcharacteristic-writevalue
The problem was that for client it won't be that useful since you
almost always start from offset 0 which would probably mean we could
just aggregate the writes and send on idle, but if we want we can add
the offset as well.
> To summarize, current solution in BlueZ works only for reliable write,
> Android solution works for both but have
> the one issue you mentioned (when there is more then one prepare for same
> characteristic)
Well Android doesn't seems to do anything except pass the request up
to the application, then it is up to the application to queue and
execute, perhaps you even need an app that behave well enough to pass
qualification.
> The whole problem is that gatt server needs to figure out if client is doing
> reliable or long write and actually
> since we queue all the prep writes we should be able to do that.
> In case of reliable session we could keep current behavior, and in case of
> long session we probably
> should aggregate complete value and do one exec write to application, does
> it makes sense?
It could in theory detect if the previous item on the queue is a
contiguous area then just append but Im not sure it is worth it since
we could do this in gatt-database with an idle timeout.
> For Android I will just skip prep and execute write but just use regular
> write request. Basically we
> will make use of logic already in gatt-server and do not bother app with
> prep/execute. That should work.
Yep, and if it already has an offset then it could handle any sequence.
>
>
>> It is no doubt a completely bogus scenario, but still I think it is
>> better be safe than sorry if some profile turns out to depend in a
>> strict sequence.
>>
>> >>>> next = queue_pop_head(server->prep_queue);
>> >>>> if (!next) {
>> >>>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP,
>> >>>> NULL, 0,
>> >>>> - NULL, NULL,
>> >>>> NULL);
>> >>>> + exec_next_write(server);
>> >>>> return;
>> >>>> }
>> >>>>
>> >>>> @@ -1192,9 +1258,9 @@ 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,
>> >>>> +
>> >>>> BT_ATT_OP_PREP_WRITE_REQ,
>> >>>> server->att,
>> >>>> -
>> >>>> exec_write_complete_cb, server);
>> >>>> +
>> >>>> prep_write_complete_cb, server);
>> >>>>
>> >>>> prep_write_data_destroy(next);
>> >>>>
>> >>>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server
>> >>>> *bt_gatt_server_new(struct gatt_db *db,
>> >>>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>> >>>> server->prep_queue = queue_new();
>> >>>>
>> >>>> + server->exec_queue = queue_new();
>> >>>> +
>> >>>> if (!gatt_server_register_att_handlers(server)) {
>> >>>> bt_gatt_server_free(server);
>> >>>> return NULL;
>> >>>> --
>> >>>> 2.5.0
>> >>>>
>> >>>> --
>> >>>> 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
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> BR
>> >>> Marcin Kraglak
>> >>> --
>> >>> 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
>> >
>> >
>> >
>> > --
>> > BR / Pozdrawiam
>> > Łukasz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
>
> --
> BR / Pozdrawiam
> Łukasz
--
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
