[RFC]Mesh: meshctl configuration output fails in gatt.c:pipe_write

[RFC]Mesh: meshctl configuration output fails in gatt.c:pipe_write

[Steve Brown]

If the first command output in a new connection exceeds 20 bytes,
mesh_gatt_write sets the SAR to FIRST as the write_mtu is initially 0
and the default is GATT_MTU-3 (20).

When pipe_write gets called, a new larger write_mtu has been set, but
the SAR is still set to FIRST. It's assumed that data->gatt_len >
max_len. However, it's not which causes lots of bogus output.

I've added code to reset the SAR and length in acquire_write_reply in
case write_mtu might have changed. 

This seems to work, but I'm sure there is a better way.

Steve

---

[config: Target = 0100]# get-ttl
mesh_gatt_write: max_len:20 len:21 sar:0 write_mtu:0 write_io:(nil)
Characteristic property changed /org/bluez/hci0/dev_C9_63_A3_A6_CD_DA/service000a/char000b
AcquireWrite success: fd 8 MTU 69
pipe_write: len:21 max_len:65
GATT-TX:         40 f4 a8 fa b2 33 ac 51 80 ff 16 2e 74 b4 a0 bb
GATT-TX:         2f 2f 54 e2 96
GATT-TX: iov[0]:         40
GATT-TX: iov[1]:         f4 a8 fa b2 33 ac 51 80 ff 16 2e 74 b4 a0 bb 2f
GATT-TX: iov[1]:         2f 54 e2
GATT-TX: iov[0]:         80
GATT-TX: iov[1]:         00 00 fe ff ff ff fe ff ff ff fe ff ff ff fe ff
GATT-TX: iov[1]:         ff ff fe ff ff ff fe ff ff ff fe ff ff ff fe ff
GATT-TX: iov[1]:         ff ff 14 00 00 00 fe ff ff ff 01 00 00 00 70 15
GATT-TX: iov[1]:         d6 01 36 00 00 00 3e 00 00 00 00 00 00 00 00 00
GATT-TX: iov[1]:         00
GATT-TX: iov[0]:         80
GATT-TX: iov[1]:         00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
GATT-TX: iov[1]:         00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00
GATT-TX: iov[1]:         00 00 00 00 00 00 00 00 00 51 00 00 00 06 00 00
GATT-TX: iov[1]:         00 0c 98 d7 76 00 00 00 00 00 00 00 00 04 00 97
GATT-TX: iov[1]:         d7
GATT-TX: iov[0]:         80
GATT-TX: iov[1]:         76 c4 97 d7 00 00 00 00 30 00 00 00 b0 4d d4 01
GATT-TX: iov[1]:         e0 b6 dc 76 20 b6 dc 76 21 00 00 00 dc 97 d7 76
GATT-TX: iov[1]:         dc 97 d7 76 28 4e d4 01 20 00 00 00 00 00 00 00
GATT-TX: iov[1]:         00 00 00 00 50 00 00 00 29 00 00 00 00 00 00 00
GATT-TX: iov[1]:         00
GATT-TX: iov[0]:         c0
GATT-TX: iov[1]:         00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
GATT-TX: iov[1]:         00

diff --git a/mesh/gatt.c b/mesh/gatt.c
index 001eb17a8..3f59268f2 100644
--- a/mesh/gatt.c
+++ b/mesh/gatt.c
@@ -358,6 +362,7 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 	struct write_data *data = user_data;
 	DBusError error;
 	int fd;
+	uint8_t max_len;
 
 	dbus_error_init(&error);
 
@@ -383,6 +388,18 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
 
 	write_io = pipe_io_new(fd);
 
+	/* Reset type and length as write_mtu may have changed */
+	max_len = write_mtu ? write_mtu - 3 : GATT_MTU - 3;
+	data->gatt_data[0] &= GATT_TYPE_MASK;
+
+	if (max_len < data->gatt_len) {
+		data->iov.iov_len = max_len;
+		data->gatt_data[0] |= GATT_SAR_FIRST;
+	}
+
+	else
+		data->iov.iov_len = data->gatt_len;
+
 	pipe_write(write_io, data);
 }
 
 

Re: [RFC]Mesh: meshctl configuration output fails in gatt.c:pipe_write

[Luiz Augusto von Dentz]

Hi Steve,

On Fri, Nov 24, 2017 at 4:39 PM, Steve Brown <sbrown@cortland.com> wrote:
> If the first command output in a new connection exceeds 20 bytes,
> mesh_gatt_write sets the SAR to FIRST as the write_mtu is initially 0
> and the default is GATT_MTU-3 (20).
>
> When pipe_write gets called, a new larger write_mtu has been set, but
> the SAR is still set to FIRST. It's assumed that data->gatt_len >
> max_len. However, it's not which causes lots of bogus output.
>
> I've added code to reset the SAR and length in acquire_write_reply in
> case write_mtu might have changed.
>
> This seems to work, but I'm sure there is a better way.
>
> Steve
>
> ---
>
> [config: Target = 0100]# get-ttl
> mesh_gatt_write: max_len:20 len:21 sar:0 write_mtu:0 write_io:(nil)
> Characteristic property changed /org/bluez/hci0/dev_C9_63_A3_A6_CD_DA/service000a/char000b
> AcquireWrite success: fd 8 MTU 69
> pipe_write: len:21 max_len:65
> GATT-TX:         40 f4 a8 fa b2 33 ac 51 80 ff 16 2e 74 b4 a0 bb
> GATT-TX:         2f 2f 54 e2 96
> GATT-TX: iov[0]:         40
> GATT-TX: iov[1]:         f4 a8 fa b2 33 ac 51 80 ff 16 2e 74 b4 a0 bb 2f
> GATT-TX: iov[1]:         2f 54 e2
> GATT-TX: iov[0]:         80
> GATT-TX: iov[1]:         00 00 fe ff ff ff fe ff ff ff fe ff ff ff fe ff
> GATT-TX: iov[1]:         ff ff fe ff ff ff fe ff ff ff fe ff ff ff fe ff
> GATT-TX: iov[1]:         ff ff 14 00 00 00 fe ff ff ff 01 00 00 00 70 15
> GATT-TX: iov[1]:         d6 01 36 00 00 00 3e 00 00 00 00 00 00 00 00 00
> GATT-TX: iov[1]:         00
> GATT-TX: iov[0]:         80
> GATT-TX: iov[1]:         00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> GATT-TX: iov[1]:         00 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00
> GATT-TX: iov[1]:         00 00 00 00 00 00 00 00 00 51 00 00 00 06 00 00
> GATT-TX: iov[1]:         00 0c 98 d7 76 00 00 00 00 00 00 00 00 04 00 97
> GATT-TX: iov[1]:         d7
> GATT-TX: iov[0]:         80
> GATT-TX: iov[1]:         76 c4 97 d7 00 00 00 00 30 00 00 00 b0 4d d4 01
> GATT-TX: iov[1]:         e0 b6 dc 76 20 b6 dc 76 21 00 00 00 dc 97 d7 76
> GATT-TX: iov[1]:         dc 97 d7 76 28 4e d4 01 20 00 00 00 00 00 00 00
> GATT-TX: iov[1]:         00 00 00 00 50 00 00 00 29 00 00 00 00 00 00 00
> GATT-TX: iov[1]:         00
> GATT-TX: iov[0]:         c0
> GATT-TX: iov[1]:         00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> GATT-TX: iov[1]:         00
>
> diff --git a/mesh/gatt.c b/mesh/gatt.c
> index 001eb17a8..3f59268f2 100644
> --- a/mesh/gatt.c
> +++ b/mesh/gatt.c
> @@ -358,6 +362,7 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
>         struct write_data *data = user_data;
>         DBusError error;
>         int fd;
> +       uint8_t max_len;
>
>         dbus_error_init(&error);
>
> @@ -383,6 +388,18 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
>
>         write_io = pipe_io_new(fd);
>
> +       /* Reset type and length as write_mtu may have changed */
> +       max_len = write_mtu ? write_mtu - 3 : GATT_MTU - 3;
> +       data->gatt_data[0] &= GATT_TYPE_MASK;

Ive never really liked the idea of putting the SAR byte into data but
it was required due to use of WriteValue, now perhaps we can even
remove that since AcquireWrite is stable the sar byte can be set in an
independent variable in pipe_write so we can move this logic there.

> +       if (max_len < data->gatt_len) {
> +               data->iov.iov_len = max_len;
> +               data->gatt_data[0] |= GATT_SAR_FIRST;
> +       }
> +
> +       else
> +               data->iov.iov_len = data->gatt_len;
> +
>         pipe_write(write_io, data);
>  }
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" 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

Re: [RFC]Mesh: meshctl configuration output fails in gatt.c:pipe_write

[Steve Brown]

Hi Luiz,

On Fri, 2017-11-24 at 17:10 +0200, Luiz Augusto von Dentz wrote:
> Hi Steve,
> 
> On Fri, Nov 24, 2017 at 4:39 PM, Steve Brown <sbrown@cortland.com>
> wrote:
> > If the first command output in a new connection exceeds 20 bytes,
> > mesh_gatt_write sets the SAR to FIRST as the write_mtu is initially
> > 0
> > and the default is GATT_MTU-3 (20).
> > 
> > When pipe_write gets called, a new larger write_mtu has been set,
> > but
> > the SAR is still set to FIRST. It's assumed that data->gatt_len >
> > max_len. However, it's not which causes lots of bogus output.
> > 
> > I've added code to reset the SAR and length in acquire_write_reply
> > in
> > case write_mtu might have changed.
> > 
> > This seems to work, but I'm sure there is a better way.
> > 
> > Steve
> > 
> > ---
> > 
> > 
> > diff --git a/mesh/gatt.c b/mesh/gatt.c
> > index 001eb17a8..3f59268f2 100644
> > --- a/mesh/gatt.c
> > +++ b/mesh/gatt.c
> > @@ -358,6 +362,7 @@ static void acquire_write_reply(DBusMessage
> > *message, void *user_data)
> >         struct write_data *data = user_data;
> >         DBusError error;
> >         int fd;
> > +       uint8_t max_len;
> > 
> >         dbus_error_init(&error);
> > 
> > @@ -383,6 +388,18 @@ static void acquire_write_reply(DBusMessage
> > *message, void *user_data)
> > 
> >         write_io = pipe_io_new(fd);
> > 
> > +       /* Reset type and length as write_mtu may have changed */
> > +       max_len = write_mtu ? write_mtu - 3 : GATT_MTU - 3;
> > +       data->gatt_data[0] &= GATT_TYPE_MASK;
> 
> Ive never really liked the idea of putting the SAR byte into data but
> it was required due to use of WriteValue, now perhaps we can even
> remove that since AcquireWrite is stable the sar byte can be set in
> an
> independent variable in pipe_write so we can move this logic there.
> 
> > +       if (max_len < data->gatt_len) {
> > +               data->iov.iov_len = max_len;
> > +               data->gatt_data[0] |= GATT_SAR_FIRST;
> > +       }
> > +
> > +       else
> > +               data->iov.iov_len = data->gatt_len;
> > +
> >         pipe_write(write_io, data);
> >  }
> > 

Thanks for your quick response.

I've just started reading the gatt-api doc. I might be ready to have at
this sometime in the spring. 

I discovered the problem while trying to configure the new zephyr
mesh_shell. It should probably be fixed sooner than later. I best defer
to somebody more experienced.

Steve

Re: [RFC]Mesh: meshctl configuration output fails in gatt.c:pipe_write

[Steve Brown]

Hi Luiz,

On Fri, 2017-11-24 at 17:10 +0200, Luiz Augusto von Dentz wrote:
> Hi Steve,
> 
> On Fri, Nov 24, 2017 at 4:39 PM, Steve Brown <sbrown@cortland.com>
> wrote:
> > If the first command output in a new connection exceeds 20 bytes,
> > mesh_gatt_write sets the SAR to FIRST as the write_mtu is initially
> > 0
> > and the default is GATT_MTU-3 (20).
> > 
> > When pipe_write gets called, a new larger write_mtu has been set,
> > but
> > the SAR is still set to FIRST. It's assumed that data->gatt_len >
> > max_len. However, it's not which causes lots of bogus output.
> > 
> > I've added code to reset the SAR and length in acquire_write_reply
> > in
> > case write_mtu might have changed.
> > 
> > This seems to work, but I'm sure there is a better way.
> > 
> > Steve
> > 

> Ive never really liked the idea of putting the SAR byte into data but
> it was required due to use of WriteValue, now perhaps we can even
> remove that since AcquireWrite is stable the sar byte can be set in
> an
> independent variable in pipe_write so we can move this logic there.
> 
> 
> 

Have you had a chance to review my changes to gatt.c?

http://www.spinics.net/lists/linux-bluetooth/msg72981.html

I removed the WriteValue code and moved the sar logic to pipe_write as
you requested. The latter fixed the bug I reported. If you have no
additional comments, let me know and I'll submit it as a patch.

I'm working on some additional commands for meshctl and they won't work
without the bug fixed.

Thanks,

Steve