fix(driver-twincat): resolve Low code-review findings (Driver.TwinCAT-004,006,014,015,016)
- Driver.TwinCAT-004: corrected the IEC time-type inline comments; documented that the driver currently surfaces them as raw UInt32 counters. - Driver.TwinCAT-006: ResolveHost returns a documented UnresolvedHost sentinel when no devices are configured instead of returning the logical DriverInstanceId (which never matches GetHostStatuses). - Driver.TwinCAT-014: wired Probe.Timeout into the probe-loop call and added a NotificationMaxDelayMs config knob threaded through AddNotificationAsync. - Driver.TwinCAT-015: Dispose() runs a genuinely synchronous teardown with bounded waits (no sync-over-async deadlock pattern). - Driver.TwinCAT-016: pinned the Structure-tag rejection and the probe-loop vs read disposal race with regression tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 5 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -112,7 +112,7 @@ does not support UDT tags, and `BrowseSymbolsAsync` already correctly yields
|
||||
| Severity | Low |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `TwinCATDataType.cs:24-27` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The inline comments for the IEC time types are inaccurate. TwinCAT `TIME` is
|
||||
a duration (32-bit, milliseconds) — not "ms since epoch of day". `DATE` is stored as seconds
|
||||
@@ -125,7 +125,7 @@ implementer who tries to add proper conversion.
|
||||
date/time semantics are intended to be exposed properly, track a follow-up to decode them to
|
||||
`DriverDataType.DateTime`; otherwise document that they surface as raw counters.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — rewrote the inline comments to match the actual IEC 61131-3 / TwinCAT encoding (TIME = duration in ms, DATE = seconds since 1970-01-01 truncated to a day boundary, DT = seconds since 1970-01-01, TOD = ms since midnight) and added a block comment documenting that the driver surfaces them as raw UDINT counters via `DriverDataType.UInt32`. Test `Iec_time_types_map_to_uint32_raw_counter` pins the mapping.
|
||||
|
||||
### Driver.TwinCAT-005
|
||||
|
||||
@@ -156,7 +156,7 @@ catch), native-notification registration failures, and host state transitions
|
||||
| Severity | Low |
|
||||
| Category | OtOpcUa conventions |
|
||||
| Location | `TwinCATDriver.cs:406-411` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `ResolveHost` falls back to `DriverInstanceId` when there are no configured
|
||||
devices and the reference is unknown. `DriverInstanceId` is a logical config-DB identifier,
|
||||
@@ -169,7 +169,7 @@ connectivity-status row.
|
||||
empty string or a documented unresolved marker), or document why the instance ID is the chosen
|
||||
fallback. Prefer the first device HostAddress only when one exists (already done).
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `ResolveHost` now returns `TwinCATDriver.UnresolvedHostSentinel` (empty string) when no devices are configured, replacing the `DriverInstanceId` collision with `GetHostStatuses()` rows. The sentinel is publicly documented on the driver type. Updated `ResolveHost_falls_back_to_unresolved_sentinel_when_no_devices` (was `_to_DriverInstanceId_`) and added `ResolveHost_returns_unresolved_sentinel_when_no_devices` + `ResolveHost_unresolved_sentinel_matches_no_GetHostStatuses_entry` regressions.
|
||||
|
||||
### Driver.TwinCAT-007
|
||||
|
||||
@@ -361,7 +361,7 @@ part of the documented driver contract, not optional.
|
||||
| Severity | Low |
|
||||
| Category | Design-document adherence |
|
||||
| Location | `TwinCATDriverOptions.cs:41-43`, `TwinCATDriverOptions.cs:57-62`, `AdsTwinCATClient.cs:145` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Several drifts between the implemented config surface and
|
||||
`docs/v2/driver-specs.md` section 6. The spec connection-settings list has separate `Host`
|
||||
@@ -376,7 +376,7 @@ the probe path connects via `_options.Timeout` — a dead config field. The spec
|
||||
shape (the doc is DRAFT, so updating it is acceptable). Remove or wire up
|
||||
`TwinCATProbeOptions.Timeout`. Expose `NotificationMaxDelayMs` if batching control is wanted.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `TwinCATProbeOptions.Timeout` is now wired into `EnsureConnectedAsync` via an optional `timeoutOverride` parameter that the probe loop passes (reads / writes keep the driver-level `_options.Timeout`). Added a `TwinCATDriverOptions.NotificationMaxDelayMs` config knob (parsed from `driverConfigJson` via `TwinCATDriverConfigDto.NotificationMaxDelayMs`) and threaded it through `ITwinCATClient.AddNotificationAsync` so `NotificationSettings` carries the configured max-delay instead of the hard-coded 0. The `Host` / `AmsNetId` / `AmsPort` triple in the spec was already implemented as the single `HostAddress` (parsed `ads://{netId}:{port}` URI) — kept as-is to match the v2 driver convention; covered by `TwinCATAmsAddress`. Regression tests: `ProbeOptions_Timeout_is_applied_to_probe_calls`, `NotificationMaxDelayMs_is_exposed_on_driver_options`, `NotificationMaxDelayMs_parses_from_driver_config_json`.
|
||||
|
||||
### Driver.TwinCAT-015
|
||||
|
||||
@@ -385,7 +385,7 @@ shape (the doc is DRAFT, so updating it is acceptable). Remove or wire up
|
||||
| Severity | Low |
|
||||
| Category | Code organization & conventions |
|
||||
| Location | `TwinCATDriver.cs:431-432` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `Dispose()` runs `DisposeAsync().AsTask().GetAwaiter().GetResult()` —
|
||||
sync-over-async. `docs/v2/driver-stability.md` section Galaxy explicitly lists "sync-over-async
|
||||
@@ -399,7 +399,7 @@ here — cancelling token sources, disposing clients, clearing dictionaries —
|
||||
synchronous, and `PollGroupEngine.DisposeAsync` completes synchronously, so factor the
|
||||
synchronous teardown out so `Dispose()` does not block on a `Task`.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `Dispose()` now does an inline synchronous teardown with no `await` and no captured sync context: dispose native subscriptions, drive `PollGroupEngine.DisposeAsync` via `.AsTask().Wait(5s)` (no context capture), per-device `ProbeCts.Cancel()` + `ProbeTask.Wait(2s)`, `DisposeClient()` / `DisposeGate()`, then clear the dictionaries. `DisposeAsync` still routes through `ShutdownAsync` for genuinely async callers. Regression test `Dispose_does_not_block_on_async_in_default_synchronization_context` runs `Dispose()` inside a single-threaded `SynchronizationContext` that would deadlock a sync-over-async teardown and asserts it completes within 5s.
|
||||
|
||||
### Driver.TwinCAT-016
|
||||
|
||||
@@ -408,7 +408,7 @@ synchronous teardown out so `Dispose()` does not block on a `Task`.
|
||||
| Severity | Low |
|
||||
| Category | Testing coverage |
|
||||
| Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Unit coverage exists for AMS-address parsing, symbol-path parsing, read/write,
|
||||
native notifications, symbol browse, and the capability surface. Gaps tied to the findings
|
||||
@@ -423,4 +423,4 @@ without truncation (Driver.TwinCAT-002).
|
||||
addressed, especially a concurrency stress test for `EnsureConnectedAsync` and a
|
||||
`ReinitializeAsync`-applies-new-config test.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — the previously-closed High findings each grew their regression coverage as they were resolved (see `TwinCATHighFindingsRegressionTests`: `ReinitializeAsync_applies_changed_device_config` for -001, `LInt_read_round_trips_value_above_int_MaxValue` + `DataType_mapping_preserves_width_and_signedness` for -002, `Concurrent_reads_on_one_device_create_a_single_client` + `Concurrent_reads_and_writes_share_one_client` for -007, `Symbol_version_changed_raises_OnRediscoveryNeeded` + `TwinCATDriver_implements_IRediscoverable` for -013). This pass added the two remaining gaps: `Structure_typed_pre_declared_tag_is_rejected_at_config_parse` (-003) and `Probe_loop_and_read_share_one_client_per_device` (-009 disposal-race coverage races 64 readers against the probe loop for 500ms and asserts a single client / single connect). All coverage lives in the test files `TwinCATHighFindingsRegressionTests.cs` and the new `TwinCATLowFindingsRegressionTests.cs`.
|
||||
|
||||
Reference in New Issue
Block a user