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

Update the Panasonic Viera component #33433

Closed
wants to merge 2 commits into from
Closed

Update the Panasonic Viera component #33433

wants to merge 2 commits into from

Conversation

joogps
Copy link
Contributor

@joogps joogps commented Mar 30, 2020

Breaking change

If your TV needs encryption, just remove the entity and restart Home Assistant. The configurator will them show up in notifications asking you to start the pairing process (very much like the webostv integration).

Proposed change

Add encryption support for the Panasonic Viera integration (through configurator) with persistent data and a send_key service to easily send key presses to the TV. It also prevents HTTP errors by renewing the connection at every update and updates manifest.json and requirements_all.txt to require the latest panasonic-viera library version (0.3.4).

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml (same as documentation)
media_player:
  - platform: panasonic_viera
    host: 192.168.0.10
# Service data example for the send_key service
entity_id: media_player.living_room_tv
key: left

Additional information

There are still some changes that need to be made, such as preventing the entity from not being created and adding sources support.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@joogps
Copy link
Contributor Author

joogps commented Mar 30, 2020

For some reason, the update of requirements_all.txt fails the CI tests. It's the first time I contribute to HA (actually probably the first time I contribute to such a big project), am I doing something wrong?

Copy link
Member

@springstan springstan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format your code with black and take a look at the flake8 errors on Azure DevOps.

@@ -2,7 +2,7 @@
"domain": "panasonic_viera",
"name": "Panasonic Viera TV",
"documentation": "https://www.home-assistant.io/integrations/panasonic_viera",
"requirements": ["panasonic_viera==0.3.2", "wakeonlan==1.1.6"],
"requirements": ["panasonic_viera==0.3.4", "wakeonlan==1.1.6", "configurator"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"configurator" is rather a dependency than a requirement since it is HA internal, therefore please move it to the dependencies.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The access mode has changed for some of the files. Eg:
homeassistant/components/panasonic_viera/init.py 100644 → 100755

Please revert those changes.

@@ -1,7 +1,7 @@
"""Support for interface with a Panasonic Viera TV."""
import logging

from panasonic_viera import RemoteControl
from panasonic_viera import *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import names explicitly. Don't use a star.

needs_pairing = False

if needs_pairing:
configurator = hass.components.configurator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The configurator is legacy and should not be used for new code. Please create a config flow instead.

We have a scaffolding script to help you get started with that.

https://developers.home-assistant.io/docs/creating_component_index

@@ -117,6 +177,14 @@ def __init__(self, mac, name, remote, host, broadcast, app_power, uuid=None):
self._volume = 0
self._app_power = app_power

hass.services.async_register(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want side effects in init method. Please move the service registration out of init.

Also, we should limit this pull request to one thing. A new service can be added in a different PR.

@springstan springstan changed the title Updating the Panasonic Viera component Update the Panasonic Viera component Mar 31, 2020
@ramondunker
Copy link

If all tests pass, will it automatically be included in the Home Assistant Core?

@joogps
Copy link
Contributor Author

joogps commented Apr 1, 2020

Hi!
I'm just saw this (I spent the entire day doing school stuff), and I'll try to fix some of these errors when I have some extra time available. @ramondunker you are welcome to help if you want to!

(Also, I'm sorry for doing so many things wrong, it's the first time I contribute to Home Assistant and I probably didn't read the guidelines right.)

@joogps
Copy link
Contributor Author

joogps commented Apr 5, 2020

Working on it!

@sgthree
Copy link

sgthree commented Apr 5, 2020

Not sure if this is helpful, but there appears to be an issue with the Panasonic_viera 0.3.4 library that needs to be taken into account with this integration. If a command is sent to a tv when it is off, an HTTPError exception is thrown. Once this happens, the remote will not work any more. This is because the library code decrements _session_seq_num when an exception is thrown, but does not take into account the possibility that the exception occurs before _session_seq_num is incremented.
The remote therefore gets out of sync. Testing for the exception, and incrementing _session_seq_num when it occurs ensures that the remote still works.
You can use this to test whether the tv is on or off by issuing a get_mute command, which returns true or false if the tv is on, or throws and exception if it is off.

@joogps
Copy link
Contributor Author

joogps commented Apr 6, 2020

Thanks, @sgthree, I will investigate that!

I'll be creating a new pull request soon.

@joogps joogps closed this Apr 6, 2020
@lock lock bot locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panasonic GX800 series with Panasonic Viera TV integration doesn't works
6 participants