Merge pull request #228 from plugwise/Fix_improve

bouwew · web-flow · commit 7690bf25d2b5 · 2022-10-16T13:10:39.000+02:00
Code improvements
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,8 @@
 # Changelog
 
+# v0.25.2: Bugfix, code improvements
+- Fix a set_temperature() and heat_cool related bug
+
 # v0.25.1: Remove sensor-keys from output when cooling is present
 
 # v0.25.0: Improve compatibility with HA Core climate platform
diff --git a/plugwise/__init__.py b/plugwise/__init__.py
@@ -1,6 +1,6 @@
 """Plugwise module."""
 
-__version__ = "0.25.1"
+__version__ = "0.25.2"
 
 from plugwise.smile import Smile
 from plugwise.stick import Stick
diff --git a/plugwise/constants.py b/plugwise/constants.py
@@ -391,14 +391,20 @@
 NONE: Final = "None"
 FAKE_APPL: Final = "aaaa0000aaaa0000aaaa0000aaaa00aa"
 FAKE_LOC: Final = "0000aaaa0000aaaa0000aaaa0000aa00"
+LEGACY_SMILES: Final[tuple[str, ...]] = (
+    "smile_thermo_v1",
+    "smile_v2",
+    "stretch_v2",
+    "stretch_v3",
+)
 LIMITS: Final[tuple[str, ...]] = (
     "setpoint",
     "lower_bound",
     "upper_bound",
     "resolution",
 )
-MAX_SETPOINT: Final[float] = 40.0
-MIN_SETPOINT: Final[float] = 0.0
+MAX_SETPOINT: Final[float] = 30.0
+MIN_SETPOINT: Final[float] = 4.0
 SPECIAL_FORMAT: Final[tuple[str, ...]] = (ENERGY_KILO_WATT_HOUR, VOLUME_CUBIC_METERS)
 SWITCH_GROUP_TYPES: Final[tuple[str, ...]] = ("switching", "report")
 ZONE_THERMOSTATS: Final[tuple[str, ...]] = (
diff --git a/plugwise/helper.py b/plugwise/helper.py
@@ -91,9 +91,9 @@ def update_helper(
 
 def check_model(name: str | None, vendor_name: str | None) -> str | None:
     """Model checking before using version_to_model."""
-    if vendor_name == "Plugwise":
-        if (model := version_to_model(name)) != "Unknown":
-            return model
+    if vendor_name == "Plugwise" and ((model := version_to_model(name)) != "Unknown"):
+        return model
+
     return name
 
 
@@ -160,15 +160,13 @@ def power_data_local_format(
     attrs: dict[str, str], key_string: str, val: str
 ) -> float | int | bool:
     """Format power data."""
-    attrs_uom = getattr(attrs, ATTR_UNIT_OF_MEASUREMENT)
-    f_val = format_measure(val, attrs_uom)
-    # Format only P1_MEASUREMENT POWER_WATT values, do not move to util-format_meaure function!
-    if attrs_uom == POWER_WATT:
-        f_val = int(round(float(val)))
+    # Special formatting of P1_MEASUREMENT POWER_WATT values, do not move to util-format_measure() function!
     if all(item in key_string for item in ("electricity", "cumulative")):
-        f_val = format_measure(val, ENERGY_KILO_WATT_HOUR)
+        return format_measure(val, ENERGY_KILO_WATT_HOUR)
+    if (attrs_uom := getattr(attrs, ATTR_UNIT_OF_MEASUREMENT)) == POWER_WATT:
+        return int(round(float(val)))
 
-    return f_val
+    return format_measure(val, attrs_uom)
 
 
 def power_data_energy_diff(
@@ -384,12 +382,12 @@ def _locations_specials(self, loc: Munch, location: str) -> Munch:
         Correct location info in special cases.
         """
         if loc.name == "Home":
-            self._home_location = loc.id
+            self._home_location = loc.loc_id
 
         # Replace location-name for P1 legacy, can contain privacy-related info
         if self._smile_legacy and self.smile_type == "power":
             loc.name = "Home"
-            self._home_location = loc.id
+            self._home_location = loc.loc_id
 
         return loc
 
@@ -405,7 +403,7 @@ def _all_locations(self) -> None:
 
         for location in locations:
             loc.name = location.find("name").text
-            loc.id = location.attrib["id"]
+            loc.loc_id = location.attrib["id"]
             # Filter the valid single location for P1 legacy: services not empty
             locator = "./services"
             if (
@@ -418,7 +416,7 @@ def _all_locations(self) -> None:
             # Specials
             loc = self._locations_specials(loc, location)
 
-            self._loc_data[loc.id] = {"name": loc.name}
+            self._loc_data[loc.loc_id] = {"name": loc.name}
 
         return
 
@@ -598,7 +596,7 @@ def _appliance_info_finder(self, appliance: etree, appl: Munch) -> Munch:
 
         return appl
 
-    def _p1_smartmeter_info_finder(self, appl: Munch) -> Munch:
+    def _p1_smartmeter_info_finder(self, appl: Munch) -> None:
         """Collect P1 DSMR Smartmeter info."""
         loc_id = next(iter(self._loc_data.keys()))
         appl.dev_id = self.gateway_id
@@ -628,44 +626,40 @@ def _p1_smartmeter_info_finder(self, appl: Munch) -> Munch:
             if value is not None or key == "location":
                 self._appl_data[appl.dev_id].update({key: value})  # type: ignore[misc]
 
-        return appl
-
     def _create_legacy_gateway(self) -> None:
         """Create the (missing) gateway devices for legacy Anna, P1 and Stretch.
 
         Use the home_location or FAKE_APPL as device id.
         """
-        if self._smile_legacy:
-            self.gateway_id = self._home_location
-            if self.smile_type == "power":
-                self.gateway_id = FAKE_APPL
-
-            self._appl_data[self.gateway_id] = {"dev_class": "gateway"}
-            for key, value in {
-                "firmware": self.smile_fw_version,
-                "location": self._home_location,
-                "mac_address": self.smile_mac_address,
-                "model": self.smile_model,
-                "name": self.smile_name,
-                "zigbee_mac_address": self.smile_zigbee_mac_address,
-                "vendor": "Plugwise",
-            }.items():
-                if value is not None:
-                    self._appl_data[self.gateway_id].update({key: value})  # type: ignore[misc]
+        self.gateway_id = self._home_location
+        if self.smile_type == "power":
+            self.gateway_id = FAKE_APPL
 
-            if self.smile_type == "power":
-                # For legacy P1 collect the connected SmartMeter info
-                appl = Munch()
-                appl = self._p1_smartmeter_info_finder(appl)
+        self._appl_data[self.gateway_id] = {"dev_class": "gateway"}
+        for key, value in {
+            "firmware": self.smile_fw_version,
+            "location": self._home_location,
+            "mac_address": self.smile_mac_address,
+            "model": self.smile_model,
+            "name": self.smile_name,
+            "zigbee_mac_address": self.smile_zigbee_mac_address,
+            "vendor": "Plugwise",
+        }.items():
+            if value is not None:
+                self._appl_data[self.gateway_id].update({key: value})  # type: ignore[misc]
 
     def _all_appliances(self) -> None:
         """Collect all appliances with relevant info."""
         self._all_locations()
 
-        self._create_legacy_gateway()
-        # Legacy P1 has no more devices
-        if self._smile_legacy and self.smile_type == "power":
-            return
+        if self._smile_legacy:
+            self._create_legacy_gateway()
+            # For legacy P1 collect the connected SmartMeter info
+            if self.smile_type == "power":
+                appl = Munch()
+                self._p1_smartmeter_info_finder(appl)
+                # Legacy P1 has no more devices
+                return
 
         for appliance in self._appliances.findall("./appliance"):
             appl = Munch()
@@ -728,12 +722,12 @@ def _all_appliances(self) -> None:
 
         # For non-legacy P1 collect the connected SmartMeter info
         if self.smile_type == "power":
-            appl = self._p1_smartmeter_info_finder(appl)
+            self._p1_smartmeter_info_finder(appl)
             # P1: for gateway and smartmeter switch device_id - part 2
             for item in self._appl_data:
                 if item != self.gateway_id:
                     self.gateway_id = item
-                    # Leave for-loop to avoid a 2nd switch
+                    # Leave for-loop to avoid a 2nd device_id switch
                     break
 
     def _match_locations(self) -> dict[str, ThermoLoc]:
@@ -744,7 +738,7 @@ def _match_locations(self) -> dict[str, ThermoLoc]:
 
         self._all_appliances()
         for location_id, location_details in self._loc_data.items():
-            for _, appliance_details in self._appl_data.items():
+            for appliance_details in list(self._appl_data.values()):
                 if appliance_details["location"] == location_id:
                     location_details.update(
                         {"master": None, "master_prio": 0, "slaves": set()}
@@ -949,6 +943,7 @@ def _get_appliance_data(self, d_id: str) -> DeviceData:
             ):
                 if data.get("c_heating_state") and not data.get("heating_state"):
                     data["heating_state"] = True
+                    # For Adam + OnOff cooling heating_state = True means cooling is active
                     if self._cooling_present:
                         self._cooling_active = True
 
diff --git a/plugwise/smile.py b/plugwise/smile.py
@@ -19,6 +19,7 @@
     DEFAULT_TIMEOUT,
     DEFAULT_USERNAME,
     DOMAIN_OBJECTS,
+    LEGACY_SMILES,
     LOCATIONS,
     LOGGER,
     MAX_SETPOINT,
@@ -53,13 +54,7 @@ class SmileData(SmileHelper):
 
     def update_for_cooling(self, devices: dict[str, DeviceData]) -> None:
         """Helper-function for adding/updating various cooling-related values."""
-        for _, device in devices.items():
-            # For Anna or Adama + cooling, modify cooling_state based on provided info by Plugwise
-            if device["dev_class"] == "heater_central" and self._cooling_present:
-                device["binary_sensors"]["cooling_state"] = False
-                if self._cooling_active:
-                    device["binary_sensors"]["cooling_state"] = True
-
+        for device in list(devices.values()):
             # For Adam + on/off cooling, modify heating_state and cooling_state
             # based on provided info by Plugwise
             if (
@@ -76,8 +71,8 @@ def update_for_cooling(self, devices: dict[str, DeviceData]) -> None:
             if device["dev_class"] not in ZONE_THERMOSTATS:
                 continue
 
+            # For heating + cooling, replace setpoint with setpoint_high/_low
             if self._cooling_present:
-                # Replace setpoint with setpoint_high/_low
                 thermostat = device["thermostat"]
                 sensors = device["sensors"]
                 max_setpoint = MAX_SETPOINT
@@ -152,7 +147,7 @@ def get_all_devices(self) -> None:
             self._cooling_present = False
             if search.find(locator_1) is not None:
                 self._cooling_present = True
-            # Alternative method for the Anna with Elga, or alternative method for the Anna with Loria/Thermastage
+            # Alternative method for the Anna with Elga
             elif search.find(locator_2) is not None:
                 self._cooling_present = True
 
@@ -188,11 +183,14 @@ def _device_data_adam(
         """Helper-function for _get_device_data().
         Determine Adam device data.
         """
-        if self.smile_name == "Adam":
-            # Indicate heating_state based on valves being open in case of city-provided heating
-            if details.get("dev_class") == "heater_central":
-                if self._on_off_device and self._heating_valves() is not None:
-                    device_data["heating_state"] = self._heating_valves() != 0
+        # Indicate heating_state based on valves being open in case of city-provided heating
+        if (
+            self.smile_name == "Adam"
+            and details.get("dev_class") == "heater_central"
+            and self._on_off_device
+            and self._heating_valves() is not None
+        ):
+            device_data["heating_state"] = self._heating_valves() != 0
 
         return device_data
 
@@ -210,7 +208,6 @@ def _device_data_climate(
         if presets := self._presets(loc_id):
             presets_list = list(presets)
             device_data["preset_modes"] = presets_list
-
             device_data["active_preset"] = self._preset(loc_id)
 
         # Schedule
@@ -258,16 +255,16 @@ def _check_availability(
         # OpenTherm device
         if details["dev_class"] == "heater_central" and details["name"] != "OnOff":
             device_data["available"] = True
-            for _, data in self._notifications.items():
-                for _, msg in data.items():
+            for data in list(self._notifications.values()):
+                for msg in list(data.values()):
                     if "no OpenTherm communication" in msg:
                         device_data["available"] = False
 
         # Smartmeter
         if details["dev_class"] == "smartmeter":
             device_data["available"] = True
-            for _, data in self._notifications.items():
-                for _, msg in data.items():
+            for data in list(self._notifications.values()):
+                for msg in list(data.values()):
                     if "P1 does not seem to be connected to a smart meter" in msg:
                         device_data["available"] = False
 
@@ -299,31 +296,30 @@ def _get_device_data(self, dev_id: str) -> DeviceData:
             device_data.pop("thermostat")
 
         # Generic
-        if details["dev_class"] == "gateway" or dev_id == self.gateway_id:
-            if self.smile_type == "thermostat":
-                # Adam & Anna: the Smile outdoor_temperature is present in DOMAIN_OBJECTS and LOCATIONS - under Home
-                # The outdoor_temperature present in APPLIANCES is a local sensor connected to the active device
-                outdoor_temperature = self._object_value(
-                    self._home_location, "outdoor_temperature"
-                )
-                if outdoor_temperature is not None:
-                    device_data["outdoor_temperature"] = outdoor_temperature
+        if self.smile_type == "thermostat" and details["dev_class"] == "gateway":
+            # Adam & Anna: the Smile outdoor_temperature is present in DOMAIN_OBJECTS and LOCATIONS - under Home
+            # The outdoor_temperature present in APPLIANCES is a local sensor connected to the active device
+            outdoor_temperature = self._object_value(
+                self._home_location, "outdoor_temperature"
+            )
+            if outdoor_temperature is not None:
+                device_data["outdoor_temperature"] = outdoor_temperature
 
-                # Show the allowed regulation modes
-                if self._reg_allowed_modes:
-                    device_data["regulation_modes"] = self._reg_allowed_modes
+            # Show the allowed regulation modes
+            if self._reg_allowed_modes:
+                device_data["regulation_modes"] = self._reg_allowed_modes
 
         # Show the allowed dhw_modes
-        if details["dev_class"] == "heater_central":
-            if self._dhw_allowed_modes:
-                device_data["dhw_modes"] = self._dhw_allowed_modes
+        if details["dev_class"] == "heater_central" and self._dhw_allowed_modes:
+            device_data["dhw_modes"] = self._dhw_allowed_modes
 
-        if details["dev_class"] == "smartmeter":
-            # Get P1 data from LOCATIONS
-            if (
-                power_data := self._power_data_from_location(details["location"])
-            ) is not None:
-                device_data.update(power_data)
+        # Get P1 data from LOCATIONS
+        if (
+            details["dev_class"] == "smartmeter"
+            and (power_data := self._power_data_from_location(details["location"]))
+            is not None
+        ):
+            device_data.update(power_data)
 
         # Check availability of non-legacy wired-connected devices
         if not self._smile_legacy:
@@ -388,14 +384,13 @@ async def connect(self) -> bool:
             models.append(model.text)
 
         dsmrmain = result.find("./module/protocols/dsmrmain")
-        if "Plugwise" not in names:
-            if dsmrmain is None:  # pragma: no cover
-                LOGGER.error(
-                    "Connected but expected text not returned, we got %s. Please create \
-                    an issue on http://github.com/plugwise/python-plugwise",
-                    result,
-                )
-                raise ResponseError
+        if "Plugwise" not in names and dsmrmain is None:  # pragma: no cover
+            LOGGER.error(
+                "Connected but expected text not returned, we got %s. Please create \
+                an issue on http://github.com/plugwise/python-plugwise",
+                result,
+            )
+            raise ResponseError
 
         # Check if Anna is connected to an Adam
         if "159.2" in models:
@@ -499,7 +494,7 @@ async def _smile_detect(self, result: etree, dsmrmain: etree) -> None:
         self.smile_type = SMILES[target_smile].smile_type
         self.smile_version = (self.smile_fw_version, ver)
 
-        if target_smile in ("smile_thermo_v1", "smile_v2", "stretch_v3", "stretch_v2"):
+        if target_smile in LEGACY_SMILES:
             self._smile_legacy = True
 
         if self.smile_type == "stretch":
diff --git a/tests/test_smile.py b/tests/test_smile.py
