[2/5] tests: more thorough test of ds1338

Test current time and set+get round trip.

The set+get test is repeated 4 times.  These cases are
spread across a single day in an attempt to trigger some potential
issues regardless of the timezone of the machine running the tests.

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 tests/Makefile.include  |   2 +
 tests/ds-rtc-i2c-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 195 insertions(+)
 create mode 100644 tests/ds-rtc-i2c-test.c

On 19.02.2018 05:03, Michael Davidsaver wrote:
> Test current time and set+get round trip.
> 
> The set+get test is repeated 4 times.  These cases are
> spread across a single day in an attempt to trigger some potential
> issues regardless of the timezone of the machine running the tests.
> 
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  tests/Makefile.include  |   2 +
>  tests/ds-rtc-i2c-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 195 insertions(+)
>  create mode 100644 tests/ds-rtc-i2c-test.c
[...]
>  tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
> diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
> new file mode 100644
> index 0000000000..464eb08558
> --- /dev/null
> +++ b/tests/ds-rtc-i2c-test.c
> @@ -0,0 +1,193 @@
> +/* Testing of Dallas/Maxim I2C bus RTC devices
> + *
> + * Copyright (c) 2017 Michael Davidsaver
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the LICENSE file in the top-level directory.
> + */
> +#include <stdio.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bcd.h"
> +#include "qemu/cutils.h"
> +#include "qemu/timer.h"
> +#include "libqtest.h"
> +#include "libqos/libqos.h"
> +#include "libqos/i2c.h"
> +
> +#define IMX25_I2C_0_BASE 0x43F80000
> +#define DS1338_ADDR 0x68
> +
> +static I2CAdapter *i2c;
> +static uint8_t addr;
> +static bool use_century;
> +
> +static
> +time_t rtc_gettime(void)
> +{
> +    struct tm parts;
> +    uint8_t buf[7];
> +
> +    buf[0] = 0;
> +    i2c_send(i2c, addr, buf, 1);
> +    i2c_recv(i2c, addr, buf, 7);
> +
> +    parts.tm_sec = from_bcd(buf[0]);
> +    parts.tm_min = from_bcd(buf[1]);
> +    if (buf[2] & 0x40) {
> +        /* 12 hour */
> +        /* HOUR register is 1-12. */
> +        parts.tm_hour = from_bcd(buf[2] & 0x1f);
> +        g_assert_cmpuint(parts.tm_hour, >=, 1);
> +        g_assert_cmpuint(parts.tm_hour, <=, 12);
> +        parts.tm_hour %= 12u; /* wrap 12 -> 0 */
> +        if (buf[2] & 0x20) {
> +            parts.tm_hour += 12u;
> +        }
> +    } else {
> +        /* 24 hour */
> +        parts.tm_hour = from_bcd(buf[2] & 0x3f);
> +    }
> +    parts.tm_wday = from_bcd(buf[3]);
> +    parts.tm_mday = from_bcd(buf[4]);
> +    parts.tm_mon =  from_bcd((buf[5] & 0x1f) - 1u);
> +    parts.tm_year = from_bcd(buf[6]);
> +    if (!use_century || (buf[5] & 0x80)) {
> +        parts.tm_year += 100u;
> +    }
> +
> +    return mktimegm(&parts);
> +}
> +
> +/* read back and compare with current system time */
> +static
> +void test_rtc_current(void)
> +{
> +    uint8_t buf;
> +    time_t expected, actual;
> +
> +    /* magic address to zero RTC time offset
> +     * as tests may be run in any order
> +     */
> +    buf = 0xff;
> +    i2c_send(i2c, addr, &buf, 1);

That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
the same problem with the m48t59 test recently, and I solved it by
moving the qtest_start() and qtest_end() calls from the main() function
into the single tests instead, so that each test starts with a clean state:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f

Could you maybe try whether that approach works for your test cases
here, too? Then you could do this without the "0xff" hack here...

> +
> +    actual = time(NULL);
> +    /* new second may start here */
> +    expected = rtc_gettime();
> +    g_assert_cmpuint(expected, <=, actual + 1);
> +    g_assert_cmpuint(expected, >=, actual);
> +}
> +
> +
> +static uint8_t test_time_24_12am[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 00:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x00, /* 12 AM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +static uint8_t test_time_24_6am[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 06:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x06, /* 6 AM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +static uint8_t test_time_24_12pm[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 12:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x12, /* 12 PM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +static uint8_t test_time_24_6pm[8] = {
> +    0, /* address */
> +    /* Wed, 22 Nov 2017 18:30:53 +0000 */
> +    0x53,
> +    0x30,
> +    0x18, /* 6 PM in 24 hour mode */
> +    0x03, /* monday is our day 1 */
> +    0x22,
> +    0x11 | 0x80,
> +    0x17,
> +};
> +
> +/* write in and read back known time */
> +static
> +void test_rtc_set(const void *raw)
> +{
> +    const uint8_t *testtime = raw;
> +    uint8_t buf[7];
> +    unsigned retry = 2;
> +
> +    for (; retry; retry--) {
> +        i2c_send(i2c, addr, testtime, 8);
> +        /* new second may start here */
> +        i2c_send(i2c, addr, testtime, 1);
> +        i2c_recv(i2c, addr, buf, 7);
> +
> +        if (testtime[1] == buf[0]) {

Please also check the minutes here (reason: see below).

> +            break;
> +        }
> +        /* we raced start of second, retry */
> +    };
> +
> +    g_assert_cmpuint(testtime[1], ==, buf[0]); /* SEC */
> +    g_assert_cmpuint(testtime[2], ==, buf[1]); /* MIN */

Could you please wrap the SEC and MIN lines in a "if (!g_test_slow()) {
... }" statement? The problem is: The "make check" tests are run as CI
on a system that is sometimes *very* overloaded. It might happen that
the test is sometimes interrupted for dozens of seconds, so it might
fail to properly read the time on a granularity of seconds. With
!g_test_slow() you can make sure that the check is not done on such
overloaded CI systems.

> +    g_assert_cmpuint(testtime[3], ==, buf[2]); /* HOUR */
> +    /* skip comparing Day of Week.  Not handled correctly */
> +    g_assert_cmpuint(testtime[5], ==, buf[4]); /* DoM */
> +    if (use_century) {
> +        g_assert_cmpuint(testtime[6], ==, buf[5]); /* MON+century */
> +    } else {
> +        g_assert_cmpuint(testtime[6] & 0x7f, ==, buf[5]); /* MON */
> +    }
> +    g_assert_cmpuint(testtime[7], ==, buf[6]); /* YEAR */
> +
> +    g_assert_cmpuint(retry, >, 0);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    int ret;
> +    const char *arch = qtest_get_arch();
> +    QTestState *s = NULL;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    if (strcmp(arch, "arm") == 0) {
> +        s = qtest_start("-display none -machine imx25-pdk");

Do you really need the "-display none" parameter here? ... I thought
that was the default for qtests anyway?

> +        i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
> +        addr = DS1338_ADDR;
> +        use_century = false;
> +
> +    }
> +
> +    qtest_add_data_func("/ds-rtc-i2c/set24_12am", test_time_24_12am, test_rtc_set);
> +    qtest_add_data_func("/ds-rtc-i2c/set24_6am", test_time_24_6am, test_rtc_set);
> +    qtest_add_data_func("/ds-rtc-i2c/set24_12pm", test_time_24_12pm, test_rtc_set);
> +    qtest_add_data_func("/ds-rtc-i2c/set24_6pm", test_time_24_6pm, test_rtc_set);
> +    qtest_add_func("/ds-rtc-i2c/current", test_rtc_current);
> +
> +    ret = g_test_run();
> +
> +    qtest_end();
> +
> +    return ret;
> +}
> 

 Thomas

On 02/18/2018 11:39 PM, Thomas Huth wrote:
> On 19.02.2018 05:03, Michael Davidsaver wrote:
>> Test current time and set+get round trip.
>>
>> The set+get test is repeated 4 times.  These cases are
>> spread across a single day in an attempt to trigger some potential
>> issues regardless of the timezone of the machine running the tests.
>>
>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>> ---
>>  tests/Makefile.include  |   2 +
>>  tests/ds-rtc-i2c-test.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 195 insertions(+)
>>  create mode 100644 tests/ds-rtc-i2c-test.c
> [...]
>>  tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
>> diff --git a/tests/ds-rtc-i2c-test.c b/tests/ds-rtc-i2c-test.c
>> new file mode 100644
>> index 0000000000..464eb08558
>> --- /dev/null
>> +++ b/tests/ds-rtc-i2c-test.c
>> @@ -0,0 +1,193 @@
>> +/* Testing of Dallas/Maxim I2C bus RTC devices
>> + *
>> + * Copyright (c) 2017 Michael Davidsaver
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the LICENSE file in the top-level directory.
>> + */
>> +#include <stdio.h>
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/bcd.h"
>> +#include "qemu/cutils.h"
>> +#include "qemu/timer.h"
>> +#include "libqtest.h"
>> +#include "libqos/libqos.h"
>> +#include "libqos/i2c.h"
>> +
>> +#define IMX25_I2C_0_BASE 0x43F80000
>> +#define DS1338_ADDR 0x68
>> +
>> +static I2CAdapter *i2c;
>> +static uint8_t addr;
>> +static bool use_century;
>> +
>> +static
>> +time_t rtc_gettime(void)
>> +{
>> +    struct tm parts;
>> +    uint8_t buf[7];
>> +
>> +    buf[0] = 0;
>> +    i2c_send(i2c, addr, buf, 1);
>> +    i2c_recv(i2c, addr, buf, 7);
>> +
>> +    parts.tm_sec = from_bcd(buf[0]);
>> +    parts.tm_min = from_bcd(buf[1]);
>> +    if (buf[2] & 0x40) {
>> +        /* 12 hour */
>> +        /* HOUR register is 1-12. */
>> +        parts.tm_hour = from_bcd(buf[2] & 0x1f);
>> +        g_assert_cmpuint(parts.tm_hour, >=, 1);
>> +        g_assert_cmpuint(parts.tm_hour, <=, 12);
>> +        parts.tm_hour %= 12u; /* wrap 12 -> 0 */
>> +        if (buf[2] & 0x20) {
>> +            parts.tm_hour += 12u;
>> +        }
>> +    } else {
>> +        /* 24 hour */
>> +        parts.tm_hour = from_bcd(buf[2] & 0x3f);
>> +    }
>> +    parts.tm_wday = from_bcd(buf[3]);
>> +    parts.tm_mday = from_bcd(buf[4]);
>> +    parts.tm_mon =  from_bcd((buf[5] & 0x1f) - 1u);
>> +    parts.tm_year = from_bcd(buf[6]);
>> +    if (!use_century || (buf[5] & 0x80)) {
>> +        parts.tm_year += 100u;
>> +    }
>> +
>> +    return mktimegm(&parts);
>> +}
>> +
>> +/* read back and compare with current system time */
>> +static
>> +void test_rtc_current(void)
>> +{
>> +    uint8_t buf;
>> +    time_t expected, actual;
>> +
>> +    /* magic address to zero RTC time offset
>> +     * as tests may be run in any order
>> +     */
>> +    buf = 0xff;
>> +    i2c_send(i2c, addr, &buf, 1);
> 
> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
> the same problem with the m48t59 test recently, and I solved it by
> moving the qtest_start() and qtest_end() calls from the main() function
> into the single tests instead, so that each test starts with a clean state:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
> 
> Could you maybe try whether that approach works for your test cases
> here, too? Then you could do this without the "0xff" hack here...

Your right, this looks clearer.  I'll try this approach.

>> +
>> +    actual = time(NULL);
>> +    /* new second may start here */
>> +    expected = rtc_gettime();
>> +    g_assert_cmpuint(expected, <=, actual + 1);
>> +    g_assert_cmpuint(expected, >=, actual);
>> +}
>> +
>> +
>> +static uint8_t test_time_24_12am[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 00:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x00, /* 12 AM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +static uint8_t test_time_24_6am[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 06:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x06, /* 6 AM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +static uint8_t test_time_24_12pm[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 12:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x12, /* 12 PM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +static uint8_t test_time_24_6pm[8] = {
>> +    0, /* address */
>> +    /* Wed, 22 Nov 2017 18:30:53 +0000 */
>> +    0x53,
>> +    0x30,
>> +    0x18, /* 6 PM in 24 hour mode */
>> +    0x03, /* monday is our day 1 */
>> +    0x22,
>> +    0x11 | 0x80,
>> +    0x17,
>> +};
>> +
>> +/* write in and read back known time */
>> +static
>> +void test_rtc_set(const void *raw)
>> +{
>> +    const uint8_t *testtime = raw;
>> +    uint8_t buf[7];
>> +    unsigned retry = 2;
>> +
>> +    for (; retry; retry--) {
>> +        i2c_send(i2c, addr, testtime, 8);
>> +        /* new second may start here */
>> +        i2c_send(i2c, addr, testtime, 1);
>> +        i2c_recv(i2c, addr, buf, 7);
>> +
>> +        if (testtime[1] == buf[0]) {
> 
> Please also check the minutes here (reason: see below).
> 
>> +            break;
>> +        }
>> +        /* we raced start of second, retry */
>> +    };
>> +
>> +    g_assert_cmpuint(testtime[1], ==, buf[0]); /* SEC */
>> +    g_assert_cmpuint(testtime[2], ==, buf[1]); /* MIN */
> 
> Could you please wrap the SEC and MIN lines in a "if (!g_test_slow()) {
> ... }" statement? The problem is: The "make check" tests are run as CI
> on a system that is sometimes *very* overloaded. It might happen that
> the test is sometimes interrupted for dozens of seconds, so it might
> fail to properly read the time on a granularity of seconds. With
> !g_test_slow() you can make sure that the check is not done on such
> overloaded CI systems.

Ok I guess.  I certainly don't want to add more false positive test failures.

>> +    g_assert_cmpuint(testtime[3], ==, buf[2]); /* HOUR */
>> +    /* skip comparing Day of Week.  Not handled correctly */
>> +    g_assert_cmpuint(testtime[5], ==, buf[4]); /* DoM */
>> +    if (use_century) {
>> +        g_assert_cmpuint(testtime[6], ==, buf[5]); /* MON+century */
>> +    } else {
>> +        g_assert_cmpuint(testtime[6] & 0x7f, ==, buf[5]); /* MON */
>> +    }
>> +    g_assert_cmpuint(testtime[7], ==, buf[6]); /* YEAR */
>> +
>> +    g_assert_cmpuint(retry, >, 0);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +    int ret;
>> +    const char *arch = qtest_get_arch();
>> +    QTestState *s = NULL;
>> +
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    if (strcmp(arch, "arm") == 0) {
>> +        s = qtest_start("-display none -machine imx25-pdk");
> 
> Do you really need the "-display none" parameter here? ... I thought
> that was the default for qtests anyway?

This is a straight copy+paste from the ds1338-test I'm replacing.
I'll remove it.

>> +        i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
>> +        addr = DS1338_ADDR;
>> +        use_century = false;
>> +
>> +    }
>> +
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_12am", test_time_24_12am, test_rtc_set);
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_6am", test_time_24_6am, test_rtc_set);
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_12pm", test_time_24_12pm, test_rtc_set);
>> +    qtest_add_data_func("/ds-rtc-i2c/set24_6pm", test_time_24_6pm, test_rtc_set);
>> +    qtest_add_func("/ds-rtc-i2c/current", test_rtc_current);
>> +
>> +    ret = g_test_run();
>> +
>> +    qtest_end();
>> +
>> +    return ret;
>> +}
>>
> 
>  Thomas
>

> On 02/20/2018 09:44 AM, Michael Davidsaver wrote:
>> On 02/18/2018 11:39 PM, Thomas Huth wrote:
...
>> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
>> the same problem with the m48t59 test recently, and I solved it by
>> moving the qtest_start() and qtest_end() calls from the main() function
>> into the single tests instead, so that each test starts with a clean state:
>>
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
>>
>> Could you maybe try whether that approach works for your test cases
>> here, too? Then you could do this without the "0xff" hack here...
> 
> Your right, this looks clearer.  I'll try this approach.

I ultimately decided not to go with this approach as test failures
didn't call qtest_quit(), and the process under test is left running
after gtester exits.

Instead I split the current time test off into a second executable.
This avoids all of the magic.


FYI.  I also looked at using g_test_add(), keeping the QTestState* in a
Fixture struct, and using setup and teardown functions to call
qtest_start()/qtest_quit().  This works, but seemed to me like too much
effort in this case.

On 24.03.2018 20:39, Michael Davidsaver wrote:
>> On 02/20/2018 09:44 AM, Michael Davidsaver wrote:
>>> On 02/18/2018 11:39 PM, Thomas Huth wrote:
> ...
>>> That magic (together with patch 1/5) is IMHO a little bit ugly. I've hit
>>> the same problem with the m48t59 test recently, and I solved it by
>>> moving the qtest_start() and qtest_end() calls from the main() function
>>> into the single tests instead, so that each test starts with a clean state:
>>>
>>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=9c29830c90d82f27f
>>>
>>> Could you maybe try whether that approach works for your test cases
>>> here, too? Then you could do this without the "0xff" hack here...
>>
>> Your right, this looks clearer.  I'll try this approach.
> 
> I ultimately decided not to go with this approach as test failures
> didn't call qtest_quit(), and the process under test is left running
> after gtester exits.
> 
> Instead I split the current time test off into a second executable.
> This avoids all of the magic.

Honestly, I don't like the idea that we put tests into different files
just because of this. We're not doing this for any other tests so far,
so just using this pattern for the ds1338 / ds1375 tests sounds wrong.

If there is an issue with test processes not being killed correctly, I
think we should rather fix that issue instead, maybe by adding a special
function with atexit() or something similar ... I haven't looked closely
into that yet.

Just my 0.02 €.

 Thomas