Add DirectWriteAbsTime time sync procedure for non-conformant outstations#403
Add DirectWriteAbsTime time sync procedure for non-conformant outstations#403jadamcrain merged 2 commits intomainfrom
Conversation
…utstations Add support for time synchronization via direct write of Group 50 Var 1 without delay measurement. This is a workaround for outstations that do not support the DELAY_MEASURE function code. This method does not account for network latency and should only be used when standard time synchronization procedures are not supported. Closes #374
Summary of ChangesHello @jadamcrain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, non-standard time synchronization procedure, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a DirectWriteAbsTime time synchronization procedure, providing a workaround for outstations that don't support the standard delay measurement. The implementation is sound, adding a new state to the TimeSyncTask state machine and updating FFI bindings and schema definitions accordingly. The new functionality is also covered by unit tests. My feedback focuses on improving code clarity and robustness by using the unreachable! macro for code paths that are logically impossible to reach in the state machine, which is a common Rust idiom.
dnp3/src/master/tasks/time.rs
Outdated
| State::WriteAbsoluteTime(_) => FunctionCode::Write, | ||
| State::RecordCurrentTime(_) => FunctionCode::RecordCurrentTime, | ||
| State::WriteLastRecordedTime(_) => FunctionCode::Write, | ||
| State::DirectWriteAbsTime => FunctionCode::Write, |
There was a problem hiding this comment.
The DirectWriteAbsTime state is transient and should always transition to WriteAbsoluteTime within the start() method. Therefore, this match arm should be unreachable. Using the unreachable! macro here makes this assumption explicit and will cause a panic during development if this logic is ever violated. This is preferable to silently proceeding with potentially incorrect behavior and can also provide hints to the compiler for optimization.
State::DirectWriteAbsTime => unreachable!("DirectWriteAbsTime should transition to WriteAbsoluteTime in start()"),
dnp3/src/master/tasks/time.rs
Outdated
| State::WriteAbsoluteTime(x) => writer.write_count_of_one(Group50Var1 { time: x }), | ||
| State::RecordCurrentTime(_) => Ok(()), | ||
| State::WriteLastRecordedTime(x) => writer.write_count_of_one(Group50Var3 { time: x }), | ||
| State::DirectWriteAbsTime => Ok(()), // Should never be reached as it transitions in start() |
There was a problem hiding this comment.
As the comment indicates, this code path should never be reached. It's more idiomatic and safer to enforce this with the unreachable! macro. This makes the code more robust by panicking if the assumption is violated, which helps catch bugs early during development or testing.
State::DirectWriteAbsTime => unreachable!("DirectWriteAbsTime should transition to WriteAbsoluteTime in start()"),
dnp3/src/master/tasks/time.rs
Outdated
| State::DirectWriteAbsTime => { | ||
| // Should never be reached as it transitions to WriteAbsoluteTime in start() | ||
| self.handle_write_absolute_time(association, response) | ||
| } |
There was a problem hiding this comment.
Similar to the other match arms for DirectWriteAbsTime, this one is expected to be unreachable. Using unreachable! is more idiomatic and safer than implementing a behavior for a state that should not be possible at this point in the execution flow.
State::DirectWriteAbsTime => unreachable!("DirectWriteAbsTime should transition to WriteAbsoluteTime in start()"),…e state Eliminates the DirectWriteAbsTime state and all 'unreachable' code paths by using WriteAbsoluteTime(Option<Timestamp>) instead. This makes the invariant more local: timestamp must be populated before write() is called, which is enforced by start(). - WriteAbsoluteTime(None) signals timestamp should be fetched in start() - WriteAbsoluteTime(Some(timestamp)) signals timestamp is ready - write() uses expect() with clear error message if invariant is violated
1 participant
Summary
DirectWriteAbsTimevariant toTimeSyncProcedureenumImplementation Details
WriteAbsoluteTimeauto_time_syncandtime_sync_modeenumsTesting
Breaking Changes
TimeSyncProcedureas#[non_exhaustive]for future compatibilityCloses #374