Skip to content

Conversation

smartin015
Copy link

@smartin015 smartin015 commented Jul 12, 2021

Thanks for contributing this excellent API - I use it all the time to indicate the status of various smart home stuff I have set up. This change sets a default timeout on all outbound requests, which prevents locking up when driving nanoleaf behaviors in a single-threaded way.

I use this in a python script which has a single thread awaiting MQTT messages, then sending a Nanoleaf command before waiting for more messages. When the nanoleaf is offline (even temporarily) the whole script hangs up indefinitely as the request is made without any timeout.

By wrapping all requests in this way, the API should be more resilient to transient network issues (assuming exceptions are handled appropriately).

Note that this also includes a small cleanup that allows running test_nanoleaf.py directly - tested using my own nanoleaf setup, running python3 test_nanoleaf.py, all was OK.

@MylesMor
Copy link
Owner

MylesMor commented Jul 13, 2021

Hi, thanks for your contribution! I just have one issue - I can't get the test file to pass as it currently is. There seems to be at least one function everytime I run it which doesn't throw the NanoleafConnectionError with the ShortTimeout.

For example:

======================================================================
FAIL: test_get_color_mode (__main__.TestNanoleafMethods)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".\test_nanoleaf.py", line 162, in test_get_color_mode
    self.nl.get_color_mode()
  File ".\test_nanoleaf.py", line 19, in __exit__
    raise exception_value
  File ".\test_nanoleaf.py", line 162, in test_get_color_mode
    self.nl.get_color_mode()
AssertionError: NanoleafConnectionError not raised

The function which does this also seems to change everytime and I'm not sure what's causing it as of yet. Any ideas?

EDIT: After further testing, it seems even with very low timeouts, the occasional request still makes it through which breaks the test. I'm still not sure why/how this happens yet, as I tried putting the timeout as low as 0.0000001 and still requests get through.

Setting the timeout to 0 fixes it, but I'm not sure there is any use to the test if it is changed to 0.

@MylesMor MylesMor added the enhancement New feature or request label Jul 13, 2021
@smartin015
Copy link
Author

Hey - sorry for the delay! Setting the timeout to zero should still test the behavior of timeout/exception handling. Can you try re-running CI with the latest commit?

@@ -205,15 +226,23 @@ def power_on(self) -> bool:
:returns: True if successful, otherwise False
"""
data = {"on" : {"value": True}}
response = requests.put(self.url + "/state", data=json.dumps(data))
try:
Copy link

@henryruhs henryruhs Oct 22, 2021

Choose a reason for hiding this comment

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

This should be moved to an method... this is insane how often you repeated this try catch block without thinking of a refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants