diff --git a/src/displaydefinition.h b/src/displaydefinition.h index 7ca234b9..06f09f6e 100644 --- a/src/displaydefinition.h +++ b/src/displaydefinition.h @@ -33,7 +33,10 @@ struct DataViewDefinitions void normalize() { DeviceId = qMax(ModbusLimits::slaveRange().from(), DeviceId); - PointAddress = qMax(1, PointAddress); + // PointAddress is deliberately not clamped here: 0 is a valid address for + // 0-based projects, and the lower bound for 1-based ones depends on the + // address base, which this struct does not know. FormDataView clamps it + // against the current base when the definition is applied. PointType = qBound(QModbusDataUnit::DiscreteInputs, PointType, QModbusDataUnit::HoldingRegisters); Length = qBound(ModbusLimits::lengthRange().from(), Length, static_cast(ModbusLimits::lengthRange().to())); diff --git a/src/formdataview.cpp b/src/formdataview.cpp index ff1c07c3..a1fc6dd1 100644 --- a/src/formdataview.cpp +++ b/src/formdataview.cpp @@ -291,6 +291,8 @@ void FormDataView::setDisplayDefinition(const DataViewDefinitions& dd) setWindowTitle(dd.FormName); const auto defs = _mbMultiServer.getModbusDefinitions(); + const auto addressRange = ModbusLimits::addressRange(defs.AddrSpace, zeroBasedAddress()); + const auto pointAddress = static_cast(qBound(addressRange.from(), static_cast(dd.PointAddress), addressRange.to())); ui->lineEditDeviceId->setLeadingZeroes(false); ui->lineEditDeviceId->setValue(dd.DeviceId); @@ -298,13 +300,13 @@ void FormDataView::setDisplayDefinition(const DataViewDefinitions& dd) { QSignalBlocker b(ui->lineEditAddress); ui->lineEditAddress->setLeadingZeroes(false); - ui->lineEditAddress->setInputRange(ModbusLimits::addressRange(defs.AddrSpace, zeroBasedAddress())); - ui->lineEditAddress->setValue(dd.PointAddress); + ui->lineEditAddress->setInputRange(addressRange); + ui->lineEditAddress->setValue(pointAddress); } { QSignalBlocker b(ui->lineEditLength); ui->lineEditLength->setLeadingZeroes(false); - ui->lineEditLength->setInputRange(lengthRangeForPointType(dd.PointAddress, zeroBasedAddress(), defs.AddrSpace, dd.PointType)); + ui->lineEditLength->setInputRange(lengthRangeForPointType(pointAddress, zeroBasedAddress(), defs.AddrSpace, dd.PointType)); ui->lineEditLength->setValue(dd.Length); } { @@ -797,6 +799,8 @@ void FormDataView::setDisplayDefinitionSilent(const DataViewDefinitions& dd) setWindowTitle(dd.FormName); const auto defs = _mbMultiServer.getModbusDefinitions(); + const auto addressRange = ModbusLimits::addressRange(defs.AddrSpace, zeroBasedAddress()); + const auto pointAddress = static_cast(qBound(addressRange.from(), static_cast(dd.PointAddress), addressRange.to())); // Handle device ID change (reference counting in server). { @@ -815,13 +819,13 @@ void FormDataView::setDisplayDefinitionSilent(const DataViewDefinitions& dd) { QSignalBlocker b(ui->lineEditAddress); ui->lineEditAddress->setLeadingZeroes(false); - ui->lineEditAddress->setInputRange(ModbusLimits::addressRange(defs.AddrSpace, zeroBasedAddress())); - ui->lineEditAddress->setValue(dd.PointAddress); + ui->lineEditAddress->setInputRange(addressRange); + ui->lineEditAddress->setValue(pointAddress); } { QSignalBlocker b(ui->lineEditLength); ui->lineEditLength->setLeadingZeroes(false); - ui->lineEditLength->setInputRange(lengthRangeForPointType(dd.PointAddress, zeroBasedAddress(), defs.AddrSpace, dd.PointType)); + ui->lineEditLength->setInputRange(lengthRangeForPointType(pointAddress, zeroBasedAddress(), defs.AddrSpace, dd.PointType)); ui->lineEditLength->setValue(dd.Length); } { diff --git a/src/legacyprojectloader.cpp b/src/legacyprojectloader.cpp index 07e1b70a..d138e6fe 100644 --- a/src/legacyprojectloader.cpp +++ b/src/legacyprojectloader.cpp @@ -94,6 +94,13 @@ DataViewDefinitions readDataViewDefinitions(QXmlStreamReader& xml, FormDataView* dd.PointType = enumFromString(attributes.value("PointType").toString()); } + if (attributes.hasAttribute("PointAddress")) { + bool ok; + const quint16 pointAddress = attributes.value("PointAddress").toUShort(&ok); + if (ok) + dd.PointAddress = pointAddress; + } + if (attributes.hasAttribute("Length")) { bool ok; const quint16 length = attributes.value("Length").toUShort(&ok); @@ -129,15 +136,6 @@ DataViewDefinitions readDataViewDefinitions(QXmlStreamReader& xml, FormDataView* dd.normalize(); - // Reload PointAddress since normalize() may wrongly clamp it from 0 to 1, - // which breaks 0-based version 1.x projects. - if (attributes.hasAttribute("PointAddress")) { - bool ok; - const quint16 pointAddress = attributes.value("PointAddress").toUShort(&ok); - if (ok) - dd.PointAddress = pointAddress; - } - return dd; } diff --git a/src/legacyprojectparser.cpp b/src/legacyprojectparser.cpp index 5bedd528..c5884349 100644 --- a/src/legacyprojectparser.cpp +++ b/src/legacyprojectparser.cpp @@ -39,6 +39,13 @@ DataViewDefinitions readDataViewDefinitions(QXmlStreamReader& xml, const QString dd.PointType = enumFromString(attributes.value("PointType").toString()); } + if (attributes.hasAttribute("PointAddress")) { + bool ok; + const quint16 pointAddress = attributes.value("PointAddress").toUShort(&ok); + if (ok) + dd.PointAddress = pointAddress; + } + if (attributes.hasAttribute("Length")) { bool ok; const quint16 length = attributes.value("Length").toUShort(&ok); @@ -64,15 +71,6 @@ DataViewDefinitions readDataViewDefinitions(QXmlStreamReader& xml, const QString dd.normalize(); - // Reload PointAddress since normalize() may wrongly clamp it from 0 to 1, - // which breaks 0-based version 1.x projects. - if (attributes.hasAttribute("PointAddress")) { - bool ok; - const quint16 pointAddress = attributes.value("PointAddress").toUShort(&ok); - if (ok) - dd.PointAddress = pointAddress; - } - return dd; } } diff --git a/src/tests/test_appproject.cpp b/src/tests/test_appproject.cpp index 2bbc9743..8448581f 100644 --- a/src/tests/test_appproject.cpp +++ b/src/tests/test_appproject.cpp @@ -35,6 +35,8 @@ private slots: void createsAndRemovesSplitClone(); void roundTripsOpenAndClosedForms(); void preservesZeroBasedStartingAddressAcrossReopen(); + void preservesZeroBasedStartingAddressZeroAcrossReopen(); + void clampsZeroStartingAddressForOneBasedProject(); void opensProjectBeforeRestoringServerState(); void rejectsMalformedProjectWithoutChangingState(); void loadsProjectWithChildlessDefinitionsAndConnection(); @@ -380,6 +382,100 @@ void TestAppProject::preservesZeroBasedStartingAddressAcrossReopen() } } +/// \brief Verifies a 0-based data view starting at address 0 keeps that address across reopen +/// (issue #142). +void TestAppProject::preservesZeroBasedStartingAddressZeroAcrossReopen() +{ + QTemporaryDir files; + const QString first = files.filePath(QStringLiteral("issue142-first.omsim")); + const QString second = files.filePath(QStringLiteral("issue142-second.omsim")); + const auto expectedValues = QVector{100, 200, 300}; + + auto& prefs = AppPreferences::instance(); + const AddressBase previousBase = prefs.globalAddressBase(); + const auto restoreBase = qScopeGuard([&prefs, previousBase] { + prefs.setGlobalAddressBase(previousBase); + }); + + { + ProjectFixture fixture; + prefs.setGlobalAddressBase(AddressBase::Base0); + auto* form = qobject_cast(fixture.Project.createMdiChild(ProjectFormKind::Data)); + QVERIFY(form); + form->setAddressBase(AddressBase::Base0); + + auto definition = form->displayDefinition(); + definition.PointType = QModbusDataUnit::HoldingRegisters; + definition.PointAddress = 0; + definition.Length = static_cast(expectedValues.size()); + form->setDisplayDefinition(definition); + QCOMPARE(form->displayDefinition().PointAddress, quint16(0)); + + QModbusDataUnit data(definition.PointType, 0, definition.Length); + data.setValues(expectedValues); + fixture.Server.setData(definition.DeviceId, data, WriteSource::User); + QVERIFY(fixture.Project.saveProject(first)); + } + + { + ProjectFixture fixture; + QVERIFY(fixture.Project.loadProject(first).Success); + QCOMPARE(prefs.globalAddressBase(), AddressBase::Base0); + auto* form = qobject_cast(fixture.Project.forms(ProjectFormKind::Data).constFirst()); + QVERIFY(form); + form->setAddressBase(prefs.globalAddressBase()); + QCOMPARE(form->displayDefinition().PointAddress, quint16(0)); + QCOMPARE(fixture.Server.data(1, QModbusDataUnit::HoldingRegisters, 0, 3).values(), expectedValues); + QVERIFY(fixture.Project.saveProject(second)); + } + + { + ProjectFixture fixture; + QVERIFY(fixture.Project.loadProject(second).Success); + QCOMPARE(prefs.globalAddressBase(), AddressBase::Base0); + auto* form = qobject_cast(fixture.Project.forms(ProjectFormKind::Data).constFirst()); + QVERIFY(form); + form->setAddressBase(prefs.globalAddressBase()); + QCOMPARE(form->displayDefinition().PointAddress, quint16(0)); + QCOMPARE(fixture.Server.data(1, QModbusDataUnit::HoldingRegisters, 0, 3).values(), expectedValues); + } +} + +/// \brief Verifies a 1-based project still falls back to address 1 when a project file +/// stores the invalid address 0. +void TestAppProject::clampsZeroStartingAddressForOneBasedProject() +{ + ProjectFixture fixture; + QTemporaryDir files; + const QString path = files.filePath(QStringLiteral("one-based-zero.omsim")); + + auto& prefs = AppPreferences::instance(); + const AddressBase previousBase = prefs.globalAddressBase(); + prefs.setGlobalAddressBase(AddressBase::Base1); + const auto restoreBase = qScopeGuard([&prefs, previousBase] { + prefs.setGlobalAddressBase(previousBase); + }); + + writeProjectFile(path, + "\n" + "\n" + " \n" + " \n" + " \n" + " \n" + " \n" + " \n" + " \n" + " \n" + "\n"); + + const auto result = fixture.Project.loadProject(path); + QVERIFY2(result.Success, qPrintable(result.Error)); + auto* form = qobject_cast(fixture.Project.forms(ProjectFormKind::Data).constFirst()); + QVERIFY(form); + QCOMPARE(form->displayDefinition().PointAddress, quint16(1)); +} + /// \brief Verifies the project-opened notification precedes restored devices and maps. void TestAppProject::opensProjectBeforeRestoringServerState() {