Refactoring to use Entity Descriptors and fixing hold functionality

[ocrease]

Hi @MindrustUK, this is a rather large PR based on the work I've been doing for the hold functionality of NeoStat climate entities, NeoStats working in Timer mode and NeoPlugs (not tested since I don't have any). I have refactored everything to use EntityDescriptors. Let me try to explain everything that I have done:

• I have added a coordinator.py which extracts the coordinator logic into its own class. Logic was previously in __init__.py
• I have added a base HeatmiserNeoEntity in entity.py and a base HeatmiserNeoEntityDescription. All of the entities previously had duplicated logic for the device info, unique id, availability etc. The base class now has this logic and the entity descriptor provides ways of customizing the entity were necessary (most of the quality default integrations seem to use the EntityDescriptor model)
• I got rid of number.py. Number entities are used to report writable numbers from a device. The three that existed in the dev branch for hold minutes, hold hours and hold temperature were for read only values from the devices. There are sensors (in sensors.py) for these values instead
• I got rid of switch.py. There were switches related to hold, standby and (broken?) neo plug functionality. All of these have been replaced with features elsewhere
• Added binary_sensor.py. Most of these were previously defined in sensors.py, but that is not the right way to do it. Entities should be defined in the correct file for their platform. Binary sensors rewritten to make use of the base entity and descriptor
  • Moving the binary sensor for the contact sensor to this file means that the device class now works correctly and the state should be shown as Open / Closed (Door sensor not reporting to HA #165)
• button.py rewritten to make use of the base entity and descriptor
• climate.py
  • rewritten to make use of the base entity and descriptor
  • added support for presets: Home, Away, Boost
    • Home is standard operation
    • Away sets the away mode on the hub (and therefore affects all entities). You get standby by setting the mode instead (say from heat to off)
    • Boost sets a hold at a temperature of 2 degrees higher than target for 30 minutes. Custom temperatures/durations have to be done via the service call
  • Improved hold service schema validation
• Added select.py - NeoStats in timeclock mode and NeoPlugs are now modelled as select entities
  • NeoPlugs have states Auto, Override On/Off (eg override the schedule/profile), On/Off (timer disabled and manually set to on or off) (should resolve Bug: NeoPlug functionality is broken in Dev and Version 1.5 #194)
  • NeoStats in timeclock mode have Auto, Override On/Off and Standby (Feature: Re-implementing Auto / Override | Hold / Standby - Old Title: NeoStat in Timer Mode in V1.5 branch #182). Note that away/holiday is separate
  • Updated strings.json and translation file to support the select entities
• sensor.py rewritten to make use of the base entity and descriptor and removed binary_sensor entities
• services.yaml added the actual state to hold on the timer hold service (eg so we can override to off)
• The base entity class has _attr_has_entity_name = True and the entity descriptors do one of three things HA Docs
  • If it is the main feature of a device, the name property is set to None. HA then uses the device name only for this entity. Examples are the climate entities themselves, the neostats in timer mode, the neoplugs, the contact sensors and the temperature sensor (as I write this, I think I may need to change some of this, see below)
  • Alternatively the name property can be omitted from the descriptor if the device_class is set. Things like battery, connectivity, the identify button do this. HA will set the name to the device name + a standard name based on the device class
  • Finally, if a name property is provided, the device name + name property is used (but in the UI when looking at a device, HA removes the device name since it is implicit
• Some sensors are hidden by default, things like away/holiday which really should be on the hub and floor temperature / current temperature
• Floor temperature and current temperature are set up with state_class=SensorStateClass.MEASUREMENT so long term statistics should work (Feature: Add support for statistics / long term statistics on data returned by integration #158)
• All the files I modified are formatted with the standard home assistant format (ruff check --fix and ruff format). Very few errors remain on those files. This should help Feature: Official core integration? #60 is still an aim, though of course lots of other things remain
  • If you are interested in how I'm doing this, let me know (basically followed the devcontainer instructions in the HA developer docs, then cloned this repo into a development folder and did a symlink to add heatmiserneo to the custom_components in the config directory.

Regarding the entity naming, I know you were working round some issues with naming and that's how you ended up with the long names. Could you elaborate what the issues were? I do suspect the door contact, external thermostat sensors and maybe neoplugs need to be explicitly named, but could you attach the output of the new diagnostics functionality to see what the data looks like? Also note that the entities can always be renamed by the user, so the default just needs to be sensible.

Here are some screenshots to accompany the above:

Neo Stat previously (with the long names and number entities)

Neo Stat now (notice sensible naming, mode on the climate entity (Home) and various hidden sensors)

Neo Stat in timer mode (notice the select dropdown. Also "Device Temperature". My neostat reports the temperature even though its not used by the timer. Might be useful for someone, so I added it as a diagnostic sensor disabled by default)

Climate card now has a preset mode selector

Renaming a sensor

Also note I haven't implemented anything to change the default time / temperature increment for the hold service. We need to explore changes to the config flow to support this, but I think that should be done separately

[ocrease]

Regarding the extra floor temperature sensor, I see there was a discussion in #89 talking about whether it should be a separate sensor or an attribute and the decision was to use an attribute instead. Looking at the entity docs, this sounds like a mistake. It says:

Warning

Entities that generate a significant amount of state changes can quickly increase the size of the database when the extra_state_attributes also change frequently. Minimize the number of extra_state_attributes for these entities by removing non-critical attributes or creating additional sensor entities.

Also, having it as sensor means that you can add it to long term statistics for example. I've added it as a disabled entity (and its shouldn't be added to a remote air sensor), so I guess users can choose. Having said that, I don't have any floor sensors connected to my stats, so if you want to remove the floor temperature sensor I'm happy to delete it

[MindrustUK]

@ocrease Just had a brief run through, all really good! Thanks very much.

I've only spotted a few minor things, when I get some time I'll give this a proper review. I'm assuming you've essentially spent most of your evenings and weekend on this by the looks of the volume of code!

Apologies that it's going to take me a little longer to review (read: probably Thursday / Friday by the time I get a proper eyes on due to real world stuff). I'll try and dip into the review if I get a minute before then.

[ocrease]

@MindrustUK No worries. I realise it's quite a big PR! In the meantime, if you can download diagnostics for your environment (assuming you have more than just NeoStat V2s like I do) I might be able to spot any issues

[MindrustUK]

@ocrease

config_entry-heatmiserneo-01JBBYB1WMEZYMXKHBGFAQQA72.zip

If you see any unusual names like swimming pool or anything else, for clarity I don't have a swimming pool I just wish I did :) So don't expect the test devices to necessarily be deployed in the way you'd expect. I'll hook up my NeoHub V1 and an underfloor temperature probe to one of the stats when I get some time.

Also of note the Heatmiser Neo HC and Neo Air 1 are actively broken, I'm avoiding putting any work into fixing them until I address: #78 where I want to refactor the product list to be a dictionary that contains attributes such as "HAS_BATTERY" or "HAS_TEMP_RANGE" etc. This should make it easier to evaluate when adding devices rather than "If device_model in [1,3,5,7]" etc.

[MindrustUK]

PS: Just realised my Kitchen and bathroom both use in floor probes, so these may do the job for whatever testing was needed with externals (I forget what it was).

PPS: The NeoAir V1 has three modes of operation, I've got it in Mode 1 at the minute.

[ocrease]

@MindrustUK thanks, the diagnostics data should be very helpful, but... I suspect the diagnostics download has a bug and not all the data is being downloaded. I think devices are grouped by zone name and my original implementation put the device in a dict so only one device per zone is being returned. I'll raise a PR (hopefully later today) to resolve that so that you can download the diagnostics again.

In the meantime, maybe don't spend too much time reviewing this PR.

[ocrease]

@MindrustUK would you be able to add a few screenshots from the heatmiser app for the NeoAir and Neostat HC? It would help to have an idea of how Heatmiser intends them to be used.

For the NeoAir, I think these are reported as two separate devices, one for the thermostat and one for the timer. Can you put each half on standby separately from the app? Or if you put the timer on standby the thermostat also goes on standby (and viceversa)?

[MindrustUK]

@ocrease

Home Assistant Screenshots:

NeoAir:

NeoStatHC

The NeoAir behaves 1:1 in mode 1 as any NeoStat V2 or V1 behaves but with increased latency due nature of the reduced polling time as to reduce power consumption. The NeoAir has no onboard switching, i.e. no relay and no external probe inputs. From the manual operation modes 1&3 are thermostatic modes, mode 2 is a timeclock mode.

The NeostatHC is a different beast all together. Configurable fans, Heating Coils etc. Electrically it just appears be five normally open relays instead of one.

Let me know if you have any more requirements and I'll see what I can gather up.

[MindrustUK]

RE: For the NeoAir, I think these are reported as two separate devices, one for the thermostat and one for the timer. Can you put each half on standby separately from the app? Or if you put the timer on standby the thermostat also goes on standby (and viceversa)?

I think this is mode 3, I've not set this mode up yet. Let's get mode 1 working, then I can try experimenting with modes 2 and 3.

[ocrease]

@MindrustUK Ok thanks, I think you can review / test when you get a chance

[MindrustUK]

@ocrease Looks like I crashed into a missing lib for some reason

    from propcache import cached_property
ModuleNotFoundError: No module named 'propcache'```

[ocrease]

@MindrustUK I've been developing against the dev branch of Home Assistant and it looks like there is a change coming which is not on the released version yet home-assistant/core#127339

I made a change to use the previous option. Can you give it a try?

[MindrustUK]

A quick smoke test and everything looks good. As previous will be a bit of time before I test everything properly and finish review but it all looks good right now.

[ocrease]

@MindrustUK great, of course take your time.

Based on the logs provided in #202, it looks like the NeoAir Thermostat and Timer devices are reported completely separate, even with different serial numbers. I think I'll revert a couple of changes I made thinking that they would all be under the same device in HA

[ocrease]

@MindrustUK changes made

[MindrustUK]

@ocrease Looks like we've got a bug on import

File "/config/custom_components/heatmiserneo/binary_sensor.py", line 22, in <module>
    from .const import (
ImportError: cannot import name 'HEATMISER_TYPE_IDS_HOLD' from 'custom_components.heatmiserneo.const' (/config/custom_components/heatmiserneo/const.py)

[MindrustUK]

@ocrease you can ignore the above, missed a commit when rolling forwards.

[MindrustUK]

@ocrease Early feedback

Failed to perform the action select/select_option. 'NeoStat' object has no attribute 'data' when changing Neoplug states.

[MindrustUK]

I'll need to setup a test plan for this I think, to ensure that every feature and configuration gets hit. I'll try and find some time to look into this.

[ocrease]

@MindrustUK the bug with the NeoPlug should be fixed. I also removed the "Identify" button from the accessories

[MindrustUK]

@ocrease other than a few my question on string substitution (https://peps.python.org/pep-0498/ I believe). I think everything is good enough to merge and get more eyes on.

The NeoStat-HC is not quite working as expected but let's deal with that as a separate issue in terms of adding support for a "new" device.

[ocrease]

@MindrustUK The string substitution changes are following these guidelines in the Home Assistant developer docs. As you can see f-strings are the preferred method, with the exception being logging. With log statements, if the log level is disabled (say debug), then by using the old style formatting the string substitution can be skipped altogether. With f-string the string substitution would be done and then the new string discarded.

The actual ruff error code is G004

Running "ruff check *.py" on the dev branch results in 415 errors. Running it on this branch its 36, 33 of which are in the config_flow.py which I haven't touched.

Did you check the behaviour of the plug, contact sensor and air sensors?

[MindrustUK]

@ocrease Interesting... answers my question on the f-string front. All good on that one.

Plug seems all good as does the contact and air sensors. Thanks again for all your work.

Merging...