RFC: virtual (overlay) system clock #504
Conversation
|
Added ability to export clock to other apps using usrvclock protocol. Not tested yet - currently I don't have access to PTP hardware, will test in 2 weeks. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #504 +/- ##
==========================================
- Coverage 63.11% 62.20% -0.92%
==========================================
Files 60 63 +3
Lines 8104 8225 +121
==========================================
+ Hits 5115 5116 +1
- Misses 2989 3109 +120 ☔ View full report in Codecov by Sentry. |
|
Thank you for the contribution. This is looking interesting, however I have a few concerns. First, I would like to ask you to split the usrvclock stuff into it's own PR. It needs additional consideration due to the extra dependencies and the fact that it is not yet a widely accepted standard, and splitting it would make adoption of the rest of the changes here easier. Also, #517 might be better for the current use cases for this. Beyond that, having had a short look at the changes, the rest looks decent. There are a few formatting issues and some commented out code that should be fixed before this is ready for merging, and I would like to ask you to reconsider the auto_impls dependency. We are looking to reduce the number of dependencies as we progress to version 1.0, and this would go against that goal. But again, other than those minor things, this is some nice work that we would be interested in. |
OK, reverted this commit in this branch.
Ran
Done. The commented out code in Tested with Dante PTPv2, both master and slave. |
statime/src/overlay_clock.rs
Outdated
| fn step_clock(&mut self, offset: Duration) -> Result<Time, Self::Error> { | ||
| self.last_sync = self.roclock.now(); | ||
| self.shift += offset; | ||
| Ok(self.time_from_underlying(self.last_sync)) | ||
| } |
There was a problem hiding this comment.
This won't work if self.freq_scale_ppm_diff is not zero, as we change the last sync time, and the shift is not update to match that change.
There was a problem hiding this comment.
You're right, offset is in overlay timescale, not underlying.
statime-linux/src/main.rs
Outdated
| @@ -249,7 +303,7 @@ async fn actual_main() { | |||
| for port_config in config.ports { | |||
| let interface = port_config.interface; | |||
| let network_mode = port_config.network_mode; | |||
| let (port_clock, timestamping) = match port_config.hardware_clock { | |||
| let (port_clock, port_clock2, timestamping) = match port_config.hardware_clock { | |||
There was a problem hiding this comment.
Why are the two clock variables needed here? Can't we get away with one? looking at the code below they essentially always are the same clock.
There was a problem hiding this comment.
Because we need separate owned impl Clock instances: for instance.add_port and port_task. Originally port_clock.clone() was used for this but we can't clone a Box. So we need to clone before boxing.
There was a problem hiding this comment.
boxes can be cloned assuming the inner thing is clone, which we can guarantee here I think, so I would strongly prefer that approach
There was a problem hiding this comment.
Done, but it required a bit of boilerplate code: https://stackoverflow.com/questions/30353462/how-to-clone-a-struct-storing-a-boxed-trait-object
|
Can you fix the failing style check? |
This reverts commit 93ccc57.
As discussed in #389
This adds configuration option
virtual-system-clockwhich is disabled by default which means that OS clock will be used directly as it always was.If set to true, OS clock will be read-only. Overlay clock will be based on it and used for synchronization.
Tested with Dante device, both master and slave. Seems to work. Hardware timestamping not tested yet due to #503 and pendulum-project/timestamped-socket#66
Exporting the clock to other processes using usrvclock local protocol (currently draft) is planned.