fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-010)
MapLibplctagStatus now casts the int to libplctag.Status and switches on named enum members (mirroring AbCipStatusMapper) instead of unverified magic integers. A strongly-typed Status overload is the canonical path; the int overload delegates to it. MapPcccStatus is retained with a comment marking it as the reference mapping for future PCCC-STS inspection. Tests updated to use Status enum members rather than raw integers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -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
|
||||
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
using libplctag;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy;
|
||||
|
||||
/// <summary>
|
||||
@@ -20,28 +22,42 @@ public static class AbLegacyStatusMapper
|
||||
public const uint BadTypeMismatch = 0x80730000u;
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>(int)Tag.GetStatus()</c> — the underlying value of the libplctag.NET
|
||||
/// <see cref="Status"/> 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.
|
||||
/// </summary>
|
||||
public static uint MapLibplctagStatus(int status)
|
||||
public static uint MapLibplctagStatus(int status) => MapLibplctagStatus((Status)status);
|
||||
|
||||
/// <summary>
|
||||
/// Map a libplctag.NET <see cref="Status"/> enum value to an OPC UA StatusCode. This is
|
||||
/// the canonical core; the <c>int</c> overload exists only for the
|
||||
/// <see cref="IAbLegacyTagRuntime.GetStatus"/> seam which boxes the enum as an int.
|
||||
/// </summary>
|
||||
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,
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="Status"/> 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.
|
||||
/// </summary>
|
||||
public static uint MapPcccStatus(byte sts) => sts switch
|
||||
{
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user