Merge pull request #216 from plugwise/Fix_schedule_old_states

Working fix for #213

.github/workflows/verify.yml

@@ -4,7 +4,7 @@
 name: Latest commit
 
 env:
-  CACHE_VERSION: 3
+  CACHE_VERSION: 4
   DEFAULT_PYTHON: "3.9"
   PRE_COMMIT_HOME: ~/.cache/pre-commit
 

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Changelog
 
+# v0.22.1: Improve solution for issue #213
+
 # v0.22.0: Smile P1 - add a P1 smartmeter device
 - Change all gateway model names to Gateway
 - Change Anna Smile name to Smile Anna, Anna model name to ThermoTouch

plugwise/__init__.py

@@ -1,6 +1,6 @@
 """Plugwise module."""
 
-__version__ = "0.22.0"
+__version__ = "0.22.1"
 
 from plugwise.smile import Smile
 from plugwise.stick import Stick

plugwise/helper.py

@@ -329,7 +329,7 @@ def __init__(self) -> None:
         self._on_off_device = False
         self._opentherm_device = False
         self._outdoor_temp: float
-        self._schedule_present_state: str
+        self._schedule_old_states: dict[str, dict[str, str]] = {}
         self._sched_setpoints: list[float] | None = None
         self._smile_legacy = False
         self._stretch_v2 = False

plugwise/smile.py

@@ -216,9 +216,14 @@ def _device_data_climate(
             if self._adam_cooling_enabled or self._lortherm_cooling_enabled:
                 device_data["mode"] = "cool"
 
-        self._schedule_present_state = "off"
-        if device_data["mode"] == "auto":
-            self._schedule_present_state = "on"
+        if "None" not in avail_schedules:
+            loc_schedule_states = {}
+            for schedule in avail_schedules:
+                loc_schedule_states[schedule] = "off"
+                if device_data["mode"] == "auto":
+                    loc_schedule_states[sel_schedule] = "on"
+
+            self._schedule_old_states[loc_id] = loc_schedule_states
 
         return device_data
 
@@ -508,7 +513,9 @@ async def async_update(self) -> list[GatewayData | dict[str, DeviceData]]:
 
         return [self.gw_data, self.gw_devices]
 
-    async def _set_schedule_state_legacy(self, name: str, status: str) -> None:
+    async def _set_schedule_state_legacy(
+        self, loc_id: str, name: str, status: str
+    ) -> None:
         """Helper-function for set_schedule_state()."""
         schedule_rule_id: str | None = None
         for rule in self._domain_objects.findall("rule"):
@@ -518,9 +525,13 @@ async def _set_schedule_state_legacy(self, name: str, status: str) -> None:
         if schedule_rule_id is None:
             raise PlugwiseError("Plugwise: no schedule with this name available.")
 
-        state = "false"
+        new_state = "false"
         if status == "on":
-            state = "true"
+            new_state = "true"
+        # If no state change is requested, do nothing
+        if new_state == self._schedule_old_states[loc_id][name]:
+            return
+
         locator = f'.//*[@id="{schedule_rule_id}"]/template'
         for rule in self._domain_objects.findall(locator):
             template_id = rule.attrib["id"]
@@ -529,42 +540,38 @@ async def _set_schedule_state_legacy(self, name: str, status: str) -> None:
         data = (
             "<rules><rule"
             f' id="{schedule_rule_id}"><name><![CDATA[{name}]]></name><template'
-            f' id="{template_id}" /><active>{state}</active></rule></rules>'
+            f' id="{template_id}" /><active>{new_state}</active></rule></rules>'
         )
 
         await self._request(uri, method="put", data=data)
+        self._schedule_old_states[loc_id][name] = new_state
 
     async def set_schedule_state(
-        self, loc_id: str, name: str | None, state: str
+        self, loc_id: str, name: str | None, new_state: str
     ) -> None:
         """Activate/deactivate the Schedule, with the given name, on the relevant Thermostat.
         Determined from - DOMAIN_OBJECTS.
         In HA Core used to set the hvac_mode: in practice switch between schedule on - off.
         """
-        if state not in ["on", "off"]:
+        # Input checking
+        if new_state not in ["on", "off"]:
             raise PlugwiseError("Plugwise: invalid schedule state.")
-
-        # Do nothing when name == None and the state does not change. No need to show
-        # an error, as doing nothing is the correct action in this scenario.
         if name is None:
-            if state == self._schedule_present_state:
-                return
-            # else, raise an error:
             raise PlugwiseError(
                 "Plugwise: cannot change schedule-state: no schedule name provided"
             )
 
         if self._smile_legacy:
-            await self._set_schedule_state_legacy(name, state)
+            await self._set_schedule_state_legacy(loc_id, name, new_state)
             return
 
         schedule_rule = self._rule_ids_by_name(name, loc_id)
         # Raise an error when the schedule name does not exist
         if not schedule_rule or schedule_rule is None:
             raise PlugwiseError("Plugwise: no schedule with this name available.")
 
-        # If schedule name is valid but no state change is requested, do nothing
-        if state == self._schedule_present_state:
+        # If no state change is requested, do nothing
+        if new_state == self._schedule_old_states[loc_id][name]:
             return
 
         schedule_rule_id: str = next(iter(schedule_rule))
@@ -584,10 +591,10 @@ async def set_schedule_state(
             subject = f'<context><zone><location id="{loc_id}" /></zone></context>'
             subject = etree.fromstring(subject)
 
-        if state == "off":
+        if new_state == "off":
             self._last_active[loc_id] = name
             contexts.remove(subject)
-        if state == "on":
+        if new_state == "on":
             contexts.append(subject)
 
         contexts = etree.tostring(contexts, encoding="unicode").rstrip()
@@ -598,8 +605,7 @@ async def set_schedule_state(
             f"{template}{contexts}</rule></rules>"
         )
         await self._request(uri, method="put", data=data)
-
-        self._schedule_present_state = state
+        self._schedule_old_states[loc_id][name] = new_state
 
     async def _set_preset_legacy(self, preset: str) -> None:
         """Set the given Preset on the relevant Thermostat - from DOMAIN_OBJECTS."""

tests/test_smile.py

@@ -584,7 +584,9 @@ async def tinker_thermostat(
 
         result_1 = await self.tinker_thermostat_temp(smile, loc_id, unhappy)
         result_2 = await self.tinker_thermostat_preset(smile, loc_id, unhappy)
-        smile._schedule_present_state = "off"
+        if smile._schedule_old_states != {}:
+            for item in smile._schedule_old_states[loc_id]:
+                smile._schedule_old_states[loc_id][item] = "off"
         result_3 = await self.tinker_thermostat_schedule(
             smile, loc_id, "on", good_schedules, single, unhappy
         )
@@ -702,7 +704,7 @@ async def test_connect_legacy_anna(self):
 
         result = await self.tinker_thermostat(
             smile,
-            "c34c6864216446528e95d88985e714cc",
+            "0000aaaa0000aaaa0000aaaa0000aa00",
             good_schedules=[
                 "Thermostat schedule",
             ],
@@ -712,9 +714,10 @@ async def test_connect_legacy_anna(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
-            "c34c6864216446528e95d88985e714cc",
+            "0000aaaa0000aaaa0000aaaa0000aa00",
             good_schedules=[
                 "Thermostat schedule",
             ],
@@ -797,24 +800,45 @@ async def test_connect_legacy_anna_2(self):
         await self.device_test(smile, testdata)
 
         assert smile.gateway_id == "be81e3f8275b4129852c4d8d550ae2eb"
-        assert self.device_items == 43
+        # assert self.device_items = 47
         assert not self.notifications
 
         result = await self.tinker_thermostat(
             smile,
-            "c34c6864216446528e95d88985e714cc",
+            "be81e3f8275b4129852c4d8d550ae2eb",
             good_schedules=[
                 "Thermostat schedule",
             ],
         )
         assert result
+
+        smile._schedule_old_states["be81e3f8275b4129852c4d8d550ae2eb"][
+            "Thermostat schedule"
+        ] = "off"
+        result_1 = await self.tinker_thermostat_schedule(
+            smile,
+            "be81e3f8275b4129852c4d8d550ae2eb",
+            "on",
+            good_schedules=["Thermostat schedule"],
+            single=True,
+        )
+        result_2 = await self.tinker_thermostat_schedule(
+            smile,
+            "be81e3f8275b4129852c4d8d550ae2eb",
+            "on",
+            good_schedules=["Thermostat schedule"],
+            single=True,
+        )
+        assert result_1 and result_2
+
         await smile.close_connection()
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
-            "c34c6864216446528e95d88985e714cc",
+            "be81e3f8275b4129852c4d8d550ae2eb",
             good_schedules=[
                 "Thermostat schedule",
             ],
@@ -882,8 +906,6 @@ async def test_connect_smile_p1_v2(self):
         await smile.close_connection()
         await self.disconnect(server, client)
 
-        server, smile, client = await self.connect_wrapper(raise_timeout=True)
-
     @pytest.mark.asyncio
     async def test_connect_smile_p1_v2_2(self):
         """Test another legacy P1 device."""
@@ -1045,6 +1067,7 @@ async def test_connect_anna_v4(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "eb5309212bf5407bb143e5bfa3b18aee",
@@ -1155,6 +1178,7 @@ async def test_connect_anna_v4_dhw(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "eb5309212bf5407bb143e5bfa3b18aee",
@@ -1201,6 +1225,7 @@ async def test_connect_anna_v4_no_tag(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "eb5309212bf5407bb143e5bfa3b18aee",
@@ -1284,6 +1309,7 @@ async def test_connect_anna_without_boiler_fw3(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "c34c6864216446528e95d88985e714cc",
@@ -1365,6 +1391,7 @@ async def test_connect_anna_without_boiler_fw4(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "c34c6864216446528e95d88985e714cc",
@@ -1446,6 +1473,7 @@ async def test_connect_anna_without_boiler_fw42(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "c34c6864216446528e95d88985e714cc",
@@ -1580,6 +1608,7 @@ async def test_connect_adam_plus_anna(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "009490cc2f674ce6b576863fbb64f867",
@@ -1827,7 +1856,9 @@ async def test_connect_adam_plus_anna_new(self):
         )
         assert result
 
-        smile._schedule_present_state = "off"
+        smile._schedule_old_states["f2bf9048bef64cc5b6d5110154e33c81"][
+            "Badkamer"
+        ] = "off"
         result_1 = await self.tinker_thermostat_schedule(
             smile,
             "f2bf9048bef64cc5b6d5110154e33c81",
@@ -2264,6 +2295,7 @@ async def test_connect_adam_zone_per_device(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
 
         result = await self.tinker_thermostat(
             smile,
@@ -2681,6 +2713,7 @@ async def test_connect_adam_multiple_devices_per_zone(self):
         await self.disconnect(server, client)
 
         server, smile, client = await self.connect_wrapper(raise_timeout=True)
+        await self.device_test(smile, testdata)
         result = await self.tinker_thermostat(
             smile,
             "c50f167537524366a5af7aa3942feb1e",