Add clock capabilities and example binary by zonque · Pull Request #137 · pendulum-project/clock-steering

zonque · 2026-01-15T14:50:02Z

No description provided.

zonque · 2026-01-15T14:51:29Z

@davidv1992 See this as a draft - just an idea on how an API could look like. Users such as statime would then need to take the returned maximums into account for their steering loop in order to avoid OutOfRange errors.

zonque · 2026-01-15T14:51:49Z

Follow-up on #134

davidv1992 · 2026-01-16T07:50:12Z

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.

zonque · 2026-01-16T09:13:33Z

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 statime needs to consider the hardware's capabilities for its steering loop, one way or another.

davidv1992

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.

davidv1992 · 2026-01-23T08:44:59Z

+
 [dependencies]
 libc = "0.2.165"
+nix = { version = "0.27", features = ["ioctl"] }


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?

Sure, can do, but could you elaborate on that choice? Just curious.

We try to keep the number of dependencies as low as reasonable. We decided a while ago that nix, while somewhat useful, did not provide enough utility for us to to justify its place in our supply chain.

davidv1992 · 2026-01-23T08:49:00Z

+
+                    Ok(ClockCapabilities {
+                        max_frequency_adjustment_ppb: caps.max_adj as u64,
+                        max_offset_adjustment_ns: caps.max_phase_adj as u32,


This should probably be a clamped value instead of an outright cast.

davidv1992 · 2026-01-23T08:53:03Z

+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub struct ClockCapabilities {
+    /// Maximum frequency adjustment capability in parts per million.
+    pub max_frequency_adjustment_ppb: u64,


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.

I will in fact make both variables here f64. That makes most sense I believe.

davidv1992 · 2026-01-23T08:54:18Z

+impl Default for ClockCapabilities {
+    fn default() -> Self {
+        Self {
+            max_frequency_adjustment_ppb: 32_768_000_000, // 32768000 ppm


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)

Right. Thanks for spotting!

davidv1992 · 2026-01-23T08:57:01Z

+    #[cfg(target_os = "linux")]
+    capabilities: Option<ClockCapabilities>,


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?

I was considering the runtime costs caused by the syscall, and the values reported by the kernel are not going to change. But you're right, this isn't a frequent operation, and dropping the cache makes the code a bit cleaner.

davidv1992 · 2026-01-23T09:00:12Z

+        assert!(max_freq > 0);
+        assert!(max_freq >= 32768000); // At least 32768000 ppm


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).

Right, will remove.

davidv1992 · 2026-01-23T09:05:00Z

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.)

davidv1992 · 2026-02-25T08:45:19Z

@zonque Would you be able to take a look at the review comments?

zonque · 2026-02-25T08:46:35Z

@zonque Would you be able to take a look at the review comments?

Yes, will do soon. Sorry, I got busy with too many other things meanwhile 🙈

zonque · 2026-02-26T13:44:40Z

Forgot to asl this, but have you checked the reported limits on actual hardware?

Yes I did. We are using a PTP driver for our own hardware, and that reports a maximum value for frequency adjustment of 100_000 ppb.

(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.)

😏

zonque · 2026-02-26T14:03:20Z

@zonque Would you be able to take a look at the review comments?

Addressed the comments now and pushed again.

davidv1992

Just a few minor things remaining, thanks for the rest of the fixes :). Feel free to ignore the note to self.

davidv1992 · 2026-03-04T17:15:41Z

+#[derive(Debug, Clone, Copy, PartialEq)]
+pub struct ClockCapabilities {
+    /// Maximum frequency adjustment capability in parts per billion.
+    pub max_frequency_adjustment_ppb: f64,


You missed the conversion to ppm in the interface here.

davidv1992 · 2026-03-04T17:19:42Z

+    pub max_frequency_adjustment_ppb: f64,
+
+    /// Maximum offset adjustment capability in nanoseconds.
+    pub max_offset_adjustment_ns: f64,


I don't agree with the choice to make this an f64. Again, lets keep the interface consistent and use a u32 here, matching TimeOffset below

davidv1992 · 2026-03-04T17:22:16Z

+#[repr(C)]
+pub struct PtpClockCaps {
+    pub max_adj: libc::c_int, // Maximum frequency adjustment in parts per billion
+    pub n_alarm: libc::c_int, // Number of programmable alarms
+    pub n_ext_ts: libc::c_int, // Number of external time stamp channels
+    pub n_per_out: libc::c_int, // Number of programmable periodic signals
+    pub pps: libc::c_int,     // Whether the clock supports a PPS callback
+    pub n_pins: libc::c_int,  // Number of input/output pins
+    pub cross_timestamping: libc::c_int, // Whether the clock supports precise system-device cross timestamps
+    pub adjust_phase: libc::c_int,       // Whether the clock supports adjust phase
+    pub max_phase_adj: libc::c_int,      // Maximum offset adjustment in nanoseconds
+    pub rsv: [libc::c_int; 11],          // Reserved for future use
+}
+
+// PTP_CLOCK_GETCAPS = _IOR('=', 1, struct ptp_clock_caps)
+//
+// Linux _IOR encoding: (IOC_READ << 30) | (size << 16) | (type << 8) | nr
+//   IOC_READ = 2, type = b'=' = 0x3D, nr = 1
+const PTP_CLOCK_GETCAPS: u32 =
+    (2u32 << 30) | ((std::mem::size_of::<PtpClockCaps>() as u32) << 16) | ((b'=' as u32) << 8) | 1;


Note to self: this needs upstreaming to libc.

davidv1992 · 2026-03-05T09:44:38Z

@zonque: I think this is really close to the finish line, would you mind if I push it the final bit or do you prefer to do that yourself?

zonque · 2026-03-05T09:48:00Z

@zonque: I think this is really close to the finish line, would you mind if I push it the final bit or do you prefer to do that yourself?

I can look into it earliest tomorrow, but if you prefer that, I have no objections of you'd take care of the final bits.

zonque · 2026-03-09T18:23:36Z

I can look into it earliest tomorrow, but if you prefer that, I have no objections of you'd take care of the final bits.

I just pushed again, please check.

If you merge this, please squash the commits.

davidv1992

Thank you for doing this work. We are looking at doing an overhaul of the algorithm soon, and that will provide us with a decent opportunity to start using this information.

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 4 times, most recently from d832824 to dbd52c9 Compare January 15, 2026 15:51

davidv1992 requested changes Jan 23, 2026

View reviewed changes

zonque force-pushed the clock-capabilities branch 2 times, most recently from 440fba9 to cf1e65d Compare February 26, 2026 14:02

zonque force-pushed the clock-capabilities branch from cf1e65d to 214e962 Compare February 26, 2026 14:11

zonque requested a review from davidv1992 February 26, 2026 17:08

davidv1992 requested changes Mar 4, 2026

View reviewed changes

davidv1992 reviewed Mar 4, 2026

View reviewed changes

davidv1992 approved these changes Mar 11, 2026

View reviewed changes

davidv1992 force-pushed the clock-capabilities branch from c448e17 to dfcf7d4 Compare March 11, 2026 09:23

davidv1992 enabled auto-merge March 11, 2026 09:23

davidv1992 added this pull request to the merge queue Mar 11, 2026

Merged via the queue into pendulum-project:main with commit 9900a29 Mar 11, 2026
6 checks passed

		#[cfg(target_os = "linux")]
		capabilities: Option<ClockCapabilities>,

		assert!(max_freq > 0);
		assert!(max_freq >= 32768000); // At least 32768000 ppm

Conversation

zonque commented Jan 15, 2026

Uh oh!

zonque commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zonque commented Jan 15, 2026

Uh oh!

davidv1992 commented Jan 16, 2026

Uh oh!

zonque commented Jan 16, 2026

Uh oh!

davidv1992 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidv1992 commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidv1992 commented Feb 25, 2026

Uh oh!

zonque commented Feb 25, 2026

Uh oh!

zonque commented Feb 26, 2026

Uh oh!

zonque commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidv1992 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidv1992 commented Mar 5, 2026

Uh oh!

zonque commented Mar 5, 2026

Uh oh!

zonque commented Mar 9, 2026

Uh oh!

davidv1992 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

zonque commented Jan 15, 2026 •

edited

Loading

davidv1992 commented Jan 23, 2026 •

edited

Loading

zonque commented Feb 26, 2026 •

edited

Loading