Skip to content

fix: Keep starting address 0 in 0-based projects (#142) #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sanny32 merged 1 commit into from
Nov 4, 2026
Merged

fix: Keep starting address 0 in 0-based projects (#142) #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/displaydefinition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ struct DataViewDefinitions
void normalize()
{
DeviceId = qMax<quint8>(ModbusLimits::slaveRange().from(), DeviceId);
PointAddress = qMax<quint16>(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<quint16>(ModbusLimits::lengthRange().from(), Length,
static_cast<quint16>(ModbusLimits::lengthRange().to()));
Expand Down
16 changes: 10 additions & 6 deletions src/formdataview.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,20 +291,22 @@ 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<quint16>(qBound(addressRange.from(), static_cast<int>(dd.PointAddress), addressRange.to()));

ui->lineEditDeviceId->setLeadingZeroes(false);
ui->lineEditDeviceId->setValue(dd.DeviceId);

{
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);
}
{
Expand Down Expand Up @@ -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<quint16>(qBound(addressRange.from(), static_cast<int>(dd.PointAddress), addressRange.to()));

// Handle device ID change (reference counting in server).
{
Expand All @@ -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);
}
{
Expand Down
16 changes: 7 additions & 9 deletions src/legacyprojectloader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@ DataViewDefinitions readDataViewDefinitions(QXmlStreamReader& xml, FormDataView*
dd.PointType = enumFromString<QModbusDataUnit::RegisterType>(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);
Expand Down Expand Up @@ -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;
}

Expand Down
16 changes: 7 additions & 9 deletions src/legacyprojectparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ DataViewDefinitions readDataViewDefinitions(QXmlStreamReader& xml, const QString
dd.PointType = enumFromString<QModbusDataUnit::RegisterType>(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);
Expand All @@ -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;
}
}
Expand Down
96 changes: 96 additions & 0 deletions src/tests/test_appproject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ private slots:
void createsAndRemovesSplitClone();
void roundTripsOpenAndClosedForms();
void preservesZeroBasedStartingAddressAcrossReopen();
void preservesZeroBasedStartingAddressZeroAcrossReopen();
void clampsZeroStartingAddressForOneBasedProject();
void opensProjectBeforeRestoringServerState();
void rejectsMalformedProjectWithoutChangingState();
void loadsProjectWithChildlessDefinitionsAndConnection();
Expand Down Expand Up @@ -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<quint16>{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<FormDataView*>(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<quint16>(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<FormDataView*>(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<FormDataView*>(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,
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
"<OpenModSim Version=\"2.0-dev\">\n"
" <ModbusDefinitions AddrSpace=\"6-Digits\"/>\n"
" <ViewSettings ViewMode=\"1\" SplitView=\"0\" GlobalZeroBasedAddress=\"0\"/>\n"
" <Forms>\n"
" <FormDataView Panel=\"L\" Title=\"Data1\" DataType=\"UInt16\" RegisterOrder=\"MSRF\" Codepage=\"\" ByteOrder=\"Direct\">\n"
" <Window Maximized=\"true\" Minimized=\"false\" Left=\"0\" Top=\"0\" Width=\"610\" Height=\"331\"/>\n"
" <DataViewDefinitions DeviceId=\"1\" PointType=\"HoldingRegisters\" PointAddress=\"0\" Length=\"8\" DataViewColumnsDistance=\"25\" LeadingZeros=\"true\"/>\n"
" </FormDataView>\n"
" </Forms>\n"
"</OpenModSim>\n");

const auto result = fixture.Project.loadProject(path);
QVERIFY2(result.Success, qPrintable(result.Error));
auto* form = qobject_cast<FormDataView*>(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()
{
Expand Down