Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions adapters/describe.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
{
"id": 0,
"ip": "127.0.0.1",
"persistent": false,
"persistent": true,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Defaults now diverge from the published contract.

persistent and int32LittleEndian were changed to true, but adapters/json-rpc-spec.md still documents both defaults as false. This creates a behavioural/API mismatch for clients consuming adapter.describe.

Proposed fix (if the default change was unintentional)
-                "persistent": true,
+                "persistent": false,
...
-                "int32LittleEndian": true,
+                "int32LittleEndian": false,

Also applies to: 26-26

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@adapters/describe.json` at line 15, The defaults in adapters/describe.json
for the fields persistent and int32LittleEndian were changed to true while
adapters/json-rpc-spec.md still documents them as false, causing an API mismatch
for adapter.describe; fix by making them consistent: either revert the two
occurrences of persistent and int32LittleEndian in adapters/describe.json back
to false to match the spec, or update adapters/json-rpc-spec.md to state the new
true defaults—ensure you update both places where those keys appear in
adapters/describe.json and update the adapter.describe section of the spec
accordingly so runtime behavior and documentation match.

"port": 502,
"timeout": 1000,
"type": "tcp"
Expand All @@ -23,7 +23,7 @@
"connectionId": 0,
"consecutiveMax": 125,
"id": 1,
"int32LittleEndian": false,
"int32LittleEndian": true,
"slaveId": 1
}
],
Expand Down
Binary file modified adapters/dummymodbusadapter
Binary file not shown.
Binary file modified adapters/dummymodbusadapter.exe
Binary file not shown.
3 changes: 2 additions & 1 deletion adapters/json-rpc-spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ Notifications are sent by the adapter to the client without a corresponding requ

### `adapter.diagnostic`

_(Reserved for future use.)_ Carries a log or diagnostic message from the adapter.
Carries a log or diagnostic message from the adapter. Emitted for every Qt log message (`qDebug`, `qInfo`, `qWarning`, `qCritical`, `qFatal`) produced during adapter operation.

```json
{
Expand All @@ -356,6 +356,7 @@ _(Reserved for future use.)_ Carries a log or diagnostic message from the adapte
| `"debug"` | Verbose internal trace |
| `"info"` | Informational |
| `"warning"` | Non-fatal issue |
| `"error"` | Critical or fatal error |

---

Expand Down
Binary file modified adapters/modbusadapter
Binary file not shown.
Binary file modified adapters/modbusadapter.exe
Binary file not shown.
68 changes: 54 additions & 14 deletions src/ProtocolAdapter/adapterclient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

#include <QJsonArray>

AdapterClient::AdapterClient(AdapterProcess* pProcess, QObject* parent) : QObject(parent), _pProcess(pProcess)
AdapterClient::AdapterClient(AdapterProcess* pProcess, QObject* parent, int handshakeTimeoutMs)
: QObject(parent), _pProcess(pProcess), _handshakeTimeoutMs(handshakeTimeoutMs)
{
Q_ASSERT(pProcess);
_pProcess->setParent(this);
Expand All @@ -16,6 +17,7 @@ AdapterClient::AdapterClient(AdapterProcess* pProcess, QObject* parent) : QObjec
connect(_pProcess, &AdapterProcess::errorReceived, this, &AdapterClient::onErrorReceived);
connect(_pProcess, &AdapterProcess::processError, this, &AdapterClient::onProcessError);
connect(_pProcess, &AdapterProcess::processFinished, this, &AdapterClient::onProcessFinished);
connect(_pProcess, &AdapterProcess::notificationReceived, this, &AdapterClient::onNotificationReceived);
}

AdapterClient::~AdapterClient() = default;
Expand All @@ -35,7 +37,7 @@ void AdapterClient::prepareAdapter(const QString& adapterPath)

qCInfo(scopeComm) << "AdapterClient: process started, sending initialize";
_state = State::INITIALIZING;
_handshakeTimer.start(cHandshakeTimeoutMs);
_handshakeTimer.start(_handshakeTimeoutMs);
_pProcess->sendRequest("adapter.initialize", QJsonObject());
}

Expand All @@ -50,7 +52,7 @@ void AdapterClient::provideConfig(QJsonObject config, QStringList registerExpres
_pendingExpressions = registerExpressions;
_pendingConfig = config;
_state = State::CONFIGURING;
_handshakeTimer.start(cHandshakeTimeoutMs);
_handshakeTimer.start(_handshakeTimeoutMs);
QJsonObject params;
params["config"] = _pendingConfig;
_pProcess->sendRequest("adapter.configure", params);
Expand Down Expand Up @@ -91,12 +93,13 @@ void AdapterClient::stopSession()
{
_state = State::STOPPING;
_pProcess->sendRequest("adapter.shutdown", QJsonObject());
_handshakeTimer.start(_handshakeTimeoutMs);
}
else
{
_state = State::STOPPING;
_pProcess->stop();
_state = State::IDLE;
emit sessionStopped();
/* sessionStopped is emitted from onProcessFinished once the process exits */
}
}

Expand All @@ -111,8 +114,10 @@ void AdapterClient::onResponseReceived(int id, const QString& method, const QJso
{
qCWarning(scopeComm) << "AdapterClient: unexpected non-object result for" << method;
_handshakeTimer.stop();
/* Set IDLE before stop() so onProcessFinished's IDLE guard suppresses any
duplicate sessionError emission when the process exits asynchronously. */
_state = State::IDLE;
QTimer::singleShot(0, this, [this]() { _pProcess->stop(); });
_pProcess->stop();
emit sessionError(QString("Unexpected non-object result for %1").arg(method));
Comment on lines 119 to 121
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Do not switch back to IDLE until the child has actually exited.

AdapterProcess::stop() now returns immediately, but these paths still clear _state early and the timeout branch even emits sessionStopped() before processFinished(). A fast retry can then re-enter prepareAdapter() while the old process is still alive, and the State::STOPPING error path drops its completion signal because the timer is stopped and onProcessFinished() sees IDLE. Keep a non-idle stopping/error state until the process exit is observed, then transition to IDLE.

Also applies to: 130-132, 147-175

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ProtocolAdapter/adapterclient.cpp` around lines 117 - 119, Do not set
_state = State::IDLE immediately after calling _pProcess->stop(); instead leave
the state as STOPPING/ERROR until the child process actually exits, and only
transition to State::IDLE inside onProcessFinished() after processFinished() is
observed; update the three locations that currently clear _state (the Unexpected
non-object path emitting sessionError, the timeout path that emits
sessionStopped(), and the other error/stop branches referenced) to call
_pProcess->stop() without resetting _state, ensure timers/signals are not
prematurely stopped so onProcessFinished() still receives the completion event,
and adjust prepareAdapter()/State::STOPPING handling so fast retries cannot
start a new process while the old process is still running.

}
}
Expand All @@ -125,8 +130,10 @@ void AdapterClient::onErrorReceived(int id, const QString& method, const QJsonOb
qCWarning(scopeComm) << "AdapterClient: error for" << method << ":" << errorMsg;

State previousState = _state;
/* Set IDLE before stop() so onProcessFinished's IDLE guard suppresses any
duplicate sessionError emission when the process exits asynchronously. */
_state = State::IDLE;
QTimer::singleShot(0, this, [this]() { _pProcess->stop(); });
_pProcess->stop();

if (previousState != State::STOPPING)
{
Expand All @@ -137,14 +144,22 @@ void AdapterClient::onErrorReceived(int id, const QString& method, const QJsonOb
void AdapterClient::onProcessError(const QString& message)
{
_handshakeTimer.stop();
_state = State::IDLE;
emit sessionError(message);
if (_state != State::STOPPING)
{
_state = State::IDLE;
emit sessionError(message);
}
}

void AdapterClient::onProcessFinished()
{
_handshakeTimer.stop();
if (_state != State::IDLE)
if (_state == State::STOPPING)
{
_state = State::IDLE;
emit sessionStopped();
}
else if (_state != State::IDLE)
{
_state = State::IDLE;
emit sessionError("Adapter process exited unexpectedly");
Expand All @@ -154,9 +169,35 @@ void AdapterClient::onProcessFinished()
void AdapterClient::onHandshakeTimeout()
{
qCWarning(scopeComm) << "AdapterClient: handshake timed out in state" << static_cast<int>(_state);
bool wasStopping = (_state == State::STOPPING);
_state = State::IDLE;
QTimer::singleShot(0, this, [this]() { _pProcess->stop(); });
emit sessionError("Adapter handshake timed out");
_pProcess->stop();
if (wasStopping)
{
emit sessionStopped();
}
else
{
emit sessionError("Adapter handshake timed out");
}
}
Comment on lines 169 to +183
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

sessionStopped emitted before process has actually exited in timeout path.

When the shutdown timer fires (line 173), sessionStopped is emitted immediately after calling _pProcess->stop(), but before onProcessFinished() is invoked. Since stop() is now non-blocking, this violates the invariant that sessionStopped should only be emitted once the process has truly exited.

Additionally, when onProcessFinished() later fires, _state will already be IDLE, so it will correctly do nothing—but the caller may have already acted on the premature sessionStopped.

Consider keeping _state = State::STOPPING and letting onProcessFinished() emit sessionStopped:

Proposed fix
 void AdapterClient::onHandshakeTimeout()
 {
     qCWarning(scopeComm) << "AdapterClient: handshake timed out in state" << static_cast<int>(_state);
-    bool wasStopping = (_state == State::STOPPING);
-    _state = State::IDLE;
+    if (_state == State::STOPPING)
+    {
+        // Let onProcessFinished emit sessionStopped after the process exits
+        _pProcess->stop();
+        return;
+    }
+    _state = State::STOPPING;
     _pProcess->stop();
-    if (wasStopping)
-    {
-        emit sessionStopped();
-    }
-    else
-    {
-        emit sessionError("Adapter handshake timed out");
-    }
+    // sessionError will not be emitted since we're now STOPPING;
+    // onProcessFinished will emit sessionStopped
 }

Actually, this changes semantics—errors during handshake should still emit sessionError. A cleaner approach may be to introduce an intermediate state or track "timed-out-during-handshake" separately.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ProtocolAdapter/adapterclient.cpp` around lines 165 - 179, In
onHandshakeTimeout(), don't emit sessionStopped immediately after calling
_pProcess->stop() because stop() is non-blocking; instead set _state =
State::STOPPING (or set a new bool like _handshakeTimedOut = true) before
calling _pProcess->stop(), call _pProcess->stop(), and remove the immediate
emit; then update onProcessFinished() to check the new flag/_state and emit
sessionStopped only once the process has actually finished, and if the handshake
timed out but an error condition should be reported emit sessionError there
instead (use symbols onHandshakeTimeout, _pProcess->stop(), _state,
_handshakeTimedOut (or State::STOPPING), onProcessFinished, sessionStopped, and
sessionError).


void AdapterClient::onNotificationReceived(QString method, QJsonValue params)
{
if (method != QStringLiteral("adapter.diagnostic"))
{
return;
}

if (!params.isObject())
{
qCWarning(scopeComm) << "AdapterClient: adapter.diagnostic params is not an object";
return;
}

QJsonObject obj = params.toObject();
emit diagnosticReceived(obj.value(QStringLiteral("level")).toString(),
obj.value(QStringLiteral("message")).toString());
}

void AdapterClient::handleLifecycleResponse(const QString& method, const QJsonObject& result)
Expand Down Expand Up @@ -215,8 +256,7 @@ void AdapterClient::handleLifecycleResponse(const QString& method, const QJsonOb
{
qCInfo(scopeComm) << "AdapterClient: shutdown acknowledged";
_pProcess->stop();
_state = State::IDLE;
emit sessionStopped();
/* sessionStopped is emitted from onProcessFinished once the process exits */
}
else
{
Expand Down
16 changes: 14 additions & 2 deletions src/ProtocolAdapter/adapterclient.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class AdapterClient : public QObject
Q_OBJECT

public:
explicit AdapterClient(AdapterProcess* pProcess, QObject* parent = nullptr);
explicit AdapterClient(AdapterProcess* pProcess, QObject* parent = nullptr, int handshakeTimeoutMs = 10000);
~AdapterClient();

/*!
Expand Down Expand Up @@ -71,7 +71,10 @@ class AdapterClient : public QObject
void requestStatus();

/*!
* \brief Send adapter.shutdown and terminate the adapter process.
* \brief Send adapter.shutdown and signal the adapter process to stop.
*
* The sessionStopped() signal is emitted asynchronously once the process
* has fully exited.
*/
void stopSession();

Expand Down Expand Up @@ -114,6 +117,13 @@ class AdapterClient : public QObject
*/
void sessionStopped();

/*!
* \brief Emitted when an adapter.diagnostic notification is received from the adapter.
* \param level Severity level string: "debug", "info", or "warning".
* \param message The diagnostic message from the adapter.
Comment on lines +120 to +123
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Signal docs are stale for diagnostic severity levels.

The comment lists only "debug", "info", and "warning", but the protocol now includes "error". Please update this docstring to avoid misleading consumers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ProtocolAdapter/adapterclient.h` around lines 117 - 120, Update the
docstring for the signal that is emitted on adapter.diagnostic notifications
(the comment block above the signal declaration in adapterclient.h) to include
the missing "error" severity level; change the severity list from "debug",
"info", or "warning" to include "error" (e.g., "debug", "info", "warning", or
"error") so the documentation matches the current protocol.

*/
void diagnosticReceived(QString level, QString message);

protected:
enum class State
{
Expand All @@ -135,6 +145,7 @@ private slots:
void onProcessError(const QString& message);
void onProcessFinished();
void onHandshakeTimeout();
void onNotificationReceived(QString method, QJsonValue params);

private:
void handleLifecycleResponse(const QString& method, const QJsonObject& result);
Expand All @@ -143,6 +154,7 @@ private slots:

AdapterProcess* _pProcess;
QTimer _handshakeTimer;
int _handshakeTimeoutMs;
QJsonObject _pendingConfig;
QStringList _pendingExpressions;
};
Expand Down
29 changes: 19 additions & 10 deletions src/ProtocolAdapter/adapterprocess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,25 @@
#include "util/scopelogging.h"

#include <QJsonDocument>
#include <QTimer>

static constexpr int cStartTimeoutMs = 3000;
static constexpr int cStopTimeoutMs = 3000;

AdapterProcess::AdapterProcess(QObject* parent) : QObject(parent)
{
_pProcess = new QProcess(this);
_pFramingReader = new FramingReader(this);

_pKillTimer = new QTimer(this);
_pKillTimer->setSingleShot(true);
connect(_pKillTimer, &QTimer::timeout, this, [this]() {
if (_pProcess->state() != QProcess::NotRunning)
{
_pProcess->kill();
}
});

connect(_pProcess, &QProcess::readyReadStandardOutput, this, &AdapterProcess::onReadyReadStdout);
connect(_pProcess, &QProcess::readyReadStandardError, this, &AdapterProcess::onReadyReadStderr);
connect(_pProcess, &QProcess::finished, this, &AdapterProcess::onProcessFinished);
Expand All @@ -25,6 +36,7 @@ bool AdapterProcess::start(const QString& path)
return true;
}

_pKillTimer->stop();
_pendingMethods.clear();
_nextRequestId = 1;

Expand All @@ -49,11 +61,8 @@ void AdapterProcess::stop()
if (_pProcess->state() != QProcess::NotRunning)
{
_pProcess->closeWriteChannel();
if (!_pProcess->waitForFinished(3000))
{
_pProcess->kill();
_pProcess->waitForFinished(1000);
}
_pKillTimer->stop();
_pKillTimer->start(cStopTimeoutMs);
}
}

Expand Down Expand Up @@ -100,11 +109,10 @@ bool AdapterProcess::writeFramed(const QByteArray& json)
qint64 written = _pProcess->write(frame);
if (written != frame.size())
{
emit processError(
QString("Failed to write to adapter process (wrote %1 of %2 bytes, error: %3)")
.arg(written)
.arg(frame.size())
.arg(_pProcess->errorString()));
emit processError(QString("Failed to write to adapter process (wrote %1 of %2 bytes, error: %3)")
.arg(written)
.arg(frame.size())
.arg(_pProcess->errorString()));
return false;
}
return true;
Expand Down Expand Up @@ -171,5 +179,6 @@ void AdapterProcess::onProcessFinished(int exitCode, QProcess::ExitStatus exitSt
{
qCInfo(scopeComm) << "AdapterProcess: process finished, exit code:" << exitCode << "status:" << exitStatus;
_pendingMethods.clear();
_pKillTimer->stop();
emit processFinished();
}
8 changes: 7 additions & 1 deletion src/ProtocolAdapter/adapterprocess.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <QObject>
#include <QProcess>
#include <QString>
#include <QTimer>

/*!
* \brief Transport layer for an external adapter process communicating via JSON-RPC 2.0 over stdio.
Expand Down Expand Up @@ -37,7 +38,11 @@ class AdapterProcess : public QObject
virtual bool start(const QString& path);

/*!
* \brief Kill the adapter process and wait for it to finish.
* \brief Signal the adapter process to stop and return immediately.
*
* Closes stdin so the adapter exits cleanly. If it has not exited within
* the stop timeout, it is killed. The \c processFinished signal is emitted
* asynchronously when the process actually exits.
*/
virtual void stop();

Expand Down Expand Up @@ -100,6 +105,7 @@ private slots:
bool writeFramed(const QByteArray& json);

QProcess* _pProcess;
QTimer* _pKillTimer;
FramingReader* _pFramingReader;
QMap<int, QString> _pendingMethods;
int _nextRequestId{ 1 };
Expand Down
38 changes: 38 additions & 0 deletions src/communication/modbuspoll.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ ModbusPoll::ModbusPoll(SettingsModel* pSettingsModel, QObject* parent) : QObject
connect(_pAdapterClient, &AdapterClient::sessionError, this, [this](QString message) {
qCWarning(scopeComm) << "AdapterClient error:" << message;
_bPollActive = false;
disconnect(_pAdapterClient, &AdapterClient::sessionStopped, this, &ModbusPoll::initAdapter);
});
connect(_pAdapterClient, &AdapterClient::sessionStopped, this, &ModbusPoll::initAdapter);
connect(_pAdapterClient, &AdapterClient::diagnosticReceived, this, &ModbusPoll::onAdapterDiagnostic);
}

ModbusPoll::~ModbusPoll() = default;
Expand All @@ -52,6 +54,10 @@ void ModbusPoll::startCommunication(QList<DataPoint>& registerList)
_registerList = registerList;
_bPollActive = true;

/* Re-establish auto-restart in case it was disconnected by a prior session error */
disconnect(_pAdapterClient, &AdapterClient::sessionStopped, this, &ModbusPoll::initAdapter);
connect(_pAdapterClient, &AdapterClient::sessionStopped, this, &ModbusPoll::initAdapter);

qCInfo(scopeComm) << QString("Start logging: %1").arg(FormatDateTime::currentDateTime());

resetCommunicationStats();
Expand Down Expand Up @@ -119,6 +125,38 @@ void ModbusPoll::onDescribeResult(const QJsonObject& description)
_pSettingsModel->updateAdapterFromDescribe("modbus", description);
}

/*! \brief Route an adapter.diagnostic notification to the diagnostics log.
*
* Maps the adapter's level string to the appropriate Qt logging severity so
* the message flows through ScopeLogging into DiagnosticModel.
*
* \param level Severity string from the adapter: "debug", "info", "warning", or "error".
* \param message The diagnostic message text.
*/
void ModbusPoll::onAdapterDiagnostic(const QString& level, const QString& message)
{
if (level == QStringLiteral("debug"))
{
qCDebug(scopeAdapter) << message;
}
else if (level == QStringLiteral("info"))
{
qCInfo(scopeAdapter) << message;
}
else if (level == QStringLiteral("warning"))
{
qCWarning(scopeAdapter) << message;
}
else if (level == QStringLiteral("error"))
{
qCCritical(scopeAdapter) << message;
}
else
{
qCWarning(scopeAdapter) << "AdapterClient: unknown diagnostic level:" << level << "-" << message;
}
}

QStringList ModbusPoll::buildRegisterExpressions(const QList<DataPoint>& registerList)
{
QStringList expressions;
Expand Down
Loading