Skip to content
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

Disconnect and ssid check #104

Merged
merged 11 commits into from Jan 3, 2020
Merged

Disconnect and ssid check #104

merged 11 commits into from Jan 3, 2020

Conversation

sellensr
Copy link
Contributor

Scope: Created a _disconnect() function in all the wifi hardware variants that calls the appropriate wifi.disconnect() and waits long enough (300 ms) for it to take effect. Created an AIO.disconnect() that calls the hardware _disconnect().

#101

Check that the ssid string is not zero length before trying to connect wifi. Call _disconnect() before trying to connect in order to break any existing connection. This allows sketches to self manage wifi by setting the AIO ssid to "".

Limitations: Only tested on WINC1500, so relying on Travis to check other builds. Will make connect() calls take about 300 ms longer. May change failure mode of code where SSID is set to "".

@@ -158,6 +158,9 @@ class AdafruitIO_AIRLIFT : public AdafruitIO {
/**************************************************************************/
void _connect()
{
if(strlen(_ssid) != 0)
{
_disconnect();
// setup ESP32 pins
if (_ssPin != -1) {
WiFi.setPins(_ssPin, _ackPin, _rstPin, _gpio0Pin, _wifi);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is WiFi.setPins occurring here? Do you mean to add a } after the call to _disconnect()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The } is down at the end of the function. The wiFi.setPins, etc. is part of the original code, left unindented so the diff shows no changes. I'll fix the indent now so it will be clearer to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that code for the airlift may be in error with the original -- trying a compile of example 00 with the current release code and #define USE_AIRLIFT uncommented produces compile errors under both ESP32 and M0. I'm not familiar with the airlift devices and was just mimicking what worked with the M0 WINC1500 setup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What compile errors are you seeing? Travis seems to be passing with commit a1ac2a2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


In file included from /Users/sellensr/Documents/Arduino/libraries/Adafruit_IO_Arduino/src/AdafruitIO_WiFi.h:22:0,
                 from sketch/config.h:32,
                 from /var/folders/3w/fph9nxcx59bgndf_ydyvphlr0000gn/T/arduino_modified_sketch_795096/adafruitio_00_publish.ino:18:
/Users/sellensr/Documents/Arduino/libraries/Adafruit_IO_Arduino/src/wifi/AdafruitIO_AIRLIFT.h: In member function 'virtual void AdafruitIO_AIRLIFT::_connect()':
/Users/sellensr/Documents/Arduino/libraries/Adafruit_IO_Arduino/src/wifi/AdafruitIO_AIRLIFT.h:166:18: error: 'class WiFiClass' has no member named 'setPins'
             WiFi.setPins(_ssPin, _ackPin, _rstPin, _gpio0Pin, _wifi);
                  ^~~~~~~
Multiple libraries were found for "SPI.h"
 Used: /Users/sellensr/Library/Arduino15/packages/adafruit/hardware/samd/1.5.7/libraries/SPI
Multiple libraries were found for "Adafruit_ZeroDMA.h"
 Used: /Users/sellensr/Library/Arduino15/packages/adafruit/hardware/samd/1.5.7/libraries/Adafruit_ZeroDMA
Multiple libraries were found for "AdafruitIO_WiFi.h"
 Used: /Users/sellensr/Documents/Arduino/libraries/Adafruit_IO_Arduino
Multiple libraries were found for "Adafruit_MQTT.h"
 Used: /Users/sellensr/Documents/Arduino/libraries/Adafruit_MQTT_Library
Multiple libraries were found for "ArduinoHttpClient.h"
 Used: /Users/sellensr/Documents/Arduino/libraries/ArduinoHttpClient
Multiple libraries were found for "WiFiNINA.h"
 Used: /Users/sellensr/Documents/Arduino/libraries/WiFiNINA
exit status 1
Error compiling for board Adafruit Feather M0.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to go back on what I said on #101. I'd like to see a negotiated disconnect where the MQTTClient sends the disconnect packet to the Adafruit IO MQTT Broker (see: https://github.com/adafruit/Adafruit_MQTT_Library/blob/master/Adafruit_MQTT.cpp#L288) prior to tearing down the WiFi interface.

We should call this PR wifi_disconnect within AdafruitIO.cpp/.h since this pull request is not performing a disconnect with the Adafruit IO MQTT broker, it's disconnecting the hardware's WiFi interface.

Will do the same with others
makes it clear the function is only disconnecting wifi
@@ -151,7 +151,7 @@ class AdafruitIO_WINC1500 : public AdafruitIO {
@return none
*/
/**************************************************************************/
void _disconnect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it as _disconnect() within the wifi classes so it matches the wifi interface's connect method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, misunderstood your request.

would it be better to simply add _mqtt->disconnect(); to the disconnect function?

void AdafruitIO::disconnect()
{

AIO_DEBUG_PRINTLN("AdafruitIO::disconnect()");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to AIO_DEBUG_PRINTLN("AdafruitIO::wifi_disconnect()");

@brentru
Copy link
Member

brentru commented Jan 2, 2020

@sellensr Ok, everything looks good. I will test on M0/AirLift and ESP32 hardware tomorrow AM.

No changes except increased indentation in _connect() for clarity of reading
@brentru
Copy link
Member

brentru commented Jan 3, 2020

@sellensr I tested this on a PyPortal, ESP32 and ESP8266. All were able to connect to Adafruit IO.

However, a blank SSID/Password will cause the Serial.print(".") in our example sketches to print ...., even if it's disconnected.

I'm not sure if we want to re-use AIO_NET_DISCONNECTED for the SSID check.

Let's make a new enum here for an invalid ssid: https://github.com/adafruit/Adafruit_IO_Arduino/blob/master/src/AdafruitIO_Definitions.h#L107 and call it AIO_INVALID_SSID.

I feel the user code would handle this after the io.connect() call.

dev_cu_usbmodem144212101_and_adafruitio_02_pubsub-config_h___Arduino_1_8_9_and_1__git__Users_brentrubell_Documents_Arduino_libraries_Adafruit_IO_Arduino__less__and_Disconnect_and_ssid_check_by_sellensr·Pull_Request__104·_adafruit_Adaf

@sellensr
Copy link
Contributor Author

sellensr commented Jan 3, 2020

I'd pick AIO_SSID_INVALID to match AIO_FINGERPRINT_INVALID and AIO_AUTH_FAILED for word order. The user code could then detect and do something after io.connect() and some wait time.

Even if I set that status, the examples will still .... because they have no code to detect why a full connection isn't completed. I don't think they need the added complexity, and this is the behaviour they have always had for anything other than fully correct ssid pass, user, and key, isn't it?

@brentru
Copy link
Member

brentru commented Jan 3, 2020

I'd pick AIO_SSID_INVALID to match AIO_FINGERPRINT_INVALID and AIO_AUTH_FAILED for word order. The user code could then detect and do something after io.connect() and some wait time.
AIO_SSID_INVALID is OK by me!

and this is the behaviour they have always had for anything other than fully correct ssid pass, user, and key, isn't it?

Yep. I agree, let's not modify the examples for now. In the future, I'd like to have them be more verbose to indicate what ... means.

Once this passes, please bump the minor version number within https://github.com/adafruit/Adafruit_IO_Arduino/blob/master/library.properties#L2 to 3.4.0 to prepare for a release :)

Thanks for adding this!

@sellensr
Copy link
Contributor Author

sellensr commented Jan 3, 2020

OK, I'll watch for Travis to succeed then bump up the version number. I'm having fun and developing some github skills I didn't have. Thanks for helping me along, as this will make my own projects more reliable, which we need when monitoring greenhouses through the cold.

@brentru
Copy link
Member

brentru commented Jan 3, 2020

No problem, thanks for contributing to Adafruit IO Arduino!

I should convert this repository to GitHub Actions instead of travis, it currently takes ~20min per build.

@brentru brentru merged commit 512dc2e into adafruit:master Jan 3, 2020
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 this pull request may close these issues.

None yet

2 participants