From 6eb25045e282ecd7c385b9c9ba864df09e4657e7 Mon Sep 17 00:00:00 2001 From: Patrick Schurig Date: Sun, 2 Aug 2026 09:10:48 +0200 Subject: [PATCH] sunspec: pin the single connection vendors against regressions The whole point of keeping the meter connection in a hash of its own was that not a line of the generic and solarEdge vendors would change. Checks it rather than claiming it: a stored solarEdge connection, whose behaviour no suite covered until now, must still end up with exactly one connection, as must a generic one. Also records the connection a discovery came from in processDiscoveryResult() rather than in the discoveryFinished handler. The initial setup calls it from a second handler on the same signal, so recording it there made the orphan sweep depend on the order Qt happens to invoke the two. Co-Authored-By: Claude Opus 5 (1M context) --- specs/fronius-sunspec.md | 11 ++++++--- sunspec/integrationpluginsunspec.cpp | 7 +++++- .../tests/multiunit/tst_sunspecmultiunit.cpp | 23 +++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/specs/fronius-sunspec.md b/specs/fronius-sunspec.md index 63612c9..584ecad 100644 --- a/specs/fronius-sunspec.md +++ b/specs/fronius-sunspec.md @@ -317,10 +317,10 @@ confirmation humaine au moment de l'implémentation. | # | Point | Statut au contrat | Hypothèse retenue | |---|---|---|---| -| H-1 | Architecture multi-unit-ID (D1) | ⬜ en attente de validation | **Option A′** : un thing `froniusConnection` portant deux `SunSpecConnection` internes (onduleur `slaveId`, compteur `meterSlaveId`). C'est l'option recommandée par le contrat : UX correcte, aucune modification de `libnymea-sunspec`, rebasabilité préservée. | +| H-1 | Architecture multi-unit-ID (D1) | ✅ tranché, **implémenté** | **Option A′** : un thing `froniusConnection` portant deux `SunSpecConnection` internes (onduleur `slaveId`, compteur `meterSlaveId`). UX correcte, aucune modification de `libnymea-sunspec`, rebasabilité préservée. La seconde connexion vit dans une table distincte de `m_sunSpecConnections`, ce qui laisse les vendors générique et `solarEdge` inchangés ligne pour ligne. | | H-2 | Watchdog batterie (D4) | ⬜ à valider | **`RvrtTms = 300 s`** à chaque consigne (AC-10). Prérequis non levé : sémantique du timer à vérifier sur le modèle 124, pas seulement sur le 123. | | H-3 | Ordre des octets du GEN24 | inférence explicite du brief | Défaut SunSpec, little-endian (AC-6). Non documenté par le constructeur. | -| H-4 | Deux connexions TCP simultanées vers le même onduleur | inférence explicite du brief | Non supposé résolu. Le constructeur recommande des requêtes séquentielles ; deux boucles d'interrogation parallèles violeraient potentiellement cette recommandation. **À mesurer au banc**, pas à supposer. En attendant, sérialiser. | +| H-4 | Deux connexions TCP simultanées vers le même onduleur | ✅ **mesuré** sur le GEN24 client | Les deux connexions tiennent simultanément. Reste distincte, et **non mesurée**, la charge de deux *découvertes* simultanées : le plugin ne monte donc la connexion compteur qu'une fois la découverte primaire terminée. La sérialisation des lectures d'interrogation reste un chantier à part. | | H-5 | Assistant de tarification écrasant les consignes | « à confirmer », absent de la doc constructeur | Constat empirique tiers. Traité en message de diagnostic (AC-13.2), pas en logique de pilotage. | | H-6 | Ordre des phases du chantier | contrat §8 : la phase 2 ne démarre pas avant que la phase 1 (simulateur) soit verte | **Précondition non vérifiée ici.** Le simulateur `hems-sim` n'a pas été exécuté. Les AC ci-dessous sont validés par tests unitaires hors ligne, ce qui ne remplace pas une validation contre un simulateur reproduisant la sémantique du constructeur. | @@ -359,7 +359,12 @@ confirmation humaine au moment de l'implémentation. ## Tableau de statut -Suite : `sunspec/tests/`, **38 tests, 38 verts**. +Suite : `sunspec/tests/`, **60 tests, 60 verts** — 44 en logique pure, 6 sur +l'aiguillage du plugin, 10 sur le multi unit ID. + +Une quatrième suite, `tests/integration`, monte le plugin face à un vrai +appareil. Elle est construite mais **jamais lancée par `make check`** : elle +exige `SUNSPEC_TEST_HOST`. **Elle n'a pas encore été exécutée sur matériel.** Build : `qmake6 NYMEA_SUNSPEC_PATH= sunspec/tests/tests.pro && make check`. Sans `NYMEA_SUNSPEC_PATH`, `sunspec.pri` résout `libnymea-sunspec` par diff --git a/sunspec/integrationpluginsunspec.cpp b/sunspec/integrationpluginsunspec.cpp index e033fc6..dc27726 100644 --- a/sunspec/integrationpluginsunspec.cpp +++ b/sunspec/integrationpluginsunspec.cpp @@ -761,6 +761,12 @@ void IntegrationPluginSunSpec::processDiscoveryResult(Thing *thing, SunSpecConne { qCInfo(dcSunSpec()) << "Processing discovery result from" << thing->name() << connection; + /* Recorded here rather than in the discoveryFinished handler: the initial + * setup calls this function from a second handler on the same signal, so + * doing it there would make the orphan sweep depend on the order Qt happens + * to invoke the two. */ + m_discoveredConnections.insert(connection); + foreach (SunSpecModel *model, connection->models()) { Thing *modelThing = getThingForSunSpecModel(model->modelId(), model->modbusStartRegister(), thing->id()); if (modelThing) { @@ -1023,7 +1029,6 @@ SunSpecConnection *IntegrationPluginSunSpec::createConnection(Thing *thing, uint connect(connection, &SunSpecConnection::discoveryFinished, thing, [this, connection, thing, isPrimary] (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; diff --git a/sunspec/tests/multiunit/tst_sunspecmultiunit.cpp b/sunspec/tests/multiunit/tst_sunspecmultiunit.cpp index 0508d67..4e1a920 100644 --- a/sunspec/tests/multiunit/tst_sunspecmultiunit.cpp +++ b/sunspec/tests/multiunit/tst_sunspecmultiunit.cpp @@ -199,6 +199,7 @@ private slots: void test_F3_meter_slave_id_zero_creates_only_one_connection(); void test_F3_meter_slave_id_colliding_with_the_inverter_is_refused(); void test_F3_generic_connection_creates_no_meter_connection(); + void test_F3_solaredge_connection_creates_no_meter_connection(); void test_F3_removal_destroys_both_connections(); private: @@ -220,6 +221,7 @@ static const ThingId FroniusWithMeterId = ThingId("1f0a5c7e-3b42-4d18-9e6a-70 static const ThingId FroniusWithoutMeterId = ThingId("2c7d9e14-8a56-4f03-b21d-63e8a95c4072"); static const ThingId FroniusCollidingId = ThingId("3b8e2f60-95c1-42a7-8d34-51fa7e60b9c8"); static const ThingId GenericConnectionId = ThingId("4a1c6d83-27e9-4b50-a6f2-8c05d31e7ba4"); +static const ThingId SolarEdgeConnectionId = ThingId("5e2b7f49-6d38-41ca-b95e-0a4c8d71e263"); static const int MeterSlaveId = 200; static const int InverterSlaveId = 1; @@ -246,6 +248,10 @@ void SunSpecMultiUnitTest::storeConnection(const ThingId &thingId, const ThingCl settings.setValue(froniusConnectionThingPortParamTypeId.toString(), 502); settings.setValue(froniusConnectionThingSlaveIdParamTypeId.toString(), InverterSlaveId); settings.setValue(froniusConnectionThingMeterSlaveIdParamTypeId.toString(), meterSlaveId); + } else if (thingClassId == solarEdgeConnectionThingClassId) { + settings.setValue(solarEdgeConnectionThingAddressParamTypeId.toString(), "192.0.2.1"); + settings.setValue(solarEdgeConnectionThingPortParamTypeId.toString(), 502); + settings.setValue(solarEdgeConnectionThingSlaveIdParamTypeId.toString(), InverterSlaveId); } else { settings.setValue(sunspecConnectionThingAddressParamTypeId.toString(), "192.0.2.1"); settings.setValue(sunspecConnectionThingPortParamTypeId.toString(), 502); @@ -299,6 +305,8 @@ void SunSpecMultiUnitTest::initTestCase() metadata.pluginId(), InverterSlaveId); storeConnection(GenericConnectionId, sunspecConnectionThingClassId, "SunSpec generique", metadata.pluginId(), 0); + storeConnection(SolarEdgeConnectionId, solarEdgeConnectionThingClassId, "SolarEdge", + metadata.pluginId(), 0); m_hardwareManager = new FakeHardwareManager(this); m_thingManager = new ThingManagerImplementation(m_hardwareManager, nullptr, QLocale(), this); @@ -397,6 +405,21 @@ void SunSpecMultiUnitTest::test_F3_generic_connection_creates_no_meter_connectio "le vendor generique garde exactement une connexion par thing"); } +/* Non regression du vendor solarEdge, celui dont aucune suite ne couvrait le + * comportement. C'est la raison pour laquelle la connexion compteur vit dans + * une table separee plutot que dans un QHash de listes : ainsi pas une seule + * ligne de solarEdge n'a change, y compris la recherche inverse + * m_sunSpecConnections.key() dont depend la decouverte des batteries. */ +void SunSpecMultiUnitTest::test_F3_solaredge_connection_creates_no_meter_connection() +{ + Thing *thing = awaitThing(SolarEdgeConnectionId); + QVERIFY(thing); + + QVERIFY(m_plugin->testPrimaryConnection(SolarEdgeConnectionId)); + QVERIFY2(!m_plugin->testMeterConnection(SolarEdgeConnectionId), + "le vendor solarEdge garde exactement une connexion par thing"); +} + /* thingRemoved() est une chaine de else if dont la premiere branche portait sur * la seule connexion primaire. Sans reprise explicite, la connexion compteur * survit a son thing : socket ouverte et signaux pointant vers un Thing detruit.