magico13 / PyEmVue

Python Library for the Emporia Vue Energy Monitor
MIT License
185 stars 36 forks source link

Bug39 fix pull #63

Closed DabblerIOT closed 4 months ago

DabblerIOT commented 4 months ago

Caught different ways api was failing and coded retries. Retries should be here instead of in ha code for others using pyemvue. One possible issue with changes are we need a delay here (to try to give enough delay if api bounces their web server or backend server, to give it time to restart). Saw once or twice I think, needs about 20-30 seconds before server will start returning valid responses again. The more devices on account (fetched with one request), the more chance of having an issue. Problem with code being in a non async function we can't define an asyncio.sleep.. concerned that the delay time may block all for delay time (normally just 2 seconds) if 1 retry.

badapiresponseseconddevicenull.txt badapiresponse.txt goodapiresponse.txt

Running before changes many (5-10) 0s and unknowns a day. Running now, clean data (2 days run).

Sorry for so many commits, sorting out python again, and how git works with ha and syntax for importing forked versions. (Docker containers have my head spinning still). :). I seemed to have to increment version number to have ha redownload it on restart. (Still figuring things out.)

http error handling was moved around so we retry on http errors instead of kicking error back to caller. If last response is an http error we will return an error still (just like before). Let's see if I did this request correctly. :).

Cheers.

Changes are just in pyemvue around line 100. Please review.

magico13 commented 4 months ago

I am inclined to put most of this functionality into the common code in auth.py that handles making requests, so that no matter the endpoint it will automatically retry on failures. Handling partial failures will still need to be implemented into get_device_list_usage like you have here. Let me take a swing at an attempt and let me know if it works for you.

DabblerIOT commented 4 months ago

Sounds Good. Is synchronous sleep ok? (I don’t think it is, but don’t know how to fix without a breaking change to published pyemvue functions which others may be consuming).

The error check for valid data for this particular response is the most important piece, where the api returns a device with 1 channel, with usage: none. So that test may need to be where it is. Say the bad response (mostly 1st device, but a couple times a day a subsequent device..)… I think error 500s and the like are not as common. Ex: my other test code has never seen one except that one day, I think they were trying to fix their api.

Retries on http errors could go into the main request function.

For whatever reason (maybe number of calls). I have never seen another of their api functions fail like this, except this one.

Code can probably be combined so it is fewer lines… left it this way to be clear to reviewers.

Thanks.

On Feb 24, 2024, at 2:59 PM, Mike Marvin @.***> wrote:

I am inclined to put most of this functionality into the common code in auth.py that handles making requests, so that no matter the endpoint it will automatically retry on failures. Handling partial failures will still need to be implemented into get_device_list_usage like you have here. Let me take a swing at an attempt and let me know if it works for you.

— Reply to this email directly, view it on GitHub https://github.com/magico13/PyEmVue/pull/63#issuecomment-1962671081, or unsubscribe https://github.com/notifications/unsubscribe-auth/BGBTIUY64C6FRO6XCARD6U3YVJBB5AVCNFSM6AAAAABDV5M2L2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSGY3TCMBYGE. You are receiving this because you authored the thread.

DabblerIOT commented 4 months ago

Thanks for the work. Running merged code now (0.18.1). Looked good. I have had clean data for 4 days running pull request code (just fyi).