Return-Path: MIME-Version: 1.0 Reply-To: yunhanw@nestlabs.com In-Reply-To: References: <20170920074417.30435-1-luiz.dentz@gmail.com> <20170920074417.30435-9-luiz.dentz@gmail.com> From: Yunhan Wang Date: Tue, 3 Oct 2017 11:48:45 -0700 Message-ID: Subject: Re: [PATCH v2 08/10] gatt: Implement AcquireNotify for server To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi, Luiz On Tue, Oct 3, 2017 at 12:36 AM, Luiz Augusto von Dentz wrote: > Hi Yunhan, > > On Tue, Oct 3, 2017 at 9:13 AM, Yunhan Wang wrote: >> Hi, Luiz >> >> On Fri, Sep 29, 2017 at 12:03 AM, Luiz Augusto von Dentz >> wrote: >>> Hi Yunhan, >>> >>> On Fri, Sep 29, 2017 at 7:47 AM, Yunhan Wang wro= te: >>>> Hi, Luiz >>>> >>>> May I have several questions about this feature? >>>> >>>> On Wed, Sep 20, 2017 at 12:44 AM, Luiz Augusto von Dentz >>>> wrote: >>>>> From: Luiz Augusto von Dentz >>>>> >>>>> This enables IO via file descriptors using AcquireWrite if server >>>>> implements it. >>>>> --- >>>>> src/gatt-database.c | 116 ++++++++++++++++++++++++++++++++++++++++++= +++++++--- >>>>> 1 file changed, 111 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/src/gatt-database.c b/src/gatt-database.c >>>>> index 239fe5384..4932568dd 100644 >>>>> --- a/src/gatt-database.c >>>>> +++ b/src/gatt-database.c >>>>> @@ -24,6 +24,7 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include "lib/bluetooth.h" >>>>> #include "lib/sdp.h" >>>>> @@ -118,7 +119,9 @@ struct external_chrc { >>>>> uint8_t props; >>>>> uint8_t ext_props; >>>>> uint32_t perm; >>>>> + uint16_t mtu; >>>>> struct io *write_io; >>>>> + struct io *notify_io; >>>>> struct gatt_db_attribute *attrib; >>>>> struct gatt_db_attribute *ccc; >>>>> struct queue *pending_reads; >>>>> @@ -152,7 +155,8 @@ struct device_state { >>>>> struct queue *ccc_states; >>>>> }; >>>>> >>>>> -typedef uint8_t (*btd_gatt_database_ccc_write_t) (uint16_t value, >>>>> +typedef uint8_t (*btd_gatt_database_ccc_write_t) (struct bt_att *att= , >>>>> + uint16_t valu= e, >>>>> void *user_da= ta); >>>>> typedef void (*btd_gatt_database_destroy_t) (void *data); >>>>> >>>>> @@ -328,6 +332,7 @@ static void chrc_free(void *data) >>>>> struct external_chrc *chrc =3D data; >>>>> >>>>> io_destroy(chrc->write_io); >>>>> + io_destroy(chrc->notify_io); >>>>> >>>>> queue_destroy(chrc->pending_reads, cancel_pending_read); >>>>> queue_destroy(chrc->pending_writes, cancel_pending_write); >>>>> @@ -792,7 +797,8 @@ static void gatt_ccc_write_cb(struct gatt_db_attr= ibute *attrib, >>>>> goto done; >>>>> >>>>> if (ccc_cb->callback) >>>>> - ecode =3D ccc_cb->callback(get_le16(value), ccc_cb->u= ser_data); >>>>> + ecode =3D ccc_cb->callback(att, get_le16(value), >>>>> + ccc_cb->user_data); >>>>> >>>>> if (!ecode) { >>>>> ccc->value[0] =3D value[0]; >>>>> @@ -1801,12 +1807,34 @@ static bool pipe_hup(struct io *io, void *use= r_data) >>>>> >>>>> if (io =3D=3D chrc->write_io) >>>>> chrc->write_io =3D NULL; >>>>> + else >>>>> + chrc->notify_io =3D NULL; >>>>> >>>>> io_destroy(io); >>>>> >>>>> return false; >>>>> } >>>>> >>>>> +static bool pipe_io_read(struct io *io, void *user_data) >>>>> +{ >>>>> + struct external_chrc *chrc =3D user_data; >>>>> + uint8_t buf[512]; >>>>> + int fd =3D io_get_fd(io); >>>>> + ssize_t bytes_read; >>>>> + >>>>> + bytes_read =3D read(fd, buf, sizeof(buf)); >>>>> + if (bytes_read < 0) >>>>> + return false; >>>>> + >>>>> + send_notification_to_devices(chrc->service->app->database, >>>>> + gatt_db_attribute_get_handle(chrc->at= trib), >>>>> + buf, bytes_read, >>>>> + gatt_db_attribute_get_handle(chrc->cc= c), >>>>> + false, NULL); >>>>> + >>>>> + return true; >>>>> +} >>>>> + >>>>> static struct io *pipe_io_new(int fd, void *user_data) >>>>> { >>>>> struct io *io; >>>>> @@ -1815,6 +1843,8 @@ static struct io *pipe_io_new(int fd, void *use= r_data) >>>>> >>>>> io_set_close_on_destroy(io, true); >>>>> >>>>> + io_set_read_handler(io, pipe_io_read, user_data, NULL); >>>>> + >>>>> io_set_disconnect_handler(io, pipe_hup, user_data, NULL); >>>>> >>>>> return io; >>>>> @@ -1915,9 +1945,64 @@ static struct pending_op *acquire_write(struct= external_chrc *chrc, >>>>> return NULL; >>>>> } >>>>> >>>>> -static uint8_t ccc_write_cb(uint16_t value, void *user_data) >>>>> +static void acquire_notify_reply(DBusMessage *message, void *user_da= ta) >>>>> { >>>>> struct external_chrc *chrc =3D user_data; >>>>> + DBusError err; >>>>> + int fd; >>>>> + uint16_t mtu; >>>>> + >>>>> + dbus_error_init(&err); >>>>> + >>>>> + if (dbus_set_error_from_message(&err, message) =3D=3D TRUE) { >>>>> + error("Failed to acquire notify: %s\n", err.name); >>>>> + dbus_error_free(&err); >>>>> + goto retry; >>>>> + } >>>>> + >>>>> + if ((dbus_message_get_args(message, NULL, DBUS_TYPE_UNIX_FD, = &fd, >>>>> + DBUS_TYPE_UINT16, &mtu, >>>>> + DBUS_TYPE_INVALID) =3D=3D fal= se)) { >>>>> + error("Invalid AcquirNotify response\n"); >>>>> + goto retry; >>>>> + } >>>>> + >>>>> + DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu); >>>>> + >>>>> + chrc->notify_io =3D pipe_io_new(fd, chrc); >>>> >>>> Why do we need to use pipe and notify_io when implementing this >>>> feature? I think fd and mtu has been pass using dbus message. Could >>>> you clarify more details? >>> >>> notify_io is the receiving end of the notification, the application >>> will write its notifications using the fd which is captured by >>> checking if the fd is readable. >>> >>>> >>>>> + >>>>> + __sync_fetch_and_add(&chrc->ntfy_cnt, 1); >>>>> + >>>>> + return; >>>>> + >>>>> +retry: >>>>> + g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL, NU= LL, >>>>> + NULL, NULL); >>>>> + >>>>> + __sync_fetch_and_add(&chrc->ntfy_cnt, 1); >>>>> +} >>>>> + >>>>> +static void acquire_notify_setup(DBusMessageIter *iter, void *user_d= ata) >>>>> +{ >>>>> + DBusMessageIter dict; >>>>> + struct external_chrc *chrc =3D user_data; >>>>> + >>>>> + dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, >>>>> + DBUS_DICT_ENTRY_BEGIN_CHAR_AS= _STRING >>>>> + DBUS_TYPE_STRING_AS_STRING >>>>> + DBUS_TYPE_VARIANT_AS_STRING >>>>> + DBUS_DICT_ENTRY_END_CHAR_AS_S= TRING, >>>>> + &dict); >>>>> + >>>>> + dict_append_entry(&dict, "MTU", DBUS_TYPE_UINT16, &chrc->mtu)= ; >>>>> + >>>>> + dbus_message_iter_close_container(iter, &dict); >>>>> +} >>>>> + >>>>> +static uint8_t ccc_write_cb(struct bt_att *att, uint16_t value, void= *user_data) >>>>> +{ >>>>> + struct external_chrc *chrc =3D user_data; >>>>> + DBusMessageIter iter; >>>>> >>>>> DBG("External CCC write received with value: 0x%04x", value); >>>>> >>>>> @@ -1929,6 +2014,12 @@ static uint8_t ccc_write_cb(uint16_t value, vo= id *user_data) >>>>> if (__sync_sub_and_fetch(&chrc->ntfy_cnt, 1)) >>>>> return 0; >>>>> >>>>> + if (chrc->notify_io) { >>>>> + io_destroy(chrc->notify_io); >>>>> + chrc->notify_io =3D NULL; >>>>> + return 0; >>>>> + } >>>>> + >>>>> /* >>>>> * Send request to stop notifying. This is best-effor= t >>>>> * operation, so simply ignore the return the value. >>>>> @@ -1949,12 +2040,27 @@ static uint8_t ccc_write_cb(uint16_t value, v= oid *user_data) >>>>> (value =3D=3D 2 && !(chrc->props & BT_GATT_CHRC_PROP_= INDICATE))) >>>>> return BT_ERROR_CCC_IMPROPERLY_CONFIGURED; >>>>> >>>>> + if (chrc->notify_io) { >>>>> + __sync_fetch_and_add(&chrc->ntfy_cnt, 1); >>>>> + return 0; >>>>> + } >>>> What is the purpose to call __sync_fetch_and_add when there is >>>> charc->notify_io? What is the scenario here=EF=BC=9F >>> >>> This is using the number of subscribed client as a refcount so when >>> there is no client left we can drop the notify_io as well. >>> >>>> >>>>> + >>>>> + chrc->mtu =3D bt_att_get_mtu(att); >>>>> + >>>>> + /* Make use of AcquireNotify if supported */ >>>>> + if (g_dbus_proxy_get_property(chrc->proxy, "NotifyAcquired", = &iter)) { >>>>> + if (g_dbus_proxy_method_call(chrc->proxy, "AcquireNot= ify", >>>>> + acquire_notify_setup, >>>>> + acquire_notify_reply, >>>>> + chrc, NULL)) >>>>> + return 0; >>>>> + } >>>> When ble central subscribe this characteristic, if NotifyAcquired is >>>> set in ble peripheral application, then it return, but how can we >>>> subscribe this characteristic? May we call "AcquireNotify" when ble >>>> central subscribe this characteristic or enable notification? >>> >>> If NotifyAcquired is supported it will call AcquireNotify, or are you >>> saying you tried that? The bluetoothctl has code to utilize this >>> feature, you just have to register a characteristic with notify flag. >>> >> In previous email, I have confirmed AcquireWrite is working, now I >> wanna send indication via fd using AcquireNotify in application, I >> have removed "check" for "notify" in >> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/gatt.c#n1= 387 > > Does this means you don't have the notify flag at all, or you are > using indications? I guess we should enable this for indications as > well, though it won't have any means to confirm. > I am using indicate flag that expects the confirmation. In regular indication operation, the indication confirmation would be received in server side per the code, https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database.c= #n935. I just send a follow-up patch to add proxy in send_notification_to_devices call that fix indication confirmation using pipe_io_read. It is working now. Thank you >> Then I try to use pipe_io_send to send indication in application, but >> it seems data cannot be sent out from peripheral to central, any idea? >> it seems there is no reference code to send data in bluetoothctl. >> >> static int pipe_io_send(struct io *io, const void *data, size_t len) >> { >> struct iovec iov; >> >> iov.iov_base =3D (void *) data; >> iov.iov_len =3D len; >> >> return io_send(io, &iov, 1); >> } > > Isn't this because of the following check: > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database= .c#n906 > > Im afraid you have quite a different use case than we were targeting, > it seems you do you GATT for acking delivery instead of doing it > directly over the protocol which is what most solutions I know would > do when emulating serial over GATT. > >>>>> + >>>>> /* >>>>> * Always call StartNotify for an incoming enable and ignore = the return >>>>> * value for now. >>>>> */ >>>>> - if (g_dbus_proxy_method_call(chrc->proxy, >>>>> - "StartNotify", NULL, = NULL, >>>>> + if (g_dbus_proxy_method_call(chrc->proxy, "StartNotify", NULL= , NULL, >>>>> NULL, NULL) =3D=3D FA= LSE) >>>>> return BT_ATT_ERROR_UNLIKELY; >>>>> >>>>> -- >>>>> 2.13.5 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluet= ooth" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >>> -- >>> Luiz Augusto von Dentz >> >> thansk >> best wishes >> yunhan > > > > -- > Luiz Augusto von Dentz Thanks Best wishes Yunhan