tests/xtimer_usleep: improved test, added pin toggle #8865

Josar · 2018-04-02T22:52:42Z

Added ERROR_US 500 to emphasize when errors occur.
Added SLEEP_PIN and to probe sleep times at SLEEP_GPIO_PIN.

Atmega mcu will "hang" when the main ends, which will not matter here but should be addressed, imho.

kYc0o · 2018-04-03T12:59:00Z

Can you explain more the purpose of this PR? I acknowledge for increasing the test cases, but I don't understand the need of the sleep pin.

Josar · 2018-04-03T14:26:59Z

You can use an oscilloscope to see if the timer runs as expected.
Or a seleae logic or other hardware, for hardware in the loop tests.

kYc0o · 2018-04-03T14:36:17Z

Well the line you removed // getchar(); is actually for that, it waits the test program to be launched to perform the checking through UART, so a pin is not needed and can be automatically tested. Have you checked the python script test under tests/?

Josar · 2018-04-03T15:42:39Z

Ok, so i run make BOARD=jiminy-mega256rfr2 flash test

Still i think measuring with a scope could come handy when debuging.

kYc0o · 2018-04-03T16:46:42Z

Ok, so i run make BOARD=jiminy-mega256rfr2 flash test

Yes, that's enough to run the test.

Still i think measuring with a scope could come handy when debuging.

Yes, however for the general use-case of the test it comes less handy, and even being optional, I think adding more code to the test may not help that much. However, if others have find it useful, I won't object.

@miri64 @kaspar030 do you have an opinion on this?

kYc0o

Several code and style errors found. I'll run murdock to see if it passes the compile tests.

kYc0o · 2018-04-03T16:48:40Z

tests/xtimer_usleep/README.md

+ and by providing capabilities to compare against an external timer.
+
+The sleep times can be probed at a pin if SLEEP_PIN` is set to 1 and the respective
+gpio pin is define as `SLEEP_GPIO_PIN`.


You missed a ` before SLEEP_PIN. You might give more hints on how to use it, e.g. add that it can be probed with an scope or something like that.

kYc0o · 2018-04-03T16:49:27Z

tests/xtimer_usleep/main.c

-    puts("Please hit any key and then ENTER to continue");
-    getchar();
+    puts("Please hit any key and then ENTER to continue\n");
+    // getchar();


Please uncomment this line, it's needed for the automated test.

kYc0o · 2018-04-03T16:50:00Z

tests/xtimer_usleep/main.c


    printf("Running test %u times with %u distinct sleep times\n", RUNS,
           (unsigned)SLEEP_TIMES_NUMOF);
-    puts("Please hit any key and then ENTER to continue");
-    getchar();
+    puts("Please hit any key and then ENTER to continue\n");


puts() doesn't need any \n. Please remove.

kYc0o · 2018-04-03T16:51:11Z

tests/xtimer_usleep/main.c

-            uint32_t diff, start;
-            start = xtimer_now_usec();
+
+            start_sleep = xtimer_now_usec();


Change the scope of your variables, they might be defined only in the context they're used.

kYc0o · 2018-04-03T16:51:44Z

tests/xtimer_usleep/main.c

        }
    }
    testtime = xtimer_now_usec() - start_test;
-    printf("Test ran for %" PRIu32 " us\n", testtime);
+    printf("\nTest ran for %" PRIu32 " us\n", testtime);


No need to add another \n.

kYc0o · 2018-04-03T16:52:22Z

tests/xtimer_usleep/main.c


+    /* atmega mcu will hang if main ends */
+    while(1){}


This is another issue. This test is intended to run on any platform, and this is not needed.

kYc0o · 2018-04-03T16:57:12Z

tests/xtimer_usleep/main.c

+            }else{
+                printf("Slept for %" PRIu32 " us (expected: %" PRIu32 " us)\n",
+                                   diff, sleep_times[n]);
+           }


Your if - else statements are very off on spacing, please stick to coding conventions.

kYc0o · 2018-04-03T16:57:30Z

tests/xtimer_usleep/main.c


+#define ERROR_US 500


Errors of more than 500 microseconds are way too big IMHO. xtimer_usleep() is used for very short periods of time. If your hardware doesn't support it, it must be blacklisted for this test (e.g. running a very low speed timer).

kYc0o · 2018-04-03T17:00:51Z

tests/xtimer_usleep/main.c

+    int32_t err;
+
+#if SLEEP_PIN
+    gpio_init( SLEEP_GPIO_PIN, GPIO_OUT );


Please remove the spaces after and before the parenthesis.

kYc0o · 2018-04-03T17:03:09Z

tests/xtimer_usleep/main.c

+#if SLEEP_PIN
+#include "board.h"
+#include "periph/gpio.h"
+#define SLEEP_GPIO_PIN  GPIO_PIN(PORT_F, 7)


This should be surrounded by an #ifndef and the pin number should be passed from the outside (Makefile or command line) if you want to use it in this way. Hardcoded GPIO might lead to failures e.g. while flashing on certain devices. The default pin might be GPIO_UNDEF, but still I'm not convinced this is a generic way to perform a test like this.

miri64 · 2018-04-03T18:14:13Z

@Josar please provide fixup commits until the review says it's you can squash, so it is easier to track your changes.

miri64 · 2018-04-03T18:15:52Z

Also, please specify in your commit message, what you did to the readme and how the tests were improved. A good style is to use an imperative style:

tests/xtimer_usleep: Update README to describe foo
tests/xtimer_usleep: Add test cases to consider bar

kaspar030 · 2018-04-04T08:37:37Z

tests/xtimer_usleep/main.c

@@ -24,16 +26,33 @@

 #include "xtimer.h"
 #include "timex.h"
+#include "stdlib.h"


this is a standard header, so include <stdlib.h> and put above our headers.

kaspar030 · 2018-04-04T08:39:41Z

tests/xtimer_usleep/main.c

+
+            diff = xtimer_now_usec() - start_sleep;
+
+            if(sleep_times[n] < diff - ERROR_US


this if needs some cleanup. parentheses, spaces, else on new line, ...

kYc0o

I found some more details.

Please look to the coding conventions to address Kaspar comments.

kYc0o · 2018-04-04T12:01:25Z

tests/xtimer_usleep/README.md

+and the respective gpio `SLEEP_PORT` is defined in the makefile.
+
+CFLAGS += SLEEP_PIN=7
+CFLAGS += SLEEP_PORT=PORTF


The doc and the Makefile are not consistent on this.

kYc0o · 2018-04-04T12:01:55Z

tests/xtimer_usleep/README.md


 ## Usage
+Executed from the project directory
+```
+make BOARD=<BoardName> flash test


Please avoid CamelCase.

kYc0o · 2018-04-04T12:02:23Z

tests/xtimer_usleep/README.md

+```
+### On Error with terminal
+```
+


Why is this empty?

Josar · 2018-04-12T21:34:25Z

@kaspar030 some further comments?

Josar · 2018-04-25T17:08:25Z

@kYc0o squashed and rebased. Also changed the port initialization. Could you please test it again. 😅
@kaspar030 could you have a second look and approve?

Josar · 2018-05-08T20:29:37Z

@kaspar030 any further thoughts?

kYc0o

Minor change to activate CI HIL tests.

kYc0o · 2018-05-09T16:44:35Z

tests/xtimer_usleep/Makefile

+#FEATURES_REQUIRED += periph_gpio
+#CFLAGS += -DSLEEP_PIN=7
+#CFLAGS += -DSLEEP_PORT=PORT_F
+


Can you add just here TEST_ON_CI_WHITELIST += all

Josar · 2018-05-14T20:47:10Z

@kYc0o @kaspar030 further requests?

kYc0o · 2018-05-14T21:40:50Z

Everything goof for me. HIL tests passed.

Josar · 2018-05-23T20:30:54Z

@kaspar030 any further requests?

kYc0o · 2018-05-29T13:59:18Z

@Josar apparently you need to rebase this one to solve the conflicts.

kYc0o · 2018-05-31T13:43:50Z

Great! It's green now. @kaspar030 can you confirm if your concerns were addressed to move on here?

kYc0o · 2018-05-31T17:38:45Z

@cladmi if you want to test it would be awesome!

cladmi · 2018-06-01T10:05:39Z

Test run correctly on IoT-LAB nodes:

iotlab-m3: offset 14 us
iotlab-a8-m3: offset 14 us
wsn430-v1_3b: offset 135 us
arduino-zero (samd21 cpu): offset 50us
samr21-xpro: offset 44us

kYc0o · 2018-06-04T13:22:44Z

@kaspar030 can you check if your concerns were addressed? This PR has 2 ACKs and I think we can merge it ASAP.

kaspar030

(untested) ACK.

Josar force-pushed the xtimer_usleep_improved branch 2 times, most recently from ec463fc to 6b3a559 Compare April 3, 2018 09:44

kYc0o self-assigned this Apr 3, 2018

kYc0o added Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Apr 3, 2018

kYc0o requested changes Apr 3, 2018

View reviewed changes

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 3, 2018

Josar force-pushed the xtimer_usleep_improved branch 2 times, most recently from 87cf471 to 3a65d23 Compare April 3, 2018 18:09

Josar force-pushed the xtimer_usleep_improved branch 9 times, most recently from 4a36c84 to 780f6a1 Compare April 3, 2018 23:25

kaspar030 requested changes Apr 4, 2018

View reviewed changes

kYc0o requested changes Apr 4, 2018

View reviewed changes

Josar force-pushed the xtimer_usleep_improved branch 3 times, most recently from bdbc326 to c812f9c Compare April 5, 2018 16:20

Josar force-pushed the xtimer_usleep_improved branch from c812f9c to 5417b7d Compare April 25, 2018 17:01

kYc0o requested changes May 9, 2018

View reviewed changes

kYc0o approved these changes May 14, 2018

View reviewed changes

Josar mentioned this pull request May 15, 2018

xtimer_core: reduce idle time #8990

Closed

Josar mentioned this pull request May 28, 2018

xtimer: timer & target overflow, hang resolved. #9211

Merged

tests/xtimer_usleep: Add SLEEP_PIN, Updated README

ffb8c27

Josar force-pushed the xtimer_usleep_improved branch from 0a25273 to ffb8c27 Compare May 29, 2018 14:11

kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 30, 2018

cladmi approved these changes Jun 1, 2018

View reviewed changes

kaspar030 approved these changes Jun 4, 2018

View reviewed changes

kaspar030 merged commit dbc70f3 into RIOT-OS:master Jun 4, 2018

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Josar deleted the xtimer_usleep_improved branch July 10, 2018 13:07


		diff = xtimer_now_usec() - start_sleep;

		if(sleep_times[n] < diff - ERROR_US

tests/xtimer_usleep: improved test, added pin toggle #8865

tests/xtimer_usleep: improved test, added pin toggle #8865

Conversation

Josar commented Apr 2, 2018 • edited Loading

kYc0o commented Apr 3, 2018 • edited Loading

Josar commented Apr 3, 2018 • edited Loading

kYc0o commented Apr 3, 2018 • edited Loading

Josar commented Apr 3, 2018 • edited Loading

kYc0o commented Apr 3, 2018

kYc0o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Apr 3, 2018

miri64 commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kYc0o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented Apr 12, 2018

Josar commented Apr 25, 2018

Josar commented May 8, 2018

kYc0o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josar commented May 14, 2018

kYc0o commented May 14, 2018

Josar commented May 23, 2018

kYc0o commented May 29, 2018

kYc0o commented May 31, 2018

kYc0o commented May 31, 2018

cladmi commented Jun 1, 2018

kYc0o commented Jun 4, 2018

kaspar030 left a comment

Choose a reason for hiding this comment

Josar commented Apr 2, 2018 •

edited

Loading

kYc0o commented Apr 3, 2018 •

edited

Loading

Josar commented Apr 3, 2018 •

edited

Loading

kYc0o commented Apr 3, 2018 •

edited

Loading

Josar commented Apr 3, 2018 •

edited

Loading