From f827b3a1349b31483f23a9dd9f6f1891e1677593 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:19:28 -0400 Subject: [PATCH] fix(modbus): enforce region/type compatibility at the tag factory P6: Coils/DiscreteInputs carry only Bool and register regions never Bool; the factory (and so the deploy-gate Inspect and the AdminUI parse consult) refuses a mismatch, and an absent dataType defaults per region (Bool for a bit region). A HoldingRegisters+Bool tag used to throw out of the coalescing planner and stall every tag of the subscription; the planner also skips any incompatible pair defensively. The tag editor keeps the type compatible when the region changes; the CLI refuses the pair up front. The array-write parity test records the Coils divergence from DriverArrayWriteSupport (owned outside this lane) as follow-up F1. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Commands/ReadCommand.cs | 8 +++ .../Commands/WriteCommand.cs | 3 + .../ModbusTagDefinitionFactory.cs | 39 ++++++++++- .../ModbusDriver.cs | 3 + .../TagEditors/ModbusTagConfigEditor.razor | 2 +- .../Uns/TagEditors/ModbusTagConfigModel.cs | 17 ++++- .../ModbusArrayWriteSupportParityTests.cs | 33 +++++++++ .../ModbusRegionTypeValidationTests.cs | 70 +++++++++++++++++++ .../Uns/ModbusTagConfigModelTests.cs | 20 ++++++ 9 files changed, 191 insertions(+), 4 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusRegionTypeValidationTests.cs diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs index a2378a55..3b48f4d1 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs @@ -57,6 +57,14 @@ public sealed class ReadCommand : ModbusCommandBase ValidateEndpoint(); var ct = console.RegisterCancellationHandler(); + // The driver's tag factory refuses an incompatible region/type pair (the tag would read + // BadNodeIdUnknown) — say why up front instead. + if (!ModbusTagDefinitionFactory.IsRegionTypeCompatible(Region, DataType)) + throw new CliFx.Exceptions.CommandException( + Region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs + ? $"Region '{Region}' only carries Bool values (--type Bool); got '{DataType}'." + : $"Region '{Region}' carries register types; use --type BitInRegister for a bit of a register, not '{DataType}'."); + var tagName = SynthesiseTagName(Region, Address, DataType); var tag = new ModbusTagDefinition( Name: tagName, diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs index 07bb3bf7..e1876a49 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs @@ -74,6 +74,9 @@ public sealed class WriteCommand : ModbusCommandBase throw new CliFx.Exceptions.CommandException( $"Region 'Coils' only supports boolean values (--type Bool). " + $"Type '{DataType}' cannot represent a single-bit coil write."); + if (!ModbusTagDefinitionFactory.IsRegionTypeCompatible(Region, DataType)) + throw new CliFx.Exceptions.CommandException( + $"Region '{Region}' carries register types; use --type BitInRegister for a bit of a register, not '{DataType}'."); var tagName = ReadCommand.SynthesiseTagName(Region, Address, DataType); var tag = new ModbusTagDefinition( diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs index 02e2ff34..86b0a4e9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs @@ -50,7 +50,13 @@ public static class ModbusTagDefinitionFactory // Strict enum reads: a present-but-invalid (typo'd) enum rejects the tag // (→ BadNodeIdUnknown) instead of silently defaulting to a wrong-width Good. if (!TagConfigJson.TryReadEnumStrict(root, "region", ModbusRegion.HoldingRegisters, out var region)) return false; - if (!TagConfigJson.TryReadEnumStrict(root, "dataType", ModbusDataType.Int16, out var dataType)) return false; + // An absent dataType defaults per region: a bit region (Coils / DiscreteInputs) only carries + // Bool, so defaulting it to Int16 would reject every bit tag authored without the key. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", DefaultDataType(region), out var dataType)) return false; + // Region/type compatibility: a bit region carries only Bool, a register region never Bool. + // A mismatch used to throw out of the coalescing planner (killing every tag's poll) or + // silently return a bool into an Int32 node. + if (!IsRegionTypeCompatible(region, dataType)) return false; if (!TagConfigJson.TryReadEnumStrict(root, "byteOrder", ModbusByteOrder.BigEndian, out var byteOrder)) return false; if (!TagConfigJson.TryReadEnumStrict(root, "stringByteOrder", ModbusStringByteOrder.HighByteFirst, out var stringByteOrder)) return false; var bitIndex = (byte)ReadInt(root, "bitIndex"); @@ -94,6 +100,25 @@ public static class ModbusTagDefinitionFactory catch (InvalidOperationException) { return false; } } + /// The data type an authored tag gets when its dataType key is absent. + /// The tag's region. + /// Bool for Coils / DiscreteInputs, Int16 for the register regions. + public static ModbusDataType DefaultDataType(ModbusRegion region) + => region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs ? ModbusDataType.Bool : ModbusDataType.Int16; + + /// + /// Region/type compatibility, the one rule every authoring path and the driver share: Coils and + /// DiscreteInputs carry only Bool; HoldingRegisters and InputRegisters carry every register + /// type (including BitInRegister) but never Bool. + /// + /// The tag's region. + /// The tag's data type. + /// when the driver can read the pair. + public static bool IsRegionTypeCompatible(ModbusRegion region, ModbusDataType dataType) + => region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs + ? dataType == ModbusDataType.Bool + : dataType != ModbusDataType.Bool; + /// /// Inverse of : serialises a typed definition back to the camelCase /// TagConfig JSON the mapper reads. Used by authoring paths (the Client CLI) that construct a @@ -247,7 +272,17 @@ public static class ModbusTagDefinitionFactory // Type-dependent guards. The byte/ushort NARROWING is part of the rule, not an accident of the // mapper: it reads `(byte)bitIndex` / `(ushort)stringLength`, so 256 is bit 0 and is ACCEPTED. // Describing the un-narrowed value here would call a blob invalid that the driver serves. - if (!TagConfigJson.TryReadEnumStrict(root, "dataType", ModbusDataType.Int16, out var dataType)) return; + var region = TagConfigJson.TryReadEnumStrict(root, "region", ModbusRegion.HoldingRegisters, out var r) + ? r : ModbusRegion.HoldingRegisters; + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", DefaultDataType(region), out var dataType)) return; + + if (root.TryGetProperty("region", out _) && !IsRegionTypeCompatible(region, dataType)) + { + warnings.Add(region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs + ? $"Modbus region {region} only carries Bool tags; got {dataType} — the tag is rejected (BadNodeIdUnknown)." + : $"Modbus Bool tags only live in Coils / DiscreteInputs; a {region} tag must use a register type " + + "(use BitInRegister for a bit of a register) — the tag is rejected (BadNodeIdUnknown)."); + } if (dataType == ModbusDataType.String && (ushort)ReadInt(root, "stringLength") < 1) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 648e4c99..eedfb70b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -1186,6 +1186,9 @@ public sealed class ModbusDriver { if (!table.TryGetValue(fullReferences[i], out var tag)) continue; if (tag.CoalesceProhibited) continue; + // Defensive: the factory refuses incompatible pairs, but the planner's register math must never + // throw for a tag that slipped through — one bad tag would otherwise fail the whole batch. + if (!ModbusTagDefinitionFactory.IsRegionTypeCompatible(tag.Region, tag.DataType)) continue; if (tag.ArrayCount.HasValue) continue; if (tag.Region is not (ModbusRegion.HoldingRegisters or ModbusRegion.InputRegisters)) continue; if (tag.DataType is ModbusDataType.String or ModbusDataType.BitInRegister) continue; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor index 116f9bd5..a3b38dd4 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor @@ -5,7 +5,7 @@
- @foreach (var v in Enum.GetValues()) { }
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs index 323830ab..29050fc4 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs @@ -30,6 +30,19 @@ public sealed class ModbusTagConfigModel /// key stays absent.
 public bool? Writable { get; set; } + /// + /// Changes the region and keeps the data type compatible with it (the driver refuses a bit region + /// with a non-Bool type, and a register region with Bool): switching to Coils / DiscreteInputs selects + /// Bool, switching from them back to a register region selects Int16. + /// + /// The newly selected region. + public void SetRegion(ModbusRegion region) + { + Region = region; + if (!ModbusTagDefinitionFactory.IsRegionTypeCompatible(region, DataType)) + DataType = ModbusTagDefinitionFactory.DefaultDataType(region); + } + private JsonObject _bag = new(); // The blob this model was LOADED FROM, or null when it was built by object initialiser rather than @@ -49,7 +62,9 @@ public sealed class ModbusTagConfigModel _source = json ?? "", Region = TagConfigJson.GetEnum(o, "region", ModbusRegion.HoldingRegisters), Address = TagConfigJson.GetInt(o, "address"), - DataType = TagConfigJson.GetEnum(o, "dataType", ModbusDataType.Int16), + // Absent dataType defaults per region, exactly as the driver's factory does (Bool for a bit region). + DataType = TagConfigJson.GetEnum(o, "dataType", + ModbusTagDefinitionFactory.DefaultDataType(TagConfigJson.GetEnum(o, "region", ModbusRegion.HoldingRegisters))), ByteOrder = TagConfigJson.GetEnum(o, "byteOrder", ModbusByteOrder.BigEndian), BitIndex = TagConfigJson.GetInt(o, "bitIndex"), StringLength = TagConfigJson.GetInt(o, "stringLength"), diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs index a47a721a..6d2363b1 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusArrayWriteSupportParityTests.cs @@ -25,10 +25,43 @@ public sealed class ModbusArrayWriteSupportParityTests return data; } + /// + /// Shapes where the driver has moved ahead of the server-side table (review 07 P6 / U3, Modbus lane) and + /// — owned outside the Modbus lane — has not followed yet + /// (follow-up F1): the tag factory now REFUSES a Coils tag with a non-Bool type (so the driver serves no + /// such tag), and the driver now encodes String[] and BitInRegister[] holding-register writes. + /// fails the moment the table follows, so this list + /// is removed with it rather than left to rot. + /// + private static bool PendingF1(ModbusRegion region, ModbusDataType dataType) => + (region == ModbusRegion.Coils && dataType != ModbusDataType.Bool) + || (region == ModbusRegion.HoldingRegisters && dataType is ModbusDataType.String or ModbusDataType.BitInRegister && PendingU3); + + // Flipped to true by the U3 commit (String[] / BitInRegister[] encoders). + private const bool PendingU3 = false; + + public static TheoryData PendingShapes() + { + var data = new TheoryData(); + foreach (var (region, dt) in Shapes().Where(s => PendingF1(s.Region, s.DataType))) data.Add(region, dt); + return data; + } + + [Theory] + [MemberData(nameof(PendingShapes))] + public void Pending_F1_divergences_still_exist(ModbusRegion region, ModbusDataType dataType) + { + var json = $$"""{"region":"{{region}}","address":10,"dataType":"{{dataType}}","stringLength":4,"isArray":true,"arrayLength":2}"""; + var driverAccepts = region == ModbusRegion.HoldingRegisters; // String[] / BitInRegister[] encode; mistyped Coils are refused + DriverArrayWriteSupport.Supports(DriverTypeNames.Modbus, "Int16", json).ShouldNotBe(driverAccepts, + "F1 landed — DriverArrayWriteSupport now agrees with the driver for this shape; delete it from PendingF1"); + } + [Theory] [MemberData(nameof(AllShapes))] public async Task Table_agrees_with_the_driver_write_path(ModbusRegion region, ModbusDataType dataType) { + if (PendingF1(region, dataType)) return; // covered by Pending_F1_divergences_still_exist var json = $$"""{"region":"{{region}}","address":10,"dataType":"{{dataType}}","stringLength":4,"isArray":true,"arrayLength":2}"""; var fake = new ModbusDriverTests.FakeTransport(); var drv = new ModbusDriver( diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusRegionTypeValidationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusRegionTypeValidationTests.cs new file mode 100644 index 00000000..b4acdaee --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusRegionTypeValidationTests.cs @@ -0,0 +1,70 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// Review 07 P6: region/type compatibility is enforced at the tag factory (and so at the deploy gate +/// and the AdminUI parse consult). A HoldingRegisters + Bool tag used to throw out of the +/// coalescing planner and kill the whole subscription's poll cycle; Coils + Int16 silently +/// returned a bool into an Int32 node. +/// +[Trait("Category", "Unit")] +public sealed class ModbusRegionTypeValidationTests +{ + [Theory] + [InlineData("""{"region":"Coils","address":1,"dataType":"Int16"}""")] + [InlineData("""{"region":"DiscreteInputs","address":1,"dataType":"Float32"}""")] + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"Bool"}""")] + [InlineData("""{"region":"InputRegisters","address":1,"dataType":"Bool"}""")] + public void Incompatible_region_and_type_is_rejected(string json) + { + ModbusTagDefinitionFactory.FromTagConfig(json, "T", out _).ShouldBeFalse(); + ModbusTagDefinitionFactory.Inspect(json).ShouldContain(w => w.Contains("only") || w.Contains("register")); + } + + [Theory] + [InlineData("""{"region":"Coils","address":1,"dataType":"Bool"}""", ModbusDataType.Bool)] + [InlineData("""{"region":"Coils","address":1}""", ModbusDataType.Bool)] + [InlineData("""{"region":"DiscreteInputs","address":1}""", ModbusDataType.Bool)] + [InlineData("""{"region":"InputRegisters","address":1,"dataType":"BitInRegister","bitIndex":3}""", ModbusDataType.BitInRegister)] + [InlineData("""{"region":"HoldingRegisters","address":1}""", ModbusDataType.Int16)] + public void Compatible_region_and_type_is_accepted(string json, ModbusDataType expected) + { + // Positive control for the rejection theory, including the absent-dataType default per region. + ModbusTagDefinitionFactory.FromTagConfig(json, "T", out var def).ShouldBeTrue(); + def.DataType.ShouldBe(expected); + ModbusTagDefinitionFactory.Inspect(json).ShouldBeEmpty(); + } + + [Fact] + public async Task A_mis_typed_tag_no_longer_kills_the_coalesced_poll_of_its_neighbours() + { + var fake = new ModbusDriverTests.FakeTransport(); + fake.HoldingRegisters[100] = 7; + fake.HoldingRegisters[102] = 9; + var good1 = new ModbusTagDefinition("G1", ModbusRegion.HoldingRegisters, 100, ModbusDataType.Int16); + var good2 = new ModbusTagDefinition("G2", ModbusRegion.HoldingRegisters, 102, ModbusDataType.Int16); + var opts = new ModbusDriverOptions + { + Host = "f", + MaxReadGap = 5, + RawTags = + [ + ModbusRawTags.Entry(good1), + new RawTagEntry("Bad", """{"region":"HoldingRegisters","address":101,"dataType":"Bool"}""", false), + ModbusRawTags.Entry(good2), + ], + Probe = new ModbusProbeOptions { Enabled = false }, + }; + await using var drv = new ModbusDriver(opts, "m1", _ => fake); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var r = await drv.ReadAsync(["G1", "Bad", "G2"], TestContext.Current.CancellationToken); + r[0].Value.ShouldBe((short)7); + r[1].StatusCode.ShouldBe(0x80340000u); // BadNodeIdUnknown — skipped at init, reported + r[2].Value.ShouldBe((short)9); + drv.GetSkippedTags()!.Sample.ShouldContain(s => s.RawPath == "Bad"); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs index 2bb03383..e1afd0c1 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs @@ -121,4 +121,24 @@ public sealed class ModbusTagConfigModelTests [InlineData("""{"region":3,"address":1,"dataType":true}""")] public void Validate_accepts_every_enum_value_the_driver_accepts(string json) => ModbusTagConfigModel.FromJson(json).Validate().ShouldBeNull(); + + /// Review 07 P6: switching region keeps the data type one the driver accepts for it. + [Theory] + [InlineData(ModbusDataType.Int16, ModbusRegion.Coils, ModbusDataType.Bool)] + [InlineData(ModbusDataType.Float32, ModbusRegion.DiscreteInputs, ModbusDataType.Bool)] + [InlineData(ModbusDataType.Bool, ModbusRegion.HoldingRegisters, ModbusDataType.Int16)] + [InlineData(ModbusDataType.Float32, ModbusRegion.InputRegisters, ModbusDataType.Float32)] + public void SetRegion_keeps_the_type_compatible(ModbusDataType from, ModbusRegion region, ModbusDataType expected) + { + var m = new ModbusTagConfigModel { DataType = from }; + m.SetRegion(region); + m.Region.ShouldBe(region); + m.DataType.ShouldBe(expected); + ModbusTagConfigModel.FromJson(m.ToJson()).Validate().ShouldBeNull(); + } + + /// A Coils + Int16 blob (CSV / API) is refused at authoring, matching the driver. + [Fact] + public void Validate_refuses_an_incompatible_region_and_type() + => ModbusTagConfigModel.FromJson("""{"region":"Coils","address":1,"dataType":"Int16"}""").Validate().ShouldNotBeNull(); }