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

Add timeout / debounce (for brightness and others) #13534

Merged
merged 3 commits into from
Apr 6, 2018
Merged

Add timeout / debounce (for brightness and others) #13534

merged 3 commits into from
Apr 6, 2018

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Mar 29, 2018

Description:

This PR fixes an issue, where for continuous characteristics slowly changing the value in the Home app caused multiple service calls, instead of just one. Since this timeout feature introduces latency, I decided to add a configuration parameter for it, so users can decide how long the timeout should be (float, between 0 and 5s). The Timeout is set to 0.5s.

In addition I did some style cleanup.

Related issue: fixes #13493
Discussion belove: #12819

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

Example entry for configuration.yaml (if applicable):

homekit:
  timeout: 2

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.
  • Documentation added/updated in home-assistant.github.io

cc: @maxclaey @DaveOke

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

I don't think that is a timeout and more like a API throttle.

I don't like the code, It's more as a hack and wired. Why do not use a throttle on the function, so you need not create a thread for every function. I think that will be more cleaner.

@@ -51,6 +54,9 @@
entity_filter = conf[CONF_FILTER]
entity_config = conf[CONF_ENTITY_CONFIG]

global TIMEOUT
Copy link
Member

Choose a reason for hiding this comment

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

global is not allow. use hass.data

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change that

@pvizeli pvizeli removed this from the 0.66 milestone Mar 29, 2018
@cdce8p
Copy link
Member Author

cdce8p commented Mar 29, 2018

@pvizeli You are right in it being an API throttle, since the Home app sends to many calls to Home Assistant. However a normal throttle function wouldn't work, because I need the last call not the first one. That's the throttle example I looked at https://gist.github.com/ChrisTM/5834503. Am I missing something?

In general I'm not that big of a fan adding this either, but I acknowledge that without it, there might be issue (like #13493). Under the current circumstances I think that is the way to move foreword.

@fabaff fabaff changed the title Homekit: Cleanup and bugfix (add timeout) Cleanup and bugfix (add timeout) Mar 29, 2018
@balloob
Copy link
Member

balloob commented Mar 29, 2018

The algorithm you're looking for is called debounce: call a function X time after the last call has been called. Calls within the X time will reset the timer.

You should not use Timer, as that uses threads. Instead, use our call_later helper to schedule a call. Calling call_later will return a function to cancel the future invocation.

Let's not make it user configurable, just set a reasonable timeout like 0.5s?

@cdce8p
Copy link
Member Author

cdce8p commented Mar 30, 2018

The timeout function is now async and timeout=0.5.

@pvizeli
Copy link
Member

pvizeli commented Mar 30, 2018

use our helper.events.call_later they return a cancelable object.

@cdce8p
Copy link
Member Author

cdce8p commented Mar 30, 2018

@pvizeli Would this be the way to go?

def add_timeout(func):

    @callback
    def call_later_listener(hass, acc, value, *args):
        nonlocal remove_listener
        remove_listener = None
        hass.async_run_job(func, acc, value)

    @wraps(func)
    def wrapper(*args):
        hass = args[0].hass
        nonlocal remove_listener
        if remove_listener:
            remove_listener()
        remove_listener = async_call_later(
            hass, TIMEOUT, partial(call_later_listener, hass, *args))
        logger.debug('%s: Start %s timeout', args[0].entity_id,
                     func.__name__.replace('set_', ''))

    remove_listener = None
    name = getmodule(func).__name__
    logger = logging.getLogger(name)
    return wrapper

I added the function call_later_listener to reset remove_listener and to prevent the function between called with now. Otherwise I did get WARNING: Unable to remove unknown listener. Is there a better way to do it?

@DaveOke
Copy link

DaveOke commented Mar 30, 2018

Would recommend adding an optional adjustable timeout between 0 and 1 so users can tweak latency given their system/configuration.

@@ -16,6 +20,37 @@
_LOGGER = logging.getLogger(__name__)


def add_timeout(func):
Copy link
Member

Choose a reason for hiding this comment

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

Please call it debounce.

nonlocal remove_listener
if remove_listener:
remove_listener()
remove_listener = async_call_later(
Copy link
Member

Choose a reason for hiding this comment

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

You are not allowed to call async methods from a sync context.

if remove_listener:
remove_listener()
remove_listener = track_point_in_utc_time(
hass, partial(call_later_listener, hass, *args),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using partial, why not store the arguments like you store the unsubscribe listener function

nonlocal lastargs
lastargs = args

"""Callback listener called from call_later."""
nonlocal remove_listener
remove_listener = None
hass.add_job(func, acc, value)
Copy link
Member

Choose a reason for hiding this comment

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

You've annotated this method with @callback, which means that it will be run inside the event loop, so this should be hass.async_add_job(func, *lastargs)

@philk philk mentioned this pull request Apr 1, 2018
4 tasks
@cdce8p cdce8p mentioned this pull request Apr 3, 2018
2 tasks
@cdce8p
Copy link
Member Author

cdce8p commented Apr 3, 2018

I extracted the style changes into #13654 and will rebase this PR once that one is merged.

@cdce8p cdce8p changed the title Cleanup and bugfix (add timeout) Add timeout (for brightness and others) Apr 5, 2018
* Decorator for setter methods to limit service calls to HA
* Changed to async
* Use async_call_later
* Use lastargs, async_add_job
@cdce8p cdce8p changed the title Add timeout (for brightness and others) Add timeout / debounce (for brightness and others) Apr 5, 2018
# pylint: disable=unsubscriptable-object
nonlocal lastargs, remove_listener
hass = lastargs[0]
hass.async_add_job(func, *lastargs[1:])
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this weird. it used to take hass but now we don't pass that on?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see now how you're storing this. Can we store it in a dictionary? That makes things a lot more readable.

@@ -13,6 +13,10 @@

from tests.common import get_test_home_assistant

patch('homeassistant.components.homekit.accessories.debounce',
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 bad as now loading this file impacts all other tests that use homekit.

Instead, you should use a pytest fixture. But to be able to leverage pytest fixtures, you will need to rewrite your tests to be just methods. Something like this: (written from top of my head, didn't test)

import pytest

@pytest.fixture(autouse=True)
def mock_debounce():
    with patch('…'):
        yield


@pytest.fixture
def mock_call_service(hass):
    events = []

    @callback
    def record_event(event):
        …

    hass.bus.listen(…)
    return events


def test_light_basic(hass, mock_call_service):
    # hass is a new test instance, will be automatically stopped for you
    # mock_call_service is equivalent to self.events
    # Instead of self.assertEqual, just use assert
    assert acc.aid == 2

from homeassistant.const import (
ATTR_SERVICE, EVENT_CALL_SERVICE, ATTR_SERVICE_DATA,
ATTR_UNIT_OF_MEASUREMENT, STATE_OFF, TEMP_CELSIUS, TEMP_FAHRENHEIT)

from tests.common import get_test_home_assistant

patch('homeassistant.components.homekit.accessories.debounce',
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed too.

@cdce8p
Copy link
Member Author

cdce8p commented Apr 6, 2018

@balloob Instead of using pytest fixtures, I opted for setUpClass and tearDownClass to limit where the patcher is active. That way I don't had to rewrite all test classes. It doesn't matter that that way debounce is patched for other test methods in those classes (like test_light_rgb_color) as well. I only want to make sure that during test_debounce it isn't patched.

@balloob
Copy link
Member

balloob commented Apr 6, 2018

The problem with class based tests is that we end up mocking more on each test than we need. Then tests will start failing randomly and we had more hacks on hacks. The function based one is what all new tests for Home Assistant are based on. They also allow async tests.

I am not against using unittest.TestCase, but it will just become harder and harder in the future to write tests, especially performant ones.

@balloob balloob dismissed pvizeli’s stale review April 6, 2018 14:45

Concerns addressed

@pvizeli
Copy link
Member

pvizeli commented Apr 6, 2018

I think you can change the test later in a PR. But with pytest, you can write faster, shorter and more readable tests very easy :)

@pvizeli pvizeli merged commit 262ea14 into home-assistant:dev Apr 6, 2018
@cdce8p cdce8p deleted the homekit-add_timeout branch April 6, 2018 22:05
@cdce8p
Copy link
Member Author

cdce8p commented Apr 6, 2018

I will lock into it :)

@balloob balloob mentioned this pull request Apr 27, 2018
Adminiuga pushed a commit to Adminiuga/home-assistant that referenced this pull request Jun 25, 2018
)

* Add async timeout feature

* Decorator for setter methods to limit service calls to HA
* Changed to async
* Use async_call_later
* Use lastargs, async_add_job

* Use dict for lastargs

* Updated tests to stop patch
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homekit - Cannot reliably set brightness level
5 participants