vpathuis / ultraheat

MIT License
5 stars 3 forks source link

Add support for Landis+Gyr UltraHeat T550 #8

Closed blomqvist closed 1 year ago

blomqvist commented 1 year ago

This will hopefully fix #7

Tests seems to check out OK. Perhaps a final test against the UH50 is needed, as I've changed the probing code (\x2F\x3F\x21\x0D\x0A was sent twice, the first time with an extra ).

I did split the extra "fixes" in separate commits, so they can be cherry-picked or left out.

vpathuis commented 1 year ago

This is awesome, I'll have a look at this soon!

vpathuis commented 1 year ago

I've quickly tested this with the UH50, but it gives some errors. It also thinks I got a LGUHT550 now :-P I'll look into that. I'm very enthousiastic about making this compatible for the T550!

I agree with adding a new output variable for MWh (as opposed to converting it). The package is a pure interface and the intelligence is done in Home Assistant. I'm assuming you're using HA? Since HA uses MWh, I can have the integration tap into the MWh variable directly if availble.

I'm not so sure though about interpreting a blank model as T550... Strange that you don't get a model .. I've seen this with my UH50 when using the wrong baudrate or timing issues. Perhaps you could try and see if there is some variation in which it does give back the model? If it really won't give it, I think I prefer to just have no model name and see if it can continue. Theoretically there could be other models out there, but being UH50 or T550. As long as the relevant measurement values (including MWh) are given, things will be okay.

blomqvist commented 1 year ago

Great feedback, @vpathuis! Found the issue. As you guessed, it was timing.

So, I've changed timeout=1 to timeout=5. I left other things mostly untouched (interface, and exposing MWh, I can make a separate commit for that).

My T550 produces /LUGCUH50, while your UH50 seems to produce !LUGCUH50. So, I made an educated model guess based on that. But I suppose we don't need the model check if we always also run the regex for MWh and just populate it if it exists…

vpathuis commented 1 year ago

Ah great to hear that the timeout did it! And how weird that it gives /LUGCUH50 as model number. This would seem to be a bug? Perhaps there are other firmware versions that do give the right model number, so I think it's still wise not to depend on the model name for any conclusions, or logic. So I agree with the agnostic commit. I have to change some things in my setup to test this properly. And probably have to figure out how to add commits to this PR as well (git-noob, busted).

vpathuis commented 1 year ago

Alright, so far so good: HeatMeterResponse(model='LUGCUH50', heat_usage_gj=331.848, heat_usage_mwh=None, volume_usage_m3=3387.03, ownership_number='66153690', volume_previous_year_m3=3188.07, heat_previous_year_gj=314.658, heat_previous_year_mwh=None, error_number='0', device_number='66153690', measurement_period_minutes=60, power_max_kw=22.4, power_max_previous_year_kw=22.4, flowrate_max_m3ph=0.744, flow_temperature_max_c=98.5, flowrate_max_previous_year_m3ph=0.744, return_temperature_max_c=96.1, flow_temperature_max_previous_year_c=98.5, return_temperature_max_previous_year_c=96.1, operating_hours=111301, fault_hours=5, fault_hours_previous_year=5, yearly_set_day='01-01 00:00', monthly_set_day='01 00:00', meter_date_time=datetime.datetime(2022, 10, 4, 20, 33, 35), measuring_range_m3ph=1.5, settings_and_firmware='0 1 0 0000 CECV CECV 1 5.16 5.16 F 101008 040404 08 0', flow_hours=29006)

vpathuis commented 1 year ago

Works perfect. And thanks for the code improvements. I'll merge and make a release tomorrow.

vpathuis commented 1 year ago

@blomqvist https://github.com/home-assistant/core/pull/79666

vpathuis commented 1 year ago

@blomqvist if you want, you can download and try https://github.com/vpathuis/core/tree/add-ultraheat-mwh/homeassistant/components/landisgyr_heat_meter as a custom component. If all works well, I'll submit it for review after the ultraheat version bump. Don't forget to set the ultraheat version to 0.5.0 in the manifest file first, see https://github.com/vpathuis/core/blob/landisgyr-ultraheat-0-5-0/homeassistant/components/landisgyr_heat_meter/manifest.json

blomqvist commented 1 year ago

@vpathuis

I figured out how to test it, and it seems to work!

Had to make some minor modifications in sensor.py. At least I think I had to make them...

I added three isinstance(prop, float) checks (two replaces is not None checks).

"""Platform for sensor integration."""
from __future__ import annotations

from dataclasses import asdict
import logging

from homeassistant.components.sensor import (
    ATTR_STATE_CLASS,
    RestoreSensor,
    SensorDeviceClass,
)
from homeassistant.config_entries import ConfigEntry
from homeassistant.const import ATTR_DEVICE_CLASS, ATTR_UNIT_OF_MEASUREMENT
from homeassistant.core import HomeAssistant
from homeassistant.helpers.entity import DeviceInfo
from homeassistant.helpers.entity_platform import AddEntitiesCallback
from homeassistant.helpers.update_coordinator import CoordinatorEntity
from homeassistant.util import dt as dt_util

from . import DOMAIN
from .const import GJ_TO_MWH, HEAT_METER_SENSOR_TYPES

_LOGGER = logging.getLogger(__name__)

async def async_setup_entry(
    hass: HomeAssistant, entry: ConfigEntry, async_add_entities: AddEntitiesCallback
) -> None:
    """Set up the sensor platform."""
    _LOGGER.info("The Landis+Gyr Heat Meter sensor platform is being set up!")

    unique_id = entry.entry_id
    coordinator = hass.data[DOMAIN][entry.entry_id]

    model = entry.data["model"]

    device = DeviceInfo(
        identifiers={(DOMAIN, unique_id)},
        manufacturer="Landis & Gyr",
        model=model,
        name="Landis+Gyr Heat Meter",
    )

    sensors = []

    for description in HEAT_METER_SENSOR_TYPES:
        sensors.append(HeatMeterSensor(coordinator, unique_id, description, device))

    async_add_entities(sensors)

class HeatMeterSensor(CoordinatorEntity, RestoreSensor):
    """Representation of a Sensor."""

    def __init__(self, coordinator, unique_id, description, device):
        """Set up the sensor with the initial values."""
        super().__init__(coordinator)
        self.key = description.key
        self._attr_unique_id = f"{DOMAIN}_{unique_id}_{description.key}"
        self._attr_name = "Heat Meter " + description.name
        if hasattr(description, "icon"):
            self._attr_icon = description.icon
        if hasattr(description, "entity_category"):
            self._attr_entity_category = description.entity_category
        if hasattr(description, ATTR_STATE_CLASS):
            self._attr_state_class = description.state_class
        if hasattr(description, ATTR_DEVICE_CLASS):
            self._attr_device_class = description.device_class
        if hasattr(description, ATTR_UNIT_OF_MEASUREMENT):
            self._attr_native_unit_of_measurement = (
                description.native_unit_of_measurement
            )
        self._attr_device_info = device
        self._attr_should_poll = bool(self.key in ("heat_usage", "heat_previous_year"))

    async def async_added_to_hass(self) -> None:
        """Call when entity about to be added to hass."""
        await super().async_added_to_hass()
        state = await self.async_get_last_sensor_data()
        if state:
            self._attr_native_value = state.native_value

    def _handle_coordinator_update(self) -> None:
        """Handle updated data from the coordinator."""
        if self.key in asdict(self.coordinator.data):
            if self.device_class == SensorDeviceClass.TIMESTAMP:
                self._attr_native_value = dt_util.as_utc(
                    asdict(self.coordinator.data)[self.key]
                )
            else:
                self._attr_native_value = asdict(self.coordinator.data)[self.key]

        # Some models will supply MWh directly. If not, GJ will be converted to MWh.
        if self.key == "heat_usage":
            if (
                hasattr(self.coordinator.data, "heat_usage_mwh")
                and isinstance(self.coordinator.data.heat_usage_mwh, float)
            ):
                self._attr_native_value = self.coordinator.data.heat_usage_mwh
            else:
                self._attr_native_value = convert_gj_to_mwh(
                    self.coordinator.data.heat_usage_gj
                )

        if self.key == "heat_previous_year":
            if (
                hasattr(self.coordinator.data, "heat_previous_year_mwh")
                and isinstance(self.coordinator.data.heat_previous_year_mwh, float)
            ):
                self._attr_native_value = self.coordinator.data.heat_previous_year_mwh
            else:
                self._attr_native_value = convert_gj_to_mwh(
                    self.coordinator.data.heat_previous_year_gj
                )

        self.async_write_ha_state()

def convert_gj_to_mwh(gigajoule) -> float:
    if isinstance(gigajoule, float):
        """Convert GJ to MWh using the conversion value."""
        return round(gigajoule * GJ_TO_MWH, 5)

How it looks in HA:

bild
vpathuis commented 1 year ago

That's great news! Thanks for testing this.

The suggestions for isinstance work for me as well, although I'm thinking they shouldn't make a difference (and it doesn't when I test against the dummy file). What happens without the changes? Any errors you could share? Although ... it doesn't hurt to just change this.

The third change, the one in convert_gj_to_mwh, that should not be touched in your case, since if all goes well, there is no translation done if the heat meter supplies the MWh-value. I'm inclined not to add this, as I'd rather have the exception occur if the conversion is actually called with something else than a float (there is no "else" here).

vpathuis commented 1 year ago

I've gone ahead and submitted this: https://github.com/home-assistant/core/pull/80346 If you encounter any issues, please let me know.