WiFi connection should not be included in the library

[kcranky]

Though admittedly it's by no means authoritative, two of the Pragmatic Programming Principles is orthogonality and decoupling - basically to separate functionality where possible. While including connection to the WiFi library is useful because it can hide things away from a user perspective, the users are programmers and so would likely prefer to initialize WiFi outside of the module as it's better practice.

What changes?
The simplest way to implement this is to pull out the toggl.init() function, and in a regular project initialize WiFi as one usually would.

The biggest change would be to the example code provided in the library, but they could be refactored (maybe) as follows:

#include <Toggl.h>

// Ensure we import the correct Wifi library
# if defined (ESP8266)
#include <ESP8266WiFi.h>
# elif defined(ESP32)
#include "WiFi.h"
# else
#error "Please define ESP processor, ESP32 or ESP8266"
# endif

// Set WiFi Username and Password
const char* SSID = "";
const char* PASS = "";

// If you're using an ESP8266, uncomment this
//void wifiSetup(){
//
//  WiFi.begin(String(SSID), String(PASS));
//
//  while(WiFi.status() != WL_CONNECTED){
//    delay(100);
//  }
//}

//If you're using an ESP32, uncomment this 
//void wifiSetup(){
//  WiFi.begin(SSID, PASS);
//  while(WiFi.status() != WL_CONNECTED){
//    delay(100);
//  }
//}

//  Toggl Information
void setup(){
...
  wifiSetup();
...
}

Or

• just put the code that's in the wifiSetup() methods in the primary setup() method
• Just leave a comment instructing the user to add the WifI code (though this is more elegant it's maybe less "polite" to beginners)

Other Notes
Ideally the library would handle connection in an agnostic way (i.e not do a if ((WiFi.status() == WL_CONNECTED)) check but rather just check if there is any sort of connection. This could be done by pulling the WiFi check into a separate method, perhaps hasConnection(), which changes depending on what connection method the user decides to implement. This is another issue though.

[JoeyStrandnes]

A goal with this was to keep things simple and hidden from the end user. I was a bit torn regarding the "hidden Wi-Fi" aspect of the code and I'm admittedly not a big fan of it.

I kept it in the code since Arduino is mostly focused towards beginners and I wanted it to be as simple as possible for people to use it.

A compromise would be to move the code out of the init function and into a wifiSetup function like you mentioned. Making it an optional function call and more clear to the end user.

I meant to include something along the lines of hasConnection() but forgot/lack of time.

Il implement this but I have finals next week and the week after so it would be about two weeks before I could start writing the code in any meaningful capacity. Just as a heads-up!

Side note

I was also looking into into replacing the default synchronous web-socket with a asynchronous alternative like me-no-dev's ESPAsyncWebServer but never had the time to fully look into it. It would require a complete rewrite of the existing code but might allow for way better performance since the code wouldn't be halted for several seconds while waiting for the toggle servers reply.

This might be overkill for a simple Arduino library/use-case.

Thanks for your engagement in the project!

//Joey

[kcranky]

I definitely agree with you about the "hidden Wi-Fi", ease of use and the Arduino principles/attitude. What ended up swaying me over to pulling out the connection was that there are likely use cases where a different connection method is used, but using this library would still be advantageous, especially given that it is listed as the official Arduino Toggl library.

So I think this could be split up into three issues/pull requests/enhancements

1. Pull out the WiFi connectivity from the library (this issue)
2. Implement a hasConnection() method which can over time know connection method is being used. Perhaps through an init method and the user setting a connection type? I.e toggl.connectionType = WiFI? Maybe connectionType can be set to one value of an included enum, which can be added to as additional connection methods are added? (I don't have anything else to add or test, but it'll make it easy if someone else wants to add a connection method).
3. Move to async calls. I think this is a great idea! Didn't even cross my mind but it is the generally approach when consuming an API. It'd be very helpful to a multi-threaded implementation.

In my opinion:

1. is pretty easy
2. is probably the most important
3. is likely better suited to be pushed for a "v2" style implementation and I don't think is all too urgent.

I don't know what you think about that decomposition, but don't worry too much about it for now - definitely focus on finals! Best of luck for them! :D

[JoeyStrandnes]

The thought that somebody would want to use a different connection method never even crossed my mind. I definitely think that it should be added!
I want this library/project to be as capable as possible while stilling being beginner friendly. :)

1:

This is pretty easy to implement and wont take much time. Its not critical so ill draft a new release once I've implemented hasConnection().

2:

This is more critical, I would however like to think of a robust/ scalable solution to prevent future (unnecessary) rewrites of the code. Not saying that your idea is bad but would like to explore a few ideas before I commit to one!

3:

Agree with you about this. The code works fine at the moment and a move to asynchronous calls would require a major rewrite. I would have added asynchronous functionality from the start if it wasn't an "Arduino principles" oriented project.
Maybe its to overkill, maybe I'm underestimating regular Arduino users? I don't know :P

But it would allow for better end users performance so I'm strongly considering it!

These enhancements could be added as separate issues by you or me. It would probably be better to create new issues specific to the subject and continue further detailed discussion in those in order to prevent future confusion and have a easier "paper trail"!

PS:
I try to work in the Experimental branch to add features and only push them to master once they are "release ready". I think it makes thing safer since the Arduino library bot constantly checks for library changes (in library.properties and release). Last thing I would want is to accidently "push" a broken copy to the Arduino library manager!