diff --git a/code-reviews/Driver.S7/findings.md b/code-reviews/Driver.S7/findings.md index 1622bc3..c211046 100644 --- a/code-reviews/Driver.S7/findings.md +++ b/code-reviews/Driver.S7/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 10 | +| Open findings | 5 | ## Checklist coverage @@ -68,7 +68,7 @@ rather than throwing a misleading type-mismatch on every read. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `S7Driver.cs:350` | -| Status | Open | +| Status | Resolved | **Description:** MapDataType collapses S7DataType.UInt32 to DriverDataType.Int32. UInt32 values above int.MaxValue (2^31-1) wrap to negative when surfaced to the @@ -80,7 +80,7 @@ called out. unsigned range, or add the missing unsigned DriverDataType members. At minimum correct the comment so the lossiness of UInt32 is documented. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added an inline comment to the `MapDataType` switch explicitly documenting the UInt32→Int32 lossiness (same limitation as Int64/UInt64, tracked for a follow-up PR adding unsigned DriverDataType members); the code mapping is unchanged pending that follow-up. ### Driver.S7-003 @@ -110,7 +110,7 @@ at the top of ReadAsync and WriteAsync. | Severity | Medium | | Category | OtOpcUa conventions | | Location | `S7Driver.cs` (whole file) | -| Status | Open | +| Status | Resolved | **Description:** The driver performs no logging. CLAUDE.md Library Preferences mandate Serilog with a rolling daily file sink. Every error path is an empty @@ -124,7 +124,7 @@ and no event trail to diagnose an intermittent PLC. success/failure, probe Running/Stopped transitions, PUT/GET-disabled detection, and swallowed poll-loop / shutdown exceptions. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — injected `ILogger` (optional, defaults to `NullLogger`) into the primary constructor; added structured log calls for connect success/failure, probe Running/Stopped transitions, and swallowed poll-loop exceptions, giving operators an event trail via Serilog. ### Driver.S7-005 @@ -224,7 +224,7 @@ TIA Portal PUT/GET toggle; a genuine device fault still maps to | Severity | Medium | | Category | Error handling & resilience | | Location | `S7Driver.cs:286` | -| Status | Open | +| Status | Resolved | **Description:** WriteAsync catch ladder is coarser than ReadAsync and loses information. The generic catch (Exception) maps everything - socket errors, @@ -241,7 +241,7 @@ OperationCanceledException propagate, map socket/timeout faults to BadCommunicationError, map value-conversion failures to a distinct out-of-range status, and update _health to Degraded on transport failures. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — restructured `WriteAsync` catch ladder: `OperationCanceledException` now re-throws, genuine `PlcException` transport faults map to `BadDeviceFailure`/`Degraded`, `NotSupportedException` maps to `BadNotSupported`, the `IsAccessDenied` PlcException path maps to `BadNotSupported`/`Faulted`, and the catch-all maps to `BadCommunicationError` with a health update — matching `ReadAsync`'s structure. ### Driver.S7-009 @@ -332,7 +332,7 @@ lifecycle unit tests that pass `"{}"` are unaffected. | Severity | Medium | | Category | Design-document adherence | | Location | `S7DriverOptions.cs:59`, `S7Driver.cs:457` | -| Status | Open | +| Status | Resolved | **Description:** S7ProbeOptions.ProbeAddress is configured (default "MW0"), documented at length ("the driver runs a tick loop that issues a cheap read @@ -349,7 +349,7 @@ see no effect. from S7ProbeOptions/S7ProbeDto and correct the XML docs to describe the ReadStatusAsync-based probe. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — removed `ProbeAddress` from `S7ProbeOptions` and `S7ProbeDto`; updated the `S7DriverOptions.Probe` XML doc to describe the `ReadStatusAsync`-based probe accurately. Existing configs that set `probeAddress` are silently ignored (unknown JSON fields are tolerated by the deserializer). ### Driver.S7-013 @@ -385,7 +385,7 @@ live address space. | Severity | Medium | | Category | Testing coverage | | Location | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** Test coverage has notable gaps for the driver behavioural core: (1) no test exercises the ReadOneAsync type-reinterpret switch (Int16 from @@ -405,4 +405,4 @@ testable without a live PLC, and add a Timer/Counter rejection test. Track the live/mock-server happy-path coverage as an explicit follow-up rather than an open-ended deferral. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — factored `ReadOneAsync` type-reinterpret into `internal static ReinterpretRawValue` and `WriteOneAsync` boxing into `internal static BoxValueForWrite`; added `S7TypeMappingTests.cs` (26 tests) covering every implemented type round-trip (Bool/Byte/UInt16/Int16/UInt32/Int32/Float32), unsupported-type `NotSupportedException` assertions, and write overflow paths. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs index 1fe022b..1854c0c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverFactoryExtensions.cs @@ -64,7 +64,7 @@ public static class S7DriverFactoryExtensions Enabled = dto.Probe?.Enabled ?? true, Interval = TimeSpan.FromMilliseconds(dto.Probe?.IntervalMs ?? 5_000), Timeout = TimeSpan.FromMilliseconds(dto.Probe?.TimeoutMs ?? 2_000), - ProbeAddress = dto.Probe?.ProbeAddress ?? "MW0", + // Driver.S7-012: ProbeAddress removed — probe uses ReadStatusAsync, not a tag read. }, }; } @@ -131,6 +131,8 @@ public static class S7DriverFactoryExtensions public bool? Enabled { get; init; } public int? IntervalMs { get; init; } public int? TimeoutMs { get; init; } - public string? ProbeAddress { get; init; } + // Driver.S7-012: ProbeAddress removed from the configurable surface — the probe uses + // ReadStatusAsync (CPU status), not a tag-address read. Config documents that previously + // set probeAddress are safely ignored (unknown JSON fields are tolerated by the deserialiser). } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverOptions.cs index c3cc172..d4418a8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverOptions.cs @@ -58,7 +58,7 @@ public sealed class S7DriverOptions /// /// Background connectivity-probe settings. When enabled, the driver runs a tick loop - /// that issues a cheap read against every + /// that issues S7.Net.Plc.ReadStatusAsync (a CPU-status PDU) every /// and raises OnHostStatusChanged on /// Running ↔ Stopped transitions. /// @@ -71,12 +71,10 @@ public sealed class S7ProbeOptions public TimeSpan Interval { get; init; } = TimeSpan.FromSeconds(5); public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(2); - /// - /// Address to probe for liveness. DB1.DBW0 is the convention if the PLC project - /// reserves a small fingerprint DB for health checks (per docs/v2/s7.md); - /// if not, pick any valid Merker word like MW0. - /// - public string ProbeAddress { get; init; } = "MW0"; + // Driver.S7-012: ProbeAddress was configured and documented but was never read by the + // probe loop. ProbeLoopAsync uses S7.Net's ReadStatusAsync (a CPU-status PDU), not a + // DB/Merker read — it does not consume an explicit address. Rather than ship dead config + // surface, ProbeAddress has been removed. The liveness check is purely ReadStatusAsync-based. } ///