Conversation
WalkthroughAdds JSON‑RPC methods for adapter data‑point schema discovery, description, validation and expression building; implements AdapterClient request APIs, introduces AdapterManager to coordinate adapter lifecycle and relay results, persists schemas in SettingsModel/AdapterData, refactors expression construction and updates UI and tests to use async buildExpression flow. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AddRegisterWidget
participant Settings as SettingsModel
participant Manager as AdapterManager
participant Client as AdapterClient
participant Adapter as AdapterProcess/Adapter
UI->>Settings: request adapterData(adapterId).dataPointSchema()
Settings-->>UI: addressSchema, dataTypes, defaultDataType
UI->>UI: build SchemaFormWidget, populate type/device
UI->>Manager: buildExpression(addressFields, dataType, deviceId)
Manager->>Client: send JSON‑RPC `adapter.buildExpression` (request id)
Client->>Adapter: transport JSON‑RPC
Adapter-->>Client: response { expression } (id)
Client-->>Manager: buildExpressionResult(expression) (matching id)
Manager-->>UI: buildExpressionResult(expression)
UI->>UI: emit graphDataConfigured(expression)
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/models/adapterdata.cpp (1)
46-49: Add Doxygen comments for the newly added public methods.Line 46 and Line 91 introduce public API without the source-level brief docs used for the rest of this file.
📝 Proposed fix
+/*! + * \brief Stores the adapter-provided register schema JSON. + * \param schema Register schema object from adapter.registerSchema. + */ void AdapterData::setRegisterSchema(const QJsonObject& schema) { _registerSchema = schema; } @@ +/*! + * \brief Returns the stored adapter register schema JSON. + */ QJsonObject AdapterData::registerSchema() const { return _registerSchema; }As per coding guidelines, "Document all public functions with brief Doxygen comments in the source file".
Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/adapterdata.cpp` around lines 46 - 49, Add brief Doxygen comments above the new public functions to match the file’s existing style: add a /// `@brief` line (and `@param` / `@return` where applicable) immediately above AdapterData::setRegisterSchema(const QJsonObject& schema) describing what the setter does and its parameter, and do the same for the other newly added public method(s) introduced at lines 91-94 (the register-schema-related getter), including a short `@return` description; follow the same comment formatting used by the other public functions in this source file.tests/ProtocolAdapter/tst_adapterclient.cpp (1)
724-762: Consider addingvalidateRegisterInActiveStatetest.
describeRegisterhas tests for bothAWAITING_CONFIG(line 666) andACTIVE(line 691) states, butvalidateRegisteronly tests theAWAITING_CONFIGstate. Since the implementation (context snippet 4) permits both states, a test forACTIVEwould ensure parity.🧪 Proposed additional test case
void TestAdapterClient::validateRegisterInActiveState() { auto* mock = new MockAdapterProcess(); AdapterClient client(mock); QSignalSpy spyValidate(&client, &AdapterClient::validateRegisterResult); driveToActive(client, mock); client.validateRegister(QStringLiteral("${40001: 16b}")); QCOMPARE(mock->sentRequests.last().method, QStringLiteral("adapter.validateRegister")); mock->injectResponse(5, "adapter.validateRegister", QJsonObject{ { "valid", true } }); QCOMPARE(spyValidate.count(), 1); QCOMPARE(spyValidate.at(0).at(0).toBool(), true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ProtocolAdapter/tst_adapterclient.cpp` around lines 724 - 762, Add a new test that mirrors the existing validateRegister coverage for AWAITING_CONFIG but runs in the ACTIVE state: create a TestAdapterClient::validateRegisterInActiveState that constructs MockAdapterProcess and AdapterClient, attaches a QSignalSpy to AdapterClient::validateRegisterResult, calls driveToActive(client, mock), invokes client.validateRegister(...) and asserts the last sent request method is "adapter.validateRegister", then injects a successful response via mock->injectResponse(...) and verifies the spy received the expected valid=true result; reference validateRegister, validateRegisterResult, driveToActive, MockAdapterProcess and AdapterClient when adding this test to ensure parity with the describeRegister ACTIVE case.src/communication/modbuspoll.cpp (1)
128-131: Add Doxygen comment for the new slot.The new
onRegisterSchemaResultslot is missing a brief Doxygen comment, unlike the other handlers in this file (e.g.,onAdapterDiagnosticat line 133 has one). As per coding guidelines: "Document all public functions with brief Doxygen comments in the source file".📝 Proposed Doxygen comment
+/*! \brief Store the adapter's register schema in settings. + * + * Called when the adapter responds to an adapter.registerSchema request. + * \param schema The full register schema object from the adapter. + */ void ModbusPoll::onRegisterSchemaResult(const QJsonObject& schema) { _pSettingsModel->setAdapterRegisterSchema("modbus", schema); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/communication/modbuspoll.cpp` around lines 128 - 131, Add a brief Doxygen comment above the ModbusPoll::onRegisterSchemaResult slot similar to other handlers (e.g., onAdapterDiagnostic) describing that this slot receives the adapter register schema and passes it to the settings model; place the short /** ... */ comment immediately above the onRegisterSchemaResult declaration/definition, mention parameters (const QJsonObject& schema) and the brief purpose (calls _pSettingsModel->setAdapterRegisterSchema("modbus", schema)), and follow the project's one-line Doxygen style for public functions.tests/models/tst_adapterdata.cpp (1)
134-137: Redundant null-pointer guard afterQVERIFY.
QVERIFY(data != nullptr)at line 133 already aborts the test on failure, making the subsequentif (data == nullptr) return;unreachable. This pattern was likely added for static analysis suppression, but it adds confusion without benefit.🔧 Option: remove the redundant check
const AdapterData* data = model.adapterData("modbus"); QVERIFY(data != nullptr); - if (data == nullptr) - { - return; - } QVERIFY(data->name().isEmpty());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/models/tst_adapterdata.cpp` around lines 134 - 137, Remove the redundant null-pointer guard that follows the test assertion: the QVERIFY(data != nullptr) already aborts on failure, so delete the subsequent if (data == nullptr) { return; } block; locate the QVERIFY(data != nullptr) assertion and remove the explicit post-check for the pointer named data (or replace it with an appropriate static-analysis annotation only if required).src/dialogs/addregisterwidget.cpp (1)
13-18: Add source Doxygen for the public constructor.The constructor definition was changed here, but the source file still has no brief Doxygen block for it. As per coding guidelines, "Document all public functions with brief Doxygen comments in the source file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dialogs/addregisterwidget.cpp` around lines 13 - 18, Add a brief Doxygen comment immediately above the AddRegisterWidget constructor definition (AddRegisterWidget::AddRegisterWidget) in the source file: include a one-line \brief describing the constructor and short `@param` tags for SettingsModel* pSettingsModel, const QString& adapterId and QWidget* parent; use the project Doxygen style (e.g. /*! \brief ... */ or ///) so the public constructor is documented per guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dialogs/addregisterwidget.cpp`:
- Around line 108-123: AddRegisterWidget::generateExpression currently assumes
SchemaFormWidget::values() contains "objectType" and "address" and
unconditionally calls ExpressionGenerator::constructRegisterString, which
hard-wires a Modbus schema; instead, check _pAddressForm->values() for the
presence and validity of "objectType" and "address" before using them (e.g. only
call constructRegisterString when both keys exist and are of expected types) or,
preferable, delegate expression construction to an adapter-facing API on the
address form (e.g. add a method on the form like
createRegisterExpression()/toExpression() that returns an optional string) and
call that from generateExpression so different adapters can supply their own
serialization; ensure you still handle deviceId/typeId extraction via
_pUi->cmbDevice and _pUi->cmbType but do not assume address keys exist when
delegating.
- Around line 54-60: The device combo population must handle an empty device
list: after calling _pSettingsModel->deviceListForAdapter(adapterId) check if
deviceList is empty and, if so, disable or hide the Add action (e.g.,
_pUi->btnAdd->setEnabled(false)) and disable the combo
(_pUi->cmbDevice->setEnabled(false)); also ensure the later code that currently
falls back to Device::cFirstDeviceId uses the combo count/currentIndex to detect
"no selection" and abort/return instead of emitting a register for a
non-existent device (update the selection logic that reads cmbDevice and the
fallback to Device::cFirstDeviceId).
In `@src/dialogs/registerdialog.cpp`:
- Around line 69-71: The code constructs AddRegisterWidget using adapterId even
when adapterIds() is empty (adapterId == QString()), which can leave the form
uninitialised; change the logic so you only create registerPopupMenu when a
valid adapter ID exists — e.g., compute ids via _pSettingsModel->adapterIds(),
find the first non-empty/valid adapter id (or test adapter validity via an
existing model method), and only call new AddRegisterWidget(_pSettingsModel,
adapterId, this) when adapterId is non-empty and valid; otherwise avoid creating
registerPopupMenu (or create a disabled/placeholder widget) and ensure any UI
actions that open it are disabled.
In `@src/util/expressiongenerator.cpp`:
- Around line 22-43: The function objectTypeToAddressPrefix currently defaults
unknown object types to "h"; change it to fail fast instead: validate the input
against the allowed values
("coil","discrete-input","input-register","holding-register") in
objectTypeToAddressPrefix and, on any other value, throw a descriptive exception
(e.g., std::invalid_argument with the offending objectType) or return an
explicit error indicator (empty QString) and log the error — choose the
project-consistent error strategy and update callers to handle the failure.
Ensure the error message includes the invalid objectType so typos/future values
are obvious, and remove the silent fallback to "h".
In `@tests/dialogs/tst_addregisterwidget.cpp`:
- Around line 63-68: The test fixture's shared _settingsModel is retaining state
across tests because registerDevice() adds a device; fix by resetting or
recreating the model for each test in TestAddRegisterWidget::init() (or fully
clearing it in cleanup()). Concretely, replace the current init() setup with
code that reinitializes _settingsModel (e.g. _settingsModel = SettingsModel();)
before calling _settingsModel.setAdapterRegisterSchema(...) and creating
AddRegisterWidget, or add an explicit clear/reset call (e.g.
_settingsModel.clearDevices() or similar API) in cleanup() so registerDevice()
side-effects do not persist between tests. Ensure you modify
TestAddRegisterWidget::init(), _settingsModel usage, and/or
TestAddRegisterWidget::cleanup() accordingly.
---
Nitpick comments:
In `@src/communication/modbuspoll.cpp`:
- Around line 128-131: Add a brief Doxygen comment above the
ModbusPoll::onRegisterSchemaResult slot similar to other handlers (e.g.,
onAdapterDiagnostic) describing that this slot receives the adapter register
schema and passes it to the settings model; place the short /** ... */ comment
immediately above the onRegisterSchemaResult declaration/definition, mention
parameters (const QJsonObject& schema) and the brief purpose (calls
_pSettingsModel->setAdapterRegisterSchema("modbus", schema)), and follow the
project's one-line Doxygen style for public functions.
In `@src/dialogs/addregisterwidget.cpp`:
- Around line 13-18: Add a brief Doxygen comment immediately above the
AddRegisterWidget constructor definition (AddRegisterWidget::AddRegisterWidget)
in the source file: include a one-line \brief describing the constructor and
short `@param` tags for SettingsModel* pSettingsModel, const QString& adapterId
and QWidget* parent; use the project Doxygen style (e.g. /*! \brief ... */ or
///) so the public constructor is documented per guidelines.
In `@src/models/adapterdata.cpp`:
- Around line 46-49: Add brief Doxygen comments above the new public functions
to match the file’s existing style: add a /// `@brief` line (and `@param` / `@return`
where applicable) immediately above AdapterData::setRegisterSchema(const
QJsonObject& schema) describing what the setter does and its parameter, and do
the same for the other newly added public method(s) introduced at lines 91-94
(the register-schema-related getter), including a short `@return` description;
follow the same comment formatting used by the other public functions in this
source file.
In `@tests/models/tst_adapterdata.cpp`:
- Around line 134-137: Remove the redundant null-pointer guard that follows the
test assertion: the QVERIFY(data != nullptr) already aborts on failure, so
delete the subsequent if (data == nullptr) { return; } block; locate the
QVERIFY(data != nullptr) assertion and remove the explicit post-check for the
pointer named data (or replace it with an appropriate static-analysis annotation
only if required).
In `@tests/ProtocolAdapter/tst_adapterclient.cpp`:
- Around line 724-762: Add a new test that mirrors the existing validateRegister
coverage for AWAITING_CONFIG but runs in the ACTIVE state: create a
TestAdapterClient::validateRegisterInActiveState that constructs
MockAdapterProcess and AdapterClient, attaches a QSignalSpy to
AdapterClient::validateRegisterResult, calls driveToActive(client, mock),
invokes client.validateRegister(...) and asserts the last sent request method is
"adapter.validateRegister", then injects a successful response via
mock->injectResponse(...) and verifies the spy received the expected valid=true
result; reference validateRegister, validateRegisterResult, driveToActive,
MockAdapterProcess and AdapterClient when adding this test to ensure parity with
the describeRegister ACTIVE case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43887b6d-34d2-446d-9755-490bdc7e915a
📒 Files selected for processing (23)
adapters/json-rpc-spec.mdsrc/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.hsrc/dialogs/addregisterwidget.cppsrc/dialogs/addregisterwidget.hsrc/dialogs/addregisterwidget.uisrc/dialogs/registerdialog.cppsrc/importexport/mbcregisterdata.cppsrc/models/adapterdata.cppsrc/models/adapterdata.hsrc/models/settingsmodel.cppsrc/models/settingsmodel.hsrc/util/expressiongenerator.cppsrc/util/expressiongenerator.hsrc/util/expressionregex.cpptests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/dialogs/tst_addregisterwidget.cpptests/dialogs/tst_addregisterwidget.htests/models/tst_adapterdata.cpptests/models/tst_adapterdata.h
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/ProtocolAdapter/adapterclient.cpp (1)
83-121: Add source-file Doxygen for the remaining public methods.
requestRegisterSchema(),describeRegister(), andvalidateRegister()are new public entry points, but onlybuildExpression()has a source-file doc block here.As per coding guidelines,
**/*.{cpp,cxx}: Document all public functions with brief Doxygen comments in the source file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adapterclient.cpp` around lines 83 - 121, Add source-file Doxygen comments for the three public methods requestRegisterSchema(), describeRegister(const QString&), and validateRegister(const QString&) above each function definition; each comment should be a brief Doxygen block describing the purpose, parameters (e.g., `@param` expression for describeRegister/validateRegister) and any relevant behavior (state preconditions like State::AWAITING_CONFIG/State::ACTIVE) to match the existing buildExpression() style and the project's documentation guideline.tests/dialogs/tst_addregisterwidget.cpp (1)
138-154: Build the expected expression fromdevId2.Line 151 and Line 153 still hard-code
@2even though the test already captures the real id fromaddNewDevice(). That makes this assertion brittle if device ids ever stop being sequential or start from a different base.🔧 Suggested tweak
- addRegister(graphData, QStringLiteral("${h0@2}")); + const QString expectedExpression = QStringLiteral("${h0@%1}").arg(devId2); + addRegister(graphData, expectedExpression); - QCOMPARE(graphData.expression(), QStringLiteral("${h0@2}")); + QCOMPARE(graphData.expression(), expectedExpression);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dialogs/tst_addregisterwidget.cpp` around lines 138 - 154, The test currently hard-codes "@2" in the expected expression making it brittle; change the assertions to build the expected expression from the actual device id returned by addNewDevice() (devId2) instead of literal "@2" — e.g. construct the expected expression using devId2 when calling addRegister and when comparing GraphData::expression(), and keep the existing assertion that _pMockModbusPoll->buildCalls[0].deviceId equals devId2 so the test no longer assumes sequential or fixed ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adapters/json-rpc-spec.md`:
- Around line 407-412: The spec for adapter.buildExpression is incomplete: add a
formal request schema describing the JSON payload (include "fields" object for
register components, optional "dataType" string and optional "deviceId" number),
document omission rules (omit "dataType" when empty/blank and omit "deviceId"
when 0), and define the response contract as a JSON object with a single
"expression" string (e.g., { "expression": "..." }); update the
adapter.buildExpression section to explicitly state these input and output
shapes so implementers and tests use the same public protocol.
In `@src/dialogs/addregisterwidget.cpp`:
- Around line 79-80: The buildExpressionResult signal from ModbusPoll is a
broadcast (QString) and AddRegisterWidget's onBuildExpressionResult will treat
any emission as its own reply; update the flow so replies are correlated to the
originating request: either (preferred) extend ModbusPoll::buildExpressionResult
to include a request identifier (e.g., requestId or sender pointer) and emit
that id along with the QString, then have AddRegisterWidget call
handleResultAccept set a temporary requestId/_inFlight flag and only accept
results whose id matches in onBuildExpressionResult; or (quick) make
AddRegisterWidget create a scoped connection when sending the build request
(connect a lambda capturing a local inFlight token or the exact request
parameters) and disconnect it when complete, and in onBuildExpressionResult
guard against handling when _pendingGraphData is empty/not in-flight before
emitting graphDataConfigured. Ensure references to
ModbusPoll::buildExpressionResult, AddRegisterWidget::handleResultAccept,
AddRegisterWidget::onBuildExpressionResult, _pendingGraphData, and
graphDataConfigured are updated accordingly.
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 83-148: The RPCs requestRegisterSchema(), describeRegister(),
validateRegister(), and buildExpression() can race with state changes and
out-of-order replies because responses aren’t matched to the originating
request; add per-RPC pending request-id tracking and ignore superseded replies.
Store the id returned when calling _pProcess->sendRequest (or generate a unique
local id) into fields like _pendingRegisterSchemaRequestId,
_pendingDescribeRequestId, _pendingValidateRequestId,
_pendingBuildExpressionRequestId (or a QHash<QString,qint64> keyed by method
name), then update onResponseReceived() to compare the incoming JSON-RPC id
against the stored id for that method, discard responses that don't match, and
clear the stored id when the matching response is handled (also clear/replace
when a new request for the same RPC is sent).
---
Nitpick comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 83-121: Add source-file Doxygen comments for the three public
methods requestRegisterSchema(), describeRegister(const QString&), and
validateRegister(const QString&) above each function definition; each comment
should be a brief Doxygen block describing the purpose, parameters (e.g., `@param`
expression for describeRegister/validateRegister) and any relevant behavior
(state preconditions like State::AWAITING_CONFIG/State::ACTIVE) to match the
existing buildExpression() style and the project's documentation guideline.
In `@tests/dialogs/tst_addregisterwidget.cpp`:
- Around line 138-154: The test currently hard-codes "@2" in the expected
expression making it brittle; change the assertions to build the expected
expression from the actual device id returned by addNewDevice() (devId2) instead
of literal "@2" — e.g. construct the expected expression using devId2 when
calling addRegister and when comparing GraphData::expression(), and keep the
existing assertion that _pMockModbusPoll->buildCalls[0].deviceId equals devId2
so the test no longer assumes sequential or fixed ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6facb36-bf93-47e1-ab77-ea1c30f98fba
📒 Files selected for processing (19)
adapters/json-rpc-spec.mdsrc/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.hsrc/dialogs/addregisterwidget.cppsrc/dialogs/addregisterwidget.hsrc/dialogs/mainwindow.cppsrc/dialogs/registerdialog.cppsrc/dialogs/registerdialog.hsrc/importexport/mbcregisterdata.cppsrc/models/adapterdata.cppsrc/util/expressiongenerator.cppsrc/util/expressiongenerator.htests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/dialogs/tst_addregisterwidget.cpptests/dialogs/tst_addregisterwidget.htests/models/tst_adapterdata.cpp
💤 Files with no reviewable changes (2)
- src/util/expressiongenerator.h
- src/util/expressiongenerator.cpp
✅ Files skipped from review due to trivial changes (1)
- src/models/adapterdata.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/communication/modbuspoll.h
- src/dialogs/addregisterwidget.h
- src/importexport/mbcregisterdata.cpp
- tests/ProtocolAdapter/tst_adapterclient.h
- src/dialogs/registerdialog.cpp
Introduces AdapterManager in src/ProtocolAdapter/ to own AdapterProcess and AdapterClient and handle the full adapter lifecycle: initialization, describe, schema retrieval, configuration, diagnostics, and expression building. ModbusPoll is reduced to polling-only responsibilities: timer management, read requests, and emitting registerDataReady. The diagnostic tests move from tst_modbuspoll to a new tst_adaptermanager in tests/ProtocolAdapter/. https://claude.ai/code/session_01AtVwF9u3BD3kqhLUfubEaX
AddRegisterWidget and RegisterDialog now reference AdapterManager directly instead of going through ModbusPoll. ModbusPoll gains an adapterManager() getter so MainWindow can pass the manager to RegisterDialog without knowing its internals. AdapterManager::buildExpression is marked virtual to allow test subclassing. MockModbusPoll in tst_addregisterwidget is replaced by MockAdapterManager, which overrides the virtual method and injects results via the buildExpressionResult signal. https://claude.ai/code/session_01AtVwF9u3BD3kqhLUfubEaX
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/ProtocolAdapter/adapterclient.cpp (1)
328-343:⚠️ Potential issue | 🟠 MajorCorrelate auxiliary RPC responses by request id, not only method/state.
These branches still accept replies solely by
method+ current_state, so delayed/out-of-order responses can be misrouted or dropped (especially around state transitions). Please track pending request ids per auxiliary RPC and ignore stale/non-matching replies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adapterclient.cpp` around lines 328 - 343, The current branches emit auxiliary RPC results based only on method and _state; change them to correlate replies by request id: when sending auxiliary RPCs (adapter.dataPointSchema, adapter.describeDataPoint, adapter.validateDataPoint, adapter.buildExpression) record the outgoing request id in a pending map keyed by method (e.g., pendingRequests["adapter.describeDataPoint"] = id); in the response-handling code check result["id"] (or the message id field) against the pending id for that method and only emit dataPointSchemaResult, describeDataPointResult, validateDataPointResult, or buildExpressionResult if the ids match; remove the id from the pending map after handling and ignore/stale responses when there is no match or the client has transitioned state (clear relevant pending ids on state changes).src/ProtocolAdapter/adaptermanager.h (1)
63-63:⚠️ Potential issue | 🟠 MajorCorrelate build-expression replies to the request that triggered them.
buildExpressionResult(QString)is a shared broadcast onAdapterManager, but the UI now attaches widget instances directly to it. If two dialogs are open, or one late reply arrives after another click, the wrong widget can accept the expression and emitgraphDataConfiguredwith stale pending state. Carry a request token or requester context through both the request and the reply.Possible API direction
+#include <QUuid> ... - virtual void buildExpression(const QJsonObject& addressFields, const QString& dataType, deviceId_t deviceId); + virtual QUuid buildExpression(const QJsonObject& addressFields, const QString& dataType, deviceId_t deviceId); ... - void buildExpressionResult(QString expression); + void buildExpressionResult(QUuid requestId, QString expression);Also applies to: 95-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adaptermanager.h` at line 63, The buildExpression request/reply path is using a shared broadcast (buildExpressionResult(QString)) so replies can be handled by the wrong UI when multiple dialogs are open; add a request token or requester context to the API and carry it through the call and the signal so replies can be correlated. Concretely, change AdapterManager::buildExpression(...) to accept an extra requestId/requesterContext (e.g. quint64 requestId or QObject* requester) and update all other buildExpression overloads accordingly, then change the emitted signal buildExpressionResult(QString) to include that same token (e.g. buildExpressionResult(QString, quint64) or buildExpressionResult(QString, QObject*)) and update all call sites and UI connections to filter by or match the token before applying graphDataConfigured. Ensure the token is forwarded from the caller that opens the dialog and that the dialog only reacts to replies with its matching token.
🧹 Nitpick comments (2)
adapters/json-rpc-spec.md (1)
409-409: Optional wording tidy-up in method description.“component parts” is slightly redundant; “components” reads tighter.
✏️ Suggested doc tweak
-Constructs a register expression string from its component parts. +Constructs a register expression string from its components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapters/json-rpc-spec.md` at line 409, Update the method description text that currently reads "Constructs a register expression string from its component parts." to use tighter wording: replace "component parts" with "components" so it reads "Constructs a register expression string from its components." Locate and edit this sentence in adapters/json-rpc-spec.md where the register expression constructor is documented.src/ProtocolAdapter/adaptermanager.cpp (1)
11-24: Add the missing source-file Doxygen for the constructor.
AdapterManager::AdapterManager(...)is public, but it is the only new public function here without a brief Doxygen block in the.cpp.As per coding guidelines, "Document all public functions with brief Doxygen comments in the source file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adaptermanager.cpp` around lines 11 - 24, Add a brief Doxygen comment block above the AdapterManager::AdapterManager(SettingsModel* pSettingsModel, QObject* parent) constructor in adaptermanager.cpp documenting its purpose and parameters (e.g., brief description and `@param` tags for pSettingsModel and parent), matching the project's source-file Doxygen style used for other public functions so the constructor is documented consistently with coding guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 139-142: The check in adapterclient.cpp currently treats
whitespace-only strings like dataType = " " as non-empty and includes them in
params; update the logic around dataType (the variable and the
params["dataType"] insertion) to trim whitespace (e.g., use QString::trimmed()
or equivalent) and only set params["dataType"] when trimmed dataType is not
empty, so that whitespace-only values are treated as omitted.
- Around line 83-121: Add brief Doxygen-style comments above the three new
public methods requestDataPointSchema(), describeDataPoint(const QString&), and
validateDataPoint(const QString&) in adapterclient.cpp (same style as the
existing buildExpression comment): include a one-line description, `@param` for
parameters where applicable, and `@see` or `@return` if relevant; place the comments
immediately above each method definition and keep wording consistent with
project style guidelines.
In `@src/ProtocolAdapter/adaptermanager.cpp`:
- Around line 31-34: AdapterManager::initAdapter builds the adapter path without
an executable suffix, which breaks on Windows; update initAdapter to derive the
platform-specific executable suffix from the running application (use
QFileInfo(QCoreApplication::applicationFilePath()).suffix()) and construct
adapterPath via QDir::filePath("modbusadapter" + (suffix.isEmpty() ? "" : "." +
suffix)) before calling _pAdapterClient->prepareAdapter(adapterPath). Ensure you
use QCoreApplication::applicationDirPath() as the base directory and
QDir::filePath to join segments so the resulting path points to the correct
executable on all platforms.
---
Duplicate comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 328-343: The current branches emit auxiliary RPC results based
only on method and _state; change them to correlate replies by request id: when
sending auxiliary RPCs (adapter.dataPointSchema, adapter.describeDataPoint,
adapter.validateDataPoint, adapter.buildExpression) record the outgoing request
id in a pending map keyed by method (e.g.,
pendingRequests["adapter.describeDataPoint"] = id); in the response-handling
code check result["id"] (or the message id field) against the pending id for
that method and only emit dataPointSchemaResult, describeDataPointResult,
validateDataPointResult, or buildExpressionResult if the ids match; remove the
id from the pending map after handling and ignore/stale responses when there is
no match or the client has transitioned state (clear relevant pending ids on
state changes).
In `@src/ProtocolAdapter/adaptermanager.h`:
- Line 63: The buildExpression request/reply path is using a shared broadcast
(buildExpressionResult(QString)) so replies can be handled by the wrong UI when
multiple dialogs are open; add a request token or requester context to the API
and carry it through the call and the signal so replies can be correlated.
Concretely, change AdapterManager::buildExpression(...) to accept an extra
requestId/requesterContext (e.g. quint64 requestId or QObject* requester) and
update all other buildExpression overloads accordingly, then change the emitted
signal buildExpressionResult(QString) to include that same token (e.g.
buildExpressionResult(QString, quint64) or buildExpressionResult(QString,
QObject*)) and update all call sites and UI connections to filter by or match
the token before applying graphDataConfigured. Ensure the token is forwarded
from the caller that opens the dialog and that the dialog only reacts to replies
with its matching token.
---
Nitpick comments:
In `@adapters/json-rpc-spec.md`:
- Line 409: Update the method description text that currently reads "Constructs
a register expression string from its component parts." to use tighter wording:
replace "component parts" with "components" so it reads "Constructs a register
expression string from its components." Locate and edit this sentence in
adapters/json-rpc-spec.md where the register expression constructor is
documented.
In `@src/ProtocolAdapter/adaptermanager.cpp`:
- Around line 11-24: Add a brief Doxygen comment block above the
AdapterManager::AdapterManager(SettingsModel* pSettingsModel, QObject* parent)
constructor in adaptermanager.cpp documenting its purpose and parameters (e.g.,
brief description and `@param` tags for pSettingsModel and parent), matching the
project's source-file Doxygen style used for other public functions so the
constructor is documented consistently with coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce6b1238-1891-4d06-b1e1-921d9995d447
⛔ Files ignored due to path filters (2)
adapters/dummymodbusadapter.exeis excluded by!**/*.exeadapters/modbusadapter.exeis excluded by!**/*.exe
📒 Files selected for processing (28)
adapters/dummymodbusadapteradapters/json-rpc-spec.mdadapters/modbusadaptersrc/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/ProtocolAdapter/adaptermanager.cppsrc/ProtocolAdapter/adaptermanager.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.hsrc/dialogs/addregisterwidget.cppsrc/dialogs/addregisterwidget.hsrc/dialogs/mainwindow.cppsrc/dialogs/registerdialog.cppsrc/dialogs/registerdialog.hsrc/models/adapterdata.cppsrc/models/adapterdata.hsrc/models/settingsmodel.cppsrc/models/settingsmodel.htests/ProtocolAdapter/CMakeLists.txttests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/ProtocolAdapter/tst_adaptermanager.cpptests/ProtocolAdapter/tst_adaptermanager.htests/communication/CMakeLists.txttests/dialogs/tst_addregisterwidget.cpptests/dialogs/tst_addregisterwidget.htests/models/tst_adapterdata.cpptests/models/tst_adapterdata.h
💤 Files with no reviewable changes (1)
- tests/communication/CMakeLists.txt
✅ Files skipped from review due to trivial changes (2)
- tests/ProtocolAdapter/CMakeLists.txt
- tests/models/tst_adapterdata.h
🚧 Files skipped from review as they are similar to previous changes (9)
- src/models/settingsmodel.cpp
- src/models/settingsmodel.h
- tests/models/tst_adapterdata.cpp
- tests/ProtocolAdapter/tst_adapterclient.cpp
- src/dialogs/registerdialog.cpp
- src/ProtocolAdapter/adapterclient.h
- tests/ProtocolAdapter/tst_adapterclient.h
- src/dialogs/mainwindow.cpp
- src/communication/modbuspoll.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ProtocolAdapter/adapterclient.h (1)
162-167:⚠️ Potential issue | 🟡 MinorUpdate diagnostic level documentation to include "error".
The Doxygen comment lists
"debug","info", or"warning"but the implementation inadaptermanager.cppalso handles"error"level, which is documented in the JSON-RPC spec.📝 Suggested fix
/*! * \brief Emitted when an adapter.diagnostic notification is received from the adapter. - * \param level Severity level string: "debug", "info", or "warning". + * \param level Severity level string: "debug", "info", "warning", or "error". * \param message The diagnostic message from the adapter. */ void diagnosticReceived(QString level, QString message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adapterclient.h` around lines 162 - 167, Update the Doxygen for the signal diagnosticReceived(QString level, QString message) to include "error" in the list of severity levels (e.g., "debug", "info", "warning", or "error") so it matches the implementation in adaptermanager.cpp and the JSON-RPC spec; edit the comment above diagnosticReceived to mention "error" as a valid severity string and ensure wording remains consistent with other doc comments.
🧹 Nitpick comments (1)
src/communication/modbuspoll.cpp (1)
23-27: Consider extracting the lambda to a named slot.This lambda contains multiple statements. Per coding guidelines, lambdas with more than two captures or multiple statements should use named functions for clarity.
♻️ Proposed refactor
In the header, add a private slot:
private slots: void onAdapterSessionError(QString message);In the source:
- connect(_pAdapterManager, &AdapterManager::sessionError, this, [this](QString message) { - qCWarning(scopeComm) << "AdapterManager error:" << message; - _bPollActive = false; - disconnect(_pAdapterManager, &AdapterManager::sessionStopped, this, &ModbusPoll::initAdapter); - }); + connect(_pAdapterManager, &AdapterManager::sessionError, this, &ModbusPoll::onAdapterSessionError);void ModbusPoll::onAdapterSessionError(QString message) { qCWarning(scopeComm) << "AdapterManager error:" << message; _bPollActive = false; disconnect(_pAdapterManager, &AdapterManager::sessionStopped, this, &ModbusPoll::initAdapter); }As per coding guidelines: "Avoid lambda expressions with more than 2 captures or multiple statements; use named functions instead for clarity".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/communication/modbuspoll.cpp` around lines 23 - 27, Extract the multi-statement lambda connected to AdapterManager::sessionError into a named private slot on ModbusPoll: declare a slot void onAdapterSessionError(QString message) in the class, implement ModbusPoll::onAdapterSessionError to log the message with qCWarning(scopeComm), set _bPollActive = false, and disconnect _pAdapterManager's sessionStopped signal from ModbusPoll::initAdapter; then replace the lambda connection with a direct connect(_pAdapterManager, &AdapterManager::sessionError, this, &ModbusPoll::onAdapterSessionError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ProtocolAdapter/adapterclient.h`:
- Around line 162-167: Update the Doxygen for the signal
diagnosticReceived(QString level, QString message) to include "error" in the
list of severity levels (e.g., "debug", "info", "warning", or "error") so it
matches the implementation in adaptermanager.cpp and the JSON-RPC spec; edit the
comment above diagnosticReceived to mention "error" as a valid severity string
and ensure wording remains consistent with other doc comments.
---
Nitpick comments:
In `@src/communication/modbuspoll.cpp`:
- Around line 23-27: Extract the multi-statement lambda connected to
AdapterManager::sessionError into a named private slot on ModbusPoll: declare a
slot void onAdapterSessionError(QString message) in the class, implement
ModbusPoll::onAdapterSessionError to log the message with qCWarning(scopeComm),
set _bPollActive = false, and disconnect _pAdapterManager's sessionStopped
signal from ModbusPoll::initAdapter; then replace the lambda connection with a
direct connect(_pAdapterManager, &AdapterManager::sessionError, this,
&ModbusPoll::onAdapterSessionError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 948ff9cb-32d1-42df-80ae-200ec8c3faad
⛔ Files ignored due to path filters (2)
adapters/dummymodbusadapter.exeis excluded by!**/*.exeadapters/modbusadapter.exeis excluded by!**/*.exe
📒 Files selected for processing (7)
adapters/dummymodbusadapteradapters/json-rpc-spec.mdadapters/modbusadaptersrc/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/ProtocolAdapter/adaptermanager.cppsrc/communication/modbuspoll.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests