Add clock capabilities and example binary #137
Conversation
|
@davidv1992 See this as a draft - just an idea on how an API could look like. Users such as |
|
Follow-up on #134 |
zonque
force-pushed
the
clock-capabilities
branch
2 times, most recently
from
98f82cf to
8d2878a
Compare
Add a type to express a clock's capability of steering the clock and its phase. For regular POSIX clocks, the reported values default to what the Linux kernel is able to handle. On the first call of `Clock::capabilities()`, and when executed on Linux, the code will attempt to call the `PTP_CLOCK_GETCAPS` `ioctl()` once. In case the file descriptor belongs to a PTP clock, this will return a struct that tells us the capabilities of the underlying hardware. Otherwise, we fall back to the POSIX defaults.
zonque
force-pushed
the
clock-capabilities
branch
from
8d2878a to
d832824
Compare
zonque
force-pushed
the
clock-capabilities
branch
from
d832824 to
dbd52c9
Compare
|
This PR moves clock_steering significantly closer to being a general use library. We are currently not setup to maintain it as such, so we'll need to have an internal discussion whether we want it to be that. This might take us a few weeks. |
No worries, I'm not in a hurry. And I am also open to other suggestions. Bottom line however is that |
davidv1992
left a comment
davidv1992
left a comment
There was a problem hiding this comment.
We've had the internal discussion, and decided to move forward with this.
Having a look, mostly this looks like a reasonable direction, however there are a number of smaller issues that need some attention, most importantly the fact that there currently seems to be going on a bit of unit confusion. If you can fix the things identified below we should most likely be good to go.
|
|
||
| [dependencies] | ||
| libc = "0.2.165" | ||
| nix = { version = "0.27", features = ["ioctl"] } |
There was a problem hiding this comment.
We conciously make the choice to not use nix, so as to avoid it and its dependencies for ntpd-rs. Could you add manual implementations for the ioctls where needed, or implement the needed functionality in libc?
|
|
||
| Ok(ClockCapabilities { | ||
| max_frequency_adjustment_ppb: caps.max_adj as u64, | ||
| max_offset_adjustment_ns: caps.max_phase_adj as u32, |
There was a problem hiding this comment.
This should probably be a clamped value instead of an outright cast.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| pub struct ClockCapabilities { | ||
| /// Maximum frequency adjustment capability in parts per million. | ||
| pub max_frequency_adjustment_ppb: u64, |
There was a problem hiding this comment.
Given the units used in the rest of the interface for clocks (f64 respresenting desired frequency in ppm) it would be better to use that same unit here.
| impl Default for ClockCapabilities { | ||
| fn default() -> Self { | ||
| Self { | ||
| max_frequency_adjustment_ppb: 32_768_000_000, // 32768000 ppm |
There was a problem hiding this comment.
This value is incorrect, should match 500 ppm. Note that the kernel uses two units for frequency offsets, ppb and scaled ppm, where the latter is ppm multiplied by 2^16 (a.k.a. a fixed point format)
| /// Get the maximum frequency adjustment limit in ppm | ||
| pub const fn max_frequency_ppm(&self) -> u64 { | ||
| self.max_frequency_adjustment_ppb | ||
| } | ||
|
|
||
| /// Get the maximum offset adjustment limit in nanoseconds | ||
| pub const fn max_offset_ns(&self) -> u32 { | ||
| self.max_offset_adjustment_ns | ||
| } |
There was a problem hiding this comment.
Given what this struct is (a plain old data carrier) let's not do these functions.
| #[cfg(target_os = "linux")] | ||
| capabilities: Option<ClockCapabilities>, |
There was a problem hiding this comment.
Is there a reason to want explicit caching for the clock capabilities, instead of just looking them up when asked and trusting the user to do any caching if need be?
| assert!(max_freq > 0); | ||
| assert!(max_freq >= 32768000); // At least 32768000 ppm |
There was a problem hiding this comment.
These checks are redundant. Also let's just check that the actual value is correct, instead of a mere reasonability check, since this is a known clock with a guaranteed max frequency of 500ppm. (which also isn't the value tested for here).
|
Forgot to asl this, but have you checked the reported limits on actual hardware? The header file suggests the max_freq_adj field to be in ppb, which is weird and inconsistent with the rest of the linux time APIs so having some confirmation this is actually correct would be nice. (not that it would be surprising that there is inconsistency here, the time APIs are notorious for being quite awkward, especially for ptp hardware clocks.) |
2 participants
No description provided.