Skip to content

Conversation

@pizi-nordic
Copy link
Contributor

@pizi-nordic pizi-nordic commented May 16, 2019

This PR updates tests to properly handle the SYS_CLOCK_TICKS_PER_SEC != 100 by taking under account effect of tick granularity in tested API. Also, as the measurement become more precise, the timing requirements were tightened.

Details are available in code and description of the commits.

Fixes: #15983
Introduces workaround for #16195

@zephyrbot
Copy link

zephyrbot commented May 16, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@aescolar aescolar added area: Kernel area: Tests Issues related to a particular existing or missing test labels May 16, 2019
@pizi-nordic pizi-nordic changed the title Handle SYS_CLOCK_TICKS_PER_SEC != 100 in tests [WIP] Handle SYS_CLOCK_TICKS_PER_SEC != 100 in tests May 16, 2019
@pizi-nordic pizi-nordic added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label May 16, 2019
@pizi-nordic pizi-nordic force-pushed the fix-15983 branch 5 times, most recently from 5213b1c to ed9adfa Compare May 17, 2019 11:26
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

This looks good to me. There's a nitpick in there about signedness choice in time values, but it's correctly implemented and local to a test, so not worth a -1.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW: you could consider k_sleep(0) instead, which is guaranteed to synchronize to the "next" tick boundary and doesn't try to sleep for a full millisecond first. Not particularly relevant here, but in situations where you want to do a bunch of tick synchronization in a tight loop on systems where the tick rate is more than 1khz it might save some time running the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good hint. Thank you. Fixed.

Copy link
Contributor Author

@pizi-nordic pizi-nordic May 20, 2019

Choose a reason for hiding this comment

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

I changed the code, ran the test and it failed.
The k_sleep(0) does not perform synchronization to tick due to:

/* wait of 0 ms is treated as a 'yield' */
if (duration == 0) {
    k_yield();
    return 0;
}

I kept k_sleep(1) in the code in order to perform synchronization.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks dangerous. Time values should almost always be signed, or else the rollover condition will produce incorrect results for time values that are "near" each other (i.e. "just a few cycles ago" can look like a delta of 2^32). Alternatively: making time unsigned means that you can never use a ">"or "<" operator on the resulting value (to mean "later" or "before"), and that's dangerous..

Stated a third way: unsigned times don't have clean definitons for "later" or "before" and you need to know a-priori which time is the one that comes later. That's indeed the way it works here in your new code (i.e. it's computing a delta and assuming that current k_cycle_get_32() is after the argument), and a quick audit of my own seems to show that there aren't any other legacy uses of that elapsed_since value. So this is all local to the test and correct. Still: I strongly recommend never using an unsigned value for a time.

Changing it to a 32 bit word seems fine, I'm just quibbling about the sign.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal here is to have full range of u32 as we do not have k_cycle_get_64.

@pizi-nordic pizi-nordic force-pushed the fix-15983 branch 3 times, most recently from d7dbafd to 489c115 Compare May 22, 2019 10:06
This commit changes the timer_api test in order to take under account
fact that timeouts used in the test might not be aligned to tick
boundary.

Signed-off-by: Piotr Zięcik <[email protected]>
The time measurement based on k_uptime_delta() might be not accurate
for some values of CONFIG_SYS_CLOCK_TICKS_PER_SEC. This commit
introduces measurement based on k_cycle_get_32(), which is more
precise.

Signed-off-by: Piotr Zięcik <[email protected]>
The time measurement based on k_uptime_delta() might be not accurate
for some values of CONFIG_SYS_CLOCK_TICKS_PER_SEC. This commit
introduces measurement based on k_cycle_get_32(), which is more
precise.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic pizi-nordic changed the title [WIP] Handle SYS_CLOCK_TICKS_PER_SEC != 100 in tests Handle SYS_CLOCK_TICKS_PER_SEC != 100 in tests May 23, 2019
@pizi-nordic pizi-nordic removed the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label May 23, 2019
@nashif nashif merged commit ba4eae1 into zephyrproject-rtos:master May 23, 2019
@pizi-nordic pizi-nordic deleted the fix-15983 branch May 29, 2019 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Kernel area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kernel tests assume SYS_CLOCK_TICKS_PER_SEC=100

6 participants