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
Support songpal wireless-only soundbar identifiers #65330
Conversation
Hey there @rytilahti, @shenxn, mind taking a look at this pull request as it has been labeled with an integration ( |
3025fae
to
5d90215
Compare
As shown in home-assistant#64868, a number of newer models don't come wiht a macAddr attributes, so for those fall back to the wireless address. This could be hidden by the python-songpal library but for now this will make it possible to have multiple modern songpal devices on the same network.
5d90215
to
966ef5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Should we tag it for a patch release? |
I think it would be fine to be in a patch release. But I'm not entirely sure whether the fact that entities might go from "no unique id" to "have unique id" would complicate things. (The unique ID is only changed for entities that currently wouldn't have one, because they don't have a wired address — that's why it prefers the wired mac as unique identifier.) |
A possible complication would be if the user would roll back the update, the entity registry would still have the entry with the unique_id and not allow the entity without the unique_id to get its old entity_id but generate a new entity_id with a suffix. |
Is non-backwards compatibility a blocker here, and if yes, what is be the proper way to solve this? My personal opinion is that this is ready to go as the pros clearly outweigh the cons. Also, I don't think most of the other integrations do offer a working downgrade path either.. |
We should definitely merge this. It's just a question of do we include it in a patch release? |
Ah, sorry for being unclear. My previous comment was for getting this into the next patch release, but I'll ping on discord to get another opinion on that. |
Proposed change
As shown in #64868, a number of newer models don't come wiht a macAddr
attributes, so for those fall back to the wireless address.
This could be hidden by the python-songpal library but for now this will
make it possible to have multiple modern songpal devices on the same
network.
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: