home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
71.83k stars 30.09k forks source link

Roku remote doesn't support `delay_secs` #102080

Open ScottG489 opened 11 months ago

ScottG489 commented 11 months ago

The problem

When calling the remote.send_command service on a roku remote entity, and specifying a delay_secs, it will be ignored and there will be no delay.

You can see it just isn't implemented here: homeassistant/components/roku/remote.py#L63.

An example of how it is implemented elsewhere: homeassistant/components/broadlink/remote.py#L242


I'd be willing to implement this. I think it would roughly look like this:

diff --git a/homeassistant/components/roku/remote.py b/homeassistant/components/roku/remote.py
index ef5350eb74..f1eb454cf8 100644
--- a/homeassistant/components/roku/remote.py
+++ b/homeassistant/components/roku/remote.py
@@ -3,8 +3,9 @@ from __future__ import annotations

 from collections.abc import Iterable
 from typing import Any
+import asyncio

-from homeassistant.components.remote import ATTR_NUM_REPEATS, RemoteEntity
+from homeassistant.components.remote import ATTR_NUM_REPEATS, RemoteEntity, ATTR_DELAY_SECS
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.core import HomeAssistant
 from homeassistant.helpers.entity_platform import AddEntitiesCallback
@@ -59,9 +60,13 @@ class RokuRemote(RokuEntity, RemoteEntity):
     async def async_send_command(self, command: Iterable[str], **kwargs: Any) -> None:
         """Send a command to one device."""
         num_repeats = kwargs[ATTR_NUM_REPEATS]
+        delay = kwargs[ATTR_DELAY_SECS]

         for _ in range(num_repeats):
             for single_command in command:
                 await self.coordinator.roku.remote(single_command)

+            if num_repeats > 1:
+                await asyncio.sleep(delay)

Looking for feedback.

Background

I found this issue when trying to run commands quickly in succession using num_repeats. However, it may also just be a problem when making multiple service calls quickly. It seems they would happen out of order or some would be missed. Didn't investigate too closely.

While I think the Roku remote.send_command implementation should implement delay_secs regardless, I think this indicates a problem with the underlying Roku calls. Ideally the underlying requests to Roku should be made synchronously so they can't be made out of order or whatever is happening. In other words, the requested remote command should be sent and we should wait for confirmation that it's actually complete. Perhaps we are already doing this and this is a problem with Roku itself, but it's something to consider.

What version of Home Assistant Core has the issue?

core-2023.10.3

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

roku

Link to integration documentation on our website

https://www.home-assistant.io/integrations/roku/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 11 months ago

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

Code owner commands Code owners of `roku` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign roku` Removes the current integration label and assignees on the issue, add the integration domain after the command.

(message by CodeOwnersMention)


roku documentation roku source (message by IssueLinks)

issue-triage-workflows[bot] commented 8 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

ScottG489 commented 8 months ago

Still relevant.

issue-triage-workflows[bot] commented 5 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

ScottG489 commented 5 months ago

Still relevant.

issue-triage-workflows[bot] commented 2 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

ScottG489 commented 2 months ago

Still relevant.