tests: New test for xtimer_now with interrupts disabled #13025

JulianHolzwarth · 2020-01-03T17:01:41Z

Contribution description

This is a new test for xtimer. It tests if xtimer now (xtimer_now_usec) returns the correct value when interrupts are disabled.

This test is for: #9530

Testing procedure

BOARD=* make -C tests/xtimer_now_irq/ flash term replace * with board
If it returns an Error it is not correct.

Issues/PRs references

#9530

MichelRottleuthner · 2020-01-03T17:04:24Z

tests/xtimer_now_irq/Makefile.ci

@@ -0,0 +1,10 @@
+BOARD_INSUFFICIENT_MEMORY := \


we should re-check this because the test is actually rather small

I checked the list and it seems to fit all these boards just fine - so why are they listed as insufficient memory?

I will remove it.

aabadie

I think the test can be improved and a Python test script should be added to allow automatic testing. The suggestions below should help with the script.

tests/xtimer_now_irq/main.c

fjmolinas · 2020-01-06T09:53:39Z

ping @aabadie!

aabadie

I found new problems with the current version.

tests/xtimer_now_irq/main.c

tests/xtimer_now_irq/tests/01-run.py

tests/xtimer_now_irq/main.c

JulianHolzwarth · 2020-01-06T15:36:51Z

@aabadie @MichelRottleuthner
Thank you for your comments.
A pushed a new commit. What do you think now?

MichelRottleuthner

The "interpreting the XTIMER_MASK as usecs doesn't make sense" isn't addressed yet

tests/xtimer_now_irq/README.md

JulianHolzwarth · 2020-01-22T15:55:14Z

@MichelRottleuthner I changed the README last week. What do you think now?

MichelRottleuthner · 2020-02-07T13:18:50Z

Apart from the INSUFFICIENT_MEMORY list in Makefile.ci I think this is good to go. @aabadie are all your requests addressed?

tests/xtimer_now_irq/tests/01-run.py

MichelRottleuthner · 2020-02-25T20:19:41Z

@aabadie how bout now?

kaspar030 · 2020-02-26T09:04:11Z

The pinetime errors are unrelated, fix for those is here: #13481

kaspar030

As is, this will be run on the CI, but always timeout. CI jobs time out after 5 minutes.
I suggest to CI blacklist this test (until there's a board with a reasonable bit width for xtimer, e.g., at most 24).

kaspar030 · 2020-02-26T09:13:01Z

This also needs some squashing.

kaspar030 · 2020-02-26T09:20:44Z

tests/xtimer_now_irq/main.c

+    while (count) {
+        unsigned int state = irq_disable();
+        uint32_t t1 = xtimer_now_usec();
+        xtimer_spin(xtimer_ticks((uint32_t)(~XTIMER_MASK) / 2));


On 32bit 1mhz, this will spin (0xffffffff / 2) ticks. Doesn't that make the second iteration always fail?

xtimer_now() is somewhere a little higher than zero, so is t1

spin half period

t2 is now somewhere a little higher than a half period

next loop iteration, t1 is now close to t2 (a little higher than a half period)

spin half period

t2 32bit xtimer_now_usec() overflows ("a little higher than a half period" + "half period" > full period)

t2 < t1, error triggers

What am I missing?

Running it as is for 32 bit timer is madness anyway and we need a better way for testing these corner cases with a bit of mock magic.
Catching the "missed overflow" of the 32 bit timer with the 32 bit xtimer interface obviously doesn't make sense, the 64 bit variant is needed for that ;)

Alright, how about straight passing if XTIMER_WIDTH == 32?

Ahh just passing it would feel like lying. Why not just replace the uint32_t t1 = xtimer_now_usec(); etc. with the 64 bit variants? Then it works for 32 bit too.
Hmm I guess the problem is then we have no way to CI-run it for 16 bit timers and keep it working for manual 32 bit testing?.
How about an additional flag for manual running of 32 bit + some terminal output warning about 32 bit not being checked without that flag?

Ahh just passing it would feel like lying. Why not just replace the uint32_t t1 = xtimer_now_usec(); etc. with the 64 bit variants? Then it works for 32 bit too.

yyyyyes, but isn't that testing something different then?

How about an additional flag for manual running of 32 bit + some terminal output warning about 32 bit not being checked without that flag?

As long as by default on 32bit on CI, this just prints "not testing anything" "OK", so CI just passes. Could also blacklist.

As is it shouldn't run at all, on 32bit, just so noone actually waits 2**32usec for something to happen. 😉

yyyyyes, but isn't that testing something different then?

Didn't test yet, but no not really

As long as by default on 32bit on CI, this just prints "not testing anything" "OK", so CI just passes

I'd prefer some "not testing anything" "OK" over blacklisting.

just so noone actually waits 2**32usec

I definitely had cases when testing timer stuff that made me desperate enough too run such long running tests^^

tests/xtimer_now_irq/README.md

tests/xtimer_now_irq/tests/01-run.py

JulianHolzwarth · 2020-02-26T17:09:57Z

@MichelRottleuthner @kaspar030
Should the test only support <24 bits timer?

JulianHolzwarth · 2020-02-26T17:13:28Z

why is this automatically:

JulianHolzwarth requested review from fjmolinas, leandrolanzieri, miri64 and smlng as code owners

Because of TEST_ON_CI_BLACKLIST += all ?

MichelRottleuthner · 2020-02-26T17:24:32Z

Should the test only support <24 bits timer?

If anything then <= 24, but actually 32 bit would still be possible when using 64 bit API. In any case 32 Bit must be optional (i.e. only for manual testing). I'd also vote against blacklisting it for all boards - why not CI-run it for 16 and 24 bit?

why is this automatically:

take a look at https://github.com/RIOT-OS/RIOT/blob/master/CODEOWNERS

JulianHolzwarth · 2020-02-26T17:34:37Z

@MichelRottleuthner

If anything then <= 24, but actually 32 bit would still be possible when using 64 bit API. In any case 32 Bit must be optional (i.e. only for manual testing). I'd also vote against blacklisting it for all boards - why not CI-run it for 16 and 24 bit?

I thought @kaspar030 wanted it this way. (only <24 bit and blacklist all)

I suggest to CI blacklist this test

As is it shouldn't run at all, on 32bit, just so noone actually waits 2**32usec for something to happen. wink

Have i misunderstood something?

JulianHolzwarth · 2020-03-05T16:38:05Z

@MichelRottleuthner @kaspar030 squashed and all comments addressed.

MichelRottleuthner · 2020-04-07T11:20:57Z

ping @kaspar030 all good now?

This test checks, if the timer returns the correct time (xtimer_now_usec() is called), when interrupts are disabled. Specifically tested is if the time is correct after a low-level timer overflow.

JulianHolzwarth · 2020-05-01T13:40:50Z

new rebase (no changes)

JulianHolzwarth · 2020-05-01T13:41:41Z

@kaspar030 Can we merge?

MichelRottleuthner · 2020-05-05T10:01:29Z

@kaspar030 would you mind removing your block on this one? All your requests should be addressed.

comments addressed

MichelRottleuthner self-requested a review January 3, 2020 17:02

MichelRottleuthner self-assigned this Jan 3, 2020

MichelRottleuthner added Area: tests Area: tests and testing framework Area: timers Area: timer subsystems Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jan 3, 2020

MichelRottleuthner reviewed Jan 3, 2020

View reviewed changes

aabadie requested changes Jan 3, 2020

View reviewed changes

tests/xtimer_now_irq/main.c Outdated Show resolved Hide resolved

tests/xtimer_now_irq/main.c Outdated Show resolved Hide resolved

aabadie reviewed Jan 3, 2020

View reviewed changes

tests/xtimer_now_irq/main.c Outdated Show resolved Hide resolved

fjmolinas mentioned this pull request Jan 6, 2020

sys: xtimer concurrency/robustness improvement #9530

Merged

aabadie requested changes Jan 6, 2020

View reviewed changes

MichelRottleuthner requested changes Jan 6, 2020

View reviewed changes

tests/xtimer_now_irq/README.md Outdated Show resolved Hide resolved

tests/xtimer_now_irq/README.md Outdated Show resolved Hide resolved

MichelRottleuthner mentioned this pull request Jan 22, 2020

xtimer: xtimer_now() not ISR safe #5338

Closed

aabadie reviewed Feb 7, 2020

View reviewed changes

tests/xtimer_now_irq/tests/01-run.py Outdated Show resolved Hide resolved

aabadie approved these changes Feb 25, 2020

View reviewed changes

MichelRottleuthner approved these changes Feb 25, 2020

View reviewed changes

MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 25, 2020

benpicco 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 Feb 26, 2020

kaspar030 requested changes Feb 26, 2020

View reviewed changes

kaspar030 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 26, 2020

kaspar030 previously requested changes Feb 26, 2020

View reviewed changes

bergzand 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 Feb 26, 2020

kaspar030 reviewed Feb 26, 2020

View reviewed changes

tests/xtimer_now_irq/README.md Outdated Show resolved Hide resolved

kaspar030 reviewed Feb 26, 2020

View reviewed changes

tests/xtimer_now_irq/tests/01-run.py Outdated Show resolved Hide resolved

JulianHolzwarth requested review from fjmolinas, leandrolanzieri, miri64 and smlng as code owners February 26, 2020 17:11

JulianHolzwarth force-pushed the pr/xtimer_test_overflow branch from dbbfdb7 to 7db72ee Compare March 5, 2020 16:37

tests/xtimer_now_irq/: New test for xtimer_now() with irq_disable()

921d9cf

This test checks, if the timer returns the correct time (xtimer_now_usec() is called), when interrupts are disabled. Specifically tested is if the time is correct after a low-level timer overflow.

JulianHolzwarth force-pushed the pr/xtimer_test_overflow branch from 7db72ee to 921d9cf Compare May 1, 2020 13:40

kaspar030 merged commit d3e18c0 into RIOT-OS:master May 5, 2020

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

JulianHolzwarth deleted the pr/xtimer_test_overflow branch May 8, 2024 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: New test for xtimer_now with interrupts disabled #13025

tests: New test for xtimer_now with interrupts disabled #13025

JulianHolzwarth commented Jan 3, 2020 •

edited

Loading

MichelRottleuthner Jan 3, 2020

MichelRottleuthner Feb 7, 2020

JulianHolzwarth Feb 14, 2020

aabadie left a comment

fjmolinas commented Jan 6, 2020

aabadie left a comment

JulianHolzwarth commented Jan 6, 2020

MichelRottleuthner left a comment

JulianHolzwarth commented Jan 22, 2020

MichelRottleuthner commented Feb 7, 2020

MichelRottleuthner commented Feb 25, 2020

kaspar030 commented Feb 26, 2020

kaspar030 left a comment

kaspar030 commented Feb 26, 2020

kaspar030 Feb 26, 2020 •

edited

Loading

MichelRottleuthner Feb 26, 2020 •

edited

Loading

kaspar030 Feb 26, 2020

MichelRottleuthner Feb 26, 2020

kaspar030 Feb 26, 2020

MichelRottleuthner Feb 26, 2020

JulianHolzwarth commented Feb 26, 2020

JulianHolzwarth commented Feb 26, 2020 •

edited

Loading

MichelRottleuthner commented Feb 26, 2020 •

edited

Loading

JulianHolzwarth commented Feb 26, 2020 •

edited

Loading

JulianHolzwarth commented Mar 5, 2020 •

edited

Loading

MichelRottleuthner commented Apr 7, 2020

JulianHolzwarth commented May 1, 2020

JulianHolzwarth commented May 1, 2020

MichelRottleuthner commented May 5, 2020

tests: New test for xtimer_now with interrupts disabled #13025

tests: New test for xtimer_now with interrupts disabled #13025

Conversation

JulianHolzwarth commented Jan 3, 2020 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

MichelRottleuthner Jan 3, 2020

Choose a reason for hiding this comment

MichelRottleuthner Feb 7, 2020

Choose a reason for hiding this comment

JulianHolzwarth Feb 14, 2020

Choose a reason for hiding this comment

aabadie left a comment

Choose a reason for hiding this comment

fjmolinas commented Jan 6, 2020

aabadie left a comment

Choose a reason for hiding this comment

JulianHolzwarth commented Jan 6, 2020

MichelRottleuthner left a comment

Choose a reason for hiding this comment

JulianHolzwarth commented Jan 22, 2020

MichelRottleuthner commented Feb 7, 2020

MichelRottleuthner commented Feb 25, 2020

kaspar030 commented Feb 26, 2020

kaspar030 left a comment

Choose a reason for hiding this comment

kaspar030 commented Feb 26, 2020

kaspar030 Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

MichelRottleuthner Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

kaspar030 Feb 26, 2020

Choose a reason for hiding this comment

MichelRottleuthner Feb 26, 2020

Choose a reason for hiding this comment

kaspar030 Feb 26, 2020

Choose a reason for hiding this comment

MichelRottleuthner Feb 26, 2020

Choose a reason for hiding this comment

JulianHolzwarth commented Feb 26, 2020

JulianHolzwarth commented Feb 26, 2020 • edited Loading

MichelRottleuthner commented Feb 26, 2020 • edited Loading

JulianHolzwarth commented Feb 26, 2020 • edited Loading

JulianHolzwarth commented Mar 5, 2020 • edited Loading

MichelRottleuthner commented Apr 7, 2020

JulianHolzwarth commented May 1, 2020

JulianHolzwarth commented May 1, 2020

MichelRottleuthner commented May 5, 2020

JulianHolzwarth commented Jan 3, 2020 •

edited

Loading

kaspar030 Feb 26, 2020 •

edited

Loading

MichelRottleuthner Feb 26, 2020 •

edited

Loading

JulianHolzwarth commented Feb 26, 2020 •

edited

Loading

MichelRottleuthner commented Feb 26, 2020 •

edited

Loading

JulianHolzwarth commented Feb 26, 2020 •

edited

Loading

JulianHolzwarth commented Mar 5, 2020 •

edited

Loading