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

Add support for Panasonic Blu-Ray players #18541

Merged
merged 3 commits into from Nov 21, 2018

Conversation

@u1f35c
Contributor

u1f35c commented Nov 17, 2018

Description:

Adds support for Panasonic Blu-Ray players within the media_player component. Enables viewing of the current status (off/idle/playing) as well as stopping/starting/pausing playback.

Related issue (if applicable): N/A

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7564

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: panasonic_bluray
    name: Blu-Ray (Cinema)
    host: 192.168.0.10

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
REQUIREMENTS = ['panacotta==0.1']
DEFAULT_NAME = "Panasonic Blu-Ray"
MIN_TIME_BETWEEN_UPDATES = timedelta(seconds=30)

This comment has been minimized.

@pvizeli

pvizeli Nov 19, 2018

Member

SCAN_INTERVAL

add_entities([PanasonicBluRay(host, name)])
elif discovery_info is not None:
add_entities(PanasonicBluRay(discovery_info.get('host'),
discovery_info.get('name')))

This comment has been minimized.

@pvizeli

pvizeli Nov 19, 2018

Member

What's a bit weird. Use this if/else above. Note: host can't be None with config.

This comment has been minimized.

@u1f35c

u1f35c Nov 19, 2018

Contributor

This pattern seems common in other media_player components, eg yamaha or ziggo_mediabox_xl. It's not clear to me how this should be done instead - can you clarify?

This comment has been minimized.

@pvizeli

pvizeli Nov 20, 2018

Member

It's a magic, but you need only 2 lines for the same thing

This comment has been minimized.

@u1f35c

u1f35c Nov 20, 2018

Contributor

Aha. Neat.

@pvizeli pvizeli merged commit 1e3930a into home-assistant:dev Nov 21, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 93.02%
Details

@wafflebot wafflebot bot removed the in progress label Nov 21, 2018

state = self._device.get_play_status()
if state[0] == 'error':
self._state = STATE_UNKNOWN

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 21, 2018

Member

Use None to represent unknown state.

self._position = state[1]
else:
self._position = 0
self._position_valid = utcnow()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 21, 2018

Member

This looks like it will spam the state machine, since a new state will be created every time a state attribute is changed and this line will change media_position_updated_at on every update interval (30 secs) regardless of state.

our favour as it means the device is still accepting commands and we
can thus turn it back on when desired.
"""
if self._state != STATE_OFF:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 21, 2018

Member

We usually don't want to check state before actuating a service. If we would have the wrong state info, we would not be able to change state via a service call.

mxworm added a commit to mxworm/home-assistant that referenced this pull request Nov 21, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant:
  Upgrade requests to 2.20.1 (home-assistant#18615)
  Fix mqtt cover inverted (home-assistant#18456)
  Update locationsharinglib requirement to 3.0.8 (home-assistant#18612)
  Add support for Panasonic Blu-Ray players (home-assistant#18541)
  Add support for HTTPS and basic HTTP authentication for Glances (home-assistant#18608)
  Add permissions check in service helper (home-assistant#18596)
  Upgrade blinkpy to 0.10.3 (Fixes home-assistant#18341) (home-assistant#18603)

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment