Re: [PATCH 3/3] Add gatttool enhancements for UPF

* Re: [PATCH 3/3] Add gatttool enhancements for UPF
  2011-02-16 21:18 ` [PATCH 3/3] Add gatttool enhancements for UPF Brian Gix
@ 2011-02-14 21:41   ` Anderson Lizardo
  2011-02-16 15:53     ` Brian Gix
  0 siblings, 1 reply; 11+ messages in thread
From: Anderson Lizardo @ 2011-02-14 21:41 UTC (permalink / raw)
  To: Brian Gix; +Cc: linux-bluetooth, johan.hedberg, padovan

Hi Brian,

On Wed, Feb 16, 2011 at 6:18 PM, Brian Gix <bgix@codeaurora.org> wrote:
> @@ -90,9 +91,9 @@ static GIOChannel *do_connect(gboolean le)
>
>        /* This check is required because currently setsockopt() returns no
>         * errors for MTU values smaller than the allowed minimum. */
> -       if (opt_mtu != 0 && opt_mtu < ATT_MIN_MTU_L2CAP) {
> +       if (opt_mtu != 0 && opt_mtu < 23) {
>                g_printerr("MTU cannot be smaller than %d\n",
> -                                                       ATT_MIN_MTU_L2CAP);
> +                                                       23);
>                return NULL;
>        }

The changes above seem unrelated to this patch.

>
> @@ -277,6 +278,14 @@ static gboolean characteristics(gpointer user_data)
>        return FALSE;
>  }
>
> +static void char_write_cb(guint8 status, const guint8 *pdu, guint16 plen,
> +                                                       gpointer user_data)
> +{
> +       if (plen == 1)
> +               g_print("Attrib Write Succeeded\n");
> +       else
> +               g_printerr("Attrib Write failed: %s\n", att_ecode2str(status));

Why not check by the status instead of plen ?

Also, I'd suggest "Characteristic write" instead of "Attrib Write".

> +}
>  static void char_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
>                                                        gpointer user_data)
>  {
> @@ -427,7 +436,40 @@ static gboolean characteristics_write(gpointer user_data)
>                goto error;
>        }
>
> +       gatt_write_char(attrib, opt_handle, value, len, char_write_cb, value);
> +       gatt_read_char(attrib, opt_handle, char_read_cb, attrib);

Why both read and write ?

> +
> +       return FALSE;
> +
> +error:
> +       g_main_loop_quit(event_loop);
> +       return FALSE;
> +}
> +
> +static gboolean characteristics_cmd(gpointer user_data)
> +{
> +       GAttrib *attrib = user_data;
> +       uint8_t *value;
> +       size_t len;
> +
> +       if (opt_handle <= 0) {
> +               g_printerr("A valid handle is required\n");
> +               goto error;
> +       }
> +
> +       if (opt_value == NULL || opt_value[0] == '\0') {
> +               g_printerr("A value is required\n");
> +               goto error;
> +       }
> +
> +       len = attr_data_from_string(opt_value, &value);
> +       if (len == 0) {
> +               g_printerr("Invalid value\n");
> +               goto error;
> +       }
> +
>        gatt_write_cmd(attrib, opt_handle, value, len, mainloop_quit, value);
> +       gatt_read_char(attrib, opt_handle, char_read_cb, attrib);

Same question here.

>
>        return FALSE;
>
> @@ -531,6 +573,8 @@ static GOptionEntry gatt_options[] = {
>                "Characteristics Value/Descriptor Read", NULL },
>        { "char-write", 0, 0, G_OPTION_ARG_NONE, &opt_char_write,
>                "Characteristics Value Write", NULL },
> +       { "char-cmd", 0, 0, G_OPTION_ARG_NONE, &opt_char_cmd,
> +               "Characteristics Value Cmd", NULL },

Suggestion:  "Characteristic Value write using Write Command" (or
something similar).

>        { "char-desc", 0, 0, G_OPTION_ARG_NONE, &opt_char_desc,
>                "Characteristics Descriptor Discovery", NULL },
>        { "listen", 0, 0, G_OPTION_ARG_NONE, &opt_listen,
> @@ -561,7 +605,7 @@ int main(int argc, char *argv[])
>        GError *gerr = NULL;
>        GAttrib *attrib;
>        GIOChannel *chan;
> -       GSourceFunc callback;
> +       GSourceFunc callback = NULL;
>
>        context = g_option_context_new(NULL);
>        g_option_context_add_main_entries(context, options, NULL);
> @@ -602,9 +646,11 @@ int main(int argc, char *argv[])
>                callback = characteristics_read;
>        else if (opt_char_write)
>                callback = characteristics_write;
> +       else if (opt_char_cmd)
> +               callback = characteristics_cmd;
>        else if (opt_char_desc)
>                callback = characteristics_desc;
> -       else {
> +       else if (!opt_listen) {
>                gchar *help = g_option_context_get_help(context, TRUE, NULL);
>                g_print("%s\n", help);
>                g_free(help);
> @@ -625,7 +671,8 @@ int main(int argc, char *argv[])
>        if (opt_listen)
>                g_idle_add(listen_start, attrib);
>
> -       g_idle_add(callback, attrib);
> +       if (callback)
> +               g_idle_add(callback, attrib);
>
>        g_main_loop_run(event_loop);

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

^ permalink raw reply	[flat|nested] 11+ messages in thread