Skip to content

Add support for max_volume#13822

Merged
balloob merged 6 commits intohome-assistant:devfrom
relvacode:dev
May 5, 2018
Merged

Add support for max_volume#13822
balloob merged 6 commits intohome-assistant:devfrom
relvacode:dev

Conversation

@relvacode
Copy link
Copy Markdown
Contributor

Description:

Adds a new max_volume option to Onkyo media players to limit the maximum volume set by the volume slider.

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

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: onkyo
    host: 192.168.1.2
    name: receiver
    max_volume: 50
    sources:
      pc: 'HTPC'

Checklist:

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

I get ERROR: typing: commands failed but no errors relate to onkyo.py

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 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.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @relvacode,

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!

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Logic are not allow on this layer.

@relvacode
Copy link
Copy Markdown
Contributor Author

@pvizeli In setup_platform()?

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Apr 14, 2018

Component/Platform they have a device are only interfaces and can only do things they handle by device/library.

@relvacode
Copy link
Copy Markdown
Contributor Author

Ah that's a shame, on my device volume level 80 will shake the entire house (as the girlfriend found out when she accidentally pressed the volume slider all the way up in the app!), 40 is about as high as we would ever go.
It also means the volume slider steps in the app are too wide to have fine grain control over the volume.

Is there another layer by which this can be achieved? The nad media player appears to implement this at the component level https://www.home-assistant.io/components/media_player.nad/

If not feel free to close, I'll continue using this as a custom component

@rsmeral
Copy link
Copy Markdown
Contributor

rsmeral commented Apr 22, 2018

This change is important for more than just customizability. This Onkyo component supports (as per documentation) also Pioneer receivers. And, for example, the volume on my Pioneer SX-S30DAB only goes to 50.
So, the hardcoded value of 80 is incorrect for some of the supposedly supported devices.

@pvizeli I'm not sure I understand your review – this PR is not introducing any logic (apart from value clamping), it's only enabling user-configurability of one value.

@fabaff fabaff changed the title onkyo: Add support for max_volume Add support for max_volume Apr 28, 2018
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_HOST): cv.string,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_MAX_VOLUME, default=SUPPORTED_MAX_VOLUME): int,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of validating this as int, validate it with a range:

vol.All(vol.Coerce(int), vol.Range(min=0, max=SUPPORTED_MAX_VOLUME))

hosts = []

max_volume = config.get(CONF_MAX_VOLUME)
if max_volume > SUPPORTED_MAX_VOLUME:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be removed as it will be handled by the config.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Few minor changes, but this is ok to merge.

Although @pvizeli is right that we don't want user config for devices in Home Assistant, I think that this is worth an exception because Onkyo is a pass-through device.

hosts.append(OnkyoDevice(
eiscp.eISCP(host), config.get(CONF_SOURCES),
name=config.get(CONF_NAME)))
name=config.get(CONF_NAME), max_volume=config.get(CONF_MAX_VOLUME)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (84 > 79 characters)

@relvacode
Copy link
Copy Markdown
Contributor Author

@balloob Requested changes made

@balloob balloob merged commit 4d08588 into home-assistant:dev May 5, 2018
@balloob balloob mentioned this pull request May 11, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants