Hi David,
On Tue, Apr 28, 2020 at 10:19 AM David Lechner <david@xxxxxxxxxxxxxx> wrote:
>
> On 4/28/20 12:03 PM, Luiz Augusto von Dentz wrote:
> > Hi David,
> >
> > On Tue, Apr 28, 2020 at 7:39 AM David Lechner <david@xxxxxxxxxxxxxx> wrote:
> >>
> >> On 4/27/20 11:28 AM, Luiz Augusto von Dentz wrote:
> >>> Hi David,
> >>>
> >>> On Mon, Apr 27, 2020 at 8:07 AM David Lechner <david@xxxxxxxxxxxxxx> wrote:
> >>>>
> >>>> This modifies the GATT client characteristic WriteValue D-Bus method to
> >>>> always check that the characteristic supports the requested type of
> >>>> write by checking for the corresponding property before attempting to
> >>>> write.
> >>>>
> >>>> Before this change, if the "type" option was used and it was set to
> >>>> "reliable" or "request", then BlueZ would attempt the write even if the
> >>>> characteristic does not support that write type. On the other hand, if
> >>>> "type" was set to "command" or was not specified, the method would
> >>>> return a org.bluez.Error.NotSupported error without attempting to write.
> >>>>
> >>>> After this change, the WriteValue method will consistently return
> >>>> org.bluez.Error.NotSupported if the corresponding property flag is not
> >>>> set for all types of writes.
> >>>> ---
> >>>>
> >>>> v2 changes:
> >>>> - remove extra check of test variable not NULL
> >>>> - fix one line over 80 chars
> >>>>
> >>>> src/gatt-client.c | 8 ++++----
> >>>> 1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/src/gatt-client.c b/src/gatt-client.c
> >>>> index a9bfc2802..f80742fbb 100644
> >>>> --- a/src/gatt-client.c
> >>>> +++ b/src/gatt-client.c
> >>>> @@ -1016,8 +1016,8 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
> >>>> * - If value is larger than MTU - 3: long-write
> >>>> * * "write-without-response" property set -> write command.
> >>>> */
> >>>> - if ((!type && (chrc->ext_props & BT_GATT_CHRC_EXT_PROP_RELIABLE_WRITE))
> >>>> - || (type && !strcasecmp(type, "reliable"))) {
> >>>> + if ((!type || !strcasecmp(type, "reliable")) && chrc->ext_props &
> >>>> + BT_GATT_CHRC_EXT_PROP_RELIABLE_WRITE) {
> >>>> supported = true;
> >>>> chrc->write_op = start_long_write(msg, chrc->value_handle, gatt,
> >>>> true, value, value_len, offset,
> >>>> @@ -1026,8 +1026,8 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> - if ((!type && chrc->props & BT_GATT_CHRC_PROP_WRITE) ||
> >>>> - (type && !strcasecmp(type, "request"))) {
> >>>> + if ((!type || !strcasecmp(type, "request")) && chrc->props &
> >>>> + BT_GATT_CHRC_PROP_WRITE) {
> >>>> uint16_t mtu;
> >>>>
> >>>> supported = true;
> >>>> --
> >>>> 2.17.1
> >>>
> >>> As far I can remember this is on purpose so the application can
> >>> overwrite the type if it needs to disabling the checks on the client
> >>> side, though the kernel can still return not supported, so if we want
> >>> to change this it will now not be possible to overwrite it anymore.
> >>>
> >>
> >> If this is the case, does it make sense to make the following change
> >> instead so that write without response can also be forced?
> >>
> >> diff --git a/src/gatt-client.c b/src/gatt-client.c
> >> index f80742fbb..3153f571f 100644
> >> --- a/src/gatt-client.c
> >> +++ b/src/gatt-client.c
> >> @@ -1050,8 +1050,8 @@ static DBusMessage *characteristic_write_value(DBusConnection *conn,
> >> return NULL;
> >> }
> >>
> >> - if ((type && strcasecmp(type, "command")) || offset ||
> >> - !(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
> >> + if ((type && strcasecmp(type, "command")) || offset || (!type &&
> >> + !(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP)))
> >> goto fail;
> >>
> >> supported = true;
> >
> > Yes, that is probably one of the use cases to use the type to force
> > sending a command if the client don't care about the response but the
> > server don't mark write without response as supported, is that what
> > you are after? That sounds like a bug since it appears the intention
> > was to allow commands all along.
> >
>
> I just noticed the inconsistency here while trying to implement
> writeValueWithResponse() and writeValueWithoutResponse() in the Web
> Bluetooth API for the Chromium browser and thought that it should
> probably all be one way or the other.
>
> FWIW, in the particular case I am working with, the device has both
> the WRITE and WRITE_WITHOUT_RESP properties set so I am using "type"
> to force writing without response since write with response would
> be selected otherwise.
>
> I will send a proper patch to fix the write without response case.
Great, that indeed shall be supported since both are allowed
bluetoothd will prefer Write since that gives a response, there is
also some means to mark the D-Bus message don't expect a reply as well
which might be another to solve this if you don't want to use type.
--
Luiz Augusto von Dentz
