Skip to content

Conversation

@cfriedt
Copy link
Member

@cfriedt cfriedt commented May 23, 2020

This change adds support for nanosleep(2) documented at
https://pubs.opengroup.org/onlinepubs/9699919799/functions/nanosleep.html

Note: Does not achieve any greater resolution than that provided by k_busy_wait(us) but at least makes the API available.

I've not researched it much in Zephyr, but I would say there is still some work to be done for a generic clock API - one that allows querying resolutions, supports multiple system clocks, etc, primarily to support POSIX time APIs.

A nanosleep system call would also require high-res timer support (See #6498)

Fixes #25554

@cfriedt cfriedt requested a review from pfalcon as a code owner May 23, 2020 09:44
@zephyrbot zephyrbot added area: POSIX POSIX API Library area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels May 23, 2020
@zephyrbot
Copy link

zephyrbot commented May 23, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@cfriedt
Copy link
Member Author

cfriedt commented May 23, 2020

I have a feeling there is a conflict with CONFIG_BOARD_NATIVE_POSIX
CONFIG_BOARD_NATIVE_POSIX_64BIT

@pfalcon
Copy link
Contributor

pfalcon commented May 23, 2020

I have a feeling there is a conflict with CONFIG_BOARD_NATIVE_POSIX
CONFIG_BOARD_NATIVE_POSIX_64BIT

I'm not sure what exactly you mean (that native_posix and native_posix_64 boards conflict between each other?), but otherwise it's a known problem that Zephyr POSIX subsys conflicts with native_posix board. It should be understanding why - it's too layery a pie, where Zephyr POSIX subsys sits on top of native Zephyr core, and naive_posix boards tries to implement that in terms of the underlying full-POSIX system. Something doesn't match regularly. Many a POSIX tests/samples simply blacklist native_posix.

@cfriedt
Copy link
Member Author

cfriedt commented May 23, 2020

I have a feeling there is a conflict with CONFIG_BOARD_NATIVE_POSIX
CONFIG_BOARD_NATIVE_POSIX_64BIT

I'm not sure what exactly you mean (that native_posix and native_posix_64 boards conflict between each other?), but otherwise it's a known problem that Zephyr POSIX subsys conflicts with native_posix board. It should be understanding why - it's too layery a pie, where Zephyr POSIX subsys sits on top

That's what I meant. Sorry, was on mobile when I commented previously.

@cfriedt
Copy link
Member Author

cfriedt commented May 23, 2020

Should be good to go now

$ ./scripts/sanitycheck --enable-slow --inline-logs -j8 --testcase-root tests/posix/common
Renaming output directory to /home/cfriedt/workspace/zephyrproject/zephyr/sanity-out.24
INFO    - JOBS: 8
INFO    - Selecting default platforms per test case
INFO    - Building initial testcase list...
INFO    - 26 test configurations selected, 972 configurations discarded due to filters.
INFO    - Adding tasks to the queue...
INFO    - Total complete:   26/  26  100%  skipped:    0, failed:    0
INFO    - 26 of 26 tests passed (100.00%), 0 failed, 0 skipped with 0 warnings in 179.42 seconds
INFO    - In total 4 test cases were executed on 13 out of total 250 platforms (5.20%)
INFO    - 20 tests executed on platforms, 6 tests were only built.

@cfriedt
Copy link
Member Author

cfriedt commented May 23, 2020

Btw, the variance in latency (in CI / qemu) is pretty drastic between architectures. E.g. on qemu_cortex_r5, it was 500us, on xtensa it was 86 us, and on qemu_cortex_m3 it was 25us.

Sample size of 1, not statistically representative, of course.

Otherwise, I would have added a more stringent check than

/* should be more accurate than ns + 1 tick period */

Also, not 100% certain if this counts as a system call or a libc function. I assumed that it would be a libc function. There is no corresponding Zephyr system call, atm. However, pointers are passed in (NULL check is made, but otherwise no address space validation).

Edit: Even qemu_x86 (I guess with heavy server load?) over 10 ms of latency.

dt_ns: 1010136310

    Assertion failed at WEST_TOPDIR/zephyr/tests/posix/common/src/nanosleep.c:198: common: (dt_ns < ns + NS_PER_TICK is false)
expected: dt_ns < 1010000000 actual: 1010136310

Giving up on that upper-bounds test for now, as I don't think it can be reliably proved in CI.

@cfriedt cfriedt marked this pull request as draft May 23, 2020 22:16
@cfriedt cfriedt marked this pull request as ready for review May 23, 2020 23:49
@cfriedt cfriedt requested a review from pfalcon May 23, 2020 23:50
@pfalcon pfalcon requested a review from carlescufi May 25, 2020 11:40
Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

Thanks!

@pfalcon
Copy link
Contributor

pfalcon commented May 25, 2020

@cfriedt, Thanks for the patch. I would personally recommend this merged for 2.3, but per the release freeze rules, 2 reviews are required at this time. I've added @carlescufi who both could review and merged (but definitely busy with a lot of other stuff now, as the release manager for this version), but feel free ping other folks who you know can help with review.

@pfalcon pfalcon added this to the v2.3.0 milestone May 25, 2020
@cfriedt
Copy link
Member Author

cfriedt commented May 25, 2020

@cfriedt, Thanks for the patch. I would personally recommend this merged for 2.3, but per the release freeze rules, 2 reviews are required at this time. I've added @carlescufi who both could review and merged (but definitely busy with a lot of other stuff now, as the release manager for this version), but feel free ping other folks who you know can help with review.

It would be awesome if nanosleep could actually get relatively close to the exact time (if there is hardware to support it). The high-res timer stuff is pretty important though, and I think it deserves a great deal of care. No rush on that one. The nice thing about it though is that the interface is there, and it's not moving, so just redoing the implementation later will have no unwanted side-effects.

@cfriedt cfriedt requested a review from jukkar May 25, 2020 21:40
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Looks good, minor nit about the macro name that was missed from previous cleanup.

@cfriedt cfriedt self-assigned this Jun 1, 2020
@carlescufi carlescufi removed this from the v2.3.0 milestone Jun 2, 2020
@cfriedt
Copy link
Member Author

cfriedt commented Jun 8, 2020

@carlescufi - rebased off master / using <stdint.h> integer types. Should be good to go as long as the build works.

@cfriedt
Copy link
Member Author

cfriedt commented Jun 9, 2020

Was green before. Looks like there is some misra change that breaks things atm

@pfalcon
Copy link
Contributor

pfalcon commented Jun 9, 2020

some misra change

New swear word in software engineering? ;-)

@pfalcon pfalcon closed this Jun 11, 2020
@pfalcon pfalcon reopened this Jun 11, 2020
cfriedt added 2 commits June 11, 2020 09:06
This change adds support for nanosleep(2) documented at
https://pubs.opengroup.org/onlinepubs/9699919799/functions/nanosleep.html

N.B: Currently, this provides no better resolution than
k_busy_wait()

Fixes #25554

Signed-off-by: Christopher Friedt <[email protected]>
This commit provides test-cases for nanosleep(2)

Signed-off-by: Christopher Friedt <[email protected]>
@pfalcon
Copy link
Contributor

pfalcon commented Jun 11, 2020

starting test - test_timeout_lifo_thread

    Assertion failed at WEST_TOPDIR/zephyr/tests/kernel/lifo/lifo_usage/src/main.c:329: test_timeout_lifo_thread: packet != NULL is false

FAIL - test_timeout_lifo_thread

I got such a failure in one of my recent PR. Looks like nondeterministic failure du jour.

@cfriedt: Please keep rebasing this, until sweet spot is found.

@carlescufi carlescufi merged commit 72e7deb into zephyrproject-rtos:master Jun 12, 2020
@cfriedt cfriedt deleted the issue/25554/lib-posix-nanosleep branch June 12, 2020 15:18
@thedjnK thedjnK mentioned this pull request Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: POSIX POSIX API Library area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lib: posix: nanosleep

5 participants