Added unique_id property back to Yamaha media_player #12373
Conversation
re-added the unique_id property because the component no longer finds zones beyond the main zone. The inherited unique_id from MediaPlayerDevice does not return a unique id for each zone.
homeassistant
added
platform: media_player.yamaha
small-pr
| def unique_id(self): | ||
| """Return an unique ID.""" | ||
| return '{0}:{1}'.format(self.receiver.ctrl_url, self._zone) | ||
|
|
There was a problem hiding this comment.
Is self._zone a user-configurable string (i.e., something that could change through, say, the Yamaha app)? The objective is to not have anything user-configurable in the unique ID that could change down the road while still representing the same thing.
There was a problem hiding this comment.
It does not appear to, it appears as if it comes from rxv.RXV() call which returns a receiver object. After tracing through the rxv code, it appears to grab the self._zone from the xml that is requested from the device. When I get home, I can verify this.
There was a problem hiding this comment.
I'll have to check the yamaha app as well.
There was a problem hiding this comment.
I'm guessing the reason it was removed was that it's based on the URL. A unique ID needs to be based on something that won't change for the device, such as a serial number or mac address. I'd see if something like that is available from the API that could be used instead.
There was a problem hiding this comment.
That makes sense. For time constraints, I only 'rolled back' the changes. When i get home I can find a better way to handle the unique_id. I may have been too hasty with this PR.
There was a problem hiding this comment.
self._zone is not user configurable, it comes from the device.
As far as I can see in both the rxv code, and poking at the XML API for the Yamaha unit, there is nothing available at the rxv layer for uniqueness. That's all operating at the url level (which there was already code to ignore by url devices previously discovered).
The only existing path I'm seeing to get an actual unique id is data that's available in the upnp discovery packet. We don't require everyone to go through that path, so that wouldn't work if discovery was disabled. I don't see any imperative path to get that.
Newer receivers have a more REST like API, but for those of us with older ones losing multizone because there is not a guarunteed way to generate stable uuid would be really sad.
There was a problem hiding this comment.
I'd try and investigate the underlying bug. Not having a unique ID shouldn't affect the behavior of the actual platform code at all.
Petro31
changed the title
|
Note: |
|
Where is Yamaha using unique ID? Unique ID was a property used internally by Home Assistant. By removing it, Home Assistant is actually enforcing less rules. How can this break things? |
|
Okay, I see it now. The problem is that the default unique ID returns Currently we don't accept URLs as unique IDs because it is user configurable. Getting the MAC address or serial number is preferred. I suggest that you rename the property to something else than |
|
This is an alternate fix. We don't actually need unique_id to be right for yamaha, we just need something to handle dedup - #12382 |
|
Should I close this if the plan is to move forward with @sdague PR? |
|
I think it should auto close after merge because of the commit message. @balloob is going to hotfix it into the 0.63.x stream. |
ghost
added
re-added the unique_id property because the component no longer finds zones beyond the main zone. The inherited unique_id from MediaPlayerDevice does not return a unique id for each zone.
Description:
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass