Add support for externally connected utility devices in HomeWizard #100684
Conversation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Will this fix also work for water meters? (just checking as you only seem to talk about gas meters in the code) |
| class HomeWizardExternalSensorEntityIdentifier(HomeWizardEntity, SensorEntity): | ||
| """Representation of externally connected HomeWizard Sensor.""" | ||
|
|
||
| _attr_icon = "mdi:alphabetical-variant" | ||
| _attr_entity_category = EntityCategory.DIAGNOSTIC | ||
| _attr_translation_key = "meter_identifier" | ||
|
|
||
| _attr_name = None | ||
|
|
||
| def __init__( | ||
| self, | ||
| coordinator: HWEnergyDeviceUpdateCoordinator, | ||
| device_info: DeviceInfo, | ||
| device_unique_id: str, | ||
| ) -> None: | ||
| """Initialize Externally connected HomeWizard Sensors.""" | ||
| super().__init__(coordinator) | ||
| self._attr_unique_id = f"{DOMAIN}_{device_unique_id}_meter_identifier" | ||
| self._attr_native_value = device_unique_id | ||
| self._attr_device_info = device_info |
There was a problem hiding this comment.
I have the feeling this can be easier.. not sure how
| device_info = DeviceInfo( | ||
| identifiers={(DOMAIN, unique_id)}, | ||
| name=description.key, | ||
| manufacturer="HomeWizard", | ||
| model=coordinator.data.device.product_type, | ||
| ) | ||
| if coordinator.data.device.serial is not None: | ||
| device_info[ATTR_VIA_DEVICE] = ( | ||
| DOMAIN, | ||
| coordinator.data.device.serial, | ||
| ) |
There was a problem hiding this comment.
This is an copy what is found in entity.py, but different. Not sure how and if we can improve that.
core/homeassistant/components/homewizard/entity.py
Lines 20 to 30 in cab3008
| identifiers={(DOMAIN, unique_id)}, | ||
| name=description.device_name, | ||
| manufacturer="HomeWizard", | ||
| model=coordinator.data.device.product_type, |
There was a problem hiding this comment.
Would this be enough instead of the sensor with this information?
| model=coordinator.data.device.product_type, | |
| model=coordinator.data.device.product_type, | |
| serial_number=unique_id, |
There was a problem hiding this comment.
We can remove the sensor and that will be enough IMO. But as it removes the sensor, which is migrated from the original configuration, we should mark it as breaking I think.
There was a problem hiding this comment.
@emontnemery Is the breaking-change label still required or will it be picked up by the PR description?
| async def async_setup_entry( | ||
| hass: HomeAssistant, entry: ConfigEntry, async_add_entities: AddEntitiesCallback | ||
| ) -> None: | ||
| """Initialize sensors.""" | ||
| coordinator: HWEnergyDeviceUpdateCoordinator = hass.data[DOMAIN][entry.entry_id] | ||
| async_add_entities( | ||
|
|
||
| # Migrate original gas meter sensor to ExternalDevice |
There was a problem hiding this comment.
Please add code which removes the now defunct gas_unique_id sensor, otherwise users will still see it, but unavailable
There was a problem hiding this comment.
Good point! Can you hint me what function to use for that or an integration that does this?
There was a problem hiding this comment.
Sure, here's an example
core/homeassistant/components/accuweather/__init__.py
Lines 52 to 58 in 95fdae5
There was a problem hiding this comment.
Nice, added removal and test.
|
Thanks for the quick end-sprint 👊 |
| """Validate unit of measurement and set device class.""" | ||
| if ( | ||
| self.native_unit_of_measurement | ||
| not in DEVICE_CLASS_UNITS[self._suggested_device_class] |
There was a problem hiding this comment.
I'm not sold on this approch. Is there really that many different possible native units of this device per suggested device class that we can't create entity descriptions for each combination? Then we could handle special cases like m3 above already in the mapping.
There was a problem hiding this comment.
It is up to the device manufacturer which unit to use and how to format that. Nothing about that is specified in any standard AFAIK. The current method supports all combination that are allowed by Home Assistant.
When we specify each combination we have to collect those combinations. That is absolutely an option. In the beginning we will see some issues pop up but when most combinations are added it works the way we hope. But I think the end result is less 'clean' as it is now.
There was a problem hiding this comment.
Even if we would include all allowed native units in the list of combinations it would only result in 23 items currently. That's just 18 more than what we have now. If it would become more than a couple of hundred combinations I'd understand that we don't want to list all of them.
It's always easier to read a declarative approach than a calculated approach.
Breaking change
The Gas meter id sensor is removed, the meter ID is now as serial number in the device info panel.
Proposed change
Continuing from #86386. Was not able to (easily) rebase many upstream changes**
Many smart meters receive data from other meters, such as gas or water meters. So far this integration expects that exactly 1 gas meter can be connected (or nothing at all), but this is not true. In some countries water meters are connected. It is also possible to have several gas meters connected at the same time.
This change exposes all connected meters as separate devices.
Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: