From 2bb3ff570b51ddbf6a8787849aec2d4479c1ab02 Mon Sep 17 00:00:00 2001 From: Patrick Schurig Date: Wed, 26 Aug 2026 16:18:20 +0200 Subject: [PATCH] =?UTF-8?q?fix(sunspec):=20ne=20pas=20confondre=20=C2=AB?= =?UTF-8?q?=20pas=20encore=20lu=20=C2=BB=20et=20=C2=AB=20pas=20de=20batter?= =?UTF-8?q?ie=20=C2=BB?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constaté au banc à 14:41:58 : l'avertissement « has a model 160 but no StCha/StDisCha block » a été émis une fois, au démarrage, sur une machine dont les deux blocs existent et dont le currentPower a ensuite suivi la batterie correctement en charge, en décharge et au repos. Cause. libnymea-sunspec crée les blocs répétés d'un modèle depuis `initFinished`, émis pendant la PREMIÈRE lecture, juste avant `blockUpdated`. Sur ce premier signal les blocs existent mais n'ont jamais été remplis : tous les `IDStr` sont vides, rien ne se classe, et `hasBatteryBlocks()` répond faux. Cette réponse est exacte et hors sujet — la question n'était pas « pas de batterie » mais « pas encore lu ». Ce que le faux positif coûtait. Ce n'est pas du bruit : le README dit à l'exploitation de chercher cette ligne EN PREMIER pour distinguer une batterie cassée d'une batterie non mesurable. Une ligne qui sort à chaque démarrage sain ne distingue plus rien, et le diagnostic que ce lot venait d'installer était mort-né. Les deux questions sont maintenant posées séparément : `blocksAreIdentified()` avant `hasBatteryBlocks()`. Sans identification, on se tait et on attend le tour suivant — `readBlockData()` passe à chaque rafraîchissement de toute façon. Le garde en double au moment du binding disparaît aussi : les deux updaters se gardent eux-mêmes, un seul endroit à tenir à jour plutôt que deux qui peuvent diverger. Deux tests figent la distinction, dont un sur le cas où l'avertissement DOIT sortir — un GEN24 sans batterie, lu, identifié, sans bloc batterie. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y3gzsuwENJfJtvuNDT3xGn --- debian/changelog | 19 ++++++++++ sunspec/froniusrules.cpp | 9 +++++ sunspec/froniusrules.h | 15 ++++++++ sunspec/integrationpluginsunspec.cpp | 19 +++++++--- sunspec/tests/logic/tst_sunspec.cpp | 52 ++++++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 5 deletions(-) diff --git a/debian/changelog b/debian/changelog index beaea64..a6bfaab 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,22 @@ +etm-powersync-plugins-modbus (1.15.2+etm11) trixie; urgency=medium + + * sunspec: stop warning about a missing battery channel on a machine that has + simply not been read yet. libnymea-sunspec creates the repeating blocks of a + model from initFinished, which fires during the FIRST block read, just + before blockUpdated. On that first signal the blocks exist but carry no + data: every IDStr is empty, nothing classifies, and hasBatteryBlocks() + answered false about an inverter whose StCha/StDisCha blocks were there all + along. Measured on the bench at 14:41:58: the warning fired once at startup + on a storage whose currentPower then tracked the battery correctly through + charge, discharge and rest. + What that false positive cost is not noise. The README tells operations to + grep that exact line first to tell a broken storage from an unmeasurable + one; a line that appears at every healthy start tells them nothing. The two + questions are now asked separately - blocksAreIdentified() before + hasBatteryBlocks() - and the warning waits for the next refresh instead. + + -- Patrick Schurig Wed, 26 Aug 2026 16:10:00 +0200 + etm-powersync-plugins-modbus (1.15.2+etm10) trixie; urgency=medium * sunspec: make the SunSpec storage visible to the energy engine. The diff --git a/sunspec/froniusrules.cpp b/sunspec/froniusrules.cpp index 7085240..ffbae4d 100644 --- a/sunspec/froniusrules.cpp +++ b/sunspec/froniusrules.cpp @@ -211,6 +211,15 @@ double batteryPower(const QList &readings) return -storageCurrentPower(readings); } +bool blocksAreIdentified(const QList &readings) +{ + foreach (const ModuleReading &reading, readings) { + if (!reading.inputIdString.trimmed().isEmpty()) + return true; + } + return false; +} + bool hasBatteryBlocks(const QList &readings) { foreach (const ModuleReading &reading, readings) { diff --git a/sunspec/froniusrules.h b/sunspec/froniusrules.h index 2c6fcdc..5a3918b 100644 --- a/sunspec/froniusrules.h +++ b/sunspec/froniusrules.h @@ -242,6 +242,21 @@ double storageCurrentPower(const QList &readings); * two zeros mean opposite things to a reserve rule. */ bool hasBatteryBlocks(const QList &readings); +/* Whether the blocks have been read at all, i.e. whether any of them carries an + * ID string. This is NOT the same question as hasBatteryBlocks(), and confusing + * the two produces a false alarm on every healthy start. + * + * libnymea-sunspec creates the repeating blocks of a model from `initFinished`, + * which fires during the FIRST block read, just before `blockUpdated`. On that + * first signal the blocks therefore exist but their data points have never been + * filled: every inputIdSting() is empty, nothing classifies, and a caller that + * only asked hasBatteryBlocks() would conclude "this inverter has no battery + * channel" about a machine it has not read yet. + * + * Measured: the warning fired once at 14:41:58 on a bench whose StCha/StDisCha + * blocks were present and whose currentPower worked from the next cycle on. */ +bool blocksAreIdentified(const QList &readings); + } #endif // FRONIUSRULES_H diff --git a/sunspec/integrationpluginsunspec.cpp b/sunspec/integrationpluginsunspec.cpp index 87252a9..307e7de 100644 --- a/sunspec/integrationpluginsunspec.cpp +++ b/sunspec/integrationpluginsunspec.cpp @@ -2190,11 +2190,11 @@ void IntegrationPluginSunSpec::bindStorageCompanionModels(Thing *thing, SunSpecM return; } - /* Publish straight away when the companion models already carry data, so a - * thing bound after a discovery does not wait a whole refresh cycle. */ - if (!m_storageMpptModels.value(thing)->repeatingBlocks().isEmpty()) - updateStorageCurrentPower(thing); - + /* Publish straight away, so a thing bound after a discovery does not wait a + * whole refresh cycle. Both updaters return quietly when their model has + * nothing readable yet - a single guard, inside them, rather than a second + * one here that would have to be kept in step. */ + updateStorageCurrentPower(thing); updateStorageCapacity(thing); } @@ -2257,6 +2257,15 @@ void IntegrationPluginSunSpec::updateStorageCurrentPower(Thing *thing) { const QList readings = storageModuleReadings(thing); + /* Not read yet, not "no battery". On the first blockUpdated the repeating + * blocks have just been created and carry no data, so nothing classifies. + * Warning here would put a false alarm in the journal of every healthy + * machine at every start - on the very line operations is told to trust to + * tell a broken storage from an unmeasurable one. Silence, and wait for the + * next cycle: readBlockData() runs on every refresh anyway. */ + if (!FroniusRules::blocksAreIdentified(readings)) + return; + if (!FroniusRules::hasBatteryBlocks(readings)) { /* An inverter with no battery publishes N=2 and no StCha/StDisCha at * all. Summing an empty set would answer a confident 0 W. */ diff --git a/sunspec/tests/logic/tst_sunspec.cpp b/sunspec/tests/logic/tst_sunspec.cpp index 5f86f29..cb7ba6e 100644 --- a/sunspec/tests/logic/tst_sunspec.cpp +++ b/sunspec/tests/logic/tst_sunspec.cpp @@ -171,6 +171,8 @@ private slots: void test_D9_current_power_ignores_the_pv_strings(); void test_D9_battery_blocks_are_detected_by_id_string(); void test_D9_an_inverter_without_battery_has_no_battery_block(); + void test_D9_unread_blocks_are_not_mistaken_for_a_missing_battery(); + void test_D9_read_blocks_are_identified(); void test_D9_capacity_converts_watthours_to_kilowatthours(); void test_D9_a_nameplate_without_capacity_is_refused(); @@ -1059,6 +1061,56 @@ void SunSpecFroniusTest::test_D9_an_inverter_without_battery_has_no_battery_bloc QVERIFY(!hasBatteryBlocks({})); } +/* La distinction que ce test protege est celle qui a produit un faux positif au + * banc le 2026-08-26 a 14:41:58 : l'avertissement « pas de bloc StCha/StDisCha » + * a ete emis sur une machine dont les deux blocs existaient et dont le + * currentPower a fonctionne des le cycle suivant. + * + * Cause : libnymea-sunspec cree les blocs repetes depuis `initFinished`, emis + * pendant la PREMIERE lecture, juste avant `blockUpdated`. Sur ce premier + * signal les blocs existent mais n'ont jamais ete remplis - tous les IDStr sont + * vides. hasBatteryBlocks() repond faux, ce qui est exact et hors sujet : la + * question n'est pas « pas de batterie » mais « pas encore lu ». + * + * Ce que coutait le faux positif : la ligne de journal est celle que le README + * dit a l'exploitation de chercher en premier pour distinguer une batterie + * cassee d'une batterie non mesurable. Une ligne qui apparait a CHAQUE demarrage + * sain ne distingue plus rien. */ +void SunSpecFroniusTest::test_D9_unread_blocks_are_not_mistaken_for_a_missing_battery() +{ + using namespace FroniusRules; + + // Blocs crees mais jamais remplis : la puissance est a zero et l'IDStr vide. + const QList jamaisLus = { + {"", 0.0}, {"", 0.0}, {"", 0.0}, {"", 0.0} + }; + + QVERIFY(!blocksAreIdentified(jamaisLus)); + // hasBatteryBlocks() repond faux aussi - c'est bien pour ca qu'il ne suffit + // pas a lui seul a decider qu'il n'y a pas de batterie. + QVERIFY(!hasBatteryBlocks(jamaisLus)); + + QVERIFY(!blocksAreIdentified({})); + QVERIFY(!blocksAreIdentified({{" ", 0.0}})); +} + +void SunSpecFroniusTest::test_D9_read_blocks_are_identified() +{ + using namespace FroniusRules; + + // Un GEN24 sans batterie : lu, identifie, et sans bloc batterie. C'est LE + // cas ou l'avertissement doit bien sortir. + const QList sansBatterie = {{"MPPT 1", 1200.0}, {"MPPT 2", 800.0}}; + QVERIFY(blocksAreIdentified(sansBatterie)); + QVERIFY(!hasBatteryBlocks(sansBatterie)); + + const QList avecBatterie = { + {"MPPT 1", 1200.0}, {"StCha 3", 0.0}, {"StDisCha 4", 0.0} + }; + QVERIFY(blocksAreIdentified(avecBatterie)); + QVERIFY(hasBatteryBlocks(avecBatterie)); +} + void SunSpecFroniusTest::test_D9_capacity_converts_watthours_to_kilowatthours() { QCOMPARE(SunSpecStorageLogic::nameplateCapacityKwh(10000.0), 10.0);