Review remarks

Author: jgeudens

src/communication/modbuspoll.cpp

@@ -125,6 +125,9 @@ void ModbusPoll::onDescribeResult(const QJsonObject& description)
     _pAdapterClient->requestRegisterSchema();
 }
 
+/*! \brief Receive the adapter register schema and forward it to the settings model.
+ * \param schema The register schema JSON object returned by adapter.registerSchema.
+ */
 void ModbusPoll::onRegisterSchemaResult(const QJsonObject& schema)
 {
     _pSettingsModel->setAdapterRegisterSchema("modbus", schema);

src/dialogs/addregisterwidget.cpp

@@ -10,6 +10,12 @@
 #include <QJsonArray>
 #include <QVBoxLayout>
 
+/*!
+ * \brief Constructs the widget and populates it from the adapter's register schema.
+ * \param pSettingsModel Pointer to the application settings model.
+ * \param adapterId      Identifier of the adapter whose register schema to use.
+ * \param parent         Optional parent widget.
+ */
 AddRegisterWidget::AddRegisterWidget(SettingsModel* pSettingsModel, const QString& adapterId, QWidget* parent)
     : QWidget(parent),
       _pUi(new Ui::AddRegisterWidget),
@@ -59,6 +65,12 @@ AddRegisterWidget::AddRegisterWidget(SettingsModel* pSettingsModel, const QStrin
         _pUi->cmbDevice->addItem(QString(tr("Device %1").arg(devId)), devId);
     }
 
+    if (deviceList.isEmpty())
+    {
+        _pUi->btnAdd->setEnabled(false);
+        _pUi->cmbDevice->setEnabled(false);
+    }
+
     connect(_pUi->btnAdd, &QPushButton::clicked, this, &AddRegisterWidget::handleResultAccept);
 
     _axisGroup.setExclusive(true);
@@ -75,7 +87,12 @@ AddRegisterWidget::~AddRegisterWidget()
 
 void AddRegisterWidget::handleResultAccept()
 {
-    QString expression = generateExpression();
+    const QString expression = generateExpression();
+    if (expression.isEmpty())
+    {
+        return;
+    }
+
     GraphData graphData;
 
     graphData.setLabel(_pUi->lineName->text());
@@ -107,10 +124,19 @@ void AddRegisterWidget::resetFields()
 
 QString AddRegisterWidget::generateExpression()
 {
+    if (_pUi->cmbDevice->count() == 0)
+    {
+        return QString();
+    }
+
     const QJsonObject addressValues = _pAddressForm->values();
+    if (!addressValues.contains("objectType") || !addressValues.contains("address"))
+    {
+        return QString();
+    }
+
     const QString objectType = addressValues["objectType"].toString();
     const int address = addressValues["address"].toInt();
-
     const QString typeId = _pUi->cmbType->currentData().toString();
 
     deviceId_t deviceId = Device::cFirstDeviceId;

src/dialogs/registerdialog.cpp

@@ -39,7 +39,8 @@ RegisterDialog::RegisterDialog(GraphDataModel* pGraphDataModel, SettingsModel* p
 
     /* Except following columns */
     _pUi->registerView->horizontalHeader()->setSectionResizeMode(GraphDataModel::column::TEXT, QHeaderView::Stretch);
-    _pUi->registerView->horizontalHeader()->setSectionResizeMode(GraphDataModel::column::EXPRESSION, QHeaderView::Stretch);
+    _pUi->registerView->horizontalHeader()->setSectionResizeMode(GraphDataModel::column::EXPRESSION,
+                                                                 QHeaderView::Stretch);
 
     auto triggers = QAbstractItemView::DoubleClicked | QAbstractItemView::EditKeyPressed;
     _pUi->registerView->setEditTriggers(triggers);
@@ -68,20 +69,23 @@ RegisterDialog::RegisterDialog(GraphDataModel* pGraphDataModel, SettingsModel* p
 
     const QStringList ids = _pSettingsModel->adapterIds();
     const QString adapterId = ids.isEmpty() ? QString() : ids.first();
-    auto registerPopupMenu = new AddRegisterWidget(_pSettingsModel, adapterId, this);
-    connect(registerPopupMenu, &AddRegisterWidget::graphDataConfigured, this, &RegisterDialog::addRegister);
+    if (!adapterId.isEmpty())
+    {
+        auto registerPopupMenu = new AddRegisterWidget(_pSettingsModel, adapterId, this);
+        connect(registerPopupMenu, &AddRegisterWidget::graphDataConfigured, this, &RegisterDialog::addRegister);
 
-    _registerPopupAction = std::make_unique<QWidgetAction>(this);
-    _registerPopupAction->setDefaultWidget(registerPopupMenu);
-    _pUi->btnAdd->addAction(_registerPopupAction.get());
+        _registerPopupAction = std::make_unique<QWidgetAction>(this);
+        _registerPopupAction->setDefaultWidget(registerPopupMenu);
+        _pUi->btnAdd->addAction(_registerPopupAction.get());
+    }
 }
 
 RegisterDialog::~RegisterDialog()
 {
     delete _pUi;
 }
 
-void RegisterDialog::addRegister(const GraphData &graphData)
+void RegisterDialog::addRegister(const GraphData& graphData)
 {
     _pGraphDataModel->add(graphData);
 }
@@ -93,10 +97,7 @@ void RegisterDialog::addDefaultRegister()
 
 void RegisterDialog::activatedCell(QModelIndex modelIndex)
 {
-    if (
-        (modelIndex.column() == GraphDataModel::column::COLOR)
-        && (modelIndex.row() < _pGraphDataModel->size())
-        )
+    if ((modelIndex.column() == GraphDataModel::column::COLOR) && (modelIndex.row() < _pGraphDataModel->size()))
     {
         QColor color = QColorDialog::getColor(_pGraphDataModel->color(modelIndex.row()));
 
@@ -107,7 +108,7 @@ void RegisterDialog::activatedCell(QModelIndex modelIndex)
     }
 }
 
-void RegisterDialog::onRegisterInserted(const QModelIndex &parent, int first, int last)
+void RegisterDialog::onRegisterInserted(const QModelIndex& parent, int first, int last)
 {
     Q_UNUSED(parent);
     Q_UNUSED(last);
@@ -119,14 +120,14 @@ void RegisterDialog::onRegisterInserted(const QModelIndex &parent, int first, in
 void RegisterDialog::removeRegisterRow()
 {
     // get list of selected rows
-    QItemSelectionModel *selected = _pUi->registerView->selectionModel();
+    QItemSelectionModel* selected = _pUi->registerView->selectionModel();
     QModelIndexList rowList = selected->selectedRows();
 
     // sort QModelIndexList
     // We need to remove the highest rows first
     std::sort(rowList.begin(), rowList.end(), &RegisterDialog::sortRegistersLastFirst);
 
-    foreach(QModelIndex rowIndex, rowList)
+    foreach (QModelIndex rowIndex, rowList)
     {
         _pGraphDataModel->removeRow(rowIndex.row());
     }
@@ -169,7 +170,7 @@ int RegisterDialog::selectedRowAfterDelete(int deletedStartIndex, int rowCnt)
     return nextSelectedRow;
 }
 
-bool RegisterDialog::sortRegistersLastFirst(const QModelIndex &s1, const QModelIndex &s2)
+bool RegisterDialog::sortRegistersLastFirst(const QModelIndex& s1, const QModelIndex& s2)
 {
     return s1.row() > s2.row();
 }

src/models/adapterdata.cpp

@@ -43,6 +43,8 @@ void AdapterData::setHasStoredConfig(bool hasStoredConfig)
     _hasStoredConfig = hasStoredConfig;
 }
 
+//! \brief Sets the register schema received from adapter.registerSchema.
+//! \param schema The full register schema JSON object.
 void AdapterData::setRegisterSchema(const QJsonObject& schema)
 {
     _registerSchema = schema;
@@ -88,6 +90,8 @@ bool AdapterData::hasStoredConfig() const
     return _hasStoredConfig;
 }
 
+//! \brief Returns the register schema set via setRegisterSchema.
+//! \return The register schema JSON object, or an empty object if not yet set.
 QJsonObject AdapterData::registerSchema() const
 {
     return _registerSchema;

src/util/expressiongenerator.cpp

@@ -1,6 +1,7 @@
 
 #include "expressiongenerator.h"
 #include "models/device.h"
+#include "util/scopelogging.h"
 
 #include <QStringLiteral>
 
@@ -22,8 +23,8 @@ QString typeSuffix(const QString& typeId)
 /*!
  * \brief Returns the single-character address prefix for an object type string.
  * \param objectType One of "coil", "discrete-input", "input-register", "holding-register".
- * \return The corresponding prefix character ("c", "d", "i", or "h").
- *         Returns "h" for unknown values as a safe default.
+ * \return The corresponding prefix character ("c", "d", "i", or "h"),
+ *         or an empty string if \a objectType is not a recognised value.
  */
 QString objectTypeToAddressPrefix(const QString& objectType)
 {
@@ -39,7 +40,12 @@ QString objectTypeToAddressPrefix(const QString& objectType)
     {
         return QStringLiteral("i");
     }
-    return QStringLiteral("h");
+    if (objectType == QStringLiteral("holding-register"))
+    {
+        return QStringLiteral("h");
+    }
+    qCWarning(scopeGeneralInfo) << "objectTypeToAddressPrefix: unknown objectType:" << objectType;
+    return QString();
 }
 
 /*!
@@ -48,11 +54,16 @@ QString objectTypeToAddressPrefix(const QString& objectType)
  * \param address    Zero-based register address within the object type space.
  * \param typeId     Data type identifier (e.g. "16b", "f32b").
  * \param devId      Device identifier; omitted from the string when it equals the first device ID.
- * \return A register expression string such as \c ${h0}, \c ${h5@2:f32b}.
+ * \return A register expression string such as \c ${h0}, \c ${h5@2:f32b},
+ *         or an empty string if \a objectType is not recognised.
  */
 QString constructRegisterString(const QString& objectType, int address, const QString& typeId, deviceId_t devId)
 {
     const QString prefix = objectTypeToAddressPrefix(objectType);
+    if (prefix.isEmpty())
+    {
+        return QString();
+    }
     const QString suffix = typeSuffix(typeId);
     const QString connStr = devId != Device::cFirstDeviceId ? QString("@%1").arg(devId) : QString();
 

tests/ProtocolAdapter/tst_adapterclient.cpp

@@ -761,6 +761,27 @@ void TestAdapterClient::validateRegisterInvalid()
     QCOMPARE(spyValidate.at(0).at(1).toString(), QStringLiteral("Unknown type 'bad'"));
 }
 
+void TestAdapterClient::validateRegisterInActiveState()
+{
+    auto* mock = new MockAdapterProcess();
+    AdapterClient client(mock);
+
+    QSignalSpy spyValidate(&client, &AdapterClient::validateRegisterResult);
+
+    driveToActive(client, mock);
+
+    client.validateRegister(QStringLiteral("${h0}"));
+
+    QCOMPARE(mock->sentRequests.last().method, QStringLiteral("adapter.validateRegister"));
+    QCOMPARE(mock->sentRequests.last().params["expression"].toString(), QStringLiteral("${h0}"));
+
+    mock->injectResponse(5, "adapter.validateRegister", QJsonObject{ { "valid", true } });
+
+    QCOMPARE(spyValidate.count(), 1);
+    QCOMPARE(spyValidate.at(0).at(0).toBool(), true);
+    QCOMPARE(spyValidate.at(0).at(1).toString(), QString());
+}
+
 void TestAdapterClient::validateRegisterInWrongStateIgnored()
 {
     auto* mock = new MockAdapterProcess();

tests/ProtocolAdapter/tst_adapterclient.h

@@ -41,6 +41,7 @@ private slots:
     void describeRegisterInWrongStateIgnored();
     void validateRegisterValid();
     void validateRegisterInvalid();
+    void validateRegisterInActiveState();
     void validateRegisterInWrongStateIgnored();
 };
 

tests/dialogs/tst_addregisterwidget.cpp

@@ -62,6 +62,7 @@ QJsonObject TestAddRegisterWidget::buildTestRegisterSchema()
 
 void TestAddRegisterWidget::init()
 {
+    _settingsModel.removeAllDevice();
     _settingsModel.setAdapterRegisterSchema("modbus", buildTestRegisterSchema());
     _settingsModel.deviceSettings(Device::cFirstDeviceId)->setAdapterId("modbus");
     _pRegWidget = new AddRegisterWidget(&_settingsModel, QStringLiteral("modbus"));

tests/models/tst_adapterdata.cpp

@@ -131,10 +131,7 @@ void TestAdapterData::settingsModelAdapterDataCreatesEntry()
     /* First access creates a default entry */
     const AdapterData* data = model.adapterData("modbus");
     QVERIFY(data != nullptr);
-    if (data == nullptr)
-    {
-        return;
-    }
+    // NOLINTNEXTLINE(clang-analyzer-core.CallAndMessage) -- QVERIFY aborts if null
     QVERIFY(data->name().isEmpty());
 
     /* updateAdapterFromDescribe updates the same entry in place */