Palazzetti integration #128259
Palazzetti integration #128259
Conversation
|
Not sure about the 2 failed checks:
|
|
It looks like it's failing because the MIT license isn't present in |
| formatted_mac = dr.format_mac(palazzetti.mac) | ||
|
|
||
| # Assign a unique ID to the flow | ||
| await self.async_set_unique_id(formatted_mac) |
There was a problem hiding this comment.
Can you give an example of the mac? If you can find it using a tool like https://www.wireshark.org/tools/oui-lookup.html, you can use dhcp discovery
There was a problem hiding this comment.
Yes. I wanted to introduce dhcp discovery in a subsequent PR, when this MVP is completed.
| LOGGER.exception(err) | ||
| available = False |
There was a problem hiding this comment.
If its not available we should rather just raise UpdateFailed
| data: PalazzettiData = { | ||
| AVAILABLE: available, | ||
| IS_HEATING: self.palazzetti.is_heating, | ||
| TARGET_TEMPERATURE: self.palazzetti.target_temperature, | ||
| ROOM_TEMPERATURE: self.palazzetti.room_temperature, | ||
| OUTLET_TEMPERATURE: self.palazzetti.outlet_temperature, | ||
| EXHAUST_TEMPERATURE: self.palazzetti.exhaust_temperature, | ||
| PELLET_QUANTITY: self.palazzetti.pellet_quantity, | ||
| FAN_SPEED: self.palazzetti.fan_speed, | ||
| } | ||
| return data |
There was a problem hiding this comment.
You can also return nothing and have the entities directly reference self.coordinator.palazzetti (or rename to client for ease of access)
There was a problem hiding this comment.
Thanks! I was slightly confused about maintaining state at this level and this makes much more sense.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| try: | ||
| await coordinator.client.connect() | ||
| await coordinator.client.update_state() | ||
| except (CommunicationError, ValidationError) as err: | ||
| raise ConfigEntryNotReady from err |
There was a problem hiding this comment.
can be moved to _async_setup in the coordinator (which will be called automatically during async_config_entry_first_refresh), making this a bit cleaner
| if coordinator.client.connected: | ||
| entities.append( | ||
| PalazzettiClimateEntity( | ||
| coordinator=coordinator, | ||
| ) | ||
| ) | ||
| async_add_entities(entities) | ||
| else: | ||
| raise ConfigEntryNotReady |
There was a problem hiding this comment.
is this really necessary? wouldn't we raise during the connection if devices are unavailable? If we can add the devices add them, they will show unavailble anyways
| """List the hvac modes.""" | ||
| if self.coordinator.client.has_on_off_switch: | ||
| return [HVACMode.OFF, HVACMode.HEAT] | ||
| return [self.hvac_mode] |
There was a problem hiding this comment.
what does this mean? if the device has no switch it's either always on or off? if it's always off does this make sense?
There was a problem hiding this comment.
The on/off switch becomes unavaiables at times, during ignition and shut off cycles. An API call to turn on/off during that time will not raise an error but it will fail.
^ adding this as a comment for clarity
There was a problem hiding this comment.
The climate should then always return both modes and we should just handle the error gracefully, letting the user know what's going on (raising HomeAssistantError)
There was a problem hiding this comment.
But the API it letting me know that this action is not available at that moment. Why let the user take it and get back an error?
There was a problem hiding this comment.
Because then you can let the user know why it's not working instead of the option just disappearing. I believe we are usually don't change supported features at runtime.
| class PalazzettiConfigFlow(ConfigFlow, domain=DOMAIN): | ||
| """Palazzetti config flow.""" | ||
|
|
||
| VERSION = 1 |
There was a problem hiding this comment.
| VERSION = 1 |
default
| self, user_input: dict[str, Any] | None = None | ||
| ) -> ConfigFlowResult: | ||
| """User configuration step.""" | ||
| if user_input and CONF_HOST in user_input: |
There was a problem hiding this comment.
| if user_input and CONF_HOST in user_input: | |
| if user_input is not None: |
host is required, so this is all we need to check for
| with ( | ||
| patch( | ||
| "homeassistant.components.palazzetti.config_flow.PalazzettiClient.connect", | ||
| return_value=True, | ||
| ) as mock_connect, | ||
| patch( | ||
| "homeassistant.components.palazzetti.config_flow.PalazzettiClient.update_state", | ||
| return_value=True, | ||
| ) as mock_update_state, | ||
| patch( | ||
| "homeassistant.components.palazzetti.config_flow.PalazzettiClient.mac", | ||
| return_value="11:22:33:44:55:66", | ||
| ) as mock_mac, | ||
| patch( | ||
| "homeassistant.components.palazzetti.config_flow.PalazzettiClient.name", | ||
| return_value="stove", | ||
| ) as mock_name, |
There was a problem hiding this comment.
is there a reason you're not using your fixture for that?
| assert len(mock_update_state.mock_calls) == 0 | ||
| assert len(mock_mac.mock_calls) > 0 | ||
| assert len(mock_name.mock_calls) > 0 |
| assert result.get("type") is FlowResultType.FORM | ||
| assert result.get("errors") == {"base": "invalid_host"} |
| await hass.config_entries.async_unload(mock_config_entry.entry_id) | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert not hass.data.get(DOMAIN) |
There was a problem hiding this comment.
| assert not hass.data.get(DOMAIN) |
| if fan_mode == FAN_SILENT: | ||
| await self.coordinator.client.set_fan_silent() | ||
| elif fan_mode == FAN_HIGH: | ||
| await self.coordinator.client.set_fan_high() | ||
| elif fan_mode == FAN_AUTO: | ||
| await self.coordinator.client.set_fan_auto() |
There was a problem hiding this comment.
those can probably throw, right?
| return True | ||
|
|
||
|
|
||
| async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: |
| _attr_temperature_unit = UnitOfTemperature.CELSIUS | ||
| _attr_translation_key = DOMAIN | ||
|
|
||
| def __init__(self, *, coordinator: PalazzettiDataUpdateCoordinator) -> None: |
There was a problem hiding this comment.
Personal opinion, having the coordinator as kwargs doesn't add much imo
| """Defines a Palazzetti climate.""" | ||
|
|
||
| _attr_has_entity_name = True | ||
| _attr_hvac_modes = [] # The available modes will be set when we know the current state |
There was a problem hiding this comment.
Don't initialize mutable objects like dicts or lists on class level. They will be shared with every instance of this class, causing side effects
| """Initialize Palazzetti climate.""" | ||
| super().__init__(coordinator=coordinator) | ||
| client = coordinator.client | ||
| self._attr_unique_id = f"{coordinator.config_entry.data[CONF_MAC]}-climate" |
There was a problem hiding this comment.
unique ids are unique per domain per platform, so unless you are going to add more climates, the suffix isn't needed
| client = coordinator.client | ||
| self._attr_unique_id = f"{coordinator.config_entry.data[CONF_MAC]}-climate" | ||
| self._attr_device_info = DeviceInfo( | ||
| identifiers={(DOMAIN, coordinator.config_entry.data[CONF_MAC])}, |
There was a problem hiding this comment.
Use connections instead to allow the device to be merged with other discovered devices (from integrations like unifi that create devices for devices found on the network)
| temperature = cast(float, kwargs.get(ATTR_TEMPERATURE)) | ||
| await self.coordinator.client.set_target_temperature(int(temperature)) |
There was a problem hiding this comment.
why do we cast to a float and then int it?
| await self.coordinator.client.set_fan_auto() | ||
| else: | ||
| await self.coordinator.client.set_fan_speed(FAN_MODES.index(fan_mode)) | ||
| except (CommunicationError, ValidationError) as err: |
There was a problem hiding this comment.
The API may throw a ValidationError if the argument is out of bounds
There was a problem hiding this comment.
But is that something the user should act on or something temporary? In that case we should raise ServiceValidationError
| # Abort the flow if a config entry with the same unique ID exists | ||
| self._abort_if_unique_id_configured() | ||
|
|
||
| name = palazzetti.name |
There was a problem hiding this comment.
Ah so the name comes from the device? Why do we store that in the config entry instead of fetching it at runtime?
| except (CommunicationError, ValidationError) as err: | ||
| raise UpdateFailed(f"Error communicating with API: {err}") from err | ||
|
|
||
| async def _async_setup(self) -> None: |
There was a problem hiding this comment.
Rather put this above the _async_update_data
|
|
||
| async def _async_setup(self) -> None: | ||
| try: | ||
| await self.client.connect() |
There was a problem hiding this comment.
Do we need to disconnect somewhere?
There was a problem hiding this comment.
No, the connect method is getting some static device config/properties. The underlying is an http GET request. It needs to be called once (at least), and there are no connections to close.
| @property | ||
| def supported_features(self) -> ClimateEntityFeature: | ||
| """Supported features.""" | ||
| return ( | ||
| ClimateEntityFeature.TARGET_TEMPERATURE | ||
| | ClimateEntityFeature.FAN_MODE | ||
| | ClimateEntityFeature.TURN_ON | ||
| | ClimateEntityFeature.TURN_OFF | ||
| ) |
There was a problem hiding this comment.
This can be moved to _attr_supported_features = ...
| try: | ||
| await self.coordinator.client.set_on(hvac_mode != HVACMode.OFF) | ||
| except CommunicationError as err: | ||
| raise HomeAssistantError from err |
There was a problem hiding this comment.
Can we add more context to this error?
| translation_domain=DOMAIN, | ||
| translation_key=TEMPERATURE_OUT_OF_BOUNDS, | ||
| translation_placeholders={ | ||
| "value": f"{temperature}", |
There was a problem hiding this comment.
| "value": f"{temperature}", | |
| "value": str(temperature), |
| DOMAIN: Final = "palazzetti" | ||
| PALAZZETTI: Final = "Palazzetti" | ||
| LOGGER = logging.getLogger(__package__) | ||
| SCAN_INTERVAL = timedelta(seconds=5) |
There was a problem hiding this comment.
I find this to be a very low scan interval. I don't believe that a heater changes every 5 seconds. If you want the device to be responsive after an action has been done, be sure to call self.coordinator.async_refresh() to get a new state
| await self.client.connect() | ||
| await self.client.update_state() | ||
| except (CommunicationError, ValidationError) as err: | ||
| raise ConfigEntryNotReady from err |
There was a problem hiding this comment.
| raise ConfigEntryNotReady from err | |
| raise UpdateFailed("...") from err |
| "config": { | ||
| "step": { | ||
| "user": { | ||
| "description": "Enter the host name or the IP address of the Palazzetti CBox", |
There was a problem hiding this comment.
Rather use data_description to add this to the config flow
| "abort": { | ||
| "single_instance_allowed": "[%key:common::config_flow::abort::single_instance_allowed%]", | ||
| "no_devices_found": "[%key:common::config_flow::abort::no_devices_found%]" | ||
| }, |
There was a problem hiding this comment.
Both are not used. Instead, you need already_configured
| }, | ||
| "exceptions": { | ||
| "on_off_not_available": { | ||
| "message": "The appliance cannot be turned on or off atthe moment. Try again later." |
There was a problem hiding this comment.
| "message": "The appliance cannot be turned on or off atthe moment. Try again later." | |
| "message": "The appliance cannot be turned on or off at the moment. Try again later." |
| "message": "The appliance cannot be turned on or off atthe moment. Try again later." | ||
| }, | ||
| "fan_speed_out_of_bounds": { | ||
| "message": "This fan mode ({value}) is not supported." |
There was a problem hiding this comment.
| "message": "This fan mode ({value}) is not supported." | |
| "message": "Fan mode {value} is not supported." |
| "message": "This fan mode ({value}) is not supported." | ||
| }, | ||
| "target_temperature_out_of_bounds": { | ||
| "message": "This target temperature ({value}) is not supported." |
There was a problem hiding this comment.
out of bounds is different than not supported imo
| mac = self.coordinator.config_entry.data[CONF_MAC] | ||
| self._attr_unique_id = mac | ||
| self._attr_device_info = dr.DeviceInfo( | ||
| identifiers={(DOMAIN, mac)}, |
There was a problem hiding this comment.
| identifiers={(DOMAIN, mac)}, |
Can be removed as connections is also fine
| _attr_temperature_unit = UnitOfTemperature.CELSIUS | ||
| _attr_translation_key = DOMAIN | ||
|
|
||
| def __init__(self, *kargs, **kwargs) -> None: | ||
| """Initialize Palazzetti climate.""" | ||
| super().__init__(*kargs, **kwargs) | ||
| client = self.coordinator.client | ||
| mac = self.coordinator.config_entry.data[CONF_MAC] | ||
| self._attr_unique_id = mac | ||
| self._attr_device_info = dr.DeviceInfo( | ||
| identifiers={(DOMAIN, mac)}, | ||
| connections={(dr.CONNECTION_NETWORK_MAC, mac)}, | ||
| name=client.name, | ||
| manufacturer=PALAZZETTI, | ||
| sw_version=client.sw_version, | ||
| hw_version=client.hw_version, | ||
| ) | ||
| self._attr_supported_features = ( | ||
| ClimateEntityFeature.TARGET_TEMPERATURE | ||
| | ClimateEntityFeature.FAN_MODE | ||
| | ClimateEntityFeature.TURN_ON | ||
| | ClimateEntityFeature.TURN_OFF | ||
| ) |
There was a problem hiding this comment.
| _attr_temperature_unit = UnitOfTemperature.CELSIUS | |
| _attr_translation_key = DOMAIN | |
| def __init__(self, *kargs, **kwargs) -> None: | |
| """Initialize Palazzetti climate.""" | |
| super().__init__(*kargs, **kwargs) | |
| client = self.coordinator.client | |
| mac = self.coordinator.config_entry.data[CONF_MAC] | |
| self._attr_unique_id = mac | |
| self._attr_device_info = dr.DeviceInfo( | |
| identifiers={(DOMAIN, mac)}, | |
| connections={(dr.CONNECTION_NETWORK_MAC, mac)}, | |
| name=client.name, | |
| manufacturer=PALAZZETTI, | |
| sw_version=client.sw_version, | |
| hw_version=client.hw_version, | |
| ) | |
| self._attr_supported_features = ( | |
| ClimateEntityFeature.TARGET_TEMPERATURE | |
| | ClimateEntityFeature.FAN_MODE | |
| | ClimateEntityFeature.TURN_ON | |
| | ClimateEntityFeature.TURN_OFF | |
| ) | |
| _attr_temperature_unit = UnitOfTemperature.CELSIUS | |
| _attr_translation_key = DOMAIN | |
| _attr_supported_features = ( | |
| ClimateEntityFeature.TARGET_TEMPERATURE | |
| | ClimateEntityFeature.FAN_MODE | |
| | ClimateEntityFeature.TURN_ON | |
| | ClimateEntityFeature.TURN_OFF | |
| ) | |
| def __init__(self, *kargs, **kwargs) -> None: | |
| """Initialize Palazzetti climate.""" | |
| super().__init__(*kargs, **kwargs) | |
| client = self.coordinator.client | |
| mac = self.coordinator.config_entry.data[CONF_MAC] | |
| self._attr_unique_id = mac | |
| self._attr_device_info = dr.DeviceInfo( | |
| identifiers={(DOMAIN, mac)}, | |
| connections={(dr.CONNECTION_NETWORK_MAC, mac)}, | |
| name=client.name, | |
| manufacturer=PALAZZETTI, | |
| sw_version=client.sw_version, | |
| hw_version=client.hw_version, | |
| ) |
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, translation_key="cannot_connect" | ||
| ) from err |
There was a problem hiding this comment.
Can we add this to the other CommuncationErrors as well?
| _attr_name = None | ||
| _attr_target_temperature_step = 1.0 | ||
| _attr_temperature_unit = UnitOfTemperature.CELSIUS | ||
| _attr_translation_key = DOMAIN |
There was a problem hiding this comment.
Why does this have a translation key
There was a problem hiding this comment.
absolutely no reason
| CONF_HOST: "127.0.0.1", | ||
| CONF_MAC: "11:22:33:44:55:66", | ||
| }, | ||
| unique_id="unique_id", |
There was a problem hiding this comment.
Can we use a proper unique_id?
| CONF_NAME: "Stove", | ||
| CONF_HOST: "127.0.0.1", | ||
| CONF_MAC: "11:22:33:44:55:66", |
There was a problem hiding this comment.
the data we create the entry with now has changed
| def mock_palazzetti(): | ||
| """Return a mocked PalazzettiClient.""" | ||
| with ( | ||
| patch( | ||
| "homeassistant.components.palazzetti.coordinator.PalazzettiClient", | ||
| AsyncMock, | ||
| ) as client, | ||
| ): |
There was a problem hiding this comment.
Recommend checking out how we patch the Airgradient client in airgradient. We patch both the normal client as the config flow ones. This gets rid of the patch in the test_config_flow.py
| mock_palazzetti: AsyncMock, | ||
| ) -> None: | ||
| """Test the creation and values of Palazzetti climate device.""" | ||
| patch("pypalazzetti.client.PalazzettiClient", mock_palazzetti) |
There was a problem hiding this comment.
With the last comment, this would not be needed
| await hass.config_entries.async_setup(mock_config_entry.entry_id) | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert mock_config_entry.state is ConfigEntryState.LOADED | ||
| assert entity_registry.async_is_registered(CLIMATE_ID) | ||
|
|
||
| entity = entity_registry.async_get(CLIMATE_ID) | ||
|
|
||
| assert entity.unique_id == "11:22:33:44:55:66" |
There was a problem hiding this comment.
I would recommend looking into snapshot testing. Check flexit_bacnet or airgradient as example. You can just have the == snapshot and then run the tests like pytest ./tests/components/palazzetti --snapshot-update and it will generate the .ambr files which fixate the entity.
| mock_palazzetti, | ||
| ) as mock_client, | ||
| ): | ||
| result2 = await hass.config_entries.flow.async_configure( |
There was a problem hiding this comment.
No need to create result2, since you never use result afterwards anyway, might as well overwrite (personal opinion)
| user_input={CONF_HOST: "192.168.1.1"}, | ||
| ) | ||
|
|
||
| assert result2["type"] is FlowResultType.CREATE_ENTRY |
There was a problem hiding this comment.
We should also patch the async_setup_entry, since otherwise the integration will just start up here, causing lingering test tasks. Please check the mock_setup_entry fixture that is frequently used.
| user_input={CONF_HOST: "192.168.1.1"}, | ||
| ) | ||
|
|
||
| assert result2["type"] is FlowResultType.CREATE_ENTRY |
There was a problem hiding this comment.
We should assert more here. Check the name of the entry, the unique_id, the data in the result
| async def test_invalid_host(hass: HomeAssistant) -> None: | ||
| """Test we handle cannot connect error.""" | ||
| with ( | ||
| patch( | ||
| "homeassistant.components.palazzetti.coordinator.PalazzettiClient.connect", | ||
| side_effect=CommunicationError(), | ||
| ), | ||
| ): | ||
| result = await hass.config_entries.flow.async_init( | ||
| DOMAIN, | ||
| context={"source": SOURCE_USER}, | ||
| data={CONF_HOST: "192.168.1.1"}, | ||
| ) | ||
|
|
||
| assert result["type"] is FlowResultType.FORM | ||
| assert result["errors"] == {"base": "cannot_connect"} |
There was a problem hiding this comment.
All the tests should end in either ABORT or CREATE_ENTRY. This way we test the flow is able to recover from an error.
So we should fix the patch and make sure it can pass and it creates sucessfuly
There was a problem hiding this comment.
The test I am missing is where you add the mock_config_entry and then try to setup the same device again. it should abort the flow
|
Aight, I pulled the code and I fixed the last things. You now will have translated fan modes, better tests (I removed the other example, I think airgradient isn't the best example anymore, but in that case I have 2 different devices and then we have something different to test, which is different from the same but different data) and a test for the device |
Breaking change
No breaking changes
Proposed change
New integration for Palazzetti stoves
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: