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

RFC: PTPv1 support #602

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

RFC: PTPv1 support #602

wants to merge 1 commit into from

Conversation

teodly
Copy link
Contributor

@teodly teodly commented Jan 20, 2025

Working on #382

Slave-only for now.

I've implemented PTPv1-related functions side-by-side in the same impl blocks that PTPv2 implementation. Some logic was adjusted to the IEEE1588-2002 specification, some was reused or copied without logical changes, so probably not yet standard-compliant. But it works as a slave in Dante network.

Putting it here early because I want your opinion on this approach, or maybe you have a better idea of code organization?

To do:

  • operation as master
  • fix code repetitions (generalization is possible in some places marked with TODO DRY)
  • fill fields in packets and data structures with meaningful values instead of placeholders: in PTPv1 packets: local_clock_stratum, local_clock_identifier, epoch_numer, in PtpInstanceState: clock_class, clock_accuracy
    • I will try to understand differences between specifications and possibility of interconnecting PTPv1 and v2 networks, however, help with this will be appreciated.
  • write tests specifically for PTPv1, resurrect commented out tests
  • remove unused code & cargo fmt

I've put the following remark in a comment:

It is technically possible to run different versions of PTP on the same physical Ethernet interface (Dante devices do this when having AES67 enabled) - this way PTPv1-to-v2 or vice versa "converter" can be created. To do this in Statime, create multiple Port instances, route packets to handle_*_receive methods according to PTP version in packet's header and merge actions from PortActionIterators. Not tested yet, beware of possible strange conditions like boundary clock loops.

But it is requiring library user to do something that could be already implemented in Statime. But implementing it in the library would require a refactor, adding an intermediate layer because we want one physical port to correspond to multiple IEEE1588 ports so that PTPv1 slave clock can be PTPv2 master or vice versa (inter-version boundary clock, Dante devices do this)

How to run:

add

protocol-version = "PTPv1"

to port config

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 5.88889% with 847 lines in your changes missing coverage. Please review.

Project coverage is 53.10%. Comparing base (856d9af) to head (2355d08).

Files with missing lines Patch % Lines
statime/src/datastructures/messages_v1/mod.rs 0.00% 128 Missing ⚠️
statime/src/port/slave.rs 6.55% 114 Missing ⚠️
statime/src/port/bmca.rs 1.09% 90 Missing ⚠️
statime/src/bmc/foreign_master.rs 14.43% 83 Missing ⚠️
statime/src/datastructures/messages_v1/header.rs 0.00% 81 Missing ⚠️
...rc/datastructures/messages_v1/sync_or_delay_req.rs 0.00% 63 Missing ⚠️
statime/src/bmc/bmca.rs 9.25% 49 Missing ⚠️
statime/src/port/mod.rs 27.27% 48 Missing ⚠️
statime/src/port/actions.rs 0.00% 43 Missing ⚠️
...tatime/src/datastructures/common/grandmaster_v1.rs 0.00% 35 Missing ⚠️
... and 10 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #602      +/-   ##
==========================================
- Coverage   62.28%   53.10%   -9.18%     
==========================================
  Files          62       70       +8     
  Lines        8211     8645     +434     
==========================================
- Hits         5114     4591     -523     
- Misses       3097     4054     +957     

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

In general the approach looks decent. However, the concat and in particular the reown function raises some red flags. Especially reown I would expect not to be needed if all the lifetime annotations are correct, so perhaps those need some rejigging. However, I don't have time to look into that in detail right now.

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