Adding timeout and API and refresh token error to fix #39

[tmenguy]

fixes issue #39

[tmenguy]

Hope it is ok this time :)

[tmenguy]

a little up

[cliviu74]

a little up

Apologies, I am a bit busy lately. I will try to give it some attention mid next week.

This is a large change that requires some testing, at first glance, the timeout implementation doesn't work (api calls keep hanging), which I suspect is the original reason for this fix.

I'm using tcgui to simulate different latency scenarios and tcpdump for packet inspection. If anyone is familiar with the tool and have some time on their hands to test and publish results here, help is greatly appreciated. https://github.com/tum-lkn/tcgui

[tmenguy]

Absolutely no issue ... we do all of that for fun, it as to stay fun :) Just wanted to send a gentle nudge in case you missed it.

The change is not so big when you look at it: I just factorized the http requests api in a common function, with retries and timeout for everyone. The "requests" doc explicitly specifies that timeout shouldn't be None, and it was the case by default, especially when used with homeassitant. For what it worth I had zero issues with my Wallbox connection since I introduced this fix in my setup :) (and well before it was hanging from time to time sometimes once a day sometimes every few hours, spent a lot of time with traces in my HA setup to get down to understand that it was the request we were doing to Wallbox that was never returning, hence hanging the integration)

[tmenguy]

fixes #39

[tmenguy]

Hi,

sorry to bother, but as I would like to add some other API calls (set back ecosmart mode, merge with the schedule support PR), it would be easier to do and a bit more robust if my current PR could be merged first, else not a big issue I can fork on my side, and as I have started, do a custom Homeassistant Wallbox component with those added features, it is just that I may diverge too much , and then the chance to get this mainstream will be a lot lower. Thanks for your help!

[cliviu74]

I've done some testing and I can't see a use for adding the timeout in the way you described. The requests timeout is already implemented and can be set-up when instantiating the Wallbox object

i.e.:

w = Wallbox("user@email", "password", requestGetTimeout = 10)

I don't see a reason for creating a wrapper to the request as it adds unnecessary complexity.

Would you be able to set the timeout as above and see if that fixes your issues?

[tmenguy]

Hi, great thx for the review and tests!

-First : you are correct on the Home Assistant part, but it would require an HA PR :) .... but I do think anyway that the default value for the timeout shouldn't be None (as stated in "request" doc), so infinite, maybe the default requestGetTimeout should be XXXs whatever by default. As for the wrapper, it adds retries, and same handling for everyone without cut and paste for all new functions, so to me (and yes it is highly subjective) could help code maintenance, common error handling: as an example adding the retry mechanism would add a lot of duplicate code to all API calls with no wrapper. (side note at one point this lib may have an async version too, could help to have a wrapper too, but I digress :) )

-There is a second fix in the PR too : on the token/auth side to force a full re-auth in case of issue ( I got it once in my HA instance and seen it through traces, hard to reproduce, and long to wait refresh token expiration or issues)

Hope this helps tell me where you are on this, and no pb it is your repo :)

BR Thomas

[tmenguy]

Hi!

Could you just tell me if there is a chance you want to get this PR in? If no interest no worries.

BR Thomas

[cliviu74]

Hey @tmenguy, did you have a chance to test if setting the timeout as suggested above fixes your issues? On the retry, my opinion is they need to be implemented at the consumer application logic, as this library is intended as a very thin python API interface with wallbox. It's not meant to store state, as it can create unexpected behaviour with the app implementations. Home Assistant would compensate the retries by pooling and reauthenticating based on the http response.

I have also considered creating a wrapper get/post/put method instead of using requests, but I have noticed the wallbox api requires different headers for each call so decided against it to improve code clarity. A wrapper may obscure some of the functionality and may lead to major release for minor changes. I am considering this down the line when we have more features packed and a better understanding on how the wallbox api handles requests.

I agree with the async requests, it's on my list, just didn't get to it.

Long story short, if the timeout issue is fixed with setting the timeout when instantiating the object, I don't see necessary to change the requests method from the current implementation.

[tmenguy]

Hey, thanks for the clarity here, and for the retry I was myself fifty fifty on where to put it (in or out of the lib)

For the timeout I still think the library should have a default that is not infinite as the Wallbox server seems to have this kind of issues. It is long to reproduce (can happen after few hours or few days… and I restart my HA more often :) … so it defeats the test) but yes seeing my traces when it happens, a timeout on the HA side would fix it. but for symmetry I think the timeout should be used for all requests I think, not only get.

There was another bug fix around the token regeneration in my PR.

What l can do: I remove the wrapper, (and so the retry code), set a default timeout and add the timeout to all requests + keep the token fix

Would you be ok with that?

(changed the PR for that)

[tmenguy]

Hi,

Did the change so you can see if it is making sense. I kept the name of the getTimeout, not found if that if used everywhere but wanted to not break any compatibility here

[tmenguy]

opened #43 as I've seen some changes in the iOS Wallbox app regarding to API usage we may want to update to the latest ... so If you could accept (or not) the latest changes I have proposed here, would be awesome, so I can open a second one with those updates, thx!

[cliviu74]

Closing as not fixed based on the comments above