From 0706ae70dc9114f6755afa65f12f6d5023b21518 Mon Sep 17 00:00:00 2001 From: Unai Date: Sun, 28 Mar 2021 00:21:20 +0100 Subject: [PATCH] Simplify maxcube integration (#48403) * Simplify maxcube integration Device objects returned by maxcube-api dependency are stable, so we do not need to resolve from the device address every time. Also, refactor and unify how maxcube integration sets temperature & mode. * Raise ValueError if missing parameters for set_temperature method Raise a ValueError exception If set_temperature does not receive a temperature parameter. Also, document properly _set_target method. * Use Type | None instead of Optional[Type] annotation * Protect set_hvac_mode and set_preset_mode from unsupported parameters --- .../components/maxcube/binary_sensor.py | 22 +-- homeassistant/components/maxcube/climate.py | 185 +++++++----------- tests/components/maxcube/conftest.py | 1 - .../maxcube/test_maxcube_binary_sensor.py | 3 - .../maxcube/test_maxcube_climate.py | 52 ++++- 5 files changed, 134 insertions(+), 129 deletions(-) diff --git a/homeassistant/components/maxcube/binary_sensor.py b/homeassistant/components/maxcube/binary_sensor.py index 376076352a6..223c0e3fc99 100644 --- a/homeassistant/components/maxcube/binary_sensor.py +++ b/homeassistant/components/maxcube/binary_sensor.py @@ -11,13 +11,10 @@ def setup_platform(hass, config, add_entities, discovery_info=None): """Iterate through all MAX! Devices and add window shutters.""" devices = [] for handler in hass.data[DATA_KEY].values(): - cube = handler.cube - for device in cube.devices: - name = f"{cube.room_by_id(device.room_id).name} {device.name}" - + for device in handler.cube.devices: # Only add Window Shutters if device.is_windowshutter(): - devices.append(MaxCubeShutter(handler, name, device.rf_address)) + devices.append(MaxCubeShutter(handler, device)) if devices: add_entities(devices) @@ -26,13 +23,12 @@ def setup_platform(hass, config, add_entities, discovery_info=None): class MaxCubeShutter(BinarySensorEntity): """Representation of a MAX! Cube Binary Sensor device.""" - def __init__(self, handler, name, rf_address): + def __init__(self, handler, device): """Initialize MAX! Cube BinarySensorEntity.""" - self._name = name - self._sensor_type = DEVICE_CLASS_WINDOW - self._rf_address = rf_address + room = handler.cube.room_by_id(device.room_id) + self._name = f"{room.name} {device.name}" self._cubehandle = handler - self._state = None + self._device = device @property def name(self): @@ -42,15 +38,13 @@ class MaxCubeShutter(BinarySensorEntity): @property def device_class(self): """Return the class of this sensor.""" - return self._sensor_type + return DEVICE_CLASS_WINDOW @property def is_on(self): """Return true if the binary sensor is on/open.""" - return self._state + return self._device.is_open def update(self): """Get latest data from MAX! Cube.""" self._cubehandle.update() - device = self._cubehandle.cube.device_by_rf(self._rf_address) - self._state = device.is_open diff --git a/homeassistant/components/maxcube/climate.py b/homeassistant/components/maxcube/climate.py index 7c114a927d6..75ee7ef21f0 100644 --- a/homeassistant/components/maxcube/climate.py +++ b/homeassistant/components/maxcube/climate.py @@ -1,4 +1,6 @@ """Support for MAX! Thermostats via MAX! Cube.""" +from __future__ import annotations + import logging import socket @@ -47,31 +49,14 @@ MAX_TEMPERATURE = 30.0 SUPPORT_FLAGS = SUPPORT_TARGET_TEMPERATURE | SUPPORT_PRESET_MODE -HASS_PRESET_TO_MAX_MODE = { - PRESET_AWAY: MAX_DEVICE_MODE_VACATION, - PRESET_BOOST: MAX_DEVICE_MODE_BOOST, - PRESET_NONE: MAX_DEVICE_MODE_AUTOMATIC, - PRESET_ON: MAX_DEVICE_MODE_MANUAL, -} - -MAX_MODE_TO_HASS_PRESET = { - MAX_DEVICE_MODE_AUTOMATIC: PRESET_NONE, - MAX_DEVICE_MODE_BOOST: PRESET_BOOST, - MAX_DEVICE_MODE_MANUAL: PRESET_NONE, - MAX_DEVICE_MODE_VACATION: PRESET_AWAY, -} - def setup_platform(hass, config, add_entities, discovery_info=None): """Iterate through all MAX! Devices and add thermostats.""" devices = [] for handler in hass.data[DATA_KEY].values(): - cube = handler.cube - for device in cube.devices: - name = f"{cube.room_by_id(device.room_id).name} {device.name}" - + for device in handler.cube.devices: if device.is_thermostat() or device.is_wallthermostat(): - devices.append(MaxCubeClimate(handler, name, device.rf_address)) + devices.append(MaxCubeClimate(handler, device)) if devices: add_entities(devices) @@ -80,11 +65,12 @@ def setup_platform(hass, config, add_entities, discovery_info=None): class MaxCubeClimate(ClimateEntity): """MAX! Cube ClimateEntity.""" - def __init__(self, handler, name, rf_address): + def __init__(self, handler, device): """Initialize MAX! Cube ClimateEntity.""" - self._name = name - self._rf_address = rf_address + room = handler.cube.room_by_id(device.room_id) + self._name = f"{room.name} {device.name}" self._cubehandle = handler + self._device = device @property def supported_features(self): @@ -104,20 +90,15 @@ class MaxCubeClimate(ClimateEntity): @property def min_temp(self): """Return the minimum temperature.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - if device.min_temperature is None: - return MIN_TEMPERATURE + temp = self._device.min_temperature or MIN_TEMPERATURE # OFF_TEMPERATURE (always off) a is valid temperature to maxcube but not to Home Assistant. # We use HVAC_MODE_OFF instead to represent a turned off thermostat. - return max(device.min_temperature, MIN_TEMPERATURE) + return max(temp, MIN_TEMPERATURE) @property def max_temp(self): """Return the maximum temperature.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - if device.max_temperature is None: - return MAX_TEMPERATURE - return device.max_temperature + return self._device.max_temperature or MAX_TEMPERATURE @property def temperature_unit(self): @@ -127,18 +108,17 @@ class MaxCubeClimate(ClimateEntity): @property def current_temperature(self): """Return the current temperature.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - return device.actual_temperature + return self._device.actual_temperature @property def hvac_mode(self): """Return current operation mode.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - if device.mode in [MAX_DEVICE_MODE_AUTOMATIC, MAX_DEVICE_MODE_BOOST]: + mode = self._device.mode + if mode in [MAX_DEVICE_MODE_AUTOMATIC, MAX_DEVICE_MODE_BOOST]: return HVAC_MODE_AUTO if ( - device.mode == MAX_DEVICE_MODE_MANUAL - and device.target_temperature == OFF_TEMPERATURE + mode == MAX_DEVICE_MODE_MANUAL + and self._device.target_temperature == OFF_TEMPERATURE ): return HVAC_MODE_OFF @@ -151,37 +131,46 @@ class MaxCubeClimate(ClimateEntity): def set_hvac_mode(self, hvac_mode: str): """Set new target hvac mode.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - temp = device.target_temperature - mode = MAX_DEVICE_MODE_MANUAL - if hvac_mode == HVAC_MODE_OFF: - temp = OFF_TEMPERATURE + self._set_target(MAX_DEVICE_MODE_MANUAL, OFF_TEMPERATURE) elif hvac_mode == HVAC_MODE_HEAT: - temp = max(temp, self.min_temp) + temp = max(self._device.target_temperature, self.min_temp) + self._set_target(MAX_DEVICE_MODE_MANUAL, temp) + elif hvac_mode == HVAC_MODE_AUTO: + self._set_target(MAX_DEVICE_MODE_AUTOMATIC, None) else: - temp = None - mode = MAX_DEVICE_MODE_AUTOMATIC + raise ValueError(f"unsupported HVAC mode {hvac_mode}") - cube = self._cubehandle.cube + def _set_target(self, mode: int | None, temp: float | None) -> None: + """ + Set the mode and/or temperature of the thermostat. + + @param mode: this is the mode to change to. + @param temp: the temperature to target. + + Both parameters are optional. When mode is undefined, it keeps + the previous mode. When temp is undefined, it fetches the + temperature from the weekly schedule when mode is + MAX_DEVICE_MODE_AUTOMATIC and keeps the previous + temperature otherwise. + """ with self._cubehandle.mutex: try: - cube.set_temperature_mode(device, temp, mode) + self._cubehandle.cube.set_temperature_mode(self._device, temp, mode) except (socket.timeout, OSError): _LOGGER.error("Setting HVAC mode failed") - return @property def hvac_action(self): """Return the current running hvac operation if supported.""" - cube = self._cubehandle.cube - device = cube.device_by_rf(self._rf_address) valve = 0 - if device.is_thermostat(): - valve = device.valve_position - elif device.is_wallthermostat(): - for device in cube.devices_by_room(cube.room_by_id(device.room_id)): + if self._device.is_thermostat(): + valve = self._device.valve_position + elif self._device.is_wallthermostat(): + cube = self._cubehandle.cube + room = cube.room_by_id(self._device.room_id) + for device in cube.devices_by_room(room): if device.is_thermostat() and device.valve_position > 0: valve = device.valve_position break @@ -199,49 +188,35 @@ class MaxCubeClimate(ClimateEntity): @property def target_temperature(self): """Return the temperature we try to reach.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - if ( - device.target_temperature is None - or device.target_temperature < self.min_temp - or device.target_temperature > self.max_temp - ): + temp = self._device.target_temperature + if temp is None or temp < self.min_temp or temp > self.max_temp: return None - return device.target_temperature + return temp def set_temperature(self, **kwargs): """Set new target temperatures.""" - if kwargs.get(ATTR_TEMPERATURE) is None: - return False - - target_temperature = kwargs.get(ATTR_TEMPERATURE) - device = self._cubehandle.cube.device_by_rf(self._rf_address) - - cube = self._cubehandle.cube - - with self._cubehandle.mutex: - try: - cube.set_target_temperature(device, target_temperature) - except (socket.timeout, OSError): - _LOGGER.error("Setting target temperature failed") - return False + temp = kwargs.get(ATTR_TEMPERATURE) + if temp is None: + raise ValueError( + f"No {ATTR_TEMPERATURE} parameter passed to set_temperature method." + ) + self._set_target(None, temp) @property def preset_mode(self): """Return the current preset mode.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - if self.hvac_mode == HVAC_MODE_OFF: - return PRESET_NONE - - if device.mode == MAX_DEVICE_MODE_MANUAL: - if device.target_temperature == device.comfort_temperature: + if self._device.mode == MAX_DEVICE_MODE_MANUAL: + if self._device.target_temperature == self._device.comfort_temperature: return PRESET_COMFORT - if device.target_temperature == device.eco_temperature: + if self._device.target_temperature == self._device.eco_temperature: return PRESET_ECO - if device.target_temperature == ON_TEMPERATURE: + if self._device.target_temperature == ON_TEMPERATURE: return PRESET_ON - return PRESET_NONE - - return MAX_MODE_TO_HASS_PRESET[device.mode] + elif self._device.mode == MAX_DEVICE_MODE_BOOST: + return PRESET_BOOST + elif self._device.mode == MAX_DEVICE_MODE_VACATION: + return PRESET_AWAY + return PRESET_NONE @property def preset_modes(self): @@ -257,37 +232,27 @@ class MaxCubeClimate(ClimateEntity): def set_preset_mode(self, preset_mode): """Set new operation mode.""" - device = self._cubehandle.cube.device_by_rf(self._rf_address) - temp = None - mode = MAX_DEVICE_MODE_AUTOMATIC - - if preset_mode in [PRESET_COMFORT, PRESET_ECO, PRESET_ON]: - mode = MAX_DEVICE_MODE_MANUAL - if preset_mode == PRESET_COMFORT: - temp = device.comfort_temperature - elif preset_mode == PRESET_ECO: - temp = device.eco_temperature - else: - temp = ON_TEMPERATURE + if preset_mode == PRESET_COMFORT: + self._set_target(MAX_DEVICE_MODE_MANUAL, self._device.comfort_temperature) + elif preset_mode == PRESET_ECO: + self._set_target(MAX_DEVICE_MODE_MANUAL, self._device.eco_temperature) + elif preset_mode == PRESET_ON: + self._set_target(MAX_DEVICE_MODE_MANUAL, ON_TEMPERATURE) + elif preset_mode == PRESET_AWAY: + self._set_target(MAX_DEVICE_MODE_VACATION, None) + elif preset_mode == PRESET_BOOST: + self._set_target(MAX_DEVICE_MODE_BOOST, None) + elif preset_mode == PRESET_NONE: + self._set_target(MAX_DEVICE_MODE_AUTOMATIC, None) else: - mode = HASS_PRESET_TO_MAX_MODE[preset_mode] or MAX_DEVICE_MODE_AUTOMATIC - - with self._cubehandle.mutex: - try: - self._cubehandle.cube.set_temperature_mode(device, temp, mode) - except (socket.timeout, OSError): - _LOGGER.error("Setting operation mode failed") - return + raise ValueError(f"unsupported preset mode {preset_mode}") @property def extra_state_attributes(self): """Return the optional state attributes.""" - cube = self._cubehandle.cube - device = cube.device_by_rf(self._rf_address) - - if not device.is_thermostat(): + if not self._device.is_thermostat(): return {} - return {ATTR_VALVE_POSITION: device.valve_position} + return {ATTR_VALVE_POSITION: self._device.valve_position} def update(self): """Get latest data from MAX! Cube.""" diff --git a/tests/components/maxcube/conftest.py b/tests/components/maxcube/conftest.py index 7f45634b986..6b283cf87c0 100644 --- a/tests/components/maxcube/conftest.py +++ b/tests/components/maxcube/conftest.py @@ -102,7 +102,6 @@ async def cube(hass, hass_config, room, thermostat, wallthermostat, windowshutte cube.devices = [thermostat, wallthermostat, windowshutter] cube.room_by_id.return_value = room cube.devices_by_room.return_value = [thermostat, wallthermostat, windowshutter] - cube.device_by_rf.side_effect = {d.rf_address: d for d in cube.devices}.get assert await async_setup_component(hass, DOMAIN, hass_config) await hass.async_block_till_done() gateway = hass_config[DOMAIN]["gateways"][0] diff --git a/tests/components/maxcube/test_maxcube_binary_sensor.py b/tests/components/maxcube/test_maxcube_binary_sensor.py index df448284e80..db5228c5c9a 100644 --- a/tests/components/maxcube/test_maxcube_binary_sensor.py +++ b/tests/components/maxcube/test_maxcube_binary_sensor.py @@ -20,9 +20,6 @@ ENTITY_ID = "binary_sensor.testroom_testshutter" async def test_window_shuttler(hass, cube: MaxCube, windowshutter: MaxWindowShutter): """Test a successful setup with a shuttler device.""" - async_fire_time_changed(hass, utcnow() + timedelta(minutes=5)) - await hass.async_block_till_done() - state = hass.states.get(ENTITY_ID) assert state is not None assert state.state == STATE_ON diff --git a/tests/components/maxcube/test_maxcube_climate.py b/tests/components/maxcube/test_maxcube_climate.py index e700763769c..b59e1372fde 100644 --- a/tests/components/maxcube/test_maxcube_climate.py +++ b/tests/components/maxcube/test_maxcube_climate.py @@ -10,6 +10,7 @@ from maxcube.device import ( ) from maxcube.thermostat import MaxThermostat from maxcube.wallthermostat import MaxWallThermostat +import pytest from homeassistant.components.climate.const import ( ATTR_CURRENT_TEMPERATURE, @@ -20,11 +21,14 @@ from homeassistant.components.climate.const import ( ATTR_MIN_TEMP, ATTR_PRESET_MODE, ATTR_PRESET_MODES, + ATTR_TARGET_TEMP_HIGH, + ATTR_TARGET_TEMP_LOW, CURRENT_HVAC_HEAT, CURRENT_HVAC_IDLE, CURRENT_HVAC_OFF, DOMAIN as CLIMATE_DOMAIN, HVAC_MODE_AUTO, + HVAC_MODE_DRY, HVAC_MODE_HEAT, HVAC_MODE_OFF, PRESET_AWAY, @@ -155,6 +159,20 @@ async def test_thermostat_set_hvac_mode_heat( assert state.state == HVAC_MODE_HEAT +async def test_thermostat_set_invalid_hvac_mode( + hass, cube: MaxCube, thermostat: MaxThermostat +): + """Set hvac mode to heat.""" + with pytest.raises(ValueError): + await hass.services.async_call( + CLIMATE_DOMAIN, + SERVICE_SET_HVAC_MODE, + {ATTR_ENTITY_ID: ENTITY_ID, ATTR_HVAC_MODE: HVAC_MODE_DRY}, + blocking=True, + ) + cube.set_temperature_mode.assert_not_called() + + async def test_thermostat_set_temperature( hass, cube: MaxCube, thermostat: MaxThermostat ): @@ -165,7 +183,7 @@ async def test_thermostat_set_temperature( {ATTR_ENTITY_ID: ENTITY_ID, ATTR_TEMPERATURE: 10.0}, blocking=True, ) - cube.set_target_temperature.assert_called_once_with(thermostat, 10.0) + cube.set_temperature_mode.assert_called_once_with(thermostat, 10.0, None) thermostat.target_temperature = 10.0 thermostat.valve_position = 0 @@ -178,6 +196,24 @@ async def test_thermostat_set_temperature( assert state.attributes.get(ATTR_HVAC_ACTION) == CURRENT_HVAC_IDLE +async def test_thermostat_set_no_temperature( + hass, cube: MaxCube, thermostat: MaxThermostat +): + """Set hvac mode to heat.""" + with pytest.raises(ValueError): + await hass.services.async_call( + CLIMATE_DOMAIN, + SERVICE_SET_TEMPERATURE, + { + ATTR_ENTITY_ID: ENTITY_ID, + ATTR_TARGET_TEMP_HIGH: 29.0, + ATTR_TARGET_TEMP_LOW: 10.0, + }, + blocking=True, + ) + cube.set_temperature_mode.assert_not_called() + + async def test_thermostat_set_preset_on(hass, cube: MaxCube, thermostat: MaxThermostat): """Set preset mode to on.""" await hass.services.async_call( @@ -317,6 +353,20 @@ async def test_thermostat_set_preset_none( ) +async def test_thermostat_set_invalid_preset( + hass, cube: MaxCube, thermostat: MaxThermostat +): + """Set hvac mode to heat.""" + with pytest.raises(ValueError): + await hass.services.async_call( + CLIMATE_DOMAIN, + SERVICE_SET_PRESET_MODE, + {ATTR_ENTITY_ID: ENTITY_ID, ATTR_PRESET_MODE: "invalid"}, + blocking=True, + ) + cube.set_temperature_mode.assert_not_called() + + async def test_wallthermostat_set_hvac_mode_heat( hass, cube: MaxCube, wallthermostat: MaxWallThermostat ):