Introduce `send_telnet_commands` service for `denonavr` receivers

[ol-iver]

Breaking change

Proposed change

This PR adds send_telnet_commands to denonavr receivers which allow sending one or multiple telnet commands to the device and subscribe for their results. Since there is not explicit start and end for the responses, the service waits for a 5 seconds timeout (DEFAULT_TIMEOUT) or for 0.2 seconds after the last message arrived (whatever happens first).

Along with this new feature denonavr library is updated to 0.11.3 (changelog).

In case you need some telnet commands for testing, check out this doc.

Type of change

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

Additional information

• This PR fixes or closes issue: fixes #92767
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• [x] The code change is tested and works locally.
• [x] Local tests pass. Your PR cannot be merged unless tests pass
• [x] There is no commented out code in this PR.
• [ ] I have followed the development checklist
• [ ] I have followed the perfect PR recommendations
• [x] 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:

• [ ] Documentation added/updated for www.home-assistant.io

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

• [x] The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• [x] New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• [ ] I have reviewed two other open pull requests in this repository.

[home-assistant[bot]]

Hey there @starkillerog, mind taking a look at this pull request as it has been labeled with an integration (denonavr) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of denonavr can trigger bot actions by commenting:

• @home-assistant close Closes the pull request.
• @home-assistant rename Awesome new title Renames the pull request.
• @home-assistant reopen Reopen the pull request.
• @home-assistant unassign denonavr Removes the current integration label and assignees on the pull request, add the integration domain after the command.

[bdraco]

I tested general operations with my devices and everything appears to be working as expected.

I did not yet test the new service

[MartinHjelmare]

I'd like core team input on this before we merge.

[bdraco]

Its a debatable if allowing telnet commands violates this rule: https://developers.home-assistant.io/docs/creating_component_code_review?_highlight=pypi#4-communication-with-devicesservices

We have remotes that allow sending commands like Play, Pause, etc. This seems close to that because the protocol data / API is (mostly) human readable, but its a stretch so it needs some more input to make sure we aren't setting the wrong precedent here.

[MartinHjelmare]

Yeah, there are many questions here.

1. Should it be a remote entity instead?
2. We currently don't allow multiple entity service responses in a single service call. What's the plan for that?
3. The integration registers a platform service for an entity without using our entity platform service helper to workaround the fact we haven't added support for service responses there yet. Should we allow that?

[bdraco]

I think it would make sense to do the lib bump in another PR while we figure out how to move forward with the service

[ol-iver]

I think it would make sense to do the lib bump in another PR while we figure out how to move forward with the service

Sounds good, here is the PR for the version update: https://github.com/home-assistant/core/pull/96467

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[Rudd-O]

Can this perhaps be tweaked so that the service call itself takes a parameter "expects response y/n" so the system does not have to wait unnecessarily for a response? Thanks.

[MartinHjelmare]

Besides the architecture discussion we're waiting for, I think this PR needs to explain more what commands are meant to be executed with the new service. When looking at the protocol documentation it looks like most commands have a response value that is an acknowledgement of the sent command including parameters. I may be misreading, so please clarify this.

Services returning a response is not meant for arbitrary commands and responses that are already available as other features of an entity. There should be a specific need for the service and service response which is not already available. Eg, for media player entities, I could think that a search function searching for songs of an artist could be appropriate (until we standardize that). A service that sets the volume and responds with the volume level set, is not appropriate, as there's already a specific service for that and the response is just an acknowledgement of the service call success.

We also want services returning a response to be as specific as possible so that the response can be described as specific as possible to make the user experience as good as possible. Different service responses are then better exposed via different services.

[ol-iver]

The new service is for people who would like to add custom sensors for properties which do not exist in media player entities (like in this issue https://github.com/home-assistant/core/issues/92767). There is already the get_command service which does not return any data and works for the same commands I enlisted in the PR description. Thus, if it is not possible/not wanted to add services which return any data, we could close this PR.

[Rudd-O]

I could use this to avoid using node-red to create my subwoofer and dialog level sliders.

[MartinHjelmare]

The new service is for people who would like to add custom sensors for properties which do not exist in media player entities

Why doesn't the integration create these sensors for these properties?

[ol-iver]

The new service is for people who would like to add custom sensors for properties which do not exist in media player entities

Why doesn't the integration create these sensors for these properties?

The receiver offers too many properties. Additionally, those properties are varying for different models. Please check the doc I attached to the PR description. It shows commands and parameters of one model.

[MartinHjelmare]

Entities can be disabled by default if they are considered not useful for the majority of users. Isn't it possible to enumerate existing properties for a device?

[Rudd-O]

Seconding what MartinHjelmare said. Most entities can be disabled on add, and the user can enable them if they so choose. This is what the NUT integration does, for example.

[github-actions[bot]]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

[Rudd-O]

Can this continue being polished?

[github-actions[bot]]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!