Discovery of Miele temperature sensors #144585
Conversation
|
Hey there @astrandb, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| if ( | ||
| definition.description.value_fn(device) | ||
| in DISABLED_TEMP_ENTITIES | ||
| and definition.description.zone > 1 | ||
| ): | ||
| # All appliances supporting temperature have at least zone 1 (including core temperature) | ||
| # Don't create entity if API signals that datapoint is disabled (other zones) | ||
| continue |
There was a problem hiding this comment.
Disabled means permanently disabled? Or could this be "appliance turned off"?
From your PR description I read
In addition to this, once that the sensor is created and the appliance is turned off, reported value is "-327,68°C".
So will a zone 2 temperature be added when the appliance was turned off when HA started?
There was a problem hiding this comment.
The problem is that API is always reporting this disabled value for all the zones, even if they are not supported. The same value is used also when the appliance is off.
The problem before this PR was that temperature sensors did not appear if HA was started and the appliance was off in the meanwhile. Once that HA has started with the appliance on, the temperature sensor was created. After that, turning off the appliance resulted in this negative value instead of "unknown".
So this value is not a permanently disabled.
So will a zone 2 temperature be added when the appliance was turned off when HA started?
No. For zone 2 and 3, sensors are added only if the appliance is on when HA started.
It seems reasonable because zones 2 and 3 are supported only by refrigerators that are supposed to be continuously running. On the other side, we can't assume that all refrigerators have 3 zones, we would create useless sensors.
There was a problem hiding this comment.
I see. Since Those devices that support this are indeed 24/7 running I guess it's fine.
A nicer way would be to add entities dynamically once the API reports the key (with valid value).
There was a problem hiding this comment.
Yes, I agree. A nicer way would be to add entities dynamically once the API reports a valid value.
I've tried locally to implement this way (a refactoring of whole async_setup_entry is needed), but I have a side effect when HA is restarted after having created the entity and the device is reporting the disabled value: the entity becomes no longer provided by the integration until the temperature value appears. I could overcome this by looking at the device/entity registry to lookup if the entity has been already created in the past, but probably it's overkilling. What do you think?
There was a problem hiding this comment.
I would change the logic to check in the entity registry if the entity was registered before. This would mean that we now need to add logic that removes entities if their values are in that list. Like, it would be a strange side effect to only see entities once the device is on and HA is turned on. So ideally we avoid that and be consistent
There was a problem hiding this comment.
Ok, I'll add the logic that checks in the entity registry if the entity was registered before and I'll update the PR. It will take me some days to complete.
This would mean that we now need to add logic that removes entities if their values are in that list.
What do you mean?
There was a problem hiding this comment.
well, currently we are showing all 3, so if we now checked if that entity existed before, that is always true. Except for new users, so we should now have some code that deletes entities that we don't expect
There was a problem hiding this comment.
Mmmh, no currently we are showing temperature sensors only if HA starts when the API is reporting a valid value. So there is no way that now we are showing all them 3.
Just to give an example, my oven temperature sensor didn't exist and I had to start a program on my oven and reload the integration to make the sensor appear.
So, if a device is not supposed to use zones 2 and 3, sensors can't be created because there is no way that such zones report a valid value.
There was a problem hiding this comment.
ah cool, in that case we can just add them if they existed before. But I do think we need some way to add entities on runtime as you don't want to reload your integration at every major event
There was a problem hiding this comment.
Yes! It's exactly what I'm implementing 😀
I'll be back in a few days, thanks!
There was a problem hiding this comment.
Would it make sense to move the entity_class assignment to the description instead of doing it by logic in _async_add_new_devices()?
match definition.description.key:
case "state_status":
entity_class = MieleStatusSensor
case "state_program_id":
entity_class = MieleProgramIdSensor
...There was a problem hiding this comment.
This makes a lot of sense, but it requires moving around items inside sensor.py and it messes up the PR. I already prepared it on a separate branch and there will be a follow-up PR (already drafted: #145578)
…e a valid value and keep them if they are in entity registry
…perature sensors and for checking that temperature sensors are still provided after reloading integration
| @property | ||
| def unique_id(self) -> str: | ||
| """Return the unique ID of the entity.""" | ||
| return self.entity_description.get_unique_id(self._device_id) |
There was a problem hiding this comment.
Moved here from attribute in order to use the fn provided now in EntityDescription, in order to share creation of unique id with its recreation for looking up in entity registry.
|
|
||
| @property | ||
| def unique_id(self) -> str: | ||
| """Return the unique ID of the entity.""" | ||
| return f"{self._device_id}-{self.entity_description.key}-{self.entity_description.zone}" |
There was a problem hiding this comment.
Do you mean why it has been moved to property? The content of unique id did not change
There was a problem hiding this comment.
That's generally a reason to set it to a shorthand property
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| def _async_add_devices() -> None: | ||
| nonlocal added_devices, added_entities | ||
| entities: list = [] | ||
| entity_class: type[MieleSensor] | ||
| new_devices_set, current_devices = coordinator.async_add_devices(added_devices) | ||
| added_devices = current_devices | ||
|
|
||
| for device_id, device in coordinator.data.devices.items(): | ||
| for definition in SENSOR_TYPES: | ||
| if ( | ||
| device_id in new_devices_set | ||
| and device.device_type in definition.types | ||
| ): | ||
| match definition.description.key: | ||
| case "state_status": | ||
| entity_class = MieleStatusSensor | ||
| case "state_program_id": | ||
| entity_class = MieleProgramIdSensor | ||
| case "state_program_phase": | ||
| entity_class = MielePhaseSensor | ||
| case "state_plate_step": | ||
| entity_class = MielePlateSensor | ||
| case _: | ||
| entity_class = MieleSensor | ||
| if ( | ||
| definition.description.device_class | ||
| == SensorDeviceClass.TEMPERATURE | ||
| and definition.description.value_fn(device) | ||
| == DISABLED_TEMPERATURE / 100 | ||
| ) or ( | ||
| definition.description.key == "state_plate_step" | ||
| and definition.description.zone | ||
| > _get_plate_count(device.tech_type) | ||
| ): | ||
| # Don't create entity if API signals that datapoint is disabled | ||
| continue | ||
| entities.append( | ||
| entity_class(coordinator, device_id, definition.description) | ||
| # device is not supported, skip | ||
| if device.device_type not in definition.types: | ||
| continue | ||
|
|
||
| entity_class = _get_entity_class(definition) | ||
| unique_id = ( | ||
| definition.description.unique_id_fn( | ||
| device_id, definition.description |
There was a problem hiding this comment.
Okay so one thing is, we should only check this on creation, not every time we get an update, do we keep track of all data we already kept track off?
There was a problem hiding this comment.
we should only check this on creation, not every time we get an update
But in this way, I would recognize new temperature sensors only at creation. We need that the temperature sensor is created when it provides a valid value for the first time because of API limitations.
do we keep track of all data we already kept track off?
Sorry, I don't get the point here :)
There was a problem hiding this comment.
But you mentioned you don't know what values are available at boot right? But do we make sure we don't check every datapoint at every update?
There was a problem hiding this comment.
Yes, the API is always reporting something, valid or not valid value. Due to how the Miele API works, temperature sensors are not exposed at initial device discovery and only start reporting values later. So we need to re-evaluate their presence when the data becomes available.
The first time a temperature becomes valid, we need to create the sensor and keep it afterwards, reporting "unknown" as the API gets back to the invalid state (appliance is off or idle for example).
For this reason, at every update we need to check whether the sensor is enabled by looking at the value currently reported by API or in the registry.
This doesn't seem different than the previous implementation or I'm missing something.
There was a problem hiding this comment.
I'm adding some details to be more clear.
The check for entity_registry is only performed:
- for newly discovered devices, or
- for newly available sensors that previously returned a disabled/null value, only for device types that potentially support zones 2 / 3 / core temp (so we will skip the check on washing machines, because it doesn't have different temperatures and it is stated into the sensor definition)
To avoid checking all datapoints every time, we track both:
added_devices: only process new devices onceadded_entities: so we don't try to add the same sensor multiple times
This ensures we don’t process already-added sensors again, and we skip entirely the devices already handled.
Open to suggestions on improving performance :) What about an explicit check for value_fn(device) transitioning from invalid → valid to tighten this further?
Proposed change
Currently, temperature sensors are discovered only if the appliance is running when HA starts. In addition to this, once that the sensor has been created created and the appliance is then turned off, instead of reporting "unknown", it says "-327,68°C". Some temperature zone sensors are not set.
Proposed changes:
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
Update miele.markdown for describing how temperature sensors are created home-assistant.io#39139
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: