fix: read-compare-write in automation to stop redundant inverter writes

Automations were writing the target value on every snapshot cycle even when
the inverter was already at that value, spamming the dongle and the
automation log (e.g. writing 0 A every 10 s when it had been 0 A for hours).

- _do_write now reads the register's current raw value first and skips the
  write (silently) when it already matches the target.
- Removed the in-memory _last_written_value dedup guard, which only tracked
  what this process wrote (reset on restart) and could not detect external
  changes. Read-compare-write is now the single source of truth.
- Fail-safe: if the read fails, the write still proceeds (a failed read
  should not block the automation from doing its job).
- Added 3 tests covering match-skip, differ-write, and read-fail-write.

Author: jmewing

collector/automation.py

@@ -631,7 +631,6 @@ def __init__(
         self._notifiers = notifiers
         self._last_eval: Dict[str, bool] = {}
         self._last_notify: Dict[str, float] = {}
-        self._last_written_value: Dict[str, Any] = {}
 
     def _db(self):
         import pymysql
@@ -797,26 +796,20 @@ def _evaluate_one(
             _, rng = match
             if rng.action_value is not None:
                 value = float(rng.action_value)
-                last = self._last_written_value.get(auto.id)
-                if last != value:
-                    self._do_write(
-                        auto.id, auto.name, auto.setting, value,
-                        dongle_host, dongle_port, datalog_serial, inverter_serial,
-                        dry_run,
-                    )
-                    self._last_written_value[auto.id] = value
-                self._last_eval[auto.id] = True
-                return
-        if was_active and auto.restore_value is not None and auto.setting is not None:
-            value = float(auto.restore_value)
-            last = self._last_written_value.get(auto.id)
-            if last != value:
                 self._do_write(
                     auto.id, auto.name, auto.setting, value,
                     dongle_host, dongle_port, datalog_serial, inverter_serial,
                     dry_run,
                 )
-                self._last_written_value[auto.id] = value
+                self._last_eval[auto.id] = True
+                return
+        if was_active and auto.restore_value is not None and auto.setting is not None:
+            value = float(auto.restore_value)
+            self._do_write(
+                auto.id, auto.name, auto.setting, value,
+                dongle_host, dongle_port, datalog_serial, inverter_serial,
+                dry_run,
+            )
         self._last_eval[auto.id] = False
 
     def _evaluate_battery_protection(
@@ -840,24 +833,18 @@ def _evaluate_battery_protection(
         if matched:
             if auto.action_value is not None:
                 value = float(auto.action_value)
-                last = self._last_written_value.get(auto.id)
-                if last != value:
-                    self._do_write(
-                        auto.id, auto.name, "shutdown_battery_voltage", value,
-                        dongle_host, dongle_port, datalog_serial, inverter_serial, dry_run,
-                    )
-                    self._last_written_value[auto.id] = value
+                self._do_write(
+                    auto.id, auto.name, "shutdown_battery_voltage", value,
+                    dongle_host, dongle_port, datalog_serial, inverter_serial, dry_run,
+                )
             self._last_eval[auto.id] = True
         else:
             if was_active and auto.restore_value is not None:
                 value = float(auto.restore_value)
-                last = self._last_written_value.get(auto.id)
-                if last != value:
-                    self._do_write(
-                        auto.id, auto.name, "shutdown_battery_voltage", value,
-                        dongle_host, dongle_port, datalog_serial, inverter_serial, dry_run,
-                    )
-                    self._last_written_value[auto.id] = value
+                self._do_write(
+                    auto.id, auto.name, "shutdown_battery_voltage", value,
+                    dongle_host, dongle_port, datalog_serial, inverter_serial, dry_run,
+                )
             self._last_eval[auto.id] = False
 
     def _evaluate_battery_soc(
@@ -944,6 +931,18 @@ def _do_write(
             logger.info("[DRY-RUN] Automation %s: %s=%s", automation_id, setting_name, clamped)
             return
 
+        # Read-compare-write: only send a Modbus write when the inverter's
+        # current value actually differs from the target. This avoids spamming
+        # the dongle (and the automation log) with redundant writes every
+        # snapshot when the value is already correct (e.g. writing 0 A every
+        # 10 s when it has been 0 A for hours). The steady-state "no change"
+        # case is silent — only actual writes (or failures) are logged.
+        read_ok, current_raw, _ = _read_holding_register(
+            dongle_host, dongle_port, datalog_serial, inverter_serial, reg_addr
+        )
+        if read_ok and current_raw == raw:
+            return
+
         ok, msg = _write_holding_register(
             dongle_host, dongle_port, datalog_serial, inverter_serial, reg_addr, raw
         )

tests/test_automation.py

@@ -300,5 +300,56 @@ def test_single_group_single_range(self):
         self.assertTrue(self.engine._last_eval[auto.id])
 
 
+class TestReadCompareWrite(unittest.TestCase):
+    """Read-compare-write: only write when the inverter value actually differs."""
+
+    def setUp(self):
+        self.engine = AutomationEngine(
+            db_host="localhost", db_port=3306, db_user="x",
+            db_password="x", db_name="x", table_prefix="lux_",
+        )
+
+    def _do_write(self, value, read_result, dry_run=False):
+        with patch("collector.automation._read_holding_register", return_value=read_result) as mock_read:
+            with patch("collector.automation._write_holding_register", return_value=(True, "ok")) as mock_write:
+                with patch("collector.automation.AutomationEngine._log"):
+                    self.engine._do_write(
+                        "a1", "Test", "grid_charge_current", value,
+                        "192.168.1.100", 8000, "BJ123", "INV123", dry_run,
+                    )
+                return mock_read, mock_write
+
+    def test_skips_write_when_value_matches(self):
+        # Inverter already at the target raw value -> no write.
+        # grid_charge_current maps to a register; use a value whose raw form
+        # we can predict. We patch the read to return the SAME raw the write
+        # would produce by reading the register map.
+        from collector.automation import SETTING_NAME_TO_REGISTER, HOLDING_BY_NAME, HOLDING_REGISTERS, _engineering_to_raw, _clamp_to_register
+        reg_name = SETTING_NAME_TO_REGISTER["grid_charge_current"]
+        reg_addr = HOLDING_BY_NAME[reg_name]
+        meta = HOLDING_REGISTERS[reg_addr]
+        target_raw = _engineering_to_raw(_clamp_to_register(10.0, meta), meta)
+
+        mock_read, mock_write = self._do_write(10.0, (True, target_raw, "read"))
+        mock_write.assert_not_called()
+
+    def test_writes_when_value_differs(self):
+        from collector.automation import SETTING_NAME_TO_REGISTER, HOLDING_BY_NAME, HOLDING_REGISTERS, _engineering_to_raw, _clamp_to_register
+        reg_name = SETTING_NAME_TO_REGISTER["grid_charge_current"]
+        reg_addr = HOLDING_BY_NAME[reg_name]
+        meta = HOLDING_REGISTERS[reg_addr]
+        target_raw = _engineering_to_raw(_clamp_to_register(10.0, meta), meta)
+        # Inverter currently at a DIFFERENT raw value.
+        different_raw = target_raw + 1
+
+        mock_read, mock_write = self._do_write(10.0, (True, different_raw, "read"))
+        mock_write.assert_called_once()
+
+    def test_writes_when_read_fails(self):
+        # If the read fails, fail-safe: still write (don't block the automation).
+        mock_read, mock_write = self._do_write(10.0, (False, None, "read failed"))
+        mock_write.assert_called_once()
+
+
 if __name__ == "__main__":
     unittest.main()