Skip to content

A bit more stable observations#208

Closed
alex3305 wants to merge 3 commits intohome-assistant-libs:masterfrom
alex3305:more-stable-observations
Closed

A bit more stable observations#208
alex3305 wants to merge 3 commits intohome-assistant-libs:masterfrom
alex3305:more-stable-observations

Conversation

@alex3305
Copy link
Copy Markdown
Contributor

This change fixes a very small bug (I suppose) in the aiocoap_api class
where the flag whether to parse the output as json was not there in the
observe method. Hopefully this makes this part of the API a bit more stable.

Also I've added an observation callback as a global variable in the
ApiResource class. When tracking this value for an object, it is possible
to invoke the callback manually. This way the observation can also be
invoked when the user makes a manual change. Thus don't having the need to
call out the observation.

Thirdly I've added an explicit error callback to the ApiResource observation
which can be used to diagnose issues easily with the use of the print function.

Finally I've also added a check to check whether or not the incoming result has
even changed from the already existing result. Thus eliminating the need to
continiously update the value. This can hopefully make some parts of the
library a little more stable, because it will limit callback calls.

In conclusion, these are very minor changes, but hopefully will make the
library and in turn the applications using this library a little bit more
stable.

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 18, 2018

Coverage Status

Coverage decreased (-1.5%) to 77.485% when pulling 7af3159 on alex3305:more-stable-observations into 506b9e8 on ggravlingen:master.

This change fixes a very small bug (I suppose) in the `aiocoap_api` class
where the flag whether to parse the output as json was not there in the
observe method. Hopefully this makes this part of the API a bit more stable.

Also I've added an observation callback as a global variable in the
`ApiResource` class. When tracking this value for an object, it is possible
to invoke the callback manually. This way the observation can also be
invoked when the user makes a manual change. Thus don't having the need to
call out the observation.

Thirdly I've added an explicit error callback to the `ApiResource` observation
which can be used to diagnose issues easily with the use of the print function.

Finally I've also added a check to check whether or not the incoming result has
even changed from the already existing result. Thus eliminating the need to
continiously update the value. This can hopefully make some parts of the
library a little more stable, because it will limit callback calls.

In conclusion, these are very minor changes, but hopefully will make the
library and in turn the applications using this library a little bit more
stable.
@alex3305 alex3305 force-pushed the more-stable-observations branch from 54739f6 to d972d9e Compare November 18, 2018 14:25
Copy link
Copy Markdown
Collaborator

@lwis lwis left a comment

Choose a reason for hiding this comment

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

Maybe we should add a way to cancel the observation?

Comment thread pytradfri/resource.py Outdated
Comment thread pytradfri/resource.py
def __init__(self, raw):
"""Initialize base object."""
self.raw = raw
self.observe_callback = None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's not really clear what this is doing, a global observe_callback for all observations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is only for a specific device/task/group. Not a global for all the objects. At least that's the way I've understand the code so far. Am I wrong about that?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My bad, misread where this was. Wouldn't it make more sense to call observe_callback with the result?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, which is done in proces_result function down below. However I guessed it was a good practice to initialise a variable with a default state in the class constructor. That's why it's set to None here.

Alex van den Hoogen added 2 commits November 18, 2018 15:47
As suggested by @lwis to remove the logging from the `err_callback`
function in the ApiResource class.
This adds a (non-breaking) cancel command to the API. This makes it possible to
cancel individual observations easily by sending a `observe_cancel` command to
the `aiocoap` API.

I have included a rudimentary example in the
[example_async.py](examples/example_async.py) file.

This also fixes a very small 'bug' which was triggered when the API was
shutdown. The logging about incorrectly ending the loop is now gone.
@alex3305
Copy link
Copy Markdown
Contributor Author

@lwis I've added a cancellation event for observations as per your suggestion. This would make it easier for integration into something like Home Assistant.

Today I've encountered the error I described in that home-assistant/core#18497 again. The only clue I've got so far is that the API is only returning decrypt_verify(): found 8 bytes cleartext and nothing more. I can also still control my lights, but the state is not being reflected in the interface anymore. I am still looking to trace down the root cause of this bug. And I am currently unsure whether it is a Home Assistant or pytradfri issue.

But this change should already (and probably) make somewhat of a difference.

@alex3305
Copy link
Copy Markdown
Contributor Author

Closing in favor of #214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants