diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index b162219..f4e158a 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -78,13 +78,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `AbCipDataType.cs:51-58`, `LibplctagTagRuntime.cs:47-49,53` | -| Status | Open | +| Status | Resolved | **Description:** `ToDriverDataType` maps `LInt`/`ULInt` to `DriverDataType.Int32` (a TODO comment notes the gap) and `Dt` to `Int32`. But `LibplctagTagRuntime.DecodeValueAt` returns an actual `long` for `LInt`/`ULInt` (`_tag.GetInt64`, `(long)_tag.GetUInt64`). The address space is built declaring an Int32 node while the driver hands the server a boxed `long` `DataValueSnapshot.Value` at runtime: a mismatch between the declared OPC UA data type and the runtime value type. For `LInt` values exceeding Int32.MaxValue there is data loss if any consumer narrows it. `UDInt` is declared Int32 but decoded as `(int)_tag.GetUInt32`, so values above int.MaxValue wrap to negative. **Recommendation:** Either add Int64/UInt32/UInt64 to `DriverDataType` and map correctly, or, until that lands, decode `LInt`/`ULInt` consistently with the declared `Int32` type (and document the truncation), and decode `UDInt` as a value that fits Int32 semantics. The declared type and the runtime value type must agree. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `ToDriverDataType` now maps `LInt`→`Int64`, `ULInt`→`UInt64`, `UDInt`→`UInt32` (all already present in `DriverDataType`); `DecodeValueAt` updated to return `uint`/`ulong` for UDInt/ULInt respectively so the declared type and runtime value agree. The `(int)` and `(long)` casts that caused truncation/wrap are removed. ### Driver.AbCip-005 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs index cc0b822..b0c4e89 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDataType.cs @@ -40,22 +40,29 @@ public enum AbCipDataType public static class AbCipDataTypeExtensions { /// - /// Map to the driver-agnostic type the server's address-space builder consumes. Unsigned - /// Logix types widen into signed equivalents until DriverDataType picks up unsigned - /// + 64-bit variants (Modbus has the same gap — see ModbusDriver.MapDataType - /// comment re: PR 25). + /// Map to the driver-agnostic type the server's address-space builder consumes. + /// DriverDataType carries Int64, UInt32, and UInt64 so each Logix type maps + /// to the widest correct signed/unsigned equivalent without silent truncation: + /// + /// LInt (signed 64-bit) → Int64; ULInt (unsigned 64-bit) → UInt64. + /// UDInt (unsigned 32-bit) → UInt32 so values above Int32.MaxValue are not + /// wrapped to negative (Driver.AbCip-004). + /// USInt / UInt widen into Int32; they can never overflow it. + /// /// public static DriverDataType ToDriverDataType(this AbCipDataType t) => t switch { AbCipDataType.Bool => DriverDataType.Boolean, AbCipDataType.SInt or AbCipDataType.Int or AbCipDataType.DInt => DriverDataType.Int32, - AbCipDataType.USInt or AbCipDataType.UInt or AbCipDataType.UDInt => DriverDataType.Int32, - AbCipDataType.LInt or AbCipDataType.ULInt => DriverDataType.Int32, // TODO: Int64 — matches Modbus gap + AbCipDataType.USInt or AbCipDataType.UInt => DriverDataType.Int32, + AbCipDataType.UDInt => DriverDataType.UInt32, + AbCipDataType.LInt => DriverDataType.Int64, + AbCipDataType.ULInt => DriverDataType.UInt64, AbCipDataType.Real => DriverDataType.Float32, AbCipDataType.LReal => DriverDataType.Float64, AbCipDataType.String => DriverDataType.String, AbCipDataType.Dt => DriverDataType.Int32, // epoch-seconds DINT - AbCipDataType.Structure => DriverDataType.String, // placeholder until UDT PR 6 introduces a structured kind + AbCipDataType.Structure => DriverDataType.String, // placeholder until UDT introduces a structured kind _ => DriverDataType.Int32, }; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index aeda12f..946b351 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -44,9 +44,9 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime AbCipDataType.Int => (int)_tag.GetInt16(offset), AbCipDataType.UInt => (int)_tag.GetUInt16(offset), AbCipDataType.DInt => _tag.GetInt32(offset), - AbCipDataType.UDInt => (int)_tag.GetUInt32(offset), - AbCipDataType.LInt => _tag.GetInt64(offset), - AbCipDataType.ULInt => (long)_tag.GetUInt64(offset), + AbCipDataType.UDInt => _tag.GetUInt32(offset), // UInt32 to match ToDriverDataType (Driver.AbCip-004) + AbCipDataType.LInt => _tag.GetInt64(offset), // Int64 to match ToDriverDataType (Driver.AbCip-004) + AbCipDataType.ULInt => _tag.GetUInt64(offset), // UInt64 to match ToDriverDataType (Driver.AbCip-004) AbCipDataType.Real => _tag.GetFloat32(offset), AbCipDataType.LReal => _tag.GetFloat64(offset), AbCipDataType.String => _tag.GetString(offset),