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
This commit is contained in:
Joseph Doherty
2026-10-22 15:19:28 -04:00
parent 620ecc41dd
commit f827b3a134
9 changed files with 191 additions and 4 deletions
@@ -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,
@@ -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(
@@ -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; }
}
/// <summary>The data type an authored tag gets when its <c>dataType</c> key is absent.</summary>
/// <param name="region">The tag's region.</param>
/// <returns><c>Bool</c> for Coils / DiscreteInputs, <c>Int16</c> for the register regions.</returns>
public static ModbusDataType DefaultDataType(ModbusRegion region)
=> region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs ? ModbusDataType.Bool : ModbusDataType.Int16;
/// <summary>
/// Region/type compatibility, the one rule every authoring path and the driver share: Coils and
/// DiscreteInputs carry only <c>Bool</c>; HoldingRegisters and InputRegisters carry every register
/// type (including <c>BitInRegister</c>) but never <c>Bool</c>.
/// </summary>
/// <param name="region">The tag's region.</param>
/// <param name="dataType">The tag's data type.</param>
/// <returns><see langword="true"/> when the driver can read the pair.</returns>
public static bool IsRegionTypeCompatible(ModbusRegion region, ModbusDataType dataType)
=> region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs
? dataType == ModbusDataType.Bool
: dataType != ModbusDataType.Bool;
/// <summary>
/// Inverse of <see cref="FromTagConfig"/>: serialises a typed definition back to the camelCase
/// <c>TagConfig</c> 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)
{
@@ -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;
@@ -5,7 +5,7 @@
<div class="row g-2">
<div class="col-md-4"><label class="form-label">Region</label>
<select class="form-select form-select-sm" value="@_m.Region" @onchange="@(e => Update(() => _m.Region = ParseEnum(e.Value, ModbusRegion.HoldingRegisters)))">
<select class="form-select form-select-sm" value="@_m.Region" @onchange="@(e => Update(() => _m.SetRegion(ParseEnum(e.Value, ModbusRegion.HoldingRegisters))))">
@foreach (var v in Enum.GetValues<ModbusRegion>()) { <option value="@v">@v</option> }
</select></div>
<div class="col-md-2"><label class="form-label">Address</label>
@@ -30,6 +30,19 @@ public sealed class ModbusTagConfigModel
/// key stays absent.</summary>
public bool? Writable { get; set; }
/// <summary>
/// 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.
/// </summary>
/// <param name="region">The newly selected region.</param>
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"),
@@ -25,10 +25,43 @@ public sealed class ModbusArrayWriteSupportParityTests
return data;
}
/// <summary>
/// Shapes where the driver has moved ahead of the server-side table (review 07 P6 / U3, Modbus lane) and
/// <see cref="DriverArrayWriteSupport"/> — 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.
/// <see cref="Pending_F1_divergences_still_exist"/> fails the moment the table follows, so this list
/// is removed with it rather than left to rot.
/// </summary>
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<ModbusRegion, ModbusDataType> PendingShapes()
{
var data = new TheoryData<ModbusRegion, ModbusDataType>();
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(
@@ -0,0 +1,70 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests;
/// <summary>
/// Review 07 P6: region/type compatibility is enforced at the tag factory (and so at the deploy gate
/// and the AdminUI parse consult). A <c>HoldingRegisters</c> + <c>Bool</c> tag used to throw out of the
/// coalescing planner and kill the whole subscription's poll cycle; <c>Coils</c> + <c>Int16</c> silently
/// returned a bool into an Int32 node.
/// </summary>
[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");
}
}
@@ -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();
/// <summary>Review 07 P6: switching region keeps the data type one the driver accepts for it.</summary>
[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();
}
/// <summary>A Coils + Int16 blob (CSV / API) is refused at authoring, matching the driver.</summary>
[Fact]
public void Validate_refuses_an_incompatible_region_and_type()
=> ModbusTagConfigModel.FromJson("""{"region":"Coils","address":1,"dataType":"Int16"}""").Validate().ShouldNotBeNull();
}