diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusDriverOptions.cs index 4f456585..fa24f271 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusDriverOptions.cs @@ -89,16 +89,24 @@ public sealed class ModbusDriverOptions public bool UseFC16ForSingleRegisterWrites { get; init; } = false; /// - /// Kill-switch for FC23 (Read/Write Multiple Registers). By default a multi-register holding - /// write (2–121 registers: 32/64-bit values, arrays, strings) is sent as ONE FC23 that writes - /// the block and reads the same block back, so the device confirms what it now holds. A unit - /// that answers FC23 with exception 01 (MELSEC QJ71MT91, S7 Modbus servers) is moved to FC16 - /// automatically, for that write and every later one; one that fails FC23 at the transport is - /// moved too (that one write is reported as a comms failure). Set true to always use - /// FC16. Default false. + /// Hard kill-switch for FC23 (Read/Write Multiple Registers): when true the driver never + /// emits FC23, even with set. Default false. + /// FC23 is OPT-IN — a config without keeps the FC16 wire + /// behaviour, so the persisted "disableFC23": false every form-saved config carries changes + /// nothing on the wire. /// public bool DisableFC23 { get; init; } = false; + /// + /// Opt-in: send a multi-register holding write (2–121 registers: 32/64-bit values, arrays, strings) + /// as ONE FC23 that writes the block and reads the same block back, so the device confirms what it + /// now holds. A unit that answers FC23 with exception 01 (MELSEC QJ71MT91, S7 Modbus servers) moves + /// to FC16 for that write and every later one until the next initialize; one that fails FC23 at the + /// transport moves too (that one write is reported as a comms failure). Ignored when + /// is set. Default false = FC16, the historical behaviour. + /// + public bool UseFC23ForMultiRegisterWrites { get; init; } = false; + /// /// Interval for the background re-probe loop that retries auto-prohibited /// coalesced ranges. When non-null, every AutoProhibitReprobeInterval @@ -110,14 +118,13 @@ public sealed class ModbusDriverOptions public TimeSpan? AutoProhibitReprobeInterval { get; init; } = null; /// - /// Block-read coalescing budget. The read planner combines tags in the same (UnitId, Region) - /// group whose addresses are at most this many registers (Coils / DiscreteInputs: bits) apart - /// into a single FC03/FC04/FC01/FC02 read, then slices the response back into per-tag values. - /// 0 coalesces only adjacent or overlapping tags (several BitInRegister tags on one - /// status word still fold into one read). Default 8. A range the device refuses is - /// auto-prohibited and read per tag, so a protected hole costs one extra round trip, not data. To - /// turn coalescing off entirely set , or exempt one tag with - /// its coalesceProhibited key. + /// Block-read coalescing budget. When non-zero, the read planner combines tags in the same + /// (UnitId, Region) group whose addresses are at most this many registers (Coils / DiscreteInputs: + /// bits) apart into a single FC03/FC04/FC01/FC02 read, then slices the response back into per-tag + /// values (several BitInRegister tags on one status word fold into one read). 0 = no + /// coalescing — an explicitly authored 0 keeps that historical meaning. Default 8 applies + /// only when the key is ABSENT. A range the device refuses is auto-prohibited and read per tag. + /// also turns it off; coalesceProhibited exempts one tag. /// public ushort MaxReadGap { get; init; } = DefaultMaxReadGap; 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 d34c55ba..633d6931 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 @@ -35,12 +35,14 @@ public static class ModbusTagDefinitionFactory => FromTagConfig(tagConfig, rawPath, ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out def); /// - /// Family-aware overload: an addressString key (the Modicon / mnemonic / family-native grammar - /// of — 40001:F:CDAB, DL205 V2000, MELSEC D100) - /// is resolved with the DRIVER's / and is - /// authoritative for region, offset, type, bit, string length, byte order and array count; the - /// editor also persists the resolved region / address keys beside it for display and for - /// readers that predate it. Without an addressString this is exactly the 3-argument overload. + /// Family-aware overload. The persisted region + address keys are AUTHORITATIVE whenever + /// present — the editor resolves an addressString with the driver's family and persists the + /// result, so a later Family / MelsecSubFamily change can never silently remap a live tag. A blob that + /// carries ONLY an addressString (CSV / API authoring, the Modicon / mnemonic / family-native + /// grammar of — 40001:F:CDAB, DL205 V2000, MELSEC + /// D100) is resolved here with the DRIVER's / + /// . Without an addressString this is exactly the 3-argument + /// overload. /// /// The authored equipment-tag TagConfig JSON. /// The tag's RawPath — becomes the definition's identity (Name). @@ -67,8 +69,9 @@ public static class ModbusTagDefinitionFactory ushort stringLength; int? arrayCount; var addressText = ReadAddressString(root, out var addressStringInvalid); - if (addressStringInvalid) return false; - if (addressText is not null) + var hasResolvedKeys = root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _); + if (!hasResolvedKeys && addressStringInvalid) return false; + if (!hasResolvedKeys && addressText is not null) { if (!ModbusAddressParser.TryParse(addressText, family, melsecSubFamily, out var parsed, out _) || parsed is null) return false; @@ -106,9 +109,12 @@ public static class ModbusTagDefinitionFactory var arrayLength = ReadInt(root, "arrayLength"); arrayCount = (isArray && arrayLength >= 1) ? arrayLength : null; } - // 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. + // Region/type compatibility. A bit region (Coils / DiscreteInputs) carries only Bool: a non-Bool + // dataType there is COERCED to Bool, never refused — the pre-#741 editor defaulted dataType to + // Int16 and persisted it for coil tags, and the old driver ignored dataType for bit regions, so + // those tags read and wrote correctly and must keep doing so. A register region with Bool is + // refused: that pair never read (it threw out of the planner and killed every tag's poll). + if (region is ModbusRegion.Coils or ModbusRegion.DiscreteInputs) dataType = ModbusDataType.Bool; if (!IsRegionTypeCompatible(region, dataType)) return false; if (!TagConfigJson.TryReadEnumStrict(root, "stringByteOrder", ModbusStringByteOrder.HighByteFirst, out var stringByteOrder)) return false; // Guard: String tags require StringLength >= 1. RegisterCount = (StringLength+1)/2, @@ -314,14 +320,15 @@ public static class ModbusTagDefinitionFactory private static void DescribeRejection(JsonElement root, List warnings) { var addressText = ReadAddressString(root, out var addressStringInvalid); - if (addressStringInvalid) + var hasResolvedKeys = root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _); + if (!hasResolvedKeys && addressStringInvalid) { warnings.Add("Modbus TagConfig 'addressString' is not a string — the tag will not resolve (BadNodeIdUnknown)."); return; } - if (addressText is not null) + if (!hasResolvedKeys && addressText is not null) { - // The addressString is authoritative; the other address keys are not read at all. + // String-only blob: the addressString is what the driver resolves. if (!ModbusAddressParser.TryParse(addressText, out _, out var error)) warnings.Add($"Modbus 'addressString' '{addressText}' does not parse under any PLC family ({error}) — " + "the tag will not resolve (BadNodeIdUnknown)."); @@ -372,12 +379,12 @@ public static class ModbusTagDefinitionFactory ? r : ModbusRegion.HoldingRegisters; if (!TagConfigJson.TryReadEnumStrict(root, "dataType", DefaultDataType(region), out var dataType)) return; - if (root.TryGetProperty("region", out _) && !IsRegionTypeCompatible(region, dataType)) + // Only a register region with Bool is refused (a bit region with any type is coerced to Bool). + if (root.TryGetProperty("region", out _) && region is not (ModbusRegion.Coils or ModbusRegion.DiscreteInputs) + && !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)."); + warnings.Add($"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 ad9b7295..83a28405 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -61,11 +61,12 @@ public sealed class ModbusDriver /// Guards , the transport/table publish and the health fields. private readonly object _stateLock = new(); - // Health is composed from two independent sides so a good write clears only a WRITE-side - // Degraded (and a good read only a read-side one). LastSuccessfulRead advances on any evidence the - // device answered — a good read, a good write, a refusal, or a probe answer. - private string? _readFault; - private string? _writeFault; + // ONE comms-fault slot for reads AND writes: a fault is a property of the shared transport, so the + // latest batch of either kind decides — a good read clears a Degraded a write's timeout raised and vice + // versa (two independent sides latched a single write blip as Degraded forever in a read-mostly plant). + // LastSuccessfulRead advances on any evidence the device answered — a good read, a good write, a + // refusal, or a probe answer. + private string? _commsFault; // Once-per-tag Warning for deterministic device refusals (exception 01/02/03, undecodable data). // Replaced (never cleared in place) on every table publish. @@ -246,16 +247,13 @@ public sealed class ModbusDriver // failure is the re-initialize window itself, which the lifecycle reports on its own. var current = _health; if (current.State is DriverState.Faulted or DriverState.Initializing or DriverState.Unknown) return; - _readFault = ex.Message; + _commsFault = ex.Message; PublishComposedHealthLocked(current.LastSuccessfulRead); } } // ---- health composition ---- - /// Which half of the driver a batch outcome came from. - private enum HealthSide { Read, Write } - /// /// Per-batch accounting: did the device answer anything, did a comms fault happen, and which units /// (or the whole endpoint) must the rest of this batch fail fast against. @@ -277,7 +275,7 @@ public sealed class ModbusDriver /// is still current. A batch that touched no device (every reference /// unknown, every write refused before the wire) changes nothing. /// - private void ReportOutcome(int generation, HealthSide side, BatchOutcome batch, DateTime now) + private void ReportOutcome(int generation, BatchOutcome batch, DateTime now) { if (!batch.Answered && batch.CommsFault is null) return; lock (_stateLock) @@ -285,16 +283,15 @@ public sealed class ModbusDriver if (generation != _generation) return; var current = _health; if (current.State is DriverState.Faulted or DriverState.Initializing or DriverState.Unknown) return; - if (side == HealthSide.Read) _readFault = batch.CommsFault; - else _writeFault = batch.CommsFault; + _commsFault = batch.CommsFault; PublishComposedHealthLocked(batch.Answered ? now : current.LastSuccessfulRead); } } - /// Writes Healthy/Degraded from the two fault sides. Caller holds . + /// Writes Healthy/Degraded from the comms-fault slot. Caller holds . private void PublishComposedHealthLocked(DateTime? lastSuccessfulRead) { - var fault = _readFault ?? _writeFault; + var fault = _commsFault; WriteHealth(fault is null ? new DriverHealth(DriverState.Healthy, lastSuccessfulRead, null) : new DriverHealth(DriverState.Degraded, lastSuccessfulRead, fault)); @@ -321,8 +318,7 @@ public sealed class ModbusDriver lock (_stateLock) { if (generation != _generation) return; - _readFault = null; - _writeFault = null; + _commsFault = null; WriteHealth(health); } } @@ -370,7 +366,10 @@ public sealed class ModbusDriver return StatusBadCommunicationError; default: batch.CommsFault ??= ex.Message; - batch.FailUnit(unit); + // Fail the unit fast only when nothing came back in time (timeout / desync / socket-level IO): + // a malformed-but-delivered reply (byte-count mismatch) proves the unit responds, so its + // siblings are still tried. + if (ex is IOException or TimeoutException or OperationCanceledException) batch.FailUnit(unit); return StatusBadCommunicationError; } } @@ -493,8 +492,7 @@ public sealed class ModbusDriver generation = ++_generation; previous = _transport; _transport = null; - _readFault = null; - _writeFault = null; + _commsFault = null; WriteHealth(new DriverHealth(DriverState.Initializing, null, null)); } @@ -728,7 +726,9 @@ public sealed class ModbusDriver var results = new DataValueSnapshot[fullReferences.Count]; var batch = new BatchOutcome(); - var coalesced = !_options.DisableReadCoalescing + // MaxReadGap 0 = coalescing OFF (its pre-lane meaning, kept for every persisted config that authored + // it); the gap-8 default applies only to an absent key. + var coalesced = !_options.DisableReadCoalescing && _options.MaxReadGap > 0 ? await ReadCoalescedAsync(transport, table, fullReferences, results, now, batch, cancellationToken).ConfigureAwait(false) : new HashSet(); @@ -774,7 +774,7 @@ public sealed class ModbusDriver // ONE health write per batch, worst-wins over the batch, generation-checked. It used to be // last-writer-wins per tag, so health flapped with tag order. - ReportOutcome(generation, HealthSide.Read, batch, now); + ReportOutcome(generation, batch, now); return results; } @@ -1503,9 +1503,9 @@ public sealed class ModbusDriver } } - // Write-side health: a comms failure degrades it (LastSuccessfulRead preserved); a good write or a - // device refusal proves the device answered and clears a write-side Degraded. Generation-checked. - ReportOutcome(generation, HealthSide.Write, batch, DateTime.UtcNow); + // Write outcome feeds the same single health slot as reads: a comms failure degrades (LastSuccessfulRead + // preserved); a good write or a device refusal proves the device answered and clears it. Generation-checked. + ReportOutcome(generation, batch, DateTime.UtcNow); return results; } @@ -1665,7 +1665,8 @@ public sealed class ModbusDriver /// /// Multi-register write via FC23 (Read/Write Multiple Registers): the device writes the block and /// returns the SAME block read back in one transaction, so the write is confirmed by the device's own - /// post-write registers. Gated by . A device that does + /// post-write registers. OPT-IN via , + /// and is the hard kill-switch. A device that does /// not implement FC23 (MELSEC QJ71MT91, S7 Modbus servers — exception 01) falls back to FC16 for this /// write and every later one on that unit, which is safe because an Illegal Function wrote nothing. /// A transport failure on FC23 is reported (its outcome is unknown, so it is not retried as FC16) and @@ -1681,7 +1682,8 @@ public sealed class ModbusDriver var unsupported = Volatile.Read(ref _fc23Unsupported); // Only genuinely multi-register writes: a single register forced onto FC16 (UseFC16ForSingleRegisterWrites) // stays on the function code the operator named. - if (_options.DisableFC23 || qty < 2 || qty > Fc23MaxWriteRegisters || unsupported.ContainsKey(unit)) return false; + if (!_options.UseFC23ForMultiRegisterWrites || _options.DisableFC23 + || qty < 2 || qty > Fc23MaxWriteRegisters || unsupported.ContainsKey(unit)) return false; var pdu = new byte[10 + 1 + bytes.Length]; pdu[0] = 0x17; @@ -1701,7 +1703,7 @@ public sealed class ModbusDriver { if (unsupported.TryAdd(unit, 0)) _logger.LogInformation( - "Modbus unit {Unit} does not implement FC23 (Illegal Function); multi-register writes use FC16. Driver={DriverInstanceId}", + "Modbus unit {Unit} does not implement FC23 (Illegal Function); multi-register writes use FC16 until the driver is re-initialized. Driver={DriverInstanceId}", unit, _driverInstanceId); return false; } @@ -1709,7 +1711,7 @@ public sealed class ModbusDriver { if (unsupported.TryAdd(unit, 0)) _logger.LogWarning(ex, - "Modbus FC23 write to unit {Unit} failed at the transport; later multi-register writes to it use FC16. Driver={DriverInstanceId}", + "Modbus FC23 write to unit {Unit} failed at the transport; later multi-register writes to it use FC16 until the driver is re-initialized (a redeploy or reconnect restores FC23). Driver={DriverInstanceId}", unit, _driverInstanceId); throw; } @@ -2318,7 +2320,13 @@ public sealed class ModbusDriver // Dispose supersedes any in-flight (abandoned) initialize up front, even when it then has to // bypass the gate: that initialize will find its generation moved on and release its own // transport instead of publishing it after this teardown. - lock (_stateLock) _generation++; + int generation; + DateTime? lastRead; + lock (_stateLock) + { + generation = ++_generation; + lastRead = _health.LastSuccessfulRead; + } if (await _lifecycleGate.WaitAsync(LifecycleGateTimeout).ConfigureAwait(false)) { try @@ -2329,14 +2337,18 @@ public sealed class ModbusDriver { _lifecycleGate.Release(); } - - return; + } + else + { + _logger.LogWarning( + "Modbus lifecycle gate still held at dispose; tearing the transport down anyway. Driver={DriverInstanceId}", + _driverInstanceId); + await TeardownAsync().ConfigureAwait(false); } - _logger.LogWarning( - "Modbus lifecycle gate still held at dispose; tearing the transport down anyway. Driver={DriverInstanceId}", - _driverInstanceId); - await TeardownAsync().ConfigureAwait(false); + // Dispose's own final health write. Bumping the generation above drops a still-running shutdown's + // Unknown write, so without this a disposed driver kept reporting whatever it last was (e.g. Healthy). + WriteLifecycleHealth(generation, new DriverHealth(DriverState.Unknown, lastRead, null)); } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs index 0886124a..e57ef5e0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriverFactoryExtensions.cs @@ -85,6 +85,7 @@ public static class ModbusDriverFactoryExtensions UseFC15ForSingleCoilWrites = dto.UseFC15ForSingleCoilWrites ?? false, UseFC16ForSingleRegisterWrites = dto.UseFC16ForSingleRegisterWrites ?? false, DisableFC23 = dto.DisableFC23 ?? false, + UseFC23ForMultiRegisterWrites = dto.UseFC23ForMultiRegisterWrites ?? false, WriteOnChangeOnly = dto.WriteOnChangeOnly ?? false, MaxReadGap = dto.MaxReadGap ?? ModbusDriverOptions.DefaultMaxReadGap, DisableReadCoalescing = dto.DisableReadCoalescing ?? false, @@ -187,6 +188,8 @@ public static class ModbusDriverFactoryExtensions public bool? UseFC16ForSingleRegisterWrites { get; init; } /// Gets or sets a value indicating whether to disable function code 23. public bool? DisableFC23 { get; init; } + /// Gets or sets a value indicating whether multi-register writes opt in to FC23 write + read-back. + public bool? UseFC23ForMultiRegisterWrites { get; init; } /// Gets or sets a value indicating whether to write only on change. public bool? WriteOnChangeOnly { get; init; } /// Gets or sets the maximum gap between register addresses for coalescing. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusSocketLifecycle.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusSocketLifecycle.cs index 34fcbdd3..3cb4fe82 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusSocketLifecycle.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusSocketLifecycle.cs @@ -103,6 +103,13 @@ public sealed class ModbusSocketLifecycle : IAsyncDisposable _stream = client.GetStream(); _client = client; client = null; // ownership transferred + if (Volatile.Read(ref _disposed) != 0) + { + // A DisposeAsync that ran between the check above and the publish has already torn down; + // tear the fresh socket down ourselves so it is never stranded for the finalizer. + await TearDownAsync().ConfigureAwait(false); + throw new ObjectDisposedException(nameof(ModbusSocketLifecycle)); + } _lastSuccessUtc = DateTime.UtcNow; } catch (OperationCanceledException) when (!ct.IsCancellationRequested) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/ModbusDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/ModbusDriverForm.razor index ce1d77fc..d3325e34 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/ModbusDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/ModbusDriverForm.razor @@ -80,7 +80,7 @@
-
Default 8. 0 = adjacent tags only. Refused ranges fall back per tag.
+
0 = no coalescing. Default 8. Refused ranges fall back per tag.
@@ -112,7 +112,13 @@
- + +
+
+
+
+ +
@@ -324,6 +330,7 @@ public bool UseFC15ForSingleCoilWrites { get; set; } = false; public bool UseFC16ForSingleRegisterWrites { get; set; } = false; public bool DisableFC23 { get; set; } = false; + public bool UseFC23ForMultiRegisterWrites { get; set; } = false; public bool WriteOnChangeOnly { get; set; } = false; // Keep-alive @@ -368,6 +375,7 @@ UseFC15ForSingleCoilWrites = o.UseFC15ForSingleCoilWrites, UseFC16ForSingleRegisterWrites = o.UseFC16ForSingleRegisterWrites, DisableFC23 = o.DisableFC23, + UseFC23ForMultiRegisterWrites = o.UseFC23ForMultiRegisterWrites, WriteOnChangeOnly = o.WriteOnChangeOnly, KeepAliveEnabled = o.KeepAlive.Enabled, KeepAliveTimeSeconds = (int)o.KeepAlive.Time.TotalSeconds, @@ -404,6 +412,7 @@ UseFC15ForSingleCoilWrites = UseFC15ForSingleCoilWrites, UseFC16ForSingleRegisterWrites = UseFC16ForSingleRegisterWrites, DisableFC23 = DisableFC23, + UseFC23ForMultiRegisterWrites = UseFC23ForMultiRegisterWrites, AutoProhibitReprobeInterval = AutoProhibitReprobeIntervalSeconds > 0 ? TimeSpan.FromSeconds(AutoProhibitReprobeIntervalSeconds) : null, MaxReadGap = (ushort)Math.Clamp(MaxReadGap, 0, 65535), 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 86842227..33eae3fe 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 @@ -80,7 +80,10 @@ if (ConfigJson == _lastConfigJson) { return; } _lastConfigJson = ConfigJson; _m = ModbusTagConfigModel.FromJson(ConfigJson); - _addressText ??= _m.AddressString; + // Reset on EVERY config change: a reused editor instance switched to another tag must not show the + // previous tag's address text. + _addressText = _m.AddressString; + _addressError = null; _family = ModbusTagConfigModel.ReadFamily(GetDriverConfigJson()).Family; } 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 3ce00ad8..25f1efdd 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 @@ -32,9 +32,10 @@ public sealed class ModbusTagConfigModel /// /// The address in the parser grammar (40001:F:CDAB, DL205 V2000, MELSEC D100) when - /// the tag was authored that way; null for a tag authored by region + numeric address. When - /// present the driver re-resolves it with ITS family at runtime, so it is authoritative — editing any - /// resolved field by hand clears it (). + /// the tag was authored that way; null for a tag authored by region + numeric address. The + /// persisted resolved fields are what the driver reads (the string is only resolved by the driver for a + /// string-only blob), so editing any resolved field by hand clears it () + /// to keep the two from disagreeing. /// public string? AddressString { get; set; } @@ -130,9 +131,12 @@ public sealed class ModbusTagConfigModel _source = json ?? "", Region = TagConfigJson.GetEnum(o, "region", ModbusRegion.HoldingRegisters), Address = TagConfigJson.GetInt(o, "address"), - // 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))), + // Absent dataType defaults per region, exactly as the driver's factory does (Bool for a bit region), + // and a bit region shows Bool whatever was persisted — the driver coerces a legacy coil/DI + // "dataType":"Int16" (what the pre-#741 editor saved) to Bool, so the editor shows what is polled. + DataType = TagConfigJson.GetEnum(o, "region", ModbusRegion.HoldingRegisters) is ModbusRegion.Coils or ModbusRegion.DiscreteInputs + ? ModbusDataType.Bool + : TagConfigJson.GetEnum(o, "dataType", ModbusDataType.Int16), 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.IntegrationTests/CommsRecoveryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs index 7aeefb70..eaf68b33 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/CommsRecoveryTests.cs @@ -87,26 +87,26 @@ public sealed class CommsRecoveryTests(ModbusSimulatorFixture sim) } /// - /// U1 (DisableFC23 wired): a multi-register write on the default (FC23 write + read-back) path + /// U1 (FC23 opt-in, DisableFC23 kill-switch): a multi-register write on the opted-in FC23 write + read-back path /// round-trips against the real server — whether it implements FC23 or answers Illegal Function and - /// is moved to FC16 — and so does the forced-FC16 path. Uses the standard profile's scratch HR[200..209]. + /// is moved to FC16 — and so does the default FC16 path. Uses the standard profile's scratch HR[200..209]. /// [Theory] - [InlineData(false, 200)] - [InlineData(true, 202)] - public async Task Multi_register_write_round_trips_with_and_without_FC23(bool disableFc23, int address) + [InlineData(true, 200)] + [InlineData(false, 202)] + public async Task Multi_register_write_round_trips_with_and_without_FC23(bool useFc23, int address) { SkipUnlessStandardLive(); var tag = new ModbusTagDefinition("F", ModbusRegion.HoldingRegisters, (ushort)address, ModbusDataType.Float32); var opts = new ModbusDriverOptions { - Host = sim.Host, Port = sim.Port, UnitId = 1, Timeout = TimeSpan.FromSeconds(2), DisableFC23 = disableFc23, + Host = sim.Host, Port = sim.Port, UnitId = 1, Timeout = TimeSpan.FromSeconds(2), UseFC23ForMultiRegisterWrites = useFc23, RawTags = ModbusRawTags.Entries([tag]), Probe = new ModbusProbeOptions { Enabled = false }, }; await using var driver = new ModbusDriver(opts, "modbus-u1"); await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); - var value = disableFc23 ? 12.25f : 6.5f; + var value = useFc23 ? 6.5f : 12.25f; for (var i = 0; i < 2; i++) // second write exercises the latched path when the server lacks FC23 { var w = await driver.WriteAsync([new WriteRequest("F", value + i)], TestContext.Current.CancellationToken); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingDefaultsTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingDefaultsTests.cs index 45940c37..14c058c4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingDefaultsTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingDefaultsTests.cs @@ -49,6 +49,32 @@ public sealed class ModbusCoalescingDefaultsTests r.Select(s => (short)s.Value!).ShouldBe(Enumerable.Range(1, 10).Select(i => (short)i)); } + /// + /// Fix round 1 (coordinator ruling 2): an explicitly authored maxReadGap 0 — what the pre-#741 form + /// persisted on every driver — keeps its old meaning, NO coalescing; the gap-8 default applies only to an + /// absent key (positive control). + /// + [Theory] + [InlineData("""{"host":"h","maxReadGap":0,"disableFC23":false}""", 2)] + [InlineData("""{"host":"h"}""", 1)] + public async Task A_persisted_maxReadGap_0_keeps_coalescing_off(string legacyConfig, int expectedReads) + { + var parsed = ModbusDriverFactoryExtensions.ParseOptions("m1", legacyConfig); + var tags = new[] + { + new ModbusTagDefinition("A", ModbusRegion.HoldingRegisters, 100, ModbusDataType.Int16), + new ModbusTagDefinition("B", ModbusRegion.HoldingRegisters, 101, ModbusDataType.Int16), + }; + var (drv, t) = await StartAsync(new ModbusDriverOptions + { + Host = "f", MaxReadGap = parsed.MaxReadGap, DisableReadCoalescing = parsed.DisableReadCoalescing, + RawTags = ModbusRawTags.Entries(tags), Probe = new ModbusProbeOptions { Enabled = false }, + }); + await using var _ = drv; + await drv.ReadAsync(["A", "B"], TestContext.Current.CancellationToken); + t.Requests(0x03).Count.ShouldBe(expectedReads); + } + [Fact] public async Task DisableReadCoalescing_reads_every_tag_alone() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingTests.cs index 72542014..187176cd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusCoalescingTests.cs @@ -42,9 +42,9 @@ public sealed class ModbusCoalescingTests public ValueTask DisposeAsync() => ValueTask.CompletedTask; } - /// MaxReadGap=0 coalesces only adjacent tags: a one-register hole splits the read. + /// MaxReadGap=0 means no coalescing (its historical meaning, kept for persisted configs). [Fact] - public async Task MaxReadGap_Zero_Does_Not_Bridge_A_Gap() + public async Task MaxReadGap_Zero_Means_No_Coalescing() { var fake = new CountingTransport(); var t1 = new ModbusTagDefinition("T1", ModbusRegion.HoldingRegisters, 100, ModbusDataType.Int16); @@ -56,7 +56,7 @@ public sealed class ModbusCoalescingTests await drv.ReadAsync(["T1", "T2"], CancellationToken.None); - // A one-register gap with MaxReadGap=0: expect 2 separate FC03 reads. + // MaxReadGap=0 is coalescing OFF: expect 2 separate FC03 reads. var fc03Reads = fake.Reads.Where(r => r.Fc == 0x03).ToList(); fc03Reads.Count.ShouldBe(2); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs index 6ed8e509..4fc4f9b6 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs @@ -39,12 +39,12 @@ public sealed class ModbusFamilyWiringTests } [Fact] - public void The_address_string_carries_type_order_and_count_and_keeps_the_other_keys() + public void A_string_only_blob_carries_type_order_and_count_and_keeps_the_other_keys() { ModbusTagDefinitionFactory.FromTagConfig( - """{"addressString":"40011:F:CDAB:4","writable":false,"unitId":3,"region":"Coils","address":9}""", + """{"addressString":"40011:F:CDAB:4","writable":false,"unitId":3}""", "T", ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out var def).ShouldBeTrue(); - def.Region.ShouldBe(ModbusRegion.HoldingRegisters, "addressString is authoritative over stale resolved keys"); + def.Region.ShouldBe(ModbusRegion.HoldingRegisters); def.Address.ShouldBe((ushort)10); def.DataType.ShouldBe(ModbusDataType.Float32); def.ByteOrder.ShouldBe(ModbusByteOrder.WordSwap); @@ -53,6 +53,22 @@ public sealed class ModbusFamilyWiringTests def.UnitId.ShouldBe((byte)3); } + /// + /// Fix round 1 (review m2): persisted region/address are authoritative over an addressString, so a + /// later Family / MelsecSubFamily change can never silently remap a live tag the editor resolved. + /// + [Fact] + public void Persisted_resolved_keys_win_over_the_address_string_so_a_family_change_cannot_remap() + { + // The editor resolved D100 under MELSEC and persisted HR 100; the driver is later switched to DL205. + const string json = """{"addressString":"D100","region":"HoldingRegisters","address":100,"dataType":"Int16"}"""; + foreach (var family in new[] { ModbusFamily.MELSEC, ModbusFamily.DL205, ModbusFamily.Generic }) + { + ModbusTagDefinitionFactory.FromTagConfig(json, "T", family, MelsecFamily.Q_L_iQR, out var def).ShouldBeTrue(); + def.Address.ShouldBe((ushort)100, $"family {family}"); + } + } + [Fact] public void An_unparseable_address_string_rejects_the_tag_and_the_deploy_gate_says_why() { @@ -83,31 +99,52 @@ public sealed class ModbusFamilyWiringTests [Fact] public void Family_and_MelsecSubFamily_parse_from_the_driver_config() { - var o = ModbusDriverFactoryExtensions.ParseOptions("m1", """{"host":"h","family":"MELSEC","melsecSubFamily":"F_iQF","disableFC23":true}"""); + var o = ModbusDriverFactoryExtensions.ParseOptions("m1", + """{"host":"h","family":"MELSEC","melsecSubFamily":"F_iQF","disableFC23":true,"useFC23ForMultiRegisterWrites":true}"""); o.Family.ShouldBe(ModbusFamily.MELSEC); o.MelsecSubFamily.ShouldBe(MelsecFamily.F_iQF); o.DisableFC23.ShouldBeTrue(); + o.UseFC23ForMultiRegisterWrites.ShouldBeTrue(); } - // ---------------- DisableFC23 gates the FC23 multi-register write ---------------- + // ---------------- FC23 is OPT-IN; DisableFC23 is the kill-switch ---------------- - private static async Task<(ModbusDriver, ScriptedTransport)> StartWriteAsync(bool disableFc23) + private static async Task<(ModbusDriver, ScriptedTransport)> StartWriteAsync(string driverConfigJson) { var t = new ScriptedTransport(); - var opts = new ModbusDriverOptions - { - Host = "f", DisableFC23 = disableFc23, Probe = new ModbusProbeOptions { Enabled = false }, - RawTags = ModbusRawTags.Entries([new ModbusTagDefinition("F", ModbusRegion.HoldingRegisters, 20, ModbusDataType.Float32)]), - }; + var opts = ModbusDriverFactoryExtensions.ParseOptions("m1", driverConfigJson) is var parsed + ? new ModbusDriverOptions + { + Host = "f", DisableFC23 = parsed.DisableFC23, + UseFC23ForMultiRegisterWrites = parsed.UseFC23ForMultiRegisterWrites, + Probe = new ModbusProbeOptions { Enabled = false }, + RawTags = ModbusRawTags.Entries([new ModbusTagDefinition("F", ModbusRegion.HoldingRegisters, 20, ModbusDataType.Float32)]), + } + : null!; var drv = new ModbusDriver(opts, "m1", _ => t); await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); return (drv, t); } - [Fact] - public async Task Multi_register_writes_use_FC23_write_and_read_back_by_default() + /// + /// Fix round 1 (coordinator ruling 1): a config without the opt-in key keeps the FC16 wire — including the + /// legacy shape every form-saved config carries ("disableFC23": false). + /// + [Theory] + [InlineData("""{"host":"h"}""")] + [InlineData("""{"host":"h","disableFC23":false,"maxReadGap":0,"useFC15ForSingleCoilWrites":false}""")] + public async Task Without_the_opt_in_multi_register_writes_stay_on_FC16(string legacyConfig) { - var (drv, t) = await StartWriteAsync(disableFc23: false); + var (drv, t) = await StartWriteAsync(legacyConfig); + await using var _ = drv; + (await drv.WriteAsync([new WriteRequest("F", 1.5f)], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); + t.Sent.Select(s => s.Pdu[0]).ShouldBe([(byte)0x10]); + } + + [Fact] + public async Task Opted_in_multi_register_writes_use_FC23_write_and_read_back() + { + var (drv, t) = await StartWriteAsync("""{"host":"h","useFC23ForMultiRegisterWrites":true}"""); await using var _ = drv; var w = await drv.WriteAsync([new WriteRequest("F", 1.5f)], TestContext.Current.CancellationToken); w[0].StatusCode.ShouldBe(0u); @@ -116,18 +153,18 @@ public sealed class ModbusFamilyWiringTests } [Fact] - public async Task DisableFC23_writes_with_FC16() + public async Task DisableFC23_overrides_the_opt_in() { - var (drv, t) = await StartWriteAsync(disableFc23: true); + var (drv, t) = await StartWriteAsync("""{"host":"h","useFC23ForMultiRegisterWrites":true,"disableFC23":true}"""); await using var _ = drv; (await drv.WriteAsync([new WriteRequest("F", 1.5f)], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); t.Sent.Select(s => s.Pdu[0]).ShouldBe([(byte)0x10]); } [Fact] - public async Task A_device_without_FC23_falls_back_to_FC16_once_and_stays_on_it() + public async Task An_opted_in_device_without_FC23_falls_back_to_FC16_once_and_stays_on_it() { - var (drv, t) = await StartWriteAsync(disableFc23: false); + var (drv, t) = await StartWriteAsync("""{"host":"h","useFC23ForMultiRegisterWrites":true}"""); await using var _ = drv; t.Handler = (_, pdu, _) => pdu[0] == 0x17 ? Task.FromException(new ModbusException(0x17, 0x01, "illegal function")) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusHealthContractTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusHealthContractTests.cs index 0e591189..8d90d7f7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusHealthContractTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusHealthContractTests.cs @@ -156,8 +156,10 @@ public sealed class ModbusHealthContractTests // ---------------- H7: write status mapping + write-side health ---------------- [Fact] - public async Task Write_transport_failure_is_BadCommunicationError_and_degrades_the_write_side() + public async Task Write_transport_failure_is_BadCommunicationError_and_a_good_read_clears_it() { + // Fix round 1 (review Important #3): ONE health writer for reads and writes. A single write blip used + // to latch Degraded until the next good WRITE — forever, in a read-mostly plant. var (drv, t) = await StartAsync(Opts(Hr("A", 10))); await using var _ = drv; t.Handler = (_, pdu, _) => pdu[0] == 0x06 ? Task.FromException(new IOException("dropped")) : null; @@ -166,16 +168,49 @@ public sealed class ModbusHealthContractTests w[0].StatusCode.ShouldBe(BadCommunicationError); drv.GetHealth().State.ShouldBe(DriverState.Degraded); - // A good READ does not clear a write-side fault... (await drv.ReadAsync(["A"], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(Good); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a good read proves the shared transport answered"); + } + + [Fact] + public async Task A_good_write_clears_a_read_side_comms_fault() + { + var (drv, t) = await StartAsync(Opts(Hr("A", 10))); + await using var _ = drv; + t.Handler = (_, pdu, _) => pdu[0] == 0x03 ? Task.FromException(new IOException("dropped")) : null; + await drv.ReadAsync(["A"], TestContext.Current.CancellationToken); drv.GetHealth().State.ShouldBe(DriverState.Degraded); - // ...a good WRITE does. - t.Handler = null; (await drv.WriteAsync([new WriteRequest("A", (short)2)], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(Good); drv.GetHealth().State.ShouldBe(DriverState.Healthy); } + [Fact] + public async Task A_malformed_reply_does_not_fail_the_rest_of_the_unit_fast() + { + // Review m3: a byte-count mismatch is a reply that ARRIVED — only a timeout / desync / IO fails the unit fast. + var (drv, t) = await StartAsync(Opts(Hr("Bad", 10), Hr("Fine", 50)) is var o + ? new ModbusDriverOptions { Host = o.Host, RawTags = o.RawTags, Probe = o.Probe, DisableReadCoalescing = true } + : null!); + await using var _ = drv; + t.Bank.HoldingRegisters[50] = 9; + t.Handler = (_, pdu, _) => Addr(pdu) == 10 ? Task.FromResult(new byte[] { 0x03, 0x04, 0, 0 }) : null; + + var r = await drv.ReadAsync(["Bad", "Fine"], TestContext.Current.CancellationToken); + r[0].StatusCode.ShouldBe(BadCommunicationError); + r[1].Value.ShouldBe((short)9, "the sibling on the same unit is still read"); + } + + [Fact] + public async Task DisposeAsync_leaves_health_Unknown() + { + // Review m1: Dispose bumps the generation (dropping a concurrent shutdown's write), so it must write its own. + var (drv, _) = await StartAsync(Opts(Hr("A", 10))); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + await drv.DisposeAsync(); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + } + [Fact] public async Task A_write_refusal_counts_as_answered_and_clears_a_write_side_fault() { 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 index b4acdaee..f7139789 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusRegionTypeValidationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusRegionTypeValidationTests.cs @@ -14,14 +14,47 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; 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) + public void A_register_region_with_Bool_is_rejected(string json) { + // That pair never read (RegisterCount threw), so refusing it breaks nothing that worked. ModbusTagDefinitionFactory.FromTagConfig(json, "T", out _).ShouldBeFalse(); - ModbusTagDefinitionFactory.Inspect(json).ShouldContain(w => w.Contains("only") || w.Contains("register")); + ModbusTagDefinitionFactory.Inspect(json).ShouldContain(w => w.Contains("register")); + } + + /// + /// Fix round 1 (review Important #2): the pre-#741 editor defaulted dataType to Int16 and persisted it for + /// coil / DI tags, and the old driver ignored it for bit regions — those tags worked and must keep loading. + /// A bit region with any type is coerced to Bool and draws no deploy warning (a warning can be promoted to + /// a blocking error by TagConfigValidationMode.Error, which would refuse an existing config). + /// + [Theory] + [InlineData("""{"region":"Coils","address":3,"dataType":"Int16","byteOrder":"BigEndian","bitIndex":0,"stringLength":0}""")] + [InlineData("""{"region":"DiscreteInputs","address":1,"dataType":"Float32"}""")] + public void A_legacy_bit_region_tag_with_a_register_type_still_loads_as_Bool(string legacyJson) + { + ModbusTagDefinitionFactory.FromTagConfig(legacyJson, "T", out var def).ShouldBeTrue(); + def.DataType.ShouldBe(ModbusDataType.Bool); + ModbusTagDefinitionFactory.Inspect(legacyJson).ShouldBeEmpty(); + } + + [Fact] + public async Task A_legacy_Int16_coil_tag_reads_and_writes_the_coil() + { + var fake = new ModbusDriverTests.FakeTransport(); + fake.Coils[3] = true; + var opts = new ModbusDriverOptions + { + Host = "f", Probe = new ModbusProbeOptions { Enabled = false }, + RawTags = [new RawTagEntry("Run", """{"region":"Coils","address":3,"dataType":"Int16","byteOrder":"BigEndian","bitIndex":0,"stringLength":0}""", false)], + }; + await using var drv = new ModbusDriver(opts, "m1", _ => fake); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + + (await drv.ReadAsync(["Run"], TestContext.Current.CancellationToken))[0].Value.ShouldBe(true); + (await drv.WriteAsync([new WriteRequest("Run", false)], TestContext.Current.CancellationToken))[0].StatusCode.ShouldBe(0u); + fake.Coils[3].ShouldBeFalse(); } [Theory] 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 49abe9ab..c868b2bf 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 @@ -137,10 +137,24 @@ public sealed class ModbusTagConfigModelTests ModbusTagConfigModel.FromJson(m.ToJson()).Validate().ShouldBeNull(); } - /// A Coils + Int16 blob (CSV / API) is refused at authoring, matching the driver. + /// A register region with Bool (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(); + public void Validate_refuses_a_register_region_with_Bool() + => ModbusTagConfigModel.FromJson("""{"region":"HoldingRegisters","address":1,"dataType":"Bool"}""").Validate().ShouldNotBeNull(); + + /// + /// Fix round 1: the exact blob the pre-#741 editor persisted for a coil (dataType defaulted to Int16) + /// still validates, loads as Bool (what the driver polls), and re-saves as Bool. + /// + [Fact] + public void A_legacy_Int16_coil_blob_validates_and_loads_as_Bool() + { + const string legacy = """{"region":"Coils","address":3,"dataType":"Int16","byteOrder":"BigEndian","bitIndex":0,"stringLength":0}"""; + var m = ModbusTagConfigModel.FromJson(legacy); + m.Validate().ShouldBeNull(); + m.DataType.ShouldBe(ModbusDataType.Bool); + m.ToJson().ShouldContain("\"dataType\":\"Bool\""); + } /// Review 07 U1: the editor resolves an address string with the DRIVER's family and persists /// both the string and the resolved fields; the driver re-resolves the string with its family.