Update PyVicare to 2.13.0 #57700 8000
Update PyVicare to 2.13.0 #57700
Conversation
With PyViCare 2.8.1 a breaking change was introduced which required changes on sensor and binary_sensor platforms. See Home Assistant Core PR home-assistant/core#57700 As an intermediate step the circuit ID config option is still present but ignored. Now all circuits are supported at the same time. As a 2nd step the option will be removed completely since the integration will introduce setup via the UI (see this PR: home-assistant/core#56691 )
|
Thanks, @oischinger, this looks mostly good, but I added a few comments. It's not clear to me if the breaking change in the library is a breaking change for a Home Assistant users once this PR is merged? |
Thanks for pointing that out. For most users this change should be unnoticeable but a few more complex installations will get new entity names to avoid duplicate entities. |
With PyViCare 2.8.1 a breaking change was introduced which required changes on sensor and binary_sensor platforms: - Circuit, Burner and Compressor have been separated out from the "main" device - Multiple circuits and burners allow "duplicate sensors". We add the circuit or burner number as suffix now At the same time the sensors are now created only when available: During entity creation we can check if the value is provided for the user's device. Sensors are not created by heating type anymore but instead the new API structure is reflected, providing device, burner or circuit sensors. For details of breaking changes from PyViCare 1.x to 2.x please see https://github.com/somm15/PyViCare#breaking-changes-in-version-2x
c18b3d4 to
4ebcaa5
Compare
|
Rebase was necessary to resolve conflicts |
|
@emontnemery Could you please review again? Should be straight forward I hope |
| if len(iterables) > 1: | ||
| suffix = f" {current.id}" |
There was a problem hiding this comment.
Since you only add a number for the 2nd+ entities, wouldn't that mean that this is not a breaking change and the original entity is never called _1?
There was a problem hiding this comment.
No because iterables are the circuits on the pyVicare API. I only add the suffix if there are more than a single circuit.
I considered not adding a suffix for the first curcuit but:
- it would atill be breaking for users who previously configured a circuit ID
- it makes it a bit harder to distinguish which sensor is for which circuit
| def unique_id(self): | ||
| """Return a unique ID.""" | ||
| return f"{self._api.service.id}-{self.entity_description.key}" | ||
| """Return unique ID for this device.""" | ||
| return f"{self._device_config.getConfig().serial}-{self._attr_name}" |
There was a problem hiding this comment.
Unique IDs must be truly unique and never change, so I don't think this is a good change. If we do this it will not be possible to change the names, either the name configured by the user or the friendly names in entity descriptions, without either breaking things or having to add a migration step.
So something like this is probably better:
f"{self._device_config.getConfig().serial}-{self.entity_description.key}-{suffix}"Also, the unique ID guarantees the entity ID is not changed, even if the name changes.
The unique id is also never seen by the user, it's just used as a key to map unique ID to a stable entity ID.
Hence, I think you should really try to not break the existing unique IDs, which would address balloob's comment too.
If _api.service.id doesn't return the same as self._device_config.getConfig().serial, is it really necessary to change?
If not necessary, only add a suffix when needed.
There was a problem hiding this comment.
The problem here is that self._api.service.id is no more available on the API. It also was a bad choice initially since this value could change if Viessmann decides so.
Serial number however should never change and uniquely identify your device, so I hope it's a better choice.
I agree that attr_name is not good. I'd suggest to change it to
"{self._device_config.getConfig().serial}-{self.entity_description.key}-{self.api.id}"
so it seems we will have a breaking change here for all users. What I don't understand is why it wasn't a problem for me (or any of the other people who already tested it). Maybe because the entity names were identical and I never renamed an entity?
There was a problem hiding this comment.
Here's my proposal:
616482d
There was a problem hiding this comment.
What I don't understand is why it wasn't a problem for me (or any of the other people who already tested it). Maybe because the entity names were identical and I never renamed an entity
That does seem strange indeed, can you explain your test in some more detail?
If an entity has a unique_id, it will be added to the entity registry which will then reserve the entity_id, and ensure the entity always gets the same entity_id regardless of if the name changes.
Hence, when you change unique_id the entity_id should also change, even if the name is the same.
You could check what happens by comparing .storage/core.entity_registry before and after the upgrade.
The entity registry allows updating the unique_id by passing a new unique_id to async_update_entity for a previously reserved entity_id, that would allow us to make this a non-breaking change if the self._api.service.id can be deduced,
but maybe that's not possible.
The proposal looks good IMO.
There was a problem hiding this comment.
@emontnemery you are right. My recent commit 616482d lead to exactly that situation.
The thing is: climate, water_heater and sensors did not have a unique_id before that PR. Only binary_sensor had it. Since my system provided not binary sensor at all for some time (in the meantime Viessmann reports more data again on my API and I do also have binary_sensors) I did not notice anything.
I believe however it won't be possible to migrate the existing unique_ids (for binary_sensor) to the new and better unique_id since one of the inputs for the unique ID doesn't exist anymore.
Anyway: I expect it to be a minor issue since only binary_sensors are affected. I will however notice that in the breaking change section.
Additionally the breaking change is there anyway due to the newly added support for multiple circuits/burners.
I believe this is the best we can do to do the migration to the new PyVicare version.
The unique ids shall not depend on the name but on the entity description key (which should not change) and the id of the circuit, burner or device.
|
@emontnemery or @balloob Summing up this will be a breaking change for 1. binary sensors |
|
@oischinger Let's merge it since it doesn't seem feasible to avoid the breaking change. |
| name = hass.data[DOMAIN][VICARE_NAME] | ||
| api = hass.data[DOMAIN][VICARE_API] | ||
|
|
||
| all_devices = [] |
There was a problem hiding this comment.
Please name this something entities instead of devices.
This PR was split out of #56691 to separate the library update from the "setup via UI" PR
Breaking change
With PyViCare 2.8.1 a breaking change was introduced which required changes on sensor and binary_sensor platforms:
Circuit, Burner and Compressor have been separated out from the "main" device
Multiple circuits and burners allow "duplicate sensors". We add the circuit or burner number as suffix now
Most users should not see a difference but complex installations with more than one circuit and/or burner will produce multiple entities now, suffixed with their burner or entity IDs.
E.g. if you have two circuits instead of a single climate.vicare entity you will see two entities:
This will give you more control but requires you to adapt your existing automations to the new entity names.
Same accounts for sensors, binary_sensors and water_heaters.
Also note that binary sensors might be created twice since the unique ids for those entities has changed. Please delete the old disabled binary_sensors in case you encounter this.
Proposed change
With PyViCare 2.8.1 a breaking change was introduced which required changes on sensor and binary_sensor platforms:
At the same time the sensors are now created only when available:
During entity creation we can check if the value is provided for the user's device.
Sensors are not created by heating type anymore but instead the new API structure is reflected, providing device, burner or circuit sensors.
For details of breaking changes from PyViCare 1.x to 2.x please see https://github.com/somm15/PyViCare#breaking-changes-in-version-2x
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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: