Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libc: minimal: implement time() API #33397

Merged
merged 2 commits into from
Mar 19, 2021
Merged

libc: minimal: implement time() API #33397

merged 2 commits into from
Mar 19, 2021

Conversation

mniestroj
Copy link
Member

Implement time() API by using clock_gettime(CLOCK_REALTIME, ...).

@mniestroj mniestroj requested a review from nashif as a code owner March 16, 2021 17:35
@mniestroj mniestroj requested a review from pabigot March 16, 2021 17:36
@mniestroj mniestroj mentioned this pull request Mar 16, 2021
Closed
ceolin
ceolin previously requested changes Mar 16, 2021
View reviewed changes
Copy link
Member

@ceolin ceolin left a comment

Choose a reason for hiding this comment

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

Shouldn't it be:
Upon successful completion, time() shall return the value of time. Otherwise, (time_t)-1 shall be returned.

@mniestroj mniestroj requested a review from ceolin March 16, 2021 21:40
enjiamai
enjiamai approved these changes Mar 17, 2021
View reviewed changes
Copy link
Collaborator

@enjiamai enjiamai left a comment

Choose a reason for hiding this comment

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

I think it would be perfect if there is also a test case added in tests/lib/c_lib/src/main.c.

KozhinovAlexander
KozhinovAlexander approved these changes Mar 17, 2021
View reviewed changes
Copy link
Collaborator

@KozhinovAlexander KozhinovAlexander left a comment

Choose a reason for hiding this comment

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

LGTM

pfalcon
pfalcon reviewed Mar 17, 2021
View reviewed changes
lib/libc/minimal/source/time/time.c Outdated
* SPDX-License-Identifier: Apache-2.0
*/

#include <posix/time.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is included for just clock_gettime prototype, I'd suggest to include a comment on that. Because otherwise it's a bit strange dependency, which can lead to further include dependency hell.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is for clock_gettime() prototype. You mean dependency hell because posix/time.h might fetch time.h or the other way around? Fortunately, this is just a C source file, not a header file, so I was not expecting any problems with dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nonetheless, the include should be conditional as it's pulling in something that isn't part of libc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Conditional on what? CONFIG_POSIX_CLOCK? Currently whole file is compiled only when CONFIG_POSIX_CLOCK=y.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean dependency hell because posix/time.h

Because posix/time.h is a full POSIX header which might have/pull things which aren't available in minimal libc or done differently (incompatibly) in it. Put it differently, at any time compiling this source file may break. Some time ago, I spent a lot of effort to straighten up dependencies between minlibc/newlib/posix, and that wasn't even done completely, and this patch is kinda retrograde action in that regard.

Copy link
Collaborator

@pabigot pabigot Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Conditional on what? CONFIG_POSIX_CLOCK? Currently whole file is compiled only when CONFIG_POSIX_CLOCK=y.

OK, then the comment is certainly desirable (because that dependency is not clear from the code).

#18892 is also relevant. It's not clear that this change is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood. I've added a comment, as suggested. Let me know if there is something more that I can do.

@mniestroj
libc: minimal: implement time() API
79d353b 
Implement time() API by using clock_gettime(CLOCK_REALTIME, ...).

Signed-off-by: Marcin Niestroj <[email protected]>
@carlescufi carlescufi dismissed ceolin’s stale review March 18, 2021 08:43

stale, please review again

@mniestroj mniestroj assigned jukkar and unassigned jukkar Mar 18, 2021
@jukkar jukkar assigned nashif and unassigned jukkar Mar 18, 2021
@jukkar
Copy link
Member

jukkar commented Mar 18, 2021

I am not proper owner of this PR, so reassigning to @nashif

nashif
nashif requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

add a test please.

@mniestroj
tests: lib: time: add test for time() function
2fdafaa 
Add test cases for time() function.

Signed-off-by: Marcin Niestroj <[email protected]>
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Mar 18, 2021
@nashif nashif merged commit 4682cf1 into zephyrproject-rtos:master Mar 19, 2021
@mniestroj mniestroj deleted the libc-minimal-time branch May 7, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KozhinovAlexander KozhinovAlexander KozhinovAlexander approved these changes

@enjiamai enjiamai enjiamai approved these changes

@jukkar jukkar jukkar approved these changes

@pfalcon pfalcon pfalcon approved these changes

@nashif nashif nashif approved these changes

@pabigot pabigot Awaiting requested review from pabigot

@ceolin ceolin Awaiting requested review from ceolin

@KangJianX KangJianX Awaiting requested review from KangJianX

@mengxianglinx mengxianglinx Awaiting requested review from mengxianglinx

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

Assignees

@nashif nashif

Labels
area: C Library C Standard Library area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mniestroj @jukkar @nashif @pfalcon @ceolin @pabigot @enjiamai @KozhinovAlexander