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.
--
Luiz Augusto von Dentz
