From 0954f66725337b5026cf518b3a0f925070c9c20a Mon Sep 17 00:00:00 2001 From: Patrick Schurig Date: Sun, 2 Aug 2026 08:53:28 +0200 Subject: [PATCH] sunspec: let createConnection() serve a secondary connection Takes the slave ID as an argument instead of reading it from the thing, and takes whether it is the primary connection of that thing. Only the primary one follows the network monitor, writes the connected state of the thing and lands in m_sunSpecConnections; a secondary one lands in a hash of its own. Child things are updated per connection rather than per parent, resolved from the model each child is bound to. With a single connection that is every child of the parent, so nothing changes here yet. No caller creates a secondary connection at this point: behaviour is unchanged for all three vendors. Co-Authored-By: Claude Opus 5 (1M context) --- sunspec/integrationpluginsunspec.cpp | 128 +++++++++++++++++++++------ sunspec/integrationpluginsunspec.h | 38 +++++++- 2 files changed, 140 insertions(+), 26 deletions(-) diff --git a/sunspec/integrationpluginsunspec.cpp b/sunspec/integrationpluginsunspec.cpp index 6197a33..09771bd 100644 --- a/sunspec/integrationpluginsunspec.cpp +++ b/sunspec/integrationpluginsunspec.cpp @@ -286,7 +286,8 @@ void IntegrationPluginSunSpec::setupThing(ThingSetupInfo *info) } }); - SunSpecConnection *connection = createConnection(info->thing()); + uint slaveId = thing->paramValue(m_connectionSlaveIdParamTypeIds.value(thing->thingClassId())).toUInt(); + SunSpecConnection *connection = createConnection(info->thing(), slaveId, true); connect(info, &ThingSetupInfo::aborted, connection, &SunSpecConnection::deleteLater); // If this is the first setup, we must connect successfully fo finishing the setup, otherwise we let the monitor do his work @@ -851,15 +852,22 @@ void IntegrationPluginSunSpec::processDiscoveryResult(Thing *thing, SunSpecConne } -SunSpecConnection *IntegrationPluginSunSpec::createConnection(Thing *thing) +/* Creates one SunSpec connection for a thing. + * + * A thing has exactly one primary connection, and a Fronius connection may have + * a second one towards its Smart Meter on another unit ID. Everything the two + * share - byte order, timeout, retries, rediscovery on reconnect - lives here; + * everything that belongs to the thing as a whole rather than to one of its + * connections is guarded by isPrimary. */ +SunSpecConnection *IntegrationPluginSunSpec::createConnection(Thing *thing, uint slaveId, bool isPrimary) { QHostAddress address = m_monitors.value(thing)->networkDeviceInfo().address(); int port = thing->paramValue(m_connectionPortParamTypeIds.value(thing->thingClassId())).toInt(); - int slaveId = thing->paramValue(m_connectionSlaveIdParamTypeIds.value(thing->thingClassId())).toInt(); - if (m_sunSpecConnections.contains(thing->id())) { - qCDebug(dcSunSpec()) << "Reconfigure SunSpec connection" << thing; - m_sunSpecConnections.take(thing->id())->deleteLater(); + QHash &connections = isPrimary ? m_sunSpecConnections : m_meterConnections; + if (connections.contains(thing->id())) { + qCDebug(dcSunSpec()) << "Reconfigure SunSpec connection" << thing << "on slave ID" << slaveId; + connections.take(thing->id())->deleteLater(); } SunSpecConnection *connection = nullptr; @@ -887,31 +895,41 @@ SunSpecConnection *IntegrationPluginSunSpec::createConnection(Thing *thing) connection->setNumberOfRetries(configValue(sunSpecPluginNumberOfRetriesParamTypeId).toUInt()); } - // Reconnect on monitor reachable changed - NetworkDeviceMonitor *monitor = m_monitors.value(thing); - connect(monitor, &NetworkDeviceMonitor::reachableChanged, thing, [=](bool reachable){ - qCDebug(dcSunSpec()) << "Network device monitor reachable changed for" << thing->name() << reachable; - if (!thing->setupComplete()) - return; + /* Reconnect on monitor reachable changed. + * + * Only the primary connection follows the monitor. The meter one is driven + * by the primary's discovery instead, so that the two never handshake at the + * same time towards the same inverter. */ + if (isPrimary) { + NetworkDeviceMonitor *monitor = m_monitors.value(thing); + connect(monitor, &NetworkDeviceMonitor::reachableChanged, thing, [=](bool reachable){ + qCDebug(dcSunSpec()) << "Network device monitor reachable changed for" << thing->name() << reachable; + if (!thing->setupComplete()) + return; - if (reachable && !thing->stateValue("connected").toBool()) { - qCDebug(dcSunSpec()) << "The monitor is reachable. Set the host address to" << monitor->networkDeviceInfo().address() << "and start connecting..."; - connection->setHostAddress(monitor->networkDeviceInfo().address()); - connection->connectDevice(); - } - }); + if (reachable && !thing->stateValue("connected").toBool()) { + qCDebug(dcSunSpec()) << "The monitor is reachable. Set the host address to" << monitor->networkDeviceInfo().address() << "and start connecting..."; + connection->setHostAddress(monitor->networkDeviceInfo().address()); + connection->connectDevice(); + } + }); + } - m_sunSpecConnections.insert(thing->id(), connection); + connections.insert(thing->id(), connection); // Update all child things connected states for this connection - connect(connection, &SunSpecConnection::connectedChanged, thing, [this, connection, thing] (bool connected) { + connect(connection, &SunSpecConnection::connectedChanged, thing, [this, connection, thing, isPrimary] (bool connected) { if (connected) { qCDebug(dcSunSpec()) << connection << "connected"; } else { qCWarning(dcSunSpec()) << connection << "disconnected"; } - thing->setStateValue("connected", connected); + /* The connected state of the thing follows the primary connection alone. + * A Smart Meter that drops must not mark the inverter unreachable, nor + * blank the data it is still delivering. */ + if (isPrimary) + thing->setStateValue("connected", connected); if (connected) { if (thing->setupComplete()) { @@ -919,8 +937,8 @@ SunSpecConnection *IntegrationPluginSunSpec::createConnection(Thing *thing) connection->startDiscovery(); } } - // Update connected state of child things - foreach (Thing *child, myThings().filterByParentId(thing->id())) { + // Update connected state of the child things of this connection + foreach (Thing *child, childThingsOn(thing, connection)) { if (connected) { child->setStateValue("connected", true); } else { @@ -932,15 +950,75 @@ SunSpecConnection *IntegrationPluginSunSpec::createConnection(Thing *thing) connect(connection, &SunSpecConnection::discoveryFinished, thing, [this, connection, thing] (bool success) { if (success) { qCDebug(dcSunSpec()) << "Discovery finished successfully of" << connection; + m_discoveredConnections.insert(connection); processDiscoveryResult(thing, connection); } else { qCWarning(dcSunSpec()) << "Discovery finished with errors on" << connection; } }); + /* m_discoveredConnections is keyed on the pointer, so it must not outlive + * it. The entry is only ever compared, never dereferenced. */ + connect(connection, &QObject::destroyed, this, [this](QObject *object) { + m_discoveredConnections.remove(static_cast(object)); + }); + return connection; } +QList IntegrationPluginSunSpec::connectionsFor(const ThingId &parentId) const +{ + QList connections; + + SunSpecConnection *primary = m_sunSpecConnections.value(parentId); + if (primary) + connections.append(primary); + + SunSpecConnection *meter = m_meterConnections.value(parentId); + if (meter) + connections.append(meter); + + return connections; +} + +QList IntegrationPluginSunSpec::allConnections() const +{ + QList connections = m_sunSpecConnections.values(); + connections.append(m_meterConnections.values()); + return connections; +} + +SunSpecConnection *IntegrationPluginSunSpec::connectionForChild(Thing *child) const +{ + if (m_sunSpecInverters.contains(child)) + return m_sunSpecInverters.value(child)->connection(); + + if (m_sunSpecMeters.contains(child)) + return m_sunSpecMeters.value(child)->connection(); + + if (m_sunSpecStorages.contains(child)) + return m_sunSpecStorages.value(child)->connection(); + + if (m_sunSpecThings.contains(child)) + return m_sunSpecThings.value(child)->connection(); + + return nullptr; +} + +QList IntegrationPluginSunSpec::childThingsOn(Thing *parent, SunSpecConnection *connection) const +{ + const bool isPrimary = (m_sunSpecConnections.value(parent->id()) == connection); + + QList children; + foreach (Thing *child, myThings().filterByParentId(parent->id())) { + SunSpecConnection *childConnection = connectionForChild(child); + if (childConnection == connection || (!childConnection && isPrimary)) + children.append(child); + } + + return children; +} + void IntegrationPluginSunSpec::setupSolarEdgeBattery(ThingSetupInfo *info) { Thing *thing = info->thing(); @@ -1414,12 +1492,12 @@ void IntegrationPluginSunSpec::onPluginConfigurationChanged(const ParamTypeId &p } } else if (paramTypeId == sunSpecPluginNumberOfRetriesParamTypeId) { qCDebug(dcSunSpec()) << "Updating number of retries" << value.toUInt(); - foreach (SunSpecConnection *connection, m_sunSpecConnections) { + foreach (SunSpecConnection *connection, allConnections()) { connection->setNumberOfRetries(value.toUInt()); } } else if (paramTypeId == sunSpecPluginTimeoutParamTypeId) { qCDebug(dcSunSpec()) << "Updating timeout" << value.toUInt() << "[ms]"; - foreach (SunSpecConnection *connection, m_sunSpecConnections) { + foreach (SunSpecConnection *connection, allConnections()) { connection->setTimeout(value.toUInt()); } } else { diff --git a/sunspec/integrationpluginsunspec.h b/sunspec/integrationpluginsunspec.h index 85bcd05..1a75316 100644 --- a/sunspec/integrationpluginsunspec.h +++ b/sunspec/integrationpluginsunspec.h @@ -76,20 +76,56 @@ private: QHash m_monitors; + /* The primary connection of a thing: the one that carries its identity, its + * connected state and its timeout and retry settings. Every vendor has + * exactly one. */ QHash m_sunSpecConnections; + + /* Second, optional connection towards the Smart Meter of a Fronius GEN24, + * on the same host and port as the primary one but on another unit ID. + * Indexed on the same parent ThingId. + * + * Kept apart from m_sunSpecConnections rather than turning that one into a + * hash of lists: this way not a single call site of the generic and + * solarEdge vendors changes, including the reverse lookup + * m_sunSpecConnections.key() the SolarEdge battery search relies on. */ + QHash m_meterConnections; + QHash m_sunSpecThings; QHash m_sunSpecInverters; QHash m_sunSpecMeters; QHash m_sunSpecStorages; + /* Connections having completed at least one successful discovery. The + * orphan sweep needs to know whether it is looking at the whole picture or + * at half of it. Entries are removed when the connection is destroyed. */ + QSet m_discoveredConnections; + Thing *getThingForSunSpecModel(uint modelId, uint modbusAddress, const ThingId &parentId); bool sunspecThingAlreadyAdded(uint modelId, uint modbusAddress, const ThingId &parentId); void processDiscoveryResult(Thing *thing, SunSpecConnection *connection); // SunSpec things - SunSpecConnection *createConnection(Thing *thing); + SunSpecConnection *createConnection(Thing *thing, uint slaveId, bool isPrimary); + + /* Every connection of a parent thing, primary first. One entry for the + * generic and solarEdge vendors, one or two for Fronius. */ + QList connectionsFor(const ThingId &parentId) const; + QList allConnections() const; + + /* The connection a child thing reads its data from, or null while it is not + * bound to a model yet. Resolved from the model the child is bound to + * rather than from a persisted unit ID: a child stores only its model ID + * and start register, and the two connections of a Fronius thing never + * announce the same model. */ + SunSpecConnection *connectionForChild(Thing *child) const; + + /* Children of a parent that read from one given connection. A child not + * bound to any model yet follows the primary connection, which is what + * makes the parent reachable at all. */ + QList childThingsOn(Thing *parent, SunSpecConnection *connection) const; // Custom types void setupSolarEdgeBattery(ThingSetupInfo *info);