tests/periph_gpio: added benchmark capabilities by haukepetersen · Pull Request #8040 · RIOT-OS/RIOT

haukepetersen · 2017-11-15T13:26:09Z

alternative to #7478

Instead of adding a dedicated benchmark application this PR simply adds the same functionality as shell command to the existing periph_gpio test application (as done some time ago also for the periph_spi test). For convenience and potentially more advanced improvements I added also a make bench target using pexpect. This way one can (semi-) automatically run the benchmark on any platform.

I know there is a 1000 things that can be build upon this (and possible also improved), but I'd like to get this in in the current state for now and we can always improve this later. But it is quite useful already and I prefer this solution over #7478.

kaspar030 · 2017-12-19T19:58:42Z

tests/periph_gpio/main.c

 #include "periph/gpio.h"
+#include "xtimer.h"
+
+#define BENCH_RUNS_DEFAULT      (1000 * 1000UL)


I think this needs to be 1000UL * 1000

kaspar030 · 2017-12-19T20:00:37Z

tests/periph_gpio/main.c

+    gpio_t pin = GPIO_PIN(atoi(argv[1]), atoi(argv[2]));
+    unsigned long runs = BENCH_RUNS_DEFAULT;
+    if (argc > 3) {
+        runs = (unsigned long)atoi(argv[3]);


The return value needs to be checked (for >0). Otherwise, later on, this can lead to div by zero. (atio() returns 0 for non-number strings...)

And maybe use atol() or atoll() so 16bitter can do more than 2^15?

kaspar030 · 2017-12-19T20:02:37Z

tests/periph_gpio/main.c

+    unsigned state = irq_disable();
+    time = xtimer_now_usec();
+    for (unsigned long i = 0; i < runs; i++) {
+        __asm("nop");


__asm__, and maybe add volatile?

kaspar030 · 2017-12-19T20:04:22Z

tests/periph_gpio/main.c

+    irq_restore(state);
+    bench_print_time(time, runs, "gpio_read");
+
+


remove extra spaces

kaspar030 · 2017-12-19T20:05:05Z

tests/periph_gpio/main.c

+    state = irq_disable();
+    time = xtimer_now_usec();
+    for (unsigned long i = 0; i < runs; i++) {
+        int tmp = gpio_read(pin);


why save the return and then void it? does it prevent optimizing the function call away?

kaspar030 · 2017-12-19T20:07:51Z

tests/periph_gpio/main.c

+    bench_print_time(time, runs, "gpio_write");
+
+    /* gpio_read */
+    state = irq_disable();


would it, in this case, make sense to create a BENCHMARK(name, runs, command) macro?

haukepetersen · 2017-12-20T10:35:06Z

Addressed comments and introduced an independent benchmark module for running this type of trivial function timing benchmarks. This makes the code much nicer to read, indeed.

Making use of the benchmak modules in other tests (e.g. periph_spi) is open for follow up PRs...

jnohlgard · 2017-12-20T11:48:18Z

I have been thinking about something similar to this benchmark module using the Cortex-M systick timer to measure actual cycles. However, that would be a platform specific hack if I ever get around to implementing it. It could be useful to be able to measure the overhead in ISRs or other low level functionality.

haukepetersen · 2017-12-20T14:37:51Z

I think for the Cortex tick counter, it would make sense to write a (sub) module to the cortexm_common code, which allows to use this capability in an easy way. This would indeed be very useful thing!

haukepetersen · 2018-01-23T11:08:25Z

rebased.

@kaspar030 what do you say?

kaspar030

I think the benchmark print function needs to use stdint types. Other than that, looks good. will do a test run now.

kaspar030 · 2018-01-27T11:19:37Z

sys/benchmark/benchmark.c

+    uint32_t div  = (time - (full * runs)) / (runs / 1000);
+
+    printf("%11s: %9luus  ---  %2u.%03uus per call\n", name,
+           (unsigned long) time, (unsigned)full, (unsigned)div);


better use PRIu32 et al ...

kaspar030 · 2018-01-27T11:20:55Z

sys/benchmark/benchmark.c

+ * @}
+ */
+
+#include <stdio.h>


here and below: usually we put a new line after system headers

I usually do this, too, but in cases where there is only a small (roughly up to 2 or 3 ) includes in a file I tend to put them in a single block, as I think its quite ugly to have a single include per 'newline separated block'... But will fix this here to make you happy :-)

kaspar030 · 2018-01-27T11:50:37Z

Tested, works flawlessly. Feel free to directly squash the printf fixes.

haukepetersen · 2018-01-29T07:49:47Z

rebased, squashed, and fixed style and printf issues pointed out above.

kaspar030

ACK.

haukepetersen · 2018-01-29T13:02:12Z

triggered Mudock (forgot earlier...)

haukepetersen · 2018-01-29T13:09:22Z

fixed style issues in the bench python script (as pointed out by the flake8 check). Bus something is broken now when running it, looking into it now.

haukepetersen · 2018-01-29T13:12:43Z

fixed, should be all good now.

PeterKietzmann · 2018-01-31T12:20:21Z

nope, still not good:
tests/periph_gpio/main.c:221: style (unsignedLessThanZero): Checking if unsigned variable 'runs' is less than zero.

haukepetersen · 2018-01-31T12:52:30Z

ups, missed that one.

This commit enables the GPIO benchmark to be run automatically (through 'make bench') using pexpect.

haukepetersen · 2018-01-31T12:56:07Z

next try (squashed fix right in)

kaspar030 · 2018-02-19T08:42:49Z

Let's go!

kaspar030 · 2018-02-19T08:43:34Z

I'll give travis another chance. Different cppcheck versions?

kaspar030 · 2018-02-19T08:48:34Z

I'll give travis another chance. Different cppcheck versions?

Yeah, and it complains about timex.h, thus unrelated.

haukepetersen assigned kaspar030 and smlng Nov 15, 2017

haukepetersen added Area: drivers Area: Device drivers Area: tests Area: tests and testing framework labels Nov 15, 2017

haukepetersen mentioned this pull request Nov 15, 2017

tests: added benchmark for periph_gpio driver #7478

Closed

kaspar030 requested changes Dec 19, 2017

View reviewed changes

haukepetersen force-pushed the add_bench_periphgpiototest branch from 4abcba6 to d823ff1 Compare December 20, 2017 10:33

haukepetersen force-pushed the add_bench_periphgpiototest branch from d823ff1 to 2bfdade Compare January 23, 2018 11:08

kaspar030 requested changes Jan 27, 2018

View reviewed changes

kaspar030 mentioned this pull request Jan 27, 2018

cpu/stm32_common: GPIO functions check for GPIO_UNDEF and IRQ #8462

Closed

haukepetersen force-pushed the add_bench_periphgpiototest branch from 2bfdade to 0262530 Compare January 29, 2018 07:49

kaspar030 approved these changes Jan 29, 2018

View reviewed changes

haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 29, 2018

haukepetersen force-pushed the add_bench_periphgpiototest branch from 0262530 to 563002a Compare January 29, 2018 13:07

haukepetersen force-pushed the add_bench_periphgpiototest branch from 563002a to 542b4a9 Compare January 29, 2018 13:12

haukepetersen added 3 commits January 31, 2018 13:55

sys: added simple benchmark module

55629e0

tests/periph_gpio: added benchmark command

94796ef

tests/periph_gpio: added 'bench' make target

9b557d2

This commit enables the GPIO benchmark to be run automatically (through 'make bench') using pexpect.

haukepetersen force-pushed the add_bench_periphgpiototest branch from 542b4a9 to 9b557d2 Compare January 31, 2018 12:55

kaspar030 merged commit f59e824 into RIOT-OS:master Feb 19, 2018

haukepetersen deleted the add_bench_periphgpiototest branch March 6, 2018 10:12

cladmi mentioned this pull request Jun 18, 2019

xtimer: xtimer_now() not ISR safe #5338

Closed

		irq_restore(state);
		bench_print_time(time, runs, "gpio_read");

Conversation

haukepetersen commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Dec 20, 2017

Uh oh!

jnohlgard commented Dec 20, 2017

Uh oh!

haukepetersen commented Dec 20, 2017

Uh oh!

haukepetersen commented Jan 23, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Jan 27, 2018

Uh oh!

haukepetersen commented Jan 29, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jan 29, 2018

Uh oh!

haukepetersen commented Jan 29, 2018

Uh oh!

haukepetersen commented Jan 29, 2018

Uh oh!

PeterKietzmann commented Jan 31, 2018

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

haukepetersen commented Jan 31, 2018

Uh oh!

kaspar030 commented Feb 19, 2018

Uh oh!

kaspar030 commented Feb 19, 2018

Uh oh!

kaspar030 commented Feb 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants