From 17ac276e9b4fcde0f2e2b9a68a01ef6cdde31cf5 Mon Sep 17 00:00:00 2001 From: Karsten Heimrich Date: Thu, 11 Feb 2021 12:48:07 +0100 Subject: Fix Modbus custom command size calculation We cannot early return with an invalid PDU if the private minimumDataSize() functions returns a negative value. The negative value might mean we hit a user defined function code, so advance and call the user data size calculater if registered. The broken function was only used by Modbus TCP implementations , so RTU was working correctly since it created the PDU differently. [ChangeLog] Fix Modbus TCP custom command size calculation. Fixes: QTBUG-62192 Fixes: QTBUG-91037 Task-number: QTBUG-91214 Change-Id: I2d421e073777306d86a83cd05a289481fa082501 Reviewed-by: Alex Blasche (cherry picked from commit 2c0bb738e988d84f36169e0103d051745b678479) Reviewed-by: Qt Cherry-pick Bot --- src/serialbus/qmodbuspdu.cpp | 105 ++++++++++++++++++++++++------------------- src/serialbus/qmodbuspdu.h | 1 + 2 files changed, 60 insertions(+), 46 deletions(-) (limited to 'src') diff --git a/src/serialbus/qmodbuspdu.cpp b/src/serialbus/qmodbuspdu.cpp index 14447e7..75e5ed8 100644 --- a/src/serialbus/qmodbuspdu.cpp +++ b/src/serialbus/qmodbuspdu.cpp @@ -48,13 +48,23 @@ Q_GLOBAL_STATIC(ReqSizeCalc, requestSizeCalculators); using ResSizeCalc = QHash; Q_GLOBAL_STATIC(ResSizeCalc, responseSizeCalculators); -namespace Private { +struct QModbusPduPrivate +{ + QModbusPduPrivate() = delete; + Q_DISABLE_COPY_MOVE(QModbusPduPrivate) enum struct Type { Request, Response }; +/*! + \internal + + Returns the minimum data size in bytes for the given \a pdu and the + Modbus PDU \a type. If the PDU's function code is invalid, undefined + or unknown, the return value will be \c {-1}. +*/ static int minimumDataSize(const QModbusPdu &pdu, Type type) { if (pdu.isException()) @@ -103,44 +113,51 @@ static int minimumDataSize(const QModbusPdu &pdu, Type type) return -1; } -static QDataStream &pduFromStream(QDataStream &stream, QModbusPdu &pdu, Type type) -{ - quint8 codeByte = 0; - if (stream.readRawData(reinterpret_cast(&codeByte), sizeof(quint8)) != sizeof(quint8)) - return stream; - QModbusPdu::FunctionCode code = QModbusPdu::FunctionCode(codeByte); - pdu.setFunctionCode(code); +/*! + \internal - auto needsAdditionalRead = [](QModbusPdu &pdu, int size) -> bool { - if (size < 0) - pdu.setFunctionCode(QModbusResponse::Invalid); - if (size <= 0) - return false; - return true; - }; + Extracts a Modbus PDU from a \a stream into the given \a pdu based on \a type. +*/ +static QDataStream &pduFromStream(QDataStream &stream, Type type, QModbusPdu *pdu) +{ + struct RAII { + QModbusPdu *tmp{ nullptr }; + ~RAII() { if (tmp) *tmp = {}; } + } raii = { pdu }; - const bool isResponse = (type == Type::Response); - int size = isResponse ? QModbusResponse::minimumDataSize(pdu) - : QModbusRequest::minimumDataSize(pdu); - if (!needsAdditionalRead(pdu, size)) + QModbusPdu::FunctionCode code = QModbusPdu::FunctionCode::Invalid; + if (stream.readRawData(reinterpret_cast(&code), sizeof(quint8)) != sizeof(quint8)) return stream; + pdu->setFunctionCode(code); - QByteArray data(size, Qt::Uninitialized); - if (stream.device()->peek(data.data(), data.size()) != size) + if (code == QModbusPdu::Invalid || code == QModbusPdu::UndefinedFunctionCode) // shortcut return stream; - pdu.setData(data); - size = isResponse ? QModbusResponse::calculateDataSize(pdu) - : QModbusRequest::calculateDataSize(pdu); - if (!needsAdditionalRead(pdu, size)) + constexpr const int MaxPduDataSize = 252; // in bytes + + // The calculateDataSize(...) function might need some data inside the + // given PDU argument to be able to figure out the right data size (e.g. + // WriteMultipleCoils contains some kind of "header"). So fake fill the PDU + // with the maximum available data but no more than the allowed max PDU + // data size. + QByteArray data(MaxPduDataSize, Qt::Uninitialized); + int read = stream.device()->peek(data.data(), MaxPduDataSize); + if (read < 0) return stream; + data.resize(read); + pdu->setData(data); + + const bool isResponse = (type == Type::Response); + int size = isResponse ? QModbusResponse::calculateDataSize(*pdu) + : QModbusRequest::calculateDataSize(*pdu); + if (isResponse && (code == QModbusPdu::EncapsulatedInterfaceTransport)) { quint8 meiType; - pdu.decodeData(&meiType); + pdu->decodeData(&meiType); if (meiType == EncapsulatedInterfaceTransport::ReadDeviceIdentification) { int left = size, offset = 0; - while ((left > 0) && (size <= 252)) { // The maximum PDU data size is 252 bytes. + while ((left > 0) && (size <= MaxPduDataSize)) { data.resize(size); const int read = stream.readRawData(data.data() + offset, size - offset); if ((read < 0) || (read != (size - offset))) { @@ -152,35 +169,31 @@ static QDataStream &pduFromStream(QDataStream &stream, QModbusPdu &pdu, Type typ left = QModbusResponse::calculateDataSize(QModbusResponse(code, data)) - offset; size += left; } - if ((stream.status() == QDataStream::Ok) && (size <= 252)) { - pdu.setData(data); + if ((stream.status() == QDataStream::Ok) && (size <= MaxPduDataSize)) { + pdu->setData(data); + raii = { nullptr }; return stream; // early return to avoid second read } } else { data.resize(int(stream.device()->size() - 1)); // One byte for the function code. } - } else if (pdu.functionCode() == QModbusPdu::Diagnostics) { + } else if (pdu->functionCode() == QModbusPdu::Diagnostics) { quint16 subCode; - pdu.decodeData(&subCode); + pdu->decodeData(&subCode); if (subCode == Diagnostics::ReturnQueryData) data.resize(int(stream.device()->size() - 1)); // One byte for the function code. } - // reset what we have so far, next read might fail as well - pdu.setData(QByteArray()); - pdu.setFunctionCode(QModbusPdu::Invalid); - - if (data.size() <= 252) { // The maximum PDU data size is 252 bytes. + if (data.size() <= MaxPduDataSize) { data.resize(size); if (stream.readRawData(data.data(), data.size()) == size) { - pdu.setData(data); - pdu.setFunctionCode(code); + pdu->setData(data); + raii = { nullptr }; } } return stream; } - -} // namespace Private +}; /*! \class QModbusPdu @@ -533,7 +546,7 @@ QDataStream &operator<<(QDataStream &stream, const QModbusPdu &pdu) */ int QModbusRequest::minimumDataSize(const QModbusRequest &request) { - return Private::minimumDataSize(request, Private::Type::Request); + return QModbusPduPrivate::minimumDataSize(request, QModbusPduPrivate::Type::Request); } /*! @@ -555,7 +568,7 @@ int QModbusRequest::calculateDataSize(const QModbusRequest &request) return 1; int size = -1; - int minimum = Private::minimumDataSize(request, Private::Type::Request); + int minimum = QModbusPduPrivate::minimumDataSize(request, QModbusPduPrivate::Type::Request); if (minimum < 0) return size; @@ -620,7 +633,7 @@ void QModbusRequest::registerDataSizeCalculator(FunctionCode fc, CalcFuncPtr cal */ QDataStream &operator>>(QDataStream &stream, QModbusRequest &pdu) { - return Private::pduFromStream(stream, pdu, Private::Type::Request); + return QModbusPduPrivate::pduFromStream(stream, QModbusPduPrivate::Type::Request, &pdu); } /*! @@ -689,7 +702,7 @@ QDataStream &operator>>(QDataStream &stream, QModbusRequest &pdu) */ int QModbusResponse::minimumDataSize(const QModbusResponse &response) { - return Private::minimumDataSize(response, Private::Type::Response); + return QModbusPduPrivate::minimumDataSize(response, QModbusPduPrivate::Type::Response); } /*! @@ -711,7 +724,7 @@ int QModbusResponse::calculateDataSize(const QModbusResponse &response) return 1; int size = -1; - int minimum = Private::minimumDataSize(response, Private::Type::Response); + int minimum = QModbusPduPrivate::minimumDataSize(response, QModbusPduPrivate::Type::Response); if (minimum < 0) return size; @@ -806,7 +819,7 @@ void QModbusResponse::registerDataSizeCalculator(FunctionCode fc, CalcFuncPtr ca */ QDataStream &operator>>(QDataStream &stream, QModbusResponse &pdu) { - return Private::pduFromStream(stream, pdu, Private::Type::Response); + return QModbusPduPrivate::pduFromStream(stream, QModbusPduPrivate::Type::Response, &pdu); } /*! diff --git a/src/serialbus/qmodbuspdu.h b/src/serialbus/qmodbuspdu.h index 68c947b..f299f24 100644 --- a/src/serialbus/qmodbuspdu.h +++ b/src/serialbus/qmodbuspdu.h @@ -183,6 +183,7 @@ private: FunctionCode m_code = Invalid; QByteArray m_data; friend class QModbusSerialAdu; + friend struct QModbusPduPrivate; }; Q_SERIALBUS_EXPORT QDebug operator<<(QDebug debug, const QModbusPdu &pdu); Q_SERIALBUS_EXPORT QDataStream &operator<<(QDataStream &stream, const QModbusPdu &pdu); -- cgit v1.2.3 From 4e7a1e844b352ca3da4dce7e96814d500d062cd8 Mon Sep 17 00:00:00 2001 From: Karsten Heimrich Date: Wed, 24 Feb 2021 10:36:25 +0100 Subject: Fix Modbus custom command response processing QModbusClient: Checking for QModbusReply::Common will suppress calling into the existing and supposed to be called QModbusClient::processPrivateRequest function, which is to only way to handle a response from a raw request. QModbusReply: There was probably a workaround used since the QModbusReply does expose the response and in case of QModbusReply::Raw the API user could still get the reply and work their way through. Now you can get the result back, proccessed by QModbusClient::processPrivateRequest if the user implements it. The function behavior does not change, as in any case except the QModbusReply::Common the the returned unit was invalid anyways. [ChangeLog] Fix Modbus custom command response processing. Task-number: QTBUG-91214 Change-Id: I09beeef9f236a29ffc0abf4b3dd323c9ce8ba1a4 Reviewed-by: Alex Blasche (cherry picked from commit 09927894825d97197ffa656576e6be780371dda0) --- src/serialbus/qmodbusclient.cpp | 2 +- src/serialbus/qmodbusreply.cpp | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/serialbus/qmodbusclient.cpp b/src/serialbus/qmodbusclient.cpp index 768a59d..a4840f6 100644 --- a/src/serialbus/qmodbusclient.cpp +++ b/src/serialbus/qmodbusclient.cpp @@ -361,7 +361,7 @@ void QModbusClientPrivate::processQueueElement(const QModbusResponse &pdu, return; } - if (element.reply->type() != QModbusReply::Common) { + if (element.reply->type() == QModbusReply::Broadcast) { element.reply->setFinished(true); return; } diff --git a/src/serialbus/qmodbusreply.cpp b/src/serialbus/qmodbusreply.cpp index 771d14c..ca65aac 100644 --- a/src/serialbus/qmodbusreply.cpp +++ b/src/serialbus/qmodbusreply.cpp @@ -147,15 +147,17 @@ void QModbusReply::setFinished(bool isFinished) If the request has not finished, has failed with an error or was a write request then the returned \l QModbusDataUnit instance is invalid. - \note If the \l type() of the reply is \l QModbusReply::Raw, the return - value will always be invalid. + \note If the \l type() of the reply is \l QModbusReply::Broadcast, the + return value will always be invalid. If the l type() of the reply is + \l QModbusReply::Raw, the return value might be invalid depending on the + implementation of \l QModbusClient::processPrivateResponse(). - \sa type(), rawResult() + \sa type(), rawResult(), QModbusClient::processPrivateResponse() */ QModbusDataUnit QModbusReply::result() const { Q_D(const QModbusReply); - if (type() == QModbusReply::Common) + if (type() != QModbusReply::Broadcast) return d->m_unit; return QModbusDataUnit(); } -- cgit v1.2.3 From 1d674e21715aeadf242dea1d567bbbf7696acbdc Mon Sep 17 00:00:00 2001 From: Karsten Heimrich Date: Thu, 4 Mar 2021 11:37:37 +0100 Subject: Fix 5.15 compile error Change-Id: Iff78d4f410e62328fce04c0a7212aec961425427 Reviewed-by: Alex Blasche --- src/serialbus/qmodbuspdu.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/serialbus/qmodbuspdu.cpp b/src/serialbus/qmodbuspdu.cpp index 75e5ed8..112b73d 100644 --- a/src/serialbus/qmodbuspdu.cpp +++ b/src/serialbus/qmodbuspdu.cpp @@ -121,6 +121,8 @@ static int minimumDataSize(const QModbusPdu &pdu, Type type) static QDataStream &pduFromStream(QDataStream &stream, Type type, QModbusPdu *pdu) { struct RAII { + RAII(QModbusPdu *ptr = nullptr) + : tmp(ptr) {} QModbusPdu *tmp{ nullptr }; ~RAII() { if (tmp) *tmp = {}; } } raii = { pdu }; @@ -170,8 +172,8 @@ static QDataStream &pduFromStream(QDataStream &stream, Type type, QModbusPdu *pd size += left; } if ((stream.status() == QDataStream::Ok) && (size <= MaxPduDataSize)) { + raii = {}; pdu->setData(data); - raii = { nullptr }; return stream; // early return to avoid second read } } else { @@ -187,8 +189,8 @@ static QDataStream &pduFromStream(QDataStream &stream, Type type, QModbusPdu *pd if (data.size() <= MaxPduDataSize) { data.resize(size); if (stream.readRawData(data.data(), data.size()) == size) { + raii = {}; pdu->setData(data); - raii = { nullptr }; } } return stream; -- cgit v1.2.3