| Message ID | 20190117161640.5496-4-jusual@mail.ru |
|---|---|
| State | New |
| Headers | show |
| Series | tests/microbit-test: Add UART device test | expand |
On Thu, Jan 17, 2019 at 07:16:40PM +0300, Julia Suvorova wrote: > diff --git a/tests/microbit-test.c b/tests/microbit-test.c > index afeb6b082a..3da6d9529f 100644 > --- a/tests/microbit-test.c > +++ b/tests/microbit-test.c > @@ -19,10 +19,93 @@ > #include "libqtest.h" > > #include "hw/arm/nrf51.h" > +#include "hw/char/nrf51_uart.h" > #include "hw/gpio/nrf51_gpio.h" > #include "hw/timer/nrf51_timer.h" > #include "hw/i2c/microbit_i2c.h" > > +#include <sys/socket.h> > +#include <sys/un.h> ./HACKING "1.2. Include directives" requires putting system header includes (<>) right after #include "qemu/osdep.h". But are these includes really needed? This code doesn't use Sockets API calls like send(2)/recv(2)/shutdown(2) or UNIX Domain Sockets specifics like struct sockaddr_un. I suggest dropping these includes (plus they are pulled in implicitly by osdep.h anyway). Peter: If there are no other issues with this patch series, could you remove these two includes when merging? Thanks!
On 2019-01-17 17:16, Julia Suvorova via Qemu-devel wrote: > Some functional tests for: > Basic reception/transmittion > Suspending > INTEN* registers > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > --- > tests/microbit-test.c | 84 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > > diff --git a/tests/microbit-test.c b/tests/microbit-test.c > index afeb6b082a..3da6d9529f 100644 > --- a/tests/microbit-test.c > +++ b/tests/microbit-test.c > @@ -19,10 +19,93 @@ > #include "libqtest.h" > > #include "hw/arm/nrf51.h" > +#include "hw/char/nrf51_uart.h" > #include "hw/gpio/nrf51_gpio.h" > #include "hw/timer/nrf51_timer.h" > #include "hw/i2c/microbit_i2c.h" > > +#include <sys/socket.h> > +#include <sys/un.h> > + > +static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr) > +{ > + int i; > + > + for (i = 0; i < 1000; i++) { > + if (qtest_readl(qts, event_addr) == 1) { > + qtest_writel(qts, event_addr, 0x00); > + return true; > + } > + g_usleep(10000); > + } So this times out after 10 seconds? ... this is likely plenty on a normal host, but we run the tests on overloaded CI systems sometimes, where 10 seconds are not that much... I'd suggest to replace the condition in the for-loop with "i < 30000" or even "i < 60000", just to be sure. > + return false; > +} > + > +static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in, > + char *out) > +{ > + int i, in_len = strlen(in); > + > + g_assert(write(sock_fd, in, in_len) == in_len); Sorry for being pedantic, but I'd really prefer to write it without side-effects in g_assert() please. (same for the other occurrences in this patch) Thomas
Thomas Huth <thuth@redhat.com> writes: > On 2019-01-17 17:16, Julia Suvorova via Qemu-devel wrote: >> Some functional tests for: >> Basic reception/transmittion >> Suspending >> INTEN* registers >> >> Signed-off-by: Julia Suvorova <jusual@mail.ru> >> --- >> tests/microbit-test.c | 84 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 84 insertions(+) >> >> diff --git a/tests/microbit-test.c b/tests/microbit-test.c >> index afeb6b082a..3da6d9529f 100644 >> --- a/tests/microbit-test.c >> +++ b/tests/microbit-test.c >> @@ -19,10 +19,93 @@ >> #include "libqtest.h" >> >> #include "hw/arm/nrf51.h" >> +#include "hw/char/nrf51_uart.h" >> #include "hw/gpio/nrf51_gpio.h" >> #include "hw/timer/nrf51_timer.h" >> #include "hw/i2c/microbit_i2c.h" >> >> +#include <sys/socket.h> >> +#include <sys/un.h> >> + >> +static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr) >> +{ >> + int i; >> + >> + for (i = 0; i < 1000; i++) { >> + if (qtest_readl(qts, event_addr) == 1) { >> + qtest_writel(qts, event_addr, 0x00); >> + return true; >> + } >> + g_usleep(10000); >> + } > > So this times out after 10 seconds? ... this is likely plenty on a > normal host, but we run the tests on overloaded CI systems sometimes, > where 10 seconds are not that much... > > I'd suggest to replace the condition in the for-loop with "i < 30000" or > even "i < 60000", just to be sure. Or use a g_timer and look at elapsed time rather than having open coded loops? > >> + return false; >> +} >> + >> +static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in, >> + char *out) >> +{ >> + int i, in_len = strlen(in); >> + >> + g_assert(write(sock_fd, in, in_len) == in_len); > > Sorry for being pedantic, but I'd really prefer to write it without > side-effects in g_assert() please. (same for the other occurrences in > this patch) +1 -- Alex Bennée
diff --git a/tests/microbit-test.c b/tests/microbit-test.c index afeb6b082a..3da6d9529f 100644 --- a/tests/microbit-test.c +++ b/tests/microbit-test.c @@ -19,10 +19,93 @@ #include "libqtest.h" #include "hw/arm/nrf51.h" +#include "hw/char/nrf51_uart.h" #include "hw/gpio/nrf51_gpio.h" #include "hw/timer/nrf51_timer.h" #include "hw/i2c/microbit_i2c.h" +#include <sys/socket.h> +#include <sys/un.h> + +static bool uart_wait_for_event(QTestState *qts, uint32_t event_addr) +{ + int i; + + for (i = 0; i < 1000; i++) { + if (qtest_readl(qts, event_addr) == 1) { + qtest_writel(qts, event_addr, 0x00); + return true; + } + g_usleep(10000); + } + + return false; +} + +static void uart_rw_to_rxd(QTestState *qts, int sock_fd, const char *in, + char *out) +{ + int i, in_len = strlen(in); + + g_assert(write(sock_fd, in, in_len) == in_len); + for (i = 0; i < in_len; i++) { + g_assert(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_RXDRDY)); + out[i] = qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD); + } + out[i] = '\0'; +} + +static void uart_w_to_txd(QTestState *qts, const char *in) +{ + int i, in_len = strlen(in); + + for (i = 0; i < in_len; i++) { + qtest_writel(qts, NRF51_UART_BASE + A_UART_TXD, in[i]); + g_assert(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_TXDRDY)); + } +} + +static void test_nrf51_uart(void) +{ + int sock_fd; + char s[10]; + QTestState *qts = qtest_init_with_serial("-M microbit", &sock_fd); + + g_assert(write(sock_fd, "c", 1) == 1); + g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD) == 0); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_ENABLE, 0x04); + qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTRX, 0x01); + + g_assert(uart_wait_for_event(qts, NRF51_UART_BASE + A_UART_RXDRDY)); + qtest_writel(qts, NRF51_UART_BASE + A_UART_RXDRDY, 0x00); + g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_RXD) == 'c'); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_INTENSET, 0x04); + g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_INTEN) == 0x04); + qtest_writel(qts, NRF51_UART_BASE + A_UART_INTENCLR, 0x04); + g_assert(qtest_readl(qts, NRF51_UART_BASE + A_UART_INTEN) == 0x00); + + uart_rw_to_rxd(qts, sock_fd, "hello", s); + g_assert(memcmp(s, "hello", 5) == 0); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTTX, 0x01); + uart_w_to_txd(qts, "d"); + g_assert(read(sock_fd, s, 10) == 1); + g_assert(s[0] == 'd'); + + qtest_writel(qts, NRF51_UART_BASE + A_UART_SUSPEND, 0x01); + qtest_writel(qts, NRF51_UART_BASE + A_UART_TXD, 'h'); + qtest_writel(qts, NRF51_UART_BASE + A_UART_STARTTX, 0x01); + uart_w_to_txd(qts, "world"); + g_assert(read(sock_fd, s, 10) == 5); + g_assert(memcmp(s, "world", 5) == 0); + + close(sock_fd); + + qtest_quit(qts); +} + /* Read a byte from I2C device at @addr from register @reg */ static uint32_t i2c_read_byte(QTestState *qts, uint32_t addr, uint32_t reg) { @@ -302,6 +385,7 @@ int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); + qtest_add_func("/microbit/nrf51/uart", test_nrf51_uart); qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio); qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer); qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
Some functional tests for: Basic reception/transmittion Suspending INTEN* registers Signed-off-by: Julia Suvorova <jusual@mail.ru> --- tests/microbit-test.c | 84 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)