Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unique_id functionality. Required for further customization in HA UI #48

Merged
merged 4 commits into from
Sep 26, 2022

Conversation

oskargert
Copy link
Contributor

Hi,

First of all, thank you for putting this together after they removed the funtionality from HA.
I've used your component for a couple of months without any problem. But I have been a little annoyed with not being able to customize the devices in the HA UI, such as putting them into a room, giving them a "nice" name and changing the default icon. To do this you need to add a unique id to each device. Because of this I decided to clone your repo and add the unique id functionality.

custom_components/rpi_rf/switch.py Outdated Show resolved Hide resolved
custom_components/rpi_rf/switch.py Outdated Show resolved Hide resolved
@markvader
Copy link
Owner

I was thinking of this as well, I have a few thoughts.
I personally want all my entities to be configurable via UI so do appreciate the pull request.

  • Unique ID's such as these would not be accepted in the main project due to the ability to change the switches name in YAML after first setup. If a switch entity's name was changed, it's configuration settings would be lost.
  • https://developers.home-assistant.io/docs/entity_registry_index/ for good practice re: unique ID's
  • I do like how you have combined the name together with the switches on and off codes to create a unique ID. I was wondering what is the likelihood that a user could have two switches with the same on/off RF codes
  • Also it is possible to send a string of on and off codes (i.e. a list), this would need to be handled so that only the first code was used to generate a unique ID. Again this code can be later changed in YAML so it's configuration settings would be lost in that situation.

@markvader markvader self-assigned this Sep 26, 2022
@oskargert
Copy link
Contributor Author

Thank you for your feedback.

  • I understand your concern with including the switch name in the unique id. I have removed this.
  • In regards to the link to good practice, I would say that the on/off codes are the most unique values we have available?
  • I think the likelihood for a user adding two switches with the same codes is low, but not zero. We could possibly solve this by appending a UUID identifier, but then isn't the UUID enough by itself? My concern is also that a new ID would be generated for the switch on changes to the file or restart of the server. Do you know if this would be the case?
  • Would it be necessary to only use the first code? I don't see the problem with using the entire string? The changing of codes is a valid problem.

Could one solution be to document that a "manual" unique_id has to be defined in cases of using the same on/off codes for more than one switch and in the case of a switch with several codes?

In the case for a switch with several codes, the auto generation could also used simply be turned off, to force the user to define a manual unique_id?

@markvader markvader changed the base branch from main to unique_id_testing September 26, 2022 12:48
@markvader markvader merged commit e0cd87f into markvader:unique_id_testing Sep 26, 2022
@markvader
Copy link
Owner

Am merging into testing branch for now

@markvader
Copy link
Owner

  • In regards to the link to good practice, I would say that the on/off codes are the most unique values we have available?

agreed

  • I think the likelihood for a user adding two switches with the same codes is low, but not zero. We could possibly solve this by appending a UUID identifier, but then isn't the UUID enough by itself? My concern is also that a new ID would be generated for the switch on changes to the file or restart of the server. Do you know if this would be the case?

I don't believe that to be the case, but would need to do further reading of the documentation.

  • Would it be necessary to only use the first code? I don't see the problem with using the entire string?

True, will test it to be sure

Could one solution be to document that a "manual" unique_id has to be defined in cases of using the same on/off codes for more than one switch and in the case of a switch with several codes?

In the case for a switch with several codes, the auto generation could also used simply be turned off, to force the user to define a manual unique_id?

Ideally we'd convert the integration to a config flow (i.e. UI based rather than yaml).

Will test out a few scenarios and come back to you

@markvader
Copy link
Owner

Tested the following scenarios

  1. added an extra device to the configuration file with the same codes as a previous device, wouldn't expect it to work as it wouldn't be a unique_id.
    From the Log file
    Platform rpi_rf does not generate unique IDs. ID [1234567]_[7654321] already exists - ignoring switch.ORIGINALSWITCHNAME
    So works as expected

  2. Trying a device with multiple codes passed. Unique_id formed with these lists of codes [1234567,5896874,5678901]_[7654321,5675896,8745678]
    Works as expected

  3. Trying a device with multiple codes passed for on and a single code passed for off. Unique_id formed with these lists of codes [1234567,5896874,5678901]_[7654321]
    Works as expected

  4. Change one of the codes in YAML after original setup (original device was called switch.test_rf_device.
    From the Log file
    2022-09-26 17:26:49.039 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new switch.rpi_rf entity: switch.test_rf_device_2
    switch.test_rf_device shows in device list as "restored", as it knows it was discovered in the past but not currently available. This entity can be safely removed.

Was just thinking again, it is so unlikely that someone uses the more than one device in their home with the same RF code as that would be impossible to control a single device. If any issues do arise it will be easy enough to append a UUID to the unique_id.

I'll add documentation to readme about added additional functionality now (icon, location etc)

Thanks for the pull request @oskargert

@oskargert
Copy link
Contributor Author

Great, seems like the tests worked out as expected. I agree that this is an edge case and can be solved through documentation. I'm glad I could help and contribute a little. thank you for you a good and useful component!

@markvader
Copy link
Owner

I've published the new release. Have a read of the documentation and see if you think it could be improved.

Thanks again.

@oskargert
Copy link
Contributor Author

The documentation is good, my only comment would be that you could add the unique_id key to the configuration example and the options table, since it can be entered manually. A manually entered id would also fix the issue of keeping the same id when changing/adding the on/off codes.

@markvader
Copy link
Owner

The documentation is good, my only comment would be that you could add the unique_id key to the configuration example and the options table, since it can be entered manually. A manually entered id would also fix the issue of keeping the same id when changing/adding the on/off codes.

Thanks. Documentation updated commit

@oskargert
Copy link
Contributor Author

Perfect, thanks for including my feedback and letting me be a small part of your project

@markvader
Copy link
Owner

Very welcome, thanks for taking the lead on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants