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

Support eco mode option on Ziggo Mediabox XL #17990

Merged
merged 5 commits into from Nov 6, 2018

Conversation

Projects
None yet
5 participants
@4lloyd
Contributor

4lloyd commented Oct 29, 2018

Description:

The Ziggo Mediabox XL has an eco function which turn off the device when it's not in use. This PR adds an option to override the online check, so it's still added when the device is in eco modus.

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

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: ziggo_mediabox_xl
    host: 192.168.0.123
    name: Ziggo Mediabox

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

@homeassistant

This comment has been minimized.

homeassistant commented Oct 29, 2018

Hi @4lloyd,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'

@4lloyd 4lloyd referenced this pull request Oct 29, 2018

Closed

Updated documentation for Ziggo Mediabox XL #7255

2 of 2 tasks complete

@homeassistant homeassistant added cla-signed and removed cla-needed labels Oct 29, 2018

@cgtobi

This comment has been minimized.

Contributor

cgtobi commented Oct 30, 2018

Just my personal opinion, isn't eco_mode_on a bit redundant?
I think eco_mode being trueor falseis explicit enough.

@4lloyd

This comment has been minimized.

Contributor

4lloyd commented Oct 30, 2018

That was also a doubt for me. In the end I did choose for eco_mode_on because I thought it's more clear that it's an option which has to be set on the device itself by the user in advance. I didn't want the users to think it's an option that enables some sort of eco mode in HA or on the mediabox itself. I have no problem changing it if eco_mode is clear enough or more the HA way. Just let me know :).

@cgtobi

This comment has been minimized.

Contributor

cgtobi commented Oct 30, 2018

You explained it in the docs, so, for me it is clear. 😄

@fabaff fabaff changed the title from Added eco mode option to Ziggo Mediabox XL to Add eco mode option to Ziggo Mediabox XL Nov 1, 2018

@balloob

This comment has been minimized.

Member

balloob commented Nov 1, 2018

We shouldn't add this. We should just always add the entity and then have the available property return False.

We should not outsource problems in our code to a config option for users.

@4lloyd 4lloyd changed the title from Add eco mode option to Ziggo Mediabox XL to Support eco mode option on Ziggo Mediabox XL Nov 2, 2018

@4lloyd 4lloyd changed the title from Support eco mode option on Ziggo Mediabox XL to [WIP] Support eco mode option on Ziggo Mediabox XL Nov 2, 2018

@4lloyd

This comment has been minimized.

Contributor

4lloyd commented Nov 2, 2018

There still seems a problem with the timeout of the Ziggo Mediabox XL package. I'm waiting for this to be changed over there.

@4lloyd 4lloyd referenced this pull request Nov 2, 2018

Closed

Socket timeout #1

@4lloyd 4lloyd changed the title from [WIP] Support eco mode option on Ziggo Mediabox XL to Support eco mode option on Ziggo Mediabox XL Nov 4, 2018

mediabox = ZiggoMediaboxXL(ip_addr, 5)
# Add the device manually or check if a connection can be
# established to the discovered device.
if manual_config or mediabox.test_connection():

This comment has been minimized.

@balloob

balloob Nov 5, 2018

Member

We should test connection on manual configured devices. Please revert.

@@ -56,8 +59,10 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
ip_addr = socket.gethostbyname(host)
if ip_addr not in known_devices:
try:
mediabox = ZiggoMediaboxXL(ip_addr)
if mediabox.test_connection():
mediabox = ZiggoMediaboxXL(ip_addr, 5)

This comment has been minimized.

@balloob
@@ -89,7 +93,7 @@ def update(self):
else:
self._state = STATE_OFF
except socket.error:
_LOGGER.error("Couldn't fetch state from %s", self._host)
self._state = STATE_UNAVAILABLE

This comment has been minimized.

@balloob

balloob Nov 5, 2018

Member

this is wrong. There is an available property which should return False.

@4lloyd 4lloyd force-pushed the 4lloyd:eco-mode-ziggo-mediabox-xl branch from ea68a16 to 2e33564 Nov 5, 2018

@balloob

balloob approved these changes Nov 6, 2018

@balloob balloob merged commit 114bc8e into home-assistant:dev Nov 6, 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 First build on eco-mode-ziggo-mediabox-xl at 93.1%
Details
@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

Nice!

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

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

Support eco mode option on Ziggo Mediabox XL (home-assistant#17990)
* Added eco mode option to Ziggo Mediabox XL

* Changed eco_mode_on to eco_mode

* Removed eco_mode option, the player is unavailable when offline

* Timeout on connection, on/off states are handled via update

* Improved state detection and added available property

@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