ETM-Schurig/etm-powersync-plugins-modbus
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

sunspec: pin the single connection vendors against regressions

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Patrick Schurig 2026-08-02 09:10:48 +02:00
parent 9e0cd44af0
commit 6eb25045e2
3 changed files with 37 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
specs/fronius-sunspec.md
View File

@ -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=<sysroot> sunspec/tests/tests.pro && make check`.
Sans `NYMEA_SUNSPEC_PATH`, `sunspec.pri` résout `libnymea-sunspec` par

7
sunspec/integrationpluginsunspec.cpp
View File

@ -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;

23
sunspec/tests/multiunit/tst_sunspecmultiunit.cpp
View File

@ -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.