Use RestoreSensor in Ecowitt #134442
Use RestoreSensor in Ecowitt #134442
Conversation
|
Hey there @pvizeli, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There are no tests for this component, yet. Except of the config flow. So that would mean I have to setup all the basic tests? |
Nope, then it's fine. But tests are always welcome :) |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Robert Resch <robert@resch.dev>
…5/core into pr/Thomas55555/134442
| @@ -33,6 +33,15 @@ async def handle_webhook( | |||
| hass, DOMAIN, entry.title, entry.data[CONF_WEBHOOK_ID], handle_webhook | |||
| ) | |||
|
|
|||
| def async_update_device_config(sensor: EcoWittSensor) -> None: | |||
| """Update device config.""" | |||
| hass.config_entries.async_update_entry( | |||
There was a problem hiding this comment.
We should not update on any sensor the config entry if the station is the same
| """Update device config.""" | ||
| hass.config_entries.async_update_entry( | ||
| entry, | ||
| data={**entry.data, "station": sensor.station}, |
There was a problem hiding this comment.
Is it possible to have multiple devices on the same webhook? If yes this solution will not work
| self.restored_last_reported = sensor_last_state.last_reported | ||
|
|
||
| if (sensor_data := await self.async_get_last_sensor_data()) is not None: | ||
| self._attr_native_value = sensor_data.native_value |
There was a problem hiding this comment.
| self._attr_native_value = sensor_data.native_value |
Will not be used as you override the base native_value property
| station = EcoWittStation( | ||
| **entry.data["station"], | ||
| ) | ||
| sensor = ecowitt.sensors[f"{config_entry_unique_id}.{sensor_key_name}"] = ( | ||
| EcoWittSensor( | ||
| name=config_entry.original_name, | ||
| key=config_entry_key, | ||
| stype=sensor_key_name, # type: ignore[arg-type] | ||
| station=station, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This should be done in the library to reduce errors, such as when you forget to add the sensor to the EcoWittListener.
In your PR the sensor will be created EcoWittSensor.
Creating them here is hacky
|
|
||
| if (sensor_data := await self.async_get_last_sensor_data()) is F438 not None: | ||
| self._attr_native_value = sensor_data.native_value | ||
| self._attr_available = True |
There was a problem hiding this comment.
Same for this one
| self._attr_available = True |
| async def check_availability(self) -> None: | ||
| """Return whether the state is based on actual reading from device.""" | ||
| if not self.restored_last_reported: | ||
| self._attr_available = False |
There was a problem hiding this comment.
| self._attr_available = False |
| await asyncio.sleep(MAX_AGE - age.seconds) | ||
| new_age = dt_util.utcnow() - self.restored_last_reported | ||
| if new_age.seconds >= MAX_AGE: | ||
| self._attr_available = False |
There was a problem hiding this comment.
This will not work as you have overwritten the available property
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Breaking change
Proposed change
Use a RestoreSensor in EcoWitt. This assures that there is sensor data available until the first data from the webhook arrives.
Entities are built out of the entity registry. To manage this, we need to store some data in the config entry. A migration of the config entry in in my opinion not required.
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: