Re: [Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality
Thomas Huth 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
>> ---
>> 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
>> +#include
>> +
>> +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(1);
>> +}
>
> 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 < 3" or
> even "i < 6", 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
Re: [Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality
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
> ---
> 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
> +#include
> +
> +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(1);
> +}
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 < 3" or
even "i < 6", 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
Re: [Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality
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 > +#include ./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! signature.asc Description: PGP signature
[Qemu-devel] [PATCH v4 3/3] tests/microbit-test: Check nRF51 UART functionality
Some functional tests for:
Basic reception/transmittion
Suspending
INTEN* registers
Signed-off-by: Julia Suvorova
---
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
+#include
+
+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(1);
+}
+
+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", _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(, , 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);
--
2.17.1
