Skip to content

Connection retries are broken for ESP_ATcontrol.connect() and ESPAT_WiFiManager.connect() #61

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

Closed
bablokb opened this issue Aug 31, 2022 · 1 comment · Fixed by #63
Closed

Comments

@bablokb
Copy link
Contributor

bablokb commented Aug 31, 2022

ESP_ATcontrol.connect() tries to connect multiple times within a loop. It has a retries-variable which is meant to control the number of retries, but it is reset to 3 within the loop, so the loop is actually endless and the retries-variable is meaningless. I think this is a programming-error which can be solved easily.

The situation with ESPAT_WiFiManager.connect() is a bit different. It has a failure-count and once it is greater than the threshold attempts which is set in the constructor, the device is reset, the failure-count is cleared and the loop starts again. This would also result in an endless loop. But since this connect-method calls the ESP_ATcontrol.connect() internally, the reset and retry here is never executed anyhow. So fixing the endless loop in ESP_ATcontrol.connect() would then trigger the endless loop here.

I would suggest changing the logic: after every failure, do a reset but don't reset the failure-count. Once the failure count is larger than attempts, the method should give up and raise a RuntimeError.

The funny thing is that these two retry-loops are not the only ones involved. ESP_ATcontrol.connect() calls ESP_ATcontrol.join_AP() which itself passes a retries-parameter to the AT-firmware of the device. So ignoring the fact of the endless loops the current code has 18=2x3x3 retries, 15 seconds each.

A clean solution should probably get rid of all the loops in the upper-levels of the code and just pass the relevant parameters down to join_AP, i.e. do the looping in one place. Personally I would also get rid of the reset()-logic within ESPAT_WiFiManager.connect() in favor of failing faster.

Any objections or maybe better suggestions? If not I will be happy to provide a pull request.

@tammymakesthings
Copy link

I think it makes sense to centralize the retry logic in one place just as a general practice. The fact that the current implementation is broken in multiple ways just reinforces that. 😄 My vote would be to go ahead and submit a PR!

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 a pull request may close this issue.

2 participants