Introduce timout handling and reconnect. Update sunspec connection state handling

libnymea-modbus/modbustcpmaster.cpp

@@ -453,7 +453,7 @@ QUuid ModbusTCPMaster::writeHoldingRegister(uint slaveAddress, uint registerAddr
 
 void ModbusTCPMaster::onModbusErrorOccurred(QModbusDevice::Error error)
 {
-    qCWarning(dcModbusTcpMaster()) << "An error occured" << error;
+    qCWarning(dcModbusTcpMaster()) << "An error occurred" << error;
 }
 
 void ModbusTCPMaster::onModbusStateChanged(QModbusDevice::State state)

libnymea-sunspec/sunspecconnection.cpp

@@ -41,6 +41,7 @@ SunSpecConnection::SunSpecConnection(const QHostAddress &hostAddress, uint port,
     m_port(port),
     m_slaveId(slaveId)
 {
+    qCDebug(dcSunSpec()) << "Creating connection for" << QString("%1:%2").arg(m_hostAddress.toString()).arg(m_port);
     createConnection();
 }
 
@@ -51,6 +52,7 @@ SunSpecConnection::SunSpecConnection(const QHostAddress &hostAddress, uint port,
     m_slaveId(slaveId),
     m_byteOrder(byteOrder)
 {
+    qCDebug(dcSunSpec()) << "Creating connection for" << QString("%1:%2").arg(m_hostAddress.toString()).arg(m_port);
     createConnection();
 }
 
@@ -68,8 +70,6 @@ void SunSpecConnection::setHostAddress(const QHostAddress &hostAddress)
 {
     m_hostAddress = hostAddress;
     m_modbusTcpClient->setConnectionParameter(QModbusDevice::NetworkAddressParameter, m_hostAddress.toString());
-
-    // TODO: reconnect if different
 }
 
 uint SunSpecConnection::port() const
@@ -81,9 +81,6 @@ void SunSpecConnection::setPort(uint port)
 {
     m_port = port;
     m_modbusTcpClient->setConnectionParameter(QModbusDevice::NetworkPortParameter, m_port);
-
-    // TODO: reconnect if different
-
 }
 
 uint SunSpecConnection::slaveId() const
@@ -123,7 +120,7 @@ void SunSpecConnection::setNumberOfRetries(uint retries)
 
 bool SunSpecConnection::connected() const
 {
-    return m_modbusTcpClient->state() == QModbusDevice::ConnectedState;
+    return m_connected;
 }
 
 bool SunSpecConnection::discoveryRunning() const
@@ -143,6 +140,20 @@ void SunSpecConnection::disconnectDevice()
     m_modbusTcpClient->disconnectDevice();
 }
 
+bool SunSpecConnection::reconnectDevice()
+{
+    // Recreate the entire connection so we clean up also any pending replies
+    qCDebug(dcSunSpec()) << "Reconnecting" << this << "...";
+    if (m_modbusTcpClient) {
+        m_modbusTcpClient->disconnectDevice();
+        delete  m_modbusTcpClient;
+        m_modbusTcpClient = nullptr;
+    }
+
+    createConnection();
+    return connectDevice();
+}
+
 quint16 SunSpecConnection::baseRegister() const
 {
     return m_baseRegister;
@@ -153,6 +164,36 @@ QList<SunSpecModel *> SunSpecConnection::models() const
     return m_models;
 }
 
+QModbusReply *SunSpecConnection::sendReadRequest(const QModbusDataUnit &read, int serverAddress)
+{
+    if (!m_modbusTcpClient)
+        return nullptr;
+
+    QModbusReply *reply = m_modbusTcpClient->sendReadRequest(read, serverAddress);
+    monitorTimoutErrors(reply);
+    return reply;
+}
+
+QModbusReply *SunSpecConnection::sendWriteRequest(const QModbusDataUnit &write, int serverAddress)
+{
+    if (!m_modbusTcpClient)
+        return nullptr;
+
+    QModbusReply *reply = m_modbusTcpClient->sendWriteRequest(write, serverAddress);
+    monitorTimoutErrors(reply);
+    return reply;
+}
+
+QModbusReply *SunSpecConnection::sendRawRequest(const QModbusRequest &request, int serverAddress)
+{
+    if (!m_modbusTcpClient)
+        return nullptr;
+
+    QModbusReply *reply = m_modbusTcpClient->sendRawRequest(request, serverAddress);
+    monitorTimoutErrors(reply);
+    return reply;
+}
+
 bool SunSpecConnection::startDiscovery()
 {
     // Verify connection state
@@ -186,7 +227,6 @@ bool SunSpecConnection::startDiscovery()
 
 void SunSpecConnection::createConnection()
 {
-    qCDebug(dcSunSpec()) << "Creating connection for" << QString("%1:%2").arg(m_hostAddress.toString()).arg(m_port);
     m_modbusTcpClient = new QModbusTcpClient(this);
     m_modbusTcpClient->setConnectionParameter(QModbusDevice::NetworkPortParameter, m_port);
     m_modbusTcpClient->setConnectionParameter(QModbusDevice::NetworkAddressParameter, m_hostAddress.toString());
@@ -207,19 +247,20 @@ void SunSpecConnection::createConnection()
     });
 
     connect(m_modbusTcpClient, &QModbusTcpClient::stateChanged, this, [this](QModbusDevice::State state){
-        bool connected = (state == QModbusDevice::ConnectedState);
+        qCDebug(dcSunSpec()) << this << "client state changed" << state;
-        bool disconnected = (state == QModbusDevice::UnconnectedState);
+        if (!m_connected && state == QModbusDevice::ConnectedState) {
-
-        if (connected) {
             m_reconnectTimer.stop();
-            emit connectedChanged(true);
+            m_timoutReplyCounter = 0;
+            m_connected = true;
+            emit connectedChanged(m_connected);
             return;
         }
 
-        if (disconnected) {
+        if (m_connected && state == QModbusDevice::UnconnectedState) {
             // Try to reconnect in 10 seconds
             m_reconnectTimer.start();
-            emit connectedChanged(false);
+            m_connected = false;
+            emit connectedChanged(m_connected);
             return;
         }
     });
@@ -436,9 +477,9 @@ void SunSpecConnection::scanModelsOnBaseRegister(quint16 offset)
             // Scan next model block, current offset + 2 header bytes + model length
             scanModelsOnBaseRegister(offset + 2 + modelLength);
         } else {
-            qCWarning(dcSunSpec()) << "Error occured while reading model header from" << this << "using offset" << offset << m_modbusTcpClient->errorString();
+            qCWarning(dcSunSpec()) << "Error occurred while reading model header from" << this << "using offset" << offset << m_modbusTcpClient->errorString();
             if (!m_modelDiscoveryResult.isEmpty()) {
-                qCWarning(dcSunSpec()) << "Error occured but already discovered" << m_modelDiscoveryResult.count() << "models. Continue with the discovered models, but the discovery may be incomplete due to header reading errors.";
+                qCWarning(dcSunSpec()) << "Error occurred but already discovered" << m_modelDiscoveryResult.count() << "models. Continue with the discovered models, but the discovery may be incomplete due to header reading errors.";
                 qCDebug(dcSunSpec()) << "Scan for SunSpec models on" << this << m_baseRegister << "finished successfully";
                 processDiscoveryResult();
             } else {
@@ -449,6 +490,36 @@ void SunSpecConnection::scanModelsOnBaseRegister(quint16 offset)
     });
 }
 
+void SunSpecConnection::monitorTimoutErrors(QModbusReply *reply)
+{
+    // Some modbus device over time seem to stop responding randomly but keep the connection up.
+    // All replies finish repeatedly with the timeout error. Normally a reconnect will fix this behaviour.
+    // In order to minimize the downtime and having proper logs when this happens, we monitor every
+    // reply sent to the sunspec connection from the models and trigger a reconnect if to many timouts occurred in a row.
+
+    if (!reply) return;
+
+    connect(reply, &QModbusReply::errorOccurred, this, [this](QModbusDevice::Error error){
+        // Note: we handle
+        switch (error) {
+        case QModbusDevice::NoError:
+            // If any reply finished succssfully, we can reset the counter since the device
+            // is responding somehow and the workaround is not required
+            m_timoutReplyCounter = 0;
+            break;
+        case QModbusDevice::TimeoutError:
+            m_timoutReplyCounter++;
+            if (m_timoutReplyCounter > m_timoutReplyCounterLimit) {
+                qCWarning(dcSunSpec()) << "More than" << m_timoutReplyCounterLimit << "modbus replies finished with timeout error on" << this << "Triggering a reconnect...";
+                reconnectDevice();
+            }
+            break;
+        default:
+            break;
+        }
+    });
+}
+
 QDebug operator<<(QDebug debug, SunSpecConnection *connection)
 {
     debug.nospace().noquote() << "SunSpecConnection(" << connection->hostAddress().toString() << ":" << connection->port() << ", Slave ID: " << connection->slaveId() << ")";

libnymea-sunspec/sunspecconnection.h

@@ -73,15 +73,20 @@ public:
     bool connected() const;
     bool discoveryRunning() const;
 
-    bool connectDevice();
-    void disconnectDevice();
-
     quint16 baseRegister() const;
 
     QList<SunSpecModel *> models() const;
 
+    // Helper methods for internal queue handling if enabled
+    QModbusReply *sendReadRequest(const QModbusDataUnit &read, int serverAddress);
+    QModbusReply *sendWriteRequest(const QModbusDataUnit &write, int serverAddress);
+    QModbusReply *sendRawRequest(const QModbusRequest &request, int serverAddress);
+
 public slots:
     bool startDiscovery();
+    bool connectDevice();
+    void disconnectDevice();
+    bool reconnectDevice();
 
 signals:
     void connectedChanged(bool connected);
@@ -95,6 +100,7 @@ private:
     uint m_port;
     int m_slaveId = 1;
     QTimer m_reconnectTimer;
+    bool m_connected = false;
 
     quint16 m_baseRegister = 40000;
     QQueue<quint16> m_baseRegisterQueue;
@@ -112,6 +118,9 @@ private:
     QList<SunSpecModel *> m_uninitializedModels;
     SunSpecDataPoint::ByteOrder m_byteOrder = SunSpecDataPoint::ByteOrderLittleEndian;
 
+    int m_timoutReplyCounter = 0;
+    int m_timoutReplyCounterLimit = 16;
+
     void createConnection();
 
     void processDiscoveryResult();
@@ -124,6 +133,7 @@ private:
 
     void scanModelsOnBaseRegister(quint16 offset = 2);
 
+    void monitorTimoutErrors(QModbusReply *reply);
 };
 
 QDebug operator<<(QDebug debug, SunSpecConnection *connection);

libnymea-sunspec/sunspecmodel.cpp

@@ -106,55 +106,55 @@ void SunSpecModel::readBlockData()
 {
     // Read the block data, start register + 2 header reisters (id, length)
     QModbusDataUnit request = QModbusDataUnit(QModbusDataUnit::RegisterType::HoldingRegisters, m_modbusStartRegister, m_modelLength + 2);
-    if (QModbusReply *reply = m_connection->modbusTcpClient()->sendReadRequest(request, m_connection->slaveId())) {
+    QModbusReply *reply = m_connection->sendReadRequest(request, m_connection->slaveId());
-        if (!reply->isFinished()) {
+    if (!reply) {
-            connect(reply, &QModbusReply::finished, reply, &QModbusReply::deleteLater);
+        qCDebug(dcSunSpecModelData()) << "Read block data error: " << m_connection->modbusTcpClient()->errorString();
-            connect(reply, &QModbusReply::finished, this, [=]() {
-                if (reply->error() != QModbusDevice::NoError) {
-                    qCWarning(dcSunSpec()) << name() << description() << "Read block data response error:" << reply->error();
-                    return;
-                }
-
-                const QModbusDataUnit unit = reply->result();
-                qCDebug(dcSunSpecModelData()) << "-->" << "Received block data" << this << unit.values().count() << SunSpecDataPoint::registersToString(unit.values());
-                m_blockData = unit.values();
-                emit blockDataChanged(m_blockData);
-
-                if (m_blockData.count() != m_modelLength + 2) {
-                    qCWarning(dcSunSpecModelData()) << "Received invalid block data count from read block data request. Model lenght:" << m_modelLength << "Response block count:" << m_blockData.count();
-                    return;
-                }
-
-                // Fill the data points
-                foreach (const QString &dataPointName, m_dataPoints.keys()) {
-                    QVector<quint16> rawData = m_blockData.mid(m_dataPoints[dataPointName].addressOffset(), m_dataPoints[dataPointName].size());
-                    m_dataPoints[dataPointName].setRawData(rawData);
-                    qCDebug(dcSunSpecModelData()) << "Set raw data:" << m_dataPoints[dataPointName] << SunSpecDataPoint::registersToString(rawData) << (m_dataPoints[dataPointName].isValid() ? "Valid" : "Invalid");
-                }
-
-                // Fill the private member data using the data points
-                processBlockData();
-
-                // Make sure initialized gets called
-                setInitializedFinished();
-
-                // Inform about the new block data
-                emit blockUpdated();
-            });
-
-            connect(reply, &QModbusReply::errorOccurred, this, [this, reply] (QModbusDevice::Error error) {
-                qCWarning(dcSunSpecModelData())  << name() << description() << "Modbus reply while reading block data. Error:" << error;
-            });
-
-        } else {
-            qCWarning(dcSunSpecModelData()) << "Read block data error: " << m_connection->modbusTcpClient()->errorString();
-            reply->deleteLater(); // broadcast replies return immediately
-            return;
-        }
-    } else {
-        qCWarning(dcSunSpecModelData()) << "Read block data error: " << m_connection->modbusTcpClient()->errorString();
         return;
     }
+
+    if (reply->isFinished()) {
+        qCWarning(dcSunSpecModelData()) << "Read block data error: " << m_connection->modbusTcpClient()->errorString();
+        reply->deleteLater(); // broadcast replies return immediately
+        return;
+    }
+
+    connect(reply, &QModbusReply::finished, reply, &QModbusReply::deleteLater);
+    connect(reply, &QModbusReply::finished, this, [=]() {
+        if (reply->error() != QModbusDevice::NoError) {
+            qCWarning(dcSunSpec()) << name() << description() << "Read block data response error:" << reply->error();
+            return;
+        }
+
+        const QModbusDataUnit unit = reply->result();
+        qCDebug(dcSunSpecModelData()) << "-->" << "Received block data" << this << unit.values().count() << SunSpecDataPoint::registersToString(unit.values());
+        m_blockData = unit.values();
+        emit blockDataChanged(m_blockData);
+
+        if (m_blockData.count() != m_modelLength + 2) {
+            qCWarning(dcSunSpecModelData()) << "Received invalid block data count from read block data request. Model lenght:" << m_modelLength << "Response block count:" << m_blockData.count();
+            return;
+        }
+
+        // Fill the data points
+        foreach (const QString &dataPointName, m_dataPoints.keys()) {
+            QVector<quint16> rawData = m_blockData.mid(m_dataPoints[dataPointName].addressOffset(), m_dataPoints[dataPointName].size());
+            m_dataPoints[dataPointName].setRawData(rawData);
+            qCDebug(dcSunSpecModelData()) << "Set raw data:" << m_dataPoints[dataPointName] << SunSpecDataPoint::registersToString(rawData) << (m_dataPoints[dataPointName].isValid() ? "Valid" : "Invalid");
+        }
+
+        // Fill the private member data using the data points
+        processBlockData();
+
+        // Make sure initialized gets called
+        setInitializedFinished();
+
+        // Inform about the new block data
+        emit blockUpdated();
+    });
+
+    connect(reply, &QModbusReply::errorOccurred, this, [this, reply] (QModbusDevice::Error error) {
+        qCWarning(dcSunSpecModelData())  << name() << description() << "Modbus reply while reading block data. Error:" << error << reply->errorString();
+    });
 }
 
 bool SunSpecModel::operator ==(const SunSpecModel &other) const

sunspec/integrationpluginsunspec.cpp

@@ -81,20 +81,6 @@ void IntegrationPluginSunSpec::init()
     m_connectionSlaveIdParamTypeIds.insert(sunspecConnectionThingClassId, sunspecConnectionThingSlaveIdParamTypeId);
     m_connectionSlaveIdParamTypeIds.insert(solarEdgeConnectionThingClassId, solarEdgeConnectionThingSlaveIdParamTypeId);
 
-    // Connected state for all things
-    m_connectedStateTypeIds.insert(sunspecConnectionThingClassId, sunspecConnectionConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(solarEdgeConnectionThingClassId, solarEdgeConnectionConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(solarEdgeBatteryThingClassId, solarEdgeBatteryConnectedStateTypeId);
-
-    // Child things
-    m_connectedStateTypeIds.insert(sunspecStorageThingClassId, sunspecStorageConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(sunspecSinglePhaseInverterThingClassId, sunspecSinglePhaseInverterConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(sunspecSplitPhaseInverterThingClassId, sunspecSplitPhaseInverterConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(sunspecThreePhaseInverterThingClassId, sunspecThreePhaseInverterConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(sunspecSinglePhaseMeterThingClassId, sunspecSinglePhaseMeterConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(sunspecSplitPhaseMeterThingClassId, sunspecSplitPhaseMeterConnectedStateTypeId);
-    m_connectedStateTypeIds.insert(sunspecThreePhaseMeterThingClassId, sunspecThreePhaseMeterConnectedStateTypeId);
-
     // Params for sunspec things
     m_modelIdParamTypeIds.insert(sunspecSinglePhaseInverterThingClassId, sunspecSinglePhaseInverterThingModelIdParamTypeId);
     m_modelIdParamTypeIds.insert(sunspecSplitPhaseInverterThingClassId, sunspecSplitPhaseInverterThingModelIdParamTypeId);
@@ -528,12 +514,17 @@ void IntegrationPluginSunSpec::setupConnection(ThingSetupInfo *info)
 
     // Update all child things connected states for this connection
     connect(connection, &SunSpecConnection::connectedChanged, thing, [this, connection, thing] (bool connected) {
-        qCDebug(dcSunSpec()) << connection << (connected ? "connected" : "disconnected");
+        if (connected) {
-        thing->setStateValue(m_connectedStateTypeIds.value(thing->thingClassId()), connected);
+            qCDebug(dcSunSpec()) << connection << "connected";
+        } else {
+            qCWarning(dcSunSpec()) << connection << "disconnected";
+        }
+
+        thing->setStateValue("connected", connected);
 
         // Update connected state of child things
         foreach (Thing *child, myThings().filterByParentId(thing->id())) {
-            child->setStateValue(m_connectedStateTypeIds.value(child->thingClassId()), connected);
+            child->setStateValue("connected", connected);
 
             // Refresh childs if connected successfully
             if (connected && m_sunSpecThings.contains(child)) {
@@ -702,7 +693,7 @@ void IntegrationPluginSunSpec::searchSolarEdgeBattery(SunSpecConnection *connect
 
         // If init failed, no battery connected
         if (!success) {
-            qCWarning(dcSunSpec()) << "No SolarEdge battery connected on register" << startRegister << ". Not creating battery device.";
+            qCDebug(dcSunSpec()) << "No SolarEdge battery connected on register" << startRegister << "- not creating thing.";
             return;
         }
 

sunspec/integrationpluginsunspec.h

@@ -59,9 +59,6 @@ public:
     void executeAction(ThingActionInfo *info) override;
 
 private:
-    // Connected state for all things
-    QHash<ThingClassId, StateTypeId> m_connectedStateTypeIds;
-
     // SunSpec Connection params map
     QHash<ThingClassId, ParamTypeId> m_connectionIpParamTypeIds;
     QHash<ThingClassId, ParamTypeId> m_connectionPortParamTypeIds;

sunspec/integrationpluginsunspec.json

@@ -17,7 +17,7 @@
             "displayName": "Timout",
             "type": "uint",
             "unit": "MilliSeconds",
-            "defaultValue": 500
+            "defaultValue": 1000
         },
         {
             "id": "9a4bfe01-315f-4ee7-98a9-f16b08ba12ad",