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
73.47k stars 30.69k forks source link

Panasonic Viera: state always reported as on after restart of Home Assistant #34894

Closed rschaeuble closed 4 years ago

rschaeuble commented 4 years ago

The problem

After restarting Home Assistant, the state of my Panasonic Viera (model TX-55EXW604S) is always reported as ON.

This is occuring since upgrading to 0.109. In 0.107 (I skipped 0.108), the state was correct after restart.

Environment

Problem-relevant configuration.yaml

No entires in configuration.yaml. This is the relevant part from core.config_entries:

{
                "connection_class": "local_poll",
                "data": {
                    "host": "xxx.xxx.xxx.xxx",
                    "name": "wohnzimmer_fernsehgeraet",
                    "port": 55000,
                    "turn_on_action": null
                },
                "domain": "panasonic_viera",
                "entry_id": "XXXXXXXXX",
                "options": {},
                "source": "user",
                "system_options": {
                    "disable_new_entities": false
                },
                "title": "wohnzimmer_fernsehgeraet",
                "unique_id": "xxx.xxx.xxx.xxx",
                "version": 1
 }

Traceback/Error logs

There are no log entires.

Additional information

I have an (unverified) theory what's going on.

Version 0.107: https://github.com/home-assistant/core/blob/d520a02b8c41b073476a615a582990a1b8e4ceff/homeassistant/components/panasonic_viera/media_player.py#L125-L132 The update method catches exceptions, setting the state to OFF when it can't fetch volume etc. from the TV.

Version 0.109: https://github.com/home-assistant/core/blob/405062d2df06c8e3861fd0010104d628306235df/homeassistant/components/panasonic_viera/media_player.py#L244-L250 The _update method (which seems to to be the replacement for the old update method) does not catch exceptions. As a result, the state stays set to ON (which is the assumed initial state).

probot-home-assistant[bot] commented 4 years ago

Hey there @joogps, mind taking a look at this issue as its been labeled with a integration (panasonic_viera) you are listed as a codeowner for? Thanks! (message by CodeOwnersMention)

joogps commented 4 years ago

@rschaeuble that could be what’s going on. However, _update does catch errors (it’s called inside async_update using a helper to catch errors). Did you try to manually update the entity (using homeassistant.update_entity) to see if the problem is that async_update is never called?

Later I’ll investigate into this ;)

rschaeuble commented 4 years ago

@joogps: I just tried homeassistant.update_entity; however, that did not correctly update the state.

On my quick glance of the code, I did indeed miss the helper that handles the exception. Also, async_turn_off invokes async_update, which invokes _update. So if the error were in _update, the state would get reset to ON even after turning off the TV.

However, that's not what I'm seeing. Once the state is correct (when I restart Home Assistant while the TV is on), it stays correct when turning it on and off through Home Assistant.

Well, it's quite late here, so I might have missed another important detail in the code. If you need me to try out something to help you find the issue, I'll gladly do so tomorrow.

joogps commented 4 years ago

Ok! Thank you! (also, good night 😂)

lolouk44 commented 4 years ago

not sure if related to same issue, if not let me know I'll create a new issue, but since 0.109, the state of my Panny (TX58DX700) is also incorrect. If I switch it off from HA, state goes to off, and then to unavailable after a few secs If I switch if off from the TV remote, state remains on on all the time. if I then switch it off from HA ( TV is already off) state does not change to off and goes to unavailable after a few secs

marijngiesen commented 4 years ago

Same here. The initial state is always ON and when I turn off the TV with the remote, the state remains ON. If I turn it on again with the remote and then use HA to turn it off, the state is correct.

03397 commented 4 years ago

Same issue. I have opened a different ticket #34934 giving some more information.

lolouk44 commented 4 years ago

Any update on this @joogps please? In the meantime I've reverted back to the old component as I use my TV for A LOT of automations...

03397 commented 4 years ago

Any update on this @joogps please? In the meantime I've reverted back to the old component as I use my TV for A LOT of automations...

How did you do that?

lolouk44 commented 4 years ago

get the files form the previous integration and replace them in the panasonic folder. Ddepending on how you've installed HA, it may be in a different place. Mine is set to: /usr/src/homeassistant/homeassistant/components/panasonic_viera/

Then remove the new integration and add it the old way (yaml)

03397 commented 4 years ago

get the files form the previous integration and replace them in the panasonic folder. Ddepending on how you've installed HA, it may be in a different place. Mine is set to: /usr/src/homeassistant/homeassistant/components/panasonic_viera/

Then remove the new integration and add it the old way (yaml)

Thanks. I will wait a bit more for joogps to solve the issue. It does not a big deal for me so I can live without it at the moment but it is good to know.

joogps commented 4 years ago

@lolouk44 Hey! I’m working on some features to the integration (such as the possibility of sending remote key commands), and I’ve implemented some checks that might fix your problem. I’ll mention you when everything’s done 🙂

lolouk44 commented 4 years ago

Great stuff, thanks. It's not so critical for me personally since I've for now reverted to the old integration, but I'd sure like this fixed indeed. If you need a "guinea pig" to test some stuff let me know :)

joogps commented 4 years ago

@lolouk44 It's strange that the older integration works since the update method is practically the same: https://github.com/home-assistant/core/blob/d520a02b8c41b073476a615a582990a1b8e4ceff/homeassistant/components/panasonic_viera/media_player.py#L125

lolouk44 commented 4 years ago

yes, it's as if it's not catching the exception I had even tried to catch all exceptions in the new integration by removing (TimeoutError, URLError, SOAPError, OSError) on line 297 https://github.com/home-assistant/core/blob/405062d2df06c8e3861fd0010104d628306235df/homeassistant/components/panasonic_viera/media_player.py#L297 But that didn't fix it

circa1665 commented 4 years ago

Same issue. The initial state is always ON and when I turn off the TV with the remote, the state remains ON. If I turn it On again with the remote and then use HA to turn it off, the state is correct.

Also volume initial state is 0 in HA. If I adjust the volume in HA then that volume is set on the TV, but any external changes to volume are not reflected in HA.

joogps commented 4 years ago

It seems like the update method is not being called at all for you... Are you sure you don't have any logs @circa1665?

joogps commented 4 years ago

I think the problem could be that the new _update function was async (which was unnecessary since it's purpose was to be an enclosure for the other sync methods)

03397 commented 4 years ago

Before the upgrade I had this kind of issue and It was solved using the app_power: true. If I removed this option the problem reappeared.

joogps commented 4 years ago

@03397 Well, app_power had no influence on the update method at all, it would only enable the support for turn on. In this new version, you can overcome this by defining the more flexible attribute turn_on_action, which can be any kind of script. More info in the documentation.

rschaeuble commented 4 years ago

@joogps Yeah, it seems removing the async from _update fixes the issue. Apparently, async_add_executor_job doesn't work with async functions.

joogps commented 4 years ago

@rschaeuble That’s great! This fix will probably be included only with the next release, so it might take some time...

rschaeuble commented 4 years ago

@joogps Oh well, it seems I cheered too early.

Removing async causes _update to be called (which didn't happen before), but the state is still ON after restarting Home Assistant.

After _update sets the state to OFF in the exception handler, it directly calls async_create_remote_control, which sets it back to ON. I'm not sure whether it's OK to delete that call, so please also have a look.

Maybe something to also add to the test suite.

joogps commented 4 years ago

If you want to, you can set the self.state property to the default STATE_OFF, and that could maybe help... I was taking a look at the homebridge Panasonic Viera component and I might adopt a different method for handling the on/off states.

rschaeuble commented 4 years ago

I'll try setting the default to STATE_OFF a bit later today; right now my wife wants to use the TV ;) Could be a nice workaround until the final fix is there.

joogps commented 4 years ago

The Homebridge component (using the viera.js library) gets the on state directly from the TV... That way we could make the error handling and state checking more robust https://github.com/g30r93g/homebridge-panasonic/blob/ef76c7f10d66c5810c1ede9a8513b0bb1ee7a364/index.js#L250

rschaeuble commented 4 years ago

Getting the state directly from the TV sounds good.

But I guess special handling for a non-responding TV is still required. When it's completely powered off, or doesn't respond for any other reason, it should still be displayed as off. And regular polling should detect when it starts responding again.

joogps commented 4 years ago

Yes, that's what I'm thinking of.

joogps commented 4 years ago
Screen Shot 2020-05-01 at 15 32 56

@rschaeuble guess that's something 🤷

joogps commented 4 years ago

I figured out how to get the TV power on state even for models that don't disconnect from the network when turned off. Also, I managed to get data for the opened app (that could allow for input selection in the future maybe, although getting the input source, AV or HDMI, would be impossible).

joogps commented 4 years ago

I'll try to work on that as soon as the current changes I'm making get approved (please keep in your minds I have school work to do too 😂)

joogps commented 4 years ago

I'll implement a method that retrieves this data on the panasonic_viera library and see how I can integrate it with Home Assistant later

circa1665 commented 4 years ago

It seems like the update method is not being called at all for you... Are you sure you don't have any logs @circa1665?

@joogps this is my YAML config for the integration:

# Panasonic TV
panasonic_viera:
  host: !secret tv_ip
  name: panasonic_tv
  turn_on_action:
    - service: wake_on_lan.send_magic_packet
      data:
        mac: !secret tv_mac

Have the following error in logs:

Log Details (ERROR) Logger: homeassistant.components.media_player Source: helpers/entity_platform.py:112 Integration: Media player (documentation, issues) First occurred: 12:19:47 AM (1 occurrences) Last logged: 12:19:47 AM

The panasonic_viera platform for the media_player integration does not support platform setup. Please remove it from your config.

I removed the previous config, so don't understand why it's moaning about "platform setup"?

joogps commented 4 years ago

@circa1665 We figured that out already in a different issue. It’s because there was still some code left for panasonic_viera in the legacy discovery integration. It was removed already, and I think it’s probably going to ship with the next patch.

For now, you can add this to you configuration to prevent the error from appearing:

discovery:
  ignore:
    - panasonic_viera
joogps commented 4 years ago
Screen Shot 2020-05-03 at 00 35 59

I was able to make the subscription requests using Python. If everything goes well, we'll soon have local push states!

legolas0802 commented 4 years ago

the same problem occurs for the volume. if I change from the remote it is not updated

joogps commented 4 years ago

@lolouk44 are you there? Would you mind to see if the patch is working? Please download this repository (on this specific branch) and run Hass from there. Configure your TV and give me some feedback. Thanks!

lolouk44 commented 4 years ago

Hi @joogps I'll try tomorrow

joogps commented 4 years ago

Great! Thank you 😊

joogps commented 4 years ago
Screen Shot 2020-05-03 at 23 07 58

App detection (made possible by the extra data that is received by the new subscription method) is working perfectly! I'm still experimenting, but it seems like it's possible to display even the media track title, duration etc if the file is playing directly from the TV (aka, not streaming)

joogps commented 4 years ago

it's funny because the TV I'm using to test these new features (the only Panasonic Viera one we have in our house) is in my parent's bedroom

I went there and confirmed they were watching a movie on Netflix 😂

lolouk44 commented 4 years ago

Hi @joogps

I've just tried (copied to content of panasonic_viera over to my install) and I get this: Error occurred loading configuration flow for integration panasonic_viera: cannot import name 'TV_TYPE_ENCRYPTED' from 'panasonic_viera' (/config/panasonic_viera/__init__.py)

Is there anything else that I'm missing? I could not see any other changes than what is in the panasonic_viera folder.

joogps commented 4 years ago

@lolouk You should update the entire installation, and not just the component, since it’s based on the dev branch. Run script/setup and then hass.

joogps commented 4 years ago

Actually, that was my bad. I’m fixing the issue now.

joogps commented 4 years ago

If Home Assistant complains about MediaPlayerEntity or RemoteEntity, just replace them with MediaPlayerDevice and RemoteDevice

lolouk44 commented 4 years ago

sorry mate :( image

Had to turn the TV on for the integration to be setup, so there is some sort of communication, but state is not detected...

joogps commented 4 years ago

that's strange...

lolouk44 commented 4 years ago

more here:

Log Details (ERROR)
Logger: homeassistant.setup
Source: setup.py:249
First occurred: 12:15:47 PM (2 occurrences)
Last logged: 12:15:47 PM

Unable to prepare setup for platform panasonic_viera.media_player: Platform not found (cannot import name 'MediaPlayerEntity' from 'homeassistant.components.media_player' (/usr/src/homeassistant/homeassistant/components/media_player/__init__.py)).
Unable to prepare setup for platform panasonic_viera.remote: Platform not found (cannot import name 'RemoteEntity' from 'homeassistant.components.remote' (/usr/src/homeassistant/homeassistant/components/remote/__init__.py)).
joogps commented 4 years ago

Oh, you just need to replace MediaPlayerEntity and RemoteEntity with MediaPlayerDevice and RemoteDevice, both in the import sections and in the class declarations

joogps commented 4 years ago

I made a new commit to the repo that fixes this so you don't need to do it again later