diff --git a/code-reviews/Driver.AbLegacy/findings.md b/code-reviews/Driver.AbLegacy/findings.md index 67c92c3..b603540 100644 --- a/code-reviews/Driver.AbLegacy/findings.md +++ b/code-reviews/Driver.AbLegacy/findings.md @@ -266,7 +266,7 @@ rethrowing, so a failed initialise leaves no live background work. | Severity | Medium | | Category | Error handling & resilience | | Location | `AbLegacyStatusMapper.cs:26-56` | -| Status | Open | +| Status | Resolved | **Description:** `MapLibplctagStatus` maps the integer codes -5/-7/-14/-16/-17. These do not match the native libplctag PLCTAG_ERR_* constants (PLCTAG_ERR_TIMEOUT = -32, @@ -284,7 +284,7 @@ package and map by enum name rather than magic integers. Either wire `MapPcccSta into a real PCCC-STS path or delete it as dead code. The same defect exists in `AbCipStatusMapper` and should be fixed in lockstep. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `MapLibplctagStatus` now casts to `libplctag.Status` and switches on named enum members (matching the AbCip mapper pattern); `MapPcccStatus` retained with a comment documenting it as a reference mapping for future PCCC-STS inspection; tests updated to use `Status` enum members. ### Driver.AbLegacy-011 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs index 70d17e9..c6d442f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs @@ -1,3 +1,5 @@ +using libplctag; + namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; /// @@ -20,28 +22,42 @@ public static class AbLegacyStatusMapper public const uint BadTypeMismatch = 0x80730000u; /// - /// Map libplctag return/status codes. Same polarity as the AbCip mapper — 0 success, - /// positive pending, negative error families. + /// Map a libplctag return/status code to an OPC UA StatusCode. The integer passed here + /// is (int)Tag.GetStatus() — the underlying value of the libplctag.NET + /// enum. Delegates to the strongly-typed overload so the mapping + /// stays correct regardless of how the wrapper renumbers native PLCTAG_ERR_* constants + /// in future releases. /// - public static uint MapLibplctagStatus(int status) + public static uint MapLibplctagStatus(int status) => MapLibplctagStatus((Status)status); + + /// + /// Map a libplctag.NET enum value to an OPC UA StatusCode. This is + /// the canonical core; the int overload exists only for the + /// seam which boxes the enum as an int. + /// + public static uint MapLibplctagStatus(Status status) => status switch { - if (status == 0) return Good; - if (status > 0) return GoodMoreData; - return status switch - { - -5 => BadTimeout, - -7 => BadCommunicationError, - -14 => BadNodeIdUnknown, - -16 => BadNotWritable, - -17 => BadOutOfRange, - _ => BadCommunicationError, - }; - } + Status.Ok => Good, + Status.Pending => GoodMoreData, + Status.ErrorTimeout => BadTimeout, + Status.ErrorNotFound or Status.ErrorNoMatch or Status.ErrorBadDevice => BadNodeIdUnknown, + Status.ErrorNotAllowed => BadNotWritable, + Status.ErrorOutOfBounds or Status.ErrorTooLarge or Status.ErrorTooSmall => BadOutOfRange, + Status.ErrorUnsupported or Status.ErrorNotImplemented => BadNotSupported, + Status.ErrorBadConnection or Status.ErrorBadGateway or Status.ErrorBadReply + or Status.ErrorWinsock or Status.ErrorOpen or Status.ErrorClose + or Status.ErrorRead or Status.ErrorWrite or Status.ErrorRemoteErr + or Status.ErrorPartial or Status.ErrorAbort => BadCommunicationError, + _ => BadCommunicationError, + }; /// /// Map a PCCC STS (status) byte. Common codes per AB PCCC reference: /// 0x00 = success, 0x10 = illegal command, 0x20 = bad address, 0x30 = protected, /// 0x40 = programmer busy, 0x50 = file locked, 0xF0 = extended status follows. + /// libplctag surfaces only its own enum rather than exposing + /// the raw STS byte, so this method is not wired into the current read/write path. + /// It is retained as the reference mapping for future PCCC-STS inspection. /// public static uint MapPcccStatus(byte sts) => sts switch { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyHostAndStatusTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyHostAndStatusTests.cs index eb76a1a..6a53630 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyHostAndStatusTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyHostAndStatusTests.cs @@ -1,3 +1,4 @@ +using libplctag; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; @@ -53,16 +54,38 @@ public sealed class AbLegacyHostAndStatusTests AbLegacyStatusMapper.MapPcccStatus(sts).ShouldBe(expected); } + // Driver.AbLegacy-010 — tests use the libplctag.NET Status enum members (what + // (int)Tag.GetStatus() actually returns) rather than the unverified magic integers + // that predated this fix (-5/-7/-14/-16/-17 matched neither native PLCTAG_ERR_* + // constants nor the .NET wrapper enum ordinals reliably). [Theory] - [InlineData(0, AbLegacyStatusMapper.Good)] - [InlineData(1, AbLegacyStatusMapper.GoodMoreData)] - [InlineData(-5, AbLegacyStatusMapper.BadTimeout)] - [InlineData(-7, AbLegacyStatusMapper.BadCommunicationError)] - [InlineData(-14, AbLegacyStatusMapper.BadNodeIdUnknown)] - [InlineData(-16, AbLegacyStatusMapper.BadNotWritable)] - [InlineData(-17, AbLegacyStatusMapper.BadOutOfRange)] - public void LibplctagStatus_maps_known_codes(int status, uint expected) + [InlineData(Status.Ok, AbLegacyStatusMapper.Good)] + [InlineData(Status.Pending, AbLegacyStatusMapper.GoodMoreData)] + [InlineData(Status.ErrorTimeout, AbLegacyStatusMapper.BadTimeout)] + [InlineData(Status.ErrorNotFound, AbLegacyStatusMapper.BadNodeIdUnknown)] + [InlineData(Status.ErrorNoMatch, AbLegacyStatusMapper.BadNodeIdUnknown)] + [InlineData(Status.ErrorNotAllowed, AbLegacyStatusMapper.BadNotWritable)] + [InlineData(Status.ErrorOutOfBounds, AbLegacyStatusMapper.BadOutOfRange)] + [InlineData(Status.ErrorTooLarge, AbLegacyStatusMapper.BadOutOfRange)] + [InlineData(Status.ErrorBadConnection, AbLegacyStatusMapper.BadCommunicationError)] + [InlineData(Status.ErrorBadGateway, AbLegacyStatusMapper.BadCommunicationError)] + [InlineData(Status.ErrorUnsupported, AbLegacyStatusMapper.BadNotSupported)] + [InlineData(Status.ErrorNoMem, AbLegacyStatusMapper.BadCommunicationError)] // unmapped → generic comms + public void LibplctagStatus_maps_real_enum_members(Status status, uint expected) { AbLegacyStatusMapper.MapLibplctagStatus(status).ShouldBe(expected); + // The int overload must agree — it is the seam IAbLegacyTagRuntime.GetStatus() drives. + AbLegacyStatusMapper.MapLibplctagStatus((int)status).ShouldBe(expected); + } + + [Fact] + public void MapLibplctagStatus_distinguishes_timeout_from_generic_comms_error() + { + // Regression for Driver.AbLegacy-010: timeout must not fall through to + // BadCommunicationError the way the old magic-integer switch did. + AbLegacyStatusMapper.MapLibplctagStatus((int)Status.ErrorTimeout) + .ShouldBe(AbLegacyStatusMapper.BadTimeout); + AbLegacyStatusMapper.MapLibplctagStatus((int)Status.ErrorNotFound) + .ShouldBe(AbLegacyStatusMapper.BadNodeIdUnknown); } }