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.

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

Support waiting on condition variable until a specific time point #665

Merged
merged 22 commits into from
Dec 18, 2024

Conversation

bjsowa
Copy link
Contributor

@bjsowa bjsowa commented Sep 16, 2024

This is my proposal for supporting waiting on condition variable till a specific time point. For now, I only implemented it for unix. If this gets approved, I can implement this function for the rest of the ports.

Copy link

PR missing one of the required labels: {'new feature', 'enhancement', 'dependencies', 'internal', 'documentation', 'breaking-change', 'bug'}

@jean-roland
Copy link
Contributor

Hello @bjsowa, thanks for your contribution. Just to make sure since your PR is in draft, do you want us to give it a look as is?

@bjsowa
Copy link
Contributor Author

bjsowa commented Sep 24, 2024

Hello @bjsowa, thanks for your contribution. Just to make sure since your PR is in draft, do you want us to give it a look as is?

Yes, I would like to discuss the interface first. Once it's approved, I will implement it for different ports

@jean-roland
Copy link
Contributor

OK then we'll check this a bit later, when we have time™.

@jean-roland
Copy link
Contributor

Hey @bjsowa, the approach looks fine to us.

Just a note, since we're in the process of releasing Zenoh 1.0, we'll only be able to merge the changes after the release.

Copy link

PR missing one of the required labels: {'dependencies', 'enhancement', 'breaking-change', 'new feature', 'bug', 'internal', 'documentation'}

Copy link

PR missing one of the required labels: {'dependencies', 'enhancement', 'documentation', 'new feature', 'breaking-change', 'internal', 'bug'}

Copy link

PR missing one of the required labels: {'bug', 'new feature', 'breaking-change', 'dependencies', 'internal', 'documentation', 'enhancement'}

Copy link

PR missing one of the required labels: {'internal', 'enhancement', 'bug', 'new feature', 'breaking-change', 'dependencies', 'documentation'}

Copy link

PR missing one of the required labels: {'documentation', 'enhancement', 'bug', 'dependencies', 'breaking-change', 'new feature', 'internal'}

Copy link

PR missing one of the required labels: {'bug', 'dependencies', 'breaking-change', 'internal', 'enhancement', 'new feature', 'documentation'}

@sashacmc sashacmc added the enhancement Things could work better label Dec 6, 2024
@bjsowa
Copy link
Contributor Author

bjsowa commented Dec 7, 2024

I'll try to finish this PR after #821 gets merged

@bjsowa bjsowa force-pushed the feature/condvar_timedwait branch from 4a06031 to ff7dc08 Compare December 15, 2024 23:47
@bjsowa bjsowa marked this pull request as ready for review December 16, 2024 00:13
@bjsowa
Copy link
Contributor Author

bjsowa commented Dec 16, 2024

This is ready for review but take in mind I only tested it on unix and freertos ports. I'm especially not sure about mbed, windows and emscripten ports.

@sashacmc
Copy link
Member

@bjsowa thanks for the PR!
IMHO, it would be better to return a timeout error as a new z_result_t code:

  • Simpler API: Users can handle all outcomes (errors, timeouts, success) directly via the return value, removing the need for a separate timeout flag.
  • Consistency: Aligns with common practices (e.g., ETIMEDOUT in POSIX), ensuring all exceptional conditions are uniformly represented by z_result_t.
  • Cleaner Code: Simplifies error handling, reduces complexity, and improves code readability and maintainability.

@bjsowa bjsowa force-pushed the feature/condvar_timedwait branch from c707624 to 57b672a Compare December 16, 2024 22:50
@bjsowa bjsowa force-pushed the feature/condvar_timedwait branch from 57b672a to 284e3ba Compare December 16, 2024 22:53
Copy link
Member

@sashacmc sashacmc left a comment

Choose a reason for hiding this comment

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

Last nit,
others LGTM

Comment on lines +217 to +220
if (clock->tv_nsec >= 1000000000) {
clock->tv_sec += 1;
clock->tv_nsec -= 1000000000;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (clock->tv_nsec >= 1000000000) {
clock->tv_sec += 1;
clock->tv_nsec -= 1000000000;
}
if (clock->tv_nsec >= 1000000000) {
clock->tv_sec += clock->tv_nsec / 1000000000;
clock->tv_nsec %= 1000000000;
}

This will be more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it really necessary? Assuming the passed clock is valid (tv_nsec < 1e9) there should be at most one second that we will have to carry from tv_nsec to tv_sec

@milyin milyin merged commit aa64a2e into eclipse-zenoh:main Dec 18, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Things could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants