Skip to content

tests/periph_timer_short_relative_set: initial commit#12610

Merged
MichelRottleuthner merged 1 commit intoRIOT-OS:masterfrom
kaspar030:pr/add_tests/periph_timer_short_relative_set
Dec 2, 2019
Merged

tests/periph_timer_short_relative_set: initial commit#12610
MichelRottleuthner merged 1 commit intoRIOT-OS:masterfrom
kaspar030:pr/add_tests/periph_timer_short_relative_set

Conversation

@kaspar030
Copy link
Contributor

Contribution description

From the readme:

This test exposes if timer_set() with very low values (down to zero) underflows.
For each value from 100 to 0, it'll try to set a timer with that length.
In human terms, that should trigger instantly.

See this example of a timer_set() implementation:

int timer_set(tim_t dev, int channel, unsigned int timeout)                                                                    
{                                                                                                                              
    return timer_set_absolute(dev, channel, timer_read(dev) + timeout);                                                        
}                                                                                                                              

This will probably underflow if "timeout" is 0, or if an ISR interrupts
somewhere between the read and the timerSet_absolute() call.
Depending on the used frequency of the underlying timer and the time it
requires for the involved function calls, reading the timer register, doing the
addition and writing back to the register, this will also fail for higher
timeouts.

For example, as of this writing (30-Oct-19), samr21-xpro fails for values below
8, nrf52dk for values below 2.

This test will probably fail on most devices. The PR is meant to give developers a tool to improve the situation.

Testing procedure

Issues/PRs references

Found while debugging ztimer (#11874).

@kaspar030 kaspar030 added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Oct 30, 2019
Copy link
Contributor

@MichelRottleuthner MichelRottleuthner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to add this. But with the implementation of the test I see a problem for 16 bit timers and fast 32 bit timers. I don't know of any default configuration that uses very fast 32 bit timers though.
The problem is, that the wraparound will happen so fast that it looks like it is actually working even if it is not. 16 bit at 1MHz will overflow after ~65ms and I don't think pexpect will time out before that ;)
There should be some way of comparing the observed time with the one that was requested.

@kaspar030
Copy link
Contributor Author

The problem is, that the wraparound will happen so fast that it looks like it is actually working even if it is not. 16 bit at 1MHz will overflow after ~65ms and I don't think pexpect will time out before that ;)
There should be some way of comparing the observed time with the one that was requested.

Yes, good point. I added some logic to read the timer in a loop, counting upwards, comparing to a definable maximum delay.

@kaspar030
Copy link
Contributor Author

@MichelRottleuthner maybe take another look?

Copy link
Contributor

@MichelRottleuthner MichelRottleuthner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep the check should do. I tested on some boards I have lying around:

pba-d-01-kw2x iotlab-m3 nucleo-l476rg nucleo-f103rb
success Error @ 1 Error @ 1 Error @ 0

Some small doc /typo comments below.

@MichelRottleuthner MichelRottleuthner added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Nov 1, 2019
@kaspar030
Copy link
Contributor Author

@MichelRottleuthner I totally overlooked your review, sorry. All comments addressed.

@MichelRottleuthner
Copy link
Contributor

All my comments were addressed you can go ahead and squash

@MichelRottleuthner MichelRottleuthner added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Nov 22, 2019
@kaspar030 kaspar030 force-pushed the pr/add_tests/periph_timer_short_relative_set branch from 895affc to 81f113b Compare December 1, 2019 13:14
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 1, 2019
@kaspar030
Copy link
Contributor Author

All my comments were addressed you can go ahead and squash

  • rebased, squashed

Copy link
Contributor

@MichelRottleuthner MichelRottleuthner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@MichelRottleuthner MichelRottleuthner merged commit 920884d into RIOT-OS:master Dec 2, 2019
uint32_t before = timer_read(TEST_TIMER_DEV);
timer_set(TEST_TIMER_DEV, 0, interval);
while(!thread_flags_clear(1)) {
uint32_t diff = timer_read(TEST_TIMER_DEV) - before;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assumes timer bit width is equal to platform bit width, e.g. 32 bit on 32 bit platform. However this will be wrong when using 16bit timer on a 32 bit platform, for example. See #9283 and #10493.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for jumping in! Yes, this calculation doesn't give the correct value for diff. The reason it does work in this test is because testing all values can be done in the first period before an overflow happens - but with 8 bit timers even that won't work.
Seeing the need for proper diff operation again ...I think I should have a look at #10493 soonish :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants