P0: make SIPROTEC Clock Sync survive Windows UDP/123 conflicts - #152
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45afd3203b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| var options = new ProcessBusCaptureOptions | ||
| { | ||
| Filter = "udp dst port 123", |
There was a problem hiding this comment.
On an 802.1Q/802.1ad station bus, the bare udp dst port 123 BPF expression rejects tagged Ethernet frames before TryParseClientRequest can inspect them because the IPv4/UDP offsets are shifted by the VLAN header. Consequently, the newly added VLAN parsing and tag-preserving reply path is unreachable and RAW Clock Sync receives no client requests on those networks; include VLAN-aware filter alternatives as the existing process-bus capture filters do.
Useful? React with 👍 / 👎.
| SetState( | ||
| SntpClockServiceState.Serving, | ||
| binding.DirectedBroadcast == null | ||
| ? $"SNTP unicast server active on {binding.LocalAddress}:123 with SIPROTEC compatibility stratum {_profile.Stratum}. This subnet has no usable directed broadcast address." | ||
| : $"SNTP server active on {binding.LocalAddress}:123 with SIPROTEC compatibility stratum {_profile.Stratum}; Mode 5 broadcast targets {binding.DirectedBroadcast}:123."); | ||
|
|
||
| _receiveTask = ReceiveLoopAsync(udp, _serviceCancellation.Token); | ||
| _broadcastTask = BroadcastLoopAsync(udp, binding, _serviceCancellation.Token); | ||
| RequestImmediateBroadcast(); | ||
| return Task.CompletedTask; | ||
| SntpClockServiceState.Faulted, | ||
| $"Npcap RAW SNTP capture failed: {exception.Message}. IEC 61850 remains unaffected."); |
There was a problem hiding this comment.
When the Npcap capture loop terminates with a runtime error, this handler only changes the displayed state and leaves _rawTransport non-null. Subsequent EnsureStartedAsync calls therefore take the existing-transport shortcut and never reopen capture, so Mode 3 requests remain unanswered until the user manually disables Clock Sync or restarts the application; tear down the faulted transport or make the active check account for a completed capture task.
Useful? React with 👍 / 👎.
1 participant
What changed
Root cause
ARSAS 1.6.24 treated exclusive UDP/123 ownership as a hard prerequisite. On normal Windows commissioning laptops, W32Time can already own that port, so IEC 61850 stayed connected while SNTP silently degraded to
PortUnavailable. The previous UX also made it too easy to interpret a Mode 5 broadcast as successful synchronization.Safety invariants
Validation
Final head
45afd3203bfbbd5cf1ef0f1ec415ae00025a057epassed:An earlier CI pass exposed one
ReadOnlyMemory<byte>→ReadOnlySpan<byte>integration mismatch at the Npcap capture boundary; it was corrected to usecaptured.Frame.Spanand the final head is green.