Skip to content

Conversation

@paulgear
Copy link
Contributor

@paulgear paulgear commented Sep 2, 2025

This is my first cut at a fix for #1138.

A few comments:

  • Most of the driver is modelled on the PPS driver, although it is a timer-driven rather than event-driven pattern.
  • I have tried to limit the extent of code changes in existing files in the project.
  • I'm not really happy with the way the polling intervals work, but because the observability code uses PollInterval rather than an f64 and I didn't want to make changes to the way that ntp-ctl consumes PollInterval, this means PTP devices must be polled on a power of 2 interval like NTP sources, even though technically they don't need to be.
  • I'm also not really happy with the lack of root dispersion figures, but I'm not sure how this could be fixed.
  • There's a separate ptp-time crate that I modelled on pps-time. It's under my GitHub namespace just so I could build the project successfully, but I'm happy to move it over to pendulum-project whenever it suits.
  • I'm not a rustacean, and I struggle with a lot of the subtleties of the language, so I've used this PR as a way to get more familiar with Rust concepts and tools.
    • I'm not sure that it will meet your criteria for AI contributions; I have used generative AI extensively to help me with the heaving lifting of reviewing the code and also for the skeleton implementation using requirements-driven development. Some of the prompts I've used are provided in docs/development/prompts/ptp/, and you'll see generated commits in the git history.
    • However, I have hand-reviewed all of the code and written some of it manually. I think the design and structure are reasonably solid in that they reflect the existing patterns in the code. I'm reasonably happy that I understand the high-level details, but there might be little details which are a bit 🤦 to a native Rust speaker. I'm happy to adjust these under your guidance.
    • All of the documentation changes were hand-written.
  • Here's a Grafana dashboard showing this code running over a roughly 14-hour period on one of my VMs: https://snapshots.raintank.io/dashboard/snapshot/mOiBklZGR6QAmO4BWm96xWJ30J1l35n1?orgId=0

Automated commit by Ollama 0.10.0, hf.co/unsloth/Qwen3-Coder-30B-A3B-Instruct-GGUF:Q6_K_XL 30.5B ctx:61440 vram:44.7 GB

Used MCP tool: get_ollama_model_info
Automated commit by Ollama 0.10.0, hf.co/unsloth/Qwen3-Coder-30B-A3B-Instruct-GGUF:Q5_K_XL 30.5B ctx:82944 vram:45.1 GB

Tools used:
- get_ollama_model_info: Retrieved Ollama model information
Automated commit by AI

No external MCP tools used.
Automated commit by AI

No external MCP tools used.
Automated commit by AI

No external MCP tools used.
Automated commit by AI

No external MCP tools used.
- Changed precision from mandatory to optional in PtpSourceConfig
- Default precision is now 0.000000001 seconds (1 nanosecond)
- Updated test to verify default precision behavior
- Use correct ptp-time API (get_sys_offset_precise instead of non-existent fetch_blocking)
- Fix data types to use ptp_sys_offset_precise from ptp-time crate
- Add ReferenceId::PTP constant for proper source identification
- Remove unused imports and fix compilation errors
- Follow established patterns from PPS driver for consistency

Automated commit by Amazon Q/Claude Sonnet 4
- Implement automatic detection of PTP timestamp capabilities at initialization
- Add fallback mechanism: Precise -> Extended -> Standard timestamps
- Support all three timestamp types from ptp-time crate
- Extract timestamp data correctly for each capability level
- Follow PRD requirement for capability detection only at initialization

Automated commit by Amazon Q/Claude Sonnet 4
- Remove unused validate() method from PtpSourceConfig
- Remove unused noise_estimate field from PtpSourceCreateParameters
- Update PTP spawner to not reference removed field

Automated commit by Amazon Q/Claude Sonnet 4
Automated commit by AI

Created comprehensive integration tests for PTP driver system integration and message passing patterns, including spawner lifecycle, error handling, and communication verification. Also included existing integration tests in module structure.
Automated commit by AI

Successfully ran comprehensive system integration tests with all 283 tests passing (205 in ntp-proto, 71 in ntpd including 10 PTP tests, 7 in integration tests). No issues found - all PTP driver system integration and message passing is working correctly.
Automated commit by AI

Successfully ran all PTP integration tests (10 total tests passing). Fixed unused import warning in ptp_integration_test.rs. All integration tests are working correctly with no issues found.
Also sort fields alphabetically
Some sections have been reordered alphabetically for easier reference
@codecov
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.56%. Comparing base (ee03739) to head (b093b35).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
ntp-proto/src/system.rs 0.00% 17 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1961       +/-   ##
===========================================
- Coverage   83.44%   21.56%   -61.89%     
===========================================
  Files          70       37       -33     
  Lines       19266     5816    -13450     
===========================================
- Hits        16076     1254    -14822     
- Misses       3190     4562     +1372     
Flag Coverage Δ
fuzz-cookie_parsing_sound 0.45% <0.00%> (-0.25%) ⬇️
fuzz-duration_from_float 0.29% <0.00%> (-0.01%) ⬇️
fuzz-encrypted_client_parsing 7.77% <0.00%> (-1.27%) ⬇️
fuzz-encrypted_server_parsing 10.68% <0.00%> (-1.36%) ⬇️
fuzz-ipfilter 2.73% <0.00%> (-0.01%) ⬇️
fuzz-key_exchange_request_parsing 2.32% <0.00%> (-1.27%) ⬇️
fuzz-key_exchange_response_parsing 2.74% <0.00%> (-1.56%) ⬇️
fuzz-packet_keyset 4.10% <0.00%> (-2.24%) ⬇️
fuzz-packet_parsing_sound 8.67% <0.00%> (-0.15%) ⬇️
fuzz-record_encode_decode 3.21% <0.00%> (-0.01%) ⬇️
test-aarch64-apple-darwin ?
test-x86_64-unknown-linux-gnu ?
test-x86_64-unknown-linux-musl ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@davidv1992
Copy link
Member

Thank you very much for this contribution. We have taken a preliminary look at this and it looks interesting and of sufficient quality that we would like to do a more complete review with the aim of including it.

However, given the way this was built and the extent to which this was built using AI we unfortunately will have to require you to explicitly confirm that despite that, you are sure that you have the rights to this contribution and that it is not infringing on anyone's else rights by being a derivative work. In other words, we need you to confirm that your contribution satisfies the requirements under https://github.com/pendulum-project/ntpd-rs/blob/main/CONTRIBUTING.md#respect-free-softwareopen-source-licenses

If you could do that for us that would clear the way for reviewing this. Otherwise, we unfortunately will have to respectfully decline this contribution.

@paulgear
Copy link
Contributor Author

paulgear commented Sep 6, 2025

However, given the way this was built and the extent to which this was built using AI we unfortunately will have to require you to explicitly confirm that despite that, you are sure that you have the rights to this contribution and that it is not infringing on anyone's else rights by being a derivative work. In other words, we need you to confirm that your contribution satisfies the requirements under https://github.com/pendulum-project/ntpd-rs/blob/main/CONTRIBUTING.md#respect-free-softwareopen-source-licenses

Thanks for the reply, @davidv1992.

I am not an intellectual property lawyer, so I can't definitively affirm anything relating to rights or infringements of this code or any other code. My understanding as a non-professional is that the copyright status of AI-generated code is still unknown (at least in U.S. legal jurisdictions).

Here's what I can affirm:

I hope the above satisfies your requirements.

@davidv1992
Copy link
Member

Just as a heads up, we are internally discussing the path forward here, but this may take a little while as this is still a bit of an edge case for us.

@paulgear
Copy link
Contributor Author

paulgear commented Sep 9, 2025

Just as a heads up, we are internally discussing the path forward here, but this may take a little while as this is still a bit of an edge case for us.

No worries - thanks for the update.

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