-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Trying to fix Errors in _async_subscribe_for_data #347 #374
Conversation
@iMicknl, this would be more important to review, as this one causes the integration to hang for 5 minutes and then re-tries the status comes through and fails on the matching, as the input is not what it expects. Mainly was due to the [ ] wrapping (making it a single element list). But after removing the [ ], then the formatting fails for the nestpy client, so it has to wrapped with the bucket to have the right format. As I noted, I don't really get the logic of the whole subscription, but it does go through and not fail on incorrect formatting. Someone should test it with wired nest projects which are sending multiple updates on motion. Otherwise, if you would provide some details how the previous fix was putting a lot of load on the Google servers, then I might could understand where could be the issue for that. |
This would fix the issue with objects and then wraps it with Bucket as expected by the pynest client, but if it is still generating high traffic on Google servers, then there is a flaw somewhere in the main logic of subscription process, and it re-subscribes again and again with the pynest client, which I cannot really test, as I have only battery powered nest protects, so there isn't live motion detection.
Also needed to fix the aforementioned issue as wrapping with Bucket fails on the WhereBucketValue otherwise.
@GSzabados I have to admit it is a long time since I wrote this integration. When a new event is pushed by Nest server, we have to register again and in this register step we have to send what we subscribe to and which values we received last. If we don't do this in the right way, it is increasing the load on the Nest protect server massively. I introduced an error error when I started to add (better) typing to this integration, so most likely the culprit is there. In the original implementation (in the main branch) no issues have been reported. For me it is hard to debug if we are doing extra requests to Nest Protects server, thus that is something where I would need to add some additional logging. |
If I am not mistaken, the code is adapted from https://github.com/chrisjshull/homebridge-nest/blob/master/lib/nest-connection.js#L1828. |
That is a tough one, but I will have a look. I literally matched as the code was suggesting, and after the matching wrapped with the bucket(), so the format should be the same, otherwise, the client was failing on it. How can you verify the additional load? What do you mean "the right way"? |
Do you have Discord by any chance? If so, can you add me there? ( |
Just messaged you. |
I have had a look at the Buckets coming and going. They are the "same", but once the integration is (re)started the value={} in each bucket is sorted, and the order does not match with what the API sends, but the content looks the same. Would that matter? The other question, when re-subscribing, should it send back all the Buckets, or just the updated one? As I see from my code, it sends back (re-subscribes) to everything, not just the updated one. |
As we discussed on Discord, the issue with the v0.4.6b fix was that it has not matched and merged the updated buckets, so it wa continuously sending back and old bucket for subscription and the API was replying with the current one, doing this in an endless loop. This fix does the match and merge correctly and subscribes with the right updated bucket. I am running it at least for 3 weeks now and have not seen any error messages from the integration. |
Running this code now as well. So far occupancy detection is quick again and I'm not seeing any errors in the log. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! If you have additional time, would be good to figure out the root cause here. As discussed this is a good patch, but more a workaround for the errors, doesn't touch the root cause.
I think the root cause is the formatting. Objects vs Updated_Bucket. I think it should be the same format passed between the init.py and the client.py. This is not correct as the It should be kept the same way either always as The issue with the WhereBucketValue is likely that it is formatted correctly, and the we do nothing with it, so it doesn't need re-casting as it is already WhereBucketValue object. Hopefully I used correctly the terminology. |
I have managed to squash all the errors popping up from that _async_subscribe_for_data, but I am not sure that the main logic is correct in the code. If after this fix, it still generates high traffic to the Google servers, then the whole logic of subscribing is flawed somewhere.
To be honest, I don't understand what is meant to be achieved with this part of the code. It might be re-subscribing after each message received from Google, and that causes a lot of traffic. I cannot test the traffic issue, because I have only battery powered Nest protects.
It should fix #338 as well, as they are duplicates.
Fixes #338