home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
71.53k stars 29.91k forks source link

0.32 broke media_player.yamaha #4226

Closed Kernald closed 7 years ago

Kernald commented 7 years ago

Home Assistant release (hass --version): 0.32

Python release (python3 --version): Don't know exactly, I'm using the docker images

Component/platform: media_player.yamaha

Description of problem: Upgrading to 0.32 from 0.31.1, the yamaha component fails to initialize

Problem-relevant configuration.yaml entries and steps to reproduce:

- platform: yamaha
  host: rx-v579
  name: AVR
  source_ignore:
    - "JUKE"
    - "AUX"
    - "AV1"
    - "AV2"
    - "AirPlay"
    - "Bluetooth"
    - "HDMI3"
    - "HDMI4"
    - "HDMI5"
    - "HDMI6"
    - "MusicCast Link"
    - "NET RADIO"
    - "SERVER"
    - "Spotify"
    - "TUNER"
    - "USB"
    - "iPod (USB)"
  source_names:
    AV3: "CD"
    AV4: "TV"
    AV5: "Vinyl"
    AV6: "SNES"
    HDMI1: "Shield"
    HDMI2: "PC"

Traceback (if applicable):

16-11-05 20:34:18 ERROR (MainThread) [homeassistant.components.media_player] Error while setting up platform yamaha
Traceback (most recent call last):
  File "/usr/src/app/homeassistant/helpers/entity_component.py", line 148, in _async_setup_platform
    entity_platform.add_entities, discovery_info
  File "uvloop/future.pyx", line 230, in __iter__ (uvloop/loop.c:94692)
  File "uvloop/task.pyx", line 186, in uvloop.loop.BaseTask._fast_wakeup (uvloop/loop.c:100074)
  File "uvloop/future.pyx", line 101, in uvloop.loop.BaseFuture._result_impl (uvloop/loop.c:93110)
  File "/usr/local/lib/python3.5/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/src/app/homeassistant/components/media_player/yamaha.py", line 82, in setup_platform
    YamahaDevice(name, receiver, source_ignore, source_names)])
  File "/usr/src/app/homeassistant/components/media_player/yamaha.py", line 101, in __init__
    self.update()
  File "/usr/src/app/homeassistant/components/media_player/yamaha.py", line 107, in update
    self._play_status = self._receiver.play_status()
  File "/usr/local/lib/python3.5/site-packages/rxv/rxv.py", line 238, in play_status
    res = self._request('GET', request_text, zone_cmd=False)
  File "/usr/local/lib/python3.5/site-packages/rxv/rxv.py", line 97, in _request
    response = ET.XML(res.content)
  File "/usr/local/lib/python3.5/xml/etree/ElementTree.py", line 1334, in XML
    return parser.close()
  File "<string>", line None
xml.etree.ElementTree.ParseError: no element found: line 1, column 0

Additional info: The exact same configuration works perfectly fine on 0.31.1.

sdague commented 7 years ago

cc: @postlund

postlund commented 7 years ago

I will look into this as soon as I can, thanks you for reporting 👍

jwl17330536 commented 7 years ago

It works for me, but I do get an error. I can't see that it is actually stopping any functionality that I use though.

Nov 05 19:01:36 automation hass[17899]: INFO:homeassistant.loader:Loaded media_player.plex from homeassistant.components.media_player.plex
Nov 05 19:01:38 automation hass[17899]: ERROR:homeassistant.components.media_player:Error while setting up platform yamaha
Nov 05 19:01:38 automation hass[17899]: Traceback (most recent call last):
Nov 05 19:01:38 automation hass[17899]: File "/srv/hass/hass_venv/lib/python3.4/site-packages/homeassistant/helpers/entity_component.py", line 148, in _async_setup_platform
Nov 05 19:01:38 automation hass[17899]: entity_platform.add_entities, discovery_info
Nov 05 19:01:38 automation hass[17899]: File "/usr/lib/python3.4/asyncio/futures.py", line 388, in __iter__
Nov 05 19:01:38 automation hass[17899]: yield self  # This tells Task to wait for completion.
Nov 05 19:01:38 automation hass[17899]: File "/usr/lib/python3.4/asyncio/tasks.py", line 286, in _wakeup
Nov 05 19:01:38 automation hass[17899]: value = future.result()
Nov 05 19:01:38 automation hass[17899]: File "/usr/lib/python3.4/asyncio/futures.py", line 277, in result
Nov 05 19:01:38 automation hass[17899]: raise self._exception
Nov 05 19:01:38 automation hass[17899]: File "/usr/lib/python3.4/concurrent/futures/thread.py", line 54, in run
Nov 05 19:01:38 automation hass[17899]: result = self.fn(*self.args, **self.kwargs)
Nov 05 19:01:38 automation hass[17899]: File "/srv/hass/hass_venv/lib/python3.4/site-packages/homeassistant/components/media_player/yamaha.py", line 80, in setup_platform
Nov 05 19:01:38 automation hass[17899]: if receiver.zone not in zone_ignore:
Nov 05 19:01:38 automation hass[17899]: TypeError: argument of type 'NoneType' is not iterable

Here is my configuration if at all helpful:

media_player:
  - platform: yamaha
    name: 'Family Room Receiver'
    source_ignore:
      - "AUDIO1"
      - "AUDIO2"
      - "AV1"
      - "AV2"
      - "AV3"
      - "AV5"
      - "AV6"
      - "HDMI1"
      - "HDMI2"
      - "HDMI3"
      - "HDMI4"
      - "HDMI5"
      - "Pandora"
      - "Rhapsody"
      - "SERVER"
      - "SiriusXM"
      - "Spotify"
      - "TUNER"
      - "USB"
      - "V-AUX"
      - "iPod (USB)"
      - "NET RADIO"
    source_names:
      AV4: "TV"
Kernald commented 7 years ago

With nearly the same configuration (the main difference I can see is I specified an host, I'm not relying on discovery), I don't have the same stacktrace, and the entity isn't instantiated at all on my end. I'll try later today without the hard-coded host.

Kernald commented 7 years ago

Update: I have the same error as above without specifying the host attribute. Same goes for 0.32.2 (seems pretty logical looking at the changes).

sdague commented 7 years ago

Ok, there are 2 issues here. The play_info one is it's own thing, the zone_ignore is because optional voluptuous attributes don't actually set a default. The zone_ignore is easy to fix. I don't know about the other.

sdague commented 7 years ago

Out of curiosity, for folks where zone_ignore fails, do you have > 1 media_player component?

sdague commented 7 years ago

The above fixed the zone_ignore issue, for the original issue, I'm not really sure what the issue is. We'd probably need a dump of the XML that is failing to parse to see what's up with it.

postlund commented 7 years ago

Good, I see there's some progress. The initial problem is because the receiver does not support the command so we should not use play_status. Not sure how to make a clean implementation, but I will think about it.

sdague commented 7 years ago

@postlund we can discover that from the desc.xml I think. It also probably makes sense to make rxv be extremely robust on all it's method calls, even in the face of bad xml.

postlund commented 7 years ago

Maybe we can, not sure. Should be easy enough to check though. Would be good to have a proper strategy in rxv for presenting/verifying support for different things. Since functionality is quite limited in rxv, adding is_x_supported-methods for things that are not supported by all receivers might be the easiest and cleanest solution. But it will explode if we add more functionality down the road. Another way is to just throw NotImplementedError and let the user take action (this is what we can do with the current version and I will provide a fix so dev works as expected).

I would not say that it's bad xml in this case, more lack of it. But yeah, better handling of received data would be nice.

Kernald commented 7 years ago

Is there any way I can fetch the desc.xml from my receiver?

postlund commented 7 years ago

Sure, just visit/download http:///YamahaRemoteControl/desc.xml

postlund commented 7 years ago

Argh, github... insert IP-address of receiver at the right place.

Kernald commented 7 years ago

Wow, I didn't except such a big file. Would any specific part (or the whole file) help for this issue?

sdague commented 7 years ago

No, we need the whole thing. Yamaha basically has a full API discovery system in there, so every function and parameter supported in the receiver is there. I would suggest pasting into a gist somewhere. I started building some testing in the rxv project that can use some of these to figure out platform differences.

postlund commented 7 years ago

@sdague: Which approach for "exposing" available support are you aiming for in rxv?

Kernald commented 7 years ago

Here it is: https://gist.github.com/Kernald/f172d0ca44dd85ef74af41245e38b27e

postlund commented 7 years ago

I've made some sort of fix for the current problem, feel free to try it out.

Kernald commented 7 years ago

I'm using docker so I won't be able to test easily, I'll set up a test instance on your branch and keep you posted. I'll probably won't be able to do this today, though.

sdague commented 7 years ago

@Kernald before we merge the work around, it would be good to know that it fixed things for you. It could just get us to another issue with the rxv support of that receiver, and if the issue is deeper than this work around I'd rather figure out how to fix it in the library for real.

sdague commented 7 years ago

@Kernald any idea which input your stereo was on when this happened?

Kernald commented 7 years ago

Sure, it was HDMI 1.

sdague commented 7 years ago

HDMI1 makes total sense. The update call is doing the play_status on update regardless of input. Which is definitely not going to work on HDMI inputs.

So, I actually think - https://github.com/wuub/rxv/pull/28/commits/82b380609e6171770f6a77359c607995457cffea (upstream fix in rxv) is probably a better fix than #4281 for this, because that makes the library behave more consistently here, and keeps the logic out of HA when possible.

@Kernald any chance you can pull that branch and try that upgraded library? If it works I'll see if we can get rxv released and bump the dependencies in HA for the next release.

Kernald commented 7 years ago

Sure, I should be home in a couple hours :-)

Le lun. 7 nov. 2016 21:01, Sean Dague notifications@github.com a écrit :

HDMI1 makes total sense. The update call is doing the play_status on update regardless of input. Which is definitely not going to work on HDMI inputs.

So, I actually think - wuub/rxv@82b3806 https://github.com/wuub/rxv/pull/28/commits/82b380609e6171770f6a77359c607995457cffea (upstream fix in rxv) is probably a better fix than #4281 https://github.com/home-assistant/home-assistant/pull/4281 for this, because that makes the library behave more consistently here, and keeps the logic out of HA when possible.

@Kernald https://github.com/Kernald any chance you can pull that branch and try that upgraded library? If it works I'll see if we can get rxv released and bump the dependencies in HA for the next release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/home-assistant/home-assistant/issues/4226#issuecomment-258945808, or mute the thread https://github.com/notifications/unsubscribe-auth/AASHiLRWlyQQeUwkI7pJhoQEdDz0DvAfks5q74OGgaJpZM4KqXRx .

Kernald commented 7 years ago

@sdague I can confirm your fix in rxv works fine. Thanks :+1:

By the way, a work-around for anyone encountering this issue: restarting hass on another input (it works for me on AV5 for example) allows the entity to be instantiated, then it's totally usable (with the same stacktrace being logged anytime I'm using HDMI1).

sdague commented 7 years ago

@Kernald ok, rxv fix is merged upstream. Once we get a release I'll propose an HA req bump.

Kernald commented 7 years ago

Great, thanks!