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

Allow measurements without delay #1673

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

michielp1807
Copy link
Contributor

@michielp1807 michielp1807 commented Oct 2, 2024

This PR allows the delay property of measurements to be either NtpDuration or (). If the delay is (), we use a constant value as the estimate of the measurement noise instead of an estimate based on the variance of delay values. This is useful for supporting alternative time sources, such as the SOCK protocol (#1632).

The functions for creating a SOCK source are currently not covered by unit tests. I will add unit tests for these functions in a separate PR together with the actual implementation of the SOCK source.

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 72.58065% with 68 lines in your changes missing coverage. Please review.

Project coverage is 81.30%. Comparing base (eba70a7) to head (75b6894).

Files with missing lines Patch % Lines
ntpd/src/force_sync/algorithm.rs 0.00% 32 Missing ⚠️
ntp-proto/src/algorithm/kalman/mod.rs 40.00% 12 Missing ⚠️
ntp-proto/src/system.rs 45.00% 11 Missing ⚠️
ntpd/src/daemon/system.rs 0.00% 6 Missing ⚠️
ntp-proto/src/algorithm/kalman/source.rs 97.36% 4 Missing ⚠️
ntp-proto/src/source.rs 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1673      +/-   ##
==========================================
- Coverage   81.33%   81.30%   -0.03%     
==========================================
  Files          64       64              
  Lines       19751    19780      +29     
==========================================
+ Hits        16065    16083      +18     
- Misses       3686     3697      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@davidv1992 davidv1992 left a comment

Choose a reason for hiding this comment

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

Some choices around type leaking that I don't agree with. The actual logic changes seem ok though.

@@ -1,5 +1,6 @@
use std::{collections::HashMap, fmt::Debug, hash::Hash, time::Duration};

pub use source::AveragingBuffer;
Copy link
Member

Choose a reason for hiding this comment

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

I'd strongly prefer if we do not make this type public. This is basically an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 180f7e1

}
}

fn add_sock_source(
Copy link
Member

Choose a reason for hiding this comment

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

bit bikeshedding, but I would prefer a more generic name, as this is likely going to be used for more than just sock sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4a5f52b

@@ -242,52 +242,48 @@ impl<SourceId: Hash + Eq + Copy + Debug, Controller: TimeSyncController<SourceId
Ok(())
}

#[allow(clippy::type_complexity)]
pub fn create_ntp_source(
pub fn create_source_controller(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should leak the controllers out of system. I'd much rather have a thin-ish wrapper for non-ntp sources as well.

Although the idea of combining both nts and non-nts source creation isn't necessarily a bad thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in cafc220 (as part of the other PR because here we do not have the sock source yet.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants