Skip to content
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

Introduce send_telnet_commands service for denonavr receivers #96457

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion homeassistant/components/denonavr/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"documentation": "https://www.home-assistant.io/integrations/denonavr",
"iot_class": "local_push",
"loggers": ["denonavr"],
"requirements": ["denonavr==0.11.2"],
"requirements": ["denonavr==0.11.3"],
"ssdp": [
{
"manufacturer": "Denon",
Expand Down
146 changes: 123 additions & 23 deletions homeassistant/components/denonavr/media_player.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,24 @@
"""Support for Denon AVR receivers using their HTTP interface."""
from __future__ import annotations

import asyncio
from collections.abc import Awaitable, Callable, Coroutine
from datetime import timedelta
from functools import wraps
import logging
import time
from typing import Any, Concatenate, ParamSpec, TypeVar

from denonavr import DenonAVR
from denonavr.const import POWER_ON, STATE_OFF, STATE_ON, STATE_PAUSED, STATE_PLAYING
from denonavr.const import (
MAIN_ZONE,
POWER_ON,
STATE_OFF,
STATE_ON,
STATE_PAUSED,
STATE_PLAYING,
TELNET_EVENTS,
)
from denonavr.exceptions import (
AvrCommandError,
AvrForbiddenError,
Expand All @@ -26,7 +36,12 @@
)
from homeassistant.config_entries import ConfigEntry
from homeassistant.const import ATTR_COMMAND, CONF_HOST, CONF_MODEL
from homeassistant.core import HomeAssistant
from homeassistant.core import (
HomeAssistant,
ServiceCall,
SupportsResponse,
)
from homeassistant.exceptions import HomeAssistantError
from homeassistant.helpers import config_validation as cv, entity_platform
from homeassistant.helpers.entity import DeviceInfo
from homeassistant.helpers.entity_platform import AddEntitiesCallback
Expand All @@ -37,6 +52,7 @@
CONF_SERIAL_NUMBER,
CONF_TYPE,
CONF_UPDATE_AUDYSSEY,
DEFAULT_TIMEOUT,
DEFAULT_UPDATE_AUDYSSEY,
DOMAIN,
)
Expand Down Expand Up @@ -70,6 +86,7 @@
# Services
SERVICE_GET_COMMAND = "get_command"
SERVICE_SET_DYNAMIC_EQ = "set_dynamic_eq"
SERVICE_TELNET_COMMANDS = "send_telnet_commands"
SERVICE_UPDATE_AUDYSSEY = "update_audyssey"

_DenonDeviceT = TypeVar("_DenonDeviceT", bound="DenonDevice")
Expand All @@ -84,6 +101,9 @@
STATE_PAUSED: MediaPlayerState.PAUSED,
}

TELNET_EVENTS_SORTED = list(TELNET_EVENTS)
TELNET_EVENTS_SORTED.sort(key=len, reverse=True)


async def async_setup_entry(
hass: HomeAssistant,
Expand Down Expand Up @@ -133,6 +153,31 @@ async def async_setup_entry(
f"async_{SERVICE_UPDATE_AUDYSSEY}",
)

async def async_service_handle(service_call: ServiceCall):
Copy link
Member

Choose a reason for hiding this comment

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

Missing return type

Copy link
Contributor Author

@ol-iver ol-iver Jul 13, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will give it a try later. Thanks 😄

"""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(
Copy link
Member

Choose a reason for hiding this comment

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

This is not allowed. We explicitly do not allow multiple response values from multiple entities to be combined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

service_call.data[ATTR_COMMAND]
)

return response

hass.services.async_register(
DOMAIN,
SERVICE_TELNET_COMMANDS,
async_service_handle,
schema=cv.make_entity_service_schema({vol.Required(ATTR_COMMAND): cv.string}),
supports_response=SupportsResponse.ONLY,
)

async_add_entities(entities, update_before_add=True)


Expand Down Expand Up @@ -304,17 +349,17 @@ def state(self) -> MediaPlayerState | None:
return DENON_STATE_MAPPING.get(self._receiver.state)

@property
def source_list(self):
def source_list(self) -> list[str]:
"""Return a list of available input sources."""
return self._receiver.input_func_list

@property
def is_volume_muted(self):
def is_volume_muted(self) -> bool:
"""Return boolean if volume is currently muted."""
return self._receiver.muted

@property
def volume_level(self):
def volume_level(self) -> float | None:
"""Volume level of the media player (0..1)."""
# Volume is sent in a format like -50.0. Minimum is -80.0,
# maximum is 18.0
Expand All @@ -323,12 +368,12 @@ def volume_level(self):
return (float(self._receiver.volume) + 80) / 100

@property
def source(self):
def source(self) -> str | None:
"""Return the current input source."""
return self._receiver.input_func

@property
def sound_mode(self):
def sound_mode(self) -> str | None:
"""Return the current matched sound mode."""
return self._receiver.sound_mode

Expand All @@ -340,7 +385,7 @@ def supported_features(self) -> MediaPlayerEntityFeature:
return self._supported_features_base

@property
def media_content_id(self):
def media_content_id(self) -> None:
"""Content ID of current playing media."""
return None

Expand All @@ -352,19 +397,19 @@ def media_content_type(self) -> MediaType:
return MediaType.CHANNEL

@property
def media_duration(self):
def media_duration(self) -> None:
"""Duration of current playing media in seconds."""
return None

@property
def media_image_url(self):
def media_image_url(self) -> str | None:
"""Image url of current playing media."""
if self._receiver.input_func in self._receiver.playing_func_list:
return self._receiver.image_url
return None

@property
def media_title(self):
def media_title(self) -> str | None:
"""Title of current playing media."""
if self._receiver.input_func not in self._receiver.playing_func_list:
return self._receiver.input_func
Expand All @@ -373,46 +418,46 @@ def media_title(self):
return self._receiver.frequency

@property
def media_artist(self):
def media_artist(self) -> str | None:
"""Artist of current playing media, music track only."""
if self._receiver.artist is not None:
return self._receiver.artist
return self._receiver.band

@property
def media_album_name(self):
def media_album_name(self) -> str | None:
"""Album name of current playing media, music track only."""
if self._receiver.album is not None:
return self._receiver.album
return self._receiver.station

@property
def media_album_artist(self):
def media_album_artist(self) -> None:
"""Album artist of current playing media, music track only."""
return None

@property
def media_track(self):
def media_track(self) -> None:
"""Track number of current playing media, music track only."""
return None

@property
def media_series_title(self):
def media_series_title(self) -> None:
"""Title of series of current playing media, TV show only."""
return None

@property
def media_season(self):
def media_season(self) -> None:
"""Season of current playing media, TV show only."""
return None

@property
def media_episode(self):
def media_episode(self) -> None:
"""Episode of current playing media, TV show only."""
return None

@property
def extra_state_attributes(self):
def extra_state_attributes(self) -> dict[str, Any]:
"""Return device specific state attributes."""
if self._receiver.power != POWER_ON:
return {}
Expand All @@ -427,7 +472,7 @@ def extra_state_attributes(self):
return state_attributes

@property
def dynamic_eq(self):
def dynamic_eq(self) -> bool | None:
"""Status of DynamicEQ."""
return self._receiver.dynamic_eq

Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def async_get_command(self, command: str, **kwargs) -> str:
async def async_get_command(self, command: str, **kwargs: Any) -> str:

"""Send generic command."""
return await self._receiver.async_get_command(command)

@async_log_errors
async def async_update_audyssey(self):
async def async_update_audyssey(self) -> None:
"""Get the latest audyssey information from device."""
await self._receiver.async_update_audyssey()

@async_log_errors
async def async_set_dynamic_eq(self, dynamic_eq: bool):
async def async_set_dynamic_eq(self, dynamic_eq: bool) -> None:
"""Turn DynamicEQ on or off."""
if dynamic_eq:
await self._receiver.async_dynamic_eq_on()
Expand All @@ -524,3 +569,58 @@ async def async_set_dynamic_eq(self, dynamic_eq: bool):

if self._update_audyssey:
await self._receiver.async_update_audyssey()

@async_log_errors
async def async_send_telnet_commands(self, commands: str) -> list[dict[str, str]]:
"""Send telnet commands to receiver and subscribe to its responses."""
command_list = commands.splitlines()
events = []
for command in command_list:
found = False
for event in TELNET_EVENTS_SORTED:
if command.startswith(event):
events.append(event)
found = True
break
if not found:
raise HomeAssistantError(
Copy link
Member

Choose a reason for hiding this comment

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

f"Invalid command {command}. Valid commands start with these prefixes: {TELNET_EVENTS_SORTED}"
)

result = []
times = {"last_message": 0.0}

async def async_telnet_callback(zone: str, event: str, parameter: str) -> None:
times["last_message"] = time.monotonic()
if zone in [MAIN_ZONE, self._receiver.zone]:
result.append({"event": event, "parameter": parameter})

for event in events:
self._receiver.register_callback(event, async_telnet_callback)

times["start"] = time.monotonic()
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)
Comment on lines +602 to +616
Copy link
Member

Choose a reason for hiding this comment

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

Maybe wrap this in a try and do the unregister in finally in case anything fails it doesn't end up leaking a registration

Comment on lines +605 to +616
Copy link
Member

Choose a reason for hiding this comment

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

This could use an asyncio.Future instead of sleep/poll/loop


for event in events:
self._receiver.unregister_callback(event, async_telnet_callback)

if not success:
raise HomeAssistantError(
"No telnet connection or connection is not healthy"
)

return result
15 changes: 15 additions & 0 deletions homeassistant/components/denonavr/services.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,21 @@ set_dynamic_eq:
default: true
selector:
boolean:
send_telnet_commands:
name: Send telnet commands
description: "Send generic telnet commands."
target:
entity:
integration: denonavr
domain: media_player
fields:
command:
name: Commands
description: One telnet command per line.
required: true
selector:
text:
multiline: true
update_audyssey:
target:
entity:
Expand Down
2 changes: 1 addition & 1 deletion requirements_all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ deluge-client==1.7.1
demetriek==0.4.0

# homeassistant.components.denonavr
denonavr==0.11.2
denonavr==0.11.3

# homeassistant.components.devolo_home_control
devolo-home-control-api==0.18.2
Expand Down
2 changes: 1 addition & 1 deletion requirements_test_all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ deluge-client==1.7.1
demetriek==0.4.0

# homeassistant.components.denonavr
denonavr==0.11.2
denonavr==0.11.3

# homeassistant.components.devolo_home_control
devolo-home-control-api==0.18.2
Expand Down