Skip to content

Commit a140d6d

Browse files
jgeudensjgeudens
authored
Merge pull request #424 from ModbusScope/defect/slave_id_mixup
Fix reading same register address on multiple devices on same serial connection.
2 parents 9fba4fa + 8385df9 commit a140d6d

12 files changed

Lines changed: 276 additions & 44 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ The latest *ModbusScope* installer or standalone version can always be downloade
88

99
### Fixed
1010

11-
* xx
11+
* Fix incorrect data displayed when polling multiple devices on the same connection (e.g. multiple slave IDs on a serial bus)
1212

1313
### Changed
1414

‎src/communication/modbusdataunit.cpp‎

Lines changed: 6 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#include "modbusdataunit.h"
22

3+
#include <tuple>
4+
35
ModbusDataUnit::ModbusDataUnit(quint16 protocolAddress, ObjectType type, slaveId_t slaveId)
46
: ModbusAddress(protocolAddress, type), _slaveId(slaveId)
57
{
@@ -31,36 +33,12 @@ QString ModbusDataUnit::toString() const
3133

3234
bool operator==(const ModbusDataUnit& unit1, const ModbusDataUnit& unit2)
3335
{
34-
if ((unit1._protocolAddress == unit2._protocolAddress) && (unit1._type == unit2._type) &&
35-
(unit1._slaveId == unit2._slaveId))
36-
{
37-
return true;
38-
}
39-
else
40-
{
41-
return false;
42-
}
36+
return std::tie(unit1._slaveId, unit1._type, unit1._protocolAddress)
37+
== std::tie(unit2._slaveId, unit2._type, unit2._protocolAddress);
4338
}
4439

4540
bool operator<(const ModbusDataUnit& unit1, const ModbusDataUnit& unit2)
4641
{
47-
if (unit1._type < unit2._type)
48-
{
49-
return true;
50-
}
51-
else if (unit1._type == unit2._type)
52-
{
53-
if (unit1._protocolAddress < unit2._protocolAddress)
54-
{
55-
return true;
56-
}
57-
else
58-
{
59-
return false;
60-
}
61-
}
62-
else
63-
{
64-
return false;
65-
}
42+
return std::tie(unit1._slaveId, unit1._type, unit1._protocolAddress)
43+
< std::tie(unit2._slaveId, unit2._type, unit2._protocolAddress);
6644
}

‎tests/CMakeLists.txt‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,7 @@ find_package(Qt${QT_VERSION_MAJOR} COMPONENTS
3333
function(add_xtest SOURCE_NAME)
3434
add_executable(${SOURCE_NAME}
3535
${SOURCE_NAME}.cpp
36-
${ARGV1}
37-
${ARGV2}
38-
${ARGV3}
39-
${ARGV4})
36+
${ARGN})
4037
target_link_libraries(${SOURCE_NAME} Qt::Test ${QT_LIB} ${SCOPESOURCE})
4138
add_test(NAME ${SOURCE_NAME} COMMAND ${SOURCE_NAME})
4239
endfunction()
@@ -45,10 +42,7 @@ function(add_xtest_mock SOURCE_NAME)
4542
add_executable(${SOURCE_NAME}
4643
${SOURCE_NAME}.cpp
4744
${GOOGLE_TEST_SOURCE}
48-
${ARGV1}
49-
${ARGV2}
50-
${ARGV3}
51-
${ARGV4})
45+
${ARGN})
5246
target_link_libraries(${SOURCE_NAME} Qt::Test Threads::Threads ${QT_LIB} ${SCOPESOURCE})
5347
add_test(NAME ${SOURCE_NAME} COMMAND ${SOURCE_NAME})
5448
endfunction()

‎tests/communication/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
SET(TEST_SRCS
33
${CMAKE_CURRENT_SOURCE_DIR}/../testslave/testslavedata.cpp
44
${CMAKE_CURRENT_SOURCE_DIR}/../testslave/testslavemodbus.cpp
5+
${CMAKE_CURRENT_SOURCE_DIR}/../testslave/testslavemodbusmulti.cpp
56
${CMAKE_CURRENT_SOURCE_DIR}/../testslave/testdevice.cpp
67
)
78

‎tests/communication/tst_modbusdataunit.cpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,29 @@ void TestModbusDataUnit::operator_less_than()
5858
ModbusDataUnit unit1(10, ModbusAddress::ObjectType::COIL, 5);
5959
ModbusDataUnit unit2(11, ModbusAddress::ObjectType::COIL, 5);
6060
ModbusDataUnit unit3(10, ModbusAddress::ObjectType::DISCRETE_INPUT, 5);
61+
ModbusDataUnit unit4(10, ModbusAddress::ObjectType::COIL, 6); // same addr+type, different slaveId
6162

6263
QVERIFY(unit1 < unit2); // protocolAddress comparison
6364
QVERIFY(unit1 < unit3); // type comparison
6465
QVERIFY(!(unit2 < unit1));
6566
QVERIFY(!(unit3 < unit1));
67+
// slaveId comparison (regression for multi-slave bug)
68+
QVERIFY(unit1 < unit4); // slaveId 5 < slaveId 6, same address+type
69+
QVERIFY(!(unit4 < unit1));
70+
}
71+
72+
void TestModbusDataUnit::qmap_different_slave_ids_are_distinct_keys()
73+
{
74+
ModbusDataUnit unit1(40001, ModbusAddress::ObjectType::HOLDING_REGISTER, 1);
75+
ModbusDataUnit unit2(40001, ModbusAddress::ObjectType::HOLDING_REGISTER, 2);
76+
77+
QMap<ModbusDataUnit, int> map;
78+
map.insert(unit1, 100);
79+
map.insert(unit2, 200);
80+
81+
QCOMPARE(map.size(), 2);
82+
QCOMPARE(map.value(unit1), 100);
83+
QCOMPARE(map.value(unit2), 200);
6684
}
6785

6886
void TestModbusDataUnit::toString()

‎tests/communication/tst_modbusdataunit.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ private slots:
1515

1616
void operator_equality();
1717
void operator_less_than();
18+
void qmap_different_slave_ids_are_distinct_keys();
1819

1920
void toString();
2021

‎tests/communication/tst_modbuspoll.cpp‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include "communication/modbuspoll.h"
55
#include "communicationhelpers.h"
66
#include "testslavemodbus.h"
7+
#include "testslavemodbusmulti.h"
78
#include "util/modbusdatatype.h"
89

910
#include <QSignalSpy>
@@ -340,6 +341,53 @@ void TestModbusPoll::multiSlaveSuccess_3()
340341
CommunicationHelpers::verifyReceivedDataSignal(arguments, expResults);
341342
}
342343

344+
void TestModbusPoll::multiSlaveSameConnectionSameAddress()
345+
{
346+
/* Reproduce the reported bug: two slaves with different IDs share the same
347+
* connection (same port) and both read the same register address.
348+
* Before the operator< fix, both curves showed the value from the last-polled
349+
* slave because the result map treated (addr=40001, slaveId=1) and
350+
* (addr=40001, slaveId=4) as the same key. */
351+
352+
const quint8 newSlaveId = 4;
353+
const deviceId_t newDevId = Device::cFirstDeviceId + 3;
354+
355+
/* Replace the single-slave server at port 5020 with a multi-slave server
356+
* that responds to slave ID 1 (device 1) and slave ID 4 (new device) */
357+
_testSlaveMap[Device::cFirstDeviceId]->disconnect();
358+
359+
TestSlaveMultiModbus multiSlave;
360+
QVERIFY(multiSlave.listenOnPort(5020));
361+
362+
multiSlave.testDevice(Device::cFirstDeviceId)->configureHoldingRegister(0, true, 1111);
363+
multiSlave.testDevice(newSlaveId)->configureHoldingRegister(0, true, 4444);
364+
365+
/* Add a 4th device that shares connection ID_1 (port 5020) with device 1 */
366+
_pSettingsModel->addDevice(newDevId);
367+
_pSettingsModel->deviceSettings(newDevId)->setConnectionId(ConnectionTypes::ID_1);
368+
_pSettingsModel->deviceSettings(newDevId)->setSlaveId(newSlaveId);
369+
370+
ModbusPoll modbusPoll(_pSettingsModel);
371+
QSignalSpy spyDataReady(&modbusPoll, &ModbusPoll::registerDataReady);
372+
373+
auto modbusRegisters = QList<ModbusRegister>()
374+
<< ModbusRegister(ModbusAddress(40001), Device::cFirstDeviceId, Type::UNSIGNED_16)
375+
<< ModbusRegister(ModbusAddress(40001), newDevId, Type::UNSIGNED_16);
376+
377+
modbusPoll.startCommunication(modbusRegisters);
378+
379+
QVERIFY(spyDataReady.wait(500));
380+
QCOMPARE(spyDataReady.count(), 1);
381+
382+
QList<QVariant> arguments = spyDataReady.takeFirst();
383+
auto expResults = ResultDoubleList() << ResultDouble(1111, State::SUCCESS)
384+
<< ResultDouble(4444, State::SUCCESS);
385+
386+
CommunicationHelpers::verifyReceivedDataSignal(arguments, expResults);
387+
388+
multiSlave.stopListening();
389+
}
390+
343391
void TestModbusPoll::multiSlaveSingleFail()
344392
{
345393
_testSlaveMap[Device::cFirstDeviceId]->disconnect();

‎tests/communication/tst_modbuspoll.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ private slots:
2525
void multiSlaveSuccess();
2626
void multiSlaveSuccess_2();
2727
void multiSlaveSuccess_3();
28+
void multiSlaveSameConnectionSameAddress();
2829
void multiSlaveSingleFail();
2930
void multiSlaveAllFail();
3031

‎tests/communication/tst_registervaluehandler.cpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -291,10 +291,10 @@ void TestRegisterValueHandler::readConnections()
291291
<< ModbusRegister(ModbusAddress(40001), Device::cFirstDeviceId, Type::UNSIGNED_16)
292292
<< ModbusRegister(ModbusAddress(40001), Device::cFirstDeviceId + 1, Type::SIGNED_16);
293293
ModbusResultMap partialResultMap1;
294-
addToResultMap(partialResultMap1, 40001, false, 256, State::SUCCESS);
294+
addToResultMap(partialResultMap1, 40001, false, 256, State::SUCCESS, 1);
295295

296296
ModbusResultMap partialResultMap2;
297-
addToResultMap(partialResultMap2, 40001, false, 100, State::SUCCESS);
297+
addToResultMap(partialResultMap2, 40001, false, 100, State::SUCCESS, 2);
298298

299299
auto expResults = ResultDoubleList() << ResultDouble(256, State::SUCCESS)
300300
<< ResultDouble(100, State::SUCCESS);
@@ -329,7 +329,7 @@ void TestRegisterValueHandler::readFail()
329329
<< ModbusRegister(ModbusAddress(40001), Device::cFirstDeviceId + 1, Type::SIGNED_16);
330330

331331
ModbusResultMap partialResultMap2;
332-
addToResultMap(partialResultMap2, 40001, false, 100, State::SUCCESS);
332+
addToResultMap(partialResultMap2, 40001, false, 100, State::SUCCESS, 2);
333333

334334
auto expResults = ResultDoubleList() << ResultDouble(0, State::INVALID)
335335
<< ResultDouble(100, State::SUCCESS);
@@ -385,14 +385,15 @@ void TestRegisterValueHandler::addToResultMap(ModbusResultMap &resultMap,
385385
quint32 addr,
386386
bool b32bit,
387387
qint64 value,
388-
State resultState
388+
State resultState,
389+
ModbusDataUnit::slaveId_t slaveId
389390
)
390391
{
391-
resultMap.insert(ModbusDataUnit(addr), Result<quint16>(static_cast<quint16>(value), resultState));
392+
resultMap.insert(ModbusDataUnit(addr, ModbusAddress::ObjectType::UNKNOWN, slaveId), Result<quint16>(static_cast<quint16>(value), resultState));
392393

393394
if (b32bit)
394395
{
395-
resultMap.insert(ModbusDataUnit(addr + 1), Result<quint16>(static_cast<quint32>(value) >> 16, resultState));
396+
resultMap.insert(ModbusDataUnit(addr + 1, ModbusAddress::ObjectType::UNKNOWN, slaveId), Result<quint16>(static_cast<quint32>(value) >> 16, resultState));
396397
}
397398
}
398399

‎tests/communication/tst_registervaluehandler.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ private slots:
4545
quint32 addr,
4646
bool b32bit,
4747
qint64 value,
48-
ResultState::State resultState
48+
ResultState::State resultState,
49+
ModbusDataUnit::slaveId_t slaveId = 1
4950
);
5051

5152
SettingsModel* _pSettingsModel;

0 commit comments

Comments
 (0)