Introduce send_telnet_commands service for denonavr receivers
#96457
Conversation
home-assistant
bot
added
cla-signed
dependency
integration: denonavr
new-feature
labels
|
Hey there @starkillerOG, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| @@ -505,17 +550,17 @@ async def async_mute_volume(self, mute: bool) -> None: | |||
| await self._receiver.async_mute(mute) | |||
|
|
|||
| @async_log_errors | |||
| async def async_get_command(self, command: str, **kwargs): | |||
| async def async_get_command(self, command: str, **kwargs) -> str: | |||
There was a problem hiding this comment.
| async def async_get_command(self, command: str, **kwargs) -> str: | |
| async def async_get_command(self, command: str, **kwargs: Any) -> str: |
| @@ -133,6 +153,31 @@ async def async_setup_entry( | |||
| f"async_{SERVICE_UPDATE_AUDYSSEY}", | |||
| ) | |||
|
|
|||
| async def async_service_handle(service_call: ServiceCall): | |||
There was a problem hiding this comment.
I was not able to enter an response type here. It appears to me mypy fails converting data types of other annotations into JsonObjectType and JsonValueType correctly.
The line could look like
async def async_service_handle(service_call: ServiceCall) -> ServiceResponse:
But when I do so and try to commit (I'm using the dev-container) mypy fails
homeassistant/components/denonavr/media_player.py:172: error: Incompatible return value type (got "dict[str, list[dict[str, str]] | None]", expected "JsonObjectType | None") [return-value]
Found 1 error in 1 file (checked 1 source file)
However, when I build a fake return for line 172 which looks like the "incompatible return type" from the message mypy succeeds.
return {"asd": [{"asdf": "asd", "xyz": "asd"}], "xyz": None}
Changing the return type of async_send_telnet_commands method just moves the problem up the chain.
Can you help me with that?
There was a problem hiding this comment.
I couldn't get the typing to work without a cast either
diff --git a/homeassistant/components/denonavr/media_player.py b/homeassistant/components/denonavr/media_player.py
index 7b7eb30aa6..8cf83b4b83 100644
--- a/homeassistant/components/denonavr/media_player.py
+++ b/homeassistant/components/denonavr/media_player.py
@@ -7,7 +7,7 @@ from datetime import timedelta
from functools import wraps
import logging
import time
-from typing import Any, Concatenate, ParamSpec, TypeVar
+from typing import Any, Concatenate, ParamSpec, TypeVar, cast
from denonavr import DenonAVR
from denonavr.const import (
@@ -39,6 +39,7 @@ from homeassistant.const import ATTR_COMMAND, CONF_HOST, CONF_MODEL
from homeassistant.core import (
HomeAssistant,
ServiceCall,
+ ServiceResponse,
SupportsResponse,
)
from homeassistant.exceptions import HomeAssistantError
@@ -111,7 +112,7 @@ async def async_setup_entry(
async_add_entities: AddEntitiesCallback,
) -> None:
"""Set up the DenonAVR receiver from a config entry."""
- entities = []
+ entities: list[DenonDevice] = [] # This should be named DenonMediaPlayerEntity
data = hass.data[DOMAIN][config_entry.entry_id]
receiver = data[CONF_RECEIVER]
update_audyssey = config_entry.options.get(
@@ -153,27 +154,30 @@ async def async_setup_entry(
f"async_{SERVICE_UPDATE_AUDYSSEY}",
)
- async def async_service_handle(service_call: ServiceCall):
+ async def _async_telnet_commands_service(
+ service_call: ServiceCall,
+ ) -> ServiceResponse:
"""Handle dispatched services."""
assert platform is not None
- entities = await platform.async_extract_from_service(service_call)
-
- response = {}
-
- for entity in entities:
- assert isinstance(entity, DenonDevice)
-
- if service_call.service == SERVICE_TELNET_COMMANDS:
- response[entity.entity_id] = await entity.async_send_telnet_commands(
- service_call.data[ATTR_COMMAND]
+ entities = cast(
+ list[DenonDevice], await platform.async_extract_from_service(service_call)
+ )
+ command_results = await asyncio.gather(
+ entity.async_send_telnet_commands(service_call.data[ATTR_COMMAND])
+ for entity in entities
+ )
+ return cast(
+ ServiceResponse,
+ {
+ entity.entity_id: command_results[idx]
+ for idx, entity in enumerate(entities)
+ },
)
-
- return response
hass.services.async_register(
DOMAIN,
SERVICE_TELNET_COMMANDS,
- async_service_handle,
+ _async_telnet_commands_service,
schema=cv.make_entity_service_schema({vol.Required(ATTR_COMMAND): cv.string}),
supports_response=SupportsResponse.ONLY,
)
@@ -587,7 +591,7 @@ class DenonDevice(MediaPlayerEntity):
f"Invalid command {command}. Valid commands start with these prefixes: {TELNET_EVENTS_SORTED}"
)
- result = []
+ result: list[dict[str, str]] = []
times = {"last_message": 0.0}
async def async_telnet_callback(zone: str, event: str, parameter: str) -> None:
There was a problem hiding this comment.
I will give it a try later. Thanks 😄
| found = True | ||
| break | ||
| if not found: | ||
| raise HomeAssistantError( |
There was a problem hiding this comment.
| raise HomeAssistantError( | |
| raise ValueError( |
| success = self._receiver.send_telnet_commands(*command_list) | ||
| if success: | ||
| while True: | ||
| if time.monotonic() - times["start"] > DEFAULT_TIMEOUT: | ||
| _LOGGER.debug("Telnet command returns because timeout elapsed") | ||
| break | ||
| if ( | ||
| times["last_message"] | ||
| and time.monotonic() - times["last_message"] > 0.2 | ||
| ): | ||
| _LOGGER.debug( | ||
| "Telnet command returns because last message timeout elapsed" | ||
| ) | ||
| break | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
Maybe wrap this in a try and do the unregister in finally in case anything fails it doesn't end up leaking a registration
| if time.monotonic() - times["start"] > DEFAULT_TIMEOUT: | ||
| _LOGGER.debug("Telnet command returns because timeout elapsed") | ||
| break | ||
| if ( | ||
| times["last_message"] | ||
| and time.monotonic() - times["last_message"] > 0.2 | ||
| ): | ||
| _LOGGER.debug( | ||
| "Telnet command returns because last message timeout elapsed" | ||
| ) | ||
| break | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
This could use an asyncio.Future instead of sleep/poll/loop
|
I tested general operations with my devices and everything appears to be working as expected. I did not yet test the new service |
|
I'd like core team input on this before we merge. |
MartinHjelmare
added
the
second-opinion-wanted
|
Its a debatable if allowing telnet commands violates this rule: We have remotes that allow sending commands like |
|
Yeah, there are many questions here.
|
|
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: #96467 |
| assert isinstance(entity, DenonDevice) | ||
|
|
||
| if service_call.service == SERVICE_TELNET_COMMANDS: | ||
| response[entity.entity_id] = await entity.async_send_telnet_commands( |
There was a problem hiding this comment.
This is not allowed. We explicitly do not allow multiple response values from multiple entities to be combined
There was a problem hiding this comment.
I'm a bit confused 😕
I started by using register_entity_service because it is very easy to implement. However, it does not support returning any values, so I switched generic service register function, which seems to be the way to go according to the developer docs.
So, here is my question. How should I implement a service for an entity which returns data?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
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. |
|
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. |
MartinHjelmare
removed
the
second-opinion-wanted
|
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 #92767). |
|
I could use this to avoid using node-red to create my subwoofer and dialog level sliders. |
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 |
|
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? |
|
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. |
|
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. |
|
Can this continue being polished? |
|
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. |
Breaking change
Proposed change
This PR adds
send_telnet_commandstodenonavrreceivers 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
denonavrlibrary is updated to0.11.3(changelog).In case you need some telnet commands for testing, check out this doc.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: