Hi Luiz, Arman
On 29 October 2014 17:11, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
> Hi Luiz,
>
> On Wed, Oct 29, 2014 at 8:25 AM, Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
>> Hi Michael,
>>
>> On Wed, Oct 29, 2014 at 5:03 PM, Michael Janssen <jamuraa@xxxxxxxxxx> wrote:
>>> On Wed, Oct 29, 2014 at 5:32 AM, Luiz Augusto von Dentz
>>> <luiz.dentz@xxxxxxxxx> wrote:
>>>> Hi Marcin,
>>>>
>>>> On Tue, Oct 28, 2014 at 9:37 PM, Marcin Kraglak
>>>> <marcin.kraglak@xxxxxxxxx> 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 <glib.h>
>>>>>
>>>>> +#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.
>
> In general, there is value in unit testing the gatt-helpers functions
> directly but overall I agree with Luiz that we should run these tests
> exactly like we would run it against the PTS. As long as the
> "database" we're returning in the response PDUs matches the complexity
> required by the test specification, a simple initialization of
> bt_gatt_client would give us all major discovery test cases. Of course
> we'll need to call gatt-helpers directly for test cases involving GATT
> procedures that don't aren't currently being used by bt_gatt_client
> (e.g. discover services by UUID).
>
> So it's probably OK to leave these ATT level tests for now but we also
> want at least a CLIENT level test to verify all of discovery test spec
> cases using the service/characteristic/include iterators.
>
>>>>> + execute_context(context);
>>>>> +}
>>>>> +
>>>>> int main(int argc, char *argv[])
>>>>> {
>>>>> g_test_init(&argc, &argv, NULL);
>>>>> @@ -259,8 +307,29 @@ int main(int argc, char *argv[])
>>>>> * The test group objective is to verify Generic Attribute Profile
>>>>> * Server Configuration.
>>>>> */
>>>>> - define_test("/TP/GAC/CL/BV-01-C", test_client,
>>>>> + define_test("/TP/GAC/CL/BV-01-C", test_client, CLIENT,
>>>>> raw_pdu(0x02, 0x00, 0x02));
>>>>>
>>>>> + /*
>>>>> + * Discovery
>>>>> + *
>>>>> + * The test group objective is to verify Generic Attribute Profile
>>>>> + * Discovery of Services and Service Characteristics.
>>>>> + */
>>>>> + define_test("/TP/GAD/CL/BV-01-C", test_search_primary, ATT,
>>>>> + raw_pdu(0x02, 0x00, 0x02),
>>>>> + raw_pdu(0x03, 0x00, 0x02),
>>>>> + raw_pdu(0x10, 0x01, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>>> + raw_pdu(0x11, 0x06, 0x10, 0x00, 0x13, 0x00, 0x00, 0x18,
>>>>> + 0x20, 0x00, 0x29, 0x00, 0xb0, 0x68,
>>>>> + 0x30, 0x00, 0x32, 0x00, 0x19, 0x18),
>>>>> + raw_pdu(0x10, 0x33, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>>> + raw_pdu(0x11, 0x14, 0x90, 0x00, 0x96, 0x00, 0xef, 0xcd,
>>>>> + 0xab, 0x89, 0x67, 0x45, 0x23, 0x01,
>>>>> + 0x00, 0x00, 0x00, 0x00, 0x85, 0x60,
>>>>> + 0x00, 0x00),
>>>>> + raw_pdu(0x10, 0x97, 0x00, 0xff, 0xff, 0x00, 0x28),
>>>>> + raw_pdu(0x01, 0x10, 0x97, 0x00, 0x0a));
>>>>> +
>>>>> return g_test_run();
>>>>> }
>>>>> --
>>>>> 1.9.3
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> Michael Janssen
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Arman
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
