Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1414525067-15076-1-git-send-email-marcin.kraglak@tieto.com> <1414525067-15076-2-git-send-email-marcin.kraglak@tieto.com> Date: Fri, 31 Oct 2014 15:03:05 +0100 Message-ID: Subject: Re: [PATCHv2 2/7] unit/test-gatt: Add search all primary services test case From: Marcin Kraglak To: Luiz Augusto von Dentz Cc: Arman Uguray , Michael Janssen , "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, On 30 October 2014 16:41, Luiz Augusto von Dentz wrote: > Hi Marcin, > > On Thu, Oct 30, 2014 at 8:26 AM, Marcin Kraglak > wrote: >> Hi Luiz, Arman >> >> On 29 October 2014 17:11, Arman Uguray wrote: >>> Hi Luiz, >>> >>> On Wed, Oct 29, 2014 at 8:25 AM, Luiz Augusto von Dentz >>> wrote: >>>> Hi Michael, >>>> >>>> On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen wrote: >>>>> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz >>>>> wrote: >>>>>> Hi Marcin, >>>>>> >>>>>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak >>>>>> wrote: >>>>>>> --- >>>>>>> unit/test-gatt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++------ >>>>>>> 1 file changed, 77 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c >>>>>>> index bbbf9a5..f84b0fc 100644 >>>>>>> --- a/unit/test-gatt.c >>>>>>> +++ b/unit/test-gatt.c >>>>>>> @@ -35,8 +35,10 @@ >>>>>>> >>>>>>> #include >>>>>>> >>>>>>> +#include "lib/uuid.h" >>>>>>> #include "src/shared/util.h" >>>>>>> #include "src/shared/att.h" >>>>>>> +#include "src/shared/gatt-helpers.h" >>>>>>> #include "src/shared/gatt-client.h" >>>>>>> >>>>>>> struct test_pdu { >>>>>>> @@ -45,14 +47,22 @@ struct test_pdu { >>>>>>> size_t size; >>>>>>> }; >>>>>>> >>>>>>> +enum context_type { >>>>>>> + ATT, >>>>>>> + CLIENT, >>>>>>> + SERVER >>>>>>> +}; >>>>>> >>>>>> Well the ATT part of this is a bit weird, do we really need to do this >>>>>> distinction. >>>>>> >>>>>>> struct test_data { >>>>>>> char *test_name; >>>>>>> struct test_pdu *pdu_list; >>>>>>> + enum context_type context_type; >>>>>>> }; >>>>>>> >>>>>>> struct context { >>>>>>> GMainLoop *main_loop; >>>>>>> struct bt_gatt_client *client; >>>>>>> + struct bt_att *att; >>>>>>> guint source; >>>>>>> guint process; >>>>>>> int fd; >>>>>>> @@ -69,13 +79,14 @@ struct context { >>>>>>> .size = sizeof(data(args)), \ >>>>>>> } >>>>>>> >>>>>>> -#define define_test(name, function, args...) \ >>>>>>> +#define define_test(name, function, type, args...) \ >>>>>>> do { \ >>>>>>> const struct test_pdu pdus[] = { \ >>>>>>> args, { } \ >>>>>>> }; \ >>>>>>> static struct test_data data; \ >>>>>>> data.test_name = g_strdup(name); \ >>>>>>> + data.context_type = type; \ >>>>>>> data.pdu_list = g_malloc(sizeof(pdus)); \ >>>>>>> memcpy(data.pdu_list, pdus, sizeof(pdus)); \ >>>>>>> g_test_add_data_func(name, &data, function); \ >>>>>>> @@ -182,6 +193,7 @@ static void gatt_debug(const char *str, void *user_data) >>>>>>> static struct context *create_context(uint16_t mtu, gconstpointer data) >>>>>>> { >>>>>>> struct context *context = g_new0(struct context, 1); >>>>>>> + const struct test_data *test_data = data; >>>>>>> GIOChannel *channel; >>>>>>> int err, sv[2]; >>>>>>> struct bt_att *att; >>>>>>> @@ -195,12 +207,25 @@ static struct context *create_context(uint16_t mtu, gconstpointer data) >>>>>>> att = bt_att_new(sv[0]); >>>>>>> g_assert(att); >>>>>>> >>>>>>> - context->client = bt_gatt_client_new(att, mtu); >>>>>>> - g_assert(context->client); >>>>>>> + switch (test_data->context_type) { >>>>>>> + case ATT: >>>>>>> + context->att = att; >>>>>>> >>>>>>> - if (g_test_verbose()) >>>>>>> - bt_gatt_client_set_debug(context->client, gatt_debug, "gatt:", >>>>>>> - NULL); >>>>>>> + bt_gatt_exchange_mtu(context->att, mtu, NULL, NULL, NULL); >>>>>>> + break; >>>>>>> + case CLIENT: >>>>>>> + context->client = bt_gatt_client_new(att, mtu); >>>>>>> + g_assert(context->client); >>>>>>> + >>>>>>> + if (g_test_verbose()) >>>>>>> + bt_gatt_client_set_debug(context->client, gatt_debug, >>>>>>> + "gatt:", NULL); >>>>>>> + >>>>>>> + bt_att_unref(att); >>>>>>> + break; >>>>>>> + default: >>>>>>> + break; >>>>>>> + } >>>>>>> >>>>>>> channel = g_io_channel_unix_new(sv[1]); >>>>>>> >>>>>>> @@ -214,7 +239,6 @@ static struct context *create_context(uint16_t mtu, gconstpointer data) >>>>>>> g_assert(context->source > 0); >>>>>>> >>>>>>> g_io_channel_unref(channel); >>>>>>> - bt_att_unref(att); >>>>>>> >>>>>>> context->fd = sv[1]; >>>>>>> context->data = data; >>>>>>> @@ -222,6 +246,17 @@ static struct context *create_context(uint16_t mtu, gconstpointer data) >>>>>>> return context; >>>>>>> } >>>>>>> >>>>>>> +static void primary_cb(bool success, uint8_t att_ecode, >>>>>>> + struct bt_gatt_result *result, >>>>>>> + void *user_data) >>>>>>> +{ >>>>>>> + struct context *context = user_data; >>>>>>> + >>>>>>> + g_assert(success); >>>>>>> + >>>>>>> + context_quit(context); >>>>>>> +} >>>>>>> + >>>>>>> static void destroy_context(struct context *context) >>>>>>> { >>>>>>> if (context->source > 0) >>>>>>> @@ -229,6 +264,9 @@ static void destroy_context(struct context *context) >>>>>>> >>>>>>> bt_gatt_client_unref(context->client); >>>>>>> >>>>>>> + if (context->att) >>>>>>> + bt_att_unref(context->att); >>>>>>> + >>>>>>> g_main_loop_unref(context->main_loop); >>>>>>> >>>>>>> test_free(context->data); >>>>>>> @@ -249,6 +287,16 @@ static void test_client(gconstpointer data) >>>>>>> execute_context(context); >>>>>>> } >>>>>>> >>>>>>> +static void test_search_primary(gconstpointer data) >>>>>>> +{ >>>>>>> + struct context *context = create_context(512, data); >>>>>>> + >>>>>>> + bt_gatt_discover_all_primary_services(context->att, NULL, primary_cb, >>>>>>> + context, NULL); >>>>>> >>>>>> Doesn't bt_gatt_client does the same here, I guess I missed this >>>>>> detail in the first time but if this is just testing client side then >>>>>> it should use bt_gatt_client and if that cannot be tested we probably >>>>>> have a problem with our API. I guess PTS would just pass if we do more >>>>>> than search the primary services, so things like setting the MTU etc >>>>>> should not be a problem here. >>>>> >>>>> Specifically setting the MTU is allowed, but the reason to avoid using >>>>> bt_gatt_client_new/init here is to avoid the automatic service >>>>> discovery. This is probably a good idea at least for testing the >>>>> different types of discovery. (is bt_gatt_client Alternately maybe >>>>> just test the type of discovery used by bt_gatt_client. When >>>>> implementing the read value tests, the auto-discovery of >>>>> bt_gatt_client may make the test data longer because it needs to be >>>>> included in every one. >>>> >>>> Isn't it what we want? To run exactly as it would be run against PTS >>>> so we avoid surprises? Not only this avoid auto-discovery but the >>>> caching done by bt_gatt_client which we would probably have to test >>>> separately, so it is much more code that we exercise if we test >>>> bt_gatt_client at the expense of a little bit more PDUs. Perhaps we >>>> could have a define containing the common PDUs that auto-discovery >>>> causes so the upcoming tests don't need to duplicate these PDUs. >>>> >> >> I agree we test much more code while testing gatt-client. My idea was >> to use bt_att in GAD group, >> because we need to send specified packets and it is more like testing >> protocol than profile. >> Whole discovery using gatt-client will be tested in most of GAR and >> GAW test cases. >> I'm sure that it will be bigger part of unit tests. > > Fair enough, but please add the test description to the commits so we > are consistent, for example: > > unit/test-gatt: Add test > > Verify that a Generic Attribute Profile client discovers Primary > Services in a GATT server. I've added descriptions to commit messages BR Marcin