search

adafruit / Adafruit_CircuitPython_ESP_ATcontrol

Use the ESP AT command sent to communicate with the Interwebs
MIT License
20 stars 17 forks source link

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

Closed bablokb closed 2 years ago

bablokb commented 2 years ago

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 commented 2 years ago

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!