Reset wifi coprocessor on disconnect

[tyeth]

This PR along with RSSI (#589) precedes #584.

Here we add reset code for the Airlift boards + WIFININA boards (arduino), along with setting the mqtt client to disconnected if we fail the mqtt ping (the mqtt client connected check is very simplistic and still returns true when wifi lost) and then calling runNetFSM to re-establish network connectivity

[tyeth]

@brentru ready for initial thoughts / review (worth turning on ignore whitespace diff option), not happy with the reset process really, I'd like to just call disconnect and then connect/begin again, probably pushing the issue back to WifiNina firmware long term.
Also not sure what the situation with the Nano_33_IOT and mkrWifi1010 are, should I grab one of each to retest, and add the board definitions (or just silently ignore them for now)?

[brentru]

@tyeth I have a few changes to request. Overall, this looks good and I believe it'll improve stability.

WRT the arduino boards, let's talk about that on Slack

[brentru]

I know why it's there, but for maintainers, please add a comment explaining the utility of this "magic" delay() call

[tyeth]

Note to self, drop the WiFiNINA changes and Pico and stick to airlift. Maybe retest PicoW with and without extra delay.

…

On Fri, 31 May 2024, 19:17 Brent Rubell, ***@***.***> wrote: ***@***.**** requested changes on this pull request. @tyeth <https://github.com/tyeth> I have a few changes to request. Overall, this looks good and I believe it'll improve stability. WRT the arduino boards, let's talk about that on Slack ------------------------------ In src/Wippersnapper.cpp <#590 (comment)> : > @@ -2527,9 +2527,15 @@ void Wippersnapper::pingBroker() { // ping within keepalive-10% to keep connection open if (millis() > (_prv_ping + (WS_KEEPALIVE_INTERVAL_MS - (WS_KEEPALIVE_INTERVAL_MS * 0.10)))) { - WS_DEBUG_PRINTLN("PING!"); + WS_DEBUG_PRINT("PING!"); // TODO: Add back, is crashing currently Seems like this was crashing at some point, is it still? Should we remove this message? ------------------------------ In src/Wippersnapper.cpp <#590 (comment)> : > // TODO: Add back, is crashing currently - WS._mqtt->ping(); + if (WS._mqtt->ping()) { + WS_DEBUG_PRINTLN("SUCCESS!"); + } else { + WS_DEBUG_PRINTLN("FAILURE! Running network FSM..."); Try to be explicit in what type of failure ("Ping Failure!") so we can trace it ------------------------------ In src/network_interfaces/Wippersnapper_AIRLIFT.h <#590 (comment)> : > _wifi = &SPIWIFI; _ssid = 0; _pass = 0; _mqtt_client = new WiFiSSLClient; // setup ESP32 co-processor pins during init. WiFi.setPins(_ssPin, _ackPin, _rstPin, _gpio0Pin, _wifi); + delay(1000); I know why it's there, but for maintainers, please add a comment explaining the utility of this "magic" delay() call ------------------------------ In src/network_interfaces/Wippersnapper_AIRLIFT.h <#590 (comment)> : > + /********************************************************/ + /*! + @brief Compares two version strings. + @param currentVersion + Current version string. + @param requiredVersion + Required version string. + @returns True if the current version is greater than or + equal to the required version, False otherwise. + */ + /********************************************************/ + bool compareVersions(const String &currentVersion, + const String &requiredVersion) { + int curMajor, curMinor, curPatch; + int reqMajor, reqMinor, reqPatch; + int per_major, per_minor, per_patch; These variables are never referenced? Consider deleting them ------------------------------ In src/network_interfaces/Wippersnapper_AIRLIFT.h <#590 (comment)> : > } else { - - // validate co-processor is physically connected connection - if (WiFi.status() == WL_NO_MODULE) { - WS_DEBUG_PRINT("No ESP32 module detected!"); - return; + // disconnect from possible previous connection + _disconnect(); + delay(100); + WiFi.end(); + _wifi->end(); + delay(100); + _wifi->begin(); + feedWDT(); + WS_DEBUG_PRINT("Reset Pin: "); We don't need the debugging here anymore, we know the pin ------------------------------ In src/network_interfaces/Wippersnapper_AIRLIFT.h <#590 (comment)> : > + pinMode(_ssPin, OUTPUT); + digitalWrite(_ssPin, HIGH); // Do we need to set SS low again? + if (_gpio0Pin != -1) { + pinMode(_gpio0Pin, OUTPUT); + digitalWrite(_gpio0Pin, LOW); + } + pinMode(_rstPin, OUTPUT); + digitalWrite(_rstPin, LOW); + delay(50); + digitalWrite(_rstPin, HIGH); + delay(10); + if (_gpio0Pin != -1) { + pinMode(_gpio0Pin, INPUT); + } + // wait for the ESP32 to boot + delay(2000); Consider refactoring this out into a function called resetAirlift() ------------------------------ In src/network_interfaces/Wippersnapper_AIRLIFT.h <#590 (comment)> : > } + feedWDT(); + WS_DEBUG_PRINT("ESP32 booted, version: "); + WS_PRINTER.flush(); + WS_DEBUG_PRINTLN(WiFi.firmwareVersion()); + WS_PRINTER.flush(); + feedWDT(); + + // // validate co-processor is physically connected connection If we aren't utilizing these lines, delete them. Otherwise, comment why they may be needed in the future. ------------------------------ In src/network_interfaces/Wippersnapper_AIRLIFT.h <#590 (comment)> : > WiFi.begin(_ssid, _pass); _status = WS_NET_DISCONNECTED; + feedWDT(); Refactor this into a loop. ------------------------------ In src/network_interfaces/ws_networking_pico.h <#590 (comment)> : > @@ -282,6 +282,9 @@ class ws_networking_pico : public Wippersnapper { } } _status = WS_NET_DISCONNECTED; + WS.feedWDT(); + delay(5000); Why is this delay added to the pico iface? ------------------------------ In src/network_interfaces/Wippersnapper_WIFININA.h <#590 (comment)> : > +// no clear recommendation, all three defined for both boards, future-proofing +#if defined(NINA_RESETN) +#define NINA_RESET_PIN NINA_RESETN +#elif defined(SPIWIFI_RESET) +#define NINA_RESET_PIN SPIWIFI_RESET +#elif defined(ESP32_RESETN) +#define NINA_RESET_PIN ESP32_RESETN +#endif Could this macro be pulled to the top of this file, or placed inside the boards.h file instead? ------------------------------ In src/network_interfaces/Wippersnapper_WIFININA.h <#590 (comment)> : > + if (NINA_RESET_PIN != -1) { + WS_DEBUG_PRINTLN("Resetting ESP32..."); + WS_PRINTER.flush(); + pinMode(NINA_RESET_PIN, OUTPUT); + digitalWrite(NINA_RESET_PIN, LOW); + delay(50); + digitalWrite(NINA_RESET_PIN, HIGH); + delay(10); + } + // wait for the ESP32 to boot + delay(2000); + feedWDT(); +#endif Break this out into a resetESP32() function ------------------------------ In src/network_interfaces/Wippersnapper_WIFININA.h <#590 (comment)> : > + pinMode(NINA_RESET_PIN, OUTPUT); + digitalWrite(NINA_RESET_PIN, LOW); + delay(50); + digitalWrite(NINA_RESET_PIN, HIGH); + delay(10); + } + // wait for the ESP32 to boot + delay(2000); + feedWDT(); +#endif + + WS_DEBUG_PRINT("Connecting to "); + WS_DEBUG_PRINTLN(_ssid); + WiFi.begin(_ssid, _pass); + _status = WS_NET_DISCONNECTED; + feedWDT(); Same feedback as above regarding changing this to a loop — Reply to this email directly, view it on GitHub <#590 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTBZ43WOBTUWF3V7AF62T3ZFC5CHAVCNFSM6AAAAABITFPD7GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJRGQZTENRRGA> . You are receiving this because you were mentioned.Message ID: ***@***.*** .com>