irrigation_unlimited
irrigation_unlimited copied to clipboard
rgc99
Implement stable entity id's for controllers and zones
Is your feature request related to a problem? Please describe.
Yes. The current irrigation_unlimited implementation (v.2022.3.0) generates automatically the entity id's for controllers and zones, which are based on the position of the element in the yaml configuration file, e.g. binary_sensor.irrigation_unlimited_c1_z1. This has two drawbacks:
(1) does not allow for user-friendly entity id's, and (2) implies that the entity id's will change every time a controller or zone is removed or relocated inside the config file.
Both are very inconvenient and error-prone if you have multiple controllers and zones, because these id's are used in many other places like UI definition files and need to be changed accordingly.
Describe the solution you'd like
A solution for the problems above would be to allow for stable, user-defined id's for controllers and zones. One possible way would be as follows:
-
Accept an optional configuration variable named controller_id in the controller definition with a default value of cN, where N is the controller index.
-
Accept an optional configuration variable named zone_id in the zone definition with default value of zN, where N is the zone index. It is worth noting that this zone_id already exists as a config variable, but is currently not being loaded from the config file because the config.get instruction refers to CONF_ID ("id") instead of CONF_ZONE_ID ("zone_id"); this seems to be a bug.
-
Generate the entity_id by combining the previous variables, e.t. binary_sensor.irrigation_unlimited_gardenctrl_northzone
Describe alternatives you've considered
No simple alternative was identified to solve the problems described above.
Additional context
I have implemented the solution described above on top of irrigation_unlimited 2022.3.0. It does not have a lot of changes and it works as expected with HA 2022.4.3. I will be happy to contribute the code changes to the project if it is deemed useful, but I am not familiar with the testing framework and therefore I cannot provide any tests. I also have not updated the usage documentation, but I am willing to do that if these changes are merged into the component.
Additional suggestions
Miscellaneous notes on collateral issues, that I took when researching this topic:
-
As stated above at point 2 of the solution description, there seems to be a bug needing correction, related to zone_id configuration.
-
The service_enable functions under IUZone and IUController would in my view be best renamed to service_call of something of the sort, to better reflect their true semantics (they are not only used to enable but also to disable and toggle).
-
The domain name "irrigation_unlimited", when used in combination with meaningful (and therefore longer) controller and zone id's tends to generate very long names, which sometimes do not fit in HA screens; this can be quite annoying. I would suggest to change this domain name to something shorter and, by the way, maybe not referencing irrigation, considering that this component provides generic sequencing and timing functionality that goes far beyond watering applications; I myself intend to use it for controlling house lights too.
The changes seem reasonable. The controller_id and zone_id parameters should be restricted to certain characters. HA validates entity_id's against the regex expression r"^(\w+).(\w+)$" but it seems convention that only lower case, numbers and underscore characters are used. Anything else is likely to have undesirable consequences. The underscore should be avoided as it is used as a separator between controller and zone ids. The validation can probably be done in the schema to limit the ids to alphanumerics and at least 1 character.
Additional suggestions
- This is indeed a bug. It should be loading 'zone_id' instead of 'id'. The parameter is used as a link to the 'zone_id' within the sequence zones. The sequence zone_id can be a list to trigger multiple zones (one to many).
- True, I think it used to be 3 functions but got consolidated into one but the name remained, such is development. Please rename as you see fit.
- The domain name is a bit of a mouth full. A bit of work renaming everything and certainly a major breaking change but probably necessary to take it beyond just the garden. Would probably need to make the icons customisable to switches and lights rather than valves and sprinklers. An icon map in the controller section would probably do the trick.
This will break some of the existing cards if users choose to use the new features so there will be a bit more work involved.
Please send in a pull request.
Thank you for your answer. I just created a pull request to fix the bug described in number 1 under "Additional suggestions". It is a minimalistic change, just to make sure I am doing it right, since I am not familiar with GitHub pull requests. Others will follow, but first I will have to come back to you to check that some implementation details are fine with you. Regarding my suggestion number 3 about renaming the domain, of course it needs some thinking and cannot be done overnight, but I am happy to know that you agree in principle.
Ok, I just sent another minimalistic change, renaming internal functions as described in number 1 under "Additional suggestions".
Here are the details of the implementation of the zone_id and controller_id config parameters. Please let me know your thoughts about them.
controller_id property in controllers
Not supported in current implementation. Will be taken from the controller_id config variable, defaulting to "cN" where N is the controller index number.
zone_id config property in zones
Current implementation defaults to a string of the form "N" (N = index number) when no zone_id value is specified in the configuration. I implemented it to default to "zN" for two reasons: (1) consistency with the current form of entity_id which ends in "cN_zM", and (2) to avoid visual confusion since it is HA practice to automatically resolving entity_id conflicts by appending a "_N" suffix.
This, however, may break compatibility with current references to zone_id's from other objects, which might be solved by interpreting the zone_id as an index if it is a numeric value. This workaround might be a temporary solution, in which case it should write a deprecation message to the log.
Property unique_id for controllers and zones
Current implementation has the form "c1_z1" for zones, which would generalize to "controllerid_zoneid", which I find prone to conflict with other entities. Since it is intended to be unique, I think it should be more distinctive, something like "binary_sensor.irrigation_unlimited_controllerid_zoneid". Note, however, that I might be misunderstanding the intention of the unique_id property; I find the documentation about it quite confusing.
Same for controllers, using a suffix of "_m" instead of the zone.
Property entity_id for controllers and zones
Current implementation has the form "binary_sensor.irrigation_unlimited_c1_z1" for zones, which would generalize to "binary_sensor.irrigation_unlimited_controllerid_zoneid". This looks fine but maybe too verbose. I am not aware of any HA standards about this, but from what I have seen in other implementations I think that entity_id's tend to be simpler, e.g. a friendly name of "Kitchen 1" for a switch may result in an entity_id like "switch.kitchen_1". Since it is intended to be used only as a reference, I think that dropping the "irrigation_unlimited" part would be appropriate. Again, let me say that I my understanding could be wrong, and you might know better.
Same for controllers, using a suffix of "_m" instead of the zone.
Not sure about possible side effects that changes in unique_id and entity_id might have, e. g. loss of entity states history.
** Validation of zone_id and controller_id **
I took your validation criteria for zone_id and controller_id on board by validating both parameters on the schema as alphanumeric starting with non-digit, with regex "^[a-z][a-z0-9]*$". I checked manually that it works, but I have included no automated test. I wonder, however, if rejecting underscores is really necessary.
zone_id config properties
This parameter is currently used (now fixed) to link to sequence zones. Rather than a breaking change or trying to do something funky with it, how about calling it something different? I know zone_id is probably a logical choice but what about zone_code, zone_entity, zone_ref or something creative. The controller part can follow suit.
Property unique_id for controllers and zones
I think you are right in that it should be the full form like you see now as the entity_id to avoid conflicts. Can be anything so long as it is guaranteed to be system unique. Seems to be just consumed internally in HA. Some docs https://www.home-assistant.io/faq/unique_id/.
Property entity_id for controllers and zones
This is the key to finding everything in the system. My experience when registering the entity is if it is not unique then HA will append a suffix. Here are some issues with changing the entity_id.
- Zombies with the old entity_id.
- Loss of history as the entity_id is the key to storage and access.
- Cards, automations, scripts and so on will need to be updated.
Changing the entity_id won't break the integration but will break everything else.
Validation of zone_id and controller_id
The original thinking behind <platform>.<domain>_cN_zN is that users can get creative with scripts and templating and easily pull it apart. I am amazed with what they come up with but is possible because of a well known id. The underscores act as separators and allowing them in the name would create ambiguities.
HA is a big toolbox. Allowing others to easily build upon your work is in my opinion paramount. Food for thought.
Hi again. My remarks:
zone_id config properties
This parameter is currently used (now fixed) to link to sequence zones. Rather than a breaking change or trying to do something funky with it, how about calling it something different? I know zone_id is probably a logical choice but what about zone_code, zone_entity, zone_ref or something creative. The controller part can follow suit.
I'm not sure I can follow here. There is no breaking change, or maybe we have a different understanding about the meaning of "breaking". Every current configuration would still work, with the parameters defined with the current convention (zone_id = index) being flagged by deprecation messages in the log. After a reasonable time (maybe mid-2023) the index convention would be removed from the code.
In fact I would even go a step further and make the controller_id and zone_id parameters mandatory in the long run, starting with a deprecation message for the time being. The whole point of this change is to make the configuration more robust by using stable (and meaningful) id's for controllers and zones, and id's based on the definition order are neither stable nor meaningful; I have experienced some trouble after inadvertently turning zone "3" into zone "4" after inserting a new one, and my plants were not happy about it. That would never have happened to zone "backyardroses".
I understand your concern about forcing users to change the configuration. Nobody likes that, and neither do I, but assuming that the deprecation messages are clear and meaningful, and that sufficient time is given to implement the changes, I believe that this is both common practice and a small price to pay for more robustness.
In line with that, I see little value in defining new parameters for the alphanumeric id's and keeping the current indexed ones as they are. Besides, having two alternate ways to refer to a zone would need additional validations (either zone_id or zone_ref must be present but not both). Not that this would be extremely complex, but I find it superfluous. Much better in my view to keep just zone_id / controller_id.
Property unique_id for controllers and zones
We're in agreement here, so this is settled.
Property entity_id for controllers and zones
I think we can settle this one too by returning for entity_id the same value as for unique_id.
Validation of zone_id and controller_id
Not fully convinced about this one, but you may know better about user creativity, and in any case it's a minor point on which I don't have a strong opinion. We can settle it on your proposal.
In summary, I think there remains only one decision to be made. You're the project master, so it's your take.
zone_id config properties
I think we are talking much the same thing to a large degree. Sorry, I am probably going over things again but to recap.
The zone_id currently links the IUZone and IUSequenceZone objects together. Using the index + 1 as a default in the IUZone allows users to quickly get started when creating a sequence. They will undoubtedly realise the benefits of defining their own zone_id's when a new zone is inserted and breaks the sequences. The zone_id is a string and because it is used internally can be almost anything you like. If the zone_id is to be used in place of the zN in the entity_id then some restrictions are required as character like '$%&.,' are sure to cause grief in HA. Sticking to lower case characters and numerals is probably safe (two minds about the underscore). As the zone_id has in reality never been able to be changed, enforcing restrictions at this point won't break anything. I guess it comes down to documentation on what impact changing the zone_id will have; sequences, entities, cards, automations, scripts etc. Suppose I was considering an internal and external id but like you say, too complex.
It would be good to have some pre-flight checks in place. Don't think this can be done in the schema so probably on startup with messages. 1) Duplicate id's (zones only need to be unique within a controller), it would be a crap shoot otherwise. Can see this happening when a user copies a block and doesn't change the id. 2) Broken/missing links or orphaned objects. Small typos would be picked up.
Not convinced about making controller_id/zone_id mandatory. Settings should have reasonable defaults so they work out of the box with little or no setup. Taking this further the current implementation should have a single controller and zone ready to go on installation. Once started, people can learn and grow it by themselves. Most users are not programmers and are quickly frustrated setting up more than about three settings. Totally get a robust config but must have a good user experience all the same. More on this later.
Hello again. Let's see if we can agree on the following points and close the issue:
controller_id property in controllers
Not supported in current implementation. Will be taken from the controller_id config variable, defaulting to "N" where N is the controller one-based index number.
zone_id config property in zones
Will be taken from the zone_id config variable, defaulting to "N" where N is the zone one-based index number.
Property unique_id for controllers and zones
Will be composed as "binary_sensor.irrigation_unlimited_c<controller_id>_z<zone_id>" for the zones, and "binary_sensor.irrigation_unlimited_c<controller_id>_m" for the controller.
Property entity_id for controllers and zones
Will have the same value as unique_id above. Note that controller_id and zone_id are preceded by "c" and "z" so that the entity_id stays the same as in the current implementation.
Validations
zone_id and controller_id will be optional, and if present must be lower-case alphanumeric starting with alphabetic char.
I agree that more checks can be added to catch eventual configuration mistakes: eventual duplicates are a new thing, but broken links can already happen now. Checks for both can be added as an enhancement at some later point.
Good to hear from you. Let's move forward on this. Please note the work that has been done on zone_id validation. It should be straight forward use this as a template for controller_id validation and duplication detection. See irrigation_unlimited.py (2848) check_links()
One a side note, I have been working on a companion card. Still a bit rough around the edges but functional and suitable as a beta, give it a go if you like. It is zero configuration but relies on well known entity ids like ..._c1_z1 etc. There will need to be some discovery mechanism employed to remain this way. Initially I thought the underscore could be reserved and used to map out the structure needed but this would be a bit limited and crude. I have a couple of other ideas in mind.
Validations
The zone_id is now being validated for snake_case compliance with the exception that the first character can be numeric. This was relaxed to allow compatibility with the existing 1's based index system. The check is performed in code and logged as a message although it could be made mandatory by the schema.
Duplicates Duplicate zone_id's are detected and reported.
Orphans Sequence zones that do have a corresponding zone reference are reported.
I just created a pull request implementing the changes as described in my previous message.
Validations for zone_id and controller_id are performed in the schema. Both are optional, but if specified must be alpanumeric with first char alphabetic. It must be noted that when not specified they will be assigned a string value containing the one-based index for that controller/zone in the definition.
I think that this is not consistent with the new validation in check_links() that you referred to. I have been more restrictive because I thought you wanted to rule out underscores, but feel free to change the validation regex as you see fit. In any case I think the validation in code is superfluous now.
The duplicates and orphans checks will certainly give an additional level of robustness. Thank you for implementing them.
Other issues
Your remark about the companion card sounds interesting; the current way to define the user interface introduces a lot of redundancy. Can you provide some indications on how to use it?
On another side note I would like to come back to your suggestion about making the icons customisable, which I think would be useful. If you agree, my suggestion would be to allow a different icon set for each zone and controller, chosen from a map of icon sets defined at coordinator level.
The PR looks fine but we will need another change. This came up on another ticket which was subsequently self solved but they posted their configuration in which they had used the zone_id links. I think we have to assume it is being used. What I don't want is users upgrading and suddenly having automations, cards etc. suddenly broken. You tend not to be very popular and is poor practice. Another reason is it would be good to make a conscious decision and understanding what will happen before turning this feature on. Finally a new user playing with the configuration probably doesn't need the side effect of the entities changing while they get on top of zones and sequences.
Perhaps add in the IRRIGATION_SCHEMA a new parameter something like rename_entities which will default to false. The documentation will clearly state this is a danger zone parameter and the likely side effects. Switch it on when you are ready and understand. Also gives a quick back out option by turning it off in case there are unintended consequences.
Other issues
The companion card has just been added to the HACS default repository. I have also just resolved a deployment issue. Version 2022.4.0 is broken so don't even bother, 2022.4.1 should work. Just search for 'irrigation' in HACS and install. The card should appear in the picker. Nothing to setup, just a name if you want.
Might be best to create a new ticket on this one but this is my thoughts on customising the icons. The controller and zone has 4 icons. Might as well throw in sequences and sequence zones with 4 each. Here is a summary:
on: (controller/zone/sequence/sequence zone) "mdi:water"/"mdi:valve-open"/"mdi:play-circle-outline"/"mdi:play-circle-outline"
off: (controller/zone/sequence/sequence zone) "mdi:water-off"/"mdi:valve-closed"/"mdi:stop-circle-outline"/"mdi:stop-circle-outline"
disabled: (controller/zone/sequence/sequence zone) "mdi:circle-off-outline"
paused: (controller) "mdi:pause-circle-outline"
blocked: (zone/sequence/sequence zone) "mdi:alert-octagon-outline"
Note they are simply strings from the Material Design library, HA does all the work - choose from thousands of icons. Perhaps create a ICON_SCHEMA and include it into the various schemas as optional. Anything missing defaults to the existing.
controllers:
- name: Controller 1
icons:
on: mdi:water
off: mdi:water-off
disabled: mdi:circle-off-outline
paused: mdi:pause-circle-outline
zones:
- name: Zone 1
icons:
on: mdi:valve-open
off: mdi:valve-closed
disabled: mdi:circle-off-outline
blocked: mdi:alert-octagon-outline
Maybe also include it into the all_zones_config to avoid repetition.
Not sure I understand the problem with the zone_id's. Maybe I'm missing something, but I don't see how the introduction of the zone_id parameter, as it is implemented, can break old configurations. Or maybe you are referring to the change in the entity_id's; then your are right because they are used at least in the UI definition, if you have one. Then I think a parameter in the irrigation schema, as you propose, would be appropriate. I would go for a less cryptic name than rename_entities, which may not be meaningful for to the average user; something like entities_naming_convention (=short|long) or something of the sort would be easier to understand.
Again, I think that the new naming option should be recommended not only in the documentation but also by a message in the log if it is not used, since it is likely to create conflicts with entities defined elsewhere.
Other issues
Thank you for the info on the companion card: I will check when possible.
On the icons issue, I think that having to specify all icons for every zone will lead to very verbose configurations, even if there is a default. But you are right, this would best be discussed as a new issue.
Digging a little more on the difference between entity_id and unique_id. This gives some insight https://developers.home-assistant.io/docs/entity_registry_index/. The current unique_id is correct, it should not include the platform or the domain and more importantly it should be immutable. The unique_id needs to stand as is. Only the entity_id will be changeable.
Cherry picked my way through the PR. Added duplicate checks for the controller, test units etc. I think it satisfies your needs and my concerns. Turn it on with rename_entities in the main section (change it if you want). Updating the documentation at the moment but please test when you get a chance.
Hello again,
Sorry for disappearing so abruptly; for unexpected reasons I have been kept completely out of touch with this matter.
Glad to know that you could use some of the changes I implemented. I'm pretty sure that any eventual problems with them should already have been reported by other users, but I'll have a check anyway as soon as I can catch up with all the changes in the component and in HA.
Hopefully I will be able to spend some time in the coming weeks in my controller project, which sadly I have to completely abandon for some time. One of the things I wanted to try when I had to leave was the companion card you mentioned, but my attempts at installing it have been so far unsucessful. Searching for 'irrigation' on HACS I can only find Irrigation Unlimited and another unrelated component. Is the companion card still available?
Good to hear from you. The companion card is still available if you go into HACS -> Frontend (not Integrations) and search for 'irrigation' it should show up.
Thank you for the tip. You guessed right, I was looking for the component under Integrations. The installation process didn't seem to work though. I followed the instructions that were shown after it, but to no avail. I will try to solve the issue, and if I can't I will be opening a separate issue; I think this one would best be closed.